Hello Bart, On Wed, Nov 22, 2023 at 11:36:19AM +0100, Bartosz Golaszewski wrote: > On Wed, Nov 22, 2023 at 10:05 AM Uwe Kleine-König > <u.kleine-koenig@xxxxxxxxxxxxxx> wrote: > > 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. It's not in this patch. Up to patch #103 I'm preparing drivers and the code that is moved into the core isn't better than what was done before in each driver. Look at patch #106 which does the relevant conversion in pwmchip_alloc(). When unbinding the mvebu gpio driver the necessary pwmchip_put() is triggered by the devm cleanup registered in devm_pwmchip_alloc(). > 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. With the complete series applied a pwmchip is allocated by pwmchip_alloc() and 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. In this respect I see a certain similarity between your gpio approach and mine for pwm. So either I don't understand your critic on my patch set, or I don't see why it shouldn't apply to your approach, too. Yes, gpio drivers look fine having only ..._alloc() paired with ..._free() and ..._get() with ..._put(). But that's only because you moved that "inconsistency" of kzalloc() <-> put_device() into the gpio core, while I kept it in the drivers. Renaming pwmchip_alloc() to pwmchip_get_new() was a honest suggestion that moves that inconsistency to the core, too. > > 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. It's not only about wasting memory and the time needed to dereference pointers. It's also about complexity that has to be grasped by humans. Also not being in a hot path doesn't mean it's bad to pick the faster approach. Having said that I'm not sure if the hot paths (e.g. gpiod_set_value()) really don't suffer from having two separate allocations. But I guess we're both biased here to our own approach because that's what each of us thought about in detail. > > 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. Please look at the state after patch #107. If you spot an issue there, please tell me. > > 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? Yes, should be fine starting with patch #107. In my tests (on top of patch #108) it works fine. I held /dev/pwmchipX open and unbound the lowlevel driver. The ioctls are caught in the core then and yield an error and the kfree of the pwmchip struct is delayed until /dev/pwmchipX is closed. Best regards Uwe -- Pengutronix e.K. | Uwe Kleine-König | Industrial Linux Solutions | https://www.pengutronix.de/ |
Attachment:
signature.asc
Description: PGP signature
