| Message ID | 20200424200135.28825-3-mathieu.poirier@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | remoteproc: Add support for synchronisaton with rproc | expand |
On Fri 24 Apr 13:01 PDT 2020, Mathieu Poirier wrote: > In scenarios where the remote processor's lifecycle is entirely > managed by another entity there is no point in allocating memory for > a firmware name since it will never be used. The same goes for a core > set of operations. > > As such introduce function rproc_alloc_internals() to decide if the > allocation of a firmware name and the core operations need to be done. > That way rproc_alloc() can be kept as clean as possible. > > Signed-off-by: Mathieu Poirier <mathieu.poirier@linaro.org> > --- > drivers/remoteproc/remoteproc_core.c | 31 +++++++++++++++++++++++----- > 1 file changed, 26 insertions(+), 5 deletions(-) > > diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c > index 448262470fc7..1b4756909584 100644 > --- a/drivers/remoteproc/remoteproc_core.c > +++ b/drivers/remoteproc/remoteproc_core.c > @@ -2076,6 +2076,30 @@ static int rproc_alloc_ops(struct rproc *rproc, const struct rproc_ops *ops) > return 0; > } > > +static int rproc_alloc_internals(struct rproc *rproc, > + const struct rproc_ops *ops, > + const char *name, const char *firmware) > +{ > + int ret; > + > + /* > + * In scenarios where the remote processor's lifecycle is entirely > + * managed by another entity there is no point in carrying a set > + * of operations that will never be used. > + * > + * And since no firmware will ever be loaded, there is no point in > + * allocating memory for it either. While this is true, I would expect that there are cases where the remoteproc has ops but no firmware. How about splitting this decision already now; i.e. moving the if(!ops) to rproc_alloc_ops() and perhaps only allocate firmware if ops->load is specified? Regards, Bjorn > + */ > + if (!ops) > + return 0; > + > + ret = rproc_alloc_firmware(rproc, name, firmware); > + if (ret) > + return ret; > + > + return rproc_alloc_ops(rproc, ops); > +} > + > /** > * rproc_alloc() - allocate a remote processor handle > * @dev: the underlying device > @@ -2105,7 +2129,7 @@ struct rproc *rproc_alloc(struct device *dev, const char *name, > { > struct rproc *rproc; > > - if (!dev || !name || !ops) > + if (!dev || !name) > return NULL; > > rproc = kzalloc(sizeof(struct rproc) + len, GFP_KERNEL); > @@ -2128,10 +2152,7 @@ struct rproc *rproc_alloc(struct device *dev, const char *name, > if (!rproc->name) > goto put_device; > > - if (rproc_alloc_firmware(rproc, name, firmware)) > - goto put_device; > - > - if (rproc_alloc_ops(rproc, ops)) > + if (rproc_alloc_internals(rproc, ops, name, firmware)) > goto put_device; > > /* Assign a unique device index and name */ > -- > 2.20.1 >
On Tue, May 05, 2020 at 03:31:58PM -0700, Bjorn Andersson wrote: > On Fri 24 Apr 13:01 PDT 2020, Mathieu Poirier wrote: > > > In scenarios where the remote processor's lifecycle is entirely > > managed by another entity there is no point in allocating memory for > > a firmware name since it will never be used. The same goes for a core > > set of operations. > > > > As such introduce function rproc_alloc_internals() to decide if the > > allocation of a firmware name and the core operations need to be done. > > That way rproc_alloc() can be kept as clean as possible. > > > > Signed-off-by: Mathieu Poirier <mathieu.poirier@linaro.org> > > --- > > drivers/remoteproc/remoteproc_core.c | 31 +++++++++++++++++++++++----- > > 1 file changed, 26 insertions(+), 5 deletions(-) > > > > diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c > > index 448262470fc7..1b4756909584 100644 > > --- a/drivers/remoteproc/remoteproc_core.c > > +++ b/drivers/remoteproc/remoteproc_core.c > > @@ -2076,6 +2076,30 @@ static int rproc_alloc_ops(struct rproc *rproc, const struct rproc_ops *ops) > > return 0; > > } > > > > +static int rproc_alloc_internals(struct rproc *rproc, > > + const struct rproc_ops *ops, > > + const char *name, const char *firmware) > > +{ > > + int ret; > > + > > + /* > > + * In scenarios where the remote processor's lifecycle is entirely > > + * managed by another entity there is no point in carrying a set > > + * of operations that will never be used. > > + * > > + * And since no firmware will ever be loaded, there is no point in > > + * allocating memory for it either. > > While this is true, I would expect that there are cases where the > remoteproc has ops but no firmware. > That is a scenario I did not envisioned, but I agree, the remote processor could be fetching from a private ROM memory and still required handling from the remoteproc core. > How about splitting this decision already now; i.e. moving the if(!ops) > to rproc_alloc_ops() and perhaps only allocate firmware if ops->load is > specified? > Or just add "if (ops->load)" before calling rproc_alloc_firmware()... Otherwise we need to change the calling order of rproc_alloc_firmware() and rproc_alloc_ops() in order to make sure 'ops' is valid when calling the former. Either way I'll add a comment with the rationale you have detailed above. > Regards, > Bjorn > > > + */ > > + if (!ops) > > + return 0; > > + > > + ret = rproc_alloc_firmware(rproc, name, firmware); > > + if (ret) > > + return ret; > > + > > + return rproc_alloc_ops(rproc, ops); > > +} > > + > > /** > > * rproc_alloc() - allocate a remote processor handle > > * @dev: the underlying device > > @@ -2105,7 +2129,7 @@ struct rproc *rproc_alloc(struct device *dev, const char *name, > > { > > struct rproc *rproc; > > > > - if (!dev || !name || !ops) > > + if (!dev || !name) > > return NULL; > > > > rproc = kzalloc(sizeof(struct rproc) + len, GFP_KERNEL); > > @@ -2128,10 +2152,7 @@ struct rproc *rproc_alloc(struct device *dev, const char *name, > > if (!rproc->name) > > goto put_device; > > > > - if (rproc_alloc_firmware(rproc, name, firmware)) > > - goto put_device; > > - > > - if (rproc_alloc_ops(rproc, ops)) > > + if (rproc_alloc_internals(rproc, ops, name, firmware)) > > goto put_device; > > > > /* Assign a unique device index and name */ > > -- > > 2.20.1 > >
diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c index 448262470fc7..1b4756909584 100644 --- a/drivers/remoteproc/remoteproc_core.c +++ b/drivers/remoteproc/remoteproc_core.c @@ -2076,6 +2076,30 @@ static int rproc_alloc_ops(struct rproc *rproc, const struct rproc_ops *ops) return 0; } +static int rproc_alloc_internals(struct rproc *rproc, + const struct rproc_ops *ops, + const char *name, const char *firmware) +{ + int ret; + + /* + * In scenarios where the remote processor's lifecycle is entirely + * managed by another entity there is no point in carrying a set + * of operations that will never be used. + * + * And since no firmware will ever be loaded, there is no point in + * allocating memory for it either. + */ + if (!ops) + return 0; + + ret = rproc_alloc_firmware(rproc, name, firmware); + if (ret) + return ret; + + return rproc_alloc_ops(rproc, ops); +} + /** * rproc_alloc() - allocate a remote processor handle * @dev: the underlying device @@ -2105,7 +2129,7 @@ struct rproc *rproc_alloc(struct device *dev, const char *name, { struct rproc *rproc; - if (!dev || !name || !ops) + if (!dev || !name) return NULL; rproc = kzalloc(sizeof(struct rproc) + len, GFP_KERNEL); @@ -2128,10 +2152,7 @@ struct rproc *rproc_alloc(struct device *dev, const char *name, if (!rproc->name) goto put_device; - if (rproc_alloc_firmware(rproc, name, firmware)) - goto put_device; - - if (rproc_alloc_ops(rproc, ops)) + if (rproc_alloc_internals(rproc, ops, name, firmware)) goto put_device; /* Assign a unique device index and name */
In scenarios where the remote processor's lifecycle is entirely managed by another entity there is no point in allocating memory for a firmware name since it will never be used. The same goes for a core set of operations. As such introduce function rproc_alloc_internals() to decide if the allocation of a firmware name and the core operations need to be done. That way rproc_alloc() can be kept as clean as possible. Signed-off-by: Mathieu Poirier <mathieu.poirier@linaro.org> --- drivers/remoteproc/remoteproc_core.c | 31 +++++++++++++++++++++++----- 1 file changed, 26 insertions(+), 5 deletions(-)