Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:7 as permitted sender) client-ip=2620:137:e000::3:7;
MIME-Version: 1.0
References: <20231212054253.50094-1-warthog618@gmail.com>
In-Reply-To: <20231212054253.50094-1-warthog618@gmail.com>
From:   Bartosz Golaszewski <brgl@bgdev.pl>
Date:   Tue, 12 Dec 2023 18:09:00 +0100
Message-ID: <CAMRc=Me90Lu7Duc8-4xSfDcHQd6M7+0t0O8FAa6jiizp-OO5=Q@mail.gmail.com>
Subject: Re: [PATCH 0/4] gpiolib: cdev: relocate debounce_period_us
To:     Kent Gibson <warthog618@gmail.com>
Cc:     linux-kernel@vger.kernel.org, linux-gpio@vger.kernel.org,
        linus.walleij@linaro.org, andy@kernel.org
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

On Tue, Dec 12, 2023 at 6:43=E2=80=AFAM Kent Gibson <warthog618@gmail.com> =
wrote:
>
> This series contains minor improvements to gpiolib-cdev.
>
> The banner change is relocating the debounce_period_us from gpiolib's
> struct gpio_desc to cdev's struct line.  The first patch stores the
> field locally in cdev.  The second removes the now unused field from
> gpiolib.
>
> The third patch is somewhat related and removes a FIXME from
> gpio_desc_to_lineinfo().  The FIXME relates to a race condition in
> the calculation of the used  flag, but I would assert that from
> the userspace perspective the read operation itself is inherently racy.
> The line being reported as unused in the info provides no guarantee -
> it just an indicator that requesting the line is likely to succeed -
> assuming the line is not otherwise requested in the meantime.
> Give the overall operation is racy, trying to stamp out an unlikely
> race within the operation is pointless. Accept it as a possibility
> that has negligible side-effects and reduce the number of locks held
> simultaneously and the duration that the gpio_lock is held.
>
> The fourth patch is unrelated to debounce or info, but addresses Andy's
> recent assertion that the linereq get/set values functions are confusing
> and under documented.  Figured I may as well add that while I was in
> there.
>
> Kent Gibson (4):
>   gpiolib: cdev: relocate debounce_period_us from struct gpio_desc
>   gpiolib: remove debounce_period_us from struct gpio_desc
>   gpiolib: cdev: reduce locking in gpio_desc_to_lineinfo()
>   gpiolib: cdev: improve documentation of get/set values
>
>  drivers/gpio/gpiolib-cdev.c | 257 ++++++++++++++++++++++++++++--------
>  drivers/gpio/gpiolib.c      |   3 -
>  drivers/gpio/gpiolib.h      |   5 -
>  3 files changed, 201 insertions(+), 64 deletions(-)
>
> --
> 2.39.2
>

Patches 2-4 look fine, I was about to review patch 1 in detail but I
thought I'd just throw this one in here before we commit to a specific
solution.

For some reason I thought this would not work but I'm now considering
it as an alternative approach: is there anything wrong with adding
struct kref to struct line, allocating it separately per-line when
gpio_chardev_data is created, referencing it from struct linereq when
the line is being requested, and dropping the reference from
gpio_chardev_data and linereq when either is being removed? Other than
the increased number of allocations?

Bartosz