Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1760303AbXIBUcX (ORCPT ); Sun, 2 Sep 2007 16:32:23 -0400 Received: (majordomo@vger.kernel.org) by vger.kernel.org id S1750993AbXIBUcO (ORCPT ); Sun, 2 Sep 2007 16:32:14 -0400 Received: from srv5.dvmed.net ([207.36.208.214]:39349 "EHLO mail.dvmed.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750715AbXIBUcM (ORCPT ); Sun, 2 Sep 2007 16:32:12 -0400 Message-ID: <46DB1DCA.3050509@garzik.org> Date: Sun, 02 Sep 2007 16:32:10 -0400 From: Jeff Garzik User-Agent: Thunderbird 2.0.0.5 (X11/20070719) MIME-Version: 1.0 To: Satyam Sharma CC: Linux Kernel Mailing List , James Bottomley , aacraid@adaptec.com, linux-scsi@vger.kernel.org Subject: Re: [PATCH -mm] IPS SCSI driver: Check return of scsi_add_host() References: In-Reply-To: Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 8bit X-Spam-Score: -4.3 (----) X-Spam-Report: SpamAssassin version 3.1.9 on srv5.dvmed.net summary: Content analysis details: (-4.3 points, 5.0 required) Sender: linux-kernel-owner@vger.kernel.org X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 2019 Lines: 57 Satyam Sharma wrote: > drivers/scsi/ips.c: In function ?ips_register_scsi?: > drivers/scsi/ips.c:6869: > warning: ignoring return value of ?scsi_add_host?, declared with attribute warn_unused_result > > scsi_add_host() is __must_check, so let's check it's return and cleanup > appropriately on errors. > > Signed-off-by: Satyam Sharma > > --- > > drivers/scsi/ips.c | 7 ++++++- > 1 file changed, 6 insertions(+), 1 deletion(-) > > --- linux-2.6.23-rc4-mm1/drivers/scsi/ips.c~fix 2007-09-02 20:21:27.000000000 +0530 > +++ linux-2.6.23-rc4-mm1/drivers/scsi/ips.c 2007-09-02 20:25:58.000000000 +0530 > @@ -6866,7 +6866,12 @@ ips_register_scsi(int index) > sh->max_channel = ha->nbus - 1; > sh->can_queue = ha->max_cmds - 1; > > - scsi_add_host(sh, NULL); > + if (scsi_add_host(sh, NULL)) { > + IPS_PRINTK(KERN_WARNING, ha->pcidev, "Unable to add SCSI host\n"); > + free_irq(ha->irq, ha); > + scsi_host_put(sh); > + return -1; ACK, as long as you add a comment to this code and the request_irq() failure code: the oldha/old irq is not restored upon failure, in this function. That is a pre-existing bug, and not your fault, but your patch "continues in the same buggy tradition" :) We should at least note the FIXME at each error handling code branch. Ideally you or somebody should do a detailed analysis to a) (preferably) get rid of the silly oldha/ double-irq-request weirdness and make it look like other drivers, or, b) analyze the code and see what it takes to _really_ unwind the error. Also, I would recommend moving the error handling code to the end of the function and using the standard 'goto' approach for function error handling. This eliminates the duplicate scsi_host_put() calls upon error. Jeff - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/