Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:2 as permitted sender) client-ip=2620:137:e000::3:2;
Date:   Wed, 13 Dec 2023 07:58:04 +0800
From:   Kent Gibson <warthog618@gmail.com>
To:     Bartosz Golaszewski <brgl@bgdev.pl>
Cc:     linux-kernel@vger.kernel.org, linux-gpio@vger.kernel.org,
        linus.walleij@linaro.org, andy@kernel.org
Subject: Re: [PATCH 0/4] gpiolib: cdev: relocate debounce_period_us
Message-ID: <ZXjzjOtKFoMRhKA-@rigel>
References: <20231212054253.50094-1-warthog618@gmail.com>
 <CAMRc=Me90Lu7Duc8-4xSfDcHQd6M7+0t0O8FAa6jiizp-OO5=Q@mail.gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <CAMRc=Me90Lu7Duc8-4xSfDcHQd6M7+0t0O8FAa6jiizp-OO5=Q@mail.gmail.com>
Precedence: bulk

On Tue, Dec 12, 2023 at 06:09:00PM +0100, Bartosz Golaszewski wrote:
> On Tue, Dec 12, 2023 at 6:43 AM 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?
>

The collection of struct line always has to be global, right, as both
gpio_chardev_data and linereq are ephemeral.  e.g. if one process requests
a line and another checks the lineinfo, those will have distinct
gpio_chardev_data.

But the key issue is that the linereq and struct line lifetimes are
strictly tied - a struct line does not live beyond the containing linereq.
Leaving the struct line alive after the linereq is released is just wrong.
The line has been released back to gpiolib so there can be no
supplemental info left.
If you want any such info to persist beyond the line release then it
should be located in gpiolib itself, not cdev.

Cheers,
Kent.