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 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(). 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? gpiochip_add_data_with_key() allocates that using kzalloc() and "frees" it with gpio_device_put() right? So your approach suffers from the same 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.) Let's just rename foo_alloc() to foo_get_new() and the problem is gone? 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. Best regards Uwe -- Pengutronix e.K. | Uwe Kleine-König | Industrial Linux Solutions | https://www.pengutronix.de/ |
Attachment:
signature.asc
Description: PGP signature
