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

["Uwe Kleine-König" <u.kleine-koenig@pengutronix.de>]

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

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.

Best regards
Uwe

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

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