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

[Lorenzo Pieralisi <lpieralisi@kernel.org>]

[+Catalin, Will: ACPI arm64 changes are sent through arm64 tree]

On Wed, Oct 26, 2022 at 05:09:40PM +0100, James Morse wrote:
> 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@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, I think though that's something we may want to handle in a
separate patch.

This one looks fine to merge to me:

Reviewed-by: Lorenzo Pieralisi <lpieralisi@kernel.org>

> 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@lists.infradead.org
> >> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
> 

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel