| Message ID | 20200904154547.3836-1-brgl@bgdev.pl |
|---|---|
| Headers | show |
| Series | gpio: mockup: support dynamically created and removed chips | expand |
On Fri, Sep 04, 2020 at 05:45:30PM +0200, Bartosz Golaszewski wrote: > From: Bartosz Golaszewski <bgolaszewski@baylibre.com> > > We don't need to specify any ranges when allocating IDs so we can switch > to ida_alloc() and ida_free() instead of the ida_simple_ counterparts. > > ida_simple_get(ida, 0, 0, gfp) is equivalent to > ida_alloc_range(ida, 0, UINT_MAX, gfp) which is equivalent to > ida_alloc(ida, gfp). Note: IDR will never actually allocate an ID > larger than INT_MAX. Have you considered switching to XArray API? > Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com> > --- > drivers/gpio/gpiolib.c | 6 +++--- > 1 file changed, 3 insertions(+), 3 deletions(-) > > diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c > index 15c99cf560ee..591777bc2285 100644 > --- a/drivers/gpio/gpiolib.c > +++ b/drivers/gpio/gpiolib.c > @@ -471,7 +471,7 @@ static void gpiodevice_release(struct device *dev) > struct gpio_device *gdev = dev_get_drvdata(dev); > > list_del(&gdev->list); > - ida_simple_remove(&gpio_ida, gdev->id); > + ida_free(&gpio_ida, gdev->id); > kfree_const(gdev->label); > kfree(gdev->descs); > kfree(gdev); > @@ -582,7 +582,7 @@ int gpiochip_add_data_with_key(struct gpio_chip *gc, void *data, > gc->of_node = gdev->dev.of_node; > #endif > > - gdev->id = ida_simple_get(&gpio_ida, 0, 0, GFP_KERNEL); > + gdev->id = ida_alloc(&gpio_ida, GFP_KERNEL); > if (gdev->id < 0) { > ret = gdev->id; > goto err_free_gdev; > @@ -753,7 +753,7 @@ int gpiochip_add_data_with_key(struct gpio_chip *gc, void *data, > err_free_descs: > kfree(gdev->descs); > err_free_ida: > - ida_simple_remove(&gpio_ida, gdev->id); > + ida_free(&gpio_ida, gdev->id); > err_free_gdev: > /* failures here can mean systems won't boot... */ > pr_err("%s: GPIOs %d..%d (%s) failed to register, %d\n", __func__, > -- > 2.26.1 >
On Fri, Sep 04, 2020 at 05:45:38PM +0200, Bartosz Golaszewski wrote: > From: Bartosz Golaszewski <bgolaszewski@baylibre.com> > > GPIO line names are currently created by the driver from the chip label. > We'll want to support custom formats for line names (for instance: to > name all lines the same) for user-space tests so create them in the > module init function and pass them to the driver using the standard > 'gpio-line-names' property. > > Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com> > --- > drivers/gpio/gpio-mockup.c | 70 +++++++++++++++++++++----------------- > 1 file changed, 38 insertions(+), 32 deletions(-) > > diff --git a/drivers/gpio/gpio-mockup.c b/drivers/gpio/gpio-mockup.c > index ce83f1df1933..96976ba66598 100644 > --- a/drivers/gpio/gpio-mockup.c > +++ b/drivers/gpio/gpio-mockup.c > @@ -18,6 +18,7 @@ > #include <linux/platform_device.h> > #include <linux/property.h> > #include <linux/slab.h> > +#include <linux/string_helpers.h> > #include <linux/uaccess.h> > > #include "gpiolib.h" > @@ -378,29 +379,6 @@ static void gpio_mockup_debugfs_setup(struct device *dev, > return; > } > > -static int gpio_mockup_name_lines(struct device *dev, > - struct gpio_mockup_chip *chip) > -{ > - struct gpio_chip *gc = &chip->gc; > - char **names; > - int i; > - > - names = devm_kcalloc(dev, gc->ngpio, sizeof(char *), GFP_KERNEL); > - if (!names) > - return -ENOMEM; > - > - for (i = 0; i < gc->ngpio; i++) { > - names[i] = devm_kasprintf(dev, GFP_KERNEL, > - "%s-%d", gc->label, i); > - if (!names[i]) > - return -ENOMEM; > - } > - > - gc->names = (const char *const *)names; > - > - return 0; > -} > - > static void gpio_mockup_dispose_mappings(void *data) > { > struct gpio_mockup_chip *chip = data; > @@ -468,12 +446,6 @@ static int gpio_mockup_probe(struct platform_device *pdev) > for (i = 0; i < gc->ngpio; i++) > chip->lines[i].dir = GPIO_LINE_DIRECTION_IN; > > - if (device_property_read_bool(dev, "named-gpio-lines")) { > - rv = gpio_mockup_name_lines(dev, chip); > - if (rv) > - return rv; > - } > - > chip->irq_sim_domain = devm_irq_domain_create_sim(dev, NULL, > gc->ngpio); > if (IS_ERR(chip->irq_sim_domain)) > @@ -524,6 +496,27 @@ static void gpio_mockup_unregister_devices(void) > } > } > > +static __init char **gpio_mockup_make_line_names(const char *label, > + unsigned int num_lines) > +{ > + unsigned int i; > + char **names; > + > + names = kcalloc(num_lines + 1, sizeof(char *), GFP_KERNEL); > + if (!names) > + return NULL; > + > + for (i = 0; i < num_lines; i++) { > + names[i] = kasprintf(GFP_KERNEL, "%s-%u", label, i); > + if (!names[i]) { > + kfree_strarray(names, i); > + return NULL; > + } > + } > + > + return names; > +} > + > static int __init gpio_mockup_init(void) > { > struct property_entry properties[GPIO_MOCKUP_MAX_PROP]; > @@ -531,6 +524,7 @@ static int __init gpio_mockup_init(void) > struct gpio_mockup_device *mockup_dev; > int i, prop, num_chips, err = 0, base; > struct platform_device_info pdevinfo; > + char **line_names; > u16 ngpio; > > if ((gpio_mockup_num_ranges < 2) || > @@ -563,6 +557,7 @@ static int __init gpio_mockup_init(void) > memset(properties, 0, sizeof(properties)); > memset(&pdevinfo, 0, sizeof(pdevinfo)); > prop = 0; > + line_names = NULL; > > snprintf(chip_label, sizeof(chip_label), > "gpio-mockup-%c", i + 'A'); > @@ -578,9 +573,18 @@ static int __init gpio_mockup_init(void) > : gpio_mockup_range_ngpio(i) - base; > properties[prop++] = PROPERTY_ENTRY_U16("nr-gpios", ngpio); > > - if (gpio_mockup_named_lines) > - properties[prop++] = PROPERTY_ENTRY_BOOL( > - "named-gpio-lines"); > + if (gpio_mockup_named_lines) { > + line_names = gpio_mockup_make_line_names(chip_label, > + ngpio); > + if (!line_names) { > + err = -ENOMEM; > + goto err_out; > + } > + > + properties[prop++] = PROPERTY_ENTRY_STRING_ARRAY_LEN( > + "gpio-line-names", > + line_names, ngpio); > + } Indentation here looks quite deep. Maybe introduce a helper in between where you assign properties? > pdevinfo.name = "gpio-mockup"; > pdevinfo.id = i; > @@ -588,11 +592,13 @@ static int __init gpio_mockup_init(void) > > mockup_dev = kzalloc(sizeof(*mockup_dev), GFP_KERNEL); > if (!mockup_dev) { > + kfree_strarray(line_names, ngpio); > err = -ENOMEM; > goto err_out; > } > > mockup_dev->pdev = platform_device_register_full(&pdevinfo); > + kfree_strarray(line_names, ngpio); > if (IS_ERR(mockup_dev->pdev)) { > pr_err("error registering device"); > kfree(mockup_dev); > -- > 2.26.1 >
On Fri, Sep 04, 2020 at 05:45:40PM +0200, Bartosz Golaszewski wrote: > From: Bartosz Golaszewski <bgolaszewski@baylibre.com> > > This is in preparation for dynamically created chips. > > Let's split out the code that can be reused when creating chips at > run-time. Let's also move the code preparing the device properties into > a separate routine. This has the advantage of simplifying the error > handling. Almost all contents of this patch should go to proposed helper as I mentioned before. Will make this patch quite small and understandable. > Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com> > --- > drivers/gpio/gpio-mockup.c | 165 ++++++++++++++++++++----------------- > 1 file changed, 90 insertions(+), 75 deletions(-) > > diff --git a/drivers/gpio/gpio-mockup.c b/drivers/gpio/gpio-mockup.c > index 995e37fef9c5..eb94ddac5fee 100644 > --- a/drivers/gpio/gpio-mockup.c > +++ b/drivers/gpio/gpio-mockup.c > @@ -524,16 +524,78 @@ static __init char **gpio_mockup_make_line_names(const char *label, > return names; > } > > -static int __init gpio_mockup_init(void) > +static int __init gpio_mockup_register_device(struct property_entry *properties) > { > - struct property_entry properties[GPIO_MOCKUP_MAX_PROP]; > - char chip_label[GPIO_MOCKUP_LABEL_SIZE]; > struct gpio_mockup_device *mockup_dev; > - int i, prop, num_chips, err = 0, base; > struct platform_device_info pdevinfo; > - char **line_names; > + > + memset(&pdevinfo, 0, sizeof(pdevinfo)); > + > + mockup_dev = kzalloc(sizeof(*mockup_dev), GFP_KERNEL); > + if (!mockup_dev) > + return -ENOMEM; > + > + pdevinfo.name = "gpio-mockup"; > + pdevinfo.properties = properties; > + > + pdevinfo.id = ida_alloc(&gpio_mockup_ida, GFP_KERNEL); > + if (pdevinfo.id < 0) { > + kfree(mockup_dev); > + return pdevinfo.id; > + } > + > + mockup_dev->pdev = platform_device_register_full(&pdevinfo); > + if (IS_ERR(mockup_dev->pdev)) { > + ida_free(&gpio_mockup_ida, pdevinfo.id); > + kfree(mockup_dev); > + return PTR_ERR(mockup_dev->pdev); > + } > + > + list_add(&mockup_dev->list, &gpio_mockup_devices); > + > + return 0; > +} > + > +static int __init gpio_mockup_register_one_chip_from_params(int idx) > +{ > + char chip_label[GPIO_MOCKUP_LABEL_SIZE], **line_names = NULL; > + struct property_entry properties[GPIO_MOCKUP_MAX_PROP]; > + int prop = 0, base, ret; > u16 ngpio; > > + memset(properties, 0, sizeof(properties)); > + > + snprintf(chip_label, sizeof(chip_label), "gpio-mockup-%c", idx + 'A'); > + properties[prop++] = PROPERTY_ENTRY_STRING("chip-label", > + chip_label); > + > + base = gpio_mockup_range_base(idx); > + if (base >= 0) > + properties[prop++] = PROPERTY_ENTRY_U32("gpio-base", > + base); > + > + ngpio = base < 0 ? gpio_mockup_range_ngpio(idx) > + : gpio_mockup_range_ngpio(idx) - base; > + properties[prop++] = PROPERTY_ENTRY_U16("nr-gpios", ngpio); > + > + if (gpio_mockup_named_lines) { > + line_names = gpio_mockup_make_line_names(chip_label, ngpio); > + if (!line_names) > + return -ENOMEM; > + > + properties[prop++] = PROPERTY_ENTRY_STRING_ARRAY_LEN( > + "gpio-line-names", line_names, ngpio); > + } > + > + ret = gpio_mockup_register_device(properties); > + kfree_strarray(line_names, ngpio); > + return ret; > +} > + > +static int __init gpio_mockup_register_chips_from_params(void) > +{ > + int num_chips, i, ret; > + > if ((gpio_mockup_num_ranges < 2) || > (gpio_mockup_num_ranges % 2) || > (gpio_mockup_num_ranges > GPIO_MOCKUP_MAX_RANGES)) > @@ -551,86 +613,39 @@ static int __init gpio_mockup_init(void) > return -EINVAL; > } > > - gpio_mockup_dbg_dir = debugfs_create_dir("gpio-mockup", NULL); > - > - err = platform_driver_register(&gpio_mockup_driver); > - if (err) { > - pr_err("error registering platform driver\n"); > - debugfs_remove_recursive(gpio_mockup_dbg_dir); > - return err; > - } > - > for (i = 0; i < num_chips; i++) { > - memset(properties, 0, sizeof(properties)); > - memset(&pdevinfo, 0, sizeof(pdevinfo)); > - prop = 0; > - line_names = NULL; > - > - snprintf(chip_label, sizeof(chip_label), > - "gpio-mockup-%c", i + 'A'); > - properties[prop++] = PROPERTY_ENTRY_STRING("chip-label", > - chip_label); > - > - base = gpio_mockup_range_base(i); > - if (base >= 0) > - properties[prop++] = PROPERTY_ENTRY_U32("gpio-base", > - base); > - > - ngpio = base < 0 ? gpio_mockup_range_ngpio(i) > - : gpio_mockup_range_ngpio(i) - base; > - properties[prop++] = PROPERTY_ENTRY_U16("nr-gpios", ngpio); > - > - if (gpio_mockup_named_lines) { > - line_names = gpio_mockup_make_line_names(chip_label, > - ngpio); > - if (!line_names) { > - err = -ENOMEM; > - goto err_out; > - } > - > - properties[prop++] = PROPERTY_ENTRY_STRING_ARRAY_LEN( > - "gpio-line-names", > - line_names, ngpio); > + ret = gpio_mockup_register_one_chip_from_params(i); > + if (ret) { > + gpio_mockup_unregister_devices(); > + return ret; > } > + } > > - pdevinfo.name = "gpio-mockup"; > - pdevinfo.properties = properties; > + return 0; > +} > > - pdevinfo.id = ida_alloc(&gpio_mockup_ida, GFP_KERNEL); > - if (pdevinfo.id < 0) { > - kfree_strarray(line_names, ngpio); > - err = pdevinfo.id; > - goto err_out; > - } > +static int __init gpio_mockup_init(void) > +{ > + int ret; > > - mockup_dev = kzalloc(sizeof(*mockup_dev), GFP_KERNEL); > - if (!mockup_dev) { > - kfree_strarray(line_names, ngpio); > - ida_free(&gpio_mockup_ida, pdevinfo.id); > - err = -ENOMEM; > - goto err_out; > - } > + gpio_mockup_dbg_dir = debugfs_create_dir("gpio-mockup", NULL); > > - mockup_dev->pdev = platform_device_register_full(&pdevinfo); > - kfree_strarray(line_names, ngpio); > - if (IS_ERR(mockup_dev->pdev)) { > - pr_err("error registering device"); > - ida_free(&gpio_mockup_ida, pdevinfo.id); > - kfree(mockup_dev); > - err = PTR_ERR(mockup_dev->pdev); > - goto err_out; > - } > + ret = platform_driver_register(&gpio_mockup_driver); > + if (ret) { > + pr_err("error registering platform driver\n"); > + debugfs_remove_recursive(gpio_mockup_dbg_dir); > + return ret; > + } > > - list_add(&mockup_dev->list, &gpio_mockup_devices); > + ret = gpio_mockup_register_chips_from_params(); > + if (ret) { > + pr_err("error registering device"); > + debugfs_remove_recursive(gpio_mockup_dbg_dir); > + platform_driver_unregister(&gpio_mockup_driver); > + return ret; > } > > return 0; > - > -err_out: > - platform_driver_unregister(&gpio_mockup_driver); > - gpio_mockup_unregister_devices(); > - debugfs_remove_recursive(gpio_mockup_dbg_dir); > - return err; > } > > static void __exit gpio_mockup_exit(void) > -- > 2.26.1 >
On Fri, Sep 04, 2020 at 08:15:59PM -0700, Randy Dunlap wrote: > On 9/4/20 8:45 AM, Bartosz Golaszewski wrote: ... > > +GPIO Testing Driver > > +=================== > > + > > +The GPIO Testing Driver (gpio-mockup) provides a way to create simulated GPIO > > +chips for testing purposes. There are two ways of configuring the chips exposed > > +by the module. The lines can be accessed using the standard GPIO character > > +device interface as well as manipulated using the dedicated debugfs directory > > +structure. > > Could configfs be used for this instead of debugfs? > debugfs is ad hoc. Actually sounds like a good idea.
On Mon, Sep 7, 2020 at 1:05 PM Bartosz Golaszewski <bgolaszewski@baylibre.com> wrote: > On Fri, Sep 4, 2020 at 6:35 PM Andy Shevchenko > <andriy.shevchenko@linux.intel.com> wrote: > > On Fri, Sep 04, 2020 at 05:45:27PM +0200, Bartosz Golaszewski wrote: > > > From: Bartosz Golaszewski <bgolaszewski@baylibre.com> > > Doesn't mm/util.c provides us something like this? > > strndup_user()? > > > > Yes, there's both strndup_user() as well as strncpy_from_user(). The > problem is that they rely on the strings being NULL-terminated. This > is not guaranteed for debugfs file_operations write callbacks. We need > some helper that takes the minimum of bytes provided by userspace and > the buffer size and figure out how many bytes to actually copy IMO. Wouldn't this [1] approach work? [1]: https://elixir.bootlin.com/linux/v5.9-rc3/source/arch/x86/kernel/cpu/mtrr/if.c#L93
On Mon, Sep 7, 2020 at 12:19 PM Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > > On Mon, Sep 7, 2020 at 1:05 PM Bartosz Golaszewski > <bgolaszewski@baylibre.com> wrote: > > On Fri, Sep 4, 2020 at 6:35 PM Andy Shevchenko > > <andriy.shevchenko@linux.intel.com> wrote: > > > On Fri, Sep 04, 2020 at 05:45:27PM +0200, Bartosz Golaszewski wrote: > > > > From: Bartosz Golaszewski <bgolaszewski@baylibre.com> > > > > Doesn't mm/util.c provides us something like this? > > > strndup_user()? > > > > > > > Yes, there's both strndup_user() as well as strncpy_from_user(). The > > problem is that they rely on the strings being NULL-terminated. This > > is not guaranteed for debugfs file_operations write callbacks. We need > > some helper that takes the minimum of bytes provided by userspace and > > the buffer size and figure out how many bytes to actually copy IMO. > > Wouldn't this [1] approach work? > > [1]: https://elixir.bootlin.com/linux/v5.9-rc3/source/arch/x86/kernel/cpu/mtrr/if.c#L93 > Sure, but this is pretty much what I do in getline_from_user(). If anything we should port mtrr_write() to using getline_from_user() once it's available upstream, no? Bart
On Fri, Sep 4, 2020 at 6:41 PM Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote: > > On Fri, Sep 04, 2020 at 05:45:30PM +0200, Bartosz Golaszewski wrote: > > From: Bartosz Golaszewski <bgolaszewski@baylibre.com> > > > > We don't need to specify any ranges when allocating IDs so we can switch > > to ida_alloc() and ida_free() instead of the ida_simple_ counterparts. > > > > ida_simple_get(ida, 0, 0, gfp) is equivalent to > > ida_alloc_range(ida, 0, UINT_MAX, gfp) which is equivalent to > > ida_alloc(ida, gfp). Note: IDR will never actually allocate an ID > > larger than INT_MAX. > > Have you considered switching to XArray API? > IDA uses an xarray internally but wraps it in a more straightforward API. No need to use the low-level API IMO. Bart
On Fri, Sep 4, 2020 at 6:44 PM Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote: > > On Fri, Sep 04, 2020 at 05:45:28PM +0200, Bartosz Golaszewski wrote: > > From: Bartosz Golaszewski <bgolaszewski@baylibre.com> > > > > devprop_gpiochip_set_names() is overly complicated with taking the > > fwnode argument (which requires using dev_fwnode() & of_fwnode_handle() > > in ACPI and OF GPIO code respectively). Let's just switch to using the > > generic device properties. > > > > This allows us to pull the code setting line names directly into > > gpiochip_add_data_with_key() instead of handling it separately for > > ACPI and OF. > > > > Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com> > > --- > > drivers/gpio/gpiolib-acpi.c | 3 --- > > drivers/gpio/gpiolib-devprop.c | 19 ++++++++++--------- > > drivers/gpio/gpiolib-of.c | 5 ----- > > drivers/gpio/gpiolib.c | 8 ++++---- > > include/linux/gpio/driver.h | 3 +-- > > 5 files changed, 15 insertions(+), 23 deletions(-) > > > > diff --git a/drivers/gpio/gpiolib-acpi.c b/drivers/gpio/gpiolib-acpi.c > > index 54ca3c18b291..834a12f3219e 100644 > > --- a/drivers/gpio/gpiolib-acpi.c > > +++ b/drivers/gpio/gpiolib-acpi.c > > @@ -1221,9 +1221,6 @@ void acpi_gpiochip_add(struct gpio_chip *chip) > > return; > > } > > > > - if (!chip->names) > > - devprop_gpiochip_set_names(chip, dev_fwnode(chip->parent)); > > - > > acpi_gpiochip_request_regions(acpi_gpio); > > acpi_gpiochip_scan_gpios(acpi_gpio); > > acpi_walk_dep_device_list(handle); > > diff --git a/drivers/gpio/gpiolib-devprop.c b/drivers/gpio/gpiolib-devprop.c > > index 26741032fa9e..a28659b4f9c9 100644 > > --- a/drivers/gpio/gpiolib-devprop.c > > +++ b/drivers/gpio/gpiolib-devprop.c > > @@ -17,25 +17,24 @@ > > /** > > * devprop_gpiochip_set_names - Set GPIO line names using device properties > > * @chip: GPIO chip whose lines should be named, if possible > > - * @fwnode: Property Node containing the gpio-line-names property > > * > > * Looks for device property "gpio-line-names" and if it exists assigns > > * GPIO line names for the chip. The memory allocated for the assigned > > - * names belong to the underlying firmware node and should not be released > > + * names belong to the underlying software node and should not be released > > * by the caller. > > */ > > -void devprop_gpiochip_set_names(struct gpio_chip *chip, > > - const struct fwnode_handle *fwnode) > > +int devprop_gpiochip_set_names(struct gpio_chip *chip) > > { > > struct gpio_device *gdev = chip->gpiodev; > > + struct device *dev = chip->parent; > > const char **names; > > int ret, i; > > int count; > > > > - count = fwnode_property_read_string_array(fwnode, "gpio-line-names", > > + count = device_property_read_string_array(dev, "gpio-line-names", > > NULL, 0); > > if (count < 0) > > - return; > > + return 0; > > Can we introduce a followup to 33ee09cd59ce ("device property: Add helpers to > count items in an array") for strings? > Sure, I'll do this in v2. Bart
On Fri, Sep 4, 2020 at 6:57 PM Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote: > > On Fri, Sep 04, 2020 at 05:45:40PM +0200, Bartosz Golaszewski wrote: > > From: Bartosz Golaszewski <bgolaszewski@baylibre.com> > > > > This is in preparation for dynamically created chips. > > > > Let's split out the code that can be reused when creating chips at > > run-time. Let's also move the code preparing the device properties into > > a separate routine. This has the advantage of simplifying the error > > handling. > > Almost all contents of this patch should go to proposed helper as I mentioned > before. Will make this patch quite small and understandable. > Sorry, I'm not sure what you're referring to. Bart
On Mon, Sep 07, 2020 at 12:28:05PM +0200, Bartosz Golaszewski wrote: > On Mon, Sep 7, 2020 at 12:19 PM Andy Shevchenko > <andy.shevchenko@gmail.com> wrote: > > > > On Mon, Sep 7, 2020 at 1:05 PM Bartosz Golaszewski > > <bgolaszewski@baylibre.com> wrote: > > > On Fri, Sep 4, 2020 at 6:35 PM Andy Shevchenko > > > <andriy.shevchenko@linux.intel.com> wrote: > > > > On Fri, Sep 04, 2020 at 05:45:27PM +0200, Bartosz Golaszewski wrote: > > > > > From: Bartosz Golaszewski <bgolaszewski@baylibre.com> > > > > > > Doesn't mm/util.c provides us something like this? > > > > strndup_user()? > > > > > > > > > > Yes, there's both strndup_user() as well as strncpy_from_user(). The > > > problem is that they rely on the strings being NULL-terminated. This > > > is not guaranteed for debugfs file_operations write callbacks. We need > > > some helper that takes the minimum of bytes provided by userspace and > > > the buffer size and figure out how many bytes to actually copy IMO. > > > > Wouldn't this [1] approach work? > > > > [1]: https://elixir.bootlin.com/linux/v5.9-rc3/source/arch/x86/kernel/cpu/mtrr/if.c#L93 > > > > Sure, but this is pretty much what I do in getline_from_user(). If > anything we should port mtrr_write() to using getline_from_user() once > it's available upstream, no? But you may provide getline_from_user() as inline in the same header where strncpy_from_user() is declared. It will be like 3 LOCs?
On Mon, Sep 07, 2020 at 01:01:02PM +0200, Bartosz Golaszewski wrote: > On Fri, Sep 4, 2020 at 6:48 PM Andy Shevchenko > <andriy.shevchenko@linux.intel.com> wrote: > > On Fri, Sep 04, 2020 at 05:45:37PM +0200, Bartosz Golaszewski wrote: > > > From: Bartosz Golaszewski <bgolaszewski@baylibre.com> > > > > > > While we do check the "chip-name" property in probe(), we never actually > > > use it. Let's pass the chip label to the driver using device properties > > > as we'll want to allow users to define their own once dynamically > > > created chips are supported. > > > > > > The property is renamed to "chip-label" to not cause any confusion with > > > the actual chip name which is of the form: "gpiochipX". > > > > > > If the "chip-label" property is missing, let's do what most devices in > > > drivers/gpio/ do and use dev_name(). > > > > Just wondering if we have a documentation in kernel how this mockup mechanism > > works and what kind of properties it uses. > > > > Side note: moving to software nodes would make some advantages in future such > > as visibility properties and their values (not yet implemented, but there is an > > idea to move forward). > > Seems like we're implicitly using software nodes already: > fwnode_create_software_node() is called when adding device properties. It's not the same APIs, though the former is called from the latter. But maybe for now it's okay.
On Mon, Sep 7, 2020 at 1:45 PM Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > > On Mon, Sep 07, 2020 at 12:28:05PM +0200, Bartosz Golaszewski wrote: > > On Mon, Sep 7, 2020 at 12:19 PM Andy Shevchenko > > <andy.shevchenko@gmail.com> wrote: > > > > > > On Mon, Sep 7, 2020 at 1:05 PM Bartosz Golaszewski > > > <bgolaszewski@baylibre.com> wrote: > > > > On Fri, Sep 4, 2020 at 6:35 PM Andy Shevchenko > > > > <andriy.shevchenko@linux.intel.com> wrote: > > > > > On Fri, Sep 04, 2020 at 05:45:27PM +0200, Bartosz Golaszewski wrote: > > > > > > From: Bartosz Golaszewski <bgolaszewski@baylibre.com> > > > > > > > > Doesn't mm/util.c provides us something like this? > > > > > strndup_user()? > > > > > > > > > > > > > Yes, there's both strndup_user() as well as strncpy_from_user(). The > > > > problem is that they rely on the strings being NULL-terminated. This > > > > is not guaranteed for debugfs file_operations write callbacks. We need > > > > some helper that takes the minimum of bytes provided by userspace and > > > > the buffer size and figure out how many bytes to actually copy IMO. > > > > > > Wouldn't this [1] approach work? > > > > > > [1]: https://elixir.bootlin.com/linux/v5.9-rc3/source/arch/x86/kernel/cpu/mtrr/if.c#L93 > > > > > > > Sure, but this is pretty much what I do in getline_from_user(). If > > anything we should port mtrr_write() to using getline_from_user() once > > it's available upstream, no? > > But you may provide getline_from_user() as inline in the same header where > strncpy_from_user() is declared. It will be like 3 LOCs? > May be more than that. I'll see what I can do. Bart
On Mon, Sep 7, 2020 at 1:53 PM Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote: > > On Mon, Sep 07, 2020 at 12:26:34PM +0200, Bartosz Golaszewski wrote: > > On Mon, Sep 7, 2020 at 11:59 AM Andy Shevchenko > > <andriy.shevchenko@linux.intel.com> wrote: > > > > > > On Fri, Sep 04, 2020 at 08:15:59PM -0700, Randy Dunlap wrote: > > > > On 9/4/20 8:45 AM, Bartosz Golaszewski wrote: > > > > > > ... > > > > > > > > +GPIO Testing Driver > > > > > +=================== > > > > > + > > > > > +The GPIO Testing Driver (gpio-mockup) provides a way to create simulated GPIO > > > > > +chips for testing purposes. There are two ways of configuring the chips exposed > > > > > +by the module. The lines can be accessed using the standard GPIO character > > > > > +device interface as well as manipulated using the dedicated debugfs directory > > > > > +structure. > > > > > > > > Could configfs be used for this instead of debugfs? > > > > debugfs is ad hoc. > > > > > > Actually sounds like a good idea. > > > > > > > Well, then we can go on and write an entirely new mockup driver > > (ditching module params and dropping any backwards compatibility) > > because we're already using debugfs for line values. > > > > How would we pass the device properties to configfs created GPIO chips > > anyway? Devices seem to only be created using mkdir. Am I missing > > something? > > Same way how USB composite works, no? > OK, so create a new chip directory in configfs, configure it using some defined configfs attributes and then finally instantiate it from sysfs? Makes sense and is probably the right way to go. Now the question is: is it fine to just entirely remove the previous gpio-mockup? Should we keep some backwards compatibility? Should we introduce an entirely new module and have a transition period before removing previous gpio-mockup? Also: this is a testing module so to me debugfs is just fine. Is configfs considered stable ABI like sysfs? Bart
On Mon, Sep 7, 2020 at 2:38 PM Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote: > > On Mon, Sep 07, 2020 at 02:06:15PM +0200, Bartosz Golaszewski wrote: > > On Mon, Sep 7, 2020 at 1:53 PM Andy Shevchenko > > <andriy.shevchenko@linux.intel.com> wrote: > > > On Mon, Sep 07, 2020 at 12:26:34PM +0200, Bartosz Golaszewski wrote: > > > > On Mon, Sep 7, 2020 at 11:59 AM Andy Shevchenko > > > > <andriy.shevchenko@linux.intel.com> wrote: > > > > > On Fri, Sep 04, 2020 at 08:15:59PM -0700, Randy Dunlap wrote: > > > > > > On 9/4/20 8:45 AM, Bartosz Golaszewski wrote: > > ... > > > > > > > > +GPIO Testing Driver > > > > > > > +=================== > > > > > > > + > > > > > > > +The GPIO Testing Driver (gpio-mockup) provides a way to create simulated GPIO > > > > > > > +chips for testing purposes. There are two ways of configuring the chips exposed > > > > > > > +by the module. The lines can be accessed using the standard GPIO character > > > > > > > +device interface as well as manipulated using the dedicated debugfs directory > > > > > > > +structure. > > > > > > > > > > > > Could configfs be used for this instead of debugfs? > > > > > > debugfs is ad hoc. > > > > > > > > > > Actually sounds like a good idea. > > > > > > > > > > > > > Well, then we can go on and write an entirely new mockup driver > > > > (ditching module params and dropping any backwards compatibility) > > > > because we're already using debugfs for line values. > > > > > > > > How would we pass the device properties to configfs created GPIO chips > > > > anyway? Devices seem to only be created using mkdir. Am I missing > > > > something? > > > > > > Same way how USB composite works, no? > > > > > > > OK, so create a new chip directory in configfs, configure it using > > some defined configfs attributes and then finally instantiate it from > > sysfs? > > > > Makes sense and is probably the right way to go. Now the question is: > > is it fine to just entirely remove the previous gpio-mockup? > > Since, for example, I never saw device property bindings for that driver I > assume that it was never considered as an ABI, so feel free to hack it in > either direction. > > > Should we > > keep some backwards compatibility? > > I wouldn't probably spend time on this. > > > Should we introduce an entirely new > > module and have a transition period before removing previous > > gpio-mockup? > > Neither transition period. > I wouldn't rush this actually. gpio-mockup is used a lot by libgpiod and probably by Kent's Go library. My main goal with this series is to extend it to allow for more advanced testing like simulating spurious irqs to test the software debouncer or custom line name formats to test name lookups. I need to think about it some more. An entirely new configfs interface would take time too. Bart
On Mon, Sep 07, 2020 at 02:57:29PM +0200, Bartosz Golaszewski wrote: > On Mon, Sep 7, 2020 at 2:38 PM Andy Shevchenko > <andriy.shevchenko@linux.intel.com> wrote: > > > > On Mon, Sep 07, 2020 at 02:06:15PM +0200, Bartosz Golaszewski wrote: > > > On Mon, Sep 7, 2020 at 1:53 PM Andy Shevchenko > > > <andriy.shevchenko@linux.intel.com> wrote: > > > > On Mon, Sep 07, 2020 at 12:26:34PM +0200, Bartosz Golaszewski wrote: > > > > > On Mon, Sep 7, 2020 at 11:59 AM Andy Shevchenko > > > > > <andriy.shevchenko@linux.intel.com> wrote: > > > > > > On Fri, Sep 04, 2020 at 08:15:59PM -0700, Randy Dunlap wrote: > > > > > > > On 9/4/20 8:45 AM, Bartosz Golaszewski wrote: > > > > ... > > > > > > > > > > +GPIO Testing Driver > > > > > > > > +=================== > > > > > > > > + > > > > > > > > +The GPIO Testing Driver (gpio-mockup) provides a way to create simulated GPIO > > > > > > > > +chips for testing purposes. There are two ways of configuring the chips exposed > > > > > > > > +by the module. The lines can be accessed using the standard GPIO character > > > > > > > > +device interface as well as manipulated using the dedicated debugfs directory > > > > > > > > +structure. > > > > > > > > > > > > > > Could configfs be used for this instead of debugfs? > > > > > > > debugfs is ad hoc. > > > > > > > > > > > > Actually sounds like a good idea. > > > > > > > > > > > > > > > > Well, then we can go on and write an entirely new mockup driver > > > > > (ditching module params and dropping any backwards compatibility) > > > > > because we're already using debugfs for line values. > > > > > > > > > > How would we pass the device properties to configfs created GPIO chips > > > > > anyway? Devices seem to only be created using mkdir. Am I missing > > > > > something? > > > > > > > > Same way how USB composite works, no? > > > > > > > > > > OK, so create a new chip directory in configfs, configure it using > > > some defined configfs attributes and then finally instantiate it from > > > sysfs? > > > > > > Makes sense and is probably the right way to go. Now the question is: > > > is it fine to just entirely remove the previous gpio-mockup? > > > > Since, for example, I never saw device property bindings for that driver I > > assume that it was never considered as an ABI, so feel free to hack it in > > either direction. > > > > > Should we > > > keep some backwards compatibility? > > > > I wouldn't probably spend time on this. > > > > > Should we introduce an entirely new > > > module and have a transition period before removing previous > > > gpio-mockup? > > > > Neither transition period. > > > > I wouldn't rush this actually. gpio-mockup is used a lot by libgpiod > and probably by Kent's Go library. My main goal with this series is to > extend it to allow for more advanced testing like simulating spurious > irqs to test the software debouncer or custom line name formats to > test name lookups. When I wrote above I didn't mean this should be done in a hurry. Just during one release to make all parties ready for the change. > I need to think about it some more. An entirely new configfs interface > would take time too. Sure!
On Mon, Sep 7, 2020 at 4:08 PM Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote: > > On Mon, Sep 07, 2020 at 03:49:23PM +0200, Bartosz Golaszewski wrote: > > On Mon, Sep 7, 2020 at 2:22 PM Greg Kroah-Hartman > > <gregkh@linuxfoundation.org> wrote: > > > On Mon, Sep 07, 2020 at 02:06:15PM +0200, Bartosz Golaszewski wrote: > > ... > > > > Yes it is. Or at least until you fix all existing users so that if you > > > do change it, no one notices it happening :) > > > > > > > Then another question is: do we really want to commit to a stable ABI > > for a module we only use for testing purposes and which doesn't > > interact with any real hardware. > > > > Rewriting this module without any legacy cruft is tempting though. :) > > Another thought spoken loudly: maybe it can be unified with GPIO aggregator > code? In that case it makes sense. > Cc'ing Geert but I don't quite see how this would make sense. :) Also one thing I'm not sure about re configfs is the interface we use to read values/set pull i.e. the line attributes in debugfs, do you think configfs allows this type of attributes? Bart
On Mon, Sep 7, 2020 at 5:23 PM Geert Uytterhoeven <geert@linux-m68k.org> wrote: > > Hi Andy, > > On Mon, Sep 7, 2020 at 4:14 PM Andy Shevchenko > <andriy.shevchenko@linux.intel.com> wrote: > > On Mon, Sep 07, 2020 at 03:49:23PM +0200, Bartosz Golaszewski wrote: > > > On Mon, Sep 7, 2020 at 2:22 PM Greg Kroah-Hartman > > > <gregkh@linuxfoundation.org> wrote: > > > > On Mon, Sep 07, 2020 at 02:06:15PM +0200, Bartosz Golaszewski wrote: > > > > ... > > > > > > Yes it is. Or at least until you fix all existing users so that if you > > > > do change it, no one notices it happening :) > > > > > > > > > > Then another question is: do we really want to commit to a stable ABI > > > for a module we only use for testing purposes and which doesn't > > > interact with any real hardware. > > > > > > Rewriting this module without any legacy cruft is tempting though. :) > > > > Another thought spoken loudly: maybe it can be unified with GPIO aggregator > > code? In that case it makes sense. > > You want to aggregate GPIOs out of thin air? > > From DT, that would be something like > > gpios = <&gpio1 2>, <0>, <0>, <&gpio2, 5>; > > ? > > For writing into ".../new_device", we could agree on something like "0" > means not backed by an existing GPIO? > I'm really not sure this makes any sense. Why complicate an otherwise elegant module that is gpio-aggregator with functionalities that obviously don't belong here? I want to add various parameters that would affect the way the simulated chips work - this really doesn't need to go into the aggregator. Bart
On Tue, Sep 08, 2020 at 07:03:30PM +0200, Bartosz Golaszewski wrote: > On Mon, Sep 7, 2020 at 2:22 PM Greg Kroah-Hartman > <gregkh@linuxfoundation.org> wrote: > > > > On Mon, Sep 07, 2020 at 02:06:15PM +0200, Bartosz Golaszewski wrote: > > > On Mon, Sep 7, 2020 at 1:53 PM Andy Shevchenko > > > <andriy.shevchenko@linux.intel.com> wrote: > > > > > > > > On Mon, Sep 07, 2020 at 12:26:34PM +0200, Bartosz Golaszewski wrote: > > > > > On Mon, Sep 7, 2020 at 11:59 AM Andy Shevchenko > > > > > <andriy.shevchenko@linux.intel.com> wrote: > > > > > > > > > > > > On Fri, Sep 04, 2020 at 08:15:59PM -0700, Randy Dunlap wrote: > > > > > > > On 9/4/20 8:45 AM, Bartosz Golaszewski wrote: > > > > > > > > > > > > ... > > > > > > > > > > > > > > +GPIO Testing Driver > > > > > > > > +=================== > > > > > > > > + > > > > > > > > +The GPIO Testing Driver (gpio-mockup) provides a way to create simulated GPIO > > > > > > > > +chips for testing purposes. There are two ways of configuring the chips exposed > > > > > > > > +by the module. The lines can be accessed using the standard GPIO character > > > > > > > > +device interface as well as manipulated using the dedicated debugfs directory > > > > > > > > +structure. > > > > > > > > > > > > > > Could configfs be used for this instead of debugfs? > > > > > > > debugfs is ad hoc. > > > > > > > > > > > > Actually sounds like a good idea. > > > > > > > > > > > > > > > > Well, then we can go on and write an entirely new mockup driver > > > > > (ditching module params and dropping any backwards compatibility) > > > > > because we're already using debugfs for line values. > > > > > > > > > > How would we pass the device properties to configfs created GPIO chips > > > > > anyway? Devices seem to only be created using mkdir. Am I missing > > > > > something? > > > > > > > > Same way how USB composite works, no? > > > > > > > > > > OK, so create a new chip directory in configfs, configure it using > > > some defined configfs attributes and then finally instantiate it from > > > sysfs? > > > > > > Makes sense and is probably the right way to go. Now the question is: > > > is it fine to just entirely remove the previous gpio-mockup? Should we > > > keep some backwards compatibility? Should we introduce an entirely new > > > module and have a transition period before removing previous > > > gpio-mockup? > > > > > > Also: this is a testing module so to me debugfs is just fine. Is > > > configfs considered stable ABI like sysfs? > > > > Yes it is. Or at least until you fix all existing users so that if you > > do change it, no one notices it happening :) > > > > Got it. One more question: the current debugfs interface we're using > in gpio-mockup exists to allow to read current values of GPIO lines in > output mode (check how the user drives dummy lines) and to set their > simulated pull-up/pull-down resistors (what values the user reads in > input mode). > > This works like this: in /sys/kernel/debug/gpio-mockup every dummy > chip creates its own directory (e.g. > /sys/kernel/debug/gpio-mockup/gpiochip0) and inside this directory > there's an attribute per line named after the line's offset (e.g. > /sys/kernel/debug/gpio-mockup/gpiochip0/4). Writing 0 or 1 to this > attribute sets the pull resistor. Reading from it yields the current > value (0 or 1 as well). > > This is pretty non-standard so I proposed to put it in debugfs. If we > were to use configfs - is this where something like this should go? Or > rather sysfs? Is it even suitable/acceptable for sysfs? That sounds like it would work in sysfs just fine as-is, why don't you all want to use that? configfs is good for "set a bunch of attributes to different values and then do a 'create/go/work'" type action. thanks, greg k-h
From: Bartosz Golaszewski <bgolaszewski@baylibre.com> We're about to merge w V2 user API for GPIO. In user-space we're using the gpio-mockup driver for testing but it's quite cumbersome (needs unloading and reloading to change chip configuration) and not very extensible (config is passed over module params). This series proposes to extend the debugfs interface to support dynamic creation and removal of dummy chips, with extensible options. First 3 patches add some lib functionality we'll use later on. Next 3 contain general gpiolib refactoring and can be picked up independently. Next we refactor gpio-mockup and finally add the delete_device and new_device attributes. Last patch adds documentation for gpio-mockup so I'm not going into detail on how the new interface works - the doc describes it pretty well. Bartosz Golaszewski (23): lib: cmdline: export next_arg() lib: string_helpers: provide kfree_strarray() lib: uaccess: provide getline_from_user() gpiolib: generalize devprop_gpiochip_set_names() for device properties gpiolib: unexport devprop_gpiochip_set_names() gpiolib: switch to simpler IDA interface gpio: mockup: drop unneeded includes gpio: mockup: use pr_fmt() gpio: mockup: use KBUILD_MODNAME gpio: mockup: fix resource leak in error path gpio: mockup: remove the limit on number of dummy chips gpio: mockup: define a constant for chip label size gpio: mockup: pass the chip label as device property gpio: mockup: use the generic 'gpio-line-names' property gpio: mockup: use dynamic device IDs gpio: mockup: refactor the module init function gpio: mockup: rename and move around debugfs callbacks gpio: mockup: require debugfs to build gpio: mockup: add a symlink for the per-chip debugfs directory gpio: mockup: add a lock for dummy device list gpio: mockup: provide a way to delete dummy chips gpio: mockup: provide a way to create new dummy chips Documentation: gpio: add documentation for gpio-mockup .../admin-guide/gpio/gpio-mockup.rst | 87 +++ drivers/gpio/Kconfig | 1 + drivers/gpio/Makefile | 1 - drivers/gpio/gpio-mockup.c | 614 ++++++++++++++---- drivers/gpio/gpiolib-acpi.c | 3 - drivers/gpio/gpiolib-devprop.c | 63 -- drivers/gpio/gpiolib-of.c | 5 - drivers/gpio/gpiolib.c | 62 +- include/linux/gpio/driver.h | 3 - include/linux/string_helpers.h | 2 + include/linux/uaccess.h | 3 + lib/cmdline.c | 1 + lib/string_helpers.c | 22 + lib/usercopy.c | 37 ++ 14 files changed, 705 insertions(+), 199 deletions(-) create mode 100644 Documentation/admin-guide/gpio/gpio-mockup.rst delete mode 100644 drivers/gpio/gpiolib-devprop.c