Re: [PATCH 7/8] remoteproc: Drop dangling find_rsc_table dummies

From: Bjorn Andersson
Date: Fri Jan 05 2018 - 13:50:59 EST

On Fri 05 Jan 08:53 PST 2018, Loic PALLARDY wrote:

>
>
> > -----Original Message-----
> > From: linux-remoteproc-owner@xxxxxxxxxxxxxxx [mailto:linux-remoteproc-
> > owner@xxxxxxxxxxxxxxx] On Behalf Of Bjorn Andersson
> > Sent: Wednesday, December 13, 2017 11:41 PM
> > To: Ohad Ben-Cohen <ohad@xxxxxxxxxx>; Bjorn Andersson
> > <bjorn.andersson@xxxxxxxxxx>
> > Cc: linux-remoteproc@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx
> > Subject: [PATCH 7/8] remoteproc: Drop dangling find_rsc_table dummies
> >
> > As the core now deals with the lack of a resource table, remove the
> > dangling custom dummy implementations of find_rsc_table from drivers.
> >
> > Signed-off-by: Bjorn Andersson <bjorn.andersson@xxxxxxxxxx>
> > ---
> > drivers/remoteproc/qcom_adsp_pil.c | 1 -
> > drivers/remoteproc/qcom_common.c | 19 -------------------
> > drivers/remoteproc/qcom_common.h | 4 ----
> > drivers/remoteproc/qcom_q6v5_pil.c | 11 -----------
> > drivers/remoteproc/qcom_wcnss.c | 1 -
> > drivers/remoteproc/st_slim_rproc.c | 18 ------------------
> > include/linux/remoteproc.h | 4 ----
> > 7 files changed, 58 deletions(-)
> >
> > diff --git a/drivers/remoteproc/qcom_adsp_pil.c
> > b/drivers/remoteproc/qcom_adsp_pil.c
> > index 7b9d810b23f1..b0b0d5ca1ca0 100644
> > --- a/drivers/remoteproc/qcom_adsp_pil.c
> > +++ b/drivers/remoteproc/qcom_adsp_pil.c
> > @@ -181,7 +181,6 @@ static const struct rproc_ops adsp_ops = {
> > .start = adsp_start,
> > .stop = adsp_stop,
> > .da_to_va = adsp_da_to_va,
> > - .find_rsc_table = qcom_mdt_find_rsc_table,
> > .load = adsp_load,
> > };
> >
> > diff --git a/drivers/remoteproc/qcom_common.c
> > b/drivers/remoteproc/qcom_common.c
> > index 818ee3657043..ce2dcc4f7de7 100644
> > --- a/drivers/remoteproc/qcom_common.c
> > +++ b/drivers/remoteproc/qcom_common.c
> > @@ -32,25 +32,6 @@
> >
> > static BLOCKING_NOTIFIER_HEAD(ssr_notifiers);
> >
> > -/**
> > - * qcom_mdt_find_rsc_table() - provide dummy resource table for
> > remoteproc
> > - * @rproc: remoteproc handle
> > - * @fw: firmware header
> > - * @tablesz: outgoing size of the table
> > - *
> > - * Returns a dummy table.
> > - */
> > -struct resource_table *qcom_mdt_find_rsc_table(struct rproc *rproc,
> > - const struct firmware *fw,
> > - int *tablesz)
> > -{
> > - static struct resource_table table = { .ver = 1, };
> > -
> > - *tablesz = sizeof(table);
> > - return &table;
> > -}
> > -EXPORT_SYMBOL_GPL(qcom_mdt_find_rsc_table);
> > -
> > static int glink_subdev_probe(struct rproc_subdev *subdev)
> > {
> > struct qcom_rproc_glink *glink = to_glink_subdev(subdev);
> > diff --git a/drivers/remoteproc/qcom_common.h
> > b/drivers/remoteproc/qcom_common.h
> > index 541586e528b3..73efed969bfd 100644
> > --- a/drivers/remoteproc/qcom_common.h
> > +++ b/drivers/remoteproc/qcom_common.h
> > @@ -30,10 +30,6 @@ struct qcom_rproc_ssr {
> > const char *name;
> > };
> >
> > -struct resource_table *qcom_mdt_find_rsc_table(struct rproc *rproc,
> > - const struct firmware *fw,
> > - int *tablesz);
> > -
> > void qcom_add_glink_subdev(struct rproc *rproc, struct qcom_rproc_glink
> > *glink);
> > void qcom_remove_glink_subdev(struct rproc *rproc, struct
> > qcom_rproc_glink *glink);
> >
> > diff --git a/drivers/remoteproc/qcom_q6v5_pil.c
> > b/drivers/remoteproc/qcom_q6v5_pil.c
> > index fbff5d842581..6f6ea0414366 100644
> > --- a/drivers/remoteproc/qcom_q6v5_pil.c
> > +++ b/drivers/remoteproc/qcom_q6v5_pil.c
> > @@ -304,16 +304,6 @@ static void q6v5_clk_disable(struct device *dev,
> > clk_disable_unprepare(clks[i]);
> > }
> >
> > -static struct resource_table *q6v5_find_rsc_table(struct rproc *rproc,
> > - const struct firmware *fw,
> > - int *tablesz)
> > -{
> > - static struct resource_table table = { .ver = 1, };
> > -
> > - *tablesz = sizeof(table);
> > - return &table;
> > -}
> > -
> > static int q6v5_xfer_mem_ownership(struct q6v5 *qproc, int
> > *current_perm,
> > bool remote_owner, phys_addr_t addr,
> > size_t size)
> > @@ -927,7 +917,6 @@ static const struct rproc_ops q6v5_ops = {
> > .start = q6v5_start,
> > .stop = q6v5_stop,
> > .da_to_va = q6v5_da_to_va,
> > - .find_rsc_table = q6v5_find_rsc_table,
> > .load = q6v5_load,
> > };
> >
> > diff --git a/drivers/remoteproc/qcom_wcnss.c
> > b/drivers/remoteproc/qcom_wcnss.c
> > index cc44ec598522..1fa5253020dd 100644
> > --- a/drivers/remoteproc/qcom_wcnss.c
> > +++ b/drivers/remoteproc/qcom_wcnss.c
> > @@ -310,7 +310,6 @@ static const struct rproc_ops wcnss_ops = {
> > .start = wcnss_start,
> > .stop = wcnss_stop,
> > .da_to_va = wcnss_da_to_va,
> > - .find_rsc_table = qcom_mdt_find_rsc_table,
> > .load = wcnss_load,
> > };
> >
> > diff --git a/drivers/remoteproc/st_slim_rproc.c
> > b/drivers/remoteproc/st_slim_rproc.c
> > index 1538ea915c49..c6a2a8b68c7a 100644
> > --- a/drivers/remoteproc/st_slim_rproc.c
> > +++ b/drivers/remoteproc/st_slim_rproc.c
> > @@ -200,28 +200,10 @@ static void *slim_rproc_da_to_va(struct rproc
> > *rproc, u64 da, int len)
> > return va;
> > }
> >
> > -/*
> > - * Firmware handler operations: sanity, boot address, load ...
> > - */
> > -
> > -static struct resource_table empty_rsc_tbl = {
> > - .ver = 1,
> > - .num = 0,
> > -};
> > -
> > -static struct resource_table *slim_rproc_find_rsc_table(struct rproc *rproc,
> > - const struct firmware *fw,
> > - int *tablesz)
> > -{
> > - *tablesz = sizeof(empty_rsc_tbl);
> > - return &empty_rsc_tbl;
> > -}
> > -
> > static const struct rproc_ops slim_rproc_ops = {
> > .start = slim_rproc_start,
> > .stop = slim_rproc_stop,
> > .da_to_va = slim_rproc_da_to_va,
> > - .find_rsc_table = slim_rproc_find_rsc_table,
> Hi Bjorn,
> Your patch is not complete for st_slim_rproc and so not working.
> In your patch 6/8, .load_rsc_table is define to default rproc_elf_load_rsc_table if no load ops defined.
>
> /* Default to ELF loader if no load function is specified */
> if (!rproc->ops->load) {
> rproc->ops->load = rproc_elf_load_segments;
> - rproc->ops->find_rsc_table = rproc_elf_find_rsc_table;
> + rproc->ops->load_rsc_table = rproc_elf_load_rsc_table;
>
> As st_slim_rproc has no load ops, it will inherit from all default ops including rproc_elf_load_rsc_table.
> As no resource table present in firmware, an error will be returned and st_slim_rproc will failed.

Thanks for catching my mistake, Loic! The expected outcome would be that
the slim_rproc_ops does point the "load" to the now exported ELF loader
symbol and by that won't get a find_rsc_table entry from the default
set..

> In case of Qualcom, load ops is defined...

Right, in the end the st_slim driver should have been defined just as
the Qualcomm one - but referencing rproc_elf_load_segments().

> See below B2260 log (with additional error message in rproc_load_rsc_table function.
>
> [ 10.201079] remoteproc remoteproc2: st231-delta is available
> [ 10.258121] remoteproc remoteproc2: powering up st231-delta
> [ 10.258143] remoteproc remoteproc2: Booting fw image rproc-st231-delta-fw, size 44416
> [ 10.258151] rproc_load_rsc_table: error -22
>
> Moreover with your proposal, as the choice to support or not a
> resource table is based on the fact rproc->ops->load_rsc_table is set
> or not, it is not possible with one unique driver to support firmware
> with resource table and firmware without resource table.

This is retaining the previous behaviour of failing to load/start a
remoteproc if no resource table is found, when the driver expects one.
The new scheme would allow you to specify a custom load_rsc_table that
calls rproc_elf_load_rsc_table() and ignores the return value to support
your use case.

> Will be better from my pov to consider that no resource table found
> in rproc_elf_load_rsc_table function as a normal case and not an
> error.

This would be a change in behavior and I can see how this could be
annoying to people (by not catching their mistakes during development).

If you think this is the preferred implementation then please submit a
separate patch for this so we can get some feedback from other users.

Regards,
Bjorn