this driver relies on exposing a char device to userspace to tx
messages. Every message can be sent using different trasmitter settings
such so the tx_cfg must be written before sending any messages.
Failing to do so will cause the message to fail silently depending on
printk/dynamic_debug settings which makes it hard to troubleshoot.
This patch add a control variable that will get initialized once tx_cfg
is set for the fd used when interacting with the char device.
Signed-off-by: Paulo Miguel Almeida <[email protected]>
---
Patch dependency:
- This patch needs the patch below to be applied first as both tweak the
same file.
https://lore.kernel.org/lkml/[email protected]/
meta-comments:
- I'm not entirely sure if -EPERM is the best error code to return here,
I'm taking suggestions
---
drivers/staging/pi433/pi433_if.c | 16 +++++++++++++++-
1 file changed, 15 insertions(+), 1 deletion(-)
diff --git a/drivers/staging/pi433/pi433_if.c b/drivers/staging/pi433/pi433_if.c
index 051c9052aeeb..3e9913f4bc7d 100644
--- a/drivers/staging/pi433/pi433_if.c
+++ b/drivers/staging/pi433/pi433_if.c
@@ -108,6 +108,9 @@ struct pi433_device {
struct pi433_instance {
struct pi433_device *device;
struct pi433_tx_cfg tx_cfg;
+
+ /* control flags */
+ bool tx_cfg_initialized;
};
/*-------------------------------------------------------------------------*/
@@ -823,6 +826,16 @@ pi433_write(struct file *filp, const char __user *buf,
if (count > MAX_MSG_SIZE)
return -EMSGSIZE;
+ /*
+ * check if tx_cfg has been initialized otherwise we won't be able to
+ * config the RF trasmitter correctly due to invalid settings
+ */
+ if (!instance->tx_cfg_initialized) {
+ dev_dbg(device->dev,
+ "write: failed due to uninitialized tx_cfg");
+ return -EPERM;
+ }
+
/*
* write the following sequence into fifo:
* - tx_cfg
@@ -897,6 +910,7 @@ pi433_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
return -EFAULT;
mutex_lock(&device->tx_fifo_lock);
memcpy(&instance->tx_cfg, &tx_cfg, sizeof(struct pi433_tx_cfg));
+ instance->tx_cfg_initialized = true;
mutex_unlock(&device->tx_fifo_lock);
break;
case PI433_IOC_RD_RX_CFG:
@@ -950,7 +964,7 @@ static int pi433_open(struct inode *inode, struct file *filp)
/* setup instance data*/
instance->device = device;
instance->tx_cfg.bit_rate = 4711;
- // TODO: fill instance->tx_cfg;
+ instance->tx_cfg_initialized = false;
/* instance data as context */
filp->private_data = instance;
--
2.25.4
On Sun, Jan 09, 2022 at 11:26:52AM +1300, Paulo Miguel Almeida wrote:
Hi, Paulo.
Thanks for the patch.
I have some opinion below.
> this driver relies on exposing a char device to userspace to tx
> messages. Every message can be sent using different trasmitter settings
> such so the tx_cfg must be written before sending any messages.
> Failing to do so will cause the message to fail silently depending on
> printk/dynamic_debug settings which makes it hard to troubleshoot.
>
> This patch add a control variable that will get initialized once tx_cfg
> is set for the fd used when interacting with the char device.
I don't know that adding flag is good idea. It seems that initializing
to default is better than this.
>
> Signed-off-by: Paulo Miguel Almeida <[email protected]>
> ---
> Patch dependency:
> - This patch needs the patch below to be applied first as both tweak the
> same file.
> https://lore.kernel.org/lkml/[email protected]/
>
> meta-comments:
> - I'm not entirely sure if -EPERM is the best error code to return here,
> I'm taking suggestions
> ---
> drivers/staging/pi433/pi433_if.c | 16 +++++++++++++++-
> 1 file changed, 15 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/staging/pi433/pi433_if.c b/drivers/staging/pi433/pi433_if.c
> index 051c9052aeeb..3e9913f4bc7d 100644
> --- a/drivers/staging/pi433/pi433_if.c
> +++ b/drivers/staging/pi433/pi433_if.c
> @@ -108,6 +108,9 @@ struct pi433_device {
> struct pi433_instance {
> struct pi433_device *device;
> struct pi433_tx_cfg tx_cfg;
> +
> + /* control flags */
> + bool tx_cfg_initialized;
> };
>
> /*-------------------------------------------------------------------------*/
> @@ -823,6 +826,16 @@ pi433_write(struct file *filp, const char __user *buf,
> if (count > MAX_MSG_SIZE)
> return -EMSGSIZE;
>
> + /*
> + * check if tx_cfg has been initialized otherwise we won't be able to
> + * config the RF trasmitter correctly due to invalid settings
> + */
> + if (!instance->tx_cfg_initialized) {
> + dev_dbg(device->dev,
> + "write: failed due to uninitialized tx_cfg");
> + return -EPERM;
> + }
> +
> /*
> * write the following sequence into fifo:
> * - tx_cfg
> @@ -897,6 +910,7 @@ pi433_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
> return -EFAULT;
> mutex_lock(&device->tx_fifo_lock);
> memcpy(&instance->tx_cfg, &tx_cfg, sizeof(struct pi433_tx_cfg));
> + instance->tx_cfg_initialized = true;
> mutex_unlock(&device->tx_fifo_lock);
> break;
> case PI433_IOC_RD_RX_CFG:
> @@ -950,7 +964,7 @@ static int pi433_open(struct inode *inode, struct file *filp)
> /* setup instance data*/
> instance->device = device;
> instance->tx_cfg.bit_rate = 4711;
> - // TODO: fill instance->tx_cfg;
> + instance->tx_cfg_initialized = false;
>
> /* instance data as context */
> filp->private_data = instance;
> --
> 2.25.4
>
On Sun, Jan 09, 2022 at 03:49:50PM +0000, Sidong Yang wrote:
> On Sun, Jan 09, 2022 at 11:26:52AM +1300, Paulo Miguel Almeida wrote:
>
> Hi, Paulo.
> Thanks for the patch.
> I have some opinion below.
thanks for taking the time to review my patch :)
> > this driver relies on exposing a char device to userspace to tx
> > messages. Every message can be sent using different trasmitter settings
> > such so the tx_cfg must be written before sending any messages.
> > Failing to do so will cause the message to fail silently depending on
> > printk/dynamic_debug settings which makes it hard to troubleshoot.
> >
> > This patch add a control variable that will get initialized once tx_cfg
> > is set for the fd used when interacting with the char device.
>
> I don't know that adding flag is good idea. It seems that initializing
> to default is better than this.
the reasons why I thought that the flag was a good approach is because of
these points:
1 - it returns an error to userspace to help the developers
troubleshooting what is missing/required to make a message to be
successfully tx'ed. Unfortunately, once the message is in the chip
queue, there isn't much that the user can know so, from my humble point
of view, any way we can avoid hard-to-debug problems, we should.
2 - rf69 work with multiple frequencies (315,433,868 and 915MHz) in which
acceptance varies from region to region. Essentially, picking 1 default
frequency may be ideal for one place but not right for another.
Let me know if you agree with my train of thought above and if not,
share with me your point of view.
thanks,
Paulo A.
Seems reasonable.
On Sun, Jan 09, 2022 at 11:26:52AM +1300, Paulo Miguel Almeida wrote:
> meta-comments:
> - I'm not entirely sure if -EPERM is the best error code to return here,
> I'm taking suggestions
Better to just return -EINVAL.
> ---
> drivers/staging/pi433/pi433_if.c | 16 +++++++++++++++-
> 1 file changed, 15 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/staging/pi433/pi433_if.c b/drivers/staging/pi433/pi433_if.c
> index 051c9052aeeb..3e9913f4bc7d 100644
> --- a/drivers/staging/pi433/pi433_if.c
> +++ b/drivers/staging/pi433/pi433_if.c
> @@ -108,6 +108,9 @@ struct pi433_device {
> struct pi433_instance {
> struct pi433_device *device;
> struct pi433_tx_cfg tx_cfg;
> +
> + /* control flags */
> + bool tx_cfg_initialized;
> };
>
> /*-------------------------------------------------------------------------*/
> @@ -823,6 +826,16 @@ pi433_write(struct file *filp, const char __user *buf,
> if (count > MAX_MSG_SIZE)
> return -EMSGSIZE;
>
> + /*
> + * check if tx_cfg has been initialized otherwise we won't be able to
> + * config the RF trasmitter correctly due to invalid settings
> + */
> + if (!instance->tx_cfg_initialized) {
> + dev_dbg(device->dev,
> + "write: failed due to uninitialized tx_cfg");
Use dev_notice_once() or similar. Maybe "unconfigured" instead of
uninitialized?
dev_notice_once(device->dev,
"write: failed due to unconfigured tx_cfg (see PI433_IOC_WR_TX_CFG)");
> + return -EPERM;
> + }
> +
> /*
> * write the following sequence into fifo:
> * - tx_cfg
> @@ -897,6 +910,7 @@ pi433_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
> return -EFAULT;
> mutex_lock(&device->tx_fifo_lock);
> memcpy(&instance->tx_cfg, &tx_cfg, sizeof(struct pi433_tx_cfg));
> + instance->tx_cfg_initialized = true;
> mutex_unlock(&device->tx_fifo_lock);
> break;
> case PI433_IOC_RD_RX_CFG:
> @@ -950,7 +964,7 @@ static int pi433_open(struct inode *inode, struct file *filp)
> /* setup instance data*/
> instance->device = device;
> instance->tx_cfg.bit_rate = 4711;
This is sort of pointless because it can't work until that gets over
written.
> - // TODO: fill instance->tx_cfg;
> + instance->tx_cfg_initialized = false;
kzalloc() will already set this to false.
regards,
dan carpenter
On Mon, Jan 10, 2022 at 08:05:52AM +1300, Paulo Miguel Almeida wrote:
> On Sun, Jan 09, 2022 at 03:49:50PM +0000, Sidong Yang wrote:
> > On Sun, Jan 09, 2022 at 11:26:52AM +1300, Paulo Miguel Almeida wrote:
> >
> > Hi, Paulo.
> > Thanks for the patch.
> > I have some opinion below.
>
> thanks for taking the time to review my patch :)
>
> > > this driver relies on exposing a char device to userspace to tx
> > > messages. Every message can be sent using different trasmitter settings
> > > such so the tx_cfg must be written before sending any messages.
> > > Failing to do so will cause the message to fail silently depending on
> > > printk/dynamic_debug settings which makes it hard to troubleshoot.
> > >
> > > This patch add a control variable that will get initialized once tx_cfg
> > > is set for the fd used when interacting with the char device.
> >
> > I don't know that adding flag is good idea. It seems that initializing
> > to default is better than this.
>
> the reasons why I thought that the flag was a good approach is because of
> these points:
>
> 1 - it returns an error to userspace to help the developers
> troubleshooting what is missing/required to make a message to be
> successfully tx'ed. Unfortunately, once the message is in the chip
> queue, there isn't much that the user can know so, from my humble point
> of view, any way we can avoid hard-to-debug problems, we should.
I understood that you mean that user has difficult to debug when it
write message before initializing. If so, user would get debug message
that some tx_cfg members are invalid. I think the point is which message
would be useful for debugging. 'Some member is invalid' or 'tx_cfg is
uninitialized'. I think the latter that you said is more accurate than
before one. I think it make sense.
>
> 2 - rf69 work with multiple frequencies (315,433,868 and 915MHz) in which
> acceptance varies from region to region. Essentially, picking 1 default
> frequency may be ideal for one place but not right for another.
Actually, I don't have idea about this. I want to know that how to other
module like this handle this.
Thanks,
Sidong
>
> Let me know if you agree with my train of thought above and if not,
> share with me your point of view.
>
> thanks,
>
> Paulo A.
>
On Mon, Jan 10, 2022 at 12:17:23PM +0300, Dan Carpenter wrote:
> Seems reasonable.
>
> On Sun, Jan 09, 2022 at 11:26:52AM +1300, Paulo Miguel Almeida wrote:
> > meta-comments:
> > - I'm not entirely sure if -EPERM is the best error code to return here,
> > I'm taking suggestions
>
> Better to just return -EINVAL.
> > ---
> > drivers/staging/pi433/pi433_if.c | 16 +++++++++++++++-
> > 1 file changed, 15 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/staging/pi433/pi433_if.c b/drivers/staging/pi433/pi433_if.c
> > index 051c9052aeeb..3e9913f4bc7d 100644
> > --- a/drivers/staging/pi433/pi433_if.c
> > +++ b/drivers/staging/pi433/pi433_if.c
> > @@ -108,6 +108,9 @@ struct pi433_device {
> > struct pi433_instance {
> > struct pi433_device *device;
> > struct pi433_tx_cfg tx_cfg;
> > +
> > + /* control flags */
> > + bool tx_cfg_initialized;
> > };
> >
> > /*-------------------------------------------------------------------------*/
> > @@ -823,6 +826,16 @@ pi433_write(struct file *filp, const char __user *buf,
> > if (count > MAX_MSG_SIZE)
> > return -EMSGSIZE;
> >
> > + /*
> > + * check if tx_cfg has been initialized otherwise we won't be able to
> > + * config the RF trasmitter correctly due to invalid settings
> > + */
> > + if (!instance->tx_cfg_initialized) {
> > + dev_dbg(device->dev,
> > + "write: failed due to uninitialized tx_cfg");
>
> Use dev_notice_once() or similar. Maybe "unconfigured" instead of
> uninitialized?
>
> dev_notice_once(device->dev,
> "write: failed due to unconfigured tx_cfg (see PI433_IOC_WR_TX_CFG)");
>
>
> > + return -EPERM;
> > + }
> > +
> > /*
> > * write the following sequence into fifo:
> > * - tx_cfg
> > @@ -897,6 +910,7 @@ pi433_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
> > return -EFAULT;
> > mutex_lock(&device->tx_fifo_lock);
> > memcpy(&instance->tx_cfg, &tx_cfg, sizeof(struct pi433_tx_cfg));
> > + instance->tx_cfg_initialized = true;
> > mutex_unlock(&device->tx_fifo_lock);
> > break;
> > case PI433_IOC_RD_RX_CFG:
> > @@ -950,7 +964,7 @@ static int pi433_open(struct inode *inode, struct file *filp)
> > /* setup instance data*/
> > instance->device = device;
> > instance->tx_cfg.bit_rate = 4711;
>
> This is sort of pointless because it can't work until that gets over
> written.
>
>
> > - // TODO: fill instance->tx_cfg;
> > + instance->tx_cfg_initialized = false;
>
> kzalloc() will already set this to false.
>
> regards,
> dan carpenter
>
I agree with you on all points mentioned above. I will send another
version of this patch shortly.
Thanks for the patch review.
Paulo A.
this driver relies on exposing a char device to userspace to tx
messages. Every message can be sent using different trasmitter settings
such so the tx_cfg must be written before sending any messages.
Failing to do so will cause the message to fail silently depending on
printk/dynamic_debug settings which makes it hard to troubleshoot.
This patch add a control variable that will get initialized once tx_cfg
is set for the fd used when interacting with the char device.
Signed-off-by: Paulo Miguel Almeida <[email protected]>
---
changelog:
v2: changed message and remove redudant assignments pointed out during
code review. (Req: Dan Carpenter, Sidong Yang)
v1: https://lore.kernel.org/lkml/[email protected]/
patch dependency:
- This patch needs the patch below to be applied first as both tweak the
same file.
https://lore.kernel.org/lkml/[email protected]/
---
drivers/staging/pi433/pi433_if.c | 16 ++++++++++++++--
1 file changed, 14 insertions(+), 2 deletions(-)
diff --git a/drivers/staging/pi433/pi433_if.c b/drivers/staging/pi433/pi433_if.c
index 051c9052aeeb..f9f86e2c44a9 100644
--- a/drivers/staging/pi433/pi433_if.c
+++ b/drivers/staging/pi433/pi433_if.c
@@ -108,6 +108,9 @@ struct pi433_device {
struct pi433_instance {
struct pi433_device *device;
struct pi433_tx_cfg tx_cfg;
+
+ /* control flags */
+ bool tx_cfg_initialized;
};
/*-------------------------------------------------------------------------*/
@@ -823,6 +826,16 @@ pi433_write(struct file *filp, const char __user *buf,
if (count > MAX_MSG_SIZE)
return -EMSGSIZE;
+ /*
+ * check if tx_cfg has been initialized otherwise we won't be able to
+ * config the RF trasmitter correctly due to invalid settings
+ */
+ if (!instance->tx_cfg_initialized) {
+ dev_notice_once(device->dev,
+ "write: failed due to unconfigured tx_cfg (see PI433_IOC_WR_TX_CFG)");
+ return -EINVAL;
+ }
+
/*
* write the following sequence into fifo:
* - tx_cfg
@@ -897,6 +910,7 @@ pi433_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
return -EFAULT;
mutex_lock(&device->tx_fifo_lock);
memcpy(&instance->tx_cfg, &tx_cfg, sizeof(struct pi433_tx_cfg));
+ instance->tx_cfg_initialized = true;
mutex_unlock(&device->tx_fifo_lock);
break;
case PI433_IOC_RD_RX_CFG:
@@ -949,8 +963,6 @@ static int pi433_open(struct inode *inode, struct file *filp)
/* setup instance data*/
instance->device = device;
- instance->tx_cfg.bit_rate = 4711;
- // TODO: fill instance->tx_cfg;
/* instance data as context */
filp->private_data = instance;
--
2.25.4