Re: [PATCH v2 3/4] of: overlay: Add sysfs attributes

[Greg KH]

On Thu, Apr 23, 2015 at 03:39:21PM +0300, Pantelis Antoniou wrote:
> Hi Greg,
> 
> > On Apr 23, 2015, at 15:33 , Greg KH <greg@xxxxxxxxx> wrote:
> > 
> > On Thu, Apr 23, 2015 at 03:00:03PM +0300, Pantelis Antoniou wrote:
> >> Hi Rob,
> >> 
> >>> On Apr 15, 2015, at 04:27 , Rob Herring <robherring2@xxxxxxxxx> wrote:
> >>> 
> >>> On Tue, Apr 7, 2015 at 2:23 PM, Pantelis Antoniou
> >>> <pantelis.antoniou@xxxxxxxxxxxx> wrote:
> >>>> Implement a number of sysfs attributes for overlays.
> >>>> 
> >>>> * A throw once master enable switch to protect against any
> >>>> further overlay applications if the administrator desires so.
> >>> 
> >>> This one should be a separate patch.
> >>> 
> >> 
> >> OK.
> >> 
> >>>> * A per overlay targets sysfs attribute listing the targets of
> >>>> the installed overlay.
> >>> 
> >>> What are targets? "targets lists targets" does not help me. The
> >>> documentation doesn't help me either.
> >>> 
> >> 
> >> It lists the targets of the overlay that has been applied. What do
> >> you need in order to be helped? I mean what do you want listed?
> >> 
> >>>> * A per overlay can_remove sysfs attribute that reports whether
> >>>> the overlay can be removed or not due to another overlapping overlay.
> >>>> 
> >>>> Signed-off-by: Pantelis Antoniou <pantelis.antoniou@xxxxxxxxxxxx>
> >>>> ---
> >>>> drivers/of/overlay.c | 167 ++++++++++++++++++++++++++++++++++++++++++++++++++-
> >>>> 1 file changed, 166 insertions(+), 1 deletion(-)
> >>>> 
> >>>> diff --git a/drivers/of/overlay.c b/drivers/of/overlay.c
> >>>> index f17f5ef..c54d097 100644
> >>>> --- a/drivers/of/overlay.c
> >>>> +++ b/drivers/of/overlay.c
> >>>> @@ -21,6 +21,7 @@
> >>>> #include <linux/err.h>
> >>>> #include <linux/idr.h>
> >>>> #include <linux/sysfs.h>
> >>>> +#include <linux/atomic.h>
> >>>> 
> >>>> #include "of_private.h"
> >>>> 
> >>>> @@ -55,8 +56,12 @@ struct of_overlay {
> >>>>       struct kobject kobj;
> >>>> };
> >>>> 
> >>>> +/* master enable switch; once set to 0 can't be re-enabled */
> >>>> +static atomic_t ov_enable = ATOMIC_INIT(1);
> >>>> +
> >>>> static int of_overlay_apply_one(struct of_overlay *ov,
> >>>>               struct device_node *target, const struct device_node *overlay);
> >>>> +static int overlay_removal_is_ok(struct of_overlay *ov);
> >>>> 
> >>>> static int of_overlay_apply_single_property(struct of_overlay *ov,
> >>>>               struct device_node *target, struct property *prop)
> >>>> @@ -345,6 +350,144 @@ static struct kobj_type of_overlay_ktype = {
> >>>> 
> >>>> static struct kset *ov_kset;
> >>>> 
> >>>> +static ssize_t enable_read(struct file *filp, struct kobject *kobj,
> >>>> +               struct bin_attribute *bin_attr, char *buf,
> >>>> +               loff_t offset, size_t count)
> >>>> +{
> >>>> +       char tbuf[3];
> >>>> +
> >>>> +       if (offset < 0)
> >>>> +               return -EINVAL;
> >>>> +
> >>>> +       if (offset >= sizeof(tbuf))
> >>>> +               return 0;
> >>>> +
> >>>> +       if (count > sizeof(tbuf) - offset)
> >>>> +               count = sizeof(tbuf) - offset;
> >>>> +
> >>>> +       /* fill in temp */
> >>>> +       tbuf[0] = '0' + atomic_read(&ov_enable);
> >>>> +       tbuf[1] = '\n';
> >>>> +       tbuf[2] = '\0';
> >>>> +
> >>>> +       /* copy to buffer */
> >>>> +       memcpy(buf, tbuf + offset, count);
> >>>> +
> >>>> +       return count;
> >>>> +}
> >>>> +
> >>>> +static ssize_t enable_write(struct file *filp, struct kobject *kobj,
> >>>> +               struct bin_attribute *bin_attr, char *buf,
> >>>> +               loff_t off, size_t count)
> >>>> +{
> >>>> +       unsigned int new_enable;
> >>>> +
> >>>> +       if (off != 0 || (buf[0] != '0' && buf[0] != '1'))
> >>>> +               return -EINVAL;
> >>>> +
> >>>> +       new_enable = (unsigned int)(buf[0] - '0');
> >>>> +       if (new_enable > 1)
> >>>> +               return -EINVAL;
> >>>> +
> >>>> +       /* NOP for same value */
> >>>> +       if (new_enable == atomic_read(&ov_enable))
> >>>> +               return count;
> >>>> +
> >>>> +       /* if we've disabled it, no going back */
> >>>> +       if (atomic_read(&ov_enable) == 0)
> >>>> +               return -EPERM;
> >>>> +
> >>>> +       atomic_set(&ov_enable, new_enable);
> >>>> +       return count;
> >>>> +}
> >>>> +
> >>>> +/* just a single char + '\n' + '\0' */
> >>>> +static BIN_ATTR_RW(enable, 3);
> >>> 
> >>> Why are you using bin attribute? You are complicating the
> >>> implementation needlessly.
> >>> 
> >> 
> >> Itâs the same reason that the device tree core is using it.
> > 
> > It is doing that for "raw" device tree files, not individual attributes,
> > right?
> > 
> 
> Each property of a device tree is a binary attribute.

Because they export binary data, right?  I don't have access to a
machine that uses device tree at the moment to check this...

Any specific file/function you are referring to?

> >> Believe it or not, this is the simplest way to do it.
> >> If you take a look at the sysfs attribute implementation, the binary
> >> implementation is the one thatâs using the least amount of code.
> > 
> > Then something is really wrong here.
> > 
> >> To use a non-binary method we have to register per ktype sysfs_ops
> >> and duplicate the way the non-binary attribute works.
> > 
> > really?  Again, something must be wrong.
> > 
> >> For the gory details look at sysfs_add_file_mode_ns() in fs/sysfs/file.c
> >> 
> >> I can add the sysfs_ops but thatâs going to be more complicated not less.
> > 
> 
> Please take a look in linux/sysfs.h.
> The non-binary sysfs accessors are all using some kind of other kobj; 
> for instance DEVICE_ATTR is using a device_attribute, etc.
> 
> For the overlay case, Iâd have to create a of_overlay_attribute and work from
> there.

Yes, that is what you should be doing here as well.

That's just the model we have to work with, the uses of "raw" kobjects
are very limited, so it does take a bit more wrapper code to use them,
sorry.

You need access to the kobject anyway, which is why you need to provide
a type of attribute function, so that you get the right kobject.

Or just use an attribute group, would that be simpler?  If you have more
than one sysfs file, that's usually the best way to do things.

thanks,

greg k-h
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/