Staging: dgnc: Use goto for error handling
Subsystems (2)
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
@@ -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
@@ -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
@@ -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
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 anyif@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 anyif(...) { ...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 ...;
}Signed-off-by Quentin Lambert <[email protected]>--- 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@@ -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@@ -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
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
--
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
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
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
Greg Kroah-Hartman
maintainerQuoted 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 carpenterWhat 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