[PATCH 0/4] Explicitly deny IRQ0 in the libata drivers

[PATCH 0/4] Explicitly deny IRQ0 in the libata drivers

[Sergey Shtylyov]

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

[PATCH 1/4] pata_bk3710: deny IRQ0

[Sergey Shtylyov]

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))

[PATCH 2/4] pata_ep93xx: deny IRQ0

[Sergey Shtylyov]

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);

[PATCH 3/4] pata_ftide010: deny IRQ0

[Sergey Shtylyov]

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)

[PATCH 4/4] pata_imx: deny IRQ0

[Sergey Shtylyov]

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);

Re: [PATCH 4/4] pata_imx: deny IRQ0

[Fabio Estevam]

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.

Re: [PATCH 0/4] Explicitly deny IRQ0 in the libata drivers

[Sergey Shtylyov]

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

Re: [PATCH 0/4] Explicitly deny IRQ0 in the libata drivers

[Sergey Shtylyov]

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

Re: [PATCH 0/4] Explicitly deny IRQ0 in the libata drivers

[Jens Axboe]

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

Re: [PATCH 0/4] Explicitly deny IRQ0 in the libata drivers

[Sergey Shtylyov]

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

Re: [PATCH 4/4] pata_imx: deny IRQ0

[Sergey Shtylyov]

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

Re: [PATCH 4/4] pata_imx: deny IRQ0

[Fabio Estevam]

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.

Re: [PATCH 4/4] pata_imx: deny IRQ0

[Fabio Estevam]

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

Re: [PATCH 4/4] pata_imx: deny IRQ0

[Sergey Shtylyov]

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

Re: [PATCH 4/4] pata_imx: deny IRQ0

[Sergey Shtylyov]

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