Re: [PATCH v4 9/9] remoteproc: Properly handle firmware name when attaching

[Mathieu Poirier]

Good afternoon,

On Thu, 4 Jun 2020 at 08:17, Arnaud POULIQUEN <arnaud.pouliquen@xxxxxx> wrote:
>
> Hi Mathieu,
>
> On 6/1/20 7:51 PM, Mathieu Poirier wrote:
> > This patch prevents the firmware image name from being displayed when
> > the remoteproc core is attaching to a remote processor. This is needed
> > needed since there is no guarantee about the nature of the firmware
> > image that is loaded by the external entity.
> >
> > Signed-off-by: Mathieu Poirier <mathieu.poirier@xxxxxxxxxx>
> > ---
> >  drivers/remoteproc/remoteproc_core.c  | 18 ++++++++++++++++++
> >  drivers/remoteproc/remoteproc_sysfs.c | 16 ++++++++++++++--
> >  include/linux/remoteproc.h            |  2 ++
> >  3 files changed, 34 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c
> > index 0e23284fbd25..a8adc712e7f6 100644
> > --- a/drivers/remoteproc/remoteproc_core.c
> > +++ b/drivers/remoteproc/remoteproc_core.c
> > @@ -1642,6 +1642,14 @@ static int rproc_stop(struct rproc *rproc, bool crashed)
> >
> >       rproc->state = RPROC_OFFLINE;
> >
> > +     /*
> > +      * The remote processor has been stopped and is now offline, which means
> > +      * that the next time it is brought back online the remoteproc core will
> > +      * be responsible to load its firmware.  As such it is no longer
> > +      * autonomous.
> > +      */
> > +     rproc->autonomous = false;
> > +
> >       dev_info(dev, "stopped remote processor %s\n", rproc->name);
> >
> >       return 0;
> > @@ -2166,6 +2174,16 @@ int rproc_add(struct rproc *rproc)
> >       /* create debugfs entries */
> >       rproc_create_debug_dir(rproc);
> >
> > +     /*
> > +      * Remind ourselves the remote processor has been attached to rather
> > +      * than booted by the remoteproc core.  This is important because the
> > +      * RPROC_DETACHED state will be lost as soon as the remote processor
> > +      * has been attached to.  Used in firmware_show() and reset in
> > +      * rproc_stop().
> > +      */
> > +     if (rproc->state == RPROC_DETACHED)
> > +             rproc->autonomous = true;
> > +
> >       /* if rproc is marked always-on, request it to boot */
> >       if (rproc->auto_boot) {
> >               ret = rproc_trigger_auto_boot(rproc);
> > diff --git a/drivers/remoteproc/remoteproc_sysfs.c b/drivers/remoteproc/remoteproc_sysfs.c
> > index 8b462c501465..4ee158431f67 100644
> > --- a/drivers/remoteproc/remoteproc_sysfs.c
> > +++ b/drivers/remoteproc/remoteproc_sysfs.c
> > @@ -14,8 +14,20 @@ static ssize_t firmware_show(struct device *dev, struct device_attribute *attr,
> >                         char *buf)
> >  {
> >       struct rproc *rproc = to_rproc(dev);
> > -
> > -     return sprintf(buf, "%s\n", rproc->firmware);
> > +     const char *firmware = rproc->firmware;
> > +
> > +     /*
> > +      * If the remote processor has been started by an external
> > +      * entity we have no idea of what image it is running.  As such
> > +      * simply display a generic string rather then rproc->firmware.
> > +      *
> > +      * Here we rely on the autonomous flag because a remote processor
> > +      * may have been attached to and currently in a running state.
> > +      */
> > +     if (rproc->autonomous)
> > +             firmware = "unknown";
> > +
> > +     return sprintf(buf, "%s\n", firmware);
> >  }
> >
> >  /* Change firmware name via sysfs */
> > diff --git a/include/linux/remoteproc.h b/include/linux/remoteproc.h
> > index bf6a310ba870..cf5e31556780 100644
> > --- a/include/linux/remoteproc.h
> > +++ b/include/linux/remoteproc.h
> > @@ -491,6 +491,7 @@ struct rproc_dump_segment {
> >   * @table_sz: size of @cached_table
> >   * @has_iommu: flag to indicate if remote processor is behind an MMU
> >   * @auto_boot: flag to indicate if remote processor should be auto-started
> > + * @autonomous: true if an external entity has booted the remote processor
> >   * @dump_segments: list of segments in the firmware
> >   * @nb_vdev: number of vdev currently handled by rproc
> >   */
> > @@ -524,6 +525,7 @@ struct rproc {
> >       size_t table_sz;
> >       bool has_iommu;
> >       bool auto_boot;
> > +     bool autonomous;
>
> Do you need to define a new field? what about using rproc->firmware value?
>
> In this case the platform driver could provide a name using rproc_alloc
> even if in attached mode, for instance to identify a firmware version...

The problem is often that what gets loaded by the external entity is
not the same as the firmware available to the kernel.  As such
displaying the firmware name provided by the platform driver may not
be accurate when attaching to a remote processor.  Moreover I had to
introduce a new flag because the RPROC_ATTACHED state disappears as
soon as the remoteproc core attaches to the remote processor.  When
attached the state is set to RPROC_RUNNING, but there is still no
telling as to what firmware is running on the remote processor.

Thanks,
Mathieu

>
> Regards,
> Arnaud
>
>
>
> >       struct list_head dump_segments;
> >       int nb_vdev;
> >       u8 elf_class;
> >