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

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

[Chris Packham]

Hi Christophe,=0A=
=0A=
On 14/01/18 06:17, Christophe JAILLET wrote:=0A=
> Le 13/01/2018 =E0 15:22, Borislav Petkov a =E9crit=A0:=0A=
>> + Chris Packham who's been fixing some stuff in here too.=0A=
>>=0A=
>> On Sat, Jan 13, 2018 at 08:28:21AM +0100, Christophe JAILLET wrote:=0A=
>>> Reorder the error handling code in order to release the resources in=0A=
>>> reverse order than allocation.=0A=
>>>=0A=
>>> Introduce a new 'release_group' label in the error handling path and us=
e=0A=
>>> it to void some code duplication.=0A=
>>>=0A=
>>> Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>=0A=
>>> ---=0A=
>>>    drivers/edac/mv64x60_edac.c | 7 ++++---=0A=
>>>    1 file changed, 4 insertions(+), 3 deletions(-)=0A=
>>>=0A=
>>> diff --git a/drivers/edac/mv64x60_edac.c b/drivers/edac/mv64x60_edac.c=
=0A=
>>> index 3c68bb525d5d..aa5bc1d8f424 100644=0A=
>>> --- a/drivers/edac/mv64x60_edac.c=0A=
>>> +++ b/drivers/edac/mv64x60_edac.c=0A=
>>> @@ -450,8 +450,8 @@ static int mv64x60_cpu_err_probe(struct platform_de=
vice *pdev)=0A=
>>>    					      "cpu", 1, NULL, 0, 0, NULL, 0,=0A=
>>>    					      edac_dev_idx);=0A=
>>>    	if (!edac_dev) {=0A=
>>> -		devres_release_group(&pdev->dev, mv64x60_cpu_err_probe);=0A=
>>> -		return -ENOMEM;=0A=
>>> +		res =3D -ENOMEM;=0A=
>>> +		goto release_group;=0A=
>>>    	}=0A=
>>>    =0A=
>>>    	pdata =3D edac_dev->pvt_info;=0A=
>>> @@ -561,8 +561,9 @@ static int mv64x60_cpu_err_probe(struct platform_de=
vice *pdev)=0A=
>>>    err2:=0A=
>>>    	edac_device_del_device(&pdev->dev);=0A=
>>>    err:=0A=
>>> -	devres_release_group(&pdev->dev, mv64x60_cpu_err_probe);=0A=
>>>    	edac_device_free_ctl_info(edac_dev);=0A=
>>> +release_group:=0A=
>>> +	devres_release_group(&pdev->dev, mv64x60_cpu_err_probe);=0A=
>>>    	return res;=0A=
>>>    }=0A=
>>>    =0A=
>>> -- =0A=
>> Thanks, looks good. But looking at this driver, mv64x60_mc_err_probe()=
=0A=
>> and mv64x60_sram_err_probe() have the same problem too. Can you address =
them=0A=
>> with your patch too pls?=0A=
> Will do. mv64x60_pci_err_probe() also needs some tweaks.=0A=
> =0A=
>> Also, if you feel like fixing more stuff in this driver, it doesn't use=
=0A=
>> the edac_printk() infrastructure but naked printk() calls. It could be=
=0A=
>> converted to it.=0A=
> I will only propose to remove a useless message and improve another one,=
=0A=
> but won't convert the whole driver, sorry.=0A=
> =0A=
=0A=
I take this you mean you have a system with a mv64x60 SoC? You might =0A=
want to make yourself known to the linuxppc-dev list. A while back the =0A=
prospects of dropping CONFIG_MV64X60 was raised[1]. I don't see anyone =0A=
actually following through on this yet but I'm not really following =0A=
linuxppc that closely.=0A=
=0A=
[1] - https://marc.info/?l=3Dlinux-edac&m=3D149518763115206&w=3D2=0A=

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 =C3=A0 15:22, Borislav Petkov a =C3=A9crit=C2=A0:
>>> + 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 u=
se
>>>> 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(-)
...
>>>>=20=20=20=20
>>>> --=20
>>> 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.
>>=20
>>> 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.
>>=20
>
> I take this you mean you have a system with a mv64x60 SoC? You might=20
> want to make yourself known to the linuxppc-dev list. A while back the=20
> prospects of dropping CONFIG_MV64X60 was raised[1]. I don't see anyone=20
> actually following through on this yet but I'm not really following=20
> 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