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

["Nuno Sá" <noname.nuno@gmail.com>]

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á