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

[Bartosz Golaszewski <brgl@xxxxxxxx>]

On Mon, Dec 4, 2023 at 9:27 PM Bartosz Golaszewski <brgl@xxxxxxxx> wrote:
>
> 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.
>

Actually there is one thing that - while not technically wrong - makes
the split solution better. In case of your abstracted lock, you find
yourself in a very all-or-nothing locking situation, where all of the
structure is locked or none is. With SRCU protecting just the pointer
to implementation, we can easily factor that part out and leave
whatever fine-grained locking is required to the subsystem.

Additionally: the pointer to implementation has many readers but only
one writer. I believe this to be the same for your "operational"
field. I don't know the PWM code very well but I can only guess that
the situation is similar, where subsystem data structures are read
more often than they are modified and multiple readers could access
the structure at the same time lowering latencies.

Just another 2 cents.

Bart

> 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/ |