[PATCH] staging: pi433: modify bit_rate from u16 to u32

[PATCH] staging: pi433: modify bit_rate from u16 to u32

[Guru Mehar Rachaputi]

(struct pi433_tx_cfg)->bit_rate is modified from u16 to u32 to
support bit rates up to 300kbps per the spec

Signed-off-by: Guru Mehar Rachaputi <gurumeharrachaputi@gmail.com>
---
 drivers/staging/pi433/pi433_if.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/staging/pi433/pi433_if.h b/drivers/staging/pi433/pi433_if.h
index 25ee0b77a32c..1f8ffaf02d99 100644
--- a/drivers/staging/pi433/pi433_if.h
+++ b/drivers/staging/pi433/pi433_if.h
@@ -51,7 +51,7 @@ enum option_on_off {
 #define PI433_TX_CFG_IOCTL_NR	0
 struct pi433_tx_cfg {
 	__u32			frequency;
-	__u16			bit_rate;
+	__u32			bit_rate;
 	__u32			dev_frequency;
 	enum modulation		modulation;
 	enum mod_shaping	mod_shaping;
-- 
2.34.1

Re: [PATCH] staging: pi433: modify bit_rate from u16 to u32

[Greg Kroah-Hartman]

On Tue, Jan 31, 2023 at 03:11:38AM +0100, Guru Mehar Rachaputi wrote:
> (struct pi433_tx_cfg)->bit_rate is modified from u16 to u32 to
> support bit rates up to 300kbps per the spec

What spec?

And how can changing the size of a variable that crosses the user/kernel
boundry like this change the bit rate max?

> 
> Signed-off-by: Guru Mehar Rachaputi <gurumeharrachaputi@gmail.com>
> ---
>  drivers/staging/pi433/pi433_if.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/staging/pi433/pi433_if.h b/drivers/staging/pi433/pi433_if.h
> index 25ee0b77a32c..1f8ffaf02d99 100644
> --- a/drivers/staging/pi433/pi433_if.h
> +++ b/drivers/staging/pi433/pi433_if.h
> @@ -51,7 +51,7 @@ enum option_on_off {
>  #define PI433_TX_CFG_IOCTL_NR	0
>  struct pi433_tx_cfg {
>  	__u32			frequency;
> -	__u16			bit_rate;
> +	__u32			bit_rate;
>  	__u32			dev_frequency;
>  	enum modulation		modulation;
>  	enum mod_shaping	mod_shaping;

And didn't you just break existing userspace code?  If not, how?  If so,
how did you test this?

thanks,

greg k-h

Re: [PATCH] staging: pi433: modify bit_rate from u16 to u32

[Guru Mehar Rachaputi]

On Tue, Jan 31, 2023 at 06:22:23AM +0100, Greg Kroah-Hartman wrote:
> On Tue, Jan 31, 2023 at 03:11:38AM +0100, Guru Mehar Rachaputi wrote:
> > (struct pi433_tx_cfg)->bit_rate is modified from u16 to u32 to
> > support bit rates up to 300kbps per the spec
> 
> What spec?
> 
> And how can changing the size of a variable that crosses the user/kernel
> boundry like this change the bit rate max?
>
Honestly, I followed the TODO file suggestion.
> > 
> > Signed-off-by: Guru Mehar Rachaputi <gurumeharrachaputi@gmail.com>
> > ---
> >  drivers/staging/pi433/pi433_if.h | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/drivers/staging/pi433/pi433_if.h b/drivers/staging/pi433/pi433_if.h
> > index 25ee0b77a32c..1f8ffaf02d99 100644
> > --- a/drivers/staging/pi433/pi433_if.h
> > +++ b/drivers/staging/pi433/pi433_if.h
> > @@ -51,7 +51,7 @@ enum option_on_off {
> >  #define PI433_TX_CFG_IOCTL_NR	0
> >  struct pi433_tx_cfg {
> >  	__u32			frequency;
> > -	__u16			bit_rate;
> > +	__u32			bit_rate;
> >  	__u32			dev_frequency;
> >  	enum modulation		modulation;
> >  	enum mod_shaping	mod_shaping;
> 
> And didn't you just break existing userspace code?  If not, how?  If so,
> how did you test this?
>
My apologies, I did not study code. While testing, the probe function of
pi433 driver didn't appear in the lsmod operation. I suspected my
testing was wrong.

> thanks,
> 
> greg k-h

Re: [PATCH] staging: pi433: modify bit_rate from u16 to u32

[Greg Kroah-Hartman]

On Tue, Jan 31, 2023 at 11:19:36AM +0100, Guru Mehar Rachaputi wrote:
> On Tue, Jan 31, 2023 at 06:22:23AM +0100, Greg Kroah-Hartman wrote:
> > On Tue, Jan 31, 2023 at 03:11:38AM +0100, Guru Mehar Rachaputi wrote:
> > > (struct pi433_tx_cfg)->bit_rate is modified from u16 to u32 to
> > > support bit rates up to 300kbps per the spec
> > 
> > What spec?
> > 
> > And how can changing the size of a variable that crosses the user/kernel
> > boundry like this change the bit rate max?
> >
> Honestly, I followed the TODO file suggestion.

Do you have this hardware to test with?

> > > Signed-off-by: Guru Mehar Rachaputi <gurumeharrachaputi@gmail.com>
> > > ---
> > >  drivers/staging/pi433/pi433_if.h | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > 
> > > diff --git a/drivers/staging/pi433/pi433_if.h b/drivers/staging/pi433/pi433_if.h
> > > index 25ee0b77a32c..1f8ffaf02d99 100644
> > > --- a/drivers/staging/pi433/pi433_if.h
> > > +++ b/drivers/staging/pi433/pi433_if.h
> > > @@ -51,7 +51,7 @@ enum option_on_off {
> > >  #define PI433_TX_CFG_IOCTL_NR	0
> > >  struct pi433_tx_cfg {
> > >  	__u32			frequency;
> > > -	__u16			bit_rate;
> > > +	__u32			bit_rate;
> > >  	__u32			dev_frequency;
> > >  	enum modulation		modulation;
> > >  	enum mod_shaping	mod_shaping;
> > 
> > And didn't you just break existing userspace code?  If not, how?  If so,
> > how did you test this?
> >
> My apologies, I did not study code. While testing, the probe function of
> pi433 driver didn't appear in the lsmod operation. I suspected my
> testing was wrong.

You have to test the existing applications that talk to the device to
ensure that this works properly.  This change just breaks the
user/kernel api and doesn't actually change anything to work different
than that :(

thanks,

greg k-h

Re: [PATCH] staging: pi433: modify bit_rate from u16 to u32

[Guru Mehar Rachaputi]

On Tue, Jan 31, 2023 at 11:41:53AM +0100, Greg Kroah-Hartman wrote:
> On Tue, Jan 31, 2023 at 11:19:36AM +0100, Guru Mehar Rachaputi wrote:
> > On Tue, Jan 31, 2023 at 06:22:23AM +0100, Greg Kroah-Hartman wrote:
> > > On Tue, Jan 31, 2023 at 03:11:38AM +0100, Guru Mehar Rachaputi wrote:
> > > > (struct pi433_tx_cfg)->bit_rate is modified from u16 to u32 to
> > > > support bit rates up to 300kbps per the spec
> > > 
> > > What spec?
> > > 
> > > And how can changing the size of a variable that crosses the user/kernel
> > > boundry like this change the bit rate max?
> > >
> > Honestly, I followed the TODO file suggestion.
> 
> Do you have this hardware to test with?
>
Unfortunately, No.

> > > > Signed-off-by: Guru Mehar Rachaputi <gurumeharrachaputi@gmail.com>
> > > > ---
> > > >  drivers/staging/pi433/pi433_if.h | 2 +-
> > > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > > 
> > > > diff --git a/drivers/staging/pi433/pi433_if.h b/drivers/staging/pi433/pi433_if.h
> > > > index 25ee0b77a32c..1f8ffaf02d99 100644
> > > > --- a/drivers/staging/pi433/pi433_if.h
> > > > +++ b/drivers/staging/pi433/pi433_if.h
> > > > @@ -51,7 +51,7 @@ enum option_on_off {
> > > >  #define PI433_TX_CFG_IOCTL_NR	0
> > > >  struct pi433_tx_cfg {
> > > >  	__u32			frequency;
> > > > -	__u16			bit_rate;
> > > > +	__u32			bit_rate;
> > > >  	__u32			dev_frequency;
> > > >  	enum modulation		modulation;
> > > >  	enum mod_shaping	mod_shaping;
> > > 
> > > And didn't you just break existing userspace code?  If not, how?  If so,
> > > how did you test this?
> > >
> > My apologies, I did not study code. While testing, the probe function of
> > pi433 driver didn't appear in the lsmod operation. I suspected my
> > testing was wrong.
> 
> You have to test the existing applications that talk to the device to
> ensure that this works properly.  This change just breaks the
> user/kernel api and doesn't actually change anything to work different
> than that :(
>
I understand. Since I do not have hardware I couldn't test it. I think I
will have to choose my patches wisely.

I would like to know, if it is mentioned in the TODO, why is it
so?

Thanks for the explaination and appreciate taking time for
helping. This was my first patch and I am already learning.

> thanks,
> 
> greg k-h

-- 
Thanks & Regards,
Guru

Re: [PATCH] staging: pi433: modify bit_rate from u16 to u32

[Dan Carpenter]

On Tue, Jan 31, 2023 at 11:52:41AM +0100, Guru Mehar Rachaputi wrote:

> I would like to know, if it is mentioned in the TODO, why is it
> so?
> 

If it were as simple as just changing the type, they would have done
that instead of adding it to the TODO.  :P

But in fairness the TODO entry is sort of vague and useless.  It should
say that this configuration stuff needs to be moved to sysfs instead of
being done through an IOCTL.  The we would need to port the userspace
tools to use sysfs instead of the IOCTL.  Then we would delete the
IOCTL.

https://lore.kernel.org/all/20220118135902.GH1951@kadam/

If you want to send a patch to update the TODO then you can do that.

regards,
dan carpenter

Re: [PATCH] staging: pi433: modify bit_rate from u16 to u32

[Guru Mehar Rachaputi]

On Tue, Jan 31, 2023 at 02:14:12PM +0300, Dan Carpenter wrote:
> On Tue, Jan 31, 2023 at 11:52:41AM +0100, Guru Mehar Rachaputi wrote:
> 
> > I would like to know, if it is mentioned in the TODO, why is it
> > so?
> > 
> 
> If it were as simple as just changing the type, they would have done
> that instead of adding it to the TODO.  :P
> 
> But in fairness the TODO entry is sort of vague and useless.  It should
> say that this configuration stuff needs to be moved to sysfs instead of
> being done through an IOCTL.  The we would need to port the userspace
> tools to use sysfs instead of the IOCTL.  Then we would delete the
> IOCTL.
> 
> https://lore.kernel.org/all/20220118135902.GH1951@kadam/
> 
> If you want to send a patch to update the TODO then you can do that.
> 
> regards,
> dan carpenter
> 
Thanks for your mail.

I have now updated TODO file and submitted as a patch.

-- 
Thanks & Regards,
Guru

RE: [PATCH] staging: pi433: modify bit_rate from u16 to u32

[David Laight]

From: Guru Mehar Rachaputi
> Sent: 31 January 2023 10:20
...
> > >  struct pi433_tx_cfg {
> > >  	__u32			frequency;
> > > -	__u16			bit_rate;
> > > +	__u32			bit_rate;
> > >  	__u32			dev_frequency;
> > >  	enum modulation		modulation;
> > >  	enum mod_shaping	mod_shaping;
> >
> > And didn't you just break existing userspace code?  If not, how?  If so,
> > how did you test this?
> >
> My apologies, I did not study code. While testing, the probe function of
> pi433 driver didn't appear in the lsmod operation. I suspected my
> testing was wrong.

You don't need to study any code, just the structure you changed.

On all architectures (except m68k) there is a two byte pad
after the bit_rate field, this is likely to be filled with
random data from the applications stack.

On little-endian architectures the low byte is in the same place,
so if the 'random data' is zero it will happen to work.

However, on big-endian architectures, the low byte moves so
the value passed (by old applications) will always be wrong.

In reality having a uapi structure with embedded padding should
be banned.
But that would need a compiler attribute to enforce it.

	David

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

Re: [PATCH] staging: pi433: modify bit_rate from u16 to u32

[Dan Carpenter]

On Wed, Feb 01, 2023 at 12:34:50PM +0000, David Laight wrote:
> In reality having a uapi structure with embedded padding should
> be banned.
> But that would need a compiler attribute to enforce it.

It would be simple enough to grep the names of all the UAPI struct and
use pahole to make a list of the existing structs which have holes.
Then re-run the script every week and complain when new holey struct
types are added.

You could do a similar thing with Smatch looking at copy_to/from_user()
struct types.

regards,
dan carpenter

RE: [PATCH] staging: pi433: modify bit_rate from u16 to u32

[David Laight]

From: Dan Carpenter
> Sent: 01 February 2023 13:23
> 
> On Wed, Feb 01, 2023 at 12:34:50PM +0000, David Laight wrote:
> > In reality having a uapi structure with embedded padding should
> > be banned.
> > But that would need a compiler attribute to enforce it.
> 
> It would be simple enough to grep the names of all the UAPI struct and
> use pahole to make a list of the existing structs which have holes.
> Then re-run the script every week and complain when new holey struct
> types are added.
> 
> You could do a similar thing with Smatch looking at copy_to/from_user()
> struct types.

Would it be possible to add aa attribute and check to sparse?

Might persuade people to use it instead of 'packed' for structures
that map defined byte layouts and so mustn't be holey - but are
never actually misaligned in memory.

	David

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

Re: [PATCH] staging: pi433: modify bit_rate from u16 to u32

[Dan Carpenter]

On Thu, Feb 02, 2023 at 10:02:25PM +0000, David Laight wrote:
> From: Dan Carpenter
> > Sent: 01 February 2023 13:23
> > 
> > On Wed, Feb 01, 2023 at 12:34:50PM +0000, David Laight wrote:
> > > In reality having a uapi structure with embedded padding should
> > > be banned.
> > > But that would need a compiler attribute to enforce it.
> > 
> > It would be simple enough to grep the names of all the UAPI struct and
> > use pahole to make a list of the existing structs which have holes.
> > Then re-run the script every week and complain when new holey struct
> > types are added.
> > 
> > You could do a similar thing with Smatch looking at copy_to/from_user()
> > struct types.
> 
> Would it be possible to add aa attribute and check to sparse?
> 
> Might persuade people to use it instead of 'packed' for structures
> that map defined byte layouts and so mustn't be holey - but are
> never actually misaligned in memory.

I mean, it's kind of a before and after thing where we have to allow
all the existing code and complain about new code.  But yeah, we could
use the __user annotation as well.  If we encountered a
`struct oldabi_stat64 __user *statbuf` then complain if oldabi_stat64
had a hole.  At the end, delete all the warnings that existed in the
previously tested kernel.

regards,
dan carpenter