Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
MIME-Version: 1.0
References: <20200115102142.11229-1-cleger@kalray.eu> <088ceab9-f135-6e70-dcf6-f75ec46110b1@st.com>
 <79048597.12371594.1579098506802.JavaMail.zimbra@kalray.eu>
 <a1116656-cf2e-c1a1-7cc3-0fe2a79f076e@st.com> <612100872.12377996.1579101063237.JavaMail.zimbra@kalray.eu>
 <20200117225217.GA27535@xps15> <377421261.12898679.1579462834671.JavaMail.zimbra@kalray.eu>
In-Reply-To: <377421261.12898679.1579462834671.JavaMail.zimbra@kalray.eu>
From:   Mathieu Poirier <mathieu.poirier@linaro.org>
Date:   Mon, 20 Jan 2020 09:28:49 -0700
Message-ID: <CANLsYky-dp3=J__x9d7BJtu1=ppEiFMfuJnRQ7ZTVn6X43BJ_g@mail.gmail.com>
Subject: Re: [PATCH] remoteproc: Add support for predefined notifyids
To:     =?UTF-8?Q?Cl=C3=A9ment_Leger?= <cleger@kalray.eu>
Cc:     Arnaud Pouliquen <arnaud.pouliquen@st.com>,
        Ohad Ben-Cohen <ohad@wizery.com>,
        Bjorn Andersson <bjorn.andersson@linaro.org>,
        linux-remoteproc <linux-remoteproc@vger.kernel.org>,
        linux-kernel <linux-kernel@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Sun, 19 Jan 2020 at 12:40, Cl=C3=A9ment Leger <cleger@kalray.eu> wrote:
>
> Hi Mathieu,
>
> ----- On 17 Jan, 2020, at 23:52, Mathieu Poirier mathieu.poirier@linaro.o=
rg wrote:
>
> > Hey guys,
> >
> > On Wed, Jan 15, 2020 at 04:11:03PM +0100, Cl=C3=A9ment Leger wrote:
> >>
> >>
> >> ----- On 15 Jan, 2020, at 16:09, Arnaud Pouliquen arnaud.pouliquen@st.=
com wrote:
> >>
> >> > On 1/15/20 3:28 PM, Cl=C3=A9ment Leger wrote:
> >> >> Hi Arnaud,
> >> >>
> >> >> ----- On 15 Jan, 2020, at 15:06, Arnaud Pouliquen arnaud.pouliquen@=
st.com wrote:
> >> >>
> >> >>> Hi Cl=C3=A9ment,
> >> >>>
> >> >>> On 1/15/20 11:21 AM, Clement Leger wrote:
> >> >>>> In order to support preallocated notify ids, if their value is
> >> >>>> equal to FW_RSC_NOTIFY_ID_ANY, then do no allocate a notify id
> >> >>>> dynamically but try to allocate the requested one. This is useful=
 when
> >> >>>> using custom ids to bind them to custom vendor resources. For ins=
tance,
> >> >>>> it allow to assign a group of queues to a specific interrupti in =
order
> >> >>>> to dispatch notifications.
> >> >>>>
> >> >>>> Signed-off-by: Clement Leger <cleger@kalray.eu>
> >> >>>> ---
> >> >>>>  drivers/remoteproc/remoteproc_core.c | 27 +++++++++++++++++++---=
-----
> >> >>>>  include/linux/remoteproc.h           |  1 +
> >> >>>>  2 files changed, 20 insertions(+), 8 deletions(-)
> >> >>>>
> >> >>>> diff --git a/drivers/remoteproc/remoteproc_core.c
> >> >>>> b/drivers/remoteproc/remoteproc_core.c
> >> >>>> index 307df98347ba..b1485fcd0f11 100644
> >> >>>> --- a/drivers/remoteproc/remoteproc_core.c
> >> >>>> +++ b/drivers/remoteproc/remoteproc_core.c
> >> >>>> @@ -351,14 +351,27 @@ int rproc_alloc_vring(struct rproc_vdev *rv=
dev, int i)
> >> >>>>         /*
> >> >>>>          * Assign an rproc-wide unique index for this vring
> >> >>>>          * TODO: assign a notifyid for rvdev updates as well
> >> >>>> -        * TODO: support predefined notifyids (via resource table=
)
> >> >>>>          */
> >> >>>> -       ret =3D idr_alloc(&rproc->notifyids, rvring, 0, 0, GFP_KE=
RNEL);
> >> >>>> -       if (ret < 0) {
> >> >>>> -               dev_err(dev, "idr_alloc failed: %d\n", ret);
> >> >>>> -               return ret;
> >> >>>> +       if (rsc->vring[i].notifyid =3D=3D FW_RSC_NOTIFY_ID_ANY) {
> >> >>>> +               ret =3D idr_alloc(&rproc->notifyids, rvring, 0, 0=
, GFP_KERNEL);
> >> >>>> +               if (ret < 0) {
> >> >>>> +                       dev_err(dev, "idr_alloc failed: %d\n", re=
t);
> >> >>>> +                       return ret;
> >> >>>> +               }
> >> >>>> +               notifyid =3D ret;
> >> >>>> +
> >> >>>> +               /* Let the rproc know the notifyid of this vring.=
*/
> >> >>>> +               rsc->vring[i].notifyid =3D notifyid;
> >> >>>> +       } else {
> >> >>>> +               /* Reserve requested notify_id */
> >> >>>> +               notifyid =3D rsc->vring[i].notifyid;
> >> >>>> +               ret =3D idr_alloc(&rproc->notifyids, rvring, noti=
fyid,
> >> >>>> +                               notifyid + 1, GFP_KERNEL);
> >> >>>> +               if (ret < 0) {
> >> >>>> +                       dev_err(dev, "idr_alloc failed: %d\n", re=
t);
> >> >>>> +                       return ret;
> >> >>>> +               }
> >> >>>>         }
> >> >>>> -       notifyid =3D ret;
> >> >>>>
> >> >>>>         /* Potentially bump max_notifyid */
> >> >>>>         if (notifyid > rproc->max_notifyid)
> >> >>>> @@ -366,8 +379,6 @@ int rproc_alloc_vring(struct rproc_vdev *rvde=
v, int i)
> >> >>>>
> >> >>>>         rvring->notifyid =3D notifyid;
> >> >>>>
> >> >>>> -       /* Let the rproc know the notifyid of this vring.*/
> >> >>>> -       rsc->vring[i].notifyid =3D notifyid;
> >> >>>>         return 0;
> >> >>>>  }
> >> >>> The rproc_free_vring function resets the notifyid to -1 on free.
> >> >>> This could generate a side effect if the resource table is not rel=
oaded.
> >> >>
> >> >> Oh indeed, I did not thought of that. What would you recommend ?
> >> >> If using -1 in free vring, notify ids will be reallocated at next
> >> >> round.
> >> > Regarding the code i'm not sure that it is useful to reset the notif=
yID to -1 on
> >> > free.
> >
> > I'm not sure setting notifyid to -1 in rproc_free_vring() is such a big=
 problem.
> > No matter the code path I look at, if rproc_free_vring() is called some=
thing
> > serious has happened and the resource table will be reloaded if another=
 attempt
> > at booting the remote processor is done.  It can also be that a gracefu=
l
> > shutdown is underway, in which case the resource table will be reloaded=
 anyway
> > if/when the slave is brought back in service.
> >
> > Let me know if I'm missing a scenario.
>
> No, you are actually right
>
> >
> > To me the real problem is if a FW image has set the notifyids in the re=
source
> > table to 0xffffffff, thinking they will be overwritten.  In that case t=
hings
> > will really south.
>
> Hum, if set to 0xFFFFFFFF, then they will be assigned dynamically and upd=
ated
> in the resource table (with this patch). But your probably mean existing =
code,
> right ?

My apologies for not expressing myself clearly here - let me try again.

At this time notifyids in the firmware's resource table can be set to
anything because the code will overwrite them.  With this patch
firmware images that don't have their notifyids set to -1 will see a
change in how ids are assigned, something that has the potential to
break user space.

Regards,
Mathieu

>
> >
> >> > In current version, on alloc, the notifyID is overwriten without che=
ck.
> >> > And as vdev status is updated, vring struct in resource table should=
 be
> >> > considered as invalid
> >> > Except if i missed a usecase/race condition...
> >> >
> >> >>
> >> >> I was also worried that it would break some existing user applicati=
ons
> >> >> which uses "0" as a notify id in vring but expect the id to be
> >> >> allocated dynamically. With my modification, it means it will try t=
o
> >> >> use "0" as a predefined id, leading to allocation failure.
> >
> > From my point of view they will have been lucky for all this time.  Eve=
n with
> > a new version of the resource table (which I think is the right way go)
> > cases like this will break.
>
> Agreed, and actually, I may have missread some code. I can't find were I =
read
> that. By the way, is there any documentation which state the allowed valu=
es of
> notify ids ?
>
> >
> > Thanks,
> > Mathieu
> >
> >> >>
> >> > Yes this could introduce regression for firmware that sets 0 as defa=
ult value.
> >> > Probably better to introduce this patch with a new version of the re=
source table
> >> > :)
> >>
> >> Understood ;)
> >>
> >> Regards,
> >>
> >> Cl=C3=A9ment
> >>
> >> >
> >> > Regards
> >> > Arnaud
> >> >>>
> >> >>>>
> >> >>>> diff --git a/include/linux/remoteproc.h b/include/linux/remotepro=
c.h
> >> >>>> index 16ad66683ad0..dcae3394243e 100644
> >> >>>> --- a/include/linux/remoteproc.h
> >> >>>> +++ b/include/linux/remoteproc.h
> >> >>>> @@ -123,6 +123,7 @@ enum fw_resource_type {
> >> >>>>  };
> >> >>>>
> >> >>>>  #define FW_RSC_ADDR_ANY (-1)
> >> >>>> +#define FW_RSC_NOTIFY_ID_ANY (-1)This define can also be used in
> >> >>>> rproc_free_vring
> >> >>
> >> >> Indeed.
> >> >>
> >> >> Thanks for your review.
> >> >>
> >> >> Regards,
> >> >>
> >> >> Cl=C3=A9ment
> >> >>
> >> >>>
> >> >>> Regards,
> >> >>> Arnaud
> >> >>>>
> >> >>>>  /**
> > > > >>>   * struct fw_rsc_carveout - physically contiguous memory reque=
st