| Message ID | 20200725041955.9985-6-warthog618@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | gpio: cdev: add uAPI V2 | expand |
On Sat, Jul 25, 2020 at 7:24 AM Kent Gibson <warthog618@gmail.com> wrote: > > Add support for requesting lines using the GPIO_GET_LINE_IOCTL, and > returning their current values using GPIOLINE_GET_VALUES_IOCTL. ... > +struct line { > + struct gpio_device *gdev; > + const char *label; > + u32 num_descs; > + /* descs must be last so it can be dynamically sized */ I guess [] implies above comment and thus comment can be dropped. > + struct gpio_desc *descs[]; > +}; ... > +static bool padding_not_zeroed(__u32 *padding, int pad_size) > +{ > + int i, sum = 0; > + > + for (i = 0; i < pad_size; i++) > + sum |= padding[i]; > + > + return sum; > +} Reimplementation of memchr_inv() ? ... > +static u64 gpioline_config_flags(struct gpioline_config *lc, int line_idx) > +{ > + int i; > + > + for (i = lc->num_attrs - 1; i >= 0; i--) { Much better to read is unsigned int i = lc->num_attrs; while (i--) { ... } > + if ((lc->attrs[i].attr.id == GPIOLINE_ATTR_ID_FLAGS) && > + test_bit(line_idx, (unsigned long *)lc->attrs[i].mask)) This casting is not good. What about BE 32-bit architecture? > + return lc->attrs[i].attr.flags; > + } > + return lc->flags; > +} > + > +static int gpioline_config_output_value(struct gpioline_config *lc, > + int line_idx) > +{ Same comments as per above. > +} ... > +static long line_get_values(struct line *line, void __user *ip) > +{ > + struct gpioline_values lv; > + unsigned long *vals = (unsigned long *)lv.bits; Casting u64 to unsigned long is not good. > +} ... > +static void line_free(struct line *line) > +{ > + int i; > + > + for (i = 0; i < line->num_descs; i++) { > + if (line->descs[i]) Redundant? > + gpiod_free(line->descs[i]); > + } > + kfree(line->label); > + put_device(&line->gdev->dev); > + kfree(line); > +} ... > + /* Make sure this is terminated */ > + linereq.consumer[sizeof(linereq.consumer)-1] = '\0'; > + if (strlen(linereq.consumer)) { > + line->label = kstrdup(linereq.consumer, GFP_KERNEL); kstrndup() ? > + if (!line->label) { > + ret = -ENOMEM; > + goto out_free_line; > + } > + } ... > + struct gpio_desc *desc = gpiochip_get_desc(gdev->chip, offset); I prefer to see this split, but it's minor. > + if (IS_ERR(desc)) { > + ret = PTR_ERR(desc); > + goto out_free_line; > + } ... > + dev_dbg(&gdev->dev, "registered chardev handle for line %d\n", > + offset); Perhaps tracepoint / event?
On Sat, Jul 25, 2020 at 11:51:54PM +0300, Andy Shevchenko wrote: > On Sat, Jul 25, 2020 at 7:24 AM Kent Gibson <warthog618@gmail.com> wrote: > > > > Add support for requesting lines using the GPIO_GET_LINE_IOCTL, and > > returning their current values using GPIOLINE_GET_VALUES_IOCTL. > > ... > > > +struct line { > > + struct gpio_device *gdev; > > + const char *label; > > + u32 num_descs; > > > + /* descs must be last so it can be dynamically sized */ > > I guess [] implies above comment and thus comment can be dropped. > > > + struct gpio_desc *descs[]; > > +}; > > ... > > > +static bool padding_not_zeroed(__u32 *padding, int pad_size) > > +{ > > + int i, sum = 0; > > + > > + for (i = 0; i < pad_size; i++) > > + sum |= padding[i]; > > + > > + return sum; > > +} > > Reimplementation of memchr_inv() ? > I was hoping to find an existing function, surely checking a region is zeroed is a common thing, right?, so this was a place holder as much as anything. Not sure memchr_inv fits the bill, but I'll give it a try... > ... > > > +static u64 gpioline_config_flags(struct gpioline_config *lc, int line_idx) > > +{ > > + int i; > > + > > + for (i = lc->num_attrs - 1; i >= 0; i--) { > > Much better to read is > > unsigned int i = lc->num_attrs; > > while (i--) { > ... > } > Really? I find that the post-decrement in the while makes determining the bounds of the loop more confusing. > > + if ((lc->attrs[i].attr.id == GPIOLINE_ATTR_ID_FLAGS) && > > > + test_bit(line_idx, (unsigned long *)lc->attrs[i].mask)) > > This casting is not good. What about BE 32-bit architecture? > I agree the casting is hideous, but I thought the outcome was correct as it is manipulating addresses, not data. You think the address of a 64-bit differs based on endian?? Happy to change it - but not sure what to. > > + return lc->attrs[i].attr.flags; > > + } > > + return lc->flags; > > +} > > + > > +static int gpioline_config_output_value(struct gpioline_config *lc, > > + int line_idx) > > +{ > > Same comments as per above. > > > +} > > ... > > > +static long line_get_values(struct line *line, void __user *ip) > > +{ > > + struct gpioline_values lv; > > > + unsigned long *vals = (unsigned long *)lv.bits; > > Casting u64 to unsigned long is not good. > Same comments as per above. > > +} > > ... > > > +static void line_free(struct line *line) > > +{ > > + int i; > > + > > + for (i = 0; i < line->num_descs; i++) { > > > + if (line->descs[i]) > > Redundant? > Actually, no. The line_free is also used to clean up construction failures, so the line may be partially constructed. num_descs is set first, but the descs themselves may have failed to allocate. And gpiod_free throws a warning if you pass a NULL, hence the extra check here. > > + gpiod_free(line->descs[i]); > > + } > > + kfree(line->label); > > + put_device(&line->gdev->dev); > > + kfree(line); > > +} > > ... > > > + /* Make sure this is terminated */ > > + linereq.consumer[sizeof(linereq.consumer)-1] = '\0'; > > + if (strlen(linereq.consumer)) { > > + line->label = kstrdup(linereq.consumer, GFP_KERNEL); > > kstrndup() ? > That was a cut-and-paste from V1... > > + if (!line->label) { > > + ret = -ENOMEM; > > + goto out_free_line; > > + } > > + } > ... and changing it would result in this logic behaving differently. You couldn't distinguish between consumer not being set, and so label not being set, and kstrndup returning NULL due to no mem. > ... > > > + struct gpio_desc *desc = gpiochip_get_desc(gdev->chip, offset); > > I prefer to see this split, but it's minor. > > > + if (IS_ERR(desc)) { > > + ret = PTR_ERR(desc); > > + goto out_free_line; > > + } > > ... > > > + dev_dbg(&gdev->dev, "registered chardev handle for line %d\n", > > + offset); > > Perhaps tracepoint / event? > Again, a cut-and-paste from V1, and I have no experience with tracepoints or events, so I have no opinion on that. So, yeah - perhaps? Cheers, Kent.
On Sun, Jul 26, 2020 at 09:12:44AM +0800, Kent Gibson wrote: > On Sat, Jul 25, 2020 at 11:51:54PM +0300, Andy Shevchenko wrote: > > On Sat, Jul 25, 2020 at 7:24 AM Kent Gibson <warthog618@gmail.com> wrote: > > > [ snip ] > > > > > +static bool padding_not_zeroed(__u32 *padding, int pad_size) > > > +{ > > > + int i, sum = 0; > > > + > > > + for (i = 0; i < pad_size; i++) > > > + sum |= padding[i]; > > > + > > > + return sum; > > > +} > > > > Reimplementation of memchr_inv() ? > > > > I was hoping to find an existing function, surely checking a region is > zeroed is a common thing, right?, so this was a place holder as much > as anything. Not sure memchr_inv fits the bill, but I'll give it a > try... > I gave it a try. It is a good fit functionally, but in my build it results in a larger object by ~104 bytes. I assume that is because padding_not_zeroed is being inlined, and otherwise optimized, while memchr_inv calls aren't. As such I'm inclined to leave it as is - unless there are other objections. Cheers, Kent.
On Sun, Jul 26, 2020 at 09:12:44AM +0800, Kent Gibson wrote: > On Sat, Jul 25, 2020 at 11:51:54PM +0300, Andy Shevchenko wrote: > > On Sat, Jul 25, 2020 at 7:24 AM Kent Gibson <warthog618@gmail.com> wrote: > > > [ snip ] > > > + test_bit(line_idx, (unsigned long *)lc->attrs[i].mask)) > > > > This casting is not good. What about BE 32-bit architecture? > > > > I agree the casting is hideous, but I thought the outcome was correct > as it is manipulating addresses, not data. > You think the address of a 64-bit differs based on endian?? > Happy to change it - but not sure what to. > You are right - using bitops on u64 is problematic for BE-32 - the 32-bit words will be swapped if userspace treats the flags as the u64 it is defined as. I'll rework that for v3. Cheers, Kent.
On Wed, Jul 29, 2020 at 5:28 AM Kent Gibson <warthog618@gmail.com> wrote: > On Sun, Jul 26, 2020 at 09:12:44AM +0800, Kent Gibson wrote: ... > I'll rework that for v3. Please give some more time to review v2. Especially the v2 API approach.
On Wed, Jul 29, 2020 at 11:05:48AM +0300, Andy Shevchenko wrote: > On Wed, Jul 29, 2020 at 5:28 AM Kent Gibson <warthog618@gmail.com> wrote: > > On Sun, Jul 26, 2020 at 09:12:44AM +0800, Kent Gibson wrote: > > ... > > > I'll rework that for v3. > > Please give some more time to review v2. Especially the v2 API approach. > For sure. I'll be spending some time setting up and testing on a BE 32 target so I wont be ready to submit a v3 for a few days anyway. Cheers, Kent.
On Sun, Jul 26, 2020 at 3:12 AM Kent Gibson <warthog618@gmail.com> wrote: > [snip] > > > > > +static bool padding_not_zeroed(__u32 *padding, int pad_size) > > > +{ > > > + int i, sum = 0; > > > + > > > + for (i = 0; i < pad_size; i++) > > > + sum |= padding[i]; > > > + > > > + return sum; > > > +} > > > > Reimplementation of memchr_inv() ? > > > > I was hoping to find an existing function, surely checking a region is > zeroed is a common thing, right?, so this was a place holder as much > as anything. Not sure memchr_inv fits the bill, but I'll give it a > try... > If you don't find an appropriate function: please put your new implementation in lib/ so that others may reuse it. > > ... > > > > > +static u64 gpioline_config_flags(struct gpioline_config *lc, int line_idx) > > > +{ > > > + int i; > > > + > > > + for (i = lc->num_attrs - 1; i >= 0; i--) { > > > > Much better to read is > > > > unsigned int i = lc->num_attrs; > > > > while (i--) { > > ... > > } > > > > Really? I find that the post-decrement in the while makes determining the > bounds of the loop more confusing. > Agreed, Andy: this is too much nit-picking. :) [snip] > > ... > > > > > + struct gpio_desc *desc = gpiochip_get_desc(gdev->chip, offset); > > > > I prefer to see this split, but it's minor. > > > > > + if (IS_ERR(desc)) { > > > + ret = PTR_ERR(desc); > > > + goto out_free_line; > > > + } > > > > ... > > > > > + dev_dbg(&gdev->dev, "registered chardev handle for line %d\n", > > > + offset); > > > > Perhaps tracepoint / event? > > > > Again, a cut-and-paste from V1, and I have no experience with > tracepoints or events, so I have no opinion on that. > > So, yeah - perhaps? > I think it's a good idea to add some proper instrumentation this time other than much less reliable logs. Can you take a look at include/trace/events/gpio.h? Adding new GPIO trace events should be pretty straightforward by copy-pasti... drawing inspiration from existing ones. Bart
On Fri, Jul 31, 2020 at 06:05:10PM +0200, Bartosz Golaszewski wrote: > On Sun, Jul 26, 2020 at 3:12 AM Kent Gibson <warthog618@gmail.com> wrote: > > > > [snip] > > > > > > > > +static bool padding_not_zeroed(__u32 *padding, int pad_size) > > > > +{ > > > > + int i, sum = 0; > > > > + > > > > + for (i = 0; i < pad_size; i++) > > > > + sum |= padding[i]; > > > > + > > > > + return sum; > > > > +} > > > > > > Reimplementation of memchr_inv() ? > > > > > > > I was hoping to find an existing function, surely checking a region is > > zeroed is a common thing, right?, so this was a place holder as much > > as anything. Not sure memchr_inv fits the bill, but I'll give it a > > try... > > > > If you don't find an appropriate function: please put your new > implementation in lib/ so that others may reuse it. > Changed to memchr_inv. > > > ... > > > > > > > +static u64 gpioline_config_flags(struct gpioline_config *lc, int line_idx) > > > > +{ > > > > + int i; > > > > + > > > > + for (i = lc->num_attrs - 1; i >= 0; i--) { > > > > > > Much better to read is > > > > > > unsigned int i = lc->num_attrs; > > > > > > while (i--) { > > > ... > > > } > > > > > > > Really? I find that the post-decrement in the while makes determining the > > bounds of the loop more confusing. > > > > Agreed, Andy: this is too much nit-picking. :) > I was actually hoping for some feedback on the direction of that loop, as it relates to the handling of multiple instances of the same attribute associated with a given line. The reverse loop here implements a last in wins policy, but I'm now thinking the kernel should be encouraging userspace to only associate a given attribute with a line once, and that a first in wins would help do that - as additional associations would be ignored. Alternatively, the kernel should enforce that an attribute can only be associated once, but that would require adding more request validation. > [snip] > > > > ... > > > > > > > + struct gpio_desc *desc = gpiochip_get_desc(gdev->chip, offset); > > > > > > I prefer to see this split, but it's minor. > > > > > > > + if (IS_ERR(desc)) { > > > > + ret = PTR_ERR(desc); > > > > + goto out_free_line; > > > > + } > > > > > > ... > > > > > > > + dev_dbg(&gdev->dev, "registered chardev handle for line %d\n", > > > > + offset); > > > > > > Perhaps tracepoint / event? > > > > > > > Again, a cut-and-paste from V1, and I have no experience with > > tracepoints or events, so I have no opinion on that. > > > > So, yeah - perhaps? > > > > I think it's a good idea to add some proper instrumentation this time > other than much less reliable logs. Can you take a look at > include/trace/events/gpio.h? Adding new GPIO trace events should be > pretty straightforward by copy-pasti... drawing inspiration from > existing ones. > You only want tracepoints to replace those dev_dbg()s, so when a line is requested? What about the release? Any other points? Cheers, Kent.
On Sun, Aug 02, 2020 at 11:31:58AM +0800, Kent Gibson wrote: > On Fri, Jul 31, 2020 at 06:05:10PM +0200, Bartosz Golaszewski wrote: > > On Sun, Jul 26, 2020 at 3:12 AM Kent Gibson <warthog618@gmail.com> wrote: > > > > > > > > > > > > > > + dev_dbg(&gdev->dev, "registered chardev handle for line %d\n", > > > > > + offset); > > > > > > > > Perhaps tracepoint / event? > > > > > > > > > > Again, a cut-and-paste from V1, and I have no experience with > > > tracepoints or events, so I have no opinion on that. > > > > > > So, yeah - perhaps? > > > > > > > I think it's a good idea to add some proper instrumentation this time > > other than much less reliable logs. Can you take a look at > > include/trace/events/gpio.h? Adding new GPIO trace events should be > > pretty straightforward by copy-pasti... drawing inspiration from > > existing ones. > > > > You only want tracepoints to replace those dev_dbg()s, so when a line > is requested? What about the release? Any other points? > Had a closer look and it seems to me that the correct place to add such tracepoints would be gpiod_request() and gpiod_free(), so they catch all requests, not just the cdev ones. And that moves it outside the scope of this patch. I personally don't have any use for the dev_dbg()s here and am happy to remove them - they were only there to match the behaviour of linehandle_create as closely as possible. Cheers, Kent.
On Sun, Aug 2, 2020 at 11:32 AM Kent Gibson <warthog618@gmail.com> wrote: > > On Sun, Aug 02, 2020 at 11:31:58AM +0800, Kent Gibson wrote: > > On Fri, Jul 31, 2020 at 06:05:10PM +0200, Bartosz Golaszewski wrote: > > > On Sun, Jul 26, 2020 at 3:12 AM Kent Gibson <warthog618@gmail.com> wrote: > > > > > > > > > > > > > > > > > > + dev_dbg(&gdev->dev, "registered chardev handle for line %d\n", > > > > > > + offset); > > > > > > > > > > Perhaps tracepoint / event? > > > > > > > > > > > > > Again, a cut-and-paste from V1, and I have no experience with > > > > tracepoints or events, so I have no opinion on that. > > > > > > > > So, yeah - perhaps? > > > > > > > > > > I think it's a good idea to add some proper instrumentation this time > > > other than much less reliable logs. Can you take a look at > > > include/trace/events/gpio.h? Adding new GPIO trace events should be > > > pretty straightforward by copy-pasti... drawing inspiration from > > > existing ones. > > > > > > > You only want tracepoints to replace those dev_dbg()s, so when a line > > is requested? What about the release? Any other points? > > > > Had a closer look and it seems to me that the correct place to add such > tracepoints would be gpiod_request() and gpiod_free(), so they catch all > requests, not just the cdev ones. And that moves it outside the scope > of this patch. > > I personally don't have any use for the dev_dbg()s here and am happy to > remove them - they were only there to match the behaviour of > linehandle_create as closely as possible. > Sounds good, we can work on trace points separately. Bartosz
On Sun, Aug 2, 2020 at 5:32 AM Kent Gibson <warthog618@gmail.com> wrote: > > On Fri, Jul 31, 2020 at 06:05:10PM +0200, Bartosz Golaszewski wrote: > > On Sun, Jul 26, 2020 at 3:12 AM Kent Gibson <warthog618@gmail.com> wrote: [snip!] > > > > > > > > > +static u64 gpioline_config_flags(struct gpioline_config *lc, int line_idx) > > > > > +{ > > > > > + int i; > > > > > + > > > > > + for (i = lc->num_attrs - 1; i >= 0; i--) { > > > > > > > > Much better to read is > > > > > > > > unsigned int i = lc->num_attrs; > > > > > > > > while (i--) { > > > > ... > > > > } > > > > > > > > > > Really? I find that the post-decrement in the while makes determining the > > > bounds of the loop more confusing. > > > > > > > Agreed, Andy: this is too much nit-picking. :) > > > > I was actually hoping for some feedback on the direction of that loop, > as it relates to the handling of multiple instances of the same > attribute associated with a given line. > > The reverse loop here implements a last in wins policy, but I'm now > thinking the kernel should be encouraging userspace to only associate a > given attribute with a line once, and that a first in wins would help do > that - as additional associations would be ignored. > > Alternatively, the kernel should enforce that an attribute can only be > associated once, but that would require adding more request validation. > I guess this would result in a lot of churn to do validation which is largely unnecessary? To me the first in wins sounds more consistent. Also: I just started going through the patches - nice idea with the GPIO attributes, I really like it. Although I need to give it a longer thought tomorrow - I'm wondering if we can maybe unify them and the flags. [snip] Bartosz
On Mon, Aug 03, 2020 at 10:02:50PM +0200, Bartosz Golaszewski wrote: > On Sun, Aug 2, 2020 at 5:32 AM Kent Gibson <warthog618@gmail.com> wrote: > > > > On Fri, Jul 31, 2020 at 06:05:10PM +0200, Bartosz Golaszewski wrote: > > > On Sun, Jul 26, 2020 at 3:12 AM Kent Gibson <warthog618@gmail.com> wrote: > > [snip!] > > > > > > > > > > > > +static u64 gpioline_config_flags(struct gpioline_config *lc, int line_idx) > > > > > > +{ > > > > > > + int i; > > > > > > + > > > > > > + for (i = lc->num_attrs - 1; i >= 0; i--) { > > > > > > > > > > Much better to read is > > > > > > > > > > unsigned int i = lc->num_attrs; > > > > > > > > > > while (i--) { > > > > > ... > > > > > } > > > > > > > > > > > > > Really? I find that the post-decrement in the while makes determining the > > > > bounds of the loop more confusing. > > > > > > > > > > Agreed, Andy: this is too much nit-picking. :) > > > > > > > I was actually hoping for some feedback on the direction of that loop, > > as it relates to the handling of multiple instances of the same > > attribute associated with a given line. > > > > The reverse loop here implements a last in wins policy, but I'm now > > thinking the kernel should be encouraging userspace to only associate a > > given attribute with a line once, and that a first in wins would help do > > that - as additional associations would be ignored. > > > > Alternatively, the kernel should enforce that an attribute can only be > > associated once, but that would require adding more request validation. > > > > I guess this would result in a lot of churn to do validation which is > largely unnecessary? To me the first in wins sounds more consistent. > Fully validating the attrs would involve a lot of tedious looping, which would be pointless 99.99% of the time, so I was hoping to avoid it. OTOH we're interacting with hardware so I don't want to be doing anything that userspace hasn't explicitly requested. But I would be satisfied with clearly documenting the behaviour - and in most cases libgpiod will be taking care of it anyway... > Also: I just started going through the patches - nice idea with the > GPIO attributes, I really like it. Although I need to give it a longer > thought tomorrow - I'm wondering if we can maybe unify them and the > flags. > I had an earlier draft that did just that - and that is partially why the loop is last in wins - I was using slot 0 as the default flags. But the default flags cover a lot of use cases, including all of v1, and it was simple and cheap to provide a default - and it simplified the initial port of libgpiod to v2... Cheers, Kent.
On Tue, Aug 4, 2020 at 1:01 AM Kent Gibson <warthog618@gmail.com> wrote: > [snip] > > > Also: I just started going through the patches - nice idea with the > > GPIO attributes, I really like it. Although I need to give it a longer > > thought tomorrow - I'm wondering if we can maybe unify them and the > > flags. > > > > I had an earlier draft that did just that - and that is partially why > the loop is last in wins - I was using slot 0 as the default flags. > But the default flags cover a lot of use cases, including all of v1, and > it was simple and cheap to provide a default - and it simplified the > initial port of libgpiod to v2... > If porting libgpiod to v2 is the only concern then I wouldn't stress about it. At the same time I'm wondering - is there any use-case where we wouldn't need the flags attribute for at least some lines? Because if it's always required than maybe having a default isn't that bad. Bartosz
On Tue, Aug 04, 2020 at 07:47:43PM +0200, Bartosz Golaszewski wrote: > On Tue, Aug 4, 2020 at 1:01 AM Kent Gibson <warthog618@gmail.com> wrote: > > > > [snip] > > > > > > Also: I just started going through the patches - nice idea with the > > > GPIO attributes, I really like it. Although I need to give it a longer > > > thought tomorrow - I'm wondering if we can maybe unify them and the > > > flags. > > > > > > > I had an earlier draft that did just that - and that is partially why > > the loop is last in wins - I was using slot 0 as the default flags. > > But the default flags cover a lot of use cases, including all of v1, and > > it was simple and cheap to provide a default - and it simplified the > > initial port of libgpiod to v2... > > > > If porting libgpiod to v2 is the only concern then I wouldn't stress > about it. At the same time I'm wondering - is there any use-case where > we wouldn't need the flags attribute for at least some lines? Because > if it's always required than maybe having a default isn't that bad. > The only case where flags are not required is an AS-IS request. I have no idea what that use case is useful for, but it is in v1 and therefore supported by v2 for backward compatibility. So there is almost always a flags attribute, and I didn't want to waste an attribute slot on it. Supporting the default in the kernel is trivial - it is literally just the default return in gpioline_config_flags: + } + return lc->flags; +} which would otherwise be 0. Cheers, Kent.
diff --git a/drivers/gpio/gpiolib-cdev.c b/drivers/gpio/gpiolib-cdev.c index e6c9b78adfc2..0908ae117b7d 100644 --- a/drivers/gpio/gpiolib-cdev.c +++ b/drivers/gpio/gpiolib-cdev.c @@ -1,7 +1,9 @@ // SPDX-License-Identifier: GPL-2.0 #include <linux/anon_inodes.h> +#include <linux/atomic.h> #include <linux/bitmap.h> +#include <linux/build_bug.h> #include <linux/cdev.h> #include <linux/compat.h> #include <linux/device.h> @@ -34,6 +36,7 @@ * GPIO line handle management */ +#ifdef CONFIG_GPIO_CDEV_V1 /** * struct linehandle_state - contains the state of a userspace handle * @gdev: the GPIO device the handle pertains to @@ -376,6 +379,366 @@ static int linehandle_create(struct gpio_device *gdev, void __user *ip) linehandle_free(lh); return ret; } +#endif /* CONFIG_GPIO_CDEV_V1 */ + +/** + * struct line - contains the state of a userspace line request + * @gdev: the GPIO device the line request pertains to + * @label: consumer label used to tag descriptors + * @num_descs: the number of descriptors held in the descs array + * @descs: the GPIO descriptors held by this line request, with @num_descs + * elements. + */ +struct line { + struct gpio_device *gdev; + const char *label; + u32 num_descs; + /* descs must be last so it can be dynamically sized */ + struct gpio_desc *descs[]; +}; + +static bool padding_not_zeroed(__u32 *padding, int pad_size) +{ + int i, sum = 0; + + for (i = 0; i < pad_size; i++) + sum |= padding[i]; + + return sum; +} + +#define GPIOLINE_BIAS_FLAGS \ + (GPIOLINE_FLAG_V2_BIAS_PULL_UP | \ + GPIOLINE_FLAG_V2_BIAS_PULL_DOWN | \ + GPIOLINE_FLAG_V2_BIAS_DISABLED) + +#define GPIOLINE_DIRECTION_FLAGS \ + (GPIOLINE_FLAG_V2_INPUT | \ + GPIOLINE_FLAG_V2_OUTPUT) + +#define GPIOLINE_DRIVE_FLAGS \ + (GPIOLINE_FLAG_V2_OPEN_DRAIN | \ + GPIOLINE_FLAG_V2_OPEN_SOURCE) + +#define GPIOLINE_VALID_FLAGS \ + (GPIOLINE_FLAG_V2_ACTIVE_LOW | \ + GPIOLINE_DIRECTION_FLAGS | \ + GPIOLINE_DRIVE_FLAGS | \ + GPIOLINE_BIAS_FLAGS) + +static u64 gpioline_config_flags(struct gpioline_config *lc, int line_idx) +{ + int i; + + for (i = lc->num_attrs - 1; i >= 0; i--) { + if ((lc->attrs[i].attr.id == GPIOLINE_ATTR_ID_FLAGS) && + test_bit(line_idx, (unsigned long *)lc->attrs[i].mask)) + return lc->attrs[i].attr.flags; + } + return lc->flags; +} + +static int gpioline_config_output_value(struct gpioline_config *lc, + int line_idx) +{ + int i; + + for (i = lc->num_attrs - 1; i >= 0; i--) { + if ((lc->attrs[i].attr.id == GPIOLINE_ATTR_ID_OUTPUT_VALUES) && + test_bit(line_idx, (unsigned long *)lc->attrs[i].mask)) + return test_bit(line_idx, + (unsigned long *)lc->attrs[i].attr.values.bits); + } + return 0; +} + +static int gpioline_flags_validate(u64 flags) +{ + /* Return an error if an unknown flag is set */ + if (flags & ~GPIOLINE_VALID_FLAGS) + return -EINVAL; + + /* + * Do not allow both INPUT & OUTPUT flags to be set as they are + * contradictory. + */ + if ((flags & GPIOLINE_FLAG_V2_INPUT) && + (flags & GPIOLINE_FLAG_V2_OUTPUT)) + return -EINVAL; + + /* + * Do not allow OPEN_SOURCE & OPEN_DRAIN flags in a single request. If + * the hardware actually supports enabling both at the same time the + * electrical result would be disastrous. + */ + if ((flags & GPIOLINE_FLAG_V2_OPEN_DRAIN) && + (flags & GPIOLINE_FLAG_V2_OPEN_SOURCE)) + return -EINVAL; + + /* Drive requires explicit output direction. */ + if ((flags & GPIOLINE_DRIVE_FLAGS) && + !(flags & GPIOLINE_FLAG_V2_OUTPUT)) + return -EINVAL; + + /* Bias requies explicit direction. */ + if ((flags & GPIOLINE_BIAS_FLAGS) && + !(flags & GPIOLINE_DIRECTION_FLAGS)) + return -EINVAL; + + /* Only one bias flag can be set. */ + if (((flags & GPIOLINE_FLAG_V2_BIAS_DISABLED) && + (flags & (GPIOLINE_FLAG_V2_BIAS_PULL_DOWN | + GPIOLINE_FLAG_V2_BIAS_PULL_UP))) || + ((flags & GPIOLINE_FLAG_V2_BIAS_PULL_DOWN) && + (flags & GPIOLINE_FLAG_V2_BIAS_PULL_UP))) + return -EINVAL; + + return 0; +} + +static int gpioline_config_validate(struct gpioline_config *lc, int num_lines) +{ + int i, ret; + u64 flags; + + if (lc->num_attrs > GPIOLINE_NUM_ATTRS_MAX) + return -EINVAL; + + if (padding_not_zeroed(lc->padding, ARRAY_SIZE(lc->padding))) + return -EINVAL; + + for (i = 0; i < num_lines; i++) { + flags = gpioline_config_flags(lc, i); + ret = gpioline_flags_validate(flags); + if (ret) + return ret; + } + return 0; +} + +static void gpioline_config_flags_to_desc_flags(u64 flags, + unsigned long *flagsp) +{ + assign_bit(FLAG_ACTIVE_LOW, flagsp, + flags & GPIOLINE_FLAG_V2_ACTIVE_LOW); + if (flags & GPIOLINE_FLAG_V2_OUTPUT) + set_bit(FLAG_IS_OUT, flagsp); + else if (flags & GPIOLINE_FLAG_V2_INPUT) + clear_bit(FLAG_IS_OUT, flagsp); + assign_bit(FLAG_OPEN_DRAIN, flagsp, + flags & GPIOLINE_FLAG_V2_OPEN_DRAIN); + assign_bit(FLAG_OPEN_SOURCE, flagsp, + flags & GPIOLINE_FLAG_V2_OPEN_SOURCE); + assign_bit(FLAG_PULL_UP, flagsp, + flags & GPIOLINE_FLAG_V2_BIAS_PULL_UP); + assign_bit(FLAG_PULL_DOWN, flagsp, + flags & GPIOLINE_FLAG_V2_BIAS_PULL_DOWN); + assign_bit(FLAG_BIAS_DISABLE, flagsp, + flags & GPIOLINE_FLAG_V2_BIAS_DISABLED); +} + +static long line_get_values(struct line *line, void __user *ip) +{ + struct gpioline_values lv; + unsigned long *vals = (unsigned long *)lv.bits; + int ret; + + /* NOTE: It's ok to read values of output lines. */ + memset(&lv, 0, sizeof(lv)); + ret = gpiod_get_array_value_complex(false, + true, + line->num_descs, + line->descs, + NULL, + vals); + if (ret) + return ret; + + if (copy_to_user(ip, &lv, sizeof(lv))) + return -EFAULT; + + return 0; +} + +static long line_ioctl(struct file *file, unsigned int cmd, + unsigned long arg) +{ + struct line *line = file->private_data; + void __user *ip = (void __user *)arg; + + if (cmd == GPIOLINE_GET_VALUES_IOCTL) + return line_get_values(line, ip); + + return -EINVAL; +} + +#ifdef CONFIG_COMPAT +static long line_ioctl_compat(struct file *file, unsigned int cmd, + unsigned long arg) +{ + return line_ioctl(file, cmd, (unsigned long)compat_ptr(arg)); +} +#endif + +static void line_free(struct line *line) +{ + int i; + + for (i = 0; i < line->num_descs; i++) { + if (line->descs[i]) + gpiod_free(line->descs[i]); + } + kfree(line->label); + put_device(&line->gdev->dev); + kfree(line); +} + +static int line_release(struct inode *inode, struct file *file) +{ + struct line *line = file->private_data; + + line_free(line); + return 0; +} + +static const struct file_operations line_fileops = { + .release = line_release, + .owner = THIS_MODULE, + .llseek = noop_llseek, + .unlocked_ioctl = line_ioctl, +#ifdef CONFIG_COMPAT + .compat_ioctl = line_ioctl_compat, +#endif +}; + +static int line_create(struct gpio_device *gdev, void __user *ip) +{ + struct gpioline_request linereq; + struct line *line; + struct file *file; + int fd, i, ret; + struct gpioline_config *lc; + unsigned long flags; + + if (copy_from_user(&linereq, ip, sizeof(linereq))) + return -EFAULT; + if ((linereq.num_lines == 0) || (linereq.num_lines > GPIOLINES_MAX)) + return -EINVAL; + + if (padding_not_zeroed(linereq.padding, ARRAY_SIZE(linereq.padding))) + return -EINVAL; + + lc = &linereq.config; + ret = gpioline_config_validate(lc, linereq.num_lines); + if (ret) + return ret; + + line = kzalloc(struct_size(line, descs, linereq.num_lines), + GFP_KERNEL); + if (!line) + return -ENOMEM; + + line->gdev = gdev; + get_device(&gdev->dev); + + /* Make sure this is terminated */ + linereq.consumer[sizeof(linereq.consumer)-1] = '\0'; + if (strlen(linereq.consumer)) { + line->label = kstrdup(linereq.consumer, GFP_KERNEL); + if (!line->label) { + ret = -ENOMEM; + goto out_free_line; + } + } + + line->num_descs = linereq.num_lines; + + /* Request each GPIO */ + for (i = 0; i < linereq.num_lines; i++) { + u32 offset = linereq.offsets[i]; + struct gpio_desc *desc = gpiochip_get_desc(gdev->chip, offset); + + if (IS_ERR(desc)) { + ret = PTR_ERR(desc); + goto out_free_line; + } + + ret = gpiod_request(desc, line->label); + if (ret) + goto out_free_line; + + line->descs[i] = desc; + flags = gpioline_config_flags(lc, i); + gpioline_config_flags_to_desc_flags(flags, &desc->flags); + + ret = gpiod_set_transitory(desc, false); + if (ret < 0) + goto out_free_line; + + /* + * Lines have to be requested explicitly for input + * or output, else the line will be treated "as is". + */ + if (flags & GPIOLINE_FLAG_V2_OUTPUT) { + int val = gpioline_config_output_value(lc, i); + + ret = gpiod_direction_output(desc, val); + if (ret) + goto out_free_line; + } else if (flags & GPIOLINE_FLAG_V2_INPUT) { + ret = gpiod_direction_input(desc); + if (ret) + goto out_free_line; + } + + blocking_notifier_call_chain(&desc->gdev->notifier, + GPIOLINE_CHANGED_REQUESTED, desc); + + dev_dbg(&gdev->dev, "registered chardev handle for line %d\n", + offset); + } + + fd = get_unused_fd_flags(O_RDONLY | O_CLOEXEC); + if (fd < 0) { + ret = fd; + goto out_free_line; + } + + file = anon_inode_getfile("gpio-line", + &line_fileops, + line, + O_RDONLY | O_CLOEXEC); + if (IS_ERR(file)) { + ret = PTR_ERR(file); + goto out_put_unused_fd; + } + + linereq.fd = fd; + if (copy_to_user(ip, &linereq, sizeof(linereq))) { + /* + * fput() will trigger the release() callback, so do not go onto + * the regular error cleanup path here. + */ + fput(file); + put_unused_fd(fd); + return -EFAULT; + } + + fd_install(fd, file); + + dev_dbg(&gdev->dev, "registered chardev handle for %d lines\n", + line->num_descs); + + return 0; + +out_put_unused_fd: + put_unused_fd(fd); +out_free_line: + line_free(line); + return ret; +} + +#ifdef CONFIG_GPIO_CDEV_V1 /* * GPIO line event management @@ -745,6 +1108,8 @@ static int lineevent_create(struct gpio_device *gdev, void __user *ip) return ret; } +#endif /* CONFIG_GPIO_CDEV_V1 */ + static void gpio_desc_to_lineinfo(struct gpio_desc *desc, struct gpioline_info *info) { @@ -850,6 +1215,7 @@ static long gpio_ioctl(struct file *file, unsigned int cmd, unsigned long arg) if (copy_to_user(ip, &chipinfo, sizeof(chipinfo))) return -EFAULT; return 0; +#ifdef CONFIG_GPIO_CDEV_V1 } else if (cmd == GPIO_GET_LINEINFO_IOCTL) { struct gpioline_info lineinfo; @@ -892,6 +1258,9 @@ static long gpio_ioctl(struct file *file, unsigned int cmd, unsigned long arg) } return 0; +#endif /* CONFIG_GPIO_CDEV_V1 */ + } else if (cmd == GPIO_GET_LINE_IOCTL) { + return line_create(gdev, ip); } else if (cmd == GPIO_GET_LINEINFO_UNWATCH_IOCTL) { if (copy_from_user(&offset, ip, sizeof(offset))) return -EFAULT; @@ -1118,4 +1487,24 @@ int gpiolib_cdev_register(struct gpio_device *gdev, dev_t devt) void gpiolib_cdev_unregister(struct gpio_device *gdev) { cdev_device_del(&gdev->chrdev, &gdev->dev); + + /* + * array sizes must be a multiple of 8 to ensure 64-bit alignment and + * to not create holes in the struct packing. + */ + BUILD_BUG_ON(GPIOLINES_MAX % 8); + BUILD_BUG_ON(GPIO_MAX_NAME_SIZE % 8); + + /* + * check that uAPI structs are 64-bit aligned for 32/64-bit + * compatibility + */ + BUILD_BUG_ON(sizeof(struct gpioline_attribute) % 8); + BUILD_BUG_ON(sizeof(struct gpioline_config_attribute) % 8); + BUILD_BUG_ON(sizeof(struct gpioline_config) % 8); + BUILD_BUG_ON(sizeof(struct gpioline_request) % 8); + BUILD_BUG_ON(sizeof(struct gpioline_info_v2) % 8); + BUILD_BUG_ON(sizeof(struct gpioline_info_changed_v2) % 8); + BUILD_BUG_ON(sizeof(struct gpioline_event) % 8); + BUILD_BUG_ON(sizeof(struct gpioline_values) % 8); }
Add support for requesting lines using the GPIO_GET_LINE_IOCTL, and returning their current values using GPIOLINE_GET_VALUES_IOCTL. Signed-off-by: Kent Gibson <warthog618@gmail.com> --- The struct line implementation is based on the V1 struct linehandle implementation. The line_ioctl() is a simple wrapper around line_get_values() here, but will be extended with other ioctls in subsequent patches. drivers/gpio/gpiolib-cdev.c | 389 ++++++++++++++++++++++++++++++++++++ 1 file changed, 389 insertions(+)