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

[Bartosz Golaszewski <brgl@xxxxxxxx>]

On Wed, Nov 22, 2023 at 10:05 AM Uwe Kleine-König
<u.kleine-koenig@xxxxxxxxxxxxxx> wrote:
>
> Hello Bart,
>
> On Tue, Nov 21, 2023 at 05:11:11PM +0100, Uwe Kleine-König wrote:
> > On Tue, Nov 21, 2023 at 03:02:39PM +0100, Bartosz Golaszewski wrote:
> > > Eh... I had a talk at LPC where I explained why I really dislike this
> > > approach but I guess this ship has sailed now and it's not a subsystem
> > > where I have any say anyway.
> >
> > Is there a record of your talk? I'm open to hear your arguments.
>
> I found your slides at
> https://lpc.events/event/17/contributions/1627/attachments/1258/2725/Linux%20Plumbers%20Conference%202023.pdf
>

My talk is here: https://www.youtube.com/watch?v=VxaAorwL89c&t=29310s

> The main critic as I understand it about the "alloc_foo() +
> register_foo()" approach is: "Breaks life-time logic - the driver
> allocates the object but is not responsible for freeing it".
>
> Yes, the driver allocates the object (via a subsystem helper). It is not
> responsible for freeing the object, but the driver must drop its
> reference to this object when going away. So foo_alloc() is paired by
> foo_put().
>

Is it though? I don't see any pwmchip_put() being called in this
patch. I assume it's done implicitly but that's just confusing and
does break the scope.

> The solution you present as the good way has the struct device in the
> foo_wrapper. In GPIO land that's struct gpio_device, right?

Exactly.

> gpiochip_add_data_with_key() allocates that using kzalloc() and "frees"
> it with gpio_device_put() right? So your approach suffers from the same

No, the structure is allocated by kzalloc() but it's life-time is tied
with the struct device embedded in it and it's freed in the device's
.release() callback when the last reference is dropped.

> inconsistency, the only upside is that you do that once at the subsystem
> level instead of in each driver. (And in return you have two allocations
> (priv + foo_wrapper) while the "alloc_foo() + register_foo()" approach
> only needs one.)

Memory is cheap and this is not a hot path, so it isn't a big deal.

>
> Let's just rename foo_alloc() to foo_get_new() and the problem is gone?
>

Nope, because from a quick glance at PWM code, I'm convinced it will
suffer from the same hot-unplug problem I described in my talk. In
which case this rework will not fix all the issues.

> In the implementation of foo_get_new() kzalloc() is still paired with
> put_device() in foo_put(), but IMHO that's fine. The responsibility to
> kfree() is traded to the struct device with device_initialize() in
> return for a reference to the device. That's something you won't get rid
> of while keeping the concept of reference counting.
>

But if the PWM driver is unbound with users still holding references -
do you have a mechanism to handle that?

Bart

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