Re: [PATCH v6 003/164] pwm: Provide pwmchip_alloc() function and a devm variant of it

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:

quoted

On Wed, Feb 14, 2024 at 10:30:50AM +0100, Uwe Kleine-König wrote:

quoted

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().

...

quoted

+#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 (local)
. 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á