[PATCH] ACPI: AGDI: Improve error reporting for problems during .remove()

[James Morse]

Hi guys,

On 18/10/2022 10:35, Lorenzo Pieralisi wrote:
> On Fri, Oct 14, 2022 at 06:06:23PM +0200, Uwe Kleine-K�nig wrote:
>> Returning an error value in a platform driver's remove callback results in
>> a generic error message being emitted by the driver core, but otherwise it
>> doesn't make a difference. The device goes away anyhow.
>>
>> So instead of triggering the generic platform error message, emit a more
>> helpful message if a problem occurs and return 0 to suppress the generic
>> message.
>>
>> This patch is a preparation for making platform remove callbacks return
>> void.
> 
> If that's the plan - I don't have anything against this patch.
> 
>> Signed-off-by: Uwe Kleine-K�nig <u.kleine-koenig at pengutronix.de>
>> ---
>> Hello,
>>
>> note that in the situations where the driver returned an error before
>> and now emits a message, there is a resource leak. Someone who knows
>> more about this driver and maybe even can test stuff, might want to
>> address this. This might not only be about non-freed memory, the device
>> disappears but it is kept in sdei_list and so might be used after being
>> gone.

> I'd need James' input on this. I guess we may ignore
> sdei_event_disable() return value and continue anyway in agdi_remove(),
> whether that's the right thing to do it is a different question.

The unregister stuff is allowed to fail if the event is 'in progress' on another CPU.
Given the handler panic()s the machine, if an event is in progress, the resource leak
isn't something worth worrying about. The real problem is that the handler code may be
free()d while another CPU is still executing it, which is only a problem for modules.

As this thing can't be built as a module, and the handler panic()s the machine, I don't
think there is going to be a problem here.


Thanks,

James


>> diff --git a/drivers/acpi/arm64/agdi.c b/drivers/acpi/arm64/agdi.c
>> index cf31abd0ed1b..f605302395c3 100644
>> --- a/drivers/acpi/arm64/agdi.c
>> +++ b/drivers/acpi/arm64/agdi.c
>> @@ -64,8 +64,11 @@ static int agdi_remove(struct platform_device *pdev)
>>  	int err, i;
>>  
>>  	err = sdei_event_disable(adata->sdei_event);
>> -	if (err)
>> -		return err;
>> +	if (err) {
>> +		dev_err(&pdev->dev, "Failed to disable sdei-event #%d (%pe)\n",
>> +			adata->sdei_event, ERR_PTR(err));
>> +		return 0;
>> +	}
>>  
>>  	for (i = 0; i < 3; i++) {
>>  		err = sdei_event_unregister(adata->sdei_event);
>> @@ -75,7 +78,11 @@ static int agdi_remove(struct platform_device *pdev)
>>  		schedule();
>>  	}
>>  
>> -	return err;
>> +	if (err)
>> +		dev_err(&pdev->dev, "Failed to unregister sdei-event #%d (%pe)\n",
>> +			adata->sdei_event, ERR_PTR(err));
>> +
>> +	return 0;
>>  }
>>  
>>  static struct platform_driver agdi_driver = {
>>
>> base-commit: 4fe89d07dcc2804c8b562f6c7896a45643d34b2f
>> -- 
>> 2.37.2
>>
>>
>> _______________________________________________
>> linux-arm-kernel mailing list
>> linux-arm-kernel at lists.infradead.org
>> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel