staging: pi433: validate min/max bit rate settings

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/[email protected]/
- https://lore.kernel.org/lkml/[email protected]/

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

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

---
 drivers/staging/pi433/rf69.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

index ee8c81d164e1..f4ac17adcd83 100644
@@ -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

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

---
 drivers/staging/pi433/pi433_if.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

index f9f86e2c44a9..17ff51f6a9da 100644
@@ -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

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

---
 drivers/staging/pi433/rf69.c | 12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

index f4ac17adcd83..70ffe291b0e6 100644
@@ -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

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/[email protected]/
- https://lore.kernel.org/lkml/[email protected]/

Changelog:

v2: change bitrate variable type to u32 as pointed by kernel test bot
v1: https://lore.kernel.org/lkml/[email protected]/

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

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

---
 drivers/staging/pi433/rf69.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

index ee8c81d164e1..f4ac17adcd83 100644
@@ -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

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

---
 drivers/staging/pi433/pi433_if.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

index f9f86e2c44a9..17ff51f6a9da 100644
@@ -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

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

---
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(-)

index f4ac17adcd83..adba69b8365e 100644
@@ -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;
 	}

index c25942f142a6..3b8184155326 100644
@@ -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

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

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;
}

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/[email protected]/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

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/[email protected]/
- https://lore.kernel.org/lkml/[email protected]/

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/[email protected]/

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

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

---
 drivers/staging/pi433/rf69.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

index ee8c81d164e1..f4ac17adcd83 100644
@@ -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

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

---
 drivers/staging/pi433/pi433_if.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

index f9f86e2c44a9..17ff51f6a9da 100644
@@ -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

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

---
 drivers/staging/pi433/TODO   |  2 ++
 drivers/staging/pi433/rf69.c | 10 +++++++++-
 2 files changed, 11 insertions(+), 1 deletion(-)

index 63a40bfcc67e..b9e6c01a02e0 100644
@@ -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.

index f4ac17adcd83..d60514a840c2 100644
@@ -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

I'm fine with this.

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/[email protected]/

thanks,

Paulo Almeida

Oh. Sorry for not noticing that. Thanks!

regards,
dan carpenter

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.

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