Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751611AbbD3M7b (ORCPT ); Thu, 30 Apr 2015 08:59:31 -0400 Received: from aserp1040.oracle.com ([141.146.126.69]:48979 "EHLO aserp1040.oracle.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750883AbbD3M7a (ORCPT ); Thu, 30 Apr 2015 08:59:30 -0400 Date: Thu, 30 Apr 2015 15:59:13 +0300 From: Dan Carpenter To: "J. German Rivera" Cc: gregkh@linuxfoundation.org, arnd@arndb.de, devel@driverdev.osuosl.org, linux-kernel@vger.kernel.org, stuart.yoder@freescale.com, bhupesh.sharma@freescale.com, agraf@suse.de, bhamciu1@freescale.com, nir.erez@freescale.com, itai.katz@freescale.com, scottwood@freescale.com, R89243@freescale.com, richard.schmitt@freescale.com Subject: Re: [PATCH 6/7] staging: fsl-mc: Add locking to serialize mc_send_command() calls Message-ID: <20150430125913.GY14154@mwanda> References: <1430242750-17745-1-git-send-email-German.Rivera@freescale.com> <1430242750-17745-7-git-send-email-German.Rivera@freescale.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1430242750-17745-7-git-send-email-German.Rivera@freescale.com> User-Agent: Mutt/1.5.21 (2010-09-15) X-Source-IP: userv0021.oracle.com [156.151.31.71] Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 1678 Lines: 60 On Tue, Apr 28, 2015 at 12:39:09PM -0500, J. German Rivera wrote: > @@ -230,15 +235,26 @@ static inline enum mc_cmd_status mc_read_response(struct mc_command __iomem * > * @cmd: command to be sent > * > * Returns '0' on Success; Error code otherwise. > - * > - * NOTE: This function cannot be invoked from from atomic contexts. > */ > int mc_send_command(struct fsl_mc_io *mc_io, struct mc_command *cmd) > { > + int error; > enum mc_cmd_status status; > unsigned long jiffies_until_timeout = > jiffies + MC_CMD_COMPLETION_TIMEOUT_JIFFIES; We busy loop while holding a spinlock for half a second. That seems bad. > > + if (preemptible()) { This is wrong. If the user asked for spinlocks they should always get spinlocks. It shouldn't matter that they are not currently holding a different lock. I'm skeptical of this locking anyway. Also what about if they have PREEMPT disabled? There aren't any users for this stuff anyway so it's impossible to review how people are FSL_MC_IO_ATOMIC_CONTEXT_PORTAL. Let's wait until there is a user before looking at this. > - return mc_status_to_error(status); > + error = mc_status_to_error(status); > + goto common_exit; > } > > - return 0; > + error = 0; > + > +common_exit: Just name this unlock:. > + if (preemptible()) > + mutex_unlock(&mc_io->mutex); > + else > + spin_unlock(&mc_io->spinlock); > + > + return error; > } regards, dan carpenter -- 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/