Skip to contentSkip to conversation

Staging: dgnc: Use goto for error handling

PATCH In review Quentin Lambert 4 participants 6 messages Mar 11, 2015 – Mar 12, 2015

Subsystems (2)
lore
Share & tools
lore
#0 lore raw

PATCH 2/3 Staging: dgnc: Use goto for error handling

This patch introduces goto statments for error handling and in cases where a lock needs to be released.

A simplified version of the semantic patch that finds this problem is as
follows: (http://coccinelle.lip6.fr)

@candidates exists@
identifier f, label;
statement s;
position p1, p2, p3;
@@

  f@p1(...) {
  ...when any
    if@p2(...) {
    ...when any
      s

      return@p3 ...;
    }

...when any
}

@good1 exists@
identifier candidates.f, candidates.label;
statement candidates.s;
position candidates.p1, candidates.p2;
@@

  f@p1(...) {
  ...when any
    if(...) {
    ...when any
      s
      return ...;
    }
    ...when any

    if@p2(...) {...}

...when any
}

@depends on good1@
identifier candidates.f, candidates.label;
position candidates.p1, candidates.p3;
@@

f@p1(...) {
...when any
* return@p3 ...;
}

---
 drivers/staging/dgnc/dgnc_cls.c    | 19 +++++++------------
 drivers/staging/dgnc/dgnc_driver.c | 14 +++-----------
 drivers/staging/dgnc/dgnc_neo.c    | 30 ++++++++++++------------------
 3 files changed, 22 insertions(+), 41 deletions(-)
drivers/staging/dgnc/dgnc_cls.c +7 -12
index bedc522..02f19f1 100644
@@ -1029,22 +1029,16 @@ static void cls_copy_data_from_queue_to_uart(struct channel_t *ch)
 	spin_lock_irqsave(&ch->ch_lock, flags);
 
 	/* No data to write to the UART */
-	if (ch->ch_w_tail = ch->ch_w_head) {
-		spin_unlock_irqrestore(&ch->ch_lock, flags);
-		return;
-	}
+	if (ch->ch_w_tail = ch->ch_w_head)
+		goto exit_unlock;
 
 	/* If port is "stopped", don't send any data to the UART */
 	if ((ch->ch_flags & CH_FORCED_STOP) ||
-				 (ch->ch_flags & CH_BREAK_SENDING)) {
-		spin_unlock_irqrestore(&ch->ch_lock, flags);
-		return;
-	}
+				 (ch->ch_flags & CH_BREAK_SENDING))
+		goto exit_unlock;
 
-	if (!(ch->ch_flags & (CH_TX_FIFO_EMPTY | CH_TX_FIFO_LWM))) {
-		spin_unlock_irqrestore(&ch->ch_lock, flags);
-		return;
-	}
+	if (!(ch->ch_flags & (CH_TX_FIFO_EMPTY | CH_TX_FIFO_LWM)))
+		goto exit_unlock;
 
 	n = 32;
 
@@ -1094,6 +1088,7 @@ static void cls_copy_data_from_queue_to_uart(struct channel_t *ch)
 	if (len_written > 0)
 		ch->ch_flags &= ~(CH_TX_FIFO_EMPTY | CH_TX_FIFO_LWM);
 
+exit_unlock:
 	spin_unlock_irqrestore(&ch->ch_lock, flags);
 }
 
drivers/staging/dgnc/dgnc_driver.c +3 -11
index b7fd2bf..9387a0e 100644
@@ -572,30 +572,19 @@ static int dgnc_found_board(struct pci_dev *pdev, int id)
 
 	rc = dgnc_tty_register(brd);
 	if (rc < 0) {
-		dgnc_tty_uninit(brd);
 		pr_err(DRVSTR ": Can't register tty devices (%d)\n", rc);
-		brd->state = BOARD_FAILED;
-		brd->dpastatus = BD_NOFEP;
 		goto failed;
 	}
 
 	rc = dgnc_finalize_board_init(brd);
 	if (rc < 0) {
-		dgnc_tty_uninit(brd);
 		pr_err(DRVSTR ": Can't finalize board init (%d)\n", rc);
-		brd->state = BOARD_FAILED;
-		brd->dpastatus = BD_NOFEP;
-
 		goto failed;
 	}
 
 	rc = dgnc_tty_init(brd);
 	if (rc < 0) {
-		dgnc_tty_uninit(brd);
 		pr_err(DRVSTR ": Can't init tty devices (%d)\n", rc);
-		brd->state = BOARD_FAILED;
-		brd->dpastatus = BD_NOFEP;
-
 		goto failed;
 	}
 
@@ -629,6 +618,9 @@ static int dgnc_found_board(struct pci_dev *pdev, int id)
 	return 0;
 
 failed:
+	dgnc_tty_uninit(brd);
+	brd->state = BOARD_FAILED;
+	brd->dpastatus = BD_NOFEP;
 
 	return -ENXIO;
 
drivers/staging/dgnc/dgnc_neo.c +12 -18
index 1268aa9..4deae2d 100644
@@ -1436,16 +1436,13 @@ static void neo_copy_data_from_queue_to_uart(struct channel_t *ch)
 	spin_lock_irqsave(&ch->ch_lock, flags);
 
 	/* No data to write to the UART */
-	if (ch->ch_w_tail = ch->ch_w_head) {
-		spin_unlock_irqrestore(&ch->ch_lock, flags);
-		return;
-	}
+	if (ch->ch_w_tail = ch->ch_w_head)
+		goto exit_unlock;
 
 	/* If port is "stopped", don't send any data to the UART */
-	if ((ch->ch_flags & CH_FORCED_STOP) || (ch->ch_flags & CH_BREAK_SENDING)) {
-		spin_unlock_irqrestore(&ch->ch_lock, flags);
-		return;
-	}
+	if ((ch->ch_flags & CH_FORCED_STOP) ||
+		 (ch->ch_flags & CH_BREAK_SENDING))
+		goto exit_unlock;
 
 	/*
 	 * If FIFOs are disabled. Send data directly to txrx register
@@ -1486,27 +1483,23 @@ static void neo_copy_data_from_queue_to_uart(struct channel_t *ch)
 			ch->ch_w_tail &= WQUEUEMASK;
 			ch->ch_txcount++;
 		}
-		spin_unlock_irqrestore(&ch->ch_lock, flags);
-		return;
+
+		goto exit_unlock;
 	}
 
 	/*
 	 * We have to do it this way, because of the EXAR TXFIFO count bug.
 	 */
 	if ((ch->ch_bd->dvid & 0xf0) < UART_XR17E158_DVID) {
-		if (!(ch->ch_flags & (CH_TX_FIFO_EMPTY | CH_TX_FIFO_LWM))) {
-			spin_unlock_irqrestore(&ch->ch_lock, flags);
-			return;
-		}
+		if (!(ch->ch_flags & (CH_TX_FIFO_EMPTY | CH_TX_FIFO_LWM)))
+			goto exit_unlock;
 
 		len_written = 0;
 
 		n = readb(&ch->ch_neo_uart->tfifo);
 
-		if ((unsigned int) n > ch->ch_t_tlevel) {
-			spin_unlock_irqrestore(&ch->ch_lock, flags);
-			return;
-		}
+		if ((unsigned int) n > ch->ch_t_tlevel)
+			goto exit_unlock;
 
 		n = UART_17158_TX_FIFOSIZE - ch->ch_t_tlevel;
 	} else {
@@ -1570,6 +1563,7 @@ static void neo_copy_data_from_queue_to_uart(struct channel_t *ch)
 		ch->ch_flags &= ~(CH_TX_FIFO_EMPTY | CH_TX_FIFO_LWM);
 	}
 
+exit_unlock:
 	spin_unlock_irqrestore(&ch->ch_lock, flags);
 }
 
Signature · 1 line
1.9.1
#1 lore raw
Quoted 138 lines · Quentin Lambert

This patch introduces goto statments for error handling and in cases where a lock needs to be released.

A simplified version of the semantic patch that finds this problem is as
follows: (http://coccinelle.lip6.fr)

@candidates exists@
identifier f, label;
statement s;
position p1, p2, p3;
@@

  f@p1(...) {
  ...when any
    if@p2(...) {
    ...when any
      s

      return@p3 ...;
    }

...when any
}

@good1 exists@
identifier candidates.f, candidates.label;
statement candidates.s;
position candidates.p1, candidates.p2;
@@

  f@p1(...) {
  ...when any
    if(...) {
    ...when any
      s
      return ...;
    }
    ...when any

    if@p2(...) {...}

...when any
}

@depends on good1@
identifier candidates.f, candidates.label;
position candidates.p1, candidates.p3;
@@

   f@p1(...) {
   ...when any

* return@p3 ...;
}

---
 drivers/staging/dgnc/dgnc_cls.c    | 19 +++++++------------
 drivers/staging/dgnc/dgnc_driver.c | 14 +++-----------
 drivers/staging/dgnc/dgnc_neo.c    | 30 ++++++++++++------------------
 3 files changed, 22 insertions(+), 41 deletions(-)
drivers/staging/dgnc/dgnc_cls.c +7 -12
index bedc522..02f19f1 100644
@@ -1029,22 +1029,16 @@ static void cls_copy_data_from_queue_to_uart(struct channel_t *ch)
 	spin_lock_irqsave(&ch->ch_lock, flags);
 
 	/* No data to write to the UART */
-	if (ch->ch_w_tail = ch->ch_w_head) {
-		spin_unlock_irqrestore(&ch->ch_lock, flags);
-		return;
-	}
+	if (ch->ch_w_tail = ch->ch_w_head)
+		goto exit_unlock;
 
 	/* If port is "stopped", don't send any data to the UART */
 	if ((ch->ch_flags & CH_FORCED_STOP) ||
-				 (ch->ch_flags & CH_BREAK_SENDING)) {
-		spin_unlock_irqrestore(&ch->ch_lock, flags);
-		return;
-	}
+				 (ch->ch_flags & CH_BREAK_SENDING))
+		goto exit_unlock;
 
-	if (!(ch->ch_flags & (CH_TX_FIFO_EMPTY | CH_TX_FIFO_LWM))) {
-		spin_unlock_irqrestore(&ch->ch_lock, flags);
-		return;
-	}
+	if (!(ch->ch_flags & (CH_TX_FIFO_EMPTY | CH_TX_FIFO_LWM)))
+		goto exit_unlock;
 
 	n = 32;
 
@@ -1094,6 +1088,7 @@ static void cls_copy_data_from_queue_to_uart(struct channel_t *ch)
 	if (len_written > 0)
 		ch->ch_flags &= ~(CH_TX_FIFO_EMPTY | CH_TX_FIFO_LWM);
 
+exit_unlock:
 	spin_unlock_irqrestore(&ch->ch_lock, flags);
 }
 
drivers/staging/dgnc/dgnc_driver.c +0 -11
index b7fd2bf..9387a0e 100644
@@ -572,30 +572,19 @@ static int dgnc_found_board(struct pci_dev *pdev, int id)
 
 	rc = dgnc_tty_register(brd);
 	if (rc < 0) {
-		dgnc_tty_uninit(brd);
 		pr_err(DRVSTR ": Can't register tty devices (%d)\n", rc);
-		brd->state = BOARD_FAILED;
-		brd->dpastatus = BD_NOFEP;
 		goto failed;
 	}
 
 	rc = dgnc_finalize_board_init(brd);
 	if (rc < 0) {
-		dgnc_tty_uninit(brd);
 		pr_err(DRVSTR ": Can't finalize board init (%d)\n", rc);
-		brd->state = BOARD_FAILED;
-		brd->dpastatus = BD_NOFEP;
-
 		goto failed;
 	}
 
 	rc = dgnc_tty_init(brd);
 	if (rc < 0) {
-		dgnc_tty_uninit(brd);
 		pr_err(DRVSTR ": Can't init tty devices (%d)\n", rc);
-		brd->state = BOARD_FAILED;
-		brd->dpastatus = BD_NOFEP;
-
 		goto failed;
 	}
 

drivers/staging/dgnc/dgnc_driver.c static int dgnc_found_board(struct pci_dev *pdev, int id) · line 623

Quoted 6 lines
drivers/staging/dgnc/dgnc_driver.c +3 -0
@@ -629,6 +618,9 @@ static int dgnc_found_board(struct pci_dev *pdev, int id)
 	return 0;
 
 failed:
+	dgnc_tty_uninit(brd);
+	brd->state = BOARD_FAILED;
+	brd->dpastatus = BD_NOFEP;

You'll probably have to remake this patch without this change because a patch I've submitted a few days ago completely removes these and 2 other ones depend on it. It's still not in staging-testing for some reason :(

Signature · 2 lines
Thanks,
Giedrius
#3 lore raw
Quoted 8 lines · Dan Carpenter
Quoted 2 lines · Giedrius StatkeviÄ�ius

It's still not in staging-testing for some reason :(

It can take several weeks to get merged. Relax.

regards,
dan carpenter

What should i do concerning that ?
I need to send a second version of this patch anyway to fix the fact that i inverted 2 statements, namely:

+exit_unlock:
+ if (ld)
+ tty_ldisc_deref(ld);
+ spin_unlock_irqrestore(&ch->ch_lock, flags);

should be

+exit_unlock:
+ spin_unlock_irqrestore(&ch->ch_lock, flags);
+ if (ld)
+ tty_ldisc_deref(ld);

Should I wait these several weeks to send the second version or should I send a second version fixing this issue a wait for someone to ask me to solve the conflict ?

Quentin
--
To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

#4 lore raw
Quoted 3 lines · Quentin Lambert

Should I wait these several weeks to send the second version or should I send a second version fixing this issue a wait for someone to ask me to solve the conflict ?

Greg applies patches in the order they come. It's normally straight forward which ones are going to be merged and which ones are not. So you could just apply Giedrius's patch and put a note in the --- cut off the it depends on the earlier patch.

If the patch doesn't apply, Greg won't take any time to figure out why. You'll just get a note to redo it.

If you two plan on working on the same driver for a while then you could coordinate your patches and send them as a patchset.

regards,
dan carpenter

#5 lore raw
In reply to Quentin Lambert
Quoted 27 lines · Quentin Lambert
Quoted 8 lines · Dan Carpenter
Quoted 2 lines · Giedrius StatkeviÄ�ius

It's still not in staging-testing for some reason :(

It can take several weeks to get merged. Relax.

regards,
dan carpenter

What should i do concerning that ?
I need to send a second version of this patch anyway to fix the fact that i inverted 2 statements, namely:

(quoted diff) +4 -0
+exit_unlock:
+	if (ld)
+		tty_ldisc_deref(ld);
+	spin_unlock_irqrestore(&ch->ch_lock, flags);

should be

(quoted diff) +4 -0
+exit_unlock:
+	spin_unlock_irqrestore(&ch->ch_lock, flags);
+	if (ld)
+		tty_ldisc_deref(ld);
Quoted 3 lines

Should I wait these several weeks to send the second version or should I send a second version fixing this issue a wait for someone to ask me to solve the conflict ?

Just wait a bit, I should catch up on patches in this area soon, and I will let everyone know if patches do not apply.

thanks,

greg k-h
--
To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html