[PATCH] EDAC, mv64x60: Remove some code duplication

[PATCH] EDAC, mv64x60: Remove some code duplication

[Christophe JAILLET]

Reorder the error handling code in order to release the resources in
reverse order than allocation.

Introduce a new 'release_group' label in the error handling path and use
it to void some code duplication.

Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
---
 drivers/edac/mv64x60_edac.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/drivers/edac/mv64x60_edac.c b/drivers/edac/mv64x60_edac.c
index 3c68bb525d5d..aa5bc1d8f424 100644
--- a/drivers/edac/mv64x60_edac.c
+++ b/drivers/edac/mv64x60_edac.c
@@ -450,8 +450,8 @@ static int mv64x60_cpu_err_probe(struct platform_device *pdev)
 					      "cpu", 1, NULL, 0, 0, NULL, 0,
 					      edac_dev_idx);
 	if (!edac_dev) {
-		devres_release_group(&pdev->dev, mv64x60_cpu_err_probe);
-		return -ENOMEM;
+		res = -ENOMEM;
+		goto release_group;
 	}
 
 	pdata = edac_dev->pvt_info;
@@ -561,8 +561,9 @@ static int mv64x60_cpu_err_probe(struct platform_device *pdev)
 err2:
 	edac_device_del_device(&pdev->dev);
 err:
-	devres_release_group(&pdev->dev, mv64x60_cpu_err_probe);
 	edac_device_free_ctl_info(edac_dev);
+release_group:
+	devres_release_group(&pdev->dev, mv64x60_cpu_err_probe);
 	return res;
 }
 
-- 
2.14.1

Re: [PATCH] EDAC, mv64x60: Remove some code duplication

[Borislav Petkov]

+ Chris Packham who's been fixing some stuff in here too.

On Sat, Jan 13, 2018 at 08:28:21AM +0100, Christophe JAILLET wrote:
> Reorder the error handling code in order to release the resources in
> reverse order than allocation.
> 
> Introduce a new 'release_group' label in the error handling path and use
> it to void some code duplication.
> 
> Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
> ---
>  drivers/edac/mv64x60_edac.c | 7 ++++---
>  1 file changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/edac/mv64x60_edac.c b/drivers/edac/mv64x60_edac.c
> index 3c68bb525d5d..aa5bc1d8f424 100644
> --- a/drivers/edac/mv64x60_edac.c
> +++ b/drivers/edac/mv64x60_edac.c
> @@ -450,8 +450,8 @@ static int mv64x60_cpu_err_probe(struct platform_device *pdev)
>  					      "cpu", 1, NULL, 0, 0, NULL, 0,
>  					      edac_dev_idx);
>  	if (!edac_dev) {
> -		devres_release_group(&pdev->dev, mv64x60_cpu_err_probe);
> -		return -ENOMEM;
> +		res = -ENOMEM;
> +		goto release_group;
>  	}
>  
>  	pdata = edac_dev->pvt_info;
> @@ -561,8 +561,9 @@ static int mv64x60_cpu_err_probe(struct platform_device *pdev)
>  err2:
>  	edac_device_del_device(&pdev->dev);
>  err:
> -	devres_release_group(&pdev->dev, mv64x60_cpu_err_probe);
>  	edac_device_free_ctl_info(edac_dev);
> +release_group:
> +	devres_release_group(&pdev->dev, mv64x60_cpu_err_probe);
>  	return res;
>  }
>  
> -- 

Thanks, looks good. But looking at this driver, mv64x60_mc_err_probe()
and mv64x60_sram_err_probe() have the same problem too. Can you address them
with your patch too pls?

Also, if you feel like fixing more stuff in this driver, it doesn't use
the edac_printk() infrastructure but naked printk() calls. It could be
converted to it.

Thx.

-- 
Regards/Gruss,
    Boris.

Good mailing practices for 400: avoid top-posting and trim the reply.

Re: [PATCH] EDAC, mv64x60: Remove some code duplication

[Christophe JAILLET]

Le 13/01/2018 à 15:22, Borislav Petkov a écrit :
> + Chris Packham who's been fixing some stuff in here too.
>
> On Sat, Jan 13, 2018 at 08:28:21AM +0100, Christophe JAILLET wrote:
>> Reorder the error handling code in order to release the resources in
>> reverse order than allocation.
>>
>> Introduce a new 'release_group' label in the error handling path and use
>> it to void some code duplication.
>>
>> Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
>> ---
>>   drivers/edac/mv64x60_edac.c | 7 ++++---
>>   1 file changed, 4 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/edac/mv64x60_edac.c b/drivers/edac/mv64x60_edac.c
>> index 3c68bb525d5d..aa5bc1d8f424 100644
>> --- a/drivers/edac/mv64x60_edac.c
>> +++ b/drivers/edac/mv64x60_edac.c
>> @@ -450,8 +450,8 @@ static int mv64x60_cpu_err_probe(struct platform_device *pdev)
>>   					      "cpu", 1, NULL, 0, 0, NULL, 0,
>>   					      edac_dev_idx);
>>   	if (!edac_dev) {
>> -		devres_release_group(&pdev->dev, mv64x60_cpu_err_probe);
>> -		return -ENOMEM;
>> +		res = -ENOMEM;
>> +		goto release_group;
>>   	}
>>   
>>   	pdata = edac_dev->pvt_info;
>> @@ -561,8 +561,9 @@ static int mv64x60_cpu_err_probe(struct platform_device *pdev)
>>   err2:
>>   	edac_device_del_device(&pdev->dev);
>>   err:
>> -	devres_release_group(&pdev->dev, mv64x60_cpu_err_probe);
>>   	edac_device_free_ctl_info(edac_dev);
>> +release_group:
>> +	devres_release_group(&pdev->dev, mv64x60_cpu_err_probe);
>>   	return res;
>>   }
>>   
>> -- 
> Thanks, looks good. But looking at this driver, mv64x60_mc_err_probe()
> and mv64x60_sram_err_probe() have the same problem too. Can you address them
> with your patch too pls?
Will do. mv64x60_pci_err_probe() also needs some tweaks.

> Also, if you feel like fixing more stuff in this driver, it doesn't use
> the edac_printk() infrastructure but naked printk() calls. It could be
> converted to it.
I will only propose to remove a useless message and improve another one, 
but won't convert the whole driver, sorry.

CJ

> Thx.
>

Re: [PATCH] EDAC, mv64x60: Remove some code duplication

[Chris Packham]

Hi Christophe,

On 14/01/18 06:17, Christophe JAILLET wrote:
> Le 13/01/2018 à 15:22, Borislav Petkov a écrit :
>> + Chris Packham who's been fixing some stuff in here too.
>>
>> On Sat, Jan 13, 2018 at 08:28:21AM +0100, Christophe JAILLET wrote:
>>> Reorder the error handling code in order to release the resources in
>>> reverse order than allocation.
>>>
>>> Introduce a new 'release_group' label in the error handling path and use
>>> it to void some code duplication.
>>>
>>> Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
>>> ---
>>>    drivers/edac/mv64x60_edac.c | 7 ++++---
>>>    1 file changed, 4 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/drivers/edac/mv64x60_edac.c b/drivers/edac/mv64x60_edac.c
>>> index 3c68bb525d5d..aa5bc1d8f424 100644
>>> --- a/drivers/edac/mv64x60_edac.c
>>> +++ b/drivers/edac/mv64x60_edac.c
>>> @@ -450,8 +450,8 @@ static int mv64x60_cpu_err_probe(struct platform_device *pdev)
>>>    					      "cpu", 1, NULL, 0, 0, NULL, 0,
>>>    					      edac_dev_idx);
>>>    	if (!edac_dev) {
>>> -		devres_release_group(&pdev->dev, mv64x60_cpu_err_probe);
>>> -		return -ENOMEM;
>>> +		res = -ENOMEM;
>>> +		goto release_group;
>>>    	}
>>>    
>>>    	pdata = edac_dev->pvt_info;
>>> @@ -561,8 +561,9 @@ static int mv64x60_cpu_err_probe(struct platform_device *pdev)
>>>    err2:
>>>    	edac_device_del_device(&pdev->dev);
>>>    err:
>>> -	devres_release_group(&pdev->dev, mv64x60_cpu_err_probe);
>>>    	edac_device_free_ctl_info(edac_dev);
>>> +release_group:
>>> +	devres_release_group(&pdev->dev, mv64x60_cpu_err_probe);
>>>    	return res;
>>>    }
>>>    
>>> -- 
>> Thanks, looks good. But looking at this driver, mv64x60_mc_err_probe()
>> and mv64x60_sram_err_probe() have the same problem too. Can you address them
>> with your patch too pls?
> Will do. mv64x60_pci_err_probe() also needs some tweaks.
> 
>> Also, if you feel like fixing more stuff in this driver, it doesn't use
>> the edac_printk() infrastructure but naked printk() calls. It could be
>> converted to it.
> I will only propose to remove a useless message and improve another one,
> but won't convert the whole driver, sorry.
> 

I take this you mean you have a system with a mv64x60 SoC? You might 
want to make yourself known to the linuxppc-dev list. A while back the 
prospects of dropping CONFIG_MV64X60 was raised[1]. I don't see anyone 
actually following through on this yet but I'm not really following 
linuxppc that closely.

[1] - https://marc.info/?l=linux-edac&m=149518763115206&w=2

Re: [PATCH] EDAC, mv64x60: Remove some code duplication

[Michael Ellerman]

Chris Packham <Chris.Packham@alliedtelesis.co.nz> writes:
> On 14/01/18 06:17, Christophe JAILLET wrote:
>> Le 13/01/2018 à 15:22, Borislav Petkov a écrit :
>>> + Chris Packham who's been fixing some stuff in here too.
>>>
>>> On Sat, Jan 13, 2018 at 08:28:21AM +0100, Christophe JAILLET wrote:
>>>> Reorder the error handling code in order to release the resources in
>>>> reverse order than allocation.
>>>>
>>>> Introduce a new 'release_group' label in the error handling path and use
>>>> it to void some code duplication.
>>>>
>>>> Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
>>>> ---
>>>>    drivers/edac/mv64x60_edac.c | 7 ++++---
>>>>    1 file changed, 4 insertions(+), 3 deletions(-)
...
>>>>    
>>>> -- 
>>> Thanks, looks good. But looking at this driver, mv64x60_mc_err_probe()
>>> and mv64x60_sram_err_probe() have the same problem too. Can you address them
>>> with your patch too pls?
>> Will do. mv64x60_pci_err_probe() also needs some tweaks.
>> 
>>> Also, if you feel like fixing more stuff in this driver, it doesn't use
>>> the edac_printk() infrastructure but naked printk() calls. It could be
>>> converted to it.
>> I will only propose to remove a useless message and improve another one,
>> but won't convert the whole driver, sorry.
>> 
>
> I take this you mean you have a system with a mv64x60 SoC? You might 
> want to make yourself known to the linuxppc-dev list. A while back the 
> prospects of dropping CONFIG_MV64X60 was raised[1]. I don't see anyone 
> actually following through on this yet but I'm not really following 
> linuxppc that closely.

That's just because I haven't had time to do it and no one else took the
hint :)

So yes, Christophe if you have a machine that uses this driver please
speak up, otherwise it will probably be removed.

cheers

Re: [PATCH] EDAC, mv64x60: Remove some code duplication

[Christophe JAILLET]

Le 15/01/2018 à 23:31, Michael Ellerman a écrit :
> Chris Packham <Chris.Packham@alliedtelesis.co.nz> writes:
>> On 14/01/18 06:17, Christophe JAILLET wrote:
>>> Le 13/01/2018 à 15:22, Borislav Petkov a écrit :
>>>> + Chris Packham who's been fixing some stuff in here too.
>>>>
>>>> On Sat, Jan 13, 2018 at 08:28:21AM +0100, Christophe JAILLET wrote:
>>>>> Reorder the error handling code in order to release the resources in
>>>>> reverse order than allocation.
>>>>>
>>>>> Introduce a new 'release_group' label in the error handling path and use
>>>>> it to void some code duplication.
>>>>>
>>>>> Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
>>>>> ---
>>>>>     drivers/edac/mv64x60_edac.c | 7 ++++---
>>>>>     1 file changed, 4 insertions(+), 3 deletions(-)
> ...
>>>>>     
>>>>> -- 
>>>> Thanks, looks good. But looking at this driver, mv64x60_mc_err_probe()
>>>> and mv64x60_sram_err_probe() have the same problem too. Can you address them
>>>> with your patch too pls?
>>> Will do. mv64x60_pci_err_probe() also needs some tweaks.
>>>
>>>> Also, if you feel like fixing more stuff in this driver, it doesn't use
>>>> the edac_printk() infrastructure but naked printk() calls. It could be
>>>> converted to it.
>>> I will only propose to remove a useless message and improve another one,
>>> but won't convert the whole driver, sorry.
>>>
>> I take this you mean you have a system with a mv64x60 SoC? You might
>> want to make yourself known to the linuxppc-dev list. A while back the
>> prospects of dropping CONFIG_MV64X60 was raised[1]. I don't see anyone
>> actually following through on this yet but I'm not really following
>> linuxppc that closely.
> That's just because I haven't had time to do it and no one else took the
> hint :)
>
> So yes, Christophe if you have a machine that uses this driver please
> speak up, otherwise it will probably be removed.
>
> cheers
> --
> To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>
Hi,

No I don't have.

I use static checker to find some potential issues in Linux (most of the 
time, with my own coccinelle scripts).
Before proposing some patches, I check if the development on the 
corresponding files is still active.
This driver looked active (i.e. there were several recent commits, even 
if only cleanups).

If it is nearly dead, my small fixes/cleanups are useless, and I will 
leave it as-is.

Thanks for pointing this out. I'll dig somewhere else :)


For the records, and if someone is interested, in order to search for 
"active" files in what I've touched, I use:
(this is a slightly updated version of a script found on Internet)

     # date of the last modification of updated files
     git status -s -uno | while read mode file; do echo "$(git log -1 
--date=format:'%Y%m%d_%H:%M:%S' --format=%cd $file)   $file"; done | 
sort -s -n -k 1,1 > last_modified.txt

When I find a potential candidate, I then have a look in its recent 
history with 'git log' or with 
'https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/log/...'

Best regards,
CJ