Re: [PATCH] soc: aspeed: deregister a misc device on error

Re: [PATCH] soc: aspeed: deregister a misc device on error

[Andrew Jeffery]

Hi Joe,

On Sat, 2024-12-07 at 12:25 +0900, Joe Hattori wrote:
> The error path of aspeed_lpc_enable_snoop() does not deregister the
> misc
> device, which results in a memory leak. Therefore, add a
> misc_deregister() call in the error path.
> 
> Fixes: 524feb799408 ("soc: add aspeed folder and misc drivers")
> Signed-off-by: Joe Hattori <joe@pf.is.s.u-tokyo.ac.jp>
> ---
>  drivers/soc/aspeed/aspeed-lpc-snoop.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/soc/aspeed/aspeed-lpc-snoop.c
> b/drivers/soc/aspeed/aspeed-lpc-snoop.c
> index 9ab5ba9cf1d6..083ddf6dcb7a 100644
> --- a/drivers/soc/aspeed/aspeed-lpc-snoop.c
> +++ b/drivers/soc/aspeed/aspeed-lpc-snoop.c
> @@ -221,6 +221,7 @@ static int aspeed_lpc_enable_snoop(struct
> aspeed_lpc_snoop *lpc_snoop,
>                 hicrb_en = HICRB_ENSNP1D;
>                 break;
>         default:
> +               misc_deregister(&lpc_snoop->chan[channel].miscdev);

We should free the kfifo too.

Anyway, all the switch statement is doing is setting up mask metadata,
the non-default cases don't depend on the acquired resources. I think
it would make more sense to move it prior to any resource acquisition,
rather than try to unwind their acquisition in the default case.

Andrew

Re: [PATCH] soc: aspeed: deregister a misc device on error

[Joe Hattori]

Hi Andrew,

Thank you for your review.

On 12/9/24 09:28, Andrew Jeffery wrote:
> Hi Joe,
> 
> On Sat, 2024-12-07 at 12:25 +0900, Joe Hattori wrote:
>> The error path of aspeed_lpc_enable_snoop() does not deregister the
>> misc
>> device, which results in a memory leak. Therefore, add a
>> misc_deregister() call in the error path.
>>
>> Fixes: 524feb799408 ("soc: add aspeed folder and misc drivers")
>> Signed-off-by: Joe Hattori <joe@pf.is.s.u-tokyo.ac.jp>
>> ---
>>   drivers/soc/aspeed/aspeed-lpc-snoop.c | 1 +
>>   1 file changed, 1 insertion(+)
>>
>> diff --git a/drivers/soc/aspeed/aspeed-lpc-snoop.c
>> b/drivers/soc/aspeed/aspeed-lpc-snoop.c
>> index 9ab5ba9cf1d6..083ddf6dcb7a 100644
>> --- a/drivers/soc/aspeed/aspeed-lpc-snoop.c
>> +++ b/drivers/soc/aspeed/aspeed-lpc-snoop.c
>> @@ -221,6 +221,7 @@ static int aspeed_lpc_enable_snoop(struct
>> aspeed_lpc_snoop *lpc_snoop,
>>                  hicrb_en = HICRB_ENSNP1D;
>>                  break;
>>          default:
>> +               misc_deregister(&lpc_snoop->chan[channel].miscdev);
> 
> We should free the kfifo too.

Yes, fixed in the V2 patch.
> 
> Anyway, all the switch statement is doing is setting up mask metadata,
> the non-default cases don't depend on the acquired resources. I think
> it would make more sense to move it prior to any resource acquisition,
> rather than try to unwind their acquisition in the default case.

Definitely, it only depends on the function arg `channel`. Modified in 
the v2 patch as well.
> 
> Andrew

Best,
Joe