Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S965242AbbDWMAO (ORCPT ); Thu, 23 Apr 2015 08:00:14 -0400 Received: from mail-wg0-f43.google.com ([74.125.82.43]:34552 "EHLO mail-wg0-f43.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S965181AbbDWMAJ convert rfc822-to-8bit (ORCPT ); Thu, 23 Apr 2015 08:00:09 -0400 Subject: Re: [PATCH v2 3/4] of: overlay: Add sysfs attributes Mime-Version: 1.0 (Mac OS X Mail 8.2 \(2070.6\)) Content-Type: text/plain; charset=utf-8 From: Pantelis Antoniou In-Reply-To: Date: Thu, 23 Apr 2015 15:00:03 +0300 Cc: Grant Likely , Andrew Morton , Matt Porter , Koen Kooi , Guenter Roeck , "devicetree@vger.kernel.org" , "linux-kernel@vger.kernel.org" , "linux-api@vger.kernel.org" Content-Transfer-Encoding: 8BIT Message-Id: <6E391B8F-936A-4FD1-B8CC-C9B5FCE0B291@konsulko.com> References: <1428434632-13789-1-git-send-email-pantelis.antoniou@konsulko.com> <1428434632-13789-4-git-send-email-pantelis.antoniou@konsulko.com> To: Rob Herring X-Mailer: Apple Mail (2.2070.6) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 8947 Lines: 295 Hi Rob, > On Apr 15, 2015, at 04:27 , Rob Herring wrote: > > On Tue, Apr 7, 2015 at 2:23 PM, Pantelis Antoniou > 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 >> --- >> 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 >> #include >> #include >> +#include >> >> #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. 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. To use a non-binary method we have to register per ktype sysfs_ops and duplicate the way the non-binary attribute works. 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. >> + >> +static ssize_t targets_read(struct file *filp, struct kobject *kobj, >> + struct bin_attribute *bin_attr, char *buf, >> + loff_t offset, size_t count) >> +{ >> + struct of_overlay *ov = kobj_to_overlay(kobj); >> + struct of_overlay_info *ovinfo; >> + char *tmpbuf, *s, *e; >> + const char *name; >> + ssize_t ret; >> + int i, len; >> + >> + /* allocate work buffer; we know that PAGE_SIZE is enough */ >> + tmpbuf = kmalloc(PAGE_SIZE, GFP_KERNEL); >> + if (tmpbuf == NULL) >> + return -ENOMEM; >> + >> + s = tmpbuf; >> + e = tmpbuf + PAGE_SIZE; >> + >> + mutex_lock(&of_mutex); >> + >> + /* targets */ >> + for (i = 0; i < ov->count; i++) { >> + ovinfo = &ov->ovinfo_tab[i]; >> + >> + name = of_node_full_name(ovinfo->target); >> + len = strlen(name); >> + if (s + len + 1 >= e) >> + return -ENOMEM; > > Leaking memory here and holding the mutex. > OK >> + memcpy(s, name, len); >> + s += len; >> + *s++ = '\n'; >> + } >> + if (s + 1 >= e) >> + return -ENOMEM; > And here. > OK >> + *s++ = '\0'; >> + >> + /* the buffer is zero terminated */ >> + len = s - tmpbuf; >> + >> + mutex_unlock(&of_mutex); >> + >> + /* perform the read */ >> + ret = memory_read_from_buffer(buf, count, &offset, tmpbuf, len); >> + >> + /* free the temporary buffer */ >> + kfree(tmpbuf); >> + >> + return ret; >> +} >> + >> +/* targets property */ >> +static BIN_ATTR_RO(targets, PAGE_SIZE); >> + >> +static ssize_t can_remove_read(struct file *filp, struct kobject *kobj, >> + struct bin_attribute *bin_attr, char *buf, >> + loff_t offset, size_t count) >> +{ >> + struct of_overlay *ov = kobj_to_overlay(kobj); >> + 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' + overlay_removal_is_ok(ov); >> + tbuf[1] = '\n'; >> + tbuf[2] = '\0'; >> + >> + /* copy to buffer */ >> + memcpy(buf, tbuf + offset, count); >> + >> + return count; >> +} >> + >> +/* can_remove property */ >> +static BIN_ATTR_RO(can_remove, 3); > > Same question about bin attr here. > Same answer as above. >> + >> /** >> * of_overlay_create() - Create and apply an overlay >> * @tree: Device node containing all the overlays >> @@ -360,6 +503,10 @@ int of_overlay_create(struct device_node *tree) >> struct of_overlay *ov; >> int err, id; >> >> + /* administratively disabled */ >> + if (!atomic_read(&ov_enable)) >> + return -EPERM; >> + >> /* allocate the overlay structure */ >> ov = kzalloc(sizeof(*ov), GFP_KERNEL); >> if (ov == NULL) >> @@ -416,6 +563,22 @@ int of_overlay_create(struct device_node *tree) >> goto err_cancel_overlay; >> } >> >> + /* create targets file */ >> + err = sysfs_create_bin_file(&ov->kobj, &bin_attr_targets); >> + if (err != 0) { >> + pr_err("%s: sysfs_create_bin_file() failed for tree@%s\n", >> + __func__, tree->full_name); >> + goto err_cancel_overlay; >> + } >> + >> + /* create can_remove file */ >> + err = sysfs_create_bin_file(&ov->kobj, &bin_attr_can_remove); >> + if (err != 0) { >> + pr_err("%s: sysfs_create_bin_file() failed for tree@%s\n", >> + __func__, tree->full_name); >> + goto err_cancel_overlay; >> + } >> + >> /* add to the tail of the overlay list */ >> list_add_tail(&ov->node, &ov_list); >> >> @@ -596,5 +759,7 @@ int of_overlay_init(void) >> if (!ov_kset) >> return -ENOMEM; >> >> - return 0; >> + rc = sysfs_create_bin_file(&ov_kset->kobj, &bin_attr_enable); >> + WARN(rc, "%s: error adding enable attribute\n", __func__); >> + return rc; >> } >> -- >> 1.7.12 Regards — Pantelis -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/