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. Best regards Uwe -- Pengutronix e.K. | Uwe Kleine-König | Industrial Linux Solutions | https://www.pengutronix.de/ |
Attachment:
signature.asc
Description: PGP signature
