Re: [PATCH v2 3/4] remoteproc: Add a sysfs interface for firmware and state

[Matt Redfearn]

Hi Bjorn,


On 18/10/16 23:16, Bjorn Andersson wrote:
> On Mon 17 Oct 08:49 PDT 2016, Matt Redfearn wrote:
>
> > This patch adds a sysfs interface to rproc allowing the firmware name
> > and processor state to be changed dynamically.
> >
> > State was previously available in debugfs, and is replicated here. The
> > firmware file allows retrieval of the running firmware name, and a new
> > one to be specified at run time, so long as the remote processor has
> > been stopped.
> >
> > Signed-off-by: Matt Redfearn <matt.redfearn@xxxxxxxxxx>
> >
> > ---
> >
> > Changes in v2:
> > Have firmware_store perform the necessary steps inline.
> > Use sysfs_streq when dealing with writes to sysfs files
> [..]
> > diff --git a/drivers/remoteproc/remoteproc_sysfs.c b/drivers/remoteproc/remoteproc_sysfs.c
> > new file mode 100644
> > index 000000000000..afa9ca040a7e
> > --- /dev/null
> > +++ b/drivers/remoteproc/remoteproc_sysfs.c
> > @@ -0,0 +1,155 @@
> > +/*
> > + * Remote Processor Framework
> > + *
> > + * This program is free software; you can redistribute it and/or
> > + * modify it under the terms of the GNU General Public License
> > + * version 2 as published by the Free Software Foundation.
> > + *
> > + * This program is distributed in the hope that it will be useful,
> > + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> > + * GNU General Public License for more details.
> > + */
> > +
> > +#include <linux/remoteproc.h>
> > +
> > +#include "remoteproc_internal.h"
> > +
> > +#define to_rproc(d) container_of(d, struct rproc, dev)
> > +
> > +/* Expose the loaded / running firmware name via sysfs */
> > +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);
> > +}
> > +
> > +/* Change firmware name via sysfs */
> > +static ssize_t firmware_store(struct device *dev,
> > +			      struct device_attribute *attr,
> > +			      const char *buf, size_t count)
> > +{
> > +	struct rproc *rproc = to_rproc(dev);
> > +	char *p;
> > +	int err, len = count;
> > +
> > +	err = mutex_lock_interruptible(&rproc->lock);
> > +	if (err) {
> > +		dev_err(dev, "can't lock rproc %s: %d\n", rproc->name, err);
> > +		return -EINVAL;
> > +	}
> > +
> > +	if (rproc->state != RPROC_OFFLINE) {
> > +		dev_err(dev, "can't change firmware while running\n");
> > +		err = -EBUSY;
> > +		goto out;
> > +	}
> > +
> > +	p = memchr(buf, '\n', count);
> > +	if (p)
> > +		len = p - buf;
> I prefer the following variant:
>
> len = strcspn(buf, '\n');

Yes - that's neater.

> > +
> > +	p = kstrndup(buf, len, GFP_KERNEL);
> > +	if (!p) {
> > +		err = -ENOMEM;
> > +		goto out;
> > +	}
> > +
> > +	kfree(rproc->firmware);
> > +	rproc->firmware = p;
> > +
> > +	err = rproc_add_virtio_devices(rproc);
> As of v4.9 rproc_add_virtio_devices() will only check to see if the
> rproc driver has auto_boot set and if so boot the core. (Yes it needs a
> new name)
>
> Also I'm not sure if it's valid to provide a sysfs API like this and
> sometimes setting firmware results in rproc_boot() and sometimes not...
>
> So, just drop patch 2 in this series and drop the call to
> rproc_add_virtio_devices() here.

Makes sense, and it's more consistent behavior with that change.

> > +out:
> > +	mutex_unlock(&rproc->lock);
> > +
> > +	return err ? err : count;
> > +}
> > +static DEVICE_ATTR_RW(firmware);
> > +
> > +/*
> > + * A state-to-string lookup table, for exposing a human readable state
> > + * via sysfs. Always keep in sync with enum rproc_state
> > + */
> > +static const char * const rproc_state_string[] = {
> > +	"offline",
> Please be overly explicit here and make these:
>
> 	[RPROC_OFFLINE] = "offline",

No problem.

Thanks,
Matt

> > +	"suspended",
> > +	"running",
> > +	"crashed",
> > +	"invalid",
> > +};
> > +
> Regards,
> Bjorn