Skip to contentSkip to conversation

staging: dgnc: replace DGNC_VERIFY_BOARD macro with dgnc_verify_board function.

PATCH In review Elise Lennion 2 participants 2 messages Oct 10, 2016 Staging

lore
Share & tools
lore

Elise Lennion

author
#0 lore raw

PATCH v2 1/1 staging: dgnc: replace DGNC_VERIFY_BOARD macro with dgnc_verify_board function.

Fix checkpatch warning:

WARNING: Macros with flow control statements should be avoided

Macros with flow control statements should be avoided. They break the flow of the calling function and make it harder to test the code.

Signed-off-by Elise Lennion <[email protected]>

---

v2: Fixed the return values of dgnc_verify_board.
Reworked to return NULL instead of 0, as suggested by Julia Lawall <[email protected]>.

 drivers/staging/dgnc/dgnc_sysfs.c | 74 +++++++++++++++++++++++++++------------
 1 file changed, 51 insertions(+), 23 deletions(-)
drivers/staging/dgnc/dgnc_sysfs.c +51 -23
index 290bf6e..cdec40a 100644
@@ -90,17 +90,21 @@ void dgnc_remove_driver_sysfiles(struct pci_driver *dgnc_driver)
 	driver_remove_file(driverfs, &driver_attr_pollrate);
 }
 
-#define DGNC_VERIFY_BOARD(p, bd)				\
-	do {							\
-		if (!p)						\
-			return 0;				\
-								\
-		bd = dev_get_drvdata(p);			\
-		if (!bd || bd->magic != DGNC_BOARD_MAGIC)	\
-			return 0;				\
-		if (bd->state != BOARD_READY)			\
-			return 0;				\
-	} while (0)
+static struct dgnc_board *dgnc_verify_board(struct device *p)
+{
+	struct dgnc_board *bd;
+
+	if (!p)
+		return NULL;
+
+	bd = dev_get_drvdata(p);
+	if (!bd || bd->magic != DGNC_BOARD_MAGIC)
+		return NULL;
+	if (bd->state != BOARD_READY)
+		return NULL;
+
+	return bd;
+}
 
 static ssize_t vpd_show(struct device *p, struct device_attribute *attr,
 			char *buf)

@@ -109,7 +113,9 @@ static ssize_t vpd_show(struct device *p, struct device_attribute *attr,
 	int count = 0;
 	int i = 0;
 
-	DGNC_VERIFY_BOARD(p, bd);
+	bd = dgnc_verify_board(p);
+	if (!bd)
+		return 0;
 
 	count += sprintf(buf + count,
 		"\n      0  1  2  3  4  5  6  7  8  9  A  B  C  D  E  F");

@@ -130,7 +136,9 @@ static ssize_t serial_number_show(struct device *p,
 	struct dgnc_board *bd;
 	int count = 0;
 
-	DGNC_VERIFY_BOARD(p, bd);
+	bd = dgnc_verify_board(p);
+	if (!bd)
+		return 0;
 
 	if (bd->serial_num[0] == '\0')
 		count += sprintf(buf + count, "<UNKNOWN>\n");

@@ -148,7 +156,9 @@ static ssize_t ports_state_show(struct device *p,
 	int count = 0;
 	int i = 0;
 
-	DGNC_VERIFY_BOARD(p, bd);
+	bd = dgnc_verify_board(p);
+	if (!bd)
+		return 0;
 
 	for (i = 0; i < bd->nasync; i++) {
 		count += snprintf(buf + count, PAGE_SIZE - count,

@@ -166,7 +176,9 @@ static ssize_t ports_baud_show(struct device *p,
 	int count = 0;
 	int i = 0;
 
-	DGNC_VERIFY_BOARD(p, bd);
+	bd = dgnc_verify_board(p);
+	if (!bd)
+		return 0;
 
 	for (i = 0; i < bd->nasync; i++) {
 		count +=  snprintf(buf + count, PAGE_SIZE - count,

@@ -184,7 +196,9 @@ static ssize_t ports_msignals_show(struct device *p,
 	int count = 0;
 	int i = 0;
 
-	DGNC_VERIFY_BOARD(p, bd);
+	bd = dgnc_verify_board(p);
+	if (!bd)
+		return 0;
 
 	for (i = 0; i < bd->nasync; i++) {
 		struct channel_t *ch = bd->channels[i];

@@ -215,7 +229,9 @@ static ssize_t ports_iflag_show(struct device *p,
 	int count = 0;
 	int i = 0;
 
-	DGNC_VERIFY_BOARD(p, bd);
+	bd = dgnc_verify_board(p);
+	if (!bd)
+		return 0;
 
 	for (i = 0; i < bd->nasync; i++) {
 		count += snprintf(buf + count, PAGE_SIZE - count, "%d %x\n",

@@ -233,7 +249,9 @@ static ssize_t ports_cflag_show(struct device *p,
 	int count = 0;
 	int i = 0;
 
-	DGNC_VERIFY_BOARD(p, bd);
+	bd = dgnc_verify_board(p);
+	if (!bd)
+		return 0;
 
 	for (i = 0; i < bd->nasync; i++) {
 		count += snprintf(buf + count, PAGE_SIZE - count, "%d %x\n",

@@ -251,7 +269,9 @@ static ssize_t ports_oflag_show(struct device *p,
 	int count = 0;
 	int i = 0;
 
-	DGNC_VERIFY_BOARD(p, bd);
+	bd = dgnc_verify_board(p);
+	if (!bd)
+		return 0;
 
 	for (i = 0; i < bd->nasync; i++) {
 		count += snprintf(buf + count, PAGE_SIZE - count, "%d %x\n",

@@ -269,7 +289,9 @@ static ssize_t ports_lflag_show(struct device *p,
 	int count = 0;
 	int i = 0;
 
-	DGNC_VERIFY_BOARD(p, bd);
+	bd = dgnc_verify_board(p);
+	if (!bd)
+		return 0;
 
 	for (i = 0; i < bd->nasync; i++) {
 		count += snprintf(buf + count, PAGE_SIZE - count, "%d %x\n",

@@ -287,7 +309,9 @@ static ssize_t ports_digi_flag_show(struct device *p,
 	int count = 0;
 	int i = 0;
 
-	DGNC_VERIFY_BOARD(p, bd);
+	bd = dgnc_verify_board(p);
+	if (!bd)
+		return 0;
 
 	for (i = 0; i < bd->nasync; i++) {
 		count += snprintf(buf + count, PAGE_SIZE - count, "%d %x\n",

@@ -305,7 +329,9 @@ static ssize_t ports_rxcount_show(struct device *p,
 	int count = 0;
 	int i = 0;
 
-	DGNC_VERIFY_BOARD(p, bd);
+	bd = dgnc_verify_board(p);
+	if (!bd)
+		return 0;
 
 	for (i = 0; i < bd->nasync; i++) {
 		count += snprintf(buf + count, PAGE_SIZE - count, "%d %ld\n",

@@ -323,7 +349,9 @@ static ssize_t ports_txcount_show(struct device *p,
 	int count = 0;
 	int i = 0;
 
-	DGNC_VERIFY_BOARD(p, bd);
+	bd = dgnc_verify_board(p);
+	if (!bd)
+		return 0;
 
 	for (i = 0; i < bd->nasync; i++) {
 		count += snprintf(buf + count, PAGE_SIZE - count, "%d %ld\n",
Signature · 1 line
2.7.4
maintainer
#1 lore raw
Quoted 41 lines · Elise Lennion

Fix checkpatch warning:

WARNING: Macros with flow control statements should be avoided

Macros with flow control statements should be avoided. They break the flow of the calling function and make it harder to test the code.

Signed-off-by Elise Lennion <[email protected]>

---

v2: Fixed the return values of dgnc_verify_board.
Reworked to return NULL instead of 0, as suggested by Julia Lawall <[email protected]>.

 drivers/staging/dgnc/dgnc_sysfs.c | 74 +++++++++++++++++++++++++++------------
 1 file changed, 51 insertions(+), 23 deletions(-)
drivers/staging/dgnc/dgnc_sysfs.c +5 -11
index 290bf6e..cdec40a 100644
@@ -90,17 +90,21 @@ void dgnc_remove_driver_sysfiles(struct pci_driver *dgnc_driver)
 	driver_remove_file(driverfs, &driver_attr_pollrate);
 }
 
-#define DGNC_VERIFY_BOARD(p, bd)				\
-	do {							\
-		if (!p)						\
-			return 0;				\
-								\
-		bd = dev_get_drvdata(p);			\
-		if (!bd || bd->magic != DGNC_BOARD_MAGIC)	\
-			return 0;				\
-		if (bd->state != BOARD_READY)			\
-			return 0;				\
-	} while (0)
+static struct dgnc_board *dgnc_verify_board(struct device *p)
+{
+	struct dgnc_board *bd;
+
+	if (!p)

drivers/staging/dgnc/dgnc_sysfs.c void dgnc_remove_driver_sysfiles(struct pci_driver *dgnc_driver) · line 107

Quoted 10 lines
drivers/staging/dgnc/dgnc_sysfs.c +10 -0
+		return NULL;
+
+	bd = dev_get_drvdata(p);
+	if (!bd || bd->magic != DGNC_BOARD_MAGIC)
+		return NULL;
+	if (bd->state != BOARD_READY)
+		return NULL;
+
+	return bd;
+}

While this is a nice cleanup, take a step back and see what is being done with this code.

First off, (!p) can never be true, so checking for it shows that the original author didn't understand how sysfs worked. After that, bd will also always be not NULL, as the driver already set that up. Same for the ->magic field, that's pointless to check as it can't be wrong.

So maybe, BOARD_READY matters, but that's impossible to be not correct as it is set before the sysfs files are created, and never changes from what I can tell.

So all of these checks are pointless and can just be removed.

But then, step back even further, what code is checking for these things? It's odd sysfs files that I have said need to be removed from the driver as they are pointless and not something that any tty driver should have.

So how about just dropping the sysfs files instead? That deletes a bunch of code (some of it wrong and pointless as described above), gets the driver closer to being mergable out of the staging directory, and it keeps anyone from ever copying bad coding patterns like this ever again.

thanks,

greg k-h