| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <df9b33b940c84d3d8095450101299b67@SFHDAG7NODE2.st.com>
Date: Mon, 8 Jan 2018 08:14:53 +0000
From: Loic PALLARDY <loic.pallardy@...com>
To: Bjorn Andersson <bjorn.andersson@...aro.org>
CC: Ohad Ben-Cohen <ohad@...ery.com>,
"linux-remoteproc@...r.kernel.org" <linux-remoteproc@...r.kernel.org>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: RE: [PATCH 7/8] remoteproc: Drop dangling find_rsc_table dummies
> -----Original Message-----
> From: Bjorn Andersson [mailto:bjorn.andersson@...aro.org]
> Sent: Friday, January 05, 2018 7:51 PM
> To: Loic PALLARDY <loic.pallardy@...com>
> Cc: Ohad Ben-Cohen <ohad@...ery.com>; linux-
> remoteproc@...r.kernel.org; linux-kernel@...r.kernel.org
> Subject: Re: [PATCH 7/8] remoteproc: Drop dangling find_rsc_table dummies
>
> On Fri 05 Jan 08:53 PST 2018, Loic PALLARDY wrote:
>
> >
> >
> > > -----Original Message-----
> > > From: linux-remoteproc-owner@...r.kernel.org [mailto:linux-
> remoteproc-
> > > owner@...r.kernel.org] On Behalf Of Bjorn Andersson
> > > Sent: Wednesday, December 13, 2017 11:41 PM
> > > To: Ohad Ben-Cohen <ohad@...ery.com>; Bjorn Andersson
> > > <bjorn.andersson@...aro.org>
> > > Cc: linux-remoteproc@...r.kernel.org; linux-kernel@...r.kernel.org
> > > 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@...aro.org>
> > > ---
> > > 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.
OK would be nice to document somewhere what's framework responsibilities and what's driver ones.
>
> > 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).
You're right
>
> 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.
No it is OK. I'll review v3.
Regards,
Loic
>
> Regards,
> Bjorn
Powered by blists - more mailing lists