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

[Bartosz Golaszewski <brgl@xxxxxxxx>]

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.

Bart

[snip]