| Message ID | 20221014160623.467195-1-u.kleine-koenig@pengutronix.de (mailing list archive) |
|---|---|
| State | Handled Elsewhere, archived |
| Headers | show |
| Series | ACPI: AGDI: Improve error reporting for problems during .remove() | expand |
[+James] 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. Lorenzo > Best regards > Uwe > > drivers/acpi/arm64/agdi.c | 13 ++++++++++--- > 1 file changed, 10 insertions(+), 3 deletions(-) > > 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
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 >> 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
Hello James, 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. Is that an Ack? Best regards Uwe
Hello, On Wed, Oct 26, 2022 at 07:23:35PM +0200, Uwe Kleine-König wrote: > On Wed, Oct 26, 2022 at 05:09:40PM +0100, James Morse wrote: > > 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. > > Is that an Ack? This patch wasn't applied anywhere (at least it didn't appear in next since October). Did it fell through the cracks? Is there anything missing? Best regards Uwe
Hello, On Mon, Dec 19, 2022 at 11:18:19PM +0100, Uwe Kleine-König wrote: > On Wed, Oct 26, 2022 at 07:23:35PM +0200, Uwe Kleine-König wrote: > > On Wed, Oct 26, 2022 at 05:09:40PM +0100, James Morse wrote: > > > 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. > > > > Is that an Ack? > > This patch wasn't applied anywhere (at least it didn't appear in next > since October). Did it fell through the cracks? Is there anything > missing? gentle ping! Working on making struct platform_driver::remove() return void, I'd like to base another patch on top of this one. For that it would be great if it entered the mainline ... Thanks for considering, Uwe
On Tue, Feb 14, 2023 at 05:36:23PM +0100, Uwe Kleine-König wrote: > Hello, > > On Mon, Dec 19, 2022 at 11:18:19PM +0100, Uwe Kleine-König wrote: > > On Wed, Oct 26, 2022 at 07:23:35PM +0200, Uwe Kleine-König wrote: > > > On Wed, Oct 26, 2022 at 05:09:40PM +0100, James Morse wrote: > > > > 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. > > > > > > Is that an Ack? > > > > This patch wasn't applied anywhere (at least it didn't appear in next > > since October). Did it fell through the cracks? Is there anything > > missing? > > gentle ping! > > Working on making struct platform_driver::remove() return void, I'd like > to base another patch on top of this one. For that it would be great if > it entered the mainline ... gentle ping ++ Would it help to resend? Best regards Uwe
[+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 >
On Thu, Apr 13, 2023 at 10:23:50AM +0200, Lorenzo Pieralisi wrote: > [+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> Cheers, Lorenzo. I'll pick this one up. Will
On Fri, 14 Oct 2022 18:06:23 +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. > > [...] Applied to arm64 (for-next/acpi), thanks! [1/1] ACPI: AGDI: Improve error reporting for problems during .remove() https://git.kernel.org/arm64/c/858a56630a84 Cheers,
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 = {
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. 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. Best regards Uwe drivers/acpi/arm64/agdi.c | 13 ++++++++++--- 1 file changed, 10 insertions(+), 3 deletions(-) base-commit: 4fe89d07dcc2804c8b562f6c7896a45643d34b2f