Thanks for the review, Dan! On Mon, Feb 16, 2015 at 10:04 AM, Dan Carpenter <dan.carpenter@xxxxxxxxxx> wrote: > On Sun, Feb 15, 2015 at 01:11:04PM +0100, Silvan Jegen wrote: >> diff --git a/drivers/media/pci/mantis/mantis_cards.c b/drivers/media/pci/mantis/mantis_cards.c >> index 801fc55..e566061 100644 >> --- a/drivers/media/pci/mantis/mantis_cards.c >> +++ b/drivers/media/pci/mantis/mantis_cards.c >> @@ -215,10 +215,11 @@ static int mantis_pci_probe(struct pci_dev *pdev, >> dprintk(MANTIS_ERROR, 1, "ERROR: Mantis DVB initialization failed <%d>", err); >> goto fail4; >> } >> + >> err = mantis_uart_init(mantis); >> if (err < 0) { >> dprintk(MANTIS_ERROR, 1, "ERROR: Mantis UART initialization failed <%d>", err); >> - goto fail6; >> + goto fail5; >> } >> >> devs++; >> @@ -226,10 +227,10 @@ static int mantis_pci_probe(struct pci_dev *pdev, >> return err; >> >> >> +fail5: >> dprintk(MANTIS_ERROR, 1, "ERROR: Mantis UART exit! <%d>", err); >> mantis_uart_exit(mantis); >> >> -fail6: >> fail4: >> dprintk(MANTIS_ERROR, 1, "ERROR: Mantis DMA exit! <%d>", err); >> mantis_dma_exit(mantis); > > This patch isn't right, I'm afraid. The person who wrote this driver > deliberately added some dead error handling code in case we change it > later and need to activate it. It's an ugly thing to do because it > causes static checker warnings, and confusion, and, in real life, then > we are not ever going to need to activate it. It's defensive > programming but we don't do defensive programming. > http://lwn.net/Articles/604813/ Just delete this dead code. I will do that. > Also this code uses GW-BASIC style numbered gotos. So ugly! The label > names should be based on what the label location does. Eg > "err_uart_exit", "err_dma_exit". I have written an essay about label > names: https://plus.google.com/106378716002406849458/posts/dnanfhQ4mHQ > > In theory, we should be calling mantis_dvb_exit() if mantis_uart_init() > fails. In reality, it can't fail but it's still wrong to leave that > out. In the next version of the patch, I will change the names of the goto labels accordingly. I will add the mantis_dvb_exit() call as well. > These patches would be easier to review if you just folded them into one > patch. Call it "fix error error handling" or something. Ok, I will fold the second patch into the first one replacing the goto label names with more appropriate ones. In the first patch I will just delete the dead code and change the jump labels to be more descriptive. I will leave for holidays at the end of the week so it's probably better if I wait with sending the new version of the patch until I am back. Thanks again, Silvan -- To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html