Received-SPF: pass (google.com: domain of linux-kernel+bounces-89744-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1;
Date: Sun, 3 Mar 2024 13:09:28 +0000
From: Jonathan Cameron <jic23@kernel.org>
To: Nuno =?UTF-8?B?U8Oh?= <noname.nuno@gmail.com>
Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com>, Vinod Koul
 <vkoul@kernel.org>, Linus Walleij <linus.walleij@linaro.org>, Jonathan
 Cameron <Jonathan.Cameron@huawei.com>, Mark Brown <broonie@kernel.org>,
 Kees Cook <keescook@chromium.org>, linux-arm-kernel@lists.infradead.org,
 dmaengine@vger.kernel.org, linux-kernel@vger.kernel.org,
 linux-iio@vger.kernel.org, linux-spi@vger.kernel.org,
 netdev@vger.kernel.org, linux-hardening@vger.kernel.org, Lars-Peter Clausen
 <lars@metafoo.de>, "David S. Miller" <davem@davemloft.net>, Eric Dumazet
 <edumazet@google.com>, Jakub Kicinski <kuba@kernel.org>, Paolo Abeni
 <pabeni@redhat.com>, "Gustavo A. R. Silva" <gustavoars@kernel.org>, Nuno Sa
 <nuno.sa@analog.com>
Subject: Re: [PATCH v4 5/8] iio: core: Use new helpers from overflow.h in
 iio_device_alloc()
Message-ID: <20240303130928.0c2fea09@jic23-huawei>
In-Reply-To: <9519dda9acd9db009dcb43102cc9b36943b35217.camel@gmail.com>
References: <20240228204919.3680786-1-andriy.shevchenko@linux.intel.com>
	<20240228204919.3680786-6-andriy.shevchenko@linux.intel.com>
	<9519dda9acd9db009dcb43102cc9b36943b35217.camel@gmail.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: quoted-printable

On Thu, 29 Feb 2024 16:29:43 +0100
Nuno S=C3=A1 <noname.nuno@gmail.com> wrote:

> On Wed, 2024-02-28 at 22:41 +0200, Andy Shevchenko wrote:
> > We have two new helpers struct_size_with_data() and struct_data_pointer=
()
> > that we can utilize in iio_device_alloc(). Do it so.
> >=20
> > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> > Reviewed-by: Nuno Sa <nuno.sa@analog.com>
> > ---
> > =C2=A0drivers/iio/industrialio-core.c | 5 ++---
> > =C2=A01 file changed, 2 insertions(+), 3 deletions(-)
> >=20
> > diff --git a/drivers/iio/industrialio-core.c b/drivers/iio/industrialio=
-core.c
> > index 1986b3386307..223013725e32 100644
> > --- a/drivers/iio/industrialio-core.c
> > +++ b/drivers/iio/industrialio-core.c
> > @@ -1644,7 +1644,7 @@ struct iio_dev *iio_device_alloc(struct device *p=
arent,
> > int sizeof_priv)
> > =C2=A0	size_t alloc_size;
> > =C2=A0
> > =C2=A0	if (sizeof_priv)
> > -		alloc_size =3D ALIGN(alloc_size, IIO_DMA_MINALIGN) +
> > sizeof_priv;
> > +		alloc_size =3D struct_size_with_data(iio_dev_opaque,
> > IIO_DMA_MINALIGN, sizeof_priv);
> > =C2=A0	else
> > =C2=A0		alloc_size =3D sizeof(struct iio_dev_opaque);
> > =C2=A0
> > @@ -1655,8 +1655,7 @@ struct iio_dev *iio_device_alloc(struct device *p=
arent,
> > int sizeof_priv)
> > =C2=A0	indio_dev =3D &iio_dev_opaque->indio_dev;
> > =C2=A0
> > =C2=A0	if (sizeof_priv)
> > -		indio_dev->priv =3D (char *)iio_dev_opaque +
> > -			ALIGN(sizeof(struct iio_dev_opaque),
> > IIO_DMA_MINALIGN);
> > +		indio_dev->priv =3D struct_data_pointer(iio_dev_opaque,
> > IIO_DMA_MINALIGN); =20
>=20
> I'd +1 for implementing what Kees suggested in IIO. Only thing is (I thin=
k), we
> need to move struct iio_dev indioo_dev to the end of struct iio_dev_opaqu=
e.

That is going to be messy and without horrible hacks (I think) add more pad=
ding we
don't need.  At the moment the struct iio_dev and the struct iio_dev_opaque
are aligned as at the start of the structure.

The priv data is aligned by padding the larger struct iio_dev_opaque,
so if you want the priv handle to be to data defined in struct iio_dev you =
would
need to add additional padding so that

struct iio_dev_opaque {
	stuff...
	// this next __aligned() is implicit anyway because of the rules for
	// a structure always being aligned to the alignment of it's max aligned
	// element.
	struct iio_dev __aligned (IIO_DMA_ALIGN) { =20
		stuff
		u8 priv[] __aligned(IIO_DMA_ALIGN);
	}
}

How about using what Kees suggests on the iio_dev_opaque (which think is cl=
eaner
anyway as that's what we are allocating) and keeping the magic pointer to p=
riv
in the struct iio_dev; The compiler looses some visibility for iio_priv() a=
ccesses
but can it do much with those anyway? They always get cast to a struct driv=
er_specific *
and getting the original allocation wrong is not easy to do as we pass
that struct size in.  Note, for others not aware of what is going on here, =
the
priv pointer in iio_dev is to allow efficient static inline iio_priv() call=
s without
needing to either make a function call, or expose the internals of the opaq=
ue
structure in which the iio_dev and the priv data are embedded.

Standard pattern is:

struct driver_specific *bob;
struct iio_dev *indio_dev =3D dev_iio_device_alloc(dev, sizeof(*bob));
// which allocates the iio_dev_opaque, but returns the contained iio_dev
bob =3D iio_priv(indio_dev);

So

struct iio_dev_opaque {
	struct iio_dev indio_dev {
		stuff..
		void *priv;=09
	};
	stuff..
	int priv_count;
	u8 priv[] __aligned(IIO_DMA_ALIGN) __counted_by(priv_count);
}
with indio_dev->priv =3D iio_dev_opaque->dev?

This cleanups up a few IIO core bits but no impact outside them.
Nice to have those cleanups.

Is there any way to have that internal iio_dev->priv pointer associated with
a __counted_by even though it's pointing elsewhere than a local variable si=
zed
trailing element? =20

struct iio_dev {
	stuff

	u32 count;
	void *priv __counted_by(count);
}
compiles with gcc but without digging further I have no idea if it does any=
thing useful!

Jonathan

>=20
> - Nuno S=C3=A1
>=20
>=20
>=20