[PATCH 0/2] Fix deferred probing in the EDAC drivers

[PATCH 0/2] Fix deferred probing in the EDAC drivers

[Sergey Shtylyov]

Here are 2 patches against the 'edac-for-next' branch of the 'ras.git' repo
(for the lack of a better branch for fixes?).  The affected drivers call
platform_get_irq[_optional]() but override its result in case of error --
which prevents the deferred probing from working.

Sergey Shtylyov (2):
  edac: altera: fix deferred probing
  edac: xgene: fix deferred probing

 drivers/edac/altera_edac.c | 2 +-
 drivers/edac/xgene_edac.c  | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

-- 
2.26.3

[PATCH 1/2] edac: altera: fix deferred probing

[Sergey Shtylyov]

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 */
-- 
2.26.3

Re: [PATCH 1/2] edac: altera: fix deferred probing

[Dinh Nguyen]

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>

Re: [PATCH 1/2] edac: altera: fix deferred probing

[Borislav Petkov]

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?

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

Re: [PATCH 1/2] edac: altera: fix deferred probing

[Sergey Shtylyov]

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

Re: [PATCH 1/2] edac: altera: fix deferred probing

[Sergey Shtylyov]

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

Re: [PATCH 1/2] edac: altera: fix deferred probing

[Borislav Petkov]

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.

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

Re: [PATCH 1/2] edac: altera: fix deferred probing

[Sergey Shtylyov]

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

Re: [PATCH 1/2] edac: altera: fix deferred probing

[Borislav Petkov]

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?

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

Re: [PATCH 1/2] edac: altera: fix deferred probing

[Sergey Shtylyov]

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

Re: [PATCH 1/2] edac: altera: fix deferred probing

[Sergey Shtylyov]

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

Re: [PATCH 1/2] edac: altera: fix deferred probing

[Greg KH]

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

Re: [PATCH 1/2] edac: altera: fix deferred probing

[Borislav Petkov]

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.

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

Re: [PATCH 1/2] edac: altera: fix deferred probing

[Sergey Shtylyov]

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

Re: [PATCH 1/2] edac: altera: fix deferred probing

[Sergey Shtylyov]

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

[PATCH 2/2] edac: xgene: fix deferred probing

[Sergey Shtylyov]

The driver overrides error codes returned by platform_get_irq_optional()
to -EINVAL 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: 0d4429301c4 ("EDAC: Add APM X-Gene SoC EDAC driver")
Signed-off-by: Sergey Shtylyov <s.shtylyov@omp.ru>
---
 drivers/edac/xgene_edac.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/edac/xgene_edac.c b/drivers/edac/xgene_edac.c
index 2ccd1db5e98f..7197f9fa0245 100644
--- a/drivers/edac/xgene_edac.c
+++ b/drivers/edac/xgene_edac.c
@@ -1919,7 +1919,7 @@ static int xgene_edac_probe(struct platform_device *pdev)
 			irq = platform_get_irq_optional(pdev, i);
 			if (irq < 0) {
 				dev_err(&pdev->dev, "No IRQ resource\n");
-				rc = -EINVAL;
+				rc = irq;
 				goto out_err;
 			}
 			rc = devm_request_irq(&pdev->dev, irq,
-- 
2.26.3

Re: [PATCH 0/2] Fix deferred probing in the EDAC drivers

[Borislav Petkov]

On Mon, Jan 24, 2022 at 09:55:01PM +0300, Sergey Shtylyov wrote:
> Here are 2 patches against the 'edac-for-next' branch of the 'ras.git' repo
> (for the lack of a better branch for fixes?).  The affected drivers call
> platform_get_irq[_optional]() but override its result in case of error --
> which prevents the deferred probing from working.
> 
> Sergey Shtylyov (2):
>   edac: altera: fix deferred probing
>   edac: xgene: fix deferred probing
> 
>  drivers/edac/altera_edac.c | 2 +-
>  drivers/edac/xgene_edac.c  | 2 +-
>  2 files changed, 2 insertions(+), 2 deletions(-)
> 
> -- 

Both applied, thanks.

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette