Re: [PATCH v1 000/101] pwm: Fix lifetime issues for pwm_chips

[Thierry Reding <thierry.reding@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 4860 bytes --]

On Fri, Oct 06, 2023 at 12:41:02PM +0200, Uwe Kleine-König wrote:
> Hello Thierry,
> 
> On Fri, Oct 06, 2023 at 11:20:03AM +0200, Thierry Reding wrote:
> > On Tue, Aug 08, 2023 at 07:17:50PM +0200, Uwe Kleine-König wrote:
> > > this series addresses the issues I reported already earlier to this
> > > list[1]. It is based on pwm/for-next and several patches I already sent
> > > out, too. Maybe some of these have to be reworked (e.g. Thierry already
> > > signalled not to like the patches dropping runtime error messages) but
> > > in the expectation that I will have to create a v2 for this series, too
> > > and it actually fixes a race condition, I sent the patches out for
> > > review anyhow. For the same reason I didn't Cc: all the individual
> > > maintainers.
> > > 
> > > If you want to actually test I suggest you fetch my complete history
> > > from
> > > 
> > > 	https://git.pengutronix.de/git/ukl/linux pwm-lifetime-tracking
> > > 
> > > . 
> > > 
> > > In the end drivers have to allocate their pwm_chip using
> > > pwmchip_alloc(). This is important for the memory backing the pwm_chip
> > > being able to have a longer life than the driver.
> > 
> > Couldn't we achieve the same thing by just making sure that drivers
> > don't use any of the device-managed APIs to do this? That seems like a
> > slightly less intrusive way of doing things.
> 
> The device-managed APIs are not the problem here. If you allocate in
> .probe and free in .remove there is a problem. (And that's exactly what
> the device managed APIs do.)

Heh... so you're saying that device-managed APIs are the problem here.
Yes, without device-managed APIs you still need to make sure you don't
free while the PWM devices are still in use, but at least you then have
that option.

My point was that with device-managed APIs, freeing memory at ->remove()
comes built-in. Hence why it's confusing to see that this series keeps
using device-managed APIs while claiming to address lifetime issues.

> So no, you cannot. The thing is the struct pwm_chip must stay around until
> the last reference is dropped. So you need some kind of reference
> counting. The canonical way to do that is using a struct device.
> 
> You can try to hide it from the low-level drivers (as gpio does) at the
> cost that you have the driver allocated structure separate from the
> reference counted memory under framework control. The cost is more
> overhead because you have >1 memory chunk (memory fragmentation, less
> cache locality) and more pointers. IMHO the cleaner way is to not hide

Single-chunk allocations can also lead to less cache locality, depending
on the size of your allocations. For instance if you primarily use small
driver-specific data structures, those may fit into the cache while a
combined data structure that consists of the core structure plus driver-
private data may need to be split across multiple cache lines, and you
may not end up using something like the core structure a lot of the
time.

Anyway, I'm not sure PWM is the kind of subsystem where cache locality
is something we need to be concerned about.

> it and have the explicit handling needed in the drivers be not error
> prone (as spi does).
> 
> I agree the switch suggested here is intrusive, but the "new way" a
> driver has to look like is fine, so I'd not hesitate here.
> 
> > > The motivation for this series is to prepare the pwm framework to add a
> > > character device for each pwm_chip for easier and faster access to PWMs
> > > from userspace compared to the sysfs API. For such an extension proper
> > > lifetime tracking is important, too, as such a device can still be open
> > > if a PWM disappears.
> > 
> > As I mentioned before, I'd like to see at least a prototype of the
> > character device patches before I merge this series. There's a whole lot
> > of churn here and without the character device support it's hard to
> > justify.
> 
> The character device stuff is only one reason to get the lifetime
> tracking right. See that oops I can trigger today that is fixed by this
> series.

My recollection is that you need to inject a 5 second sleep into an
apply function in order to trigger this, which is not something you
would usually do, or that someone could trigger by accident. Yes, it
might be theoretically possible to run into this, but so far nobody
has reported this to be an actual problem in practice.

Also, as you have mentioned before, other mechanisms should already
take care of tearing things down properly. If that's not happening for
some reason, that's probably what we should investigate. It'd probably
be less intrusive than 100 patches with churn in every driver.

I'm not saying we don't need this, just trying to put it into
perspective.

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]