* [PATCH v1 1/2] ata: libahci_platform: Get rid of dup message when IRQ can't be retrieved @ 2021-12-09 14:59 Andy Shevchenko 2021-12-09 14:59 ` [PATCH v1 2/2] ata: libahci_platform: Remove bogus 32-bit DMA mask attempt Andy Shevchenko ` (3 more replies) 0 siblings, 4 replies; 44+ messages in thread From: Andy Shevchenko @ 2021-12-09 14:59 UTC (permalink / raw) To: Andy Shevchenko, linux-ide, linux-kernel Cc: Hans de Goede, Jens Axboe, Damien Le Moal platform_get_irq() will print a message when it fails. No need to repeat this. While at it, drop redundant check for 0 as platform_get_irq() spills out a big WARN() in such case. Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> --- drivers/ata/libahci_platform.c | 7 +------ 1 file changed, 1 insertion(+), 6 deletions(-) diff --git a/drivers/ata/libahci_platform.c b/drivers/ata/libahci_platform.c index 0910441321f7..1af642c84e7b 100644 --- a/drivers/ata/libahci_platform.c +++ b/drivers/ata/libahci_platform.c @@ -579,13 +579,8 @@ int ahci_platform_init_host(struct platform_device *pdev, int i, irq, n_ports, rc; irq = platform_get_irq(pdev, 0); - if (irq < 0) { - if (irq != -EPROBE_DEFER) - dev_err(dev, "no irq\n"); + if (irq < 0) return irq; - } - if (!irq) - return -EINVAL; hpriv->irq = irq; -- 2.33.0 ^ permalink raw reply related [flat|nested] 44+ messages in thread
* [PATCH v1 2/2] ata: libahci_platform: Remove bogus 32-bit DMA mask attempt 2021-12-09 14:59 [PATCH v1 1/2] ata: libahci_platform: Get rid of dup message when IRQ can't be retrieved Andy Shevchenko @ 2021-12-09 14:59 ` Andy Shevchenko 2021-12-10 19:34 ` Andy Shevchenko 2021-12-17 0:58 ` Damien Le Moal 2021-12-09 15:21 ` [PATCH v1 1/2] ata: libahci_platform: Get rid of dup message when IRQ can't be retrieved Hans de Goede ` (2 subsequent siblings) 3 siblings, 2 replies; 44+ messages in thread From: Andy Shevchenko @ 2021-12-09 14:59 UTC (permalink / raw) To: Andy Shevchenko, linux-ide, linux-kernel Cc: Hans de Goede, Jens Axboe, Damien Le Moal If 64-bit mask attempt fails, the 32-bit will fail by the very same reason. Don't even try the latter. It's a continuation of the changes that contains, e.g. dcc02c19cc06 ("sata_sil24: use dma_set_mask_and_coherent"). Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> --- drivers/ata/libahci_platform.c | 9 ++------- 1 file changed, 2 insertions(+), 7 deletions(-) diff --git a/drivers/ata/libahci_platform.c b/drivers/ata/libahci_platform.c index 1af642c84e7b..972f5ec86a27 100644 --- a/drivers/ata/libahci_platform.c +++ b/drivers/ata/libahci_platform.c @@ -637,13 +637,8 @@ int ahci_platform_init_host(struct platform_device *pdev, if (hpriv->cap & HOST_CAP_64) { rc = dma_coerce_mask_and_coherent(dev, DMA_BIT_MASK(64)); if (rc) { - rc = dma_coerce_mask_and_coherent(dev, - DMA_BIT_MASK(32)); - if (rc) { - dev_err(dev, "Failed to enable 64-bit DMA.\n"); - return rc; - } - dev_warn(dev, "Enable 32-bit DMA instead of 64-bit.\n"); + dev_err(dev, "Failed to enable 64-bit DMA.\n"); + return rc; } } -- 2.33.0 ^ permalink raw reply related [flat|nested] 44+ messages in thread
* Re: [PATCH v1 2/2] ata: libahci_platform: Remove bogus 32-bit DMA mask attempt 2021-12-09 14:59 ` [PATCH v1 2/2] ata: libahci_platform: Remove bogus 32-bit DMA mask attempt Andy Shevchenko @ 2021-12-10 19:34 ` Andy Shevchenko 2021-12-11 0:04 ` Damien Le Moal 2021-12-17 0:58 ` Damien Le Moal 1 sibling, 1 reply; 44+ messages in thread From: Andy Shevchenko @ 2021-12-10 19:34 UTC (permalink / raw) To: linux-ide, linux-kernel; +Cc: Hans de Goede, Jens Axboe, Damien Le Moal On Thu, Dec 09, 2021 at 04:59:37PM +0200, Andy Shevchenko wrote: > If 64-bit mask attempt fails, the 32-bit will fail by the very same reason. > Don't even try the latter. It's a continuation of the changes that contains, > e.g. dcc02c19cc06 ("sata_sil24: use dma_set_mask_and_coherent"). I understand that some people have nothing besides bikeshedding, but this patch seems fine to everybody, am I right? Can it be applied (it's independent from patch 1 anyways)? -- With Best Regards, Andy Shevchenko ^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH v1 2/2] ata: libahci_platform: Remove bogus 32-bit DMA mask attempt 2021-12-10 19:34 ` Andy Shevchenko @ 2021-12-11 0:04 ` Damien Le Moal 0 siblings, 0 replies; 44+ messages in thread From: Damien Le Moal @ 2021-12-11 0:04 UTC (permalink / raw) To: Andy Shevchenko, linux-ide, linux-kernel; +Cc: Hans de Goede, Jens Axboe On 2021/12/11 4:34, Andy Shevchenko wrote: > On Thu, Dec 09, 2021 at 04:59:37PM +0200, Andy Shevchenko wrote: >> If 64-bit mask attempt fails, the 32-bit will fail by the very same reason. >> Don't even try the latter. It's a continuation of the changes that contains, >> e.g. dcc02c19cc06 ("sata_sil24: use dma_set_mask_and_coherent"). > > I understand that some people have nothing besides bikeshedding, but this patch > seems fine to everybody, am I right? Can it be applied (it's independent from > patch 1 anyways)? > Yes, this one seems fine to me. It would be good to get a different review though (I know hard to get reviews on ata patches...). I will queue it for 5.17. -- Damien Le Moal Western Digital Research ^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH v1 2/2] ata: libahci_platform: Remove bogus 32-bit DMA mask attempt 2021-12-09 14:59 ` [PATCH v1 2/2] ata: libahci_platform: Remove bogus 32-bit DMA mask attempt Andy Shevchenko 2021-12-10 19:34 ` Andy Shevchenko @ 2021-12-17 0:58 ` Damien Le Moal 2021-12-17 11:57 ` Andy Shevchenko 1 sibling, 1 reply; 44+ messages in thread From: Damien Le Moal @ 2021-12-17 0:58 UTC (permalink / raw) To: Andy Shevchenko, linux-ide, linux-kernel; +Cc: Hans de Goede, Jens Axboe On 12/9/21 23:59, Andy Shevchenko wrote: > If 64-bit mask attempt fails, the 32-bit will fail by the very same reason. > Don't even try the latter. It's a continuation of the changes that contains, > e.g. dcc02c19cc06 ("sata_sil24: use dma_set_mask_and_coherent"). > > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > --- > drivers/ata/libahci_platform.c | 9 ++------- > 1 file changed, 2 insertions(+), 7 deletions(-) > > diff --git a/drivers/ata/libahci_platform.c b/drivers/ata/libahci_platform.c > index 1af642c84e7b..972f5ec86a27 100644 > --- a/drivers/ata/libahci_platform.c > +++ b/drivers/ata/libahci_platform.c > @@ -637,13 +637,8 @@ int ahci_platform_init_host(struct platform_device *pdev, > if (hpriv->cap & HOST_CAP_64) { > rc = dma_coerce_mask_and_coherent(dev, DMA_BIT_MASK(64)); > if (rc) { > - rc = dma_coerce_mask_and_coherent(dev, > - DMA_BIT_MASK(32)); > - if (rc) { > - dev_err(dev, "Failed to enable 64-bit DMA.\n"); > - return rc; > - } > - dev_warn(dev, "Enable 32-bit DMA instead of 64-bit.\n"); > + dev_err(dev, "Failed to enable 64-bit DMA.\n"); > + return rc; > } > } > > Applied to for-5.17. For patch 1/2, waiting for you v2 restoring the irq == 0 check. -- Damien Le Moal Western Digital Research ^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH v1 2/2] ata: libahci_platform: Remove bogus 32-bit DMA mask attempt 2021-12-17 0:58 ` Damien Le Moal @ 2021-12-17 11:57 ` Andy Shevchenko 0 siblings, 0 replies; 44+ messages in thread From: Andy Shevchenko @ 2021-12-17 11:57 UTC (permalink / raw) To: Damien Le Moal; +Cc: linux-ide, linux-kernel, Hans de Goede, Jens Axboe On Fri, Dec 17, 2021 at 09:58:09AM +0900, Damien Le Moal wrote: > On 12/9/21 23:59, Andy Shevchenko wrote: ... > For patch 1/2, waiting for you v2 restoring the irq == 0 check. Just sent a v2 of it, thanks! -- With Best Regards, Andy Shevchenko ^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH v1 1/2] ata: libahci_platform: Get rid of dup message when IRQ can't be retrieved 2021-12-09 14:59 [PATCH v1 1/2] ata: libahci_platform: Get rid of dup message when IRQ can't be retrieved Andy Shevchenko 2021-12-09 14:59 ` [PATCH v1 2/2] ata: libahci_platform: Remove bogus 32-bit DMA mask attempt Andy Shevchenko @ 2021-12-09 15:21 ` Hans de Goede 2021-12-09 17:24 ` Sergey Shtylyov 2021-12-09 22:49 ` Damien Le Moal 3 siblings, 0 replies; 44+ messages in thread From: Hans de Goede @ 2021-12-09 15:21 UTC (permalink / raw) To: Andy Shevchenko, linux-ide, linux-kernel; +Cc: Jens Axboe, Damien Le Moal Hi, On 12/9/21 15:59, Andy Shevchenko wrote: > platform_get_irq() will print a message when it fails. > No need to repeat this. > > While at it, drop redundant check for 0 as platform_get_irq() spills > out a big WARN() in such case. > > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> Thanks, the series looks good to me: Reviewed-by: Hans de Goede <hdegoede@redhat.com> for the series. Regards, Hans > --- > drivers/ata/libahci_platform.c | 7 +------ > 1 file changed, 1 insertion(+), 6 deletions(-) > > diff --git a/drivers/ata/libahci_platform.c b/drivers/ata/libahci_platform.c > index 0910441321f7..1af642c84e7b 100644 > --- a/drivers/ata/libahci_platform.c > +++ b/drivers/ata/libahci_platform.c > @@ -579,13 +579,8 @@ int ahci_platform_init_host(struct platform_device *pdev, > int i, irq, n_ports, rc; > > irq = platform_get_irq(pdev, 0); > - if (irq < 0) { > - if (irq != -EPROBE_DEFER) > - dev_err(dev, "no irq\n"); > + if (irq < 0) > return irq; > - } > - if (!irq) > - return -EINVAL; > > hpriv->irq = irq; > > ^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH v1 1/2] ata: libahci_platform: Get rid of dup message when IRQ can't be retrieved 2021-12-09 14:59 [PATCH v1 1/2] ata: libahci_platform: Get rid of dup message when IRQ can't be retrieved Andy Shevchenko 2021-12-09 14:59 ` [PATCH v1 2/2] ata: libahci_platform: Remove bogus 32-bit DMA mask attempt Andy Shevchenko 2021-12-09 15:21 ` [PATCH v1 1/2] ata: libahci_platform: Get rid of dup message when IRQ can't be retrieved Hans de Goede @ 2021-12-09 17:24 ` Sergey Shtylyov 2021-12-09 17:42 ` Andy Shevchenko 2021-12-09 22:49 ` Damien Le Moal 3 siblings, 1 reply; 44+ messages in thread From: Sergey Shtylyov @ 2021-12-09 17:24 UTC (permalink / raw) To: Andy Shevchenko, linux-ide, linux-kernel Cc: Hans de Goede, Jens Axboe, Damien Le Moal On 12/9/21 5:59 PM, Andy Shevchenko wrote: > platform_get_irq() will print a message when it fails. > No need to repeat this. > > While at it, drop redundant check for 0 as platform_get_irq() spills > out a big WARN() in such case. And? IRQ0 is still returned! :-( > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > --- > drivers/ata/libahci_platform.c | 7 +------ > 1 file changed, 1 insertion(+), 6 deletions(-) > > diff --git a/drivers/ata/libahci_platform.c b/drivers/ata/libahci_platform.c > index 0910441321f7..1af642c84e7b 100644 > --- a/drivers/ata/libahci_platform.c > +++ b/drivers/ata/libahci_platform.c > @@ -579,13 +579,8 @@ int ahci_platform_init_host(struct platform_device *pdev, > int i, irq, n_ports, rc; > > irq = platform_get_irq(pdev, 0); > - if (irq < 0) { > - if (irq != -EPROBE_DEFER) > - dev_err(dev, "no irq\n"); > + if (irq < 0) > return irq; > - } > - if (!irq) > - return -EINVAL; This is prermature -- let's wait till my patch that stops returning IRQ0 from platform_get_irq() and friends gets merged.... MBR, Sergey ^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH v1 1/2] ata: libahci_platform: Get rid of dup message when IRQ can't be retrieved 2021-12-09 17:24 ` Sergey Shtylyov @ 2021-12-09 17:42 ` Andy Shevchenko 2021-12-09 18:22 ` Sergey Shtylyov 0 siblings, 1 reply; 44+ messages in thread From: Andy Shevchenko @ 2021-12-09 17:42 UTC (permalink / raw) To: Sergey Shtylyov Cc: linux-ide, linux-kernel, Hans de Goede, Jens Axboe, Damien Le Moal On Thu, Dec 09, 2021 at 08:24:59PM +0300, Sergey Shtylyov wrote: > On 12/9/21 5:59 PM, Andy Shevchenko wrote: > > > platform_get_irq() will print a message when it fails. > > No need to repeat this. > > > > While at it, drop redundant check for 0 as platform_get_irq() spills > > out a big WARN() in such case. > > And? IRQ0 is still returned! :-( It should not be returned in the first place. ... > > - if (!irq) > > - return -EINVAL; > > This is prermature -- let's wait till my patch that stops returning IRQ0 from > platform_get_irq() and friends gets merged.... What patch? Does it fix platform_get_irq_optional()? -- With Best Regards, Andy Shevchenko ^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH v1 1/2] ata: libahci_platform: Get rid of dup message when IRQ can't be retrieved 2021-12-09 17:42 ` Andy Shevchenko @ 2021-12-09 18:22 ` Sergey Shtylyov 2021-12-09 19:22 ` Andy Shevchenko 0 siblings, 1 reply; 44+ messages in thread From: Sergey Shtylyov @ 2021-12-09 18:22 UTC (permalink / raw) To: Andy Shevchenko Cc: linux-ide, linux-kernel, Hans de Goede, Jens Axboe, Damien Le Moal On 12/9/21 8:42 PM, Andy Shevchenko wrote: >>> platform_get_irq() will print a message when it fails. >>> No need to repeat this. >>> >>> While at it, drop redundant check for 0 as platform_get_irq() spills >>> out a big WARN() in such case. >> >> And? IRQ0 is still returned! :-( > > It should not be returned in the first place. But it still is, despite the WARN(), right? > ... > >>> - if (!irq) >>> - return -EINVAL; >> >> This is prermature -- let's wait till my patch that stops returning IRQ0 from >> platform_get_irq() and friends gets merged.... > > What patch? https://marc.info/?l=linux-kernel&m=163623041902285 > Does it fix platform_get_irq_optional()? Of course! :-) MBR, Sergey ^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH v1 1/2] ata: libahci_platform: Get rid of dup message when IRQ can't be retrieved 2021-12-09 18:22 ` Sergey Shtylyov @ 2021-12-09 19:22 ` Andy Shevchenko 2021-12-09 19:27 ` Andy Shevchenko 2021-12-09 20:29 ` Sergey Shtylyov 0 siblings, 2 replies; 44+ messages in thread From: Andy Shevchenko @ 2021-12-09 19:22 UTC (permalink / raw) To: Sergey Shtylyov Cc: linux-ide, linux-kernel, Hans de Goede, Jens Axboe, Damien Le Moal On Thu, Dec 09, 2021 at 09:22:32PM +0300, Sergey Shtylyov wrote: > On 12/9/21 8:42 PM, Andy Shevchenko wrote: ... > >>> No need to repeat this. > >>> > >>> While at it, drop redundant check for 0 as platform_get_irq() spills > >>> out a big WARN() in such case. > >> > >> And? IRQ0 is still returned! :-( > > > > It should not be returned in the first place. > > But it still is, despite the WARN(), right? So, you admit that there is a code which does that? That code should be fixed first. Have you sent a patch? ... > >>> - if (!irq) > >>> - return -EINVAL; > >> > >> This is prermature -- let's wait till my patch that stops returning IRQ0 from > >> platform_get_irq() and friends gets merged.... > > > > What patch? > > https://marc.info/?l=linux-kernel&m=163623041902285 > > > Does it fix platform_get_irq_optional()? > > Of course! :-) Can you share link to lore.kernel.org, please? It will make much easier to try and comment. -- With Best Regards, Andy Shevchenko ^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH v1 1/2] ata: libahci_platform: Get rid of dup message when IRQ can't be retrieved 2021-12-09 19:22 ` Andy Shevchenko @ 2021-12-09 19:27 ` Andy Shevchenko 2021-12-09 20:31 ` Sergey Shtylyov 2021-12-09 20:29 ` Sergey Shtylyov 1 sibling, 1 reply; 44+ messages in thread From: Andy Shevchenko @ 2021-12-09 19:27 UTC (permalink / raw) To: Sergey Shtylyov Cc: linux-ide, linux-kernel, Hans de Goede, Jens Axboe, Damien Le Moal On Thu, Dec 09, 2021 at 09:22:55PM +0200, Andy Shevchenko wrote: > On Thu, Dec 09, 2021 at 09:22:32PM +0300, Sergey Shtylyov wrote: > > On 12/9/21 8:42 PM, Andy Shevchenko wrote: ... > > > Does it fix platform_get_irq_optional()? > > > > Of course! :-) > > Can you share link to lore.kernel.org, please? > It will make much easier to try and comment. For the record it does not fix platform_get_irq_optional() AFAICS. Have you read the discussion: https://lore.kernel.org/lkml/20210331144526.19439-1-andriy.shevchenko@linux.intel.com/T/#u ? -- With Best Regards, Andy Shevchenko ^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH v1 1/2] ata: libahci_platform: Get rid of dup message when IRQ can't be retrieved 2021-12-09 19:27 ` Andy Shevchenko @ 2021-12-09 20:31 ` Sergey Shtylyov 0 siblings, 0 replies; 44+ messages in thread From: Sergey Shtylyov @ 2021-12-09 20:31 UTC (permalink / raw) To: Andy Shevchenko Cc: linux-ide, linux-kernel, Hans de Goede, Jens Axboe, Damien Le Moal On 12/9/21 10:27 PM, Andy Shevchenko wrote: [...] >>>> Does it fix platform_get_irq_optional()? >>> >>> Of course! :-) >> >> Can you share link to lore.kernel.org, please? >> It will make much easier to try and comment. > > For the record it does not fix platform_get_irq_optional() AFAICS. Again, why? My only issue is stoppping to return IRQ0 --- which it does. :-) > Have you read the discussion: > https://lore.kernel.org/lkml/20210331144526.19439-1-andriy.shevchenko@linux.intel.com/T/#u > ? Yes, I have. I just don't care about it much... :-) MBR, Sergey ^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH v1 1/2] ata: libahci_platform: Get rid of dup message when IRQ can't be retrieved 2021-12-09 19:22 ` Andy Shevchenko 2021-12-09 19:27 ` Andy Shevchenko @ 2021-12-09 20:29 ` Sergey Shtylyov 2021-12-10 10:44 ` Andy Shevchenko 1 sibling, 1 reply; 44+ messages in thread From: Sergey Shtylyov @ 2021-12-09 20:29 UTC (permalink / raw) To: Andy Shevchenko Cc: linux-ide, linux-kernel, Hans de Goede, Jens Axboe, Damien Le Moal On 12/9/21 10:22 PM, Andy Shevchenko wrote: [...] >>>>> No need to repeat this. >>>>> >>>>> While at it, drop redundant check for 0 as platform_get_irq() spills >>>>> out a big WARN() in such case. >>>> >>>> And? IRQ0 is still returned! :-( >>> >>> It should not be returned in the first place. >> >> But it still is, despite the WARN(), right? > > So, you admit that there is a code which does that? I admit *what*?! That platfrom_get_irq() and its ilk return IRQ0 while they shouldn't? =) > That code should be fixed first. Have you sent a patch? Which code?! You got me totally muddled. =) > ... > >>>>> - if (!irq) >>>>> - return -EINVAL; >>>> >>>> This is prermature -- let's wait till my patch that stops returning IRQ0 from >>>> platform_get_irq() and friends gets merged.... >>> >>> What patch? >> >> https://marc.info/?l=linux-kernel&m=163623041902285 >> >>> Does it fix platform_get_irq_optional()? >> >> Of course! :-) > > Can you share link to lore.kernel.org, please? > It will make much easier to try and comment. I don't know how to uise it yet, and I'm a little busy with other IRQ0 issues ATM, so I'm afraid you're on your own here... MBR, Sergey ^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH v1 1/2] ata: libahci_platform: Get rid of dup message when IRQ can't be retrieved 2021-12-09 20:29 ` Sergey Shtylyov @ 2021-12-10 10:44 ` Andy Shevchenko 2021-12-10 11:14 ` Sergey Shtylyov 0 siblings, 1 reply; 44+ messages in thread From: Andy Shevchenko @ 2021-12-10 10:44 UTC (permalink / raw) To: Sergey Shtylyov Cc: linux-ide, linux-kernel, Hans de Goede, Jens Axboe, Damien Le Moal On Thu, Dec 09, 2021 at 11:29:07PM +0300, Sergey Shtylyov wrote: > On 12/9/21 10:22 PM, Andy Shevchenko wrote: ... > >>>>> While at it, drop redundant check for 0 as platform_get_irq() spills > >>>>> out a big WARN() in such case. > >>>> > >>>> And? IRQ0 is still returned! :-( > >>> > >>> It should not be returned in the first place. > >> > >> But it still is, despite the WARN(), right? > > > > So, you admit that there is a code which does that? > > I admit *what*?! That platfrom_get_irq() and its ilk return IRQ0 while they > shouldn't? =) That there is a code beneath platform_get_irq() that returns 0, yes. > > That code should be fixed first. Have you sent a patch? > > Which code?! You got me totally muddled. =) Above mentioned. ... > >>>>> - if (!irq) > >>>>> - return -EINVAL; > >>>> > >>>> This is prermature -- let's wait till my patch that stops returning IRQ0 from > >>>> platform_get_irq() and friends gets merged.... > >>> > >>> What patch? > >> > >> https://marc.info/?l=linux-kernel&m=163623041902285 > >> > >>> Does it fix platform_get_irq_optional()? > >> > >> Of course! :-) > > > > Can you share link to lore.kernel.org, please? > > It will make much easier to try and comment. > > I don't know how to uise it yet, and I'm a little busy with other IRQ0 issues ATM, > so I'm afraid you're on your own here... lore.kernel.org is the official mailing list archive for Linux kernel work AFAIU. Other sites may do whatever they want with that information, so --> they are unreliable. If you wish to follow the better process, use lore.kernel.org. Understanding how it works takes no more than 5 minutes by engineer with your kind of experience with Linux kernel development. -- With Best Regards, Andy Shevchenko ^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH v1 1/2] ata: libahci_platform: Get rid of dup message when IRQ can't be retrieved 2021-12-10 10:44 ` Andy Shevchenko @ 2021-12-10 11:14 ` Sergey Shtylyov 2021-12-10 11:28 ` Andy Shevchenko 0 siblings, 1 reply; 44+ messages in thread From: Sergey Shtylyov @ 2021-12-10 11:14 UTC (permalink / raw) To: Andy Shevchenko Cc: linux-ide, linux-kernel, Hans de Goede, Jens Axboe, Damien Le Moal Hello! On 12/10/21 1:44 PM, Andy Shevchenko wrote: >>>>>>> While at it, drop redundant check for 0 as platform_get_irq() spills >>>>>>> out a big WARN() in such case. >>>>>> >>>>>> And? IRQ0 is still returned! :-( >>>>> >>>>> It should not be returned in the first place. >>>> >>>> But it still is, despite the WARN(), right? >>> >>> So, you admit that there is a code which does that? >> >> I admit *what*?! That platfrom_get_irq() and its ilk return IRQ0 while they >> shouldn't? =) > > That there is a code beneath platform_get_irq() that returns 0, yes. Look at the ACPI-specific GpioInt handling code (just above the out_not_found label) -- I'm not sure the check there is correct -- I'm not very familiar with ACPI, you seem to know it much better. :-) Also, 0 can be specified via the normal IRQ resource. I know of e.g. the Alchemy MIPS SoCs that have IRQ0 used by UART0; luckily, currently SoC IRQs are mapped starting at Linux IRQ8 (but it wasn't the case in the 2.6.1x time frame where we had issue with the serial driver)... >>> That code should be fixed first. Have you sent a patch? >> >> Which code?! You got me totally muddled. =) > > Above mentioned. What needs to be fixed in this case is the interrupt controller driver. Quoting Linus (imprecisely :-)), IRQ #s should be either mapped starting with #1 or IRQ0 remapped at the end of the controller's interrupt range... I currently have no information on the platforms requiring such kind of fixing (Alchemy don't seem to need it now)... > ... > >>>>>>> - if (!irq) >>>>>>> - return -EINVAL; >>>>>> >>>>>> This is prermature -- let's wait till my patch that stops returning IRQ0 from >>>>>> platform_get_irq() and friends gets merged.... >>>>> >>>>> What patch? >>>> >>>> https://marc.info/?l=linux-kernel&m=163623041902285 >>>> >>>>> Does it fix platform_get_irq_optional()? >>>> >>>> Of course! :-) >>> >>> Can you share link to lore.kernel.org, please? >>> It will make much easier to try and comment. >> >> I don't know how to uise it yet, and I'm a little busy with other IRQ0 issues ATM, A little bit, I meant to type. >> so I'm afraid you're on your own here... > > lore.kernel.org is the official mailing list archive for Linux kernel work > AFAIU. Other sites may do whatever they want with that information, so --> > they are unreliable. If you wish to follow the better process, use > lore.kernel.org. Understanding how it works takes no more than 5 minutes > by engineer with your kind of experience with Linux kernel development. OK, I'll explore this archive when I have time. BTW, does it keep the messages not posted to LKML (I tend to only CC LKML if there's no other mailing lists to post to)? MBR, Sergey ^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH v1 1/2] ata: libahci_platform: Get rid of dup message when IRQ can't be retrieved 2021-12-10 11:14 ` Sergey Shtylyov @ 2021-12-10 11:28 ` Andy Shevchenko 2021-12-10 17:39 ` Sergey Shtylyov 0 siblings, 1 reply; 44+ messages in thread From: Andy Shevchenko @ 2021-12-10 11:28 UTC (permalink / raw) To: Sergey Shtylyov Cc: linux-ide, linux-kernel, Hans de Goede, Jens Axboe, Damien Le Moal On Fri, Dec 10, 2021 at 02:14:15PM +0300, Sergey Shtylyov wrote: > On 12/10/21 1:44 PM, Andy Shevchenko wrote: > > >>>>>>> While at it, drop redundant check for 0 as platform_get_irq() spills > >>>>>>> out a big WARN() in such case. > >>>>>> > >>>>>> And? IRQ0 is still returned! :-( > >>>>> > >>>>> It should not be returned in the first place. > >>>> > >>>> But it still is, despite the WARN(), right? > >>> > >>> So, you admit that there is a code which does that? > >> > >> I admit *what*?! That platfrom_get_irq() and its ilk return IRQ0 while they > >> shouldn't? =) > > > > That there is a code beneath platform_get_irq() that returns 0, yes. > > Look at the ACPI-specific GpioInt handling code (just above the out_not_found label) -- > I'm not sure the check there is correct -- I'm not very familiar with ACPI, you seem to > know it much better. :-) And what is your point here exactly? If == 0 case happens, it will be immediately WARN() and reported (I hope) since it will mean bug in the code. > Also, 0 can be specified via the normal IRQ resource. I know of e.g. the Alchemy MIPS SoCs > that have IRQ0 used by UART0; luckily, currently SoC IRQs are mapped starting at Linux IRQ8 > (but it wasn't the case in the 2.6.1x time frame where we had issue with the serial driver)... You mixed up HW IRQ with vIRQ. The former one may be 0 and it's completely valid case, while the second one is not. > >>> That code should be fixed first. Have you sent a patch? > >> > >> Which code?! You got me totally muddled. =) > > > > Above mentioned. > > What needs to be fixed in this case is the interrupt controller driver. What do you mean by that? vIRQ is handled by IRQ core, IRQ controller driver just a mere provider of the resource. And those exceptions for vIRQ == 0 shouldn't be propagated to the platform code or so. > Quoting Linus > (imprecisely :-)), IRQ #s should be either mapped starting with #1 or IRQ0 remapped at > the end of the controller's interrupt range... I currently have no information on the > platforms requiring such kind of fixing (Alchemy don't seem to need it now)... Again, do not mix vIRQ (about which Linus ranted) and HW IRQ. ... > >>>>>>> - if (!irq) > >>>>>>> - return -EINVAL; > >>>>>> > >>>>>> This is prermature -- let's wait till my patch that stops returning IRQ0 from > >>>>>> platform_get_irq() and friends gets merged.... > >>>>> > >>>>> What patch? > >>>> > >>>> https://marc.info/?l=linux-kernel&m=163623041902285 > >>>> > >>>>> Does it fix platform_get_irq_optional()? > >>>> > >>>> Of course! :-) > >>> > >>> Can you share link to lore.kernel.org, please? > >>> It will make much easier to try and comment. > >> > >> I don't know how to uise it yet, and I'm a little busy with other IRQ0 issues ATM, > A little bit, I meant to type. No problem. I just haven't got what other IRQ0 issues except fixing platform_get_irq_optional() et al. could be possibly needed... > >> so I'm afraid you're on your own here... > > > > lore.kernel.org is the official mailing list archive for Linux kernel work > > AFAIU. Other sites may do whatever they want with that information, so --> > > they are unreliable. If you wish to follow the better process, use > > lore.kernel.org. Understanding how it works takes no more than 5 minutes > > by engineer with your kind of experience with Linux kernel development. > > OK, I'll explore this archive when I have time. BTW, does it keep the messages not > posted to LKML (I tend to only CC LKML if there's no other mailing lists to post to)? TL;DR: yes. -- With Best Regards, Andy Shevchenko ^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH v1 1/2] ata: libahci_platform: Get rid of dup message when IRQ can't be retrieved 2021-12-10 11:28 ` Andy Shevchenko @ 2021-12-10 17:39 ` Sergey Shtylyov 2021-12-10 17:51 ` Andy Shevchenko 0 siblings, 1 reply; 44+ messages in thread From: Sergey Shtylyov @ 2021-12-10 17:39 UTC (permalink / raw) To: Andy Shevchenko Cc: linux-ide, linux-kernel, Hans de Goede, Jens Axboe, Damien Le Moal On 12/10/21 2:28 PM, Andy Shevchenko wrote: >>>>>>>>> While at it, drop redundant check for 0 as platform_get_irq() spills >>>>>>>>> out a big WARN() in such case. >>>>>>>> >>>>>>>> And? IRQ0 is still returned! :-( >>>>>>> >>>>>>> It should not be returned in the first place. >>>>>> >>>>>> But it still is, despite the WARN(), right? >>>>> >>>>> So, you admit that there is a code which does that? >>>> >>>> I admit *what*?! That platfrom_get_irq() and its ilk return IRQ0 while they >>>> shouldn't? =) >>> >>> That there is a code beneath platform_get_irq() that returns 0, yes. >> >> Look at the ACPI-specific GpioInt handling code (just above the out_not_found label) -- >> I'm not sure the check there is correct -- I'm not very familiar with ACPI, you seem to >> know it much better. :-) > > And what is your point here exactly? You're saying IRQ0 shouldn't be returned (by the ACPI code) -- from this fragment we can see that it may be returned... > If == 0 case happens, it will be > immediately WARN() and reported (I hope) Well, "hope dies last"... :-) > since it will mean bug in the code. > >> Also, 0 can be specified via the normal IRQ resource. I know of e.g. the Alchemy MIPS SoCs >> that have IRQ0 used by UART0; luckily, currently SoC IRQs are mapped starting at Linux IRQ8 >> (but it wasn't the case in the 2.6.1x time frame where we had issue with the serial driver)... > > You mixed up HW IRQ with vIRQ. I didn't. Linux expects the vIRQs (I called them Linux IRQs). In the 2.6.1x time frame those corresponded 1:1 on Alchemy. Also, there's 8259 which is always mapped at vIRQ0 (or the legacy drivers won't work). > The former one may be 0 and it's completely valid case, while > the second one is not. Well, request_irq() happilly takes vIRQ0. Moreover, there are 8253 drivers in e.g. the arch/x86/ (PPC and MIPS too) which do use vIRQ0. >>>>> That code should be fixed first. Have you sent a patch? >>>> >>>> Which code?! You got me totally muddled. =) >>> >>> Above mentioned. >> >> What needs to be fixed in this case is the interrupt controller driver. > > What do you mean by that? You better ask Linus... ;-) > vIRQ is handled by IRQ core, IRQ controller driver > just a mere provider of the resource. And those exceptions for vIRQ == 0 > shouldn't be propagated to the platform code or so. >> Quoting Linus >> (imprecisely :-)), IRQ #s should be either mapped starting with #1 or IRQ0 remapped at >> the end of the controller's interrupt range... I currently have no information on the >> platforms requiring such kind of fixing (Alchemy don't seem to need it now)... Well, actually that Linus' quote predates drivers/irqchip/, so I must confess this argument was wrong... :-) > Again, do not mix vIRQ (about which Linus ranted) and HW IRQ. > > ... > >>>>>>>>> - if (!irq) >>>>>>>>> - return -EINVAL; >>>>>>>> >>>>>>>> This is prermature -- let's wait till my patch that stops returning IRQ0 from >>>>>>>> platform_get_irq() and friends gets merged.... >>>>>>> >>>>>>> What patch? >>>>>> >>>>>> https://marc.info/?l=linux-kernel&m=163623041902285 >>>>>> >>>>>>> Does it fix platform_get_irq_optional()? >>>>>> >>>>>> Of course! :-) >>>>> >>>>> Can you share link to lore.kernel.org, please? >>>>> It will make much easier to try and comment. >>>> >>>> I don't know how to uise it yet, and I'm a little busy with other IRQ0 issues ATM, > >> A little bit, I meant to type. > > No problem. I just haven't got what other IRQ0 issues except fixing > platform_get_irq_optional() et al. could be possibly needed... There is other IRQ0 issue which is very old already... [...] MBR, Sergey ^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH v1 1/2] ata: libahci_platform: Get rid of dup message when IRQ can't be retrieved 2021-12-10 17:39 ` Sergey Shtylyov @ 2021-12-10 17:51 ` Andy Shevchenko 0 siblings, 0 replies; 44+ messages in thread From: Andy Shevchenko @ 2021-12-10 17:51 UTC (permalink / raw) To: Sergey Shtylyov Cc: linux-ide, linux-kernel, Hans de Goede, Jens Axboe, Damien Le Moal On Fri, Dec 10, 2021 at 08:39:38PM +0300, Sergey Shtylyov wrote: > On 12/10/21 2:28 PM, Andy Shevchenko wrote: > > >>>>>>>>> While at it, drop redundant check for 0 as platform_get_irq() spills > >>>>>>>>> out a big WARN() in such case. > >>>>>>>> > >>>>>>>> And? IRQ0 is still returned! :-( > >>>>>>> > >>>>>>> It should not be returned in the first place. > >>>>>> > >>>>>> But it still is, despite the WARN(), right? > >>>>> > >>>>> So, you admit that there is a code which does that? > >>>> > >>>> I admit *what*?! That platfrom_get_irq() and its ilk return IRQ0 while they > >>>> shouldn't? =) > >>> > >>> That there is a code beneath platform_get_irq() that returns 0, yes. > >> > >> Look at the ACPI-specific GpioInt handling code (just above the out_not_found label) -- > >> I'm not sure the check there is correct -- I'm not very familiar with ACPI, you seem to > >> know it much better. :-) > > > > And what is your point here exactly? > > You're saying IRQ0 shouldn't be returned (by the ACPI code) -- from this fragment > we can see that it may be returned... Please, provide your analysis, I really don't see how it's possible. If you prove that, we must fix the severe bug then. > > If == 0 case happens, it will be > > immediately WARN() and reported (I hope) > > Well, "hope dies last"... :-) Believe, big WARNs are quite likely to be reported if not by humans, then by CIs and fuzzers. So, the hope is rather to word 'immediately'. > > since it will mean bug in the code. > > > >> Also, 0 can be specified via the normal IRQ resource. I know of e.g. the Alchemy MIPS SoCs > >> that have IRQ0 used by UART0; luckily, currently SoC IRQs are mapped starting at Linux IRQ8 > >> (but it wasn't the case in the 2.6.1x time frame where we had issue with the serial driver)... > > > > You mixed up HW IRQ with vIRQ. > > I didn't. Linux expects the vIRQs (I called them Linux IRQs). In the 2.6.1x time frame > those corresponded 1:1 on Alchemy. Also, there's 8259 which is always mapped at vIRQ0 (or > the legacy drivers won't work). > > > The former one may be 0 and it's completely valid case, while > > the second one is not. > > Well, request_irq() happilly takes vIRQ0. Moreover, there are 8253 drivers in e.g. the arch/x86/ > (PPC and MIPS too) which do use vIRQ0. This is an exception which is not in the scope here. Let me remind that the topic here is libahci_platform and platform_get_irq(). > >>>>> That code should be fixed first. Have you sent a patch? > >>>> > >>>> Which code?! You got me totally muddled. =) > >>> > >>> Above mentioned. > >> > >> What needs to be fixed in this case is the interrupt controller driver. > > > > What do you mean by that? > > You better ask Linus... ;-) If you cite somebody you have to understand what they said, right? Lemme repeat the question, what do you mean by that? In your own words, please. > > vIRQ is handled by IRQ core, IRQ controller driver > > just a mere provider of the resource. And those exceptions for vIRQ == 0 > > shouldn't be propagated to the platform code or so. > > >> Quoting Linus > >> (imprecisely :-)), IRQ #s should be either mapped starting with #1 or IRQ0 remapped at > >> the end of the controller's interrupt range... I currently have no information on the > >> platforms requiring such kind of fixing (Alchemy don't seem to need it now)... > > Well, actually that Linus' quote predates drivers/irqchip/, so I must confess this > argument was wrong... :-) > > > Again, do not mix vIRQ (about which Linus ranted) and HW IRQ. ... > >>>>>>>>> - if (!irq) > >>>>>>>>> - return -EINVAL; > >>>>>>>> > >>>>>>>> This is prermature -- let's wait till my patch that stops returning IRQ0 from > >>>>>>>> platform_get_irq() and friends gets merged.... > >>>>>>> > >>>>>>> What patch? > >>>>>> > >>>>>> https://marc.info/?l=linux-kernel&m=163623041902285 > >>>>>> > >>>>>>> Does it fix platform_get_irq_optional()? > >>>>>> > >>>>>> Of course! :-) > >>>>> > >>>>> Can you share link to lore.kernel.org, please? > >>>>> It will make much easier to try and comment. > >>>> > >>>> I don't know how to uise it yet, and I'm a little busy with other IRQ0 issues ATM, > > > >> A little bit, I meant to type. > > > > No problem. I just haven't got what other IRQ0 issues except fixing > > platform_get_irq_optional() et al. could be possibly needed... > > There is other IRQ0 issue which is very old already... Is it big secret? What is that issue? -- With Best Regards, Andy Shevchenko ^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH v1 1/2] ata: libahci_platform: Get rid of dup message when IRQ can't be retrieved 2021-12-09 14:59 [PATCH v1 1/2] ata: libahci_platform: Get rid of dup message when IRQ can't be retrieved Andy Shevchenko ` (2 preceding siblings ...) 2021-12-09 17:24 ` Sergey Shtylyov @ 2021-12-09 22:49 ` Damien Le Moal 2021-12-09 22:57 ` Damien Le Moal ` (2 more replies) 3 siblings, 3 replies; 44+ messages in thread From: Damien Le Moal @ 2021-12-09 22:49 UTC (permalink / raw) To: Andy Shevchenko, linux-ide, linux-kernel; +Cc: Hans de Goede, Jens Axboe On 2021/12/09 23:59, Andy Shevchenko wrote: > platform_get_irq() will print a message when it fails. > No need to repeat this. > > While at it, drop redundant check for 0 as platform_get_irq() spills > out a big WARN() in such case. The reason you should be able to remove the "if (!irq)" test is that platform_get_irq() never returns 0. At least, that is what the function kdoc says. But looking at platform_get_irq_optional(), which is called by platform_get_irq(), the out label is: WARN(ret == 0, "0 is an invalid IRQ number\n"); return ret; So 0 will be returned as-is. That is rather weird. That should be fixed to return -ENXIO: if (WARN(ret == 0, "0 is an invalid IRQ number\n")) return -ENXIO; return ret; Otherwise, I do not think that removing the "if (!irq)" hunk is safe. no ? > > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > --- > drivers/ata/libahci_platform.c | 7 +------ > 1 file changed, 1 insertion(+), 6 deletions(-) > > diff --git a/drivers/ata/libahci_platform.c b/drivers/ata/libahci_platform.c > index 0910441321f7..1af642c84e7b 100644 > --- a/drivers/ata/libahci_platform.c > +++ b/drivers/ata/libahci_platform.c > @@ -579,13 +579,8 @@ int ahci_platform_init_host(struct platform_device *pdev, > int i, irq, n_ports, rc; > > irq = platform_get_irq(pdev, 0); > - if (irq < 0) { > - if (irq != -EPROBE_DEFER) > - dev_err(dev, "no irq\n"); > + if (irq < 0) > return irq; > - } > - if (!irq) > - return -EINVAL; > > hpriv->irq = irq; > -- Damien Le Moal Western Digital Research ^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH v1 1/2] ata: libahci_platform: Get rid of dup message when IRQ can't be retrieved 2021-12-09 22:49 ` Damien Le Moal @ 2021-12-09 22:57 ` Damien Le Moal 2021-12-10 8:47 ` Andy Shevchenko 2021-12-10 8:59 ` Sergey Shtylyov 2 siblings, 0 replies; 44+ messages in thread From: Damien Le Moal @ 2021-12-09 22:57 UTC (permalink / raw) To: Andy Shevchenko, linux-ide, linux-kernel; +Cc: Hans de Goede, Jens Axboe On 2021/12/10 7:49, Damien Le Moal wrote: > On 2021/12/09 23:59, Andy Shevchenko wrote: >> platform_get_irq() will print a message when it fails. >> No need to repeat this. >> >> While at it, drop redundant check for 0 as platform_get_irq() spills >> out a big WARN() in such case. > > The reason you should be able to remove the "if (!irq)" test is that > platform_get_irq() never returns 0. At least, that is what the function kdoc > says. But looking at platform_get_irq_optional(), which is called by > platform_get_irq(), the out label is: > > WARN(ret == 0, "0 is an invalid IRQ number\n"); > return ret; > > So 0 will be returned as-is. That is rather weird. That should be fixed to > return -ENXIO: > > if (WARN(ret == 0, "0 is an invalid IRQ number\n")) > return -ENXIO; > return ret; > > Otherwise, I do not think that removing the "if (!irq)" hunk is safe. no ? Replying to myself :) I replied before reading Sergei replies that points this out. > >> >> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> >> --- >> drivers/ata/libahci_platform.c | 7 +------ >> 1 file changed, 1 insertion(+), 6 deletions(-) >> >> diff --git a/drivers/ata/libahci_platform.c b/drivers/ata/libahci_platform.c >> index 0910441321f7..1af642c84e7b 100644 >> --- a/drivers/ata/libahci_platform.c >> +++ b/drivers/ata/libahci_platform.c >> @@ -579,13 +579,8 @@ int ahci_platform_init_host(struct platform_device *pdev, >> int i, irq, n_ports, rc; >> >> irq = platform_get_irq(pdev, 0); >> - if (irq < 0) { >> - if (irq != -EPROBE_DEFER) >> - dev_err(dev, "no irq\n"); >> + if (irq < 0) >> return irq; >> - } >> - if (!irq) >> - return -EINVAL; >> >> hpriv->irq = irq; >> > > -- Damien Le Moal Western Digital Research ^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH v1 1/2] ata: libahci_platform: Get rid of dup message when IRQ can't be retrieved 2021-12-09 22:49 ` Damien Le Moal 2021-12-09 22:57 ` Damien Le Moal @ 2021-12-10 8:47 ` Andy Shevchenko 2021-12-10 16:38 ` Sergey Shtylyov 2021-12-10 8:59 ` Sergey Shtylyov 2 siblings, 1 reply; 44+ messages in thread From: Andy Shevchenko @ 2021-12-10 8:47 UTC (permalink / raw) To: Damien Le Moal Cc: Andy Shevchenko, linux-ide, Linux Kernel Mailing List, Hans de Goede, Jens Axboe On Fri, Dec 10, 2021 at 4:47 AM Damien Le Moal <damien.lemoal@opensource.wdc.com> wrote: > On 2021/12/09 23:59, Andy Shevchenko wrote: > > platform_get_irq() will print a message when it fails. > > No need to repeat this. > > > > While at it, drop redundant check for 0 as platform_get_irq() spills > > out a big WARN() in such case. > > The reason you should be able to remove the "if (!irq)" test is that > platform_get_irq() never returns 0. At least, that is what the function kdoc > says. But looking at platform_get_irq_optional(), which is called by > platform_get_irq(), the out label is: > > WARN(ret == 0, "0 is an invalid IRQ number\n"); > return ret; > > So 0 will be returned as-is. That is rather weird. That should be fixed to > return -ENXIO: > > if (WARN(ret == 0, "0 is an invalid IRQ number\n")) > return -ENXIO; No, this is wrong for the same reasons I explained to Sergey. The problem is that this is _optional API and it has been misdesigned. Replacing things like above will increase the mess. > return ret; > > Otherwise, I do not think that removing the "if (!irq)" hunk is safe. no ? No. This is not a business of the caller to workaround implementation details (bugs) of the core APIs. If something goes wrong, then it's platform_get_irq() to blame, and not the libahci_platform. -- With Best Regards, Andy Shevchenko ^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH v1 1/2] ata: libahci_platform: Get rid of dup message when IRQ can't be retrieved 2021-12-10 8:47 ` Andy Shevchenko @ 2021-12-10 16:38 ` Sergey Shtylyov 2021-12-10 17:57 ` Andy Shevchenko 0 siblings, 1 reply; 44+ messages in thread From: Sergey Shtylyov @ 2021-12-10 16:38 UTC (permalink / raw) To: Andy Shevchenko, Damien Le Moal Cc: Andy Shevchenko, linux-ide, Linux Kernel Mailing List, Hans de Goede, Jens Axboe On 12/10/21 11:47 AM, Andy Shevchenko wrote: >>> platform_get_irq() will print a message when it fails. >>> No need to repeat this. >>> >>> While at it, drop redundant check for 0 as platform_get_irq() spills >>> out a big WARN() in such case. >> >> The reason you should be able to remove the "if (!irq)" test is that >> platform_get_irq() never returns 0. At least, that is what the function kdoc >> says. But looking at platform_get_irq_optional(), which is called by >> platform_get_irq(), the out label is: >> >> WARN(ret == 0, "0 is an invalid IRQ number\n"); >> return ret; >> >> So 0 will be returned as-is. That is rather weird. That should be fixed to >> return -ENXIO: >> >> if (WARN(ret == 0, "0 is an invalid IRQ number\n")) >> return -ENXIO; -ENXIO seems to me more fitting indeed (than -EINVAL that I used). > > No, this is wrong for the same reasons I explained to Sergey. I fail to understand you, sorry. We're going in circles, it seems... :-/ > The problem is that this is _optional API and it has been misdesigned. > Replacing things like above will increase the mess. What's wrong with replacing IRQ0 with -ENXIO now? platform_get_irq_optional() (as in your patch) could then happily return 0 ISO -ENXIO. Contrarywise, if we don't replace IRQ0 with -ENXIO, platform_get_irq_optional() will return 0 for both IRQ0 and missing IRQ! Am I clear enough? If you don't understand me now, I don't know what to say... :-/ > >> return ret; >> >> Otherwise, I do not think that removing the "if (!irq)" hunk is safe. no ? > > No. This is not a business of the caller to workaround implementation > details (bugs) of the core APIs. > If something goes wrong, then it's platform_get_irq() to blame, and > not the libahci_platform. I'm repeating myself already: we don't work around the bug in platform_get_irq(), we're working around the driver subsystems that treat 0 specially (and so don't support IRQ0); libata treats 0 as an indication of the polling mode (moreover, it will curse if you pass to it both IRQ == 0 and a pointer to an interrupt handler! Am I clear enough this time? :-) MBR, Sergey ^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH v1 1/2] ata: libahci_platform: Get rid of dup message when IRQ can't be retrieved 2021-12-10 16:38 ` Sergey Shtylyov @ 2021-12-10 17:57 ` Andy Shevchenko 0 siblings, 0 replies; 44+ messages in thread From: Andy Shevchenko @ 2021-12-10 17:57 UTC (permalink / raw) To: Sergey Shtylyov Cc: Damien Le Moal, linux-ide, Linux Kernel Mailing List, Hans de Goede, Jens Axboe On Fri, Dec 10, 2021 at 07:38:40PM +0300, Sergey Shtylyov wrote: > On 12/10/21 11:47 AM, Andy Shevchenko wrote: > > >>> platform_get_irq() will print a message when it fails. > >>> No need to repeat this. > >>> > >>> While at it, drop redundant check for 0 as platform_get_irq() spills > >>> out a big WARN() in such case. > >> > >> The reason you should be able to remove the "if (!irq)" test is that > >> platform_get_irq() never returns 0. At least, that is what the function kdoc > >> says. But looking at platform_get_irq_optional(), which is called by > >> platform_get_irq(), the out label is: > >> > >> WARN(ret == 0, "0 is an invalid IRQ number\n"); > >> return ret; > >> > >> So 0 will be returned as-is. That is rather weird. That should be fixed to > >> return -ENXIO: > >> > >> if (WARN(ret == 0, "0 is an invalid IRQ number\n")) > >> return -ENXIO; > > -ENXIO seems to me more fitting indeed (than -EINVAL that I used). > > > > > No, this is wrong for the same reasons I explained to Sergey. > > I fail to understand you, sorry. We're going in circles, it seems... :-/ platform_get_irq_optional() is supposed to return 0 when there is no IRQ found, but everything else went alright. I'm tired to waste my time to go circles. Again, the problem is that platform_get_irq_optional() has wrong set of output values. And your patch doesn't fix that. And it has nothing to do with my code here. > > The problem is that this is _optional API and it has been misdesigned. > > Replacing things like above will increase the mess. > > What's wrong with replacing IRQ0 with -ENXIO now? platform_get_irq_optional() > (as in your patch) could then happily return 0 ISO -ENXIO. Contrarywise, if we don't > replace IRQ0 with -ENXIO, platform_get_irq_optional() will return 0 for both IRQ0 > and missing IRQ! Am I clear enough? If you don't understand me now, I don't know what > to say... :-/ See above. Read my messages again, please. I'm really tired to explain again and again the same. TL;DR: You simply try to "fix" in a correct place but in a wrong way. > >> return ret; > >> > >> Otherwise, I do not think that removing the "if (!irq)" hunk is safe. no ? > > > > No. This is not a business of the caller to workaround implementation > > details (bugs) of the core APIs. > > If something goes wrong, then it's platform_get_irq() to blame, and > > not the libahci_platform. > > I'm repeating myself already: we don't work around the bug in platform_get_irq(), Yes, you do. > we're working around the driver subsystems that treat 0 specially (and so don't > support IRQ0); libata treats 0 as an indication of the polling mode (moreover, > it will curse if you pass to it both IRQ == 0 and a pointer to an interrupt handler! > Am I clear enough this time? :-) Yes, and it doesn't contradict to what my patch does. Read comment against platform_get_irq(). If it returns 0, it's not a business of the callers to work around it. Am I clear enough this time? :-) -- With Best Regards, Andy Shevchenko ^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH v1 1/2] ata: libahci_platform: Get rid of dup message when IRQ can't be retrieved 2021-12-09 22:49 ` Damien Le Moal 2021-12-09 22:57 ` Damien Le Moal 2021-12-10 8:47 ` Andy Shevchenko @ 2021-12-10 8:59 ` Sergey Shtylyov 2021-12-10 10:46 ` Andy Shevchenko 2021-12-10 23:45 ` Damien Le Moal 2 siblings, 2 replies; 44+ messages in thread From: Sergey Shtylyov @ 2021-12-10 8:59 UTC (permalink / raw) To: Damien Le Moal, Andy Shevchenko, linux-ide, linux-kernel Cc: Hans de Goede, Jens Axboe On 12/10/21 1:49 AM, Damien Le Moal wrote: >> platform_get_irq() will print a message when it fails. >> No need to repeat this. >> >> While at it, drop redundant check for 0 as platform_get_irq() spills >> out a big WARN() in such case. > > The reason you should be able to remove the "if (!irq)" test is that > platform_get_irq() never returns 0. At least, that is what the function kdoc > says. But looking at platform_get_irq_optional(), which is called by > platform_get_irq(), the out label is: > > WARN(ret == 0, "0 is an invalid IRQ number\n"); > return ret; > > So 0 will be returned as-is. That is rather weird. That should be fixed to > return -ENXIO: > > if (WARN(ret == 0, "0 is an invalid IRQ number\n")) > return -ENXIO; > return ret; My unmerged patch (https://marc.info/?l=linux-kernel&m=163623041902285) does this but returns -EINVAL instead. > Otherwise, I do not think that removing the "if (!irq)" hunk is safe. no ? Of course it isn't... >> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> [...] MBR, Sergey ^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH v1 1/2] ata: libahci_platform: Get rid of dup message when IRQ can't be retrieved 2021-12-10 8:59 ` Sergey Shtylyov @ 2021-12-10 10:46 ` Andy Shevchenko 2021-12-10 11:19 ` Sergey Shtylyov 2021-12-10 23:45 ` Damien Le Moal 1 sibling, 1 reply; 44+ messages in thread From: Andy Shevchenko @ 2021-12-10 10:46 UTC (permalink / raw) To: Sergey Shtylyov Cc: Damien Le Moal, linux-ide, linux-kernel, Hans de Goede, Jens Axboe On Fri, Dec 10, 2021 at 11:59:00AM +0300, Sergey Shtylyov wrote: > On 12/10/21 1:49 AM, Damien Le Moal wrote: > > >> platform_get_irq() will print a message when it fails. > >> No need to repeat this. > >> > >> While at it, drop redundant check for 0 as platform_get_irq() spills > >> out a big WARN() in such case. > > > > The reason you should be able to remove the "if (!irq)" test is that > > platform_get_irq() never returns 0. At least, that is what the function kdoc > > says. But looking at platform_get_irq_optional(), which is called by > > platform_get_irq(), the out label is: > > > > WARN(ret == 0, "0 is an invalid IRQ number\n"); > > return ret; > > > > So 0 will be returned as-is. That is rather weird. That should be fixed to > > return -ENXIO: > > > > if (WARN(ret == 0, "0 is an invalid IRQ number\n")) > > return -ENXIO; > > return ret; > > My unmerged patch (https://marc.info/?l=linux-kernel&m=163623041902285) does this > but returns -EINVAL instead. > > > Otherwise, I do not think that removing the "if (!irq)" hunk is safe. no ? > > Of course it isn't... It's unsubstantiated statement. The vIRQ 0 shouldn't be returned by any of those API calls. If it is the case, go and fix them, no need to workaround in each of the callers. -- With Best Regards, Andy Shevchenko ^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH v1 1/2] ata: libahci_platform: Get rid of dup message when IRQ can't be retrieved 2021-12-10 10:46 ` Andy Shevchenko @ 2021-12-10 11:19 ` Sergey Shtylyov 2021-12-10 11:36 ` Andy Shevchenko 0 siblings, 1 reply; 44+ messages in thread From: Sergey Shtylyov @ 2021-12-10 11:19 UTC (permalink / raw) To: Andy Shevchenko Cc: Damien Le Moal, linux-ide, linux-kernel, Hans de Goede, Jens Axboe On 12/10/21 1:46 PM, Andy Shevchenko wrote: >>>> platform_get_irq() will print a message when it fails. >>>> No need to repeat this. >>>> >>>> While at it, drop redundant check for 0 as platform_get_irq() spills >>>> out a big WARN() in such case. >>> >>> The reason you should be able to remove the "if (!irq)" test is that >>> platform_get_irq() never returns 0. At least, that is what the function kdoc >>> says. But looking at platform_get_irq_optional(), which is called by >>> platform_get_irq(), the out label is: >>> >>> WARN(ret == 0, "0 is an invalid IRQ number\n"); >>> return ret; >>> >>> So 0 will be returned as-is. That is rather weird. That should be fixed to >>> return -ENXIO: >>> >>> if (WARN(ret == 0, "0 is an invalid IRQ number\n")) >>> return -ENXIO; >>> return ret; >> >> My unmerged patch (https://marc.info/?l=linux-kernel&m=163623041902285) does this >> but returns -EINVAL instead. >> >>> Otherwise, I do not think that removing the "if (!irq)" hunk is safe. no ? >> >> Of course it isn't... > > It's unsubstantiated statement. The vIRQ 0 shouldn't be returned by any of > those API calls. We do _not_ know what needs to be fixed, that's the problem, and that's why the WARN() is there... > If it is the case, go and fix them, no need to workaround > in each of the callers. There's a need to work around as long as IRQ0 ican be returned, otherwise we get partly functioning or non-functioning drivers... MBR, Sergey ^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH v1 1/2] ata: libahci_platform: Get rid of dup message when IRQ can't be retrieved 2021-12-10 11:19 ` Sergey Shtylyov @ 2021-12-10 11:36 ` Andy Shevchenko 2021-12-10 17:15 ` Sergei Shtylyov 0 siblings, 1 reply; 44+ messages in thread From: Andy Shevchenko @ 2021-12-10 11:36 UTC (permalink / raw) To: Sergey Shtylyov Cc: Damien Le Moal, linux-ide, linux-kernel, Hans de Goede, Jens Axboe On Fri, Dec 10, 2021 at 02:19:52PM +0300, Sergey Shtylyov wrote: > On 12/10/21 1:46 PM, Andy Shevchenko wrote: > > >>>> platform_get_irq() will print a message when it fails. > >>>> No need to repeat this. > >>>> > >>>> While at it, drop redundant check for 0 as platform_get_irq() spills > >>>> out a big WARN() in such case. > >>> > >>> The reason you should be able to remove the "if (!irq)" test is that > >>> platform_get_irq() never returns 0. At least, that is what the function kdoc > >>> says. But looking at platform_get_irq_optional(), which is called by > >>> platform_get_irq(), the out label is: > >>> > >>> WARN(ret == 0, "0 is an invalid IRQ number\n"); > >>> return ret; > >>> > >>> So 0 will be returned as-is. That is rather weird. That should be fixed to > >>> return -ENXIO: > >>> > >>> if (WARN(ret == 0, "0 is an invalid IRQ number\n")) > >>> return -ENXIO; > >>> return ret; > >> > >> My unmerged patch (https://marc.info/?l=linux-kernel&m=163623041902285) does this > >> but returns -EINVAL instead. > >> > >>> Otherwise, I do not think that removing the "if (!irq)" hunk is safe. no ? > >> > >> Of course it isn't... > > > > It's unsubstantiated statement. The vIRQ 0 shouldn't be returned by any of > > those API calls. > > We do _not_ know what needs to be fixed, that's the problem, and that's why the WARN() > is there... So, have you seen this warning (being reported) related to libahci_platform? If no, what we are discussing about then? The workaround is redundant and no need to have a dead code in the driver, really. > > If it is the case, go and fix them, no need to workaround > > in each of the callers. > > There's a need to work around as long as IRQ0 ican be returned, otherwise > we get partly functioning or non-functioning drivers... You get them unfunctioning anyways and you get the big WARN() even before this patch. -- With Best Regards, Andy Shevchenko ^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH v1 1/2] ata: libahci_platform: Get rid of dup message when IRQ can't be retrieved 2021-12-10 11:36 ` Andy Shevchenko @ 2021-12-10 17:15 ` Sergei Shtylyov 2021-12-10 17:59 ` Andy Shevchenko 0 siblings, 1 reply; 44+ messages in thread From: Sergei Shtylyov @ 2021-12-10 17:15 UTC (permalink / raw) To: Andy Shevchenko, Sergey Shtylyov Cc: Damien Le Moal, linux-ide, linux-kernel, Hans de Goede, Jens Axboe On 12/10/21 2:36 PM, Andy Shevchenko wrote: >>>>>> platform_get_irq() will print a message when it fails. >>>>>> No need to repeat this. >>>>>> >>>>>> While at it, drop redundant check for 0 as platform_get_irq() spills >>>>>> out a big WARN() in such case. >>>>> >>>>> The reason you should be able to remove the "if (!irq)" test is that >>>>> platform_get_irq() never returns 0. At least, that is what the function kdoc >>>>> says. But looking at platform_get_irq_optional(), which is called by >>>>> platform_get_irq(), the out label is: >>>>> >>>>> WARN(ret == 0, "0 is an invalid IRQ number\n"); >>>>> return ret; >>>>> >>>>> So 0 will be returned as-is. That is rather weird. That should be fixed to >>>>> return -ENXIO: >>>>> >>>>> if (WARN(ret == 0, "0 is an invalid IRQ number\n")) >>>>> return -ENXIO; >>>>> return ret; >>>> >>>> My unmerged patch (https://marc.info/?l=linux-kernel&m=163623041902285) does this >>>> but returns -EINVAL instead. >>>> >>>>> Otherwise, I do not think that removing the "if (!irq)" hunk is safe. no ? >>>> >>>> Of course it isn't... >>> >>> It's unsubstantiated statement. The vIRQ 0 shouldn't be returned by any of >>> those API calls. >> >> We do _not_ know what needs to be fixed, that's the problem, and that's why the WARN() >> is there... > > So, have you seen this warning (being reported) related to libahci_platform? No (as if you need to really see this while it's obvious from the code review). > If no, what we are discussing about then? The workaround is redundant and I don't know. :-) Your arguments so far seem bogus (sorry! :-))... > no need to have a dead code in the driver, really. "Jazz isn't dead, it just smells funny". :-) >>> If it is the case, go and fix them, no need to workaround >>> in each of the callers. >> >> There's a need to work around as long as IRQ0 ican be returned, otherwise >> we get partly functioning or non-functioning drivers... > > You get them unfunctioning anyways The drivers would be broken in not quite obvious ways. With IRQ0 check, they just don't probe anymore. See the explanation of the IRQ0 check (in the drivers) in my previous mail... > and you get the big WARN() even before this patch. As if that was enough... The IRQ0 problem exists for at least 15 (if not 20) years... MBR, Sergey ^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH v1 1/2] ata: libahci_platform: Get rid of dup message when IRQ can't be retrieved 2021-12-10 17:15 ` Sergei Shtylyov @ 2021-12-10 17:59 ` Andy Shevchenko 2021-12-10 19:01 ` Sergey Shtylyov 0 siblings, 1 reply; 44+ messages in thread From: Andy Shevchenko @ 2021-12-10 17:59 UTC (permalink / raw) To: Sergei Shtylyov Cc: Sergey Shtylyov, Damien Le Moal, linux-ide, linux-kernel, Hans de Goede, Jens Axboe On Fri, Dec 10, 2021 at 08:15:43PM +0300, Sergei Shtylyov wrote: > On 12/10/21 2:36 PM, Andy Shevchenko wrote: > > >>>>>> platform_get_irq() will print a message when it fails. > >>>>>> No need to repeat this. > >>>>>> > >>>>>> While at it, drop redundant check for 0 as platform_get_irq() spills > >>>>>> out a big WARN() in such case. > >>>>> > >>>>> The reason you should be able to remove the "if (!irq)" test is that > >>>>> platform_get_irq() never returns 0. At least, that is what the function kdoc > >>>>> says. But looking at platform_get_irq_optional(), which is called by > >>>>> platform_get_irq(), the out label is: > >>>>> > >>>>> WARN(ret == 0, "0 is an invalid IRQ number\n"); > >>>>> return ret; > >>>>> > >>>>> So 0 will be returned as-is. That is rather weird. That should be fixed to > >>>>> return -ENXIO: > >>>>> > >>>>> if (WARN(ret == 0, "0 is an invalid IRQ number\n")) > >>>>> return -ENXIO; > >>>>> return ret; > >>>> > >>>> My unmerged patch (https://marc.info/?l=linux-kernel&m=163623041902285) does this > >>>> but returns -EINVAL instead. > >>>> > >>>>> Otherwise, I do not think that removing the "if (!irq)" hunk is safe. no ? > >>>> > >>>> Of course it isn't... > >>> > >>> It's unsubstantiated statement. The vIRQ 0 shouldn't be returned by any of > >>> those API calls. > >> > >> We do _not_ know what needs to be fixed, that's the problem, and that's why the WARN() > >> is there... > > > > So, have you seen this warning (being reported) related to libahci_platform? > > No (as if you need to really see this while it's obvious from the code review). > > > If no, what we are discussing about then? The workaround is redundant and > > I don't know. :-) Your arguments so far seem bogus (sorry! :-))... It seems you haven't got them at all. The problems of platform_get_irq() et al shouldn't be worked around in the callers. > > no need to have a dead code in the driver, really. > > "Jazz isn't dead, it just smells funny". :-) > > >>> If it is the case, go and fix them, no need to workaround > >>> in each of the callers. > >> > >> There's a need to work around as long as IRQ0 ican be returned, otherwise > >> we get partly functioning or non-functioning drivers... > > > > You get them unfunctioning anyways > > The drivers would be broken in not quite obvious ways. With IRQ0 check, they just > don't probe anymore. See the explanation of the IRQ0 check (in the drivers) in my > previous mail... > > > and you get the big WARN() even before this patch. > > As if that was enough... > The IRQ0 problem exists for at least 15 (if not 20) years... -- With Best Regards, Andy Shevchenko ^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH v1 1/2] ata: libahci_platform: Get rid of dup message when IRQ can't be retrieved 2021-12-10 17:59 ` Andy Shevchenko @ 2021-12-10 19:01 ` Sergey Shtylyov 2021-12-10 19:25 ` Andy Shevchenko 0 siblings, 1 reply; 44+ messages in thread From: Sergey Shtylyov @ 2021-12-10 19:01 UTC (permalink / raw) To: Andy Shevchenko, Sergei Shtylyov Cc: Damien Le Moal, linux-ide, linux-kernel, Hans de Goede, Jens Axboe On 12/10/21 8:59 PM, Andy Shevchenko wrote: >>>>>>>> platform_get_irq() will print a message when it fails. >>>>>>>> No need to repeat this. >>>>>>>> >>>>>>>> While at it, drop redundant check for 0 as platform_get_irq() spills >>>>>>>> out a big WARN() in such case. >>>>>>> >>>>>>> The reason you should be able to remove the "if (!irq)" test is that >>>>>>> platform_get_irq() never returns 0. At least, that is what the function kdoc >>>>>>> says. But looking at platform_get_irq_optional(), which is called by >>>>>>> platform_get_irq(), the out label is: >>>>>>> >>>>>>> WARN(ret == 0, "0 is an invalid IRQ number\n"); >>>>>>> return ret; >>>>>>> >>>>>>> So 0 will be returned as-is. That is rather weird. That should be fixed to >>>>>>> return -ENXIO: >>>>>>> >>>>>>> if (WARN(ret == 0, "0 is an invalid IRQ number\n")) >>>>>>> return -ENXIO; >>>>>>> return ret; >>>>>> >>>>>> My unmerged patch (https://marc.info/?l=linux-kernel&m=163623041902285) does this >>>>>> but returns -EINVAL instead. >>>>>> >>>>>>> Otherwise, I do not think that removing the "if (!irq)" hunk is safe. no ? >>>>>> >>>>>> Of course it isn't... >>>>> >>>>> It's unsubstantiated statement. The vIRQ 0 shouldn't be returned by any of >>>>> those API calls. >>>> >>>> We do _not_ know what needs to be fixed, that's the problem, and that's why the WARN() >>>> is there... >>> >>> So, have you seen this warning (being reported) related to libahci_platform? >> >> No (as if you need to really see this while it's obvious from the code review). >> >>> If no, what we are discussing about then? The workaround is redundant and >> >> I don't know. :-) Your arguments so far seem bogus (sorry! :-))... > > It seems you haven't got them at all. The problems of platform_get_irq() et al > shouldn't be worked around in the callers. I have clearly explained to you what I'm working around there. If that wasn't clear enough, I don't want to continue this talk anymore. Good luck with your patch (not this one). [...] MBR, Sergey ^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH v1 1/2] ata: libahci_platform: Get rid of dup message when IRQ can't be retrieved 2021-12-10 19:01 ` Sergey Shtylyov @ 2021-12-10 19:25 ` Andy Shevchenko 2021-12-10 19:30 ` Sergey Shtylyov 0 siblings, 1 reply; 44+ messages in thread From: Andy Shevchenko @ 2021-12-10 19:25 UTC (permalink / raw) To: Sergey Shtylyov Cc: Sergei Shtylyov, Damien Le Moal, linux-ide, linux-kernel, Hans de Goede, Jens Axboe On Fri, Dec 10, 2021 at 10:01:04PM +0300, Sergey Shtylyov wrote: > On 12/10/21 8:59 PM, Andy Shevchenko wrote: > > >>>>>>>> platform_get_irq() will print a message when it fails. > >>>>>>>> No need to repeat this. > >>>>>>>> > >>>>>>>> While at it, drop redundant check for 0 as platform_get_irq() spills > >>>>>>>> out a big WARN() in such case. > >>>>>>> > >>>>>>> The reason you should be able to remove the "if (!irq)" test is that > >>>>>>> platform_get_irq() never returns 0. At least, that is what the function kdoc > >>>>>>> says. But looking at platform_get_irq_optional(), which is called by > >>>>>>> platform_get_irq(), the out label is: > >>>>>>> > >>>>>>> WARN(ret == 0, "0 is an invalid IRQ number\n"); > >>>>>>> return ret; > >>>>>>> > >>>>>>> So 0 will be returned as-is. That is rather weird. That should be fixed to > >>>>>>> return -ENXIO: > >>>>>>> > >>>>>>> if (WARN(ret == 0, "0 is an invalid IRQ number\n")) > >>>>>>> return -ENXIO; > >>>>>>> return ret; > >>>>>> > >>>>>> My unmerged patch (https://marc.info/?l=linux-kernel&m=163623041902285) does this > >>>>>> but returns -EINVAL instead. > >>>>>> > >>>>>>> Otherwise, I do not think that removing the "if (!irq)" hunk is safe. no ? > >>>>>> > >>>>>> Of course it isn't... > >>>>> > >>>>> It's unsubstantiated statement. The vIRQ 0 shouldn't be returned by any of > >>>>> those API calls. > >>>> > >>>> We do _not_ know what needs to be fixed, that's the problem, and that's why the WARN() > >>>> is there... > >>> > >>> So, have you seen this warning (being reported) related to libahci_platform? > >> > >> No (as if you need to really see this while it's obvious from the code review). > >> > >>> If no, what we are discussing about then? The workaround is redundant and > >> > >> I don't know. :-) Your arguments so far seem bogus (sorry! :-))... > > > > It seems you haven't got them at all. The problems of platform_get_irq() et al > > shouldn't be worked around in the callers. > > I have clearly explained to you what I'm working around there. If that wasn't clear > enough, I don't want to continue this talk anymore. Good luck with your patch (not this > one). Good luck with yours, not the one that touches platform_get_irq_optional() though! -- With Best Regards, Andy Shevchenko ^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH v1 1/2] ata: libahci_platform: Get rid of dup message when IRQ can't be retrieved 2021-12-10 19:25 ` Andy Shevchenko @ 2021-12-10 19:30 ` Sergey Shtylyov 2021-12-10 19:35 ` Sergei Shtylyov 0 siblings, 1 reply; 44+ messages in thread From: Sergey Shtylyov @ 2021-12-10 19:30 UTC (permalink / raw) To: Andy Shevchenko Cc: Sergei Shtylyov, Damien Le Moal, linux-ide, linux-kernel, Hans de Goede, Jens Axboe On 12/10/21 10:25 PM, Andy Shevchenko wrote: [...] >>>>>>>>>> platform_get_irq() will print a message when it fails. >>>>>>>>>> No need to repeat this. >>>>>>>>>> >>>>>>>>>> While at it, drop redundant check for 0 as platform_get_irq() spills >>>>>>>>>> out a big WARN() in such case. >>>>>>>>> >>>>>>>>> The reason you should be able to remove the "if (!irq)" test is that >>>>>>>>> platform_get_irq() never returns 0. At least, that is what the function kdoc >>>>>>>>> says. But looking at platform_get_irq_optional(), which is called by >>>>>>>>> platform_get_irq(), the out label is: >>>>>>>>> >>>>>>>>> WARN(ret == 0, "0 is an invalid IRQ number\n"); >>>>>>>>> return ret; >>>>>>>>> >>>>>>>>> So 0 will be returned as-is. That is rather weird. That should be fixed to >>>>>>>>> return -ENXIO: >>>>>>>>> >>>>>>>>> if (WARN(ret == 0, "0 is an invalid IRQ number\n")) >>>>>>>>> return -ENXIO; >>>>>>>>> return ret; >>>>>>>> >>>>>>>> My unmerged patch (https://marc.info/?l=linux-kernel&m=163623041902285) does this >>>>>>>> but returns -EINVAL instead. >>>>>>>> >>>>>>>>> Otherwise, I do not think that removing the "if (!irq)" hunk is safe. no ? >>>>>>>> >>>>>>>> Of course it isn't... >>>>>>> >>>>>>> It's unsubstantiated statement. The vIRQ 0 shouldn't be returned by any of >>>>>>> those API calls. >>>>>> >>>>>> We do _not_ know what needs to be fixed, that's the problem, and that's why the WARN() >>>>>> is there... >>>>> >>>>> So, have you seen this warning (being reported) related to libahci_platform? >>>> >>>> No (as if you need to really see this while it's obvious from the code review). >>>> >>>>> If no, what we are discussing about then? The workaround is redundant and >>>> >>>> I don't know. :-) Your arguments so far seem bogus (sorry! :-))... >>> >>> It seems you haven't got them at all. The problems of platform_get_irq() et al >>> shouldn't be worked around in the callers. >> >> I have clearly explained to you what I'm working around there. If that wasn't clear >> enough, I don't want to continue this talk anymore. Good luck with your patch (not this >> one). > > Good luck with yours, not the one that touches platform_get_irq_optional() though! Mmh, I'm not touching it any way that would break what your patch was trying to do, unless you've re-thopught that. It also shoudn't matter whose patch gets merged 1st other than some small adaptation). MBR, Sergey ^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH v1 1/2] ata: libahci_platform: Get rid of dup message when IRQ can't be retrieved 2021-12-10 19:30 ` Sergey Shtylyov @ 2021-12-10 19:35 ` Sergei Shtylyov 2021-12-11 10:13 ` Sergey Shtylyov 0 siblings, 1 reply; 44+ messages in thread From: Sergei Shtylyov @ 2021-12-10 19:35 UTC (permalink / raw) To: Sergey Shtylyov, Andy Shevchenko Cc: Damien Le Moal, linux-ide, linux-kernel, Hans de Goede, Jens Axboe On 12/10/21 10:30 PM, Sergey Shtylyov wrote: [...] >>>>>>>>>>> platform_get_irq() will print a message when it fails. >>>>>>>>>>> No need to repeat this. >>>>>>>>>>> >>>>>>>>>>> While at it, drop redundant check for 0 as platform_get_irq() spills >>>>>>>>>>> out a big WARN() in such case. >>>>>>>>>> >>>>>>>>>> The reason you should be able to remove the "if (!irq)" test is that >>>>>>>>>> platform_get_irq() never returns 0. At least, that is what the function kdoc >>>>>>>>>> says. But looking at platform_get_irq_optional(), which is called by >>>>>>>>>> platform_get_irq(), the out label is: >>>>>>>>>> >>>>>>>>>> WARN(ret == 0, "0 is an invalid IRQ number\n"); >>>>>>>>>> return ret; >>>>>>>>>> >>>>>>>>>> So 0 will be returned as-is. That is rather weird. That should be fixed to >>>>>>>>>> return -ENXIO: >>>>>>>>>> >>>>>>>>>> if (WARN(ret == 0, "0 is an invalid IRQ number\n")) >>>>>>>>>> return -ENXIO; >>>>>>>>>> return ret; >>>>>>>>> >>>>>>>>> My unmerged patch (https://marc.info/?l=linux-kernel&m=163623041902285) does this >>>>>>>>> but returns -EINVAL instead. >>>>>>>>> >>>>>>>>>> Otherwise, I do not think that removing the "if (!irq)" hunk is safe. no ? >>>>>>>>> >>>>>>>>> Of course it isn't... >>>>>>>> >>>>>>>> It's unsubstantiated statement. The vIRQ 0 shouldn't be returned by any of >>>>>>>> those API calls. >>>>>>> >>>>>>> We do _not_ know what needs to be fixed, that's the problem, and that's why the WARN() >>>>>>> is there... >>>>>> >>>>>> So, have you seen this warning (being reported) related to libahci_platform? >>>>> >>>>> No (as if you need to really see this while it's obvious from the code review). >>>>> >>>>>> If no, what we are discussing about then? The workaround is redundant and >>>>> >>>>> I don't know. :-) Your arguments so far seem bogus (sorry! :-))... >>>> >>>> It seems you haven't got them at all. The problems of platform_get_irq() et al >>>> shouldn't be worked around in the callers. >>> >>> I have clearly explained to you what I'm working around there. If that wasn't clear >>> enough, I don't want to continue this talk anymore. Good luck with your patch (not this >>> one). >> >> Good luck with yours, not the one that touches platform_get_irq_optional() though! > > Mmh, I'm not touching it any way that would break what your patch was trying to do, > unless you've re-thopught that. It also shoudn't matter whose patch gets merged 1st > other than some small adaptation). BTW, looking at [1], this comment is wrong: + * Return: non-zero IRQ number on success, negative error number on failure. It doesn't mention 0 which you return from this function. [1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=ed7027fdf4ec41ed6df6814956dc11860232a9d5 MBR, Sergey ^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH v1 1/2] ata: libahci_platform: Get rid of dup message when IRQ can't be retrieved 2021-12-10 19:35 ` Sergei Shtylyov @ 2021-12-11 10:13 ` Sergey Shtylyov 2021-12-13 11:26 ` Andy Shevchenko 0 siblings, 1 reply; 44+ messages in thread From: Sergey Shtylyov @ 2021-12-11 10:13 UTC (permalink / raw) To: Sergey Shtylyov, Andy Shevchenko Cc: Damien Le Moal, linux-ide, linux-kernel, Hans de Goede, Jens Axboe On 10.12.2021 22:35, Sergei Shtylyov wrote: [...] >>>>>>>>>>>> platform_get_irq() will print a message when it fails. >>>>>>>>>>>> No need to repeat this. >>>>>>>>>>>> >>>>>>>>>>>> While at it, drop redundant check for 0 as platform_get_irq() spills >>>>>>>>>>>> out a big WARN() in such case. >>>>>>>>>>> >>>>>>>>>>> The reason you should be able to remove the "if (!irq)" test is that >>>>>>>>>>> platform_get_irq() never returns 0. At least, that is what the function kdoc >>>>>>>>>>> says. But looking at platform_get_irq_optional(), which is called by >>>>>>>>>>> platform_get_irq(), the out label is: >>>>>>>>>>> >>>>>>>>>>> WARN(ret == 0, "0 is an invalid IRQ number\n"); >>>>>>>>>>> return ret; >>>>>>>>>>> >>>>>>>>>>> So 0 will be returned as-is. That is rather weird. That should be fixed to >>>>>>>>>>> return -ENXIO: >>>>>>>>>>> >>>>>>>>>>> if (WARN(ret == 0, "0 is an invalid IRQ number\n")) >>>>>>>>>>> return -ENXIO; >>>>>>>>>>> return ret; >>>>>>>>>> >>>>>>>>>> My unmerged patch (https://marc.info/?l=linux-kernel&m=163623041902285) does this >>>>>>>>>> but returns -EINVAL instead. >>>>>>>>>> >>>>>>>>>>> Otherwise, I do not think that removing the "if (!irq)" hunk is safe. no ? >>>>>>>>>> >>>>>>>>>> Of course it isn't... >>>>>>>>> >>>>>>>>> It's unsubstantiated statement. The vIRQ 0 shouldn't be returned by any of >>>>>>>>> those API calls. >>>>>>>> >>>>>>>> We do _not_ know what needs to be fixed, that's the problem, and that's why the WARN() >>>>>>>> is there... >>>>>>> >>>>>>> So, have you seen this warning (being reported) related to libahci_platform? >>>>>> >>>>>> No (as if you need to really see this while it's obvious from the code review). >>>>>> >>>>>>> If no, what we are discussing about then? The workaround is redundant and >>>>>> >>>>>> I don't know. :-) Your arguments so far seem bogus (sorry! :-))... >>>>> >>>>> It seems you haven't got them at all. The problems of platform_get_irq() et al >>>>> shouldn't be worked around in the callers. >>>> >>>> I have clearly explained to you what I'm working around there. If that wasn't clear >>>> enough, I don't want to continue this talk anymore. Good luck with your patch (not this >>>> one). >>> >>> Good luck with yours, not the one that touches platform_get_irq_optional() though! >> >> Mmh, I'm not touching it any way that would break what your patch was trying to do, >> unless you've re-thopught that. It also shoudn't matter whose patch gets merged 1st >> other than some small adaptation). > > BTW, looking at [1], this comment is wrong: > > + * Return: non-zero IRQ number on success, negative error number on failure. > > It doesn't mention 0 which you return from this function. Also, your commit log is wrong in the description of how to handle the result: << Now: ret = platform_get_irq_optional(...); if (ret != -ENXIO) return ret; // respect deferred probe if (ret > 0) ...we get an IRQ... >> The (ret != -ENXIO) check also succeeds on the (positive) IRQ #s, so the following code becomes unreachable. :-/ > [1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=ed7027fdf4ec41ed6df6814956dc11860232a9d5 MBR, Sergey ^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH v1 1/2] ata: libahci_platform: Get rid of dup message when IRQ can't be retrieved 2021-12-11 10:13 ` Sergey Shtylyov @ 2021-12-13 11:26 ` Andy Shevchenko 0 siblings, 0 replies; 44+ messages in thread From: Andy Shevchenko @ 2021-12-13 11:26 UTC (permalink / raw) To: Sergey Shtylyov Cc: Damien Le Moal, linux-ide, linux-kernel, Hans de Goede, Jens Axboe On Sat, Dec 11, 2021 at 01:13:52PM +0300, Sergey Shtylyov wrote: > On 10.12.2021 22:35, Sergei Shtylyov wrote: ... > Also, your commit log is wrong in the description of how to handle the result: > > << > Now: > ret = platform_get_irq_optional(...); > if (ret != -ENXIO) > return ret; // respect deferred probe > if (ret > 0) > ...we get an IRQ... > >> > > The (ret != -ENXIO) check also succeeds on the (positive) IRQ #s, so the > following code becomes unreachable. :-/ Indeed, thanks! Should be if (ret < 0 && ret != -ENXIO) -- With Best Regards, Andy Shevchenko ^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH v1 1/2] ata: libahci_platform: Get rid of dup message when IRQ can't be retrieved 2021-12-10 8:59 ` Sergey Shtylyov 2021-12-10 10:46 ` Andy Shevchenko @ 2021-12-10 23:45 ` Damien Le Moal 2021-12-11 10:25 ` Sergey Shtylyov 2021-12-13 11:46 ` Andy Shevchenko 1 sibling, 2 replies; 44+ messages in thread From: Damien Le Moal @ 2021-12-10 23:45 UTC (permalink / raw) To: Sergey Shtylyov, Andy Shevchenko, linux-ide, linux-kernel Cc: Hans de Goede, Jens Axboe On 2021/12/10 17:59, Sergey Shtylyov wrote: > On 12/10/21 1:49 AM, Damien Le Moal wrote: > >>> platform_get_irq() will print a message when it fails. >>> No need to repeat this. >>> >>> While at it, drop redundant check for 0 as platform_get_irq() spills >>> out a big WARN() in such case. >> >> The reason you should be able to remove the "if (!irq)" test is that >> platform_get_irq() never returns 0. At least, that is what the function kdoc >> says. But looking at platform_get_irq_optional(), which is called by >> platform_get_irq(), the out label is: >> >> WARN(ret == 0, "0 is an invalid IRQ number\n"); >> return ret; >> >> So 0 will be returned as-is. That is rather weird. That should be fixed to >> return -ENXIO: >> >> if (WARN(ret == 0, "0 is an invalid IRQ number\n")) >> return -ENXIO; >> return ret; > > My unmerged patch (https://marc.info/?l=linux-kernel&m=163623041902285) does this > but returns -EINVAL instead. Thinking more about this, shouldn't this change go into platform_get_irq() instead of platform_get_irq_optional() ? The way I see it, I think that the intended behavior for platform_get_irq_optional() is: 1) If have IRQ, return it, always > 0 2) If no IRQ, return 0 3) If error, return < 0 no ? And for platform_get_irq(), case (2) becomes an error. Is this the intended semantic ? I am really not sure here as the functions kdoc description and the code do not match. Which one is correct ? > >> Otherwise, I do not think that removing the "if (!irq)" hunk is safe. no ? > > Of course it isn't... > >>> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > [...] > > MBR, Sergey -- Damien Le Moal Western Digital Research ^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH v1 1/2] ata: libahci_platform: Get rid of dup message when IRQ can't be retrieved 2021-12-10 23:45 ` Damien Le Moal @ 2021-12-11 10:25 ` Sergey Shtylyov 2021-12-12 22:39 ` Damien Le Moal 2021-12-13 11:49 ` Andy Shevchenko 2021-12-13 11:46 ` Andy Shevchenko 1 sibling, 2 replies; 44+ messages in thread From: Sergey Shtylyov @ 2021-12-11 10:25 UTC (permalink / raw) To: Damien Le Moal, Sergey Shtylyov, Andy Shevchenko, linux-ide, linux-kernel Cc: Hans de Goede, Jens Axboe Hello! On 11.12.2021 2:45, Damien Le Moal wrote: [...] >>>> platform_get_irq() will print a message when it fails. >>>> No need to repeat this. >>>> >>>> While at it, drop redundant check for 0 as platform_get_irq() spills >>>> out a big WARN() in such case. >>> >>> The reason you should be able to remove the "if (!irq)" test is that >>> platform_get_irq() never returns 0. At least, that is what the function kdoc >>> says. But looking at platform_get_irq_optional(), which is called by >>> platform_get_irq(), the out label is: >>> >>> WARN(ret == 0, "0 is an invalid IRQ number\n"); >>> return ret; >>> >>> So 0 will be returned as-is. That is rather weird. That should be fixed to >>> return -ENXIO: >>> >>> if (WARN(ret == 0, "0 is an invalid IRQ number\n")) >>> return -ENXIO; >>> return ret; >> >> My unmerged patch (https://marc.info/?l=linux-kernel&m=163623041902285) does this >> but returns -EINVAL instead. > > Thinking more about this, shouldn't this change go into platform_get_irq() > instead of platform_get_irq_optional() ? Why? platform_get_irq() currently just calls platform_get_irq_optional()... > The way I see it, I think that the intended behavior for > platform_get_irq_optional() is: > 1) If have IRQ, return it, always > 0 > 2) If no IRQ, return 0 That does include the IRQ0 case, right? > 3) If error, return < 0 > no ? I completely agree, I (after thinking a bit) have no issues with that... > And for platform_get_irq(), case (2) becomes an error. > Is this the intended semantic ? I don't see how it's different from the current behavior. But we can do that as well, I just don't see whether it's really better... > I am really not sure here as the functions kdoc description and the code do not > match. Which one is correct ? It seems both are wrong. :-) [...] MBR, Sergey ^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH v1 1/2] ata: libahci_platform: Get rid of dup message when IRQ can't be retrieved 2021-12-11 10:25 ` Sergey Shtylyov @ 2021-12-12 22:39 ` Damien Le Moal 2021-12-13 11:52 ` Andy Shevchenko 2021-12-13 11:49 ` Andy Shevchenko 1 sibling, 1 reply; 44+ messages in thread From: Damien Le Moal @ 2021-12-12 22:39 UTC (permalink / raw) To: Sergey Shtylyov, Andy Shevchenko, linux-ide, linux-kernel Cc: Hans de Goede, Jens Axboe On 2021/12/11 19:25, Sergey Shtylyov wrote: > Hello! > > On 11.12.2021 2:45, Damien Le Moal wrote: > > [...] >>>>> platform_get_irq() will print a message when it fails. >>>>> No need to repeat this. >>>>> >>>>> While at it, drop redundant check for 0 as platform_get_irq() spills >>>>> out a big WARN() in such case. >>>> >>>> The reason you should be able to remove the "if (!irq)" test is that >>>> platform_get_irq() never returns 0. At least, that is what the function kdoc >>>> says. But looking at platform_get_irq_optional(), which is called by >>>> platform_get_irq(), the out label is: >>>> >>>> WARN(ret == 0, "0 is an invalid IRQ number\n"); >>>> return ret; >>>> >>>> So 0 will be returned as-is. That is rather weird. That should be fixed to >>>> return -ENXIO: >>>> >>>> if (WARN(ret == 0, "0 is an invalid IRQ number\n")) >>>> return -ENXIO; >>>> return ret; >>> >>> My unmerged patch (https://marc.info/?l=linux-kernel&m=163623041902285) does this >>> but returns -EINVAL instead. >> >> Thinking more about this, shouldn't this change go into platform_get_irq() >> instead of platform_get_irq_optional() ? > > Why? platform_get_irq() currently just calls platform_get_irq_optional()... > >> The way I see it, I think that the intended behavior for >> platform_get_irq_optional() is: >> 1) If have IRQ, return it, always > 0 >> 2) If no IRQ, return 0 > > That does include the IRQ0 case, right? IRQ 0 being invalid, I think that case should be dealt with internally within platform_get_irq_optional() and warn/error return. IRQ 0 showing up would thus be case (3), an error. > >> 3) If error, return < 0 >> no ? > > I completely agree, I (after thinking a bit) have no issues with that... > >> And for platform_get_irq(), case (2) becomes an error. >> Is this the intended semantic ? > > I don't see how it's different from the current behavior. But we can do > that as well, I just don't see whether it's really better... The problem I see is that the current behavior is unclear: what does platform_get_irq_optional() returning 0 mean ? IRQ == 0 ? or "no IRQ" ? I think it should be the latter rather than the former. Note that the function could return ENOENT (or similar) for the "no IRQ" case. With that, case (2) goes away, but then I do not see any difference between platform_get_irq_optional() and platform_get_irq(). If the preferred API semantic is to allow returning IRQ 0 with a warning, then the kdoc comments of platform_get_irq_optional() and platform_get_irq() are totally broken, and the code for many drivers is probably wrong too. > >> I am really not sure here as the functions kdoc description and the code do not >> match. Which one is correct ? > > It seems both are wrong. :-) > > [...] > > MBR, Sergey -- Damien Le Moal Western Digital Research ^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH v1 1/2] ata: libahci_platform: Get rid of dup message when IRQ can't be retrieved 2021-12-12 22:39 ` Damien Le Moal @ 2021-12-13 11:52 ` Andy Shevchenko 2021-12-13 21:36 ` Damien Le Moal 0 siblings, 1 reply; 44+ messages in thread From: Andy Shevchenko @ 2021-12-13 11:52 UTC (permalink / raw) To: Damien Le Moal Cc: Sergey Shtylyov, linux-ide, linux-kernel, Hans de Goede, Jens Axboe On Mon, Dec 13, 2021 at 07:39:31AM +0900, Damien Le Moal wrote: > On 2021/12/11 19:25, Sergey Shtylyov wrote: > > On 11.12.2021 2:45, Damien Le Moal wrote: ... > >>>> So 0 will be returned as-is. That is rather weird. That should be fixed to > >>>> return -ENXIO: > >>>> > >>>> if (WARN(ret == 0, "0 is an invalid IRQ number\n")) > >>>> return -ENXIO; > >>>> return ret; > >>> > >>> My unmerged patch (https://marc.info/?l=linux-kernel&m=163623041902285) does this > >>> but returns -EINVAL instead. > >> > >> Thinking more about this, shouldn't this change go into platform_get_irq() > >> instead of platform_get_irq_optional() ? > > > > Why? platform_get_irq() currently just calls platform_get_irq_optional()... > > > >> The way I see it, I think that the intended behavior for > >> platform_get_irq_optional() is: > >> 1) If have IRQ, return it, always > 0 > >> 2) If no IRQ, return 0 > > > > That does include the IRQ0 case, right? > > IRQ 0 being invalid, I think that case should be dealt with internally within > platform_get_irq_optional() and warn/error return. IRQ 0 showing up would thus > be case (3), an error. > > > > >> 3) If error, return < 0 > >> no ? > > > > I completely agree, I (after thinking a bit) have no issues with that... > > > >> And for platform_get_irq(), case (2) becomes an error. > >> Is this the intended semantic ? > > > > I don't see how it's different from the current behavior. But we can do > > that as well, I just don't see whether it's really better... > > The problem I see is that the current behavior is unclear: what does > platform_get_irq_optional() returning 0 mean ? IRQ == 0 ? or "no IRQ" ? I think > it should be the latter rather than the former. Note that the function could > return ENOENT (or similar) for the "no IRQ" case. With that, case (2) goes away, > but then I do not see any difference between platform_get_irq_optional() and > platform_get_irq(). > > If the preferred API semantic is to allow returning IRQ 0 with a warning, then > the kdoc comments of platform_get_irq_optional() and platform_get_irq() are > totally broken, and the code for many drivers is probably wrong too. Yeah, what we need to do is that (roughly a roadmap): - revisit callers of platform_get_irq_optional() to be prepared for new behaviour - rewrite platform_get_irq() to return -ENOENT - rewrite platform_get_irq_optional() to return 0 on -ENOENT This is how other similar (i.e. _optional) APIs do. -- With Best Regards, Andy Shevchenko ^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH v1 1/2] ata: libahci_platform: Get rid of dup message when IRQ can't be retrieved 2021-12-13 11:52 ` Andy Shevchenko @ 2021-12-13 21:36 ` Damien Le Moal 2021-12-13 22:02 ` Andy Shevchenko 0 siblings, 1 reply; 44+ messages in thread From: Damien Le Moal @ 2021-12-13 21:36 UTC (permalink / raw) To: Andy Shevchenko Cc: Sergey Shtylyov, linux-ide, linux-kernel, Hans de Goede, Jens Axboe On 2021/12/13 20:52, Andy Shevchenko wrote: > On Mon, Dec 13, 2021 at 07:39:31AM +0900, Damien Le Moal wrote: >> On 2021/12/11 19:25, Sergey Shtylyov wrote: >>> On 11.12.2021 2:45, Damien Le Moal wrote: > > ... > >>>>>> So 0 will be returned as-is. That is rather weird. That should be fixed to >>>>>> return -ENXIO: >>>>>> >>>>>> if (WARN(ret == 0, "0 is an invalid IRQ number\n")) >>>>>> return -ENXIO; >>>>>> return ret; >>>>> >>>>> My unmerged patch (https://marc.info/?l=linux-kernel&m=163623041902285) does this >>>>> but returns -EINVAL instead. >>>> >>>> Thinking more about this, shouldn't this change go into platform_get_irq() >>>> instead of platform_get_irq_optional() ? >>> >>> Why? platform_get_irq() currently just calls platform_get_irq_optional()... >>> >>>> The way I see it, I think that the intended behavior for >>>> platform_get_irq_optional() is: >>>> 1) If have IRQ, return it, always > 0 >>>> 2) If no IRQ, return 0 >>> >>> That does include the IRQ0 case, right? >> >> IRQ 0 being invalid, I think that case should be dealt with internally within >> platform_get_irq_optional() and warn/error return. IRQ 0 showing up would thus >> be case (3), an error. >> >>> >>>> 3) If error, return < 0 >>>> no ? >>> >>> I completely agree, I (after thinking a bit) have no issues with that... >>> >>>> And for platform_get_irq(), case (2) becomes an error. >>>> Is this the intended semantic ? >>> >>> I don't see how it's different from the current behavior. But we can do >>> that as well, I just don't see whether it's really better... >> >> The problem I see is that the current behavior is unclear: what does >> platform_get_irq_optional() returning 0 mean ? IRQ == 0 ? or "no IRQ" ? I think >> it should be the latter rather than the former. Note that the function could >> return ENOENT (or similar) for the "no IRQ" case. With that, case (2) goes away, >> but then I do not see any difference between platform_get_irq_optional() and >> platform_get_irq(). >> >> If the preferred API semantic is to allow returning IRQ 0 with a warning, then >> the kdoc comments of platform_get_irq_optional() and platform_get_irq() are >> totally broken, and the code for many drivers is probably wrong too. > > Yeah, what we need to do is that (roughly a roadmap): > - revisit callers of platform_get_irq_optional() to be prepared for > new behaviour > - rewrite platform_get_irq() to return -ENOENT > - rewrite platform_get_irq_optional() to return 0 on -ENOENT > > This is how other similar (i.e. _optional) APIs do. Sounds like a good plan to me. In the mean time though, your patch 1/2 should keep the "if (!irq)" test and return an error for that case. No ? -- Damien Le Moal Western Digital Research ^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH v1 1/2] ata: libahci_platform: Get rid of dup message when IRQ can't be retrieved 2021-12-13 21:36 ` Damien Le Moal @ 2021-12-13 22:02 ` Andy Shevchenko 0 siblings, 0 replies; 44+ messages in thread From: Andy Shevchenko @ 2021-12-13 22:02 UTC (permalink / raw) To: Damien Le Moal Cc: Sergey Shtylyov, linux-ide, linux-kernel, Hans de Goede, Jens Axboe On Tue, Dec 14, 2021 at 06:36:00AM +0900, Damien Le Moal wrote: > On 2021/12/13 20:52, Andy Shevchenko wrote: > > On Mon, Dec 13, 2021 at 07:39:31AM +0900, Damien Le Moal wrote: > >> On 2021/12/11 19:25, Sergey Shtylyov wrote: ... > >> The problem I see is that the current behavior is unclear: what does > >> platform_get_irq_optional() returning 0 mean ? IRQ == 0 ? or "no IRQ" ? I think > >> it should be the latter rather than the former. Note that the function could > >> return ENOENT (or similar) for the "no IRQ" case. With that, case (2) goes away, > >> but then I do not see any difference between platform_get_irq_optional() and > >> platform_get_irq(). > >> > >> If the preferred API semantic is to allow returning IRQ 0 with a warning, then > >> the kdoc comments of platform_get_irq_optional() and platform_get_irq() are > >> totally broken, and the code for many drivers is probably wrong too. > > > > Yeah, what we need to do is that (roughly a roadmap): > > - revisit callers of platform_get_irq_optional() to be prepared for > > new behaviour > > - rewrite platform_get_irq() to return -ENOENT > > - rewrite platform_get_irq_optional() to return 0 on -ENOENT > > > > This is how other similar (i.e. _optional) APIs do. > > Sounds like a good plan to me. In the mean time though, your patch 1/2 should > keep the "if (!irq)" test and return an error for that case. No ? No problem. I can send a v2. -- With Best Regards, Andy Shevchenko ^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH v1 1/2] ata: libahci_platform: Get rid of dup message when IRQ can't be retrieved 2021-12-11 10:25 ` Sergey Shtylyov 2021-12-12 22:39 ` Damien Le Moal @ 2021-12-13 11:49 ` Andy Shevchenko 1 sibling, 0 replies; 44+ messages in thread From: Andy Shevchenko @ 2021-12-13 11:49 UTC (permalink / raw) To: Sergey Shtylyov Cc: Damien Le Moal, linux-ide, linux-kernel, Hans de Goede, Jens Axboe On Sat, Dec 11, 2021 at 01:25:20PM +0300, Sergey Shtylyov wrote: > On 11.12.2021 2:45, Damien Le Moal wrote: ... > > > > > platform_get_irq() will print a message when it fails. > > > > > No need to repeat this. > > > > > > > > > > While at it, drop redundant check for 0 as platform_get_irq() spills > > > > > out a big WARN() in such case. > > > > > > > > The reason you should be able to remove the "if (!irq)" test is that > > > > platform_get_irq() never returns 0. At least, that is what the function kdoc > > > > says. But looking at platform_get_irq_optional(), which is called by > > > > platform_get_irq(), the out label is: > > > > > > > > WARN(ret == 0, "0 is an invalid IRQ number\n"); > > > > return ret; > > > > > > > > So 0 will be returned as-is. That is rather weird. That should be fixed to > > > > return -ENXIO: > > > > > > > > if (WARN(ret == 0, "0 is an invalid IRQ number\n")) > > > > return -ENXIO; > > > > return ret; > > > > > > My unmerged patch (https://marc.info/?l=linux-kernel&m=163623041902285) does this > > > but returns -EINVAL instead. > > > > Thinking more about this, shouldn't this change go into platform_get_irq() > > instead of platform_get_irq_optional() ? > > Why? platform_get_irq() currently just calls platform_get_irq_optional()... > > > The way I see it, I think that the intended behavior for > > platform_get_irq_optional() is: > > 1) If have IRQ, return it, always > 0 > > 2) If no IRQ, return 0 > > That does include the IRQ0 case, right? What IRQ0 case? We do not expect platform APIs ever to handle vIRQ0 (mind letter v) case. Platforms, where it's the case, should handle it exceptionally on per architecture basis. > > 3) If error, return < 0 > > no ? > > I completely agree, I (after thinking a bit) have no issues with that... > > > And for platform_get_irq(), case (2) becomes an error. > > Is this the intended semantic ? > > I don't see how it's different from the current behavior. But we can do > that as well, I just don't see whether it's really better... > > > I am really not sure here as the functions kdoc description and the code do not > > match. Which one is correct ? > > It seems both are wrong. :-) -- With Best Regards, Andy Shevchenko ^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: [PATCH v1 1/2] ata: libahci_platform: Get rid of dup message when IRQ can't be retrieved 2021-12-10 23:45 ` Damien Le Moal 2021-12-11 10:25 ` Sergey Shtylyov @ 2021-12-13 11:46 ` Andy Shevchenko 1 sibling, 0 replies; 44+ messages in thread From: Andy Shevchenko @ 2021-12-13 11:46 UTC (permalink / raw) To: Damien Le Moal Cc: Sergey Shtylyov, linux-ide, linux-kernel, Hans de Goede, Jens Axboe On Sat, Dec 11, 2021 at 08:45:51AM +0900, Damien Le Moal wrote: > On 2021/12/10 17:59, Sergey Shtylyov wrote: > > On 12/10/21 1:49 AM, Damien Le Moal wrote: > > > >>> platform_get_irq() will print a message when it fails. > >>> No need to repeat this. > >>> > >>> While at it, drop redundant check for 0 as platform_get_irq() spills > >>> out a big WARN() in such case. > >> > >> The reason you should be able to remove the "if (!irq)" test is that > >> platform_get_irq() never returns 0. At least, that is what the function kdoc > >> says. But looking at platform_get_irq_optional(), which is called by > >> platform_get_irq(), the out label is: > >> > >> WARN(ret == 0, "0 is an invalid IRQ number\n"); > >> return ret; > >> > >> So 0 will be returned as-is. That is rather weird. That should be fixed to > >> return -ENXIO: > >> > >> if (WARN(ret == 0, "0 is an invalid IRQ number\n")) > >> return -ENXIO; > >> return ret; > > > > My unmerged patch (https://marc.info/?l=linux-kernel&m=163623041902285) does this > > but returns -EINVAL instead. > > Thinking more about this, shouldn't this change go into platform_get_irq() > instead of platform_get_irq_optional() ? > > The way I see it, I think that the intended behavior for > platform_get_irq_optional() is: > 1) If have IRQ, return it, always > 0 > 2) If no IRQ, return 0 > 3) If error, return < 0 > no ? At least this is my understanding on how it _should_ be. > And for platform_get_irq(), case (2) becomes an error. Precisely! > Is this the intended semantic ? > I am really not sure here as the functions kdoc description and the code do not > match. Which one is correct ? The problem is that platform_get_irq_optional() doesn't follow above mentioned logic and needs to be fixed. While trying to fix that it appears that it's not an simple and 5 minutes task since it needs a revisiting of all callers first followed by rectifying the API itself. > >> Otherwise, I do not think that removing the "if (!irq)" hunk is safe. no ? > > > > Of course it isn't... -- With Best Regards, Andy Shevchenko ^ permalink raw reply [flat|nested] 44+ messages in thread
end of thread, other threads:[~2021-12-17 11:58 UTC | newest] Thread overview: 44+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-12-09 14:59 [PATCH v1 1/2] ata: libahci_platform: Get rid of dup message when IRQ can't be retrieved Andy Shevchenko 2021-12-09 14:59 ` [PATCH v1 2/2] ata: libahci_platform: Remove bogus 32-bit DMA mask attempt Andy Shevchenko 2021-12-10 19:34 ` Andy Shevchenko 2021-12-11 0:04 ` Damien Le Moal 2021-12-17 0:58 ` Damien Le Moal 2021-12-17 11:57 ` Andy Shevchenko 2021-12-09 15:21 ` [PATCH v1 1/2] ata: libahci_platform: Get rid of dup message when IRQ can't be retrieved Hans de Goede 2021-12-09 17:24 ` Sergey Shtylyov 2021-12-09 17:42 ` Andy Shevchenko 2021-12-09 18:22 ` Sergey Shtylyov 2021-12-09 19:22 ` Andy Shevchenko 2021-12-09 19:27 ` Andy Shevchenko 2021-12-09 20:31 ` Sergey Shtylyov 2021-12-09 20:29 ` Sergey Shtylyov 2021-12-10 10:44 ` Andy Shevchenko 2021-12-10 11:14 ` Sergey Shtylyov 2021-12-10 11:28 ` Andy Shevchenko 2021-12-10 17:39 ` Sergey Shtylyov 2021-12-10 17:51 ` Andy Shevchenko 2021-12-09 22:49 ` Damien Le Moal 2021-12-09 22:57 ` Damien Le Moal 2021-12-10 8:47 ` Andy Shevchenko 2021-12-10 16:38 ` Sergey Shtylyov 2021-12-10 17:57 ` Andy Shevchenko 2021-12-10 8:59 ` Sergey Shtylyov 2021-12-10 10:46 ` Andy Shevchenko 2021-12-10 11:19 ` Sergey Shtylyov 2021-12-10 11:36 ` Andy Shevchenko 2021-12-10 17:15 ` Sergei Shtylyov 2021-12-10 17:59 ` Andy Shevchenko 2021-12-10 19:01 ` Sergey Shtylyov 2021-12-10 19:25 ` Andy Shevchenko 2021-12-10 19:30 ` Sergey Shtylyov 2021-12-10 19:35 ` Sergei Shtylyov 2021-12-11 10:13 ` Sergey Shtylyov 2021-12-13 11:26 ` Andy Shevchenko 2021-12-10 23:45 ` Damien Le Moal 2021-12-11 10:25 ` Sergey Shtylyov 2021-12-12 22:39 ` Damien Le Moal 2021-12-13 11:52 ` Andy Shevchenko 2021-12-13 21:36 ` Damien Le Moal 2021-12-13 22:02 ` Andy Shevchenko 2021-12-13 11:49 ` Andy Shevchenko 2021-12-13 11:46 ` Andy Shevchenko
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.