From: Bartosz Golaszewski <bgolaszewski@baylibre.com> To: Geert Uytterhoeven <geert@linux-m68k.org> Cc: Geert Uytterhoeven <geert+renesas@glider.be>, Linus Walleij <linus.walleij@linaro.org>, Alexander Graf <agraf@suse.de>, Peter Maydell <peter.maydell@linaro.org>, Paolo Bonzini <pbonzini@redhat.com>, Magnus Damm <magnus.damm@gmail.com>, linux-gpio <linux-gpio@vger.kernel.org>, QEMU Developers <qemu-devel@nongnu.org>, Linux-Renesas <linux-renesas-soc@vger.kernel.org>, LKML <linux-kernel@vger.kernel.org> Subject: Re: [PATCH RFC] gpio: Add Virtual Aggregator GPIO Driver Date: Tue, 9 Jul 2019 16:58:48 +0200 [thread overview] Message-ID: <CAMpxmJUF1s1zyXVtoUGfbV7Yk+heua4rNjY=DrX=jr-v8UfNxA@mail.gmail.com> (raw) In-Reply-To: <CAMuHMdWdb0dcS8Nvk-Poz2dT7nuHjFhqpsRPZZnSKsc3VffcRA@mail.gmail.com> pon., 8 lip 2019 o 12:24 Geert Uytterhoeven <geert@linux-m68k.org> napisał(a): > > Hi Bartosz, > > On Mon, Jul 8, 2019 at 11:45 AM Bartosz Golaszewski > <bgolaszewski@baylibre.com> wrote: > > pt., 5 lip 2019 o 18:05 Geert Uytterhoeven <geert+renesas@glider.be> napisał(a): > > > GPIO controllers are exported to userspace using /dev/gpiochip* > > > character devices. Access control to these devices is provided by > > > standard UNIX file system permissions, on an all-or-nothing basis: > > > either a GPIO controller is accessible for a user, or it is not. > > > Currently no mechanism exists to control access to individual GPIOs. > > > > > > Hence add a virtual GPIO driver to aggregate existing GPIOs (up to 32), > > > and expose them as a new gpiochip. This is useful for implementing > > > access control, and assigning a set of GPIOs to a specific user. > > > Furthermore, it would simplify and harden exporting GPIOs to a virtual > > > machine, as the VM can just grab the full virtual GPIO controller, and > > > no longer needs to care about which GPIOs to grab and which not, > > > reducing the attack surface. > > > > > > Virtual GPIO controllers are instantiated by writing to the "new_device" > > > attribute file in sysfs: > > > > > > $ echo "<gpiochipA> <gpioA1> [<gpioA2> ...]" > > > "[, <gpiochipB> <gpioB1> [<gpioB2> ...]] ...]" > > > > /sys/bus/platform/drivers/gpio-virt-agg/new_device > > > > > > Likewise, virtual GPIO controllers can be destroyed after use: > > > > > > $ echo gpio-virt-agg.<N> \ > > > > /sys/bus/platform/drivers/gpio-virt-agg/delete_device > > > > > > Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be> > > > I like the general idea and the interface looks mostly fine. Since > > this is new ABI I think it needs to be documented as well. > > Sure. > > > I'm having trouble building this module: > > > > CALL scripts/atomic/check-atomics.sh > > CALL scripts/checksyscalls.sh > > CHK include/generated/compile.h > > Kernel: arch/arm/boot/Image is ready > > Building modules, stage 2. > > MODPOST 235 modules > > ERROR: "gpiod_request" [drivers/gpio/gpio-virt-agg.ko] undefined! > > ERROR: "gpiochip_get_desc" [drivers/gpio/gpio-virt-agg.ko] undefined! > > ERROR: "gpiod_free" [drivers/gpio/gpio-virt-agg.ko] undefined! > > scripts/Makefile.modpost:91: recipe for target '__modpost' failed > > make[1]: *** [__modpost] Error 1 > > Makefile:1287: recipe for target 'modules' failed > > make: *** [modules] Error 2 > > make: *** Waiting for unfinished jobs.... > > > > I'm not sure what the problem is. > > Oops. As this is an RFC, I didn't bother trying to build this driver as > a module, only builtin. Apparently the 3 symbols above are not yet > exported using EXPORT_SYMBOL_GPL(). > Am I doing it right? I'm trying to create a device and am only getting this: # echo gpiochip2 23 > new_device [ 707.507039] gpio-virt-agg gpio-virt-agg.0: Cannot find gpiochip gpiochip2 gpiochip2 *does* exist in the system. > > > --- /dev/null > > > +++ b/drivers/gpio/gpio-virt-agg.c > > > @@ -0,0 +1,390 @@ > > > +// SPDX-License-Identifier: GPL-2.0-only > > > +// > > > +// GPIO Virtual Aggregator > > > +// > > > +// Copyright (C) 2019 Glider bvba > > > + > > > +#include <linux/gpio/driver.h> > > > +#include <linux/idr.h> > > > +#include <linux/kernel.h> > > > +#include <linux/module.h> > > > +#include <linux/mutex.h> > > > +#include <linux/platform_device.h> > > > + > > > +#include "gpiolib.h" > > > + > > > +#define DRV_NAME "gpio-virt-agg" > > > +#define MAX_GPIOS 32 > > > > Do we really need this limit? I see it simplifies the code, but maybe > > we can allocate the relevant arrays dynamically and not limit users? > > Sure. That limit can be lifted. > > > > +static int gpio_virt_agg_set_config(struct gpio_chip *chip, > > > + unsigned int offset, unsigned long config) > > > +{ > > > + struct gpio_virt_agg_priv *priv = gpiochip_get_data(chip); > > > + > > > + chip = priv->desc[offset]->gdev->chip; > > > + if (chip->set_config) > > > + return chip->set_config(chip, offset, config); > > > + > > > + // FIXME gpiod_set_transitory() expects success if not implemented > > BTW, do you have a comment about this FIXME? > Ha! Interesting. I'll give it a thought and respond elsewhere as it's a different subject. > > > + return -ENOTSUPP; > > > +} > > > > +static int gpio_virt_agg_probe(struct platform_device *pdev) > > > +{ > > > + struct device *dev = &pdev->dev; > > > + const char *param = dev_get_platdata(dev); > > > + struct gpio_virt_agg_priv *priv; > > > + const char *label = NULL; > > > + struct gpio_chip *chip; > > > + struct gpio_desc *desc; > > > + unsigned int offset; > > > + int error, i; > > > > This 'i' here is reported as possibly not initialized: > > > > drivers/gpio/gpio-virt-agg.c: In function ‘gpio_virt_agg_probe’: > > drivers/gpio/gpio-virt-agg.c:230:13: warning: ‘i’ may be used > > uninitialized in this function [-Wmaybe-uninitialized] > > int error, i; > > ^ > > Oops, should be preinitialized to zero. WIll fix. > > > > +static int gpio_virt_agg_remove(struct platform_device *pdev) > > > +{ > > > + struct gpio_virt_agg_priv *priv = platform_get_drvdata(pdev); > > > + unsigned int i; > > > + > > > + gpiochip_remove(&priv->chip); > > > + > > > + for (i = 0; i < priv->chip.ngpio; i++) > > > + gpiod_free(priv->desc[i]); > > Perhaps I should use gpiod_put() instead, which is exported to modules? > > > > + > > > + return 0; > > > +} > > > > You shouldn't need this function at all. It's up to users to free descriptors. > > This frees the upstream descriptors, not the descriptors used by users > of the virtual gpiochip. Shouldn't they be freed, as they are no longer > in use? > > Note that .probe() doesn't use devm_gpiochip_add_data(), as the upstream > descriptors need to be freed after the call to gpiochip_remove(). > > Thanks! I see. I'll try to review it more thoroughly once I get to play with it. So far I'm stuck on creating the virtual chip. Bart > > Gr{oetje,eeting}s, > > Geert > > -- > Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org > > In personal conversations with technical people, I call myself a hacker. But > when I'm talking to journalists I just say "programmer" or something like that. > -- Linus Torvalds
WARNING: multiple messages have this Message-ID (diff)
From: Bartosz Golaszewski <bgolaszewski@baylibre.com> To: Geert Uytterhoeven <geert@linux-m68k.org> Cc: Peter Maydell <peter.maydell@linaro.org>, Geert Uytterhoeven <geert+renesas@glider.be>, LKML <linux-kernel@vger.kernel.org>, Linus Walleij <linus.walleij@linaro.org>, Magnus Damm <magnus.damm@gmail.com>, Alexander Graf <agraf@suse.de>, QEMU Developers <qemu-devel@nongnu.org>, Linux-Renesas <linux-renesas-soc@vger.kernel.org>, linux-gpio <linux-gpio@vger.kernel.org>, Paolo Bonzini <pbonzini@redhat.com> Subject: Re: [Qemu-devel] [PATCH RFC] gpio: Add Virtual Aggregator GPIO Driver Date: Tue, 9 Jul 2019 16:58:48 +0200 [thread overview] Message-ID: <CAMpxmJUF1s1zyXVtoUGfbV7Yk+heua4rNjY=DrX=jr-v8UfNxA@mail.gmail.com> (raw) In-Reply-To: <CAMuHMdWdb0dcS8Nvk-Poz2dT7nuHjFhqpsRPZZnSKsc3VffcRA@mail.gmail.com> pon., 8 lip 2019 o 12:24 Geert Uytterhoeven <geert@linux-m68k.org> napisał(a): > > Hi Bartosz, > > On Mon, Jul 8, 2019 at 11:45 AM Bartosz Golaszewski > <bgolaszewski@baylibre.com> wrote: > > pt., 5 lip 2019 o 18:05 Geert Uytterhoeven <geert+renesas@glider.be> napisał(a): > > > GPIO controllers are exported to userspace using /dev/gpiochip* > > > character devices. Access control to these devices is provided by > > > standard UNIX file system permissions, on an all-or-nothing basis: > > > either a GPIO controller is accessible for a user, or it is not. > > > Currently no mechanism exists to control access to individual GPIOs. > > > > > > Hence add a virtual GPIO driver to aggregate existing GPIOs (up to 32), > > > and expose them as a new gpiochip. This is useful for implementing > > > access control, and assigning a set of GPIOs to a specific user. > > > Furthermore, it would simplify and harden exporting GPIOs to a virtual > > > machine, as the VM can just grab the full virtual GPIO controller, and > > > no longer needs to care about which GPIOs to grab and which not, > > > reducing the attack surface. > > > > > > Virtual GPIO controllers are instantiated by writing to the "new_device" > > > attribute file in sysfs: > > > > > > $ echo "<gpiochipA> <gpioA1> [<gpioA2> ...]" > > > "[, <gpiochipB> <gpioB1> [<gpioB2> ...]] ...]" > > > > /sys/bus/platform/drivers/gpio-virt-agg/new_device > > > > > > Likewise, virtual GPIO controllers can be destroyed after use: > > > > > > $ echo gpio-virt-agg.<N> \ > > > > /sys/bus/platform/drivers/gpio-virt-agg/delete_device > > > > > > Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be> > > > I like the general idea and the interface looks mostly fine. Since > > this is new ABI I think it needs to be documented as well. > > Sure. > > > I'm having trouble building this module: > > > > CALL scripts/atomic/check-atomics.sh > > CALL scripts/checksyscalls.sh > > CHK include/generated/compile.h > > Kernel: arch/arm/boot/Image is ready > > Building modules, stage 2. > > MODPOST 235 modules > > ERROR: "gpiod_request" [drivers/gpio/gpio-virt-agg.ko] undefined! > > ERROR: "gpiochip_get_desc" [drivers/gpio/gpio-virt-agg.ko] undefined! > > ERROR: "gpiod_free" [drivers/gpio/gpio-virt-agg.ko] undefined! > > scripts/Makefile.modpost:91: recipe for target '__modpost' failed > > make[1]: *** [__modpost] Error 1 > > Makefile:1287: recipe for target 'modules' failed > > make: *** [modules] Error 2 > > make: *** Waiting for unfinished jobs.... > > > > I'm not sure what the problem is. > > Oops. As this is an RFC, I didn't bother trying to build this driver as > a module, only builtin. Apparently the 3 symbols above are not yet > exported using EXPORT_SYMBOL_GPL(). > Am I doing it right? I'm trying to create a device and am only getting this: # echo gpiochip2 23 > new_device [ 707.507039] gpio-virt-agg gpio-virt-agg.0: Cannot find gpiochip gpiochip2 gpiochip2 *does* exist in the system. > > > --- /dev/null > > > +++ b/drivers/gpio/gpio-virt-agg.c > > > @@ -0,0 +1,390 @@ > > > +// SPDX-License-Identifier: GPL-2.0-only > > > +// > > > +// GPIO Virtual Aggregator > > > +// > > > +// Copyright (C) 2019 Glider bvba > > > + > > > +#include <linux/gpio/driver.h> > > > +#include <linux/idr.h> > > > +#include <linux/kernel.h> > > > +#include <linux/module.h> > > > +#include <linux/mutex.h> > > > +#include <linux/platform_device.h> > > > + > > > +#include "gpiolib.h" > > > + > > > +#define DRV_NAME "gpio-virt-agg" > > > +#define MAX_GPIOS 32 > > > > Do we really need this limit? I see it simplifies the code, but maybe > > we can allocate the relevant arrays dynamically and not limit users? > > Sure. That limit can be lifted. > > > > +static int gpio_virt_agg_set_config(struct gpio_chip *chip, > > > + unsigned int offset, unsigned long config) > > > +{ > > > + struct gpio_virt_agg_priv *priv = gpiochip_get_data(chip); > > > + > > > + chip = priv->desc[offset]->gdev->chip; > > > + if (chip->set_config) > > > + return chip->set_config(chip, offset, config); > > > + > > > + // FIXME gpiod_set_transitory() expects success if not implemented > > BTW, do you have a comment about this FIXME? > Ha! Interesting. I'll give it a thought and respond elsewhere as it's a different subject. > > > + return -ENOTSUPP; > > > +} > > > > +static int gpio_virt_agg_probe(struct platform_device *pdev) > > > +{ > > > + struct device *dev = &pdev->dev; > > > + const char *param = dev_get_platdata(dev); > > > + struct gpio_virt_agg_priv *priv; > > > + const char *label = NULL; > > > + struct gpio_chip *chip; > > > + struct gpio_desc *desc; > > > + unsigned int offset; > > > + int error, i; > > > > This 'i' here is reported as possibly not initialized: > > > > drivers/gpio/gpio-virt-agg.c: In function ‘gpio_virt_agg_probe’: > > drivers/gpio/gpio-virt-agg.c:230:13: warning: ‘i’ may be used > > uninitialized in this function [-Wmaybe-uninitialized] > > int error, i; > > ^ > > Oops, should be preinitialized to zero. WIll fix. > > > > +static int gpio_virt_agg_remove(struct platform_device *pdev) > > > +{ > > > + struct gpio_virt_agg_priv *priv = platform_get_drvdata(pdev); > > > + unsigned int i; > > > + > > > + gpiochip_remove(&priv->chip); > > > + > > > + for (i = 0; i < priv->chip.ngpio; i++) > > > + gpiod_free(priv->desc[i]); > > Perhaps I should use gpiod_put() instead, which is exported to modules? > > > > + > > > + return 0; > > > +} > > > > You shouldn't need this function at all. It's up to users to free descriptors. > > This frees the upstream descriptors, not the descriptors used by users > of the virtual gpiochip. Shouldn't they be freed, as they are no longer > in use? > > Note that .probe() doesn't use devm_gpiochip_add_data(), as the upstream > descriptors need to be freed after the call to gpiochip_remove(). > > Thanks! I see. I'll try to review it more thoroughly once I get to play with it. So far I'm stuck on creating the virtual chip. Bart > > Gr{oetje,eeting}s, > > Geert > > -- > Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org > > In personal conversations with technical people, I call myself a hacker. But > when I'm talking to journalists I just say "programmer" or something like that. > -- Linus Torvalds
next prev parent reply other threads:[~2019-07-09 14:59 UTC|newest] Thread overview: 34+ messages / expand[flat|nested] mbox.gz Atom feed top 2019-07-05 16:05 [PATCH RFC] gpio: Add Virtual Aggregator GPIO Driver Geert Uytterhoeven 2019-07-05 16:05 ` [Qemu-devel] " Geert Uytterhoeven 2019-07-08 9:44 ` Bartosz Golaszewski 2019-07-08 9:44 ` [Qemu-devel] " Bartosz Golaszewski 2019-07-08 10:24 ` Geert Uytterhoeven 2019-07-08 10:24 ` [Qemu-devel] " Geert Uytterhoeven 2019-07-09 14:58 ` Bartosz Golaszewski [this message] 2019-07-09 14:58 ` Bartosz Golaszewski 2019-07-09 15:59 ` Geert Uytterhoeven 2019-07-09 15:59 ` [Qemu-devel] " Geert Uytterhoeven 2019-07-12 9:27 ` Bartosz Golaszewski 2019-07-12 9:27 ` [Qemu-devel] " Bartosz Golaszewski 2019-09-06 11:14 ` Geert Uytterhoeven 2019-09-06 11:14 ` [Qemu-devel] " Geert Uytterhoeven 2019-09-06 11:09 ` Geert Uytterhoeven 2019-09-06 11:09 ` [Qemu-devel] " Geert Uytterhoeven 2019-07-10 2:00 ` Phil Reid 2019-07-10 2:00 ` [Qemu-devel] " Phil Reid 2019-07-10 10:21 ` Geert Uytterhoeven 2019-07-10 10:21 ` [Qemu-devel] " Geert Uytterhoeven 2019-07-11 9:24 ` Phil Reid 2019-07-11 9:24 ` [Qemu-devel] " Phil Reid 2019-07-11 9:54 ` Geert Uytterhoeven 2019-07-11 9:54 ` [Qemu-devel] " Geert Uytterhoeven 2019-08-01 8:41 ` Linus Walleij 2019-08-01 8:41 ` [Qemu-devel] " Linus Walleij 2019-08-05 10:21 ` Marc Zyngier 2019-08-05 10:21 ` [Qemu-devel] " Marc Zyngier 2019-08-05 10:56 ` Linus Walleij 2019-08-05 10:56 ` [Qemu-devel] " Linus Walleij 2019-09-12 8:56 ` Linus Walleij 2019-09-12 8:56 ` [Qemu-devel] " Linus Walleij 2019-09-12 8:59 ` Geert Uytterhoeven 2019-09-12 8:59 ` [Qemu-devel] " Geert Uytterhoeven
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to='CAMpxmJUF1s1zyXVtoUGfbV7Yk+heua4rNjY=DrX=jr-v8UfNxA@mail.gmail.com' \ --to=bgolaszewski@baylibre.com \ --cc=agraf@suse.de \ --cc=geert+renesas@glider.be \ --cc=geert@linux-m68k.org \ --cc=linus.walleij@linaro.org \ --cc=linux-gpio@vger.kernel.org \ --cc=linux-kernel@vger.kernel.org \ --cc=linux-renesas-soc@vger.kernel.org \ --cc=magnus.damm@gmail.com \ --cc=pbonzini@redhat.com \ --cc=peter.maydell@linaro.org \ --cc=qemu-devel@nongnu.org \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.