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

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

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

On Sun, Oct 01, 2023 at 01:10:24PM +0200, Uwe Kleine-König wrote:
> Hello again,
> 
> On Tue, Sep 26, 2023 at 12:06:25PM +0200, Uwe Kleine-König 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.
> > > 
> > > 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.
> > 
> > I wonder how this topic will continue. This series fixes a lifetime
> > issue that can result in a userspace triggered oops and it builds the
> > base for my efforts to create a /dev/pwmchipX for faster control of PWMs
> > from userspace (compared to sysfs). (Currently in the prototype stage.)
> > 
> > I'd like to get this in during the next merge window, please tell me
> > what needs to be done to make this happen.
> 
> One problem I noticed yesterday is that this series depends on patch
> "drm/ssd130x: Print the PWM's label instead of its number" that
> currently waits in drm-misc-next for getting in the main line. The
> series could for sure be reworked to not rely on this patch, but I'd
> prefer to wait until after the next merge window instead of reworking
> it.
> 
> Still, getting some feedback here in the mean time would be nice. The
> questions I wonder about myself are:
> 
>  - In patch #1, devm_pwmchip_alloc() could get another parameter for the
>    .ops member. This would save a line per driver like
> 
>    	chip->ops = &pwm_clk_ops;
> 
>    in return for an additional parameter that yields longer lines in the
>    drivers.
> 
>  - In patch #101 instead of using &pwm_lock a per-pwmchip lock could be
>    used for pwm_apply_state(). This would allow to parallelize pwm calls
>    for different chips; I don't know how much this matters. Maybe the
>    sensible option here is to keep it simple for now (i.e. how I
>    implemented it now) until someone complains? (But see also the next
>    item.)
> 
>  - A further complication is the requirement of pwm-ir-tx for faster
>    pwm_apply_state() calls, see
> 
> 	https://lore.kernel.org/linux-pwm/ZRb5OWvx3GxYWf9g@gofer.mess.org
>    	https://lore.kernel.org/linux-pwm/1bd5241d584ceb4d6b731c4dc3203fb9686ee1d1.1696156485.git.sean@mess.org
> 
>    . This complicates the locking scheme, I didn't try to address that
>    yet.

Frankly, I think per-chip locking is the only way to support slow
busses. If we use the subsystem-wide lock, effectively all chips become
slow and unusable for things like pwm-ir-tx.

Perhaps we can draw some inspiration from how the IRQ infrastructure
deals with this? IRQ chips have irq_bus_lock() and irq_bus_sync_unlock()
callbacks to deal with this. We could have something similar for PWM
chips. Perhaps that's even something that could be handled in the core
by checking a "might_sleep" flag for the chip.

Thierry

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