Message ID | cover.1707900770.git.u.kleine-koenig@pengutronix.de |
---|---|
Headers | show |
Series | pwm: Improve lifetime tracking for pwm_chips | expand |
Hello, On Wed, Feb 14, 2024 at 10:30:47AM +0100, Uwe Kleine-König wrote: > this is v6 of the series introducing better lifetime tracking for > pwmchips that addresses (for now theoretic) lifetime issues of pwm > chips. Addressing these is a necessary precondition to introduce chardev > support for PWMs. > > Locking got more complicated due to non-sleeping chips, so I dropped > the character device patch because it got still more incomplete now. > Also I'm not yet entirely sure about patches #162 and #163 and I expect > them to change before they can go in. My plan for the next merge window > is to get the patches in up to #160. After that the addition of chardev > support (including correct locking) can continue without having to touch > the lowlevel driver. So the idea of this series is to get the driver > adaptions out of the way as this requires some cross-tree coordination. > > The patches that touch files outside of drivers/pwm include: > > - gpio: mvebu: Make use of devm_pwmchip_alloc() function > It already has an Ack by Linus Walleij. > > - drm/bridge: ti-sn65dsi86: Make use of pwmchip_parent() accessor > - drm/bridge: ti-sn65dsi86: Make use of devm_pwmchip_alloc() function > The 2nd already has an Ack by Douglas Anderson which I tend to assume > good enough to merge this via my pwm tree, too. An Ack for the first > patch would be nice. > > - leds: qcom-lpg: Make use of devm_pwmchip_alloc() function > Already has an Ack by Lee Jones. > > - staging: greybus: pwm: Change prototype of helpers to prepare further changes > - staging: greybus: pwm: Make use of pwmchip_parent() accessor > - staging: greybus: pwm: Rely on pwm framework to pass a valid hwpwm > - staging: greybus: pwm: Drop unused gb_connection_set_data() > - staging: greybus: pwm: Rework how the number of PWM lines is determined > - staging: greybus: pwm: Make use of devm_pwmchip_alloc() function > The greybus patches already got an Ack by Greg Kroah-Hartman in an > earlier series, but I dropped it as the patches changed considerably. After getting the needed acks, I pushed out this series in https://git.kernel.org/pub/scm/linux/kernel/git/ukleinek/linux.git pwm/for-next up to patch #161. (But don't let you stop looking at the changes, reviews are still welcome.) Best regards Uwe
On Wed, Feb 14, 2024 at 02:49:26PM +0200, Andy Shevchenko wrote: > On Wed, Feb 14, 2024 at 10:30:50AM +0100, Uwe Kleine-König wrote: > > This function allocates a struct pwm_chip and driver data. Compared to > > the status quo the split into pwm_chip and driver data is new, otherwise > > it doesn't change anything relevant (yet). > > > > The intention is that after all drivers are switched to use this > > allocation function, its possible to add a struct device to struct > > pwm_chip to properly track the latter's lifetime without touching all > > drivers again. Proper lifetime tracking is a necessary precondition to > > introduce character device support for PWMs (that implements atomic > > setting and doesn't suffer from the sysfs overhead of the /sys/class/pwm > > userspace support). > > > > The new function pwmchip_priv() (obviously?) only works for chips > > allocated with pwmchip_alloc(). > > ... > > > +#define PWMCHIP_ALIGN ARCH_DMA_MINALIGN > > + > > +static void *pwmchip_priv(struct pwm_chip *chip) > > +{ > > + return (void *)chip + ALIGN(sizeof(*chip), PWMCHIP_ALIGN); > > +} > > Why not use dma_get_cache_alignment() ? Hmm, that function returns 1 if ARCH_HAS_DMA_MINALIGN isn't defined. The idea of using ARCH_DMA_MINALIGN was to ensure that the priv data has the same minimal alignment as kmalloc(). Took my inspriration from https://lore.kernel.org/r/20240209-counter-align-fix-v2-1-5777ea0a2722@analog.com . The implementation of dma_get_cache_alignment suggests that not all archs provide ARCH_DMA_MINALIGN? Also there is ARCH_KMALLOC_MINALIGN. Hmm, don't know yet what to do here. > > +/* This is the counterpart to pwmchip_alloc */ > > pwmchip_alloc() Ack. > > +EXPORT_SYMBOL_GPL(pwmchip_put); > > > +EXPORT_SYMBOL_GPL(pwmchip_alloc); > > > +EXPORT_SYMBOL_GPL(devm_pwmchip_alloc); > > Are these exported via namespace? If no, can they be from day 1? I added that to my todo list for all pwm functions. Will address that separately. Thanks for your feedback Uwe
On Thu, 2024-02-15 at 13:01 +0100, Uwe Kleine-König wrote: > On Wed, Feb 14, 2024 at 02:49:26PM +0200, Andy Shevchenko wrote: > > On Wed, Feb 14, 2024 at 10:30:50AM +0100, Uwe Kleine-König wrote: > > > This function allocates a struct pwm_chip and driver data. Compared to > > > the status quo the split into pwm_chip and driver data is new, otherwise > > > it doesn't change anything relevant (yet). > > > > > > The intention is that after all drivers are switched to use this > > > allocation function, its possible to add a struct device to struct > > > pwm_chip to properly track the latter's lifetime without touching all > > > drivers again. Proper lifetime tracking is a necessary precondition to > > > introduce character device support for PWMs (that implements atomic > > > setting and doesn't suffer from the sysfs overhead of the /sys/class/pwm > > > userspace support). > > > > > > The new function pwmchip_priv() (obviously?) only works for chips > > > allocated with pwmchip_alloc(). > > > > ... > > > > > +#define PWMCHIP_ALIGN ARCH_DMA_MINALIGN > > > + > > > +static void *pwmchip_priv(struct pwm_chip *chip) > > > +{ > > > + return (void *)chip + ALIGN(sizeof(*chip), PWMCHIP_ALIGN); > > > +} > > > > Why not use dma_get_cache_alignment() ? > > Hmm, that function returns 1 if ARCH_HAS_DMA_MINALIGN isn't defined. The > idea of using ARCH_DMA_MINALIGN was to ensure that the priv data has the > same minimal alignment as kmalloc(). Took my inspriration from > https://lore.kernel.org/r/20240209-counter-align-fix-v2-1-5777ea0a2722@analog.com > . The implementation of dma_get_cache_alignment suggests that not all > archs provide ARCH_DMA_MINALIGN? Also there is ARCH_KMALLOC_MINALIGN. > Hmm, don't know yet what to do here. Here it goes my 2 cents... AFAIK, ARCH_DMA_MINALIGN gives you the same alignment guarantees than devm_kmalloc() for instance. In some archs it will effectively be the same as ARCH_KMALLOC_MINALIGN. Now, I think it only matters if the owners of private data intend to have a DMA safe buffer in their structs. If that is the case, we need to ensure a proper alignment for that structure. In IIO for example, the construct is like this: https://elixir.bootlin.com/linux/latest/source/drivers/iio/dac/ltc2688.c#L96 The buffers should come last in the struct so they are alone in the line. In IIO, Jonathan has a strict policy for this. Like, even if you just want to transfer 2/4 bytes via spi, we need to make the buffer safe (apparently there are some controllers only doing DMA - even for small transfers). I would say that if unsure, go with ARCH_DMA_MINALIGN. You just might waste some space in some archs. OTOH, if you think DMA is not really a thing for pwm chips, you might go ARCH_KMALLOC_MINALIGN. And since you already have your own PWMCHIP_ALIGN, it should be easy to change the requirements down the road (if needed). That said, I'm not familiar with dma_get_cache_alignment(). - Nuno Sá
Hello: This series was applied to chrome-platform/linux.git (for-next) by Uwe Kleine-König <u.kleine-koenig@pengutronix.de>: On Wed, 14 Feb 2024 10:30:47 +0100 you wrote: > Hello, > > this is v6 of the series introducing better lifetime tracking for > pwmchips that addresses (for now theoretic) lifetime issues of pwm > chips. Addressing these is a necessary precondition to introduce chardev > support for PWMs. > > [...] Here is the summary with links: - [v6,001/164] pwm: Provide an inline function to get the parent device of a given chip https://git.kernel.org/chrome-platform/c/4e59267c7a20 - [v6,003/164] pwm: Provide pwmchip_alloc() function and a devm variant of it https://git.kernel.org/chrome-platform/c/024913dbf99f - [v6,029/164] pwm: cros-ec: Change prototype of helpers to prepare further changes https://git.kernel.org/chrome-platform/c/7256c2e79b8e - [v6,030/164] pwm: cros-ec: Make use of pwmchip_parent() accessor https://git.kernel.org/chrome-platform/c/19a568a8d3c4 - [v6,031/164] pwm: cros-ec: Make use of devm_pwmchip_alloc() function https://git.kernel.org/chrome-platform/c/452be9421eda You are awesome, thank you!