Re: [PATCH v2 3/3] staging: pi433: validate max bit_rate based on modulation used

[Dan Carpenter]

On Mon, Jan 17, 2022 at 07:02:20PM +1300, Paulo Miguel Almeida wrote:
> Max bit rate is dependent on which modulation is used. Previous
> validation routine only took into consideration min bit rate which can
> lead a misconfiguration of the rf69 chip causing the packets not to be
> sent/read.
> 
> This patch enhances that input check in set_bit_rate to account for
> modulation values and their respective max bit rate
> 
> Signed-off-by: Paulo Miguel Almeida <paulo.miguel.almeida.rodenas@xxxxxxxxx>
> ---
> Meta-comments:
> 
> In the patchset v1 I kept bit_rate argument's original type as I thought that
> changing it to accomodate values as high as 300kbps couldn't be part of this
> patchset and therefore it should be a separate patchset. 
> 
> Given that kernel test bot compilation/test process 'complained' about the 
> argument's type, I decided to send the v2 patch that addresses the data type
> problem while I work on the patch that will change bit_rate type across
> tx_cfg and rx_cfg as this will require a bit more work.
> 
> Please let me know if anyone dislikes the approach and wants me to deal with it
> in a different way.

I always think that correct static checker warnings should be preserved
instead of papered over and silenced.  Someone went to a lot of work to
create that warning and then we're disabling it for being correct?  It
doesn't make sense.

At the same time, this is a GCC warning so it will break the build.

Instead of applying this patch, I wish you would just make a note of it
in the drivers/staging/pi433/TODO file.  

    "Change (struct pi433_tx_cfg)->bit_rate to be a u32 so that we can
     support bit rates up to 300kbps per the spec."

But you're right that it's complicated to fix this because it's part of
the UAPI.  I think that the UAPI for pi433 is kind of garbage.  No one
like custom ioctls.  It would be better to use sysfs.  Each pi433_tx_cfg
struct member would become it's own file obviously.  We could pick sane
default values where ever possible instead of leaving it all up to the
user.

So my idea is that instead of modifying the custom ioctl then we can
just add a new sysfs file to set the bit_rate and all the other stuff.
Eventually we will delete the ioctl after all the users have updated to
the new userspace.

(I say this with the complete confidence of someone who has never used
pi433 and is not really sure what it is).

regards,
dan carpenter