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 <paulo.miguel.almeida.rodenas@xxxxxxxxx> > --- > Patch dependency: > - This patch needs the patch below to be applied first as both tweak the > same file. > https://lore.kernel.org/lkml/20220108212728.GA7784@xxxxxxxxxxxxxxx/ > > 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 >
