Here are 4 patches against the 'master' branch of the Jens Axboe's 'linux-blobk.git' repo plus the 'pataep_93xx' driver patch re-posted yesterday. The affected drivers use platform_get_irq() which may return IRQ0 (considered invalid, according to Linus) that means polling when passed to ata_host_activate() called at the end of the probe methods. I think that the solution to this issue is to explicitly deny IRQ0 returned by platform_get_irq()... [1/4] pata_bk3710: deny IRQ0 [2/4] pata_ep93xx: deny IRQ0 [3/4] pata_ftide010: deny IRQ0 [4/4] pata_imx: deny IRQ0
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: 76a40ca82f34 ("Add Palmchip BK3710 PATA controller driver.") Signed-off-by: Sergey Shtylyov <s.shtylyov@omprussia.ru> --- drivers/ata/pata_bk3710.c | 2 ++ 1 file changed, 2 insertions(+) Index: linux-block/drivers/ata/pata_bk3710.c =================================================================== --- linux-block.orig/drivers/ata/pata_bk3710.c +++ linux-block/drivers/ata/pata_bk3710.c @@ -317,6 +317,8 @@ static int __init pata_bk3710_probe(stru pr_err(DRV_NAME ": failed to get IRQ resource\n"); return irq; } + if (!irq) + return -EINVAL; base = devm_ioremap_resource(&pdev->dev, mem); if (IS_ERR(base))
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: 2fff27512600 ("PATA host controller driver for ep93xx") Signed-off-by: Sergey Shtylyov <s.shtylyov@omprussia.ru> --- drivers/ata/pata_ep93xx.c | 4 ++++ 1 file changed, 4 insertions(+) Index: linux-block/drivers/ata/pata_ep93xx.c =================================================================== --- linux-block.orig/drivers/ata/pata_ep93xx.c +++ linux-block/drivers/ata/pata_ep93xx.c @@ -931,6 +931,10 @@ static int ep93xx_pata_probe(struct plat err = irq; goto err_rel_gpio; } + if (!irq) { + err = -EINVAL; + goto err_rel_gpio; + } mem_res = platform_get_resource(pdev, IORESOURCE_MEM, 0); ide_base = devm_ioremap_resource(&pdev->dev, mem_res);
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: be4e456ed3a5 ("ata: Add driver for Faraday Technology FTIDE010") Signed-off-by: Sergey Shtylyov <s.shtylyov@omprussia.ru> --- drivers/ata/pata_ftide010.c | 2 ++ 1 file changed, 2 insertions(+) Index: linux-block/drivers/ata/pata_ftide010.c =================================================================== --- linux-block.orig/drivers/ata/pata_ftide010.c +++ linux-block/drivers/ata/pata_ftide010.c @@ -469,6 +469,8 @@ static int pata_ftide010_probe(struct pl irq = platform_get_irq(pdev, 0); if (irq < 0) return irq; + if (!irq) + return -EINVAL; res = platform_get_resource(pdev, IORESOURCE_MEM, 0); if (!res)
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(+) 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);
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! What about this series? I got no feedback whatsoever -- it seems to have been lost. On 3/21/21 9:50 PM, Sergey Shtylyov wrote: > Here are 4 patches against the 'master' branch of the Jens Axboe's 'linux-blobk.git' > repo plus the 'pataep_93xx' driver patch re-posted yesterday. The affected drivers > use platform_get_irq() which may return IRQ0 (considered invalid, according to Linus) > that means polling when passed to ata_host_activate() called at the end of the probe > methods. I might not have been clear enough: 'irq == 0' means that the libata core would WARN about the non-NULL 'handler' paramter which seems a to be a problem... > I think that the solution to this issue is to explicitly deny IRQ0 returned > by platform_get_irq()... [...] MBR, Sergei
On 5/10/21 11:48 PM, Sergey Shtylyov wrote: > Hello! > > What about this series? > I got no feedback whatsoever -- it seems to have been lost. Almost 4 months have paseed from this reminder. Still no commnets whatsoever... > On 3/21/21 9:50 PM, Sergey Shtylyov wrote: > >> Here are 4 patches against the 'master' branch of the Jens Axboe's 'linux-blobk.git' >> repo plus the 'pataep_93xx' driver patch re-posted yesterday. The affected drivers >> use platform_get_irq() which may return IRQ0 (considered invalid, according to Linus) >> that means polling when passed to ata_host_activate() called at the end of the probe >> methods. > > I might not have been clear enough: 'irq == 0' means that the libata core would WARN about > the non-NULL 'handler' paramter which seems a to be a problem... > >> I think that the solution to this issue is to explicitly deny IRQ0 returned >> by platform_get_irq()... [...] MBR, Sergey
On 9/4/21 12:57 PM, Sergey Shtylyov wrote:
> On 5/10/21 11:48 PM, Sergey Shtylyov wrote:
>
>> Hello!
>>
>> What about this series?
>> I got no feedback whatsoever -- it seems to have been lost.
>
> Almost 4 months have paseed from this reminder. Still no commnets whatsoever...
You did get a review almost immediately, but it wasn't responded to.
--
Jens Axboe
On 05.09.2021 0:25, Jens Axboe wrote:
[...]
>>> What about this series?
>>> I got no feedback whatsoever -- it seems to have been lost.
>>
>> Almost 4 months have paseed from this reminder. Still no commnets whatsoever...
>
> You did get a review almost immediately, but it wasn't responded to.
Oh, seeing it now (again)! Indeed, I forgot to reply back then, sorry!
MBR, Sergey
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