| Message ID | 9577d6053a5a52536057dc8654ff567181c2da82.1707900770.git.u.kleine-koenig@pengutronix.de (mailing list archive) |
|---|---|
| State | Handled Elsewhere, archived |
| Headers | show |
| Series | pwm: Improve lifetime tracking for pwm_chips | expand |
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() ? ... > +/* This is the counterpart to pwmchip_alloc */ pwmchip_alloc() ... > +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? ... > +static inline void pwmchip_put(struct pwm_chip *chip) > +{ > +} Can be one line, but it's up to the present style in this header.
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á
diff --git a/Documentation/driver-api/driver-model/devres.rst b/Documentation/driver-api/driver-model/devres.rst index c5f99d834ec5..e4df72c408d2 100644 --- a/Documentation/driver-api/driver-model/devres.rst +++ b/Documentation/driver-api/driver-model/devres.rst @@ -420,6 +420,7 @@ POWER devm_reboot_mode_unregister() PWM + devm_pwmchip_alloc() devm_pwmchip_add() devm_pwm_get() devm_fwnode_pwm_get() diff --git a/Documentation/driver-api/pwm.rst b/Documentation/driver-api/pwm.rst index 3c28ccc4b611..b41b1c56477f 100644 --- a/Documentation/driver-api/pwm.rst +++ b/Documentation/driver-api/pwm.rst @@ -143,11 +143,12 @@ to implement the pwm_*() functions itself. This means that it's impossible to have multiple PWM drivers in the system. For this reason it's mandatory for new drivers to use the generic PWM framework. -A new PWM controller/chip can be added using pwmchip_add() and removed -again with pwmchip_remove(). pwmchip_add() takes a filled in struct -pwm_chip as argument which provides a description of the PWM chip, the -number of PWM devices provided by the chip and the chip-specific -implementation of the supported PWM operations to the framework. +A new PWM controller/chip can be allocated using pwmchip_alloc(), then +registered using pwmchip_add() and removed again with pwmchip_remove(). To undo +pwmchip_alloc() use pwmchip_put(). pwmchip_add() takes a filled in struct +pwm_chip as argument which provides a description of the PWM chip, the number +of PWM devices provided by the chip and the chip-specific implementation of the +supported PWM operations to the framework. When implementing polarity support in a PWM driver, make sure to respect the signal conventions in the PWM framework. By definition, normal polarity diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c index 830a697826af..9fc6f4fa71d6 100644 --- a/drivers/pwm/core.c +++ b/drivers/pwm/core.c @@ -454,6 +454,64 @@ of_pwm_single_xlate(struct pwm_chip *chip, const struct of_phandle_args *args) } EXPORT_SYMBOL_GPL(of_pwm_single_xlate); +#define PWMCHIP_ALIGN ARCH_DMA_MINALIGN + +static void *pwmchip_priv(struct pwm_chip *chip) +{ + return (void *)chip + ALIGN(sizeof(*chip), PWMCHIP_ALIGN); +} + +/* This is the counterpart to pwmchip_alloc */ +void pwmchip_put(struct pwm_chip *chip) +{ + kfree(chip); +} +EXPORT_SYMBOL_GPL(pwmchip_put); + +struct pwm_chip *pwmchip_alloc(struct device *parent, unsigned int npwm, size_t sizeof_priv) +{ + struct pwm_chip *chip; + size_t alloc_size; + + alloc_size = size_add(ALIGN(sizeof(*chip), PWMCHIP_ALIGN), sizeof_priv); + + chip = kzalloc(alloc_size, GFP_KERNEL); + if (!chip) + return ERR_PTR(-ENOMEM); + + chip->dev = parent; + chip->npwm = npwm; + + pwmchip_set_drvdata(chip, pwmchip_priv(chip)); + + return chip; +} +EXPORT_SYMBOL_GPL(pwmchip_alloc); + +static void devm_pwmchip_put(void *data) +{ + struct pwm_chip *chip = data; + + pwmchip_put(chip); +} + +struct pwm_chip *devm_pwmchip_alloc(struct device *parent, unsigned int npwm, size_t sizeof_priv) +{ + struct pwm_chip *chip; + int ret; + + chip = pwmchip_alloc(parent, npwm, sizeof_priv); + if (IS_ERR(chip)) + return chip; + + ret = devm_add_action_or_reset(parent, devm_pwmchip_put, chip); + if (ret) + return ERR_PTR(ret); + + return chip; +} +EXPORT_SYMBOL_GPL(devm_pwmchip_alloc); + static void of_pwmchip_add(struct pwm_chip *chip) { if (!pwmchip_parent(chip) || !pwmchip_parent(chip)->of_node) diff --git a/include/linux/pwm.h b/include/linux/pwm.h index 29a7d9140f77..4a6568dfdf3f 100644 --- a/include/linux/pwm.h +++ b/include/linux/pwm.h @@ -403,6 +403,10 @@ static inline bool pwm_might_sleep(struct pwm_device *pwm) int pwm_capture(struct pwm_device *pwm, struct pwm_capture *result, unsigned long timeout); +void pwmchip_put(struct pwm_chip *chip); +struct pwm_chip *pwmchip_alloc(struct device *parent, unsigned int npwm, size_t sizeof_priv); +struct pwm_chip *devm_pwmchip_alloc(struct device *parent, unsigned int npwm, size_t sizeof_priv); + int __pwmchip_add(struct pwm_chip *chip, struct module *owner); #define pwmchip_add(chip) __pwmchip_add(chip, THIS_MODULE) void pwmchip_remove(struct pwm_chip *chip); @@ -475,6 +479,24 @@ static inline int pwm_capture(struct pwm_device *pwm, return -EINVAL; } +static inline void pwmchip_put(struct pwm_chip *chip) +{ +} + +static inline struct pwm_chip *pwmchip_alloc(struct device *parent, + unsigned int npwm, + size_t sizeof_priv) +{ + return ERR_PTR(-EINVAL); +} + +static inline struct pwm_chip *devm_pwmchip_alloc(struct device *parent, + unsigned int npwm, + size_t sizeof_priv) +{ + return pwmchip_alloc(parent, npwm, sizeof_priv); +} + static inline int pwmchip_add(struct pwm_chip *chip) { return -EINVAL;
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(). Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de> --- .../driver-api/driver-model/devres.rst | 1 + Documentation/driver-api/pwm.rst | 11 ++-- drivers/pwm/core.c | 58 +++++++++++++++++++ include/linux/pwm.h | 22 +++++++ 4 files changed, 87 insertions(+), 5 deletions(-)