| Message ID | d9aa4052-f3d1-6595-65d5-0b0bfc489047@omprussia.ru |
|---|---|
| State | New |
| Headers | show |
| Series | Explicitly deny IRQ0 in the libata drivers | expand |
Hi Sergey, On Sun, Mar 21, 2021 at 3:59 PM Sergey Shtylyov <s.shtylyov@omprussia.ru> wrote: > > If platform_get_irq() returns IRQ0 (considered invalid according to Linus) How can platform_get_irq() return 0 on i.MX? This driver only runs on imx31 and imx51 and in both platforms the PATA IRQ is non-zero. IMHO the current code is correct as-is.
Hello! On 3/21/21 10:06 PM, Fabio Estevam wrote: [...] Sorry for replying this late. If I don't reply to mails right away, I tend to forget about the unreplied mails. I might have been busy with other stuff ATM too... :-) >> If platform_get_irq() returns IRQ0 (considered invalid according to Linus) > > How can platform_get_irq() return 0 on i.MX? > This driver only runs on imx31 and imx51 and in both platforms the > PATA IRQ is non-zero. Maybe this is impossible indeed -- iff someone doesn't change the kernel (or DT) on purpose (the driver wouldn't work correctly in this case as libata would ignore the driver's IRQ handler). > IMHO the current code is correct as-is. Not quite... I don't want to leave a bad example for the future driver authors. What should I change in the patch description for the patch to become acceptable for you? MBR, Sergey
Hi Sergey, On Sat, Sep 18, 2021 at 5:49 PM Sergey Shtylyov <s.shtylyov@omp.ru> wrote: > > IMHO the current code is correct as-is. > > Not quite... I don't want to leave a bad example for the future driver authors. What should > I change in the patch description for the patch to become acceptable for you? Please see how the PCI subsystem has converted the handling of platform_get_irq(): https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?h=v5.15-rc2&id=0584bff09629666eea97c7ac428e55b00df211f5 Why does drivers/ata/ need to handle platform_get_irq() differently? I still don't see the need for changing drivers/ata/pata_imx.c in this aspect.
On Mon, Sep 20, 2021 at 9:45 AM Fabio Estevam <festevam@gmail.com> wrote: > > Hi Sergey, > > On Sat, Sep 18, 2021 at 5:49 PM Sergey Shtylyov <s.shtylyov@omp.ru> wrote: > > > > IMHO the current code is correct as-is. > > > > Not quite... I don't want to leave a bad example for the future driver authors. What should > > I change in the patch description for the patch to become acceptable for you? > > Please see how the PCI subsystem has converted the handling of > platform_get_irq(): > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?h=v5.15-rc2&id=0584bff09629666eea97c7ac428e55b00df211f5 > > Why does drivers/ata/ need to handle platform_get_irq() differently? > > I still don't see the need for changing drivers/ata/pata_imx.c in this aspect. Also, please check this commit too: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?h=v5.15-rc2&id=a85a6c86c25be2d2a5f9c31491f612ce0edc7869
On 9/20/21 3:45 PM, Fabio Estevam wrote: >>> IMHO the current code is correct as-is. >> >> Not quite... I don't want to leave a bad example for the future driver authors. What should >> I change in the patch description for the patch to become acceptable for you? > > Please see how the PCI subsystem has converted the handling of > platform_get_irq(): > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?h=v5.15-rc2&id=0584bff09629666eea97c7ac428e55b00df211f5 Thanks for the link -- that's what I've been doing for the drivers outside PCI in the past few months. :-) > Why does drivers/ata/ need to handle platform_get_irq() differently? Because ata_host_activate() treats irq0 as polling indicater and complains about the 'handler' being non-NULL. > I still don't see the need for changing drivers/ata/pata_imx.c in this aspect. And now? MBR, Sergei
On 9/20/21 3:52 PM, Fabio Estevam wrote: [...] >>>> IMHO the current code is correct as-is. >>> >>> Not quite... I don't want to leave a bad example for the future driver authors. What should >>> I change in the patch description for the patch to become acceptable for you? >> >> Please see how the PCI subsystem has converted the handling of >> platform_get_irq(): >> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?h=v5.15-rc2&id=0584bff09629666eea97c7ac428e55b00df211f5 >> >> Why does drivers/ata/ need to handle platform_get_irq() differently? >> >> I still don't see the need for changing drivers/ata/pata_imx.c in this aspect. > > Also, please check this commit too: > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?h=v5.15-rc2&id=a85a6c86c25be2d2a5f9c31491f612ce0edc7869 You think I haven't seen this? :-) WARN() is not enough to make IRQ invalid, isn't it? MBR, Sergey
Index: linux-block/drivers/ata/pata_imx.c =================================================================== --- linux-block.orig/drivers/ata/pata_imx.c +++ linux-block/drivers/ata/pata_imx.c @@ -135,6 +135,8 @@ static int pata_imx_probe(struct platfor irq = platform_get_irq(pdev, 0); if (irq < 0) return irq; + if (!irq) + return -EINVAL; priv = devm_kzalloc(&pdev->dev, sizeof(struct pata_imx_priv), GFP_KERNEL);
If platform_get_irq() returns IRQ0 (considered invalid according to Linus) the driver blithely passes it to ata_host_activate() that treats IRQ0 as a sign that libata should use polling and thus complains about non-NULL IRQ handler passed to it. Deny IRQ0 right away, returning -EINVAL from the probe() method... Fixes: e39c75cf3e04 ("ata: Add iMX pata support") Signed-off-by: Sergey Shtylyov <s.shtylyov@omprussia.ru> --- drivers/ata/pata_imx.c | 2 ++ 1 file changed, 2 insertions(+)