lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
Open Source and information security mailing list archives
| ||
|
Message-ID: <CAAVeFuJJ8TGNDKA-MQa1_5sigvWXfOeBDsujmZ4GDBpkHDe+dw@mail.gmail.com> Date: Tue, 14 Oct 2014 15:04:31 +0900 From: Alexandre Courbot <gnurou@...il.com> To: Sören Brinkmann <soren.brinkmann@...inx.com> Cc: Linus Walleij <linus.walleij@...aro.org>, Linux Kernel Mailing List <linux-kernel@...r.kernel.org>, "linux-gpio@...r.kernel.org" <linux-gpio@...r.kernel.org> Subject: Re: [PATCH RESEND] gpio: lib-sysfs: Add 'wakeup' attribute On Tue, Oct 14, 2014 at 8:47 AM, Sören Brinkmann <soren.brinkmann@...inx.com> wrote: > Hi Alexandre, > > On Sat, 2014-10-11 at 01:54PM +0900, Alexandre Courbot wrote: >> On Fri, Sep 5, 2014 at 2:58 AM, Soren Brinkmann >> <soren.brinkmann@...inx.com> wrote: >> > Add an attribute 'wakeup' to the GPIO sysfs interface which allows >> > marking/unmarking a GPIO as wake IRQ. >> > The file 'wakeup' is created in each exported GPIOs directory, if an IRQ >> > is associated with that GPIO and the irqchip implements set_wake(). >> > Writing 'enabled' to that file will enable wake for that GPIO, while >> > writing 'disabled' will disable wake. >> > Reading that file will return either 'disabled' or 'enabled' depening on >> > the currently set flag for the GPIO's IRQ. >> > >> > Signed-off-by: Soren Brinkmann <soren.brinkmann@...inx.com> >> > --- >> > Hi all, >> > >> > I originally submitted this patch with a few fixes for Zynq's GPIO driver >> > (https://lkml.org/lkml/2014/8/29/391). Since this change is not just >> > Zynq-related and has broader impact, Linus asked me to post this again, separate >> > from the Zynq series. >> > >> > Let me just quote myself from the original submission: >> > "I'm still not fully convinced that the gpio_keys are the best >> > replacement for the sysfs interface when it comes to inputs. For that >> > reason and to have a way to do some quick wake testing, I'd like to >> > propose adding the ability to control wake through the sysfs interface >> > (patch 3)." >> >> I'm really sorry that I did not provide feedback sooner. This is the >> kind of area (IRQ) where I am not too confident and typically like to >> hear what Linus has to say first. But I also have a few questions that >> you could maybe answer for my own education. :) >> >> > >> > Thanks, >> > Sören >> > >> > drivers/gpio/gpiolib-sysfs.c | 75 ++++++++++++++++++++++++++++++++++++++++---- >> > 1 file changed, 69 insertions(+), 6 deletions(-) >> > >> > diff --git a/drivers/gpio/gpiolib-sysfs.c b/drivers/gpio/gpiolib-sysfs.c >> > index 5f2150b619a7..aaf021eaaff5 100644 >> > --- a/drivers/gpio/gpiolib-sysfs.c >> > +++ b/drivers/gpio/gpiolib-sysfs.c >> > @@ -286,6 +286,56 @@ found: >> > >> > static DEVICE_ATTR(edge, 0644, gpio_edge_show, gpio_edge_store); >> > >> > +static ssize_t gpio_wakeup_show(struct device *dev, >> > + struct device_attribute *attr, char *buf) >> > +{ >> > + ssize_t status; >> > + const struct gpio_desc *desc = dev_get_drvdata(dev); >> > + int irq = gpiod_to_irq(desc); >> > + struct irq_desc *irq_desc = irq_to_desc(irq); >> > + >> > + mutex_lock(&sysfs_lock); >> > + >> > + if (irqd_is_wakeup_set(&irq_desc->irq_data)) >> > + status = sprintf(buf, "enabled\n"); >> > + else >> > + status = sprintf(buf, "disabled\n"); >> > + >> > + mutex_unlock(&sysfs_lock); >> > + >> > + return status; >> > +} >> > + >> > +static ssize_t gpio_wakeup_store(struct device *dev, >> > + struct device_attribute *attr, const char *buf, size_t size) >> > +{ >> > + int ret; >> > + unsigned int on; >> > + struct gpio_desc *desc = dev_get_drvdata(dev); >> > + int irq = gpiod_to_irq(desc); >> > + >> > + mutex_lock(&sysfs_lock); >> > + >> > + if (sysfs_streq("enabled", buf)) >> > + on = true; >> > + else if (sysfs_streq("disabled", buf)) >> > + on = false; >> > + else >> > + return -EINVAL; >> >> You forgot to release sysfs_lock before returning here. > > Right, I will fix this and send a v2. Good catch. > >> >> > + >> > + ret = irq_set_irq_wake(irq, on); >> >> Just wondering: is it always safe to set the wake property of an IRQ >> even if the direction of its associated GPIO is output? Does it make >> sense at all to have the "wakeup" attribute file visible if the GPIO >> is an output one? >> >> > + >> > + mutex_unlock(&sysfs_lock); >> > + >> > + if (ret) >> > + pr_warn("%s: failed to %s wake\n", __func__, >> > + on ? "enable" : "disable"); >> > + >> > + return size; >> > +} >> > + >> > +static DEVICE_ATTR(wakeup, 0644, gpio_wakeup_show, gpio_wakeup_store); >> > + >> > static int sysfs_set_active_low(struct gpio_desc *desc, struct device *dev, >> > int value) >> > { >> > @@ -526,7 +576,7 @@ static struct class gpio_class = { >> > int gpiod_export(struct gpio_desc *desc, bool direction_may_change) >> > { >> > unsigned long flags; >> > - int status; >> > + int status, irq; >> > const char *ioname = NULL; >> > struct device *dev; >> > int offset; >> > @@ -582,11 +632,24 @@ int gpiod_export(struct gpio_desc *desc, bool direction_may_change) >> > goto fail_unregister_device; >> > } >> > >> > - if (gpiod_to_irq(desc) >= 0 && (direction_may_change || >> > - !test_bit(FLAG_IS_OUT, &desc->flags))) { >> > - status = device_create_file(dev, &dev_attr_edge); >> > - if (status) >> > - goto fail_unregister_device; >> > + irq = gpiod_to_irq(desc); >> > + if (irq >= 0) { >> > + struct irq_desc *irq_desc = irq_to_desc(irq); >> > + struct irq_chip *irqchip = irq_desc_get_chip(irq_desc); >> > + >> > + if (direction_may_change || >> > + !test_bit(FLAG_IS_OUT, &desc->flags)) { >> > + status = device_create_file(dev, &dev_attr_edge); >> > + if (status) >> > + goto fail_unregister_device; >> > + } >> > + >> > + if (irqchip->flags & IRQCHIP_SKIP_SET_WAKE || >> > + irqchip->irq_set_wake) { >> > + status = device_create_file(dev, &dev_attr_wakeup); >> > + if (status) >> > + goto fail_unregister_device; >> > + } >> >> Ok, I guess that answers my question. The edge property is always >> visible so it probably doesn't hurt if the wakeup property also is. > > Right, it is probably not optimal, but serves the purpose. Users still > might have to turn on their brain. But in general, setting those > properties should be safe, IMHO. > I think being able to mess with GPIO's direction and value is a much > bigger risk since they might be used for something more critical than > just LEDs :) > So, this does not really add a lot of more potential to break things :) Good point. > >> >> The only thing that bothers me a little bit is that both "edge" and >> "wakeup" are not very explicit names, but I guess it can't be helped. > > I guess edge is ABI and that ship has sailed. For 'wakeup' we can still > discuss the name. But wakeup is the standard property that is exposed in > sysfs for other devices that support wake and choose to expose > user-space control for it. So, I thought 'wakeup' would be appropriate > here as well. ... and another good point. Let's see what Linus thinks about this, but as far as I'm concerned your v2 looks ok. Cheers, Alex. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@...r.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists