Re: [PATCH v1 070/101] pwm: Ensure a struct pwm have the same lifetime as its pwm_chip

[Thierry Reding <thierry.reding@gmail.com>]

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

On Fri, Oct 06, 2023 at 01:04:51PM +0200, Uwe Kleine-König wrote:
> On Fri, Oct 06, 2023 at 11:38:12AM +0200, Thierry Reding wrote:
> > On Tue, Aug 08, 2023 at 07:19:00PM +0200, Uwe Kleine-König wrote:
> > > It's required to not free the memory underlying a requested PWM
> > > while a consumer still has a reference to it. While currently a pwm_chip
> > > doesn't life long enough in all cases, linking the struct pwm to the
> > > pwm_chip results in the right lifetime as soon as the pwmchip is living
> > > long enough. This happens with the following commits.
> > > 
> > > Note this is a breaking change for all pwm drivers that don't use
> > > pwmchip_alloc().
> > > 
> > > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> > > ---
> > >  drivers/pwm/core.c  | 24 +++++++++---------------
> > >  include/linux/pwm.h |  2 +-
> > >  2 files changed, 10 insertions(+), 16 deletions(-)
> > > 
> > > diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c
> > > index cfcddf62ab01..3b8d41fdda1b 100644
> > > --- a/drivers/pwm/core.c
> > > +++ b/drivers/pwm/core.c
> > > @@ -198,7 +198,7 @@ static bool pwm_ops_check(const struct pwm_chip *chip)
> > >  
> > >  void *pwmchip_priv(struct pwm_chip *chip)
> > >  {
> > > -	return &chip[1];
> > > +	return &chip->pwms[chip->npwm];
> > 
> > I already disliked &chip[1] and this isn't making things any better. I
> > fully realize that this is going to give us the right address, but it
> > just looks wrong. Can we not do something like:
> > 
> > 	return (void *)chip + sizeof(*chip);
> 
> In practise this works, but I'm not 100% confident that the compiler
> might not add padding that breaks this. I don't particularly like this
> function either and will think a bit more about it for v2.

I'm not at all a fan of this whole pwmchip_alloc() business and I would
prefer if we could somehow just keep embedding this into the driver-
specific structures and take care of the lifetime management with less
intrusion.

However, I don't see how that could easily be done. It would be slightly
easier if we didn't use the flexible array, I suppose.

> 
> > instead? That would make it more explict that we're trying to get at the
> > extra data that was allocated. It also makes things a bit more robust
> > and doesn't explode when somebody decides to add fields after "pwms".
> 
> Things will explode if the flexible array member isn't at the end of
> struct pwm_chip no matter how you implement pwmchip_priv.

Perhaps one more reason to avoid the flexible array member. It's not
that big a deal, but I'm all for keeping things simple, and that whole
business of computing the allocation size and then making sure we point
at the right offsets just doesn't seem like the optimal way to do
things, even though I'm well aware that it's common practice elsewhere.

Thierry

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