[PATCH 0/3] staging: pi433: validate min/max bit rate settings

[PATCH 0/3] staging: pi433: validate min/max bit rate settings

[Paulo Miguel Almeida]

RF69 chip supports different bit rate settings depending on which frequency
modulation is used. Failing to correctly validate bit rate can lead to a 
silently failure and cause packets not to be read/sent.

This series changes the order in which the rf69 chip is configured and adds
max bit rate validation.

This series depend on these patches as they change the same set of files:

- https://lore.kernel.org/lkml/20220108212728.GA7784@mail.google.com/
- https://lore.kernel.org/lkml/20220114221643.GA7843@mail.google.com/

Paulo Miguel Almeida (3):
  staging: pi433: fix validation for min bit rate supported by the
    device
  staging: pi433: change order in which driver config the rf69 chip
  staging: pi433: validate max bit_rate based on modulation used

 drivers/staging/pi433/pi433_if.c |  8 ++++----
 drivers/staging/pi433/rf69.c     | 14 +++++++++++---
 2 files changed, 15 insertions(+), 7 deletions(-)

-- 
2.25.4

[PATCH 1/3] staging: pi433: fix validation for min bit rate supported by the device

[Paulo Miguel Almeida]

rf69 datasheets establishes that the minimum supported bit rate is
1.2 kbps regardless of modulation.

this patch replaces the errouneous validation with the correct value

Signed-off-by: Paulo Miguel Almeida <paulo.miguel.almeida.rodenas@gmail.com>
---
 drivers/staging/pi433/rf69.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/staging/pi433/rf69.c b/drivers/staging/pi433/rf69.c
index ee8c81d164e1..f4ac17adcd83 100644
--- a/drivers/staging/pi433/rf69.c
+++ b/drivers/staging/pi433/rf69.c
@@ -226,14 +226,12 @@ int rf69_set_modulation_shaping(struct spi_device *spi,
 int rf69_set_bit_rate(struct spi_device *spi, u16 bit_rate)
 {
 	int retval;
-	u32 bit_rate_min;
 	u32 bit_rate_reg;
 	u8 msb;
 	u8 lsb;
 
 	// check input value
-	bit_rate_min = F_OSC / 8388608; // 8388608 = 2^23;
-	if (bit_rate < bit_rate_min) {
+	if (bit_rate < 1200) {
 		dev_dbg(&spi->dev, "setBitRate: illegal input param");
 		return -EINVAL;
 	}
-- 
2.25.4

[PATCH 2/3] staging: pi433: change order in which driver config the rf69 chip

[Paulo Miguel Almeida]

There is an explicitly dependency between modulation and bit rate
configurations. To ensure proper validation of input value for the
set_bit_rate routine, we must ensure that modulation has been set
before.

This patch ensure that set_modulation is always called before
set_bit_rate for both RX and TX routines

Signed-off-by: Paulo Miguel Almeida <paulo.miguel.almeida.rodenas@gmail.com>
---
 drivers/staging/pi433/pi433_if.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/staging/pi433/pi433_if.c b/drivers/staging/pi433/pi433_if.c
index f9f86e2c44a9..17ff51f6a9da 100644
--- a/drivers/staging/pi433/pi433_if.c
+++ b/drivers/staging/pi433/pi433_if.c
@@ -167,10 +167,10 @@ rf69_set_rx_cfg(struct pi433_device *dev, struct pi433_rx_cfg *rx_cfg)
 	ret = rf69_set_frequency(dev->spi, rx_cfg->frequency);
 	if (ret < 0)
 		return ret;
-	ret = rf69_set_bit_rate(dev->spi, rx_cfg->bit_rate);
+	ret = rf69_set_modulation(dev->spi, rx_cfg->modulation);
 	if (ret < 0)
 		return ret;
-	ret = rf69_set_modulation(dev->spi, rx_cfg->modulation);
+	ret = rf69_set_bit_rate(dev->spi, rx_cfg->bit_rate);
 	if (ret < 0)
 		return ret;
 	ret = rf69_set_antenna_impedance(dev->spi, rx_cfg->antenna_impedance);
@@ -290,10 +290,10 @@ rf69_set_tx_cfg(struct pi433_device *dev, struct pi433_tx_cfg *tx_cfg)
 	ret = rf69_set_frequency(dev->spi, tx_cfg->frequency);
 	if (ret < 0)
 		return ret;
-	ret = rf69_set_bit_rate(dev->spi, tx_cfg->bit_rate);
+	ret = rf69_set_modulation(dev->spi, tx_cfg->modulation);
 	if (ret < 0)
 		return ret;
-	ret = rf69_set_modulation(dev->spi, tx_cfg->modulation);
+	ret = rf69_set_bit_rate(dev->spi, tx_cfg->bit_rate);
 	if (ret < 0)
 		return ret;
 	ret = rf69_set_deviation(dev->spi, tx_cfg->dev_frequency);
-- 
2.25.4

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

[Paulo Miguel Almeida]

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@gmail.com>
---
 drivers/staging/pi433/rf69.c | 12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

diff --git a/drivers/staging/pi433/rf69.c b/drivers/staging/pi433/rf69.c
index f4ac17adcd83..70ffe291b0e6 100644
--- a/drivers/staging/pi433/rf69.c
+++ b/drivers/staging/pi433/rf69.c
@@ -229,9 +229,19 @@ int rf69_set_bit_rate(struct spi_device *spi, u16 bit_rate)
 	u32 bit_rate_reg;
 	u8 msb;
 	u8 lsb;
+	enum modulation mod;
+
+	// check if modulation is configured
+	mod = rf69_get_modulation(spi);
+	if (mod == UNDEF) {
+		dev_dbg(&spi->dev, "setBitRate: modulation is undefined");
+		return -EINVAL;
+	}
 
 	// check input value
-	if (bit_rate < 1200) {
+	if (bit_rate < 1200 ||
+	    (mod == FSK && bit_rate > 300000) ||
+	    (mod == OOK && bit_rate > 32768)) {
 		dev_dbg(&spi->dev, "setBitRate: illegal input param");
 		return -EINVAL;
 	}
-- 
2.25.4

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

[kernel test robot]

Hi Paulo,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on staging/staging-testing]

url:    https://github.com/0day-ci/linux/commits/Paulo-Miguel-Almeida/staging-pi433-validate-min-max-bit-rate-settings/20220116-082432
base:   https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/staging.git fa783154524a71ab74e293cd8251155e5971952b
config: i386-randconfig-a002 (https://download.01.org/0day-ci/archive/20220116/202201162116.8r1xEw9q-lkp@intel.com/config)
compiler: clang version 14.0.0 (https://github.com/llvm/llvm-project c63a3175c2947e8c1a2d3bbe16a8586600705c54)
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/0day-ci/linux/commit/4bbc9e444fd3bab38d93ae4261102db64385a26c
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Paulo-Miguel-Almeida/staging-pi433-validate-min-max-bit-rate-settings/20220116-082432
        git checkout 4bbc9e444fd3bab38d93ae4261102db64385a26c
        # save the config file to linux build tree
        mkdir build_dir
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=i386 SHELL=/bin/bash drivers/staging/pi433/

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All warnings (new ones prefixed by >>):

>> drivers/staging/pi433/rf69.c:238:30: warning: result of comparison of constant 300000 with expression of type 'u16' (aka 'unsigned short') is always false [-Wtautological-constant-out-of-range-compare]
               (mod == FSK && bit_rate > 300000) ||
                              ~~~~~~~~ ^ ~~~~~~
   1 warning generated.


vim +238 drivers/staging/pi433/rf69.c

   220	
   221	int rf69_set_bit_rate(struct spi_device *spi, u16 bit_rate)
   222	{
   223		int retval;
   224		u32 bit_rate_reg;
   225		u8 msb;
   226		u8 lsb;
   227		enum modulation mod;
   228	
   229		// check if modulation is configured
   230		mod = rf69_get_modulation(spi);
   231		if (mod == UNDEF) {
   232			dev_dbg(&spi->dev, "setBitRate: modulation is undefined");
   233			return -EINVAL;
   234		}
   235	
   236		// check input value
   237		if (bit_rate < 1200 ||
 > 238		    (mod == FSK && bit_rate > 300000) ||
   239		    (mod == OOK && bit_rate > 32768)) {
   240			dev_dbg(&spi->dev, "setBitRate: illegal input param");
   241			return -EINVAL;
   242		}
   243	
   244		// calculate reg settings
   245		bit_rate_reg = (F_OSC / bit_rate);
   246	
   247		msb = (bit_rate_reg & 0xff00) >> 8;
   248		lsb = (bit_rate_reg & 0xff);
   249	
   250		// transmit to RF 69
   251		retval = rf69_write_reg(spi, REG_BITRATE_MSB, msb);
   252		if (retval)
   253			return retval;
   254		retval = rf69_write_reg(spi, REG_BITRATE_LSB, lsb);
   255		if (retval)
   256			return retval;
   257	
   258		return 0;
   259	}
   260	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

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

[kernel test robot]

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

Hi Paulo,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on staging/staging-testing]

url:    https://github.com/0day-ci/linux/commits/Paulo-Miguel-Almeida/staging-pi433-validate-min-max-bit-rate-settings/20220116-082432
base:   https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/staging.git fa783154524a71ab74e293cd8251155e5971952b
config: i386-randconfig-a002 (https://download.01.org/0day-ci/archive/20220116/202201162116.8r1xEw9q-lkp(a)intel.com/config)
compiler: clang version 14.0.0 (https://github.com/llvm/llvm-project c63a3175c2947e8c1a2d3bbe16a8586600705c54)
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/0day-ci/linux/commit/4bbc9e444fd3bab38d93ae4261102db64385a26c
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Paulo-Miguel-Almeida/staging-pi433-validate-min-max-bit-rate-settings/20220116-082432
        git checkout 4bbc9e444fd3bab38d93ae4261102db64385a26c
        # save the config file to linux build tree
        mkdir build_dir
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=i386 SHELL=/bin/bash drivers/staging/pi433/

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All warnings (new ones prefixed by >>):

>> drivers/staging/pi433/rf69.c:238:30: warning: result of comparison of constant 300000 with expression of type 'u16' (aka 'unsigned short') is always false [-Wtautological-constant-out-of-range-compare]
               (mod == FSK && bit_rate > 300000) ||
                              ~~~~~~~~ ^ ~~~~~~
   1 warning generated.


vim +238 drivers/staging/pi433/rf69.c

   220	
   221	int rf69_set_bit_rate(struct spi_device *spi, u16 bit_rate)
   222	{
   223		int retval;
   224		u32 bit_rate_reg;
   225		u8 msb;
   226		u8 lsb;
   227		enum modulation mod;
   228	
   229		// check if modulation is configured
   230		mod = rf69_get_modulation(spi);
   231		if (mod == UNDEF) {
   232			dev_dbg(&spi->dev, "setBitRate: modulation is undefined");
   233			return -EINVAL;
   234		}
   235	
   236		// check input value
   237		if (bit_rate < 1200 ||
 > 238		    (mod == FSK && bit_rate > 300000) ||
   239		    (mod == OOK && bit_rate > 32768)) {
   240			dev_dbg(&spi->dev, "setBitRate: illegal input param");
   241			return -EINVAL;
   242		}
   243	
   244		// calculate reg settings
   245		bit_rate_reg = (F_OSC / bit_rate);
   246	
   247		msb = (bit_rate_reg & 0xff00) >> 8;
   248		lsb = (bit_rate_reg & 0xff);
   249	
   250		// transmit to RF 69
   251		retval = rf69_write_reg(spi, REG_BITRATE_MSB, msb);
   252		if (retval)
   253			return retval;
   254		retval = rf69_write_reg(spi, REG_BITRATE_LSB, lsb);
   255		if (retval)
   256			return retval;
   257	
   258		return 0;
   259	}
   260	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all(a)lists.01.org

[PATCH v2 0/3] staging: pi433: validate min/max bit rate settings

[Paulo Miguel Almeida]

RF69 chip supports different bit rate settings depending on which frequency
modulation is used. Failing to correctly validate bit rate can lead to a 
silent failure and cause packets not to be read/sent.

This series change the order in which the rf69 chip is configured and add
max bit rate validation.

Patch dependency:

This series depend on these patches as they change the same set of files:

- https://lore.kernel.org/lkml/20220108212728.GA7784@mail.google.com/
- https://lore.kernel.org/lkml/20220114221643.GA7843@mail.google.com/

Changelog:

v2: change bitrate variable type to u32 as pointed by kernel test bot
v1: https://lore.kernel.org/lkml/20220116001726.GA22699@mail.google.com/

Paulo Miguel Almeida (3):
  staging: pi433: fix validation for min bit rate supported by the
    device
  staging: pi433: change order in which driver config the rf69 chip
  staging: pi433: validate max bit_rate based on modulation used

 drivers/staging/pi433/pi433_if.c |  8 ++++----
 drivers/staging/pi433/rf69.c     | 16 ++++++++++++----
 drivers/staging/pi433/rf69.h     |  2 +-
 3 files changed, 17 insertions(+), 9 deletions(-)

-- 
2.25.4

[PATCH v2 1/3] staging: pi433: fix validation for min bit rate supported by the device

[Paulo Miguel Almeida]

rf69 datasheets establishes that the minimum supported bit rate is
1.2 kbps regardless of modulation.

this patch replaces the errouneous validation with the correct value

Signed-off-by: Paulo Miguel Almeida <paulo.miguel.almeida.rodenas@gmail.com>
---
 drivers/staging/pi433/rf69.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/staging/pi433/rf69.c b/drivers/staging/pi433/rf69.c
index ee8c81d164e1..f4ac17adcd83 100644
--- a/drivers/staging/pi433/rf69.c
+++ b/drivers/staging/pi433/rf69.c
@@ -226,14 +226,12 @@ int rf69_set_modulation_shaping(struct spi_device *spi,
 int rf69_set_bit_rate(struct spi_device *spi, u16 bit_rate)
 {
 	int retval;
-	u32 bit_rate_min;
 	u32 bit_rate_reg;
 	u8 msb;
 	u8 lsb;
 
 	// check input value
-	bit_rate_min = F_OSC / 8388608; // 8388608 = 2^23;
-	if (bit_rate < bit_rate_min) {
+	if (bit_rate < 1200) {
 		dev_dbg(&spi->dev, "setBitRate: illegal input param");
 		return -EINVAL;
 	}
-- 
2.25.4

[PATCH v2 2/3] staging: pi433: change order in which driver config the rf69 chip

[Paulo Miguel Almeida]

There is an explicit dependency between modulation and bit rate
configurations. To ensure proper validation of input value for the
set_bit_rate routine, we must ensure that modulation has been set
before.

This patch ensures that set_modulation is always called before
set_bit_rate for both RX and TX routines

Signed-off-by: Paulo Miguel Almeida <paulo.miguel.almeida.rodenas@gmail.com>
---
 drivers/staging/pi433/pi433_if.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/staging/pi433/pi433_if.c b/drivers/staging/pi433/pi433_if.c
index f9f86e2c44a9..17ff51f6a9da 100644
--- a/drivers/staging/pi433/pi433_if.c
+++ b/drivers/staging/pi433/pi433_if.c
@@ -167,10 +167,10 @@ rf69_set_rx_cfg(struct pi433_device *dev, struct pi433_rx_cfg *rx_cfg)
 	ret = rf69_set_frequency(dev->spi, rx_cfg->frequency);
 	if (ret < 0)
 		return ret;
-	ret = rf69_set_bit_rate(dev->spi, rx_cfg->bit_rate);
+	ret = rf69_set_modulation(dev->spi, rx_cfg->modulation);
 	if (ret < 0)
 		return ret;
-	ret = rf69_set_modulation(dev->spi, rx_cfg->modulation);
+	ret = rf69_set_bit_rate(dev->spi, rx_cfg->bit_rate);
 	if (ret < 0)
 		return ret;
 	ret = rf69_set_antenna_impedance(dev->spi, rx_cfg->antenna_impedance);
@@ -290,10 +290,10 @@ rf69_set_tx_cfg(struct pi433_device *dev, struct pi433_tx_cfg *tx_cfg)
 	ret = rf69_set_frequency(dev->spi, tx_cfg->frequency);
 	if (ret < 0)
 		return ret;
-	ret = rf69_set_bit_rate(dev->spi, tx_cfg->bit_rate);
+	ret = rf69_set_modulation(dev->spi, tx_cfg->modulation);
 	if (ret < 0)
 		return ret;
-	ret = rf69_set_modulation(dev->spi, tx_cfg->modulation);
+	ret = rf69_set_bit_rate(dev->spi, tx_cfg->bit_rate);
 	if (ret < 0)
 		return ret;
 	ret = rf69_set_deviation(dev->spi, tx_cfg->dev_frequency);
-- 
2.25.4

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

[Paulo Miguel Almeida]

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@gmail.com>
---
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.
---
 drivers/staging/pi433/rf69.c | 14 ++++++++++++--
 drivers/staging/pi433/rf69.h |  2 +-
 2 files changed, 13 insertions(+), 3 deletions(-)

diff --git a/drivers/staging/pi433/rf69.c b/drivers/staging/pi433/rf69.c
index f4ac17adcd83..adba69b8365e 100644
--- a/drivers/staging/pi433/rf69.c
+++ b/drivers/staging/pi433/rf69.c
@@ -223,15 +223,25 @@ int rf69_set_modulation_shaping(struct spi_device *spi,
 	}
 }
 
-int rf69_set_bit_rate(struct spi_device *spi, u16 bit_rate)
+int rf69_set_bit_rate(struct spi_device *spi, u32 bit_rate)
 {
 	int retval;
 	u32 bit_rate_reg;
 	u8 msb;
 	u8 lsb;
+	enum modulation mod;
+
+	// check if modulation is configured
+	mod = rf69_get_modulation(spi);
+	if (mod == UNDEF) {
+		dev_dbg(&spi->dev, "setBitRate: modulation is undefined");
+		return -EINVAL;
+	}
 
 	// check input value
-	if (bit_rate < 1200) {
+	if (bit_rate < 1200 ||
+	    (mod == FSK && bit_rate > 300000) ||
+	    (mod == OOK && bit_rate > 32768)) {
 		dev_dbg(&spi->dev, "setBitRate: illegal input param");
 		return -EINVAL;
 	}
diff --git a/drivers/staging/pi433/rf69.h b/drivers/staging/pi433/rf69.h
index c25942f142a6..3b8184155326 100644
--- a/drivers/staging/pi433/rf69.h
+++ b/drivers/staging/pi433/rf69.h
@@ -23,7 +23,7 @@ int rf69_set_data_mode(struct spi_device *spi, u8 data_mode);
 int rf69_set_modulation(struct spi_device *spi, enum modulation modulation);
 int rf69_set_modulation_shaping(struct spi_device *spi,
 				enum mod_shaping mod_shaping);
-int rf69_set_bit_rate(struct spi_device *spi, u16 bit_rate);
+int rf69_set_bit_rate(struct spi_device *spi, u32 bit_rate);
 int rf69_set_deviation(struct spi_device *spi, u32 deviation);
 int rf69_set_frequency(struct spi_device *spi, u32 frequency);
 int rf69_enable_amplifier(struct spi_device *spi, u8 amplifier_mask);
-- 
2.25.4

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@gmail.com>
> ---
> 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

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

[Paulo Miguel Almeida]

On Tue, Jan 18, 2022 at 04:59:02PM +0300, Dan Carpenter wrote:
> 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."

thanks for taking the time to review this patchset.

It makes sense to me. So essentially for this version of the patchset I
would only validate OOK max bit rate as it fits within the u16 boundary
of the current implementation, right?

Something akin to this:

// check input value
if (bit_rate < 1200 || (mod == OOK && bit_rate > 32768)) {
	dev_dbg(&spi->dev, "setBitRate: illegal input param");
	return -EINVAL;
}

> 
> 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.  
> ...
> 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.
> 

Using sysfs or even possibly configfs was one of the things I had in
mind too. 

I started a similar discussion on how to change/remove ioctl:
https://lore.kernel.org/kernelnewbies/YeVoAlv0ubKrmckV@kroah.com/T/#t

The tldr;:

If there is a userspace tool that interfaces with char
device and we can change it at the same time as the kernel code then we 
can have a bit more flexibility on how to send/retrieve values to/from 
the driver. If not then we would be required to keep the original ioctl
compatibility even if we move to a different approach due to the
impossibility of knowing whether or not someone is relying on that. :(

Everything leads me to believe that there isn't such userspace tool /
lib so I emailed the original author just to be 100% sure that this is
the case.

thanks,

Paulo Almeida

[PATCH v3 0/3] staging: pi433: validate min/max bit rate settings

[Paulo Miguel Almeida]

RF69 chip supports different bit rate settings depending on which frequency
modulation is used. Failing to correctly validate bit rate can lead to a 
silent failure and cause packets not to be read/sent.

This series change the order in which the rf69 chip is configured and add
max bit rate validation.

Patch dependency:

This series depend on these patches as they change the same set of files:

- https://lore.kernel.org/lkml/20220108212728.GA7784@mail.google.com/
- https://lore.kernel.org/lkml/20220114221643.GA7843@mail.google.com/

Changelog:

v3: revert bitrate variable data type change. Req: Dan Carpenter
v2: change bitrate variable type to u32 as pointed by kernel test bot
v1: https://lore.kernel.org/lkml/20220116001726.GA22699@mail.google.com/

Paulo Miguel Almeida (3):
  staging: pi433: fix validation for min bit rate supported by the
    device
  staging: pi433: change order in which driver config the rf69 chip
  staging: pi433: validate max bit_rate based on modulation used

 drivers/staging/pi433/TODO       |  2 ++
 drivers/staging/pi433/pi433_if.c |  8 ++++----
 drivers/staging/pi433/rf69.c     | 12 +++++++++---
 3 files changed, 15 insertions(+), 7 deletions(-)

-- 
2.25.4

[PATCH v3 1/3] staging: pi433: fix validation for min bit rate supported by the device

[Paulo Miguel Almeida]

rf69 datasheets establishes that the minimum supported bit rate is
1.2 kbps regardless of modulation.

this patch replaces the errouneous validation with the correct value

Signed-off-by: Paulo Miguel Almeida <paulo.miguel.almeida.rodenas@gmail.com>
---
 drivers/staging/pi433/rf69.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/staging/pi433/rf69.c b/drivers/staging/pi433/rf69.c
index ee8c81d164e1..f4ac17adcd83 100644
--- a/drivers/staging/pi433/rf69.c
+++ b/drivers/staging/pi433/rf69.c
@@ -226,14 +226,12 @@ int rf69_set_modulation_shaping(struct spi_device *spi,
 int rf69_set_bit_rate(struct spi_device *spi, u16 bit_rate)
 {
 	int retval;
-	u32 bit_rate_min;
 	u32 bit_rate_reg;
 	u8 msb;
 	u8 lsb;
 
 	// check input value
-	bit_rate_min = F_OSC / 8388608; // 8388608 = 2^23;
-	if (bit_rate < bit_rate_min) {
+	if (bit_rate < 1200) {
 		dev_dbg(&spi->dev, "setBitRate: illegal input param");
 		return -EINVAL;
 	}
-- 
2.25.4

[PATCH v3 2/3] staging: pi433: change order in which driver config the rf69 chip

[Paulo Miguel Almeida]

There is an explicit dependency between modulation and bit rate
configurations. To ensure proper validation of input value for the
set_bit_rate routine, we must ensure that modulation has been set
before.

This patch ensures that set_modulation is always called before
set_bit_rate for both RX and TX routines

Signed-off-by: Paulo Miguel Almeida <paulo.miguel.almeida.rodenas@gmail.com>
---
 drivers/staging/pi433/pi433_if.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/staging/pi433/pi433_if.c b/drivers/staging/pi433/pi433_if.c
index f9f86e2c44a9..17ff51f6a9da 100644
--- a/drivers/staging/pi433/pi433_if.c
+++ b/drivers/staging/pi433/pi433_if.c
@@ -167,10 +167,10 @@ rf69_set_rx_cfg(struct pi433_device *dev, struct pi433_rx_cfg *rx_cfg)
 	ret = rf69_set_frequency(dev->spi, rx_cfg->frequency);
 	if (ret < 0)
 		return ret;
-	ret = rf69_set_bit_rate(dev->spi, rx_cfg->bit_rate);
+	ret = rf69_set_modulation(dev->spi, rx_cfg->modulation);
 	if (ret < 0)
 		return ret;
-	ret = rf69_set_modulation(dev->spi, rx_cfg->modulation);
+	ret = rf69_set_bit_rate(dev->spi, rx_cfg->bit_rate);
 	if (ret < 0)
 		return ret;
 	ret = rf69_set_antenna_impedance(dev->spi, rx_cfg->antenna_impedance);
@@ -290,10 +290,10 @@ rf69_set_tx_cfg(struct pi433_device *dev, struct pi433_tx_cfg *tx_cfg)
 	ret = rf69_set_frequency(dev->spi, tx_cfg->frequency);
 	if (ret < 0)
 		return ret;
-	ret = rf69_set_bit_rate(dev->spi, tx_cfg->bit_rate);
+	ret = rf69_set_modulation(dev->spi, tx_cfg->modulation);
 	if (ret < 0)
 		return ret;
-	ret = rf69_set_modulation(dev->spi, tx_cfg->modulation);
+	ret = rf69_set_bit_rate(dev->spi, tx_cfg->bit_rate);
 	if (ret < 0)
 		return ret;
 	ret = rf69_set_deviation(dev->spi, tx_cfg->dev_frequency);
-- 
2.25.4

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

[Paulo Miguel Almeida]

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@gmail.com>
---
 drivers/staging/pi433/TODO   |  2 ++
 drivers/staging/pi433/rf69.c | 10 +++++++++-
 2 files changed, 11 insertions(+), 1 deletion(-)

diff --git a/drivers/staging/pi433/TODO b/drivers/staging/pi433/TODO
index 63a40bfcc67e..b9e6c01a02e0 100644
--- a/drivers/staging/pi433/TODO
+++ b/drivers/staging/pi433/TODO
@@ -3,3 +3,5 @@
 * currently the code introduces new IOCTLs. I'm afraid this is a bad idea.
   -> Replace this with another interface, hints are welcome!
 * Some missing data (marked with ###) needs to be added in the documentation
+* Change (struct pi433_tx_cfg)->bit_rate to be a u32 so that we can support
+    bit rates up to 300kbps per the spec.
diff --git a/drivers/staging/pi433/rf69.c b/drivers/staging/pi433/rf69.c
index f4ac17adcd83..d60514a840c2 100644
--- a/drivers/staging/pi433/rf69.c
+++ b/drivers/staging/pi433/rf69.c
@@ -229,9 +229,17 @@ int rf69_set_bit_rate(struct spi_device *spi, u16 bit_rate)
 	u32 bit_rate_reg;
 	u8 msb;
 	u8 lsb;
+	enum modulation mod;
+
+	// check if modulation is configured
+	mod = rf69_get_modulation(spi);
+	if (mod == UNDEF) {
+		dev_dbg(&spi->dev, "setBitRate: modulation is undefined");
+		return -EINVAL;
+	}
 
 	// check input value
-	if (bit_rate < 1200) {
+	if (bit_rate < 1200 || (mod == OOK && bit_rate > 32768)) {
 		dev_dbg(&spi->dev, "setBitRate: illegal input param");
 		return -EINVAL;
 	}
-- 
2.25.4

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

[Dan Carpenter]

On Wed, Jan 19, 2022 at 08:34:22AM +1300, Paulo Miguel Almeida wrote:
> On Tue, Jan 18, 2022 at 04:59:02PM +0300, Dan Carpenter wrote:
> > 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."
> 
> thanks for taking the time to review this patchset.
> 
> It makes sense to me. So essentially for this version of the patchset I
> would only validate OOK max bit rate as it fits within the u16 boundary
> of the current implementation, right?
> 
> Something akin to this:
> 
> // check input value
> if (bit_rate < 1200 || (mod == OOK && bit_rate > 32768)) {
> 	dev_dbg(&spi->dev, "setBitRate: illegal input param");
> 	return -EINVAL;
> }

Actually, on further reflection, your patch is fine.  But please put a
note in the TODO that ->bit_rate needs to be changed to a u32.  Just
resend this patch series with that additional note in the TODO.

> 
> > 
> > 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.  
> > ...
> > 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.
> > 
> 
> Using sysfs or even possibly configfs was one of the things I had in
> mind too. 
> 
> I started a similar discussion on how to change/remove ioctl:
> https://lore.kernel.org/kernelnewbies/YeVoAlv0ubKrmckV@kroah.com/T/#t 
> 
> The tldr;:
> 
> If there is a userspace tool that interfaces with char
> device and we can change it at the same time as the kernel code then we 
> can have a bit more flexibility on how to send/retrieve values to/from 
> the driver. If not then we would be required to keep the original ioctl
> compatibility even if we move to a different approach due to the
> impossibility of knowing whether or not someone is relying on that. :(
> 
> Everything leads me to believe that there isn't such userspace tool /
> lib so I emailed the original author just to be 100% sure that this is
> the case.

I don't know which is more appropriate sysfs or configfs.  I haven't
used configfs much and also I don't know much about pi433.

But I would encourage you to leave the ioctl as is.  There is no
advantage to the ioctl except backwards compatability.  This stuff can
all be done with sysfs.  It would be easier and more flexible going
forward.

regards,
dan carpenter

Re: [PATCH v3 0/3] staging: pi433: validate min/max bit rate settings

[Dan Carpenter]

I'm fine with this.

Acked-by: Dan Carpenter <dan.carpenter@oracle.com>

But please do add the note to the TODO.

regards,
dan carpenter

Re: [PATCH v3 0/3] staging: pi433: validate min/max bit rate settings

[Paulo Miguel Almeida]

On Wed, Jan 19, 2022 at 09:04:00AM +0300, Dan Carpenter wrote:
> I'm fine with this.
> 
> Acked-by: Dan Carpenter <dan.carpenter@oracle.com>
> 
> But please do add the note to the TODO.
> 
> regards,
> dan carpenter

Hi Dan, 

Thanks for the Acked-by. The TODO note was added on 0/3 of this patch
series:

https://lore.kernel.org/lkml/20220118230555.GA4961@mail.google.com/

thanks,

Paulo Almeida

Re: [PATCH v3 0/3] staging: pi433: validate min/max bit rate settings

[Dan Carpenter]

On Sun, Jan 23, 2022 at 08:43:08PM +1300, Paulo Miguel Almeida wrote:
> On Wed, Jan 19, 2022 at 09:04:00AM +0300, Dan Carpenter wrote:
> > I'm fine with this.
> > 
> > Acked-by: Dan Carpenter <dan.carpenter@oracle.com>
> > 
> > But please do add the note to the TODO.
> > 
> > regards,
> > dan carpenter
> 
> Hi Dan, 
> 
> Thanks for the Acked-by. The TODO note was added

Oh.  Sorry for not noticing that.  Thanks!

regards,
dan carpenter