[PATCH] divide by 3*sizeof(u32) when computing array_size

[PATCH] divide by 3*sizeof(u32) when computing array_size

[Salah Triki]

Divide by 3*sizeof(u32) when computing array_size, since stm32_breakinput
has 3 fields of type u32.

Signed-off-by: Salah Triki <salah.triki@gmail.com>
---
 drivers/pwm/pwm-stm32.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/pwm/pwm-stm32.c b/drivers/pwm/pwm-stm32.c
index 794ca5b02968..fb21bc2b2dd6 100644
--- a/drivers/pwm/pwm-stm32.c
+++ b/drivers/pwm/pwm-stm32.c
@@ -544,7 +544,7 @@ static int stm32_pwm_probe_breakinputs(struct stm32_pwm *priv,
 		return -EINVAL;
 
 	priv->num_breakinputs = nb;
-	array_size = nb * sizeof(struct stm32_breakinput) / sizeof(u32);
+	array_size = nb * sizeof(struct stm32_breakinput) / (3 * sizeof(u32));
 	ret = of_property_read_u32_array(np, "st,breakinput",
 					 (u32 *)priv->breakinputs, array_size);
 	if (ret)
-- 
2.25.1

Re: [PATCH] divide by 3*sizeof(u32) when computing array_size

[Philipp Hahn]

Hello,

Disclaimer: I have no idea what 'pwm-stm32' is or does

Am 13.07.21 um 01:19 schrieb Salah Triki:
> Divide by 3*sizeof(u32) when computing array_size, since stm32_breakinput
> has 3 fields of type u32.
...
> --- a/drivers/pwm/pwm-stm32.c
> +++ b/drivers/pwm/pwm-stm32.c
> @@ -544,7 +544,7 @@ static int stm32_pwm_probe_breakinputs(struct stm32_pwm *priv,
>   		return -EINVAL;
>   
>   	priv->num_breakinputs = nb;
> -	array_size = nb * sizeof(struct stm32_breakinput) / sizeof(u32);
> +	array_size = nb * sizeof(struct stm32_breakinput) / (3 * sizeof(u32));

Maybe it's too early in the morning for me, but this does not look right:

> struct stm32_breakinput {
> 	u32 index;
> 	u32 level;
> 	u32 filter;
> };

then "sizeof(struct stm32_breakinput)" == "(3 * sizeof(u32))", which 
would simply make "arrray_site := nb" ?

Philipp

Re: [PATCH] divide by 3*sizeof(u32) when computing array_size

[Uwe Kleine-König]

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

Hello Salah,

On Tue, Jul 13, 2021 at 12:19:10AM +0100, Salah Triki wrote:
> Divide by 3*sizeof(u32) when computing array_size, since stm32_breakinput
> has 3 fields of type u32.
> 
> Signed-off-by: Salah Triki <salah.triki@gmail.com>
> ---
>  drivers/pwm/pwm-stm32.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/pwm/pwm-stm32.c b/drivers/pwm/pwm-stm32.c
> index 794ca5b02968..fb21bc2b2dd6 100644
> --- a/drivers/pwm/pwm-stm32.c
> +++ b/drivers/pwm/pwm-stm32.c
> @@ -544,7 +544,7 @@ static int stm32_pwm_probe_breakinputs(struct stm32_pwm *priv,
>  		return -EINVAL;
>  
>  	priv->num_breakinputs = nb;
> -	array_size = nb * sizeof(struct stm32_breakinput) / sizeof(u32);
> +	array_size = nb * sizeof(struct stm32_breakinput) / (3 * sizeof(u32));
>  	ret = of_property_read_u32_array(np, "st,breakinput",
>  					 (u32 *)priv->breakinputs, array_size);
>  	if (ret)

I agree with Philipp here; this looks strange and needs a better
description.

Looking a bit more in details:

 - priv->breakinputs has type struct stm32_breakinput[MAX_BREAKINPUT]
 - nb is in [0 .. MAX_BREAKINPUT]
 - sizeof(struct stm32_breakinput) == 3 * sizeof(u32)
 - of_property_read_u32_array reads $array_size u32 quantities

so to read $nb members of type stm32_breakinput array_size must be a
multiple of 3 which isn't given any more after your patch. This makes me
believe your suggested change to be wrong.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

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

Re: [PATCH] divide by 3*sizeof(u32) when computing array_size

[Uwe Kleine-König]

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

Hello again,

one more thing: If and when you resend a reworked patch, please start
the Subject with

	pwm: stm32:

Thanks
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

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

Re: [PATCH] divide by 3*sizeof(u32) when computing array_size

[Russell King (Oracle)]

On Tue, Jul 13, 2021 at 08:30:53AM +0200, Uwe Kleine-König wrote:
> Hello Salah,
> 
> On Tue, Jul 13, 2021 at 12:19:10AM +0100, Salah Triki wrote:
> > Divide by 3*sizeof(u32) when computing array_size, since stm32_breakinput
> > has 3 fields of type u32.
> > 
> > Signed-off-by: Salah Triki <salah.triki@gmail.com>
> > ---
> >  drivers/pwm/pwm-stm32.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/drivers/pwm/pwm-stm32.c b/drivers/pwm/pwm-stm32.c
> > index 794ca5b02968..fb21bc2b2dd6 100644
> > --- a/drivers/pwm/pwm-stm32.c
> > +++ b/drivers/pwm/pwm-stm32.c
> > @@ -544,7 +544,7 @@ static int stm32_pwm_probe_breakinputs(struct stm32_pwm *priv,
> >  		return -EINVAL;
> >  
> >  	priv->num_breakinputs = nb;
> > -	array_size = nb * sizeof(struct stm32_breakinput) / sizeof(u32);
> > +	array_size = nb * sizeof(struct stm32_breakinput) / (3 * sizeof(u32));
> >  	ret = of_property_read_u32_array(np, "st,breakinput",
> >  					 (u32 *)priv->breakinputs, array_size);
> >  	if (ret)
> 
> I agree with Philipp here; this looks strange and needs a better
> description.
> 
> Looking a bit more in details:
> 
>  - priv->breakinputs has type struct stm32_breakinput[MAX_BREAKINPUT]
>  - nb is in [0 .. MAX_BREAKINPUT]
>  - sizeof(struct stm32_breakinput) == 3 * sizeof(u32)
>  - of_property_read_u32_array reads $array_size u32 quantities
> 
> so to read $nb members of type stm32_breakinput array_size must be a
> multiple of 3 which isn't given any more after your patch. This makes me
> believe your suggested change to be wrong.

I concur with your analysis. "array_size" is the number of u32 values
to read from DT. It is not the number of entries in priv->breakinputs.

I would also note that the code relies on there being no padding in
struct stm32_breakinput - it should be noted that a strict
interpretation of the C standard allows padding to be added anywhere
to a structure - at the start, end or between members.

Some further thoughts... DT is effectively an interface (we maintain
definitions of what we expect.) The way the code is structured,
"struct stm32_breakinput" defines that interface. Maybe this should
be commented, and maybe there should be a build time assert that
"sizeof(struct stm32_breakinput)" is "3 * sizeof(u32)" since the
code is relying on that property?

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!

RE: [PATCH] divide by 3*sizeof(u32) when computing array_size

[David Laight]

From: Russell King
> Sent: 13 July 2021 10:20
....
> I would also note that the code relies on there being no padding in
> struct stm32_breakinput - it should be noted that a strict
> interpretation of the C standard allows padding to be added anywhere
> to a structure - at the start, end or between members.

I'm pretty certain I remember that padding before the first member
isn't allowed.

In any case the kernel generally assumes there is no extra padding.
(eg for structures that map hardware registers.)

For big structures it is worth adding a compile-time check of
the structure size - but not really for three u32.

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)

Re: [PATCH] divide by 3*sizeof(u32) when computing array_size

[Russell King (Oracle)]

On Tue, Jul 13, 2021 at 11:07:00AM +0000, David Laight wrote:
> From: Russell King
> > Sent: 13 July 2021 10:20
> ....
> > I would also note that the code relies on there being no padding in
> > struct stm32_breakinput - it should be noted that a strict
> > interpretation of the C standard allows padding to be added anywhere
> > to a structure - at the start, end or between members.
> 
> I'm pretty certain I remember that padding before the first member
> isn't allowed.

You may be right there.

> In any case the kernel generally assumes there is no extra padding.
> (eg for structures that map hardware registers.)

That's incorrect. Places where we care either generally end up with
__packed or are carefully layed out to ensure members are naturally
aligned to reduce the likelyhood of it. 32-bit OABI ARM has been
particularly "fun" in this respect.

> For big structures it is worth adding a compile-time check of
> the structure size - but not really for three u32.

Sorry, structure size has absolutely nothing to do with whether it's
a good idea to have a compile-time check. The deciding factor is
whether the code relies on some property such as it being a certain
size. Such as in this exact case. If you grep for "BUILD_BUG_ON.*sizeof"
in fs/ for example, this illustrates the point rather well.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!

RE: [PATCH] divide by 3*sizeof(u32) when computing array_size

[David Laight]

From: Russell King
> Sent: 13 July 2021 12:23
> 
> On Tue, Jul 13, 2021 at 11:07:00AM +0000, David Laight wrote:
> > From: Russell King
> > > Sent: 13 July 2021 10:20
> > ....
> > > I would also note that the code relies on there being no padding in
> > > struct stm32_breakinput - it should be noted that a strict
> > > interpretation of the C standard allows padding to be added anywhere
> > > to a structure - at the start, end or between members.
> >
> > I'm pretty certain I remember that padding before the first member
> > isn't allowed.
> 
> You may be right there.
> 
> > In any case the kernel generally assumes there is no extra padding.
> > (eg for structures that map hardware registers.)
> 
> That's incorrect. Places where we care either generally end up with
> __packed or are carefully layed out to ensure members are naturally
> aligned to reduce the likelyhood of it. 32-bit OABI ARM has been
> particularly "fun" in this respect.

I did say 'extra padding'.
Ensuring everything is naturally aligned is best - shame the standards
bodies don't do that - just look at the SCTP socket options.

Adding __packed is right sometimes, but it isn't without cost
and is probably wrong for anything hardware related.
Definitely useful on structure members to remove the padding
before that specific member (eg for 64bit in x86 compat code).
But marking a structure __packed is usually wrong (or bad).

> > For big structures it is worth adding a compile-time check of
> > the structure size - but not really for three u32.
> 
> Sorry, structure size has absolutely nothing to do with whether it's
> a good idea to have a compile-time check. The deciding factor is
> whether the code relies on some property such as it being a certain
> size. Such as in this exact case. If you grep for "BUILD_BUG_ON.*sizeof"
> in fs/ for example, this illustrates the point rather well.

I'd not bother if the size is obviously going to be correct.

I did get some odd bugs a few years ago from a compiler that aligned
all structures on 4-byte boundaries.
I had to change a structure of two u16 into an array :-)

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)

Re: [PATCH] divide by 3*sizeof(u32) when computing array_size

[Russell King (Oracle)]

On Tue, Jul 13, 2021 at 12:20:26PM +0000, David Laight wrote:
> > > For big structures it is worth adding a compile-time check of
> > > the structure size - but not really for three u32.
> > 
> > Sorry, structure size has absolutely nothing to do with whether it's
> > a good idea to have a compile-time check. The deciding factor is
> > whether the code relies on some property such as it being a certain
> > size. Such as in this exact case. If you grep for "BUILD_BUG_ON.*sizeof"
> > in fs/ for example, this illustrates the point rather well.
> 
> I'd not bother if the size is obviously going to be correct.

That's fine if you assume that the structure isn't going to be changed.
In this case, you can't do that - the structure looks to be a driver
internal structure. It certainly doesn't look like an interface to
anything that matters.

The code as written relies on the assumption that an array of
struct stm32_breakinput can be directly mapped to an array of u32,
where every third element of the u32 array falls on the first member
of each stm32_breakinput member. That is a _significant_ assumption
that the code _should_ be checking for.

> I did get some odd bugs a few years ago from a compiler that aligned
> all structures on 4-byte boundaries.
> I had to change a structure of two u16 into an array :-)

ARM OABI will do exactly that.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!