| Message ID | bea9842e-6910-4452-8f90-cb7482f3e76f@web.de (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | remoteproc: qcom: q6v5-mss: Use common error handling code in q6v5_mpss_load() | expand |
On Tue, Sep 24, 2024 at 04:08:11PM GMT, Markus Elfring wrote: > From: Markus Elfring <elfring@users.sourceforge.net> > Date: Tue, 24 Sep 2024 15:55:06 +0200 > > Add jump targets so that a bit of exception handling can be better reused > at the end of this function implementation. > > Signed-off-by: Markus Elfring <elfring@users.sourceforge.net> > --- > drivers/remoteproc/qcom_q6v5_mss.c | 17 +++++++++-------- > 1 file changed, 9 insertions(+), 8 deletions(-) > > diff --git a/drivers/remoteproc/qcom_q6v5_mss.c b/drivers/remoteproc/qcom_q6v5_mss.c > index 2a42215ce8e0..b398ae3083a1 100644 > --- a/drivers/remoteproc/qcom_q6v5_mss.c > +++ b/drivers/remoteproc/qcom_q6v5_mss.c > @@ -1451,9 +1451,7 @@ static int q6v5_mpss_load(struct q6v5 *qproc) > dev_err(qproc->dev, > "failed to load segment %d from truncated file %s\n", > i, fw_name); > - ret = -EINVAL; Please keep error assignment where it is. It is much cleaner to read it this way, rather than checking the error-handling basement. > - memunmap(ptr); > - goto release_firmware; > + goto e_inval_unmap; > } > > memcpy(ptr, fw->data + phdr->p_offset, phdr->p_filesz); > @@ -1464,18 +1462,15 @@ static int q6v5_mpss_load(struct q6v5 *qproc) > ptr, phdr->p_filesz); > if (ret) { > dev_err(qproc->dev, "failed to load %s\n", fw_name); > - memunmap(ptr); > - goto release_firmware; > + goto unmap_mem; > } > > if (seg_fw->size != phdr->p_filesz) { > dev_err(qproc->dev, > "failed to load segment %d from truncated file %s\n", > i, fw_name); > - ret = -EINVAL; > release_firmware(seg_fw); > - memunmap(ptr); > - goto release_firmware; > + goto e_inval_unmap; > } > > release_firmware(seg_fw); > @@ -1528,6 +1523,12 @@ static int q6v5_mpss_load(struct q6v5 *qproc) > kfree(fw_name); > > return ret < 0 ? ret : 0; > + > +e_inval_unmap: > + ret = -EINVAL; > +unmap_mem: > + memunmap(ptr); > + goto release_firmware; Ugh. No. ptr should be a variable that is declared inside the loop. Calling memunmap outside of the for loop is incorrect. And goto just complicates things by adding non-linearity. > } > > static void qcom_q6v5_dump_segment(struct rproc *rproc, > -- > 2.46.1 >
diff --git a/drivers/remoteproc/qcom_q6v5_mss.c b/drivers/remoteproc/qcom_q6v5_mss.c index 2a42215ce8e0..b398ae3083a1 100644 --- a/drivers/remoteproc/qcom_q6v5_mss.c +++ b/drivers/remoteproc/qcom_q6v5_mss.c @@ -1451,9 +1451,7 @@ static int q6v5_mpss_load(struct q6v5 *qproc) dev_err(qproc->dev, "failed to load segment %d from truncated file %s\n", i, fw_name); - ret = -EINVAL; - memunmap(ptr); - goto release_firmware; + goto e_inval_unmap; } memcpy(ptr, fw->data + phdr->p_offset, phdr->p_filesz); @@ -1464,18 +1462,15 @@ static int q6v5_mpss_load(struct q6v5 *qproc) ptr, phdr->p_filesz); if (ret) { dev_err(qproc->dev, "failed to load %s\n", fw_name); - memunmap(ptr); - goto release_firmware; + goto unmap_mem; } if (seg_fw->size != phdr->p_filesz) { dev_err(qproc->dev, "failed to load segment %d from truncated file %s\n", i, fw_name); - ret = -EINVAL; release_firmware(seg_fw); - memunmap(ptr); - goto release_firmware; + goto e_inval_unmap; } release_firmware(seg_fw); @@ -1528,6 +1523,12 @@ static int q6v5_mpss_load(struct q6v5 *qproc) kfree(fw_name); return ret < 0 ? ret : 0; + +e_inval_unmap: + ret = -EINVAL; +unmap_mem: + memunmap(ptr); + goto release_firmware; } static void qcom_q6v5_dump_segment(struct rproc *rproc,