Re: [PATCH v3 100/108] gpio: mvebu: Make use of devm_pwmchip_alloc() function

[Bartosz Golaszewski <brgl@xxxxxxxx>]

On Sat, Dec 2, 2023 at 1:43 AM Uwe Kleine-König
<u.kleine-koenig@xxxxxxxxxxxxxx> wrote:
>
> On Fri, Dec 01, 2023 at 11:14:32AM +0100, Bartosz Golaszewski wrote:
> > On Tue, Nov 28, 2023 at 10:07 AM Uwe Kleine-König
> > <u.kleine-koenig@xxxxxxxxxxxxxx> wrote:
> > >
> >
> > [snip]
> >
> > >
> > > > I see the
> > > > chip->operational field that is set to false on release. In my
> > > > version, we just use a NULL-pointer to carry the same information.
> > >
> > > Yup, sounds obvious. Your usage of "just" sounds as if your variant was
> > > better. To give the alternative view where the suggested approach sounds
> > > better would be:
> > >
> > > You need a pointer and I "just" a bool that even has a name implying its
> > > function. You need to dereference the pointer in several places as the
> > > needed information is distributed over two structures while it's all
> > > together in a single struct for the usual foo_alloc() + foo_register()
> > > approach.
> > >
> >
> > There's another reason we do that. I'm no longer sure if I mentioned
> > it in my talk (I meant to anyway).
> >
> > In GPIO we have API functions that may be called from any context -
> > thus needing spinlocks for locking - but also driver callbacks that
> > may use mutexes internally or otherwise sleep. I don't know if this is
> > the case for PWM too but in GPIO we may end up in a situation where if
> > we used a spinlock to protect some kind of an "is_operational" field,
> > we'd end up sleeping with a spinlock taken and if we used a mutex, we
> > couldn't use API function from atomic contexts.
> >
> > This is the reason behind locking being so broken in GPIO at the
> > moment and why I'm trying to fix it this release cycle.
> >
> > Splitting the implementation into two structures and protecting the
> > pointer to the provider structure with SRCU has the benefit of not
> > limiting us in what locks we use underneath.
> >
> > Every subsystem has its own issues and we need to find something
> > generic enough to cover them all (or most of them anyway). I don't
> > think having a single structure cuts it.
>
> I'm convinced it works. I introduced a wrapper pwmchip_lock() that for
> now uses a mutex and once we have fast pwm_chips it uses a mutex for
> sleeping pwm_chips and a spinlock for the fast ones.
>
> That's similar to how struct irq_chip::irq_bus_lock works. For sleeping
> chips that callback uses a mutex, for fast chips a spinlock.
>

Fair enough. I'd love to see a benchmark of what's faster one day
though: two structures with dereferencing and SRCU or one structure
with mutex/spinlock.

By "fair enough" I mean: I still don't like it for the reasons I
mentioned before but I cannot point out anything technically wrong.

Bart

> Best regards
> Uwe
>
> --
> Pengutronix e.K.                           | Uwe Kleine-König            |
> Industrial Linux Solutions                 | https://www.pengutronix.de/ |