| Message ID | 20241204111130.2218497-4-b-padhi@ti.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Use Device Lifecycle managed functions in TI R5 Remoteproc | expand |
On 12/4/24 5:11 AM, Beleswar Padhi wrote: > Use a device lifecycle managed action to release tsp ti_sci_proc handle. > This helps prevent mistakes like releasing out of order in cleanup > functions and forgetting to release on error paths. > > Signed-off-by: Beleswar Padhi <b-padhi@ti.com> > --- > drivers/remoteproc/ti_k3_r5_remoteproc.c | 17 +++++++++++------ > 1 file changed, 11 insertions(+), 6 deletions(-) > > diff --git a/drivers/remoteproc/ti_k3_r5_remoteproc.c b/drivers/remoteproc/ti_k3_r5_remoteproc.c > index 0753a5c35c7e..2966cb210403 100644 > --- a/drivers/remoteproc/ti_k3_r5_remoteproc.c > +++ b/drivers/remoteproc/ti_k3_r5_remoteproc.c > @@ -1513,6 +1513,13 @@ static int k3_r5_core_of_get_sram_memories(struct platform_device *pdev, > return 0; > } > > +static void k3_r5_release_tsp(void *data) > +{ > + struct ti_sci_proc *tsp = data; > + > + ti_sci_proc_release(tsp); > +} > + > static int k3_r5_core_of_init(struct platform_device *pdev) > { > struct device *dev = &pdev->dev; > @@ -1606,6 +1613,10 @@ static int k3_r5_core_of_init(struct platform_device *pdev) > goto err; > } > > + ret = devm_add_action_or_reset(dev, k3_r5_release_tsp, core->tsp); > + if (ret) > + goto err; > + > platform_set_drvdata(pdev, core); > devres_close_group(dev, k3_r5_core_of_init); > > @@ -1622,13 +1633,7 @@ static int k3_r5_core_of_init(struct platform_device *pdev) > */ > static void k3_r5_core_of_exit(struct platform_device *pdev) > { > - struct k3_r5_core *core = platform_get_drvdata(pdev); > struct device *dev = &pdev->dev; > - int ret; > - > - ret = ti_sci_proc_release(core->tsp); > - if (ret) > - dev_err(dev, "failed to release proc, ret = %d\n", ret); > One thing to remember is devm unrolling happens after remove(). So here you are changing the order things happen. ti_sci_proc_release() now will get called after the below functions. This most likely isn't wrong, but to make review easier it helps to start from the last called function in remove() and work backwards so nothing is reordered. Andrew > platform_set_drvdata(pdev, NULL); > devres_release_group(dev, k3_r5_core_of_init);
Hi Andrew, On 17/12/24 21:30, Andrew Davis wrote: > On 12/4/24 5:11 AM, Beleswar Padhi wrote: >> Use a device lifecycle managed action to release tsp ti_sci_proc handle. >> This helps prevent mistakes like releasing out of order in cleanup >> functions and forgetting to release on error paths. >> >> Signed-off-by: Beleswar Padhi <b-padhi@ti.com> >> --- >> drivers/remoteproc/ti_k3_r5_remoteproc.c | 17 +++++++++++------ >> 1 file changed, 11 insertions(+), 6 deletions(-) >> >> diff --git a/drivers/remoteproc/ti_k3_r5_remoteproc.c >> b/drivers/remoteproc/ti_k3_r5_remoteproc.c >> index 0753a5c35c7e..2966cb210403 100644 >> --- a/drivers/remoteproc/ti_k3_r5_remoteproc.c >> +++ b/drivers/remoteproc/ti_k3_r5_remoteproc.c >> @@ -1513,6 +1513,13 @@ static int >> k3_r5_core_of_get_sram_memories(struct platform_device *pdev, >> return 0; >> } >> +static void k3_r5_release_tsp(void *data) >> +{ >> + struct ti_sci_proc *tsp = data; >> + >> + ti_sci_proc_release(tsp); >> +} >> + >> static int k3_r5_core_of_init(struct platform_device *pdev) >> { >> struct device *dev = &pdev->dev; >> @@ -1606,6 +1613,10 @@ static int k3_r5_core_of_init(struct >> platform_device *pdev) >> goto err; >> } >> + ret = devm_add_action_or_reset(dev, k3_r5_release_tsp, >> core->tsp); >> + if (ret) >> + goto err; >> + >> platform_set_drvdata(pdev, core); >> devres_close_group(dev, k3_r5_core_of_init); >> @@ -1622,13 +1633,7 @@ static int k3_r5_core_of_init(struct >> platform_device *pdev) >> */ >> static void k3_r5_core_of_exit(struct platform_device *pdev) >> { >> - struct k3_r5_core *core = platform_get_drvdata(pdev); >> struct device *dev = &pdev->dev; >> - int ret; >> - >> - ret = ti_sci_proc_release(core->tsp); >> - if (ret) >> - dev_err(dev, "failed to release proc, ret = %d\n", ret); > > One thing to remember is devm unrolling happens after remove(). So > here you are changing the order things happen. ti_sci_proc_release() > now will get called after the below functions. This most likely > isn't wrong, but to make review easier it helps to start from the > last called function in remove() and work backwards so nothing > is reordered. That's a great insight! Will send out v2 following this order. Thanks, Beleswar > > Andrew > >> platform_set_drvdata(pdev, NULL); >> devres_release_group(dev, k3_r5_core_of_init);
diff --git a/drivers/remoteproc/ti_k3_r5_remoteproc.c b/drivers/remoteproc/ti_k3_r5_remoteproc.c index 0753a5c35c7e..2966cb210403 100644 --- a/drivers/remoteproc/ti_k3_r5_remoteproc.c +++ b/drivers/remoteproc/ti_k3_r5_remoteproc.c @@ -1513,6 +1513,13 @@ static int k3_r5_core_of_get_sram_memories(struct platform_device *pdev, return 0; } +static void k3_r5_release_tsp(void *data) +{ + struct ti_sci_proc *tsp = data; + + ti_sci_proc_release(tsp); +} + static int k3_r5_core_of_init(struct platform_device *pdev) { struct device *dev = &pdev->dev; @@ -1606,6 +1613,10 @@ static int k3_r5_core_of_init(struct platform_device *pdev) goto err; } + ret = devm_add_action_or_reset(dev, k3_r5_release_tsp, core->tsp); + if (ret) + goto err; + platform_set_drvdata(pdev, core); devres_close_group(dev, k3_r5_core_of_init); @@ -1622,13 +1633,7 @@ static int k3_r5_core_of_init(struct platform_device *pdev) */ static void k3_r5_core_of_exit(struct platform_device *pdev) { - struct k3_r5_core *core = platform_get_drvdata(pdev); struct device *dev = &pdev->dev; - int ret; - - ret = ti_sci_proc_release(core->tsp); - if (ret) - dev_err(dev, "failed to release proc, ret = %d\n", ret); platform_set_drvdata(pdev, NULL); devres_release_group(dev, k3_r5_core_of_init);
Use a device lifecycle managed action to release tsp ti_sci_proc handle. This helps prevent mistakes like releasing out of order in cleanup functions and forgetting to release on error paths. Signed-off-by: Beleswar Padhi <b-padhi@ti.com> --- drivers/remoteproc/ti_k3_r5_remoteproc.c | 17 +++++++++++------ 1 file changed, 11 insertions(+), 6 deletions(-)