From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-12.8 required=3.0 tests=BAYES_00,DKIM_SIGNED, DKIM_VALID,DKIM_VALID_AU,FREEMAIL_FORGED_FROMDOMAIN,FREEMAIL_FROM, HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_CR_TRAILER,INCLUDES_PATCH, MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS autolearn=ham autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 170C7C433DB for ; Sun, 24 Jan 2021 19:08:46 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id C006722CF6 for ; Sun, 24 Jan 2021 19:08:45 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1726348AbhAXTIo (ORCPT ); Sun, 24 Jan 2021 14:08:44 -0500 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:50568 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1726344AbhAXTIo (ORCPT ); Sun, 24 Jan 2021 14:08:44 -0500 Received: from mail-io1-xd35.google.com (mail-io1-xd35.google.com [IPv6:2607:f8b0:4864:20::d35]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 26A29C061573; Sun, 24 Jan 2021 11:08:04 -0800 (PST) Received: by mail-io1-xd35.google.com with SMTP id q129so22237529iod.0; Sun, 24 Jan 2021 11:08:04 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=qFpNtBMDAZ9hyw4qVfpem4HXhb2WLSapntydUYNf/JA=; b=PiTn7W+bMU0NjC1WVneb/zNjcc1PrYWzUHa1DhP9Cheerl4o6ll38Py6xR0cLZTk/+ KMYDs+g/ZqdLXpBAeONhJGPVuT8ytigUzBcm5iHwusXDAA99FQbpPdBG5a6uBG/2WBq5 Xbd7YJ0QSCIauPdQfjmNMJg5d5gjE0j9C5BreLiVUaYosNnQjdYYxmYYVZ6KgKBFPxwB GZpoL5fzl5yrBbdXVkpXZROrVUmmVsRpxHO/B8Pmto96o3pHDrXSx5hPcQUi2tHyXvzI 2qgXKwtGurhgFH78G6ZkP9U9x9EN9NnZgl/m0A9jaXR3bMlGwg5iL6HsMnTJzGwGkM7U UxDQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=qFpNtBMDAZ9hyw4qVfpem4HXhb2WLSapntydUYNf/JA=; b=KlzjBCq25P8oVC1ZXeyxIm71uq0UtoW7JYQMUhy+r8DRZI9986jeD9hqcUJ1M4Olwz dNYWj4gkN+N6hAr2X1xHYgHvqJbAegVyQugn/8KqIplLRwzGXPs/pmCMqLq+qzAXlRcT wIxChgT943iVzwqvrX1wzyHD4OeW6GQjy4zfXAPwu/uTHANLidFFoQXJiZBM8fGKPEYa BXsYFca2+Sfj9xyqjnk0t9ngaZiLvI1tEZO3G9gP58iiIlfnv+IDF1/wg5T84oeDq1qU xSh6pUrwsm4B9CHrZUE4E+rafcyLYBr/t2YzrGpESMkRnmN41VrSzRNlb9FugX5x98HG WMkg== X-Gm-Message-State: AOAM532PsExtHNzi4IR8oeXqHt6S7pAKppvT7SBmCP5b8M7YkP3M4n2W ahHaMPkbmzaML3b3CtkiIhjcKKh0gd2CuoYlsnE= X-Google-Smtp-Source: ABdhPJx+rYpwGHO83k72QG/ptk1coy8RBMUk7SNsSZrYmviBdiPNSwa6fwadTrwdvwYxY1dXt38KlzObQwOnxOHThCA= X-Received: by 2002:a6b:4003:: with SMTP id k3mr309234ioa.105.1611515283280; Sun, 24 Jan 2021 11:08:03 -0800 (PST) MIME-Version: 1.0 References: <20210122162529.84978-1-alexandru.ardelean@analog.com> <20210122162529.84978-4-alexandru.ardelean@analog.com> <20210124181126.07c100a5@archlinux> In-Reply-To: <20210124181126.07c100a5@archlinux> From: Alexandru Ardelean Date: Sun, 24 Jan 2021 21:07:52 +0200 Message-ID: Subject: Re: [PATCH v2 03/12][RESEND] iio: buffer: rework buffer & scan_elements dir creation To: Jonathan Cameron Cc: Alexandru Ardelean , LKML , linux-iio , Lars-Peter Clausen , "Hennerich, Michael" , nuno.sa@analog.com, "Bogdan, Dragos" , Greg Kroah-Hartman , "Rafael J. Wysocki" Content-Type: text/plain; charset="UTF-8" Precedence: bulk List-ID: X-Mailing-List: linux-iio@vger.kernel.org On Sun, Jan 24, 2021 at 8:13 PM Jonathan Cameron wrote: > > On Fri, 22 Jan 2021 18:25:20 +0200 > Alexandru Ardelean wrote: > > > When adding more than one IIO buffer per IIO device, we will need to create > > a buffer & scan_elements directory for each buffer. > > We also want to move the 'scan_elements' to be a sub-directory of the > > 'buffer' folder. > > > > The format we want to reach is, for a iio:device0 folder, for 2 buffers > > [for example], we have a 'buffer0' and a 'buffer1' subfolder, and each with > > it's own 'scan_elements' subfolder. > > > > So, for example: > > iio:device0/buffer0 > > scan_elements/ > > > > iio:device0/buffer1 > > scan_elements/ > > > > The other attributes under 'bufferX' would remain unchanged. > > > > However, we would also need to symlink back to the old 'buffer' & > > 'scan_elements' folders, to keep backwards compatibility. > > > > Doing all these, require that we maintain the kobjects for each 'bufferX' > > and 'scan_elements' so that we can symlink them back. We also need to > > implement the sysfs_ops for these folders. > > > > Signed-off-by: Alexandru Ardelean > > +CC GregKH and Rafael W for feedback on various things inline. > > It might be that this is the neatest solution that we can come up with but > more eyes would be good! > > Whilst I think this looks fine, I'm less confident than I'd like to be. > > Jonathan > > > --- > > drivers/iio/industrialio-buffer.c | 195 +++++++++++++++++++++++++++--- > > drivers/iio/industrialio-core.c | 24 ++-- > > include/linux/iio/buffer_impl.h | 14 ++- > > include/linux/iio/iio.h | 2 +- > > 4 files changed, 200 insertions(+), 35 deletions(-) > > > > diff --git a/drivers/iio/industrialio-buffer.c b/drivers/iio/industrialio-buffer.c > > index 0412c4fda4c1..0f470d902790 100644 > > --- a/drivers/iio/industrialio-buffer.c > > +++ b/drivers/iio/industrialio-buffer.c > > @@ -1175,8 +1175,6 @@ static ssize_t iio_buffer_store_enable(struct device *dev, > > return (ret < 0) ? ret : len; > > } > > > > -static const char * const iio_scan_elements_group_name = "scan_elements"; > > - > > static ssize_t iio_buffer_show_watermark(struct device *dev, > > struct device_attribute *attr, > > char *buf) > > @@ -1252,6 +1250,124 @@ static struct attribute *iio_buffer_attrs[] = { > > &dev_attr_data_available.attr, > > }; > > > > +#define to_dev_attr(_attr) container_of(_attr, struct device_attribute, attr) > > + > > +static ssize_t iio_buffer_dir_attr_show(struct kobject *kobj, > > + struct attribute *attr, > > + char *buf) > > +{ > > + struct iio_buffer *buffer = container_of(kobj, struct iio_buffer, buffer_dir); > > + struct device_attribute *dattr; > > + > > + dattr = to_dev_attr(attr); > > + > > + return dattr->show(&buffer->indio_dev->dev, dattr, buf); > > +} > > + > > +static ssize_t iio_buffer_dir_attr_store(struct kobject *kobj, > > + struct attribute *attr, > > + const char *buf, > > + size_t len) > > +{ > > + struct iio_buffer *buffer = container_of(kobj, struct iio_buffer, buffer_dir); > > + struct device_attribute *dattr; > > + > > + dattr = to_dev_attr(attr); > > + > > + return dattr->store(&buffer->indio_dev->dev, dattr, buf, len); > > +} > > + > > +static const struct sysfs_ops iio_buffer_dir_sysfs_ops = { > > + .show = iio_buffer_dir_attr_show, > > + .store = iio_buffer_dir_attr_store, > > +}; > > + > > +static struct kobj_type iio_buffer_dir_ktype = { > > + .sysfs_ops = &iio_buffer_dir_sysfs_ops, > > +}; > > + > > +static ssize_t iio_scan_el_dir_attr_show(struct kobject *kobj, > > + struct attribute *attr, > > + char *buf) > > +{ > > + struct iio_buffer *buffer = container_of(kobj, struct iio_buffer, scan_el_dir); > > + struct device_attribute *dattr = to_dev_attr(attr); > > + > > + return dattr->show(&buffer->indio_dev->dev, dattr, buf); > > +} > > + > > +static ssize_t iio_scan_el_dir_attr_store(struct kobject *kobj, > > + struct attribute *attr, > > + const char *buf, > > + size_t len) > > +{ > > + struct iio_buffer *buffer = container_of(kobj, struct iio_buffer, scan_el_dir); > > + struct device_attribute *dattr = to_dev_attr(attr); > > + > > + return dattr->store(&buffer->indio_dev->dev, dattr, buf, len); > > +} > > + > > +static const struct sysfs_ops iio_scan_el_dir_sysfs_ops = { > > + .show = iio_scan_el_dir_attr_show, > > + .store = iio_scan_el_dir_attr_store, > > +}; > > + > > +static struct kobj_type iio_scan_el_dir_ktype = { > > + .sysfs_ops = &iio_scan_el_dir_sysfs_ops, > > +}; > > + > > +/* > > + * These iio_sysfs_{add,del}_attrs() are essentially re-implementations of > > + * sysfs_create_files() & sysfs_remove_files(), but they are meant to get > > + * around the const-pointer mismatch situation with using them. > > + * > > + * sysfs_{create,remove}_files() uses 'const struct attribute * const *ptr', > > + * while these are happy with just 'struct attribute **ptr' > > + */ > > Then to my mind the question becomes why sysfs_create_files() etc requires > the second level of const. If there is justification for that relaxation here > we should make it more generally. > > > +static int iio_sysfs_add_attrs(struct kobject *kobj, struct attribute **ptr) > > +{ > > + int err = 0; > > + int i; > > + > > + for (i = 0; ptr[i] && !err; i++) > > + err = sysfs_create_file(kobj, ptr[i]); > > + if (err) > > + while (--i >= 0) > > + sysfs_remove_file(kobj, ptr[i]); > > + return err; > > +} > > + > > +static void iio_sysfs_del_attrs(struct kobject *kobj, struct attribute **ptr) > > +{ > > + int i; > > + > > + for (i = 0; ptr[i]; i++) > > + sysfs_remove_file(kobj, ptr[i]); > > +} > > + > > +/** > > + * __iio_buffer_alloc_sysfs_and_mask() - Allocate sysfs attributes to an attached buffer > > + * @buffer: the buffer object for which the sysfs attributes are created for > > + * @indio_dev: the iio device to which the iio buffer belongs to > > + * > > + * Return 0, or negative for error. > > + * > > + * This function must be called for each single buffer. The sysfs attributes for that > > + * buffer will be created, and the IIO device object will be the parent kobject of that > > + * the kobjects created here. > > + * Because we need to redirect sysfs attribute to it's IIO buffer object, we need to > > + * implement our own sysfs_ops, and each IIO buffer will keep a kobject for the > > + * 'bufferX' directory and one for the 'scan_elements' directory. > > + * And in order to do this, this function must be called after the IIO device object > > + * has been added via device_add(). This fundamentally changes how sysfs attributes > > + * were created before (with one single IIO buffer per IIO device), where the > > + * sysfs attributes for the buffer were mapped as attribute groups on the IIO device > > + * groups object list. > > Been a while, so I can't recall the exact reasons this can cause problems. > I've +CC Greg and Rafael for comments. > > For their reference, the aim of this set is undo an old restriction on IIO that devices > only had one buffer. To do that we need to keep a iio:deviceX/buffer0 directory > also exposed as iio:deviceX/buffer/* and > iio:deviceX/buffer0/scan_elements/ as iio:deviceX/scan_elements. > to maintain backwards compatibility. > > > > + * Using kobjects directly for the 'bufferX' and 'scan_elements' directories allows > > + * us to symlink them back to keep backwards compatibility for the old sysfs interface > > + * for IIO buffers while also allowing us to support multiple IIO buffers per one > > + * IIO device. > > + */ > > static int __iio_buffer_alloc_sysfs_and_mask(struct iio_buffer *buffer, > > struct iio_dev *indio_dev) > > { > > @@ -1282,12 +1398,16 @@ static int __iio_buffer_alloc_sysfs_and_mask(struct iio_buffer *buffer, > > memcpy(&attr[ARRAY_SIZE(iio_buffer_attrs)], buffer->attrs, > > sizeof(struct attribute *) * attrcount); > > > > - attr[attrcount + ARRAY_SIZE(iio_buffer_attrs)] = NULL; > > + buffer->buffer_attrs = attr; > > > > - buffer->buffer_group.name = "buffer"; > > - buffer->buffer_group.attrs = attr; > > + ret = kobject_init_and_add(&buffer->buffer_dir, &iio_buffer_dir_ktype, > > + &indio_dev->dev.kobj, "buffer"); > > + if (ret) > > + goto error_buffer_free_attrs; > > > > Do we potentially want kobject_uevent calls for these? > (based on looking at similar code in edac). Not sure if this question is also for me. I can take a look. Hopefully I can make some sense of it. But I don't know of a way to see if it actually does anything by adding it. So, suggestions/ideas are welcome. > > > - indio_dev->groups[indio_dev->groupcounter++] = &buffer->buffer_group; > > + ret = iio_sysfs_add_attrs(&buffer->buffer_dir, buffer->buffer_attrs); > > + if (ret) > > + goto error_buffer_kobject_put; > > > > attrcount = 0; > > INIT_LIST_HEAD(&buffer->scan_el_dev_attr_list); > > @@ -1317,32 +1437,57 @@ static int __iio_buffer_alloc_sysfs_and_mask(struct iio_buffer *buffer, > > } > > } > > > > - buffer->scan_el_group.name = iio_scan_elements_group_name; > > - > > - buffer->scan_el_group.attrs = kcalloc(attrcount + 1, > > - sizeof(buffer->scan_el_group.attrs[0]), > > - GFP_KERNEL); > > - if (buffer->scan_el_group.attrs == NULL) { > > + buffer->scan_el_attrs = kcalloc(attrcount + 1, > > + sizeof(buffer->scan_el_attrs[0]), > > + GFP_KERNEL); > > + if (buffer->scan_el_attrs == NULL) { > > ret = -ENOMEM; > > goto error_free_scan_mask; > > } > > - attrn = 0; > > > > + ret = kobject_init_and_add(&buffer->scan_el_dir, &iio_scan_el_dir_ktype, > > + &indio_dev->dev.kobj, "scan_elements"); > > + if (ret) > > + goto error_free_scan_attrs; > > + > > + attrn = 0; > > list_for_each_entry(p, &buffer->scan_el_dev_attr_list, l) > > - buffer->scan_el_group.attrs[attrn++] = &p->dev_attr.attr; > > - indio_dev->groups[indio_dev->groupcounter++] = &buffer->scan_el_group; > > + buffer->scan_el_attrs[attrn++] = &p->dev_attr.attr; > > + > > + ret = iio_sysfs_add_attrs(&buffer->scan_el_dir, buffer->scan_el_attrs); > > + if (ret) > > + goto error_scan_kobject_put; > > > > return 0; > > > > +error_scan_kobject_put: > > + kobject_put(&buffer->scan_el_dir); > > +error_free_scan_attrs: > > + kfree(buffer->scan_el_attrs); > > error_free_scan_mask: > > bitmap_free(buffer->scan_mask); > > error_cleanup_dynamic: > > + iio_sysfs_del_attrs(&buffer->buffer_dir, buffer->buffer_attrs); > > iio_free_chan_devattr_list(&buffer->scan_el_dev_attr_list); > > - kfree(buffer->buffer_group.attrs); > > +error_buffer_kobject_put: > > + kobject_put(&buffer->buffer_dir); > > +error_buffer_free_attrs: > > + kfree(buffer->buffer_attrs); > > > > return ret; > > } > > > > +/** > > + * iio_buffer_alloc_sysfs_and_mask() - Allocate sysfs attributes to attached buffers > > + * @indio_dev: the iio device for which to create the buffer sysfs attributes > > + * > > + * Return 0, or negative for error. > > + * > > + * If the IIO device has no buffer attached, no sysfs attributes will be created. > > + * This function must be called after the IIO device object has been created and > > + * registered with device_add(). See __iio_buffer_alloc_sysfs_and_mask() for more > > + * details. > > + */ > > int iio_buffer_alloc_sysfs_and_mask(struct iio_dev *indio_dev) > > { > > struct iio_buffer *buffer = indio_dev->buffer; > > @@ -1364,14 +1509,28 @@ int iio_buffer_alloc_sysfs_and_mask(struct iio_dev *indio_dev) > > return __iio_buffer_alloc_sysfs_and_mask(buffer, indio_dev); > > } > > > > +/** > > + * __iio_buffer_free_sysfs_and_mask() - Free sysfs objects for a single IIO buffer > > + * @buffer: the iio buffer for which to destroy the objects > > + */ > > static void __iio_buffer_free_sysfs_and_mask(struct iio_buffer *buffer) > > { > > + iio_sysfs_del_attrs(&buffer->scan_el_dir, buffer->scan_el_attrs); > > + kobject_put(&buffer->scan_el_dir); > > + kfree(buffer->scan_el_attrs); > > bitmap_free(buffer->scan_mask); > > - kfree(buffer->buffer_group.attrs); > > - kfree(buffer->scan_el_group.attrs); > > + iio_sysfs_del_attrs(&buffer->buffer_dir, buffer->buffer_attrs); > > iio_free_chan_devattr_list(&buffer->scan_el_dev_attr_list); > > + kobject_put(&buffer->buffer_dir); > > + kfree(buffer->buffer_attrs); > > } > > > > +/** > > + * iio_buffer_free_sysfs_and_mask() - Free sysfs objects for all IIO buffers > > + * @indio_dev: the iio device for which to destroy the objects > > + * > > + * If the IIO device has no buffer attached, nothing will be done. > > + */ > > void iio_buffer_free_sysfs_and_mask(struct iio_dev *indio_dev) > > { > > struct iio_buffer *buffer = indio_dev->buffer; > > diff --git a/drivers/iio/industrialio-core.c b/drivers/iio/industrialio-core.c > > index 0a6fd299a978..95d66745f118 100644 > > --- a/drivers/iio/industrialio-core.c > > +++ b/drivers/iio/industrialio-core.c > > @@ -1817,18 +1817,11 @@ int __iio_device_register(struct iio_dev *indio_dev, struct module *this_mod) > > > > iio_device_register_debugfs(indio_dev); > > > > - ret = iio_buffer_alloc_sysfs_and_mask(indio_dev); > > - if (ret) { > > - dev_err(indio_dev->dev.parent, > > - "Failed to create buffer sysfs interfaces\n"); > > - goto error_unreg_debugfs; > > - } > > - > > ret = iio_device_register_sysfs(indio_dev); > > if (ret) { > > dev_err(indio_dev->dev.parent, > > "Failed to register sysfs interfaces\n"); > > - goto error_buffer_free_sysfs; > > + goto error_unreg_debugfs; > > } > > ret = iio_device_register_eventset(indio_dev); > > if (ret) { > > @@ -1857,14 +1850,21 @@ int __iio_device_register(struct iio_dev *indio_dev, struct module *this_mod) > > if (ret < 0) > > goto error_unreg_eventset; > > > > + ret = iio_buffer_alloc_sysfs_and_mask(indio_dev); > > + if (ret) { > > + dev_err(indio_dev->dev.parent, > > + "Failed to create buffer sysfs interfaces\n"); > > + goto error_device_del; > > + } > > + > > return 0; > > > > +error_device_del: > > + cdev_device_del(&indio_dev->chrdev, &indio_dev->dev); > > error_unreg_eventset: > > iio_device_unregister_eventset(indio_dev); > > error_free_sysfs: > > iio_device_unregister_sysfs(indio_dev); > > -error_buffer_free_sysfs: > > - iio_buffer_free_sysfs_and_mask(indio_dev); > > error_unreg_debugfs: > > iio_device_unregister_debugfs(indio_dev); > > return ret; > > @@ -1880,6 +1880,8 @@ void iio_device_unregister(struct iio_dev *indio_dev) > > struct iio_dev_opaque *iio_dev_opaque = to_iio_dev_opaque(indio_dev); > > struct iio_ioctl_handler *h, *t; > > > > + iio_buffer_free_sysfs_and_mask(indio_dev); > > + > > cdev_device_del(&indio_dev->chrdev, &indio_dev->dev); > > > > mutex_lock(&indio_dev->info_exist_lock); > > @@ -1897,8 +1899,6 @@ void iio_device_unregister(struct iio_dev *indio_dev) > > iio_buffer_wakeup_poll(indio_dev); > > > > mutex_unlock(&indio_dev->info_exist_lock); > > - > > - iio_buffer_free_sysfs_and_mask(indio_dev); > > } > > EXPORT_SYMBOL(iio_device_unregister); > > > > diff --git a/include/linux/iio/buffer_impl.h b/include/linux/iio/buffer_impl.h > > index 67d73d465e02..77e169e51434 100644 > > --- a/include/linux/iio/buffer_impl.h > > +++ b/include/linux/iio/buffer_impl.h > > @@ -103,14 +103,20 @@ struct iio_buffer { > > /* @scan_el_dev_attr_list: List of scan element related attributes. */ > > struct list_head scan_el_dev_attr_list; > > > > - /* @buffer_group: Attributes of the buffer group. */ > > - struct attribute_group buffer_group; > > + /* @buffer_dir: kobject for the 'buffer' directory of this buffer */ > > + struct kobject buffer_dir; > > + > > + /* @buffer_attrs: Attributes of the buffer group. */ > > + struct attribute **buffer_attrs; > > + > > + /* @scan_el_dir: kobject for the 'scan_elements' directory of this buffer */ > > + struct kobject scan_el_dir; > > > > /* > > - * @scan_el_group: Attribute group for those attributes not > > + * @scan_el_attrs: Array of attributes for those attributes not > > * created from the iio_chan_info array. > > */ > > - struct attribute_group scan_el_group; > > + struct attribute **scan_el_attrs; > > > > /* @attrs: Standard attributes of the buffer. */ > > const struct attribute **attrs; > > diff --git a/include/linux/iio/iio.h b/include/linux/iio/iio.h > > index e4a9822e6495..59b317dc45b8 100644 > > --- a/include/linux/iio/iio.h > > +++ b/include/linux/iio/iio.h > > @@ -556,7 +556,7 @@ struct iio_dev { > > struct mutex info_exist_lock; > > const struct iio_buffer_setup_ops *setup_ops; > > struct cdev chrdev; > > -#define IIO_MAX_GROUPS 6 > > +#define IIO_MAX_GROUPS 4 > > const struct attribute_group *groups[IIO_MAX_GROUPS + 1]; > > int groupcounter; > > >