This patchset improves readability, maintainability & fixes 1 bug:
- Rename device related vars.
- Update pi433_receive param type.
- Remove duplicated code in pi433_init.
- Reorder pi433_exit calls.
- Fix a potential debugfs resource leak.
v2->v1:
Followed by Dan Carpenter's <[email protected]> comments:
- Remove empty "fail" goto tag.
- Reorder pi433 init & exit calls so they have reverse order.
- Add "unreg_spi_drv" goto tag.
- Check "debugfs_create_dir" return value.
- Update "if" statements for consistency.
- Rename pi433_init return var to the more common used "ret".
v2->v3:
Followed by more of Dan Carpenter's <[email protected]> comments:
- Undo pi433 init calls reordering.
- Remove "unreg_spi_drv" tag.
- Undo return value check for "debugfs_create_dir".
- Undo "if" statements update.
- Undo pi433_init return var renaming.
- Split patch 5 into 3: duplicate code removal, bug fix, reorder
exit calls.
Shahar Avidar (7):
staging: pi433: Rename struct pi433_device buffer field to tx_buffer.
staging: pi433: Rename struct pi433_device instances to pi433.
staging: pi433: Replace pi433_receive param void type to struct
pi433_device.
staging: pi433: Rename "pi433_dev" of type "dev_t" to "pi433_devt"
staging: pi433: Remove duplicated code using the "goto" error recovery
scheme.
staging: pi433: Add debugfs_remove in case of driver register fails.
staging: pi433: Reorder pi433_exit cleanup calls.
drivers/staging/pi433/pi433_if.c | 690 +++++++++++++++----------------
1 file changed, 345 insertions(+), 345 deletions(-)
base-commit: a103e5ad21992384b0b4332df52e0467107eb113
prerequisite-patch-id: 91943193af2fea74182be67fb583235a3fbeb77b
prerequisite-patch-id: 2cad031ba6a0782a67ab1645ff034a8be65c2e76
prerequisite-patch-id: 1a852ed8f9d133aec7c651fd9007e59e39c55fb7
--
2.34.1
Driver holds buffers in different structs, as does the HW.
Using explicit names for buffers increases readability.
Signed-off-by: Shahar Avidar <[email protected]>
---
drivers/staging/pi433/pi433_if.c | 18 +++++++++---------
1 file changed, 9 insertions(+), 9 deletions(-)
diff --git a/drivers/staging/pi433/pi433_if.c b/drivers/staging/pi433/pi433_if.c
index 8c513ac62156..13b464ab7db8 100644
--- a/drivers/staging/pi433/pi433_if.c
+++ b/drivers/staging/pi433/pi433_if.c
@@ -90,7 +90,7 @@ struct pi433_device {
struct task_struct *tx_task_struct;
wait_queue_head_t tx_wait_queue;
u8 free_in_fifo;
- char buffer[MAX_MSG_SIZE];
+ char tx_buffer[MAX_MSG_SIZE];
/* rx related values */
struct pi433_rx_cfg rx_cfg;
@@ -613,8 +613,8 @@ static int pi433_tx_thread(void *data)
if (tx_cfg.enable_address_byte == OPTION_ON)
size++;
- /* prime buffer */
- memset(device->buffer, 0, size);
+ /* prime tx_buffer */
+ memset(device->tx_buffer, 0, size);
position = 0;
/* add length byte, if requested */
@@ -623,15 +623,15 @@ static int pi433_tx_thread(void *data)
* according to spec, length byte itself must be
* excluded from the length calculation
*/
- device->buffer[position++] = size - 1;
+ device->tx_buffer[position++] = size - 1;
/* add adr byte, if requested */
if (tx_cfg.enable_address_byte == OPTION_ON)
- device->buffer[position++] = tx_cfg.address_byte;
+ device->tx_buffer[position++] = tx_cfg.address_byte;
/* finally get message data from fifo */
- retval = kfifo_out(&device->tx_fifo, &device->buffer[position],
- sizeof(device->buffer) - position);
+ retval = kfifo_out(&device->tx_fifo, &device->tx_buffer[position],
+ sizeof(device->tx_buffer) - position);
dev_dbg(device->dev,
"read %d message byte(s) from fifo queue.\n", retval);
@@ -715,7 +715,7 @@ static int pi433_tx_thread(void *data)
device->free_in_fifo = 0;
rf69_write_fifo(spi,
- &device->buffer[position],
+ &device->tx_buffer[position],
write_size);
position += write_size;
} else {
@@ -723,7 +723,7 @@ static int pi433_tx_thread(void *data)
device->free_in_fifo -= size;
repetitions--;
rf69_write_fifo(spi,
- &device->buffer[position],
+ &device->tx_buffer[position],
(size - position));
position = 0; /* reset for next repetition */
}
--
2.34.1
debugfs_remove was called out of order.
Ensure pi433 init & exit have reverse function calls order.
Signed-off-by: Shahar Avidar <[email protected]>
---
drivers/staging/pi433/pi433_if.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/staging/pi433/pi433_if.c b/drivers/staging/pi433/pi433_if.c
index 77e37a5bd1a2..67b945a41067 100644
--- a/drivers/staging/pi433/pi433_if.c
+++ b/drivers/staging/pi433/pi433_if.c
@@ -1427,9 +1427,9 @@ module_init(pi433_init);
static void __exit pi433_exit(void)
{
spi_unregister_driver(&pi433_spi_driver);
+ debugfs_remove(root_dir);
class_unregister(&pi433_class);
unregister_chrdev(MAJOR(pi433_devt), pi433_spi_driver.driver.name);
- debugfs_remove(root_dir);
}
module_exit(pi433_exit);
--
2.34.1
pi433_receive is only called once.
It immediately assigns the data param to a struct pi433_device.
Rename param name to pi433.
Signed-off-by: Shahar Avidar <[email protected]>
---
drivers/staging/pi433/pi433_if.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/drivers/staging/pi433/pi433_if.c b/drivers/staging/pi433/pi433_if.c
index 7efe6b8501e3..208c0c6d3649 100644
--- a/drivers/staging/pi433/pi433_if.c
+++ b/drivers/staging/pi433/pi433_if.c
@@ -409,9 +409,8 @@ static int pi433_start_rx(struct pi433_device *pi433)
/*-------------------------------------------------------------------------*/
-static int pi433_receive(void *data)
+static int pi433_receive(struct pi433_device *pi433)
{
- struct pi433_device *pi433 = data;
struct spi_device *spi = pi433->spi;
int bytes_to_read, bytes_total;
int retval;
--
2.34.1
Distinguish struct device type instances from dev_t instances
to enhance readability.
Signed-off-by: Shahar Avidar <[email protected]>
---
drivers/staging/pi433/pi433_if.c | 14 +++++++-------
1 file changed, 7 insertions(+), 7 deletions(-)
diff --git a/drivers/staging/pi433/pi433_if.c b/drivers/staging/pi433/pi433_if.c
index 208c0c6d3649..62ce75b07bf0 100644
--- a/drivers/staging/pi433/pi433_if.c
+++ b/drivers/staging/pi433/pi433_if.c
@@ -53,7 +53,7 @@
#define FIFO_THRESHOLD 15 /* bytes */
#define NUM_DIO 2
-static dev_t pi433_dev;
+static dev_t pi433_devt;
static DEFINE_IDR(pi433_idr);
static DEFINE_MUTEX(minor_lock); /* Protect idr accesses */
static struct dentry *root_dir; /* debugfs root directory for the driver */
@@ -1262,7 +1262,7 @@ static int pi433_probe(struct spi_device *spi)
}
/* create device */
- pi433->devt = MKDEV(MAJOR(pi433_dev), pi433->minor);
+ pi433->devt = MKDEV(MAJOR(pi433_devt), pi433->minor);
pi433->dev = device_create(&pi433_class,
&spi->dev,
pi433->devt,
@@ -1276,7 +1276,7 @@ static int pi433_probe(struct spi_device *spi)
} else {
dev_dbg(pi433->dev,
"created device for major %d, minor %d\n",
- MAJOR(pi433_dev),
+ MAJOR(pi433_devt),
pi433->minor);
}
@@ -1398,13 +1398,13 @@ static int __init pi433_init(void)
* that will key udev/mdev to add/remove /dev nodes.
* Last, register the driver which manages those device numbers.
*/
- status = alloc_chrdev_region(&pi433_dev, 0, N_PI433_MINORS, "pi433");
+ status = alloc_chrdev_region(&pi433_devt, 0, N_PI433_MINORS, "pi433");
if (status < 0)
return status;
status = class_register(&pi433_class);
if (status) {
- unregister_chrdev(MAJOR(pi433_dev),
+ unregister_chrdev(MAJOR(pi433_devt),
pi433_spi_driver.driver.name);
return status;
}
@@ -1414,7 +1414,7 @@ static int __init pi433_init(void)
status = spi_register_driver(&pi433_spi_driver);
if (status < 0) {
class_unregister(&pi433_class);
- unregister_chrdev(MAJOR(pi433_dev),
+ unregister_chrdev(MAJOR(pi433_devt),
pi433_spi_driver.driver.name);
}
@@ -1427,7 +1427,7 @@ static void __exit pi433_exit(void)
{
spi_unregister_driver(&pi433_spi_driver);
class_unregister(&pi433_class);
- unregister_chrdev(MAJOR(pi433_dev), pi433_spi_driver.driver.name);
+ unregister_chrdev(MAJOR(pi433_devt), pi433_spi_driver.driver.name);
debugfs_remove(root_dir);
}
module_exit(pi433_exit);
--
2.34.1
pi433_init had "unregister_chrdev" called twice.
Remove it using goto statements.
Signed-off-by: Shahar Avidar <[email protected]>
---
v2->v1:
Followed by Dan Carpenter's <[email protected]> comments:
- Remove empty "fail" goto tag.
- Reorder pi433 init & exit calls so they have reverse order.
- Add "unreg_spi_drv" goto tag.
- Check "debugfs_create_dir" return value.
- Update "if" statments for consistency.
v2->v3:
- Undo pi433 init & exit calls reordering. Reorder exit calls in
a seperate patch.
- Remove "unreg_spi_drv" tag.
- Undo return value checking for "debugfs_create_dir".
- Undo "if" statements update.
drivers/staging/pi433/pi433_if.c | 20 ++++++++++----------
1 file changed, 10 insertions(+), 10 deletions(-)
diff --git a/drivers/staging/pi433/pi433_if.c b/drivers/staging/pi433/pi433_if.c
index 62ce75b07bf0..b01ee145ff3c 100644
--- a/drivers/staging/pi433/pi433_if.c
+++ b/drivers/staging/pi433/pi433_if.c
@@ -1403,21 +1403,21 @@ static int __init pi433_init(void)
return status;
status = class_register(&pi433_class);
- if (status) {
- unregister_chrdev(MAJOR(pi433_devt),
- pi433_spi_driver.driver.name);
- return status;
- }
+ if (status)
+ goto unreg_chrdev;
root_dir = debugfs_create_dir(KBUILD_MODNAME, NULL);
status = spi_register_driver(&pi433_spi_driver);
- if (status < 0) {
- class_unregister(&pi433_class);
- unregister_chrdev(MAJOR(pi433_devt),
- pi433_spi_driver.driver.name);
- }
+ if (status < 0)
+ goto unreg_class_and_remove_dbfs;
+ return 0;
+
+unreg_class_and_remove_dbfs:
+ class_unregister(&pi433_class);
+unreg_chrdev:
+ unregister_chrdev(MAJOR(pi433_devt), pi433_spi_driver.driver.name);
return status;
}
--
2.34.1
I suggest to use the summary phrase “Use common error handling code
in pi433_init()” instead.
> pi433_init had "unregister_chrdev" called twice.
> Remove it using goto statements.
How do you think about to use the following change description?
unregister_chrdev() was called in two if branches.
Thus add jump targets so that a bit of exception handling can be better
reused at the end of this function implementation.
…
v2->v3:
…
a seperate patch.
…
Would you like to avoid a typo here?
Regards,
Markus
On Fri, Apr 05, 2024 at 12:05:56PM +0200, Markus Elfring wrote:
> I suggest to use the summary phrase “Use common error handling code
> in pi433_init()” instead.
>
>
> > pi433_init had "unregister_chrdev" called twice.
> > Remove it using goto statements.
>
> How do you think about to use the following change description?
>
> unregister_chrdev() was called in two if branches.
> Thus add jump targets so that a bit of exception handling can be better
> reused at the end of this function implementation.
>
>
> …
> v2->v3:
> …
> a seperate patch.
> …
>
> Would you like to avoid a typo here?
>
> Regards,
> Markus
>
Hi,
This is the semi-friendly patch-bot of Greg Kroah-Hartman.
Markus, you seem to have sent a nonsensical or otherwise pointless
review comment to a patch submission on a Linux kernel developer mailing
list. I strongly suggest that you not do this anymore. Please do not
bother developers who are actively working to produce patches and
features with comments that, in the end, are a waste of time.
Patch submitter, please ignore Markus's suggestion; you do not need to
follow it at all. The person/bot/AI that sent it is being ignored by
almost all Linux kernel maintainers for having a persistent pattern of
behavior of producing distracting and pointless commentary, and
inability to adapt to feedback. Please feel free to also ignore emails
from them.
thanks,
greg k-h's patch email bot
Thanks!
Reviewed-by: Dan Carpenter <[email protected]>
regards,
dan carpenter