From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from bombadil.infradead.org (bombadil.infradead.org [198.137.202.133]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id 2E19AE7F150 for ; Wed, 27 Sep 2023 01:32:53 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20210309; h=Sender: Content-Transfer-Encoding:Content-Type:List-Subscribe:List-Help:List-Post: List-Archive:List-Unsubscribe:List-Id:In-Reply-To:MIME-Version:References: Message-ID:Subject:Cc:To:From:Date:Reply-To:Content-ID:Content-Description: Resent-Date:Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID: List-Owner; bh=lbYZxCNo3ByU6ExGAYQIBStAysl7c82Y8Foj3yBvjGQ=; b=4c+bYVhDux0LPK XRjLgSbmWE/LJKHGYVJWOPqWS1tIpL9z+H1Qg8whzE58DSJmoht2ErNvnyt+W9ge6ZYNyO9jsMRib GjJLaM3XZNNt1C9Z13Qji9/cg4+3suC23VQ3uKc3+fMSHZY2m2WDQwIVW2pOoh7GqLSqPGsXEl1mQ eHwlSDdIIfBPZwjYtuvt0caCzptaJIxaVaR9pxNEcl+vQkUcmHPMo8eFMP4S34C3POGofG+yrBCpx JJftiUqpXtGczCV5rqMQ9EiZlRoBRG3qltUE0OiLcOrya6BbwPwBZ8Qc0eIV+Vso2tvBUkG2Sv8qu 47It7QLoR/wHc5i3CGfA==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.96 #2 (Red Hat Linux)) id 1qlJPc-00HJ9B-0t; Wed, 27 Sep 2023 01:32:24 +0000 Received: from mail-pj1-x1036.google.com ([2607:f8b0:4864:20::1036]) by bombadil.infradead.org with esmtps (Exim 4.96 #2 (Red Hat Linux)) id 1qlJPZ-00HJ8h-1H for linux-arm-kernel@lists.infradead.org; Wed, 27 Sep 2023 01:32:23 +0000 Received: by mail-pj1-x1036.google.com with SMTP id 98e67ed59e1d1-27747002244so4986342a91.2 for ; Tue, 26 Sep 2023 18:32:20 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20230601; t=1695778340; x=1696383140; darn=lists.infradead.org; h=in-reply-to:content-disposition:mime-version:references:message-id :subject:cc:to:from:date:from:to:cc:subject:date:message-id:reply-to; bh=hP+g2Zhox+Th+oVPcl2goCX8vInD7cvVYSsX0s/QkpE=; b=LPvRNedu1nrpyNIekPxxi3xCotfiNVEu+xscwBi5V15Wbb5E46YxshaGtChMZBlW0m z9M49KLSPhmGJO3TiLDaFKhbGvqIZt7DeZLYbsHy5hFb4xlAHFoqbxbQyXI/BTpJQ4xt rc42enzmfiLj/O+3JWvGZy6oj4MRNShyqxkisi0U+ER3rRI9hvsqLt5UjSXrWD6ZbOJm +iUKGmSMj5O0LJIkTE/GdeFkR6Xc//elSax3ff82aahF4sX5Tj5u7yH3K7tN1Ce9Qctw xUNE9UbjM5bEPadRFqyA++h8Nl2pkQ3HmQN1AVi3obXDHyd+u+yy9iNbax9DJlvK44HE 8hKg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1695778340; x=1696383140; h=in-reply-to:content-disposition:mime-version:references:message-id :subject:cc:to:from:date:x-gm-message-state:from:to:cc:subject:date :message-id:reply-to; bh=hP+g2Zhox+Th+oVPcl2goCX8vInD7cvVYSsX0s/QkpE=; b=hh1D4BQKAGzc8A4sIL7kfmruxwFlso8xYjEUq5SYC9enIb1MagKBXzGy2a4htWC71+ 6vcQt+WKCQryMZkWWOUpLEjJmGEfkzypXkd8aLDjg8gyPPt576wTmn2t9Gl/At8+Uc1D E4i7MLTjS55CkNRj7IuW6/nwSgKpYAcmHVWdCmdRUj3S4azJ3zpVHJRA/ZB1S7NnbJvh Us7XGcZcynnOUKpWmCktZ4sbiAydcIFO37yW+zibOoBJmTSD2UtkTU2WepTrS+ala5rN FHeR5xv3W8uFTVtDB4450E95IpCvPX57yGrTsWK9psDqRhkoKzChLR1Jy4zdb2bbi6hq RFuw== X-Gm-Message-State: AOJu0YzWLRb48fdY4jXNuS49VonIgEIYf5D4FGbfy7Rrc8Q/vyWAWy9Y H7vH+xT2NeqfiHRz2Fvbm9o= X-Google-Smtp-Source: AGHT+IGPiO2n9poA5FBFxwbSW7wzdgA+aBNcevYeCBgsoPmyd4ftTzGq/klGeCdciA9mUzGWIBP/GQ== X-Received: by 2002:a17:90a:6046:b0:25c:8b5e:814 with SMTP id h6-20020a17090a604600b0025c8b5e0814mr377145pjm.44.1695778339932; Tue, 26 Sep 2023 18:32:19 -0700 (PDT) Received: from sol (60-242-83-31.tpgi.com.au. [60.242.83.31]) by smtp.gmail.com with ESMTPSA id 14-20020a17090a004e00b0025dc5749b4csm13516723pjb.21.2023.09.26.18.32.14 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Tue, 26 Sep 2023 18:32:19 -0700 (PDT) Date: Wed, 27 Sep 2023 09:32:11 +0800 From: Kent Gibson To: Andy Shevchenko Cc: Linus Walleij , Bartosz Golaszewski , Yury Norov , linux-gpio@vger.kernel.org, linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org, Shubhrajyoti Datta , Srinivas Neeli , Michal Simek , Bartosz Golaszewski , Andy Shevchenko , Rasmus Villemoes , Marek =?iso-8859-1?Q?Beh=FAn?= Subject: Re: [PATCH v1 5/5] gpiolib: cdev: Utilize more bitmap APIs Message-ID: References: <20230926052007.3917389-1-andriy.shevchenko@linux.intel.com> <20230926052007.3917389-6-andriy.shevchenko@linux.intel.com> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <20230926052007.3917389-6-andriy.shevchenko@linux.intel.com> X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20230926_183221_438559_4B3642F9 X-CRM114-Status: GOOD ( 27.69 ) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org On Tue, Sep 26, 2023 at 08:20:07AM +0300, Andy Shevchenko wrote: > 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. > Firstly, I didn't consider using the bitmap module here as, in my mind at least, that is intended for bitmaps wider than 64 bits, or with variable width. In this case the bitmap is fixed at 64 bits, so bitops seemed more appropriate. And I would argue that they aren't "open coded" - they are parallelized to reduce the number of passes over the bitmap. This change serialises them, e.g. the get used to require 2 passes over the bitmap, it now requires 3 or 4. The set used to require 1 and now requires 2. And there are additional copies that the original doesn't require. So your change looks less efficient to me - unless there is direct hardware support for bitmap ops?? Wrt the argument that the serialized form is clearer and more maintainable, optimised code is frequently more cryptic - as noted in bitmap.c itself, and this code has remained unchanged since it was merged 3 years ago, so the only maintenance it has required is to be more maintainable?? Ok then. Your patch is functionally equivalent and pass my uAPI tests, so Tested-by: Kent Gibson but my preference is to leave it as is. Cheers, Kent. > Signed-off-by: Andy Shevchenko > --- > drivers/gpio/gpiolib-cdev.c | 79 +++++++++++++++++-------------------- > 1 file changed, 36 insertions(+), 43 deletions(-) > > 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); > -- > 2.40.0.1.gaa8946217a0b > _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel