2022-01-16 16:28:02

by Paulo Miguel Almeida

[permalink] [raw]
Subject: [PATCH 0/3] 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


2022-01-17 00:46:57

by Paulo Miguel Almeida

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

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

2022-01-17 01:15:08

by Paulo Miguel Almeida

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

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

2022-01-17 01:15:14

by Paulo Miguel Almeida

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

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

2022-01-17 08:01:09

by kernel test robot

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

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

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

2022-01-17 14:14:00

by Paulo Miguel Almeida

[permalink] [raw]
Subject: [PATCH v2 0/3] 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
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

2022-01-17 14:14:57

by Paulo Miguel Almeida

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

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

2022-01-17 14:15:00

by Paulo Miguel Almeida

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

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

2022-01-17 14:15:03

by Paulo Miguel Almeida

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

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

2022-01-20 12:00:05

by Dan Carpenter

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

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

2022-01-21 06:03:04

by Paulo Miguel Almeida

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

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/[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

2022-01-21 11:14:40

by Paulo Miguel Almeida

[permalink] [raw]
Subject: [PATCH v3 0/3] 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
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

2022-01-21 11:14:51

by Paulo Miguel Almeida

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

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

2022-01-21 11:14:53

by Paulo Miguel Almeida

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

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

2022-01-21 11:24:01

by Paulo Miguel Almeida

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

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

2022-01-21 16:55:53

by Dan Carpenter

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

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

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

2022-01-21 16:57:41

by Dan Carpenter

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

I'm fine with this.

Acked-by: Dan Carpenter <[email protected]>

But please do add the note to the TODO.

regards,
dan carpenter

2022-01-24 07:13:21

by Paulo Miguel Almeida

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

On Wed, Jan 19, 2022 at 09:04:00AM +0300, Dan Carpenter wrote:
> I'm fine with this.
>
> Acked-by: Dan Carpenter <[email protected]>
>
> 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

2022-01-24 18:54:05

by Dan Carpenter

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

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