| Message ID | 20230926052007.3917389-6-andriy.shevchenko@linux.intel.com |
|---|---|
| State | New |
| Headers | show |
| Series | bitmap: get rid of bitmap_remap() and bitmap_biremap() uses | expand |
On Wed, Sep 27, 2023 at 10:23:12PM +0800, Kent Gibson wrote: > On Wed, Sep 27, 2023 at 04:59:34PM +0300, Andy Shevchenko wrote: > > On Wed, Sep 27, 2023 at 09:49:35PM +0800, Kent Gibson wrote: > > > On Wed, Sep 27, 2023 at 03:17:06PM +0300, Andy Shevchenko wrote: > > > > On Wed, Sep 27, 2023 at 09:32:11AM +0800, Kent Gibson wrote: ... > > > > Yet, it opens a way to scale this in case we might have v3 ABI that let's say > > > > allows to work with 512 GPIOs at a time. With your code it will be much harder > > > > to achieve and see what you wrote about maintenance (in that case). > > > > > > v3 ABI?? libgpiod v2 is barely out the door! > > > Do you have any cases where 64 lines per request is limiting? > > > > IIRC it was SO question where the OP asks exactly about breaking the 64 lines > > limitation in the current ABI. > > > > > If that sort of speculation isn't premature optimisation then I don't know > > > what is. > > > > No, based on the real question / discussion, just have no link at hand. > > But it's quite a niche, I can agree. > > Let me know if you find a ref to that discussion - I'm curious. Here it is (read comments as well): https://stackoverflow.com/questions/76307370/control-gpio-from-linux-userspace-with-linux-gpio-h
On Mon, Oct 02, 2023 at 12:05:11PM +0300, Andy Shevchenko wrote: > On Wed, Sep 27, 2023 at 10:23:12PM +0800, Kent Gibson wrote: > > On Wed, Sep 27, 2023 at 04:59:34PM +0300, Andy Shevchenko wrote: > > > On Wed, Sep 27, 2023 at 09:49:35PM +0800, Kent Gibson wrote: > > > > On Wed, Sep 27, 2023 at 03:17:06PM +0300, Andy Shevchenko wrote: > > > > > On Wed, Sep 27, 2023 at 09:32:11AM +0800, Kent Gibson wrote: > > ... > > > > > > Yet, it opens a way to scale this in case we might have v3 ABI that let's say > > > > > allows to work with 512 GPIOs at a time. With your code it will be much harder > > > > > to achieve and see what you wrote about maintenance (in that case). > > > > > > > > v3 ABI?? libgpiod v2 is barely out the door! > > > > Do you have any cases where 64 lines per request is limiting? > > > > > > IIRC it was SO question where the OP asks exactly about breaking the 64 lines > > > limitation in the current ABI. > > > > > > > If that sort of speculation isn't premature optimisation then I don't know > > > > what is. > > > > > > No, based on the real question / discussion, just have no link at hand. > > > But it's quite a niche, I can agree. > > > > Let me know if you find a ref to that discussion - I'm curious. > > Here it is (read comments as well): > https://stackoverflow.com/questions/76307370/control-gpio-from-linux-userspace-with-linux-gpio-h > That question looks to me to be confusing how many GPIOs can be requested per request (64) and in total (effectively unlimited) - thinking they are the same. That could be due to their desire to use the gpiod_chip_get_all_lines() convenience function with a chip with more than 64 lines, rather than because they have an actual need for the lines to be managed in a single request. So that doesn't look like a genuine use case to me - just a "what if I want to do X" question. Certainly not something that would warrant a v3 ABI. Cheers, Kent.
On Mon, Oct 02, 2023 at 05:25:05PM +0800, Kent Gibson wrote: > On Mon, Oct 02, 2023 at 12:05:11PM +0300, Andy Shevchenko wrote: > > On Wed, Sep 27, 2023 at 10:23:12PM +0800, Kent Gibson wrote: > > > On Wed, Sep 27, 2023 at 04:59:34PM +0300, Andy Shevchenko wrote: > > > > On Wed, Sep 27, 2023 at 09:49:35PM +0800, Kent Gibson wrote: > > > > > On Wed, Sep 27, 2023 at 03:17:06PM +0300, Andy Shevchenko wrote: > > > > > > On Wed, Sep 27, 2023 at 09:32:11AM +0800, Kent Gibson wrote: ... > > > > > > Yet, it opens a way to scale this in case we might have v3 ABI that let's say > > > > > > allows to work with 512 GPIOs at a time. With your code it will be much harder > > > > > > to achieve and see what you wrote about maintenance (in that case). > > > > > > > > > > v3 ABI?? libgpiod v2 is barely out the door! > > > > > Do you have any cases where 64 lines per request is limiting? > > > > > > > > IIRC it was SO question where the OP asks exactly about breaking the 64 lines > > > > limitation in the current ABI. > > > > > > > > > If that sort of speculation isn't premature optimisation then I don't know > > > > > what is. > > > > > > > > No, based on the real question / discussion, just have no link at hand. > > > > But it's quite a niche, I can agree. > > > > > > Let me know if you find a ref to that discussion - I'm curious. > > > > Here it is (read comments as well): > > https://stackoverflow.com/questions/76307370/control-gpio-from-linux-userspace-with-linux-gpio-h > > > > That question looks to me to be confusing how many GPIOs can be > requested per request (64) and in total (effectively unlimited) - thinking > they are the same. > That could be due to their desire to use the gpiod_chip_get_all_lines() > convenience function with a chip with more than 64 lines, rather than > because they have an actual need for the lines to be managed in a single > request. > > So that doesn't look like a genuine use case to me - just a "what if I > want to do X" question. Certainly not something that would warrant a v3 > ABI. Sure, and I'm not talking about v3 ABI to go for, see the word "might" in my reply in the first paragraph of this message.
On Mon, Oct 02, 2023 at 12:32:22PM +0300, Andy Shevchenko wrote: > On Mon, Oct 02, 2023 at 05:25:05PM +0800, Kent Gibson wrote: > > On Mon, Oct 02, 2023 at 12:05:11PM +0300, Andy Shevchenko wrote: > > > On Wed, Sep 27, 2023 at 10:23:12PM +0800, Kent Gibson wrote: > > > > On Wed, Sep 27, 2023 at 04:59:34PM +0300, Andy Shevchenko wrote: > > > > > On Wed, Sep 27, 2023 at 09:49:35PM +0800, Kent Gibson wrote: > > > > > > On Wed, Sep 27, 2023 at 03:17:06PM +0300, Andy Shevchenko wrote: > > > > > > > On Wed, Sep 27, 2023 at 09:32:11AM +0800, Kent Gibson wrote: > > ... > > > > > > > > Yet, it opens a way to scale this in case we might have v3 ABI that let's say > > > > > > > allows to work with 512 GPIOs at a time. With your code it will be much harder > > > > > > > to achieve and see what you wrote about maintenance (in that case). > > > > > > > > > > > > v3 ABI?? libgpiod v2 is barely out the door! > > > > > > Do you have any cases where 64 lines per request is limiting? > > > > > > > > > > IIRC it was SO question where the OP asks exactly about breaking the 64 lines > > > > > limitation in the current ABI. > > > > > > > > > > > If that sort of speculation isn't premature optimisation then I don't know > > > > > > what is. > > > > > > > > > > No, based on the real question / discussion, just have no link at hand. > > > > > But it's quite a niche, I can agree. > > > > > > > > Let me know if you find a ref to that discussion - I'm curious. > > > > > > Here it is (read comments as well): > > > https://stackoverflow.com/questions/76307370/control-gpio-from-linux-userspace-with-linux-gpio-h > > > > > > > That question looks to me to be confusing how many GPIOs can be > > requested per request (64) and in total (effectively unlimited) - thinking > > they are the same. > > That could be due to their desire to use the gpiod_chip_get_all_lines() > > convenience function with a chip with more than 64 lines, rather than > > because they have an actual need for the lines to be managed in a single > > request. > > > > So that doesn't look like a genuine use case to me - just a "what if I > > want to do X" question. Certainly not something that would warrant a v3 > > ABI. > > Sure, and I'm not talking about v3 ABI to go for, see the word "might" in my > reply in the first paragraph of this message. > Ok, so your original point was pure speculation. Cheers, Kent.
diff --git a/drivers/gpio/gpiolib-cdev.c b/drivers/gpio/gpiolib-cdev.c index e39d344feb28..a5bbbd44531f 100644 --- a/drivers/gpio/gpiolib-cdev.c +++ b/drivers/gpio/gpiolib-cdev.c @@ -1263,35 +1263,32 @@ static long linereq_get_values(struct linereq *lr, void __user *ip) { struct gpio_v2_line_values lv; DECLARE_BITMAP(vals, GPIO_V2_LINES_MAX); + DECLARE_BITMAP(mask, GPIO_V2_LINES_MAX); + DECLARE_BITMAP(bits, GPIO_V2_LINES_MAX); struct gpio_desc **descs; unsigned int i, didx, num_get; - bool val; int ret; /* NOTE: It's ok to read values of output lines. */ if (copy_from_user(&lv, ip, sizeof(lv))) return -EFAULT; - for (num_get = 0, i = 0; i < lr->num_lines; i++) { - if (lv.mask & BIT_ULL(i)) { - num_get++; - descs = &lr->lines[i].desc; - } - } + bitmap_from_arr64(mask, &lv.mask, GPIO_V2_LINES_MAX); + num_get = bitmap_weight(mask, lr->num_lines); if (num_get == 0) return -EINVAL; - if (num_get != 1) { + if (num_get == 1) { + descs = &lr->lines[find_first_bit(mask, lr->num_lines)].desc; + } else { descs = kmalloc_array(num_get, sizeof(*descs), GFP_KERNEL); if (!descs) return -ENOMEM; - for (didx = 0, i = 0; i < lr->num_lines; i++) { - if (lv.mask & BIT_ULL(i)) { - descs[didx] = lr->lines[i].desc; - didx++; - } - } + + didx = 0; + for_each_set_bit(i, mask, lr->num_lines) + descs[didx++] = lr->lines[i].desc; } ret = gpiod_get_array_value_complex(false, true, num_get, descs, NULL, vals); @@ -1301,19 +1298,15 @@ static long linereq_get_values(struct linereq *lr, void __user *ip) if (ret) return ret; - lv.bits = 0; - for (didx = 0, i = 0; i < lr->num_lines; i++) { - if (lv.mask & BIT_ULL(i)) { - if (lr->lines[i].sw_debounced) - val = debounced_value(&lr->lines[i]); - else - val = test_bit(didx, vals); - if (val) - lv.bits |= BIT_ULL(i); - didx++; - } + bitmap_scatter(bits, vals, mask, lr->num_lines); + + for_each_set_bit(i, mask, lr->num_lines) { + if (lr->lines[i].sw_debounced) + __assign_bit(i, bits, debounced_value(&lr->lines[i])); } + bitmap_to_arr64(&lv.bits, bits, GPIO_V2_LINES_MAX); + if (copy_to_user(ip, &lv, sizeof(lv))) return -EFAULT; @@ -1324,35 +1317,35 @@ static long linereq_set_values_unlocked(struct linereq *lr, struct gpio_v2_line_values *lv) { DECLARE_BITMAP(vals, GPIO_V2_LINES_MAX); + DECLARE_BITMAP(mask, GPIO_V2_LINES_MAX); + DECLARE_BITMAP(bits, GPIO_V2_LINES_MAX); struct gpio_desc **descs; unsigned int i, didx, num_set; int ret; - bitmap_zero(vals, GPIO_V2_LINES_MAX); - for (num_set = 0, i = 0; i < lr->num_lines; i++) { - if (lv->mask & BIT_ULL(i)) { - if (!test_bit(FLAG_IS_OUT, &lr->lines[i].desc->flags)) - return -EPERM; - if (lv->bits & BIT_ULL(i)) - __set_bit(num_set, vals); - num_set++; - descs = &lr->lines[i].desc; - } - } + bitmap_from_arr64(mask, &lv->mask, GPIO_V2_LINES_MAX); + bitmap_from_arr64(bits, &lv->bits, GPIO_V2_LINES_MAX); + + num_set = bitmap_gather(vals, bits, mask, lr->num_lines); if (num_set == 0) return -EINVAL; - if (num_set != 1) { + for_each_set_bit(i, mask, lr->num_lines) { + if (!test_bit(FLAG_IS_OUT, &lr->lines[i].desc->flags)) + return -EPERM; + } + + if (num_set == 1) { + descs = &lr->lines[find_first_bit(mask, lr->num_lines)].desc; + } else { /* build compacted desc array and values */ descs = kmalloc_array(num_set, sizeof(*descs), GFP_KERNEL); if (!descs) return -ENOMEM; - for (didx = 0, i = 0; i < lr->num_lines; i++) { - if (lv->mask & BIT_ULL(i)) { - descs[didx] = lr->lines[i].desc; - didx++; - } - } + + didx = 0; + for_each_set_bit(i, mask, lr->num_lines) + descs[didx++] = lr->lines[i].desc; } ret = gpiod_set_array_value_complex(false, true, num_set, descs, NULL, vals);
Currently we have a few bitmap calls that are open coded in the library module. Let's convert them to use generic bitmap APIs instead. Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> --- drivers/gpio/gpiolib-cdev.c | 79 +++++++++++++++++-------------------- 1 file changed, 36 insertions(+), 43 deletions(-)