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

Joe Hattori joe at pf.is.s.u-tokyo.ac.jp
Wed Dec 11 14:52:30 AEDT 2024


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 at 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


More information about the Linux-aspeed mailing list