| Message ID | 9485e427a9041cc76cfd3dbcc34874af495e160a.1725653543.git.christophe.jaillet@wanadoo.fr (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v2] remoteproc: k3-dsp: Fix an error handling path in k3_dsp_rproc_probe() | expand |
On Sat, Sep 07, 2024 at 08:33:36AM +0200, Christophe JAILLET wrote: > If an error occurs after the k3_dsp_rproc_request_mbox() call, > mbox_free_channel() must be called, as already done in the remove function. > > Instead of adding an error handling path in the probe and changing all > error handling in the function, add a new devm_add_action_or_reset() and > simplify the .remove() function. > > Fixes: ea1d6fb5b571 ("remoteproc: k3-dsp: Acquire mailbox handle during probe routine") > Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr> > Reviewed-by: Andrew Davis <afd@ti.com> > --- > Compile tested only > > Change in v2: > - fix the subject (cut'n'paste issue) [Andrew Davis] > - add R-b tag > > v1: https://lore.kernel.org/all/9485e127a00419c76cf13dbccf4874af395ef6ba.1725653543.git.christophe.jaillet@wanadoo.fr/ > --- > drivers/remoteproc/ti_k3_dsp_remoteproc.c | 13 +++++++++++-- > 1 file changed, 11 insertions(+), 2 deletions(-) > > diff --git a/drivers/remoteproc/ti_k3_dsp_remoteproc.c b/drivers/remoteproc/ti_k3_dsp_remoteproc.c > index 8be3f631c192..f29780de37a5 100644 > --- a/drivers/remoteproc/ti_k3_dsp_remoteproc.c > +++ b/drivers/remoteproc/ti_k3_dsp_remoteproc.c > @@ -610,6 +610,13 @@ static void k3_dsp_release_tsp(void *data) > ti_sci_proc_release(tsp); > } > > +static void k3_dsp_free_channel(void *data) > +{ > + struct k3_dsp_rproc *kproc = data; How did the struct rproc from devm_add_action_or_reset() got turned into a struct k3_dsp_rproc? > + > + mbox_free_channel(kproc->mbox); > +} > + > static int k3_dsp_rproc_probe(struct platform_device *pdev) > { > struct device *dev = &pdev->dev; > @@ -649,6 +656,10 @@ static int k3_dsp_rproc_probe(struct platform_device *pdev) > if (ret) > return ret; > > + ret = devm_add_action_or_reset(dev, k3_dsp_free_channel, rproc); > + if (ret) > + return ret; > + > kproc->ti_sci = devm_ti_sci_get_by_phandle(dev, "ti,sci"); > if (IS_ERR(kproc->ti_sci)) > return dev_err_probe(dev, PTR_ERR(kproc->ti_sci), > @@ -741,8 +752,6 @@ static void k3_dsp_rproc_remove(struct platform_device *pdev) > if (ret) > dev_err(dev, "failed to detach proc (%pe)\n", ERR_PTR(ret)); > } > - > - mbox_free_channel(kproc->mbox); > } > > static const struct k3_dsp_mem_data c66_mems[] = { > -- > 2.46.0 >
Le 10/09/2024 à 17:29, Mathieu Poirier a écrit : >> +static void k3_dsp_free_channel(void *data) >> +{ >> + struct k3_dsp_rproc *kproc = data; > > How did the struct rproc from devm_add_action_or_reset() got turned into a > struct k3_dsp_rproc? > Well, Linux is a wonderful system, that is able to make wonderful thinks! In this particular case, if it is not the correct explanation, it is likely an unfortunate typo :( Sorry about that, and thanks for the careful review. I'll send a v3. But, looking at it, and trying to see if similar issues may exist, the following naive script spots 2 similar issues. 1 of them looks valid. I'll try to improve it (Julia in copy, if she wants to give it a look as-well :)) CJ @devm@ expression RET, DEV; identifier FCT, VAR; type T; @@ T VAR; ... ( RET = devm_add_action_or_reset(DEV, FCT, VAR); | devm_add_action_or_reset(DEV, FCT, VAR); ) @depends on devm@ identifier FCT = devm.FCT, x, VAR; type T1 = devm.T, T2; @@ void FCT(void *x) { ... ( T1 VAR = x; | * T2 VAR = x; ) ... }
diff --git a/drivers/remoteproc/ti_k3_dsp_remoteproc.c b/drivers/remoteproc/ti_k3_dsp_remoteproc.c index 8be3f631c192..f29780de37a5 100644 --- a/drivers/remoteproc/ti_k3_dsp_remoteproc.c +++ b/drivers/remoteproc/ti_k3_dsp_remoteproc.c @@ -610,6 +610,13 @@ static void k3_dsp_release_tsp(void *data) ti_sci_proc_release(tsp); } +static void k3_dsp_free_channel(void *data) +{ + struct k3_dsp_rproc *kproc = data; + + mbox_free_channel(kproc->mbox); +} + static int k3_dsp_rproc_probe(struct platform_device *pdev) { struct device *dev = &pdev->dev; @@ -649,6 +656,10 @@ static int k3_dsp_rproc_probe(struct platform_device *pdev) if (ret) return ret; + ret = devm_add_action_or_reset(dev, k3_dsp_free_channel, rproc); + if (ret) + return ret; + kproc->ti_sci = devm_ti_sci_get_by_phandle(dev, "ti,sci"); if (IS_ERR(kproc->ti_sci)) return dev_err_probe(dev, PTR_ERR(kproc->ti_sci), @@ -741,8 +752,6 @@ static void k3_dsp_rproc_remove(struct platform_device *pdev) if (ret) dev_err(dev, "failed to detach proc (%pe)\n", ERR_PTR(ret)); } - - mbox_free_channel(kproc->mbox); } static const struct k3_dsp_mem_data c66_mems[] = {
If an error occurs after the k3_dsp_rproc_request_mbox() call, mbox_free_channel() must be called, as already done in the remove function. Instead of adding an error handling path in the probe and changing all error handling in the function, add a new devm_add_action_or_reset() and simplify the .remove() function. Fixes: ea1d6fb5b571 ("remoteproc: k3-dsp: Acquire mailbox handle during probe routine") Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr> Reviewed-by: Andrew Davis <afd@ti.com> --- Compile tested only Change in v2: - fix the subject (cut'n'paste issue) [Andrew Davis] - add R-b tag v1: https://lore.kernel.org/all/9485e127a00419c76cf13dbccf4874af395ef6ba.1725653543.git.christophe.jaillet@wanadoo.fr/ --- drivers/remoteproc/ti_k3_dsp_remoteproc.c | 13 +++++++++++-- 1 file changed, 11 insertions(+), 2 deletions(-)