| Message ID | 20220124185503.6720-2-s.shtylyov@omp.ru (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Fix deferred probing in the EDAC drivers | expand |
On 1/24/22 12:55, Sergey Shtylyov wrote: > The driver overrides the error codes returned by platform_get_irq() to > -ENODEV for some strange reason, so if it returns -EPROBE_DEFER, the > driver will fail the probe permanently instead of the deferred probing. > Switch to propagating the error codes upstream. > > Fixes: 71bcada88b0f ("edac: altera: Add Altera SDRAM EDAC support") > Signed-off-by: Sergey Shtylyov <s.shtylyov@omp.ru> > --- > drivers/edac/altera_edac.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/edac/altera_edac.c b/drivers/edac/altera_edac.c > index 3a6d2416cb0f..5dd29789f97d 100644 > --- a/drivers/edac/altera_edac.c > +++ b/drivers/edac/altera_edac.c > @@ -350,7 +350,7 @@ static int altr_sdram_probe(struct platform_device *pdev) > if (irq < 0) { > edac_printk(KERN_ERR, EDAC_MC, > "No irq %d in DT\n", irq); > - return -ENODEV; > + return irq; > } > > /* Arria10 has a 2nd IRQ */ Acked-by: Dinh Nguyen <dinguyen@kernel.org>
On Wed, Jan 26, 2022 at 10:52:20AM -0600, Dinh Nguyen wrote: > On 1/24/22 12:55, Sergey Shtylyov wrote: > > The driver overrides the error codes returned by platform_get_irq() to > > -ENODEV for some strange reason, so if it returns -EPROBE_DEFER, the > > driver will fail the probe permanently instead of the deferred probing. > > Switch to propagating the error codes upstream. > > > > Fixes: 71bcada88b0f ("edac: altera: Add Altera SDRAM EDAC support") > > Signed-off-by: Sergey Shtylyov <s.shtylyov@omp.ru> > > --- > > drivers/edac/altera_edac.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/drivers/edac/altera_edac.c b/drivers/edac/altera_edac.c > > index 3a6d2416cb0f..5dd29789f97d 100644 > > --- a/drivers/edac/altera_edac.c > > +++ b/drivers/edac/altera_edac.c > > @@ -350,7 +350,7 @@ static int altr_sdram_probe(struct platform_device *pdev) > > if (irq < 0) { > > edac_printk(KERN_ERR, EDAC_MC, > > "No irq %d in DT\n", irq); > > - return -ENODEV; > > + return irq; > > } > > /* Arria10 has a 2nd IRQ */ > > > Acked-by: Dinh Nguyen <dinguyen@kernel.org> It sounds to me like we want this CC: stable@ too? If so, looking at 2043727c2882 ("driver core: platform: Make use of the helper function dev_err_probe()") that added that dev_err_probe() call, which was in Nov. 2021, which would mean, even if stable, only 5.15 and not in all stable trees judging by the Fixes: tag which is a patch from 3.17, i.e., 2014. Right?
On 1/28/22 9:36 PM, Borislav Petkov wrote: >>> The driver overrides the error codes returned by platform_get_irq() to >>> -ENODEV for some strange reason, so if it returns -EPROBE_DEFER, the >>> driver will fail the probe permanently instead of the deferred probing. >>> Switch to propagating the error codes upstream. >>> >>> Fixes: 71bcada88b0f ("edac: altera: Add Altera SDRAM EDAC support") >>> Signed-off-by: Sergey Shtylyov <s.shtylyov@omp.ru> >>> --- >>> drivers/edac/altera_edac.c | 2 +- >>> 1 file changed, 1 insertion(+), 1 deletion(-) >>> >>> diff --git a/drivers/edac/altera_edac.c b/drivers/edac/altera_edac.c >>> index 3a6d2416cb0f..5dd29789f97d 100644 >>> --- a/drivers/edac/altera_edac.c >>> +++ b/drivers/edac/altera_edac.c >>> @@ -350,7 +350,7 @@ static int altr_sdram_probe(struct platform_device *pdev) >>> if (irq < 0) { >>> edac_printk(KERN_ERR, EDAC_MC, >>> "No irq %d in DT\n", irq); >>> - return -ENODEV; >>> + return irq; >>> } >>> /* Arria10 has a 2nd IRQ */ >> >> >> Acked-by: Dinh Nguyen <dinguyen@kernel.org> > > It sounds to me like we want this CC: stable@ too? I think the -stable people will pick it up based on the Fixes: tag. > If so, looking at > > 2043727c2882 ("driver core: platform: Make use of the helper function dev_err_probe()") > > that added that dev_err_probe() call, which was in Nov. 2021, which It didn't add anything new -- there was dev_err() there before that, added by 7723f4c5ecdb8d832f049f8483beb0d1081cedf6. > would mean, even if stable, only 5.15 and not in all stable trees > judging by the Fixes: tag which is a patch from 3.17, i.e., 2014. > > Right? Mmm, platfrom_get_irq() started to return errors long ago, no? MBR, Sergey
On 1/28/22 9:53 PM, Sergey Shtylyov wrote: >>>> The driver overrides the error codes returned by platform_get_irq() to >>>> -ENODEV for some strange reason, so if it returns -EPROBE_DEFER, the >>>> driver will fail the probe permanently instead of the deferred probing. >>>> Switch to propagating the error codes upstream. >>>> >>>> Fixes: 71bcada88b0f ("edac: altera: Add Altera SDRAM EDAC support") >>>> Signed-off-by: Sergey Shtylyov <s.shtylyov@omp.ru> >>>> --- >>>> drivers/edac/altera_edac.c | 2 +- >>>> 1 file changed, 1 insertion(+), 1 deletion(-) >>>> >>>> diff --git a/drivers/edac/altera_edac.c b/drivers/edac/altera_edac.c >>>> index 3a6d2416cb0f..5dd29789f97d 100644 >>>> --- a/drivers/edac/altera_edac.c >>>> +++ b/drivers/edac/altera_edac.c >>>> @@ -350,7 +350,7 @@ static int altr_sdram_probe(struct platform_device *pdev) >>>> if (irq < 0) { >>>> edac_printk(KERN_ERR, EDAC_MC, >>>> "No irq %d in DT\n", irq); >>>> - return -ENODEV; >>>> + return irq; >>>> } >>>> /* Arria10 has a 2nd IRQ */ >>> >>> >>> Acked-by: Dinh Nguyen <dinguyen@kernel.org> >> >> It sounds to me like we want this CC: stable@ too? > > I think the -stable people will pick it up based on the Fixes: tag. > >> If so, looking at >> >> 2043727c2882 ("driver core: platform: Make use of the helper function dev_err_probe()") >> >> that added that dev_err_probe() call, which was in Nov. 2021, which > > It didn't add anything new -- there was dev_err() there before that, > added by 7723f4c5ecdb8d832f049f8483beb0d1081cedf6. > >> would mean, even if stable, only 5.15 and not in all stable trees >> judging by the Fixes: tag which is a patch from 3.17, i.e., 2014. >> >> Right? > > Mmm, platfrom_get_irq() started to return errors long ago, no? Ah, you were wondering about returing -EPROBE_DEFER? It started to be returned by the commit 9ec36cafe43bf835f8f29273597a5b0cbc8267ef several months before that... MBR, Sergey
On Fri, Jan 28, 2022 at 10:02:46PM +0300, Sergey Shtylyov wrote: > > I think the -stable people will pick it up based on the Fixes: tag. My last info is that they don't do that yet. > Ah, you were wondering about returing -EPROBE_DEFER? It started to be returned > by the commit 9ec36cafe43bf835f8f29273597a5b0cbc8267ef several months before that... More precisely, I'm wondering after which commit does the deferred probing code interpret -EPROBE_DEFER properly so that a backport of this commit would be even worth the trouble? Because if we backport it to kernel where there's not even deferred probing support, then that backport is a waste of time.
On 1/28/22 10:16 PM, Borislav Petkov wrote: >>> I think the -stable people will pick it up based on the Fixes: tag. > > My last info is that they don't do that yet. My experience tells they do. >> Ah, you were wondering about returing -EPROBE_DEFER? It started to be returned >> by the commit 9ec36cafe43bf835f8f29273597a5b0cbc8267ef several months before that... > > More precisely, I'm wondering after which commit does the deferred > probing code interpret -EPROBE_DEFER properly so that a backport of this > commit would be even worth the trouble? > > Because if we backport it to kernel where there's not even deferred > probing support, then that backport is a waste of time. See my other mail... MBR, Sergey
On Fri, Jan 28, 2022 at 10:17:55PM +0300, Sergey Shtylyov wrote: > My experience tells they do. Let's ask them: @stable folks, do you guys take patches based only on Fixes: tags nowadays or you still require CC:stable to be present in the commit message? > See my other mail... Which other mail?
On 1/28/22 10:20 PM, Borislav Petkov wrote: [...] >> See my other mail... > > Which other mail? https://lore.kernel.org/all/9f28d2de-5119-a7a6-9da7-08b2ce13f1a0@omp.ru/ MBR, Sergey
On 1/28/22 10:35 PM, Sergey Shtylyov wrote: [...] >>> See my other mail... >> >> Which other mail? > > https://lore.kernel.org/all/9f28d2de-5119-a7a6-9da7-08b2ce13f1a0@omp.ru/ Ah, you've already replied to that! Sorry, I'm in a little bit of haste, so didn't always read your replies attentively enough. :-/ MBR, Sergey
On 1/28/22 10:16 PM, Borislav Petkov wrote: [...] >> Ah, you were wondering about returing -EPROBE_DEFER? It started to be returned >> by the commit 9ec36cafe43bf835f8f29273597a5b0cbc8267ef several months before that... > > More precisely, I'm wondering after which commit does the deferred > probing code interpret -EPROBE_DEFER properly so that a backport of this > commit would be even worth the trouble? That was even longer ago, done by the commit d1c3414c2a9d10ef7f0f7665f5d2947cd088c093 in 20212, if you must know. :-) > Because if we backport it to kernel where there's not even deferred > probing support, then that backport is a waste of time. I think you're overly caustious in this case. :-) MBR, Sergey
On 1/28/22 10:53 PM, Sergey Shtylyov wrote: [...] >>> Ah, you were wondering about returing -EPROBE_DEFER? It started to be returned >>> by the commit 9ec36cafe43bf835f8f29273597a5b0cbc8267ef several months before that... >> >> More precisely, I'm wondering after which commit does the deferred >> probing code interpret -EPROBE_DEFER properly so that a backport of this >> commit would be even worth the trouble? > > That was even longer ago, done by the commit d1c3414c2a9d10ef7f0f7665f5d2947cd088c093 > in 20212, if you must know. :-) Heh, 2012, of/c. :-) >> Because if we backport it to kernel where there's not even deferred >> probing support, then that backport is a waste of time. > > I think you're overly caustious in this case. :-) Cautious -- my typing still sucks and even the spell checker doesn't help. /me blames laptops with their smallish dot sizes! :-) MBR, Sergey
On Fri, Jan 28, 2022 at 08:20:17PM +0100, Borislav Petkov wrote: > On Fri, Jan 28, 2022 at 10:17:55PM +0300, Sergey Shtylyov wrote: > > My experience tells they do. > > Let's ask them: > > @stable folks, do you guys take patches based only on Fixes: tags > nowadays or you still require CC:stable to be present in the commit > message? If you know you want a patch in the stable tree, add cc: stable. Because not all maintainers remember to do so, we do dig through all patches with just the fixes: tag, and try to backport them if needed, but it does not always happen, and there can be long lags as well. So again, if you know you want it in a stable kernel, add the cc: stable. thanks, greg k-h
On Sat, Jan 29, 2022 at 07:48:34AM +0100, Greg KH wrote: > If you know you want a patch in the stable tree, add cc: stable. > > Because not all maintainers remember to do so, we do dig through all > patches with just the fixes: tag, and try to backport them if needed, > but it does not always happen, and there can be long lags as well. > > So again, if you know you want it in a stable kernel, add the cc: > stable. Thanks for clarifying. I already did so - I figured having cc:stable won't hurt anyway. Besides, it is an explicit statement that "that patch is stable material" because Fixes: doesn't always necessarily mean, stable material. Thx.
diff --git a/drivers/edac/altera_edac.c b/drivers/edac/altera_edac.c index 3a6d2416cb0f..5dd29789f97d 100644 --- a/drivers/edac/altera_edac.c +++ b/drivers/edac/altera_edac.c @@ -350,7 +350,7 @@ static int altr_sdram_probe(struct platform_device *pdev) if (irq < 0) { edac_printk(KERN_ERR, EDAC_MC, "No irq %d in DT\n", irq); - return -ENODEV; + return irq; } /* Arria10 has a 2nd IRQ */
The driver overrides the error codes returned by platform_get_irq() to -ENODEV for some strange reason, so if it returns -EPROBE_DEFER, the driver will fail the probe permanently instead of the deferred probing. Switch to propagating the error codes upstream. Fixes: 71bcada88b0f ("edac: altera: Add Altera SDRAM EDAC support") Signed-off-by: Sergey Shtylyov <s.shtylyov@omp.ru> --- drivers/edac/altera_edac.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)