Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1756543AbbLAO57 (ORCPT ); Tue, 1 Dec 2015 09:57:59 -0500 Received: from mail1.bemta12.messagelabs.com ([216.82.251.1]:29186 "EHLO mail1.bemta12.messagelabs.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1755847AbbLAO56 (ORCPT ); Tue, 1 Dec 2015 09:57:58 -0500 X-Greylist: delayed 391 seconds by postgrey-1.27 at vger.kernel.org; Tue, 01 Dec 2015 09:57:58 EST X-Env-Sender: Benjamin.Romer@unisys.com X-Msg-Ref: server-4.tower-164.messagelabs.com!1448981485!2775373!2 X-Originating-IP: [192.61.61.104] X-StarScan-Received: X-StarScan-Version: 7.19.2; banners=-,-,- X-VirusChecked: Checked Subject: Re: [PATCH] staging: unisys: use common return path To: Dan Carpenter , Sudip Mukherjee References: <1448950555-8846-1-git-send-email-sudipm.mukherjee@gmail.com> <20151201080045.GE18797@mwanda> CC: David Kershner , Greg Kroah-Hartman , , , From: Ben Romer Message-ID: <565DB4BA.80208@unisys.com> Date: Tue, 1 Dec 2015 09:54:50 -0500 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.3.0 MIME-Version: 1.0 In-Reply-To: <20151201080045.GE18797@mwanda> Content-Type: text/plain; charset="windows-1252"; format=flowed Content-Transfer-Encoding: 7bit X-Originating-IP: [192.63.205.71] X-ClientProxiedBy: US-EXCH13-2.na.uis.unisys.com (129.224.78.76) To US-EXCH13-1.na.uis.unisys.com (129.224.78.75) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 1639 Lines: 51 On 12/01/2015 03:00 AM, Dan Carpenter wrote: > Doing One Err style error handling is often a mistake but it's ok here. Why is it okay here? I don't understand why this function would be any different than the other places where the code used a goto. If we *have* to change it I would prefer that we not add a goto and instead add an additional boolean local variable to control serverdown completion. That's less complex and makes the intent clear. like this: visornic_serverdown(struct visornic_devdata *devdata, visorbus_state_complete_func complete_func) { unsigned long flags; int retval = 0; bool complete_serverdown = false; spin_lock_irqsave(&devdata->priv_lock, flags); if (!devdata->server_down && !devdata->server_change_state) { if (devdata->going_away) { dev_dbg(&devdata->dev->device, "%s aborting because device removal pending\n", __func__); retval = -ENODEV; } else { devdata->server_change_state = true; devdata->server_down_complete_func = complete_func; complete_serverdown = true; } } else if (devdata->server_change_state) { dev_dbg(&devdata->dev->device, "%s changing state\n", __func__); spin_unlock_irqrestore(&devdata->priv_lock, flags); retval = -EINVAL; } spin_unlock_irqrestore(&devdata->priv_lock, flags); if (complete_serverdown) visornic_serverdown_complete(devdata); return retval; } -- Ben -- 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/