| Message ID | 20231123211657.518181-6-u.kleine-koenig@pengutronix.de (mailing list archive) |
|---|---|
| State | Accepted |
| Headers | show |
| Series | remoteproc: k3-dsp: Some cleanups | expand |
Hi Uwe, On Thu, Nov 23, 2023 at 10:16:59PM +0100, Uwe Kleine-König wrote: > When the remove callback returns non-zero, the driver core emits an > error message about the error value being ignored. As the driver already > emits an error message already, return zero. This has no effect apart > from suppressing the core's message. The platform device gets unbound > irrespective of the return value. > > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de> > --- > drivers/remoteproc/ti_k3_dsp_remoteproc.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/drivers/remoteproc/ti_k3_dsp_remoteproc.c b/drivers/remoteproc/ti_k3_dsp_remoteproc.c > index ef8415a7cd54..40a5fd8763fa 100644 > --- a/drivers/remoteproc/ti_k3_dsp_remoteproc.c > +++ b/drivers/remoteproc/ti_k3_dsp_remoteproc.c > @@ -835,8 +835,9 @@ static int k3_dsp_rproc_remove(struct platform_device *pdev) > if (rproc->state == RPROC_ATTACHED) { > ret = rproc_detach(rproc); > if (ret) { > + /* Note this error path leaks resources */ I'm not sure why this comment has been added... > dev_err(dev, "failed to detach proc, ret = %d\n", ret); And why this isn't refactored in the next patch. > - return ret; > + return 0; Appart from the above I'm good with this patchset. Thanks, Mathieu > } > } > > -- > 2.42.0 >
Helo Mathieu, On Wed, Nov 29, 2023 at 10:35:32AM -0700, Mathieu Poirier wrote: > On Thu, Nov 23, 2023 at 10:16:59PM +0100, Uwe Kleine-König wrote: > > diff --git a/drivers/remoteproc/ti_k3_dsp_remoteproc.c b/drivers/remoteproc/ti_k3_dsp_remoteproc.c > > index ef8415a7cd54..40a5fd8763fa 100644 > > --- a/drivers/remoteproc/ti_k3_dsp_remoteproc.c > > +++ b/drivers/remoteproc/ti_k3_dsp_remoteproc.c > > @@ -835,8 +835,9 @@ static int k3_dsp_rproc_remove(struct platform_device *pdev) > > if (rproc->state == RPROC_ATTACHED) { > > ret = rproc_detach(rproc); > > if (ret) { > > + /* Note this error path leaks resources */ > > I'm not sure why this comment has been added... The comment was added because there is a real problem and I didn't try to fix it as doing that without the hardware is hard. > > dev_err(dev, "failed to detach proc, ret = %d\n", ret); > > And why this isn't refactored in the next patch. the next patch has: - dev_err(dev, "failed to detach proc, ret = %d\n", ret); + dev_err(dev, "failed to detach proc (%pe)\n", ERR_PTR(ret)); so this is refactored?! > > - return ret; > > + return 0; > > Appart from the above I'm good with this patchset. Best regards Uwe
On Wed, Nov 29, 2023 at 11:50:10PM +0100, Uwe Kleine-König wrote: > Helo Mathieu, > > On Wed, Nov 29, 2023 at 10:35:32AM -0700, Mathieu Poirier wrote: > > On Thu, Nov 23, 2023 at 10:16:59PM +0100, Uwe Kleine-König wrote: > > > diff --git a/drivers/remoteproc/ti_k3_dsp_remoteproc.c b/drivers/remoteproc/ti_k3_dsp_remoteproc.c > > > index ef8415a7cd54..40a5fd8763fa 100644 > > > --- a/drivers/remoteproc/ti_k3_dsp_remoteproc.c > > > +++ b/drivers/remoteproc/ti_k3_dsp_remoteproc.c > > > @@ -835,8 +835,9 @@ static int k3_dsp_rproc_remove(struct platform_device *pdev) > > > if (rproc->state == RPROC_ATTACHED) { > > > ret = rproc_detach(rproc); > > > if (ret) { > > > + /* Note this error path leaks resources */ > > > > I'm not sure why this comment has been added... > > The comment was added because there is a real problem and I didn't try > to fix it as doing that without the hardware is hard. > I've looked at this again and as it turns out, you are correct on both front. I will apply your patches as-is and ask people at TI to look at this code again. Thanks, Mathieu > > > dev_err(dev, "failed to detach proc, ret = %d\n", ret); > > > > And why this isn't refactored in the next patch. > > the next patch has: > > - dev_err(dev, "failed to detach proc, ret = %d\n", ret); > + dev_err(dev, "failed to detach proc (%pe)\n", ERR_PTR(ret)); > > so this is refactored?! > > > > - return ret; > > > + return 0; > > > > Appart from the above I'm good with this patchset. > > Best regards > Uwe > > -- > Pengutronix e.K. | Uwe Kleine-König | > Industrial Linux Solutions | https://www.pengutronix.de/ |
Hari and Nishanth, On Thu, 23 Nov 2023 at 14:17, Uwe Kleine-König <u.kleine-koenig@pengutronix.de> wrote: > > When the remove callback returns non-zero, the driver core emits an > error message about the error value being ignored. As the driver already > emits an error message already, return zero. This has no effect apart > from suppressing the core's message. The platform device gets unbound > irrespective of the return value. > > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de> > --- > drivers/remoteproc/ti_k3_dsp_remoteproc.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/drivers/remoteproc/ti_k3_dsp_remoteproc.c b/drivers/remoteproc/ti_k3_dsp_remoteproc.c > index ef8415a7cd54..40a5fd8763fa 100644 > --- a/drivers/remoteproc/ti_k3_dsp_remoteproc.c > +++ b/drivers/remoteproc/ti_k3_dsp_remoteproc.c > @@ -835,8 +835,9 @@ static int k3_dsp_rproc_remove(struct platform_device *pdev) > if (rproc->state == RPROC_ATTACHED) { > ret = rproc_detach(rproc); > if (ret) { > + /* Note this error path leaks resources */ > dev_err(dev, "failed to detach proc, ret = %d\n", ret); > - return ret; > + return 0; Please have a look at this error path. As with the scenario where the remote processor is controlled by the remoteproc core, nothing can be done in .remove() to prevent the driver from going away. As such and even if rproc_detach() fails, other resources associated with this remote processor should be cleaned-up. Thanks, Mathieu > } > } > > -- > 2.42.0 >
Hello, On Thu, Nov 30, 2023 at 10:19:42AM -0700, Mathieu Poirier wrote: > Hari and Nishanth, > > On Thu, 23 Nov 2023 at 14:17, Uwe Kleine-König > <u.kleine-koenig@pengutronix.de> wrote: > > > > When the remove callback returns non-zero, the driver core emits an > > error message about the error value being ignored. As the driver already > > emits an error message already, return zero. This has no effect apart > > from suppressing the core's message. The platform device gets unbound > > irrespective of the return value. > > > > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de> > > --- > > drivers/remoteproc/ti_k3_dsp_remoteproc.c | 3 ++- > > 1 file changed, 2 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/remoteproc/ti_k3_dsp_remoteproc.c b/drivers/remoteproc/ti_k3_dsp_remoteproc.c > > index ef8415a7cd54..40a5fd8763fa 100644 > > --- a/drivers/remoteproc/ti_k3_dsp_remoteproc.c > > +++ b/drivers/remoteproc/ti_k3_dsp_remoteproc.c > > @@ -835,8 +835,9 @@ static int k3_dsp_rproc_remove(struct platform_device *pdev) > > if (rproc->state == RPROC_ATTACHED) { > > ret = rproc_detach(rproc); > > if (ret) { > > + /* Note this error path leaks resources */ > > dev_err(dev, "failed to detach proc, ret = %d\n", ret); > > - return ret; > > + return 0; > > Please have a look at this error path. As with the scenario where the > remote processor is controlled by the remoteproc core, nothing can be > done in .remove() to prevent the driver from going away. As such and > even if rproc_detach() fails, other resources associated with this > remote processor should be cleaned-up. Without having done a deep dive into the driver and the remoteproc core I think the remoteproc core should provide a function that deregisters the software representation of a rproc device and returns void. If you look at rproc_detach, that can even fail if it doesn't get the mutex. So I'm convinced there is something to do on the framework level before removing the ti_k3_dsp_remoteproc driver can be done without leaking stuff. Best regards Uwe
Hi Mathieu, Uwe, On 11/30/23 11:19, Mathieu Poirier wrote: >> --- a/drivers/remoteproc/ti_k3_dsp_remoteproc.c >> +++ b/drivers/remoteproc/ti_k3_dsp_remoteproc.c >> @@ -835,8 +835,9 @@ static int k3_dsp_rproc_remove(struct platform_device *pdev) >> if (rproc->state == RPROC_ATTACHED) { >> ret = rproc_detach(rproc); >> if (ret) { >> + /* Note this error path leaks resources */the >> dev_err(dev, "failed to detach proc, ret = %d\n", ret); >> - return ret; >> + return 0; > Please have a look at this error path. As with the scenario where the > remote processor is controlled by the remoteproc core, nothing can be > done in .remove() to prevent the driver from going away. As such and > even if rproc_detach() fails, other resources associated with this > remote processor should be cleaned-up. Since, anyway the driver goes away we probably need a force cleanup of the resources even if 'rproc_detach' fails. Looking a bit into the remote core driver, the detach can fail if it fails to get mutex lock or unable to reset resource table. It appears to me failure of reset resource table is remote IMO, we can throw an error message when 'rproc_detach' fails in 'dsp_rproc_remove' and proceed with the rest of the resource clean up,i.e call 'rproc_del()', followed by calls to ti device manager to relinquish the remote dsp processor. Best, Hari Nagalla
diff --git a/drivers/remoteproc/ti_k3_dsp_remoteproc.c b/drivers/remoteproc/ti_k3_dsp_remoteproc.c index ef8415a7cd54..40a5fd8763fa 100644 --- a/drivers/remoteproc/ti_k3_dsp_remoteproc.c +++ b/drivers/remoteproc/ti_k3_dsp_remoteproc.c @@ -835,8 +835,9 @@ static int k3_dsp_rproc_remove(struct platform_device *pdev) if (rproc->state == RPROC_ATTACHED) { ret = rproc_detach(rproc); if (ret) { + /* Note this error path leaks resources */ dev_err(dev, "failed to detach proc, ret = %d\n", ret); - return ret; + return 0; } }
When the remove callback returns non-zero, the driver core emits an error message about the error value being ignored. As the driver already emits an error message already, return zero. This has no effect apart from suppressing the core's message. The platform device gets unbound irrespective of the return value. Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de> --- drivers/remoteproc/ti_k3_dsp_remoteproc.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)