Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751822AbbEEQUb (ORCPT ); Tue, 5 May 2015 12:20:31 -0400 Received: from mail-bl2on0106.outbound.protection.outlook.com ([65.55.169.106]:6272 "EHLO na01-bl2-obe.outbound.protection.outlook.com" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1751564AbbEEQU1 convert rfc822-to-8bit (ORCPT ); Tue, 5 May 2015 12:20:27 -0400 From: Jose Rivera To: Dan Carpenter CC: "gregkh@linuxfoundation.org" , "arnd@arndb.de" , "devel@driverdev.osuosl.org" , "linux-kernel@vger.kernel.org" , Stuart Yoder , "bhupesh.sharma@freescale.com" , "agraf@suse.de" , "bhamciu1@freescale.com" , "nir.erez@freescale.com" , "itai.katz@freescale.com" , Scott Wood , "R89243@freescale.com" , Richard Schmitt Subject: RE: [PATCH 6/7] staging: fsl-mc: Add locking to serialize mc_send_command() calls Thread-Topic: [PATCH 6/7] staging: fsl-mc: Add locking to serialize mc_send_command() calls Thread-Index: AQHQgdtbo4Lz0RrVsU6vJBGZpRS+eJ1lhzKAgAgTKRA= Date: Tue, 5 May 2015 16:20:25 +0000 Message-ID: References: <1430242750-17745-1-git-send-email-German.Rivera@freescale.com> <1430242750-17745-7-git-send-email-German.Rivera@freescale.com> <20150430125913.GY14154@mwanda> In-Reply-To: <20150430125913.GY14154@mwanda> Accept-Language: en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: authentication-results: oracle.com; dkim=none (message not signed) header.d=none; x-originating-ip: [192.88.168.49] x-microsoft-antispam: UriScan:;BCL:0;PCL:0;RULEID:;SRVR:DM2PR0301MB0656; x-microsoft-antispam-prvs: x-exchange-antispam-report-test: UriScan:; x-exchange-antispam-report-cfa-test: BCL:0;PCL:0;RULEID:(601004)(5005006)(3002001);SRVR:DM2PR0301MB0656;BCL:0;PCL:0;RULEID:;SRVR:DM2PR0301MB0656; x-forefront-prvs: 0567A15835 x-forefront-antispam-report: SFV:NSPM;SFS:(10019020)(6009001)(51704005)(377454003)(24454002)(13464003)(76576001)(19580395003)(122556002)(87936001)(46102003)(19580405001)(106116001)(66066001)(2656002)(50986999)(86362001)(74316001)(76176999)(54356999)(33656002)(62966003)(2900100001)(102836002)(99286002)(5001960100002)(110136002)(77156002)(92566002)(5001920100001)(40100003)(107886002)(2950100001)(4001430100001);DIR:OUT;SFP:1102;SCL:1;SRVR:DM2PR0301MB0656;H:DM2PR0301MB1309.namprd03.prod.outlook.com;FPR:;SPF:None;MLV:sfv;LANG:en; Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 8BIT MIME-Version: 1.0 X-OriginatorOrg: freescale.com X-MS-Exchange-CrossTenant-originalarrivaltime: 05 May 2015 16:20:25.8051 (UTC) X-MS-Exchange-CrossTenant-fromentityheader: Hosted X-MS-Exchange-CrossTenant-id: 710a03f5-10f6-4d38-9ff4-a80b81da590d X-MS-Exchange-Transport-CrossTenantHeadersStamped: DM2PR0301MB0656 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 2407 Lines: 76 > -----Original Message----- > From: Dan Carpenter [mailto:dan.carpenter@oracle.com] > Sent: Thursday, April 30, 2015 7:59 AM > To: Rivera Jose-B46482 > Cc: gregkh@linuxfoundation.org; arnd@arndb.de; > devel@driverdev.osuosl.org; linux-kernel@vger.kernel.org; Yoder Stuart- > B08248; Sharma Bhupesh-B45370; agraf@suse.de; Hamciuc Bogdan-BHAMCIU1; > Erez Nir-RM30794; katz Itai-RM05202; Wood Scott-B07421; Marginean > Alexandru-R89243; Schmitt Richard-B43082 > Subject: Re: [PATCH 6/7] staging: fsl-mc: Add locking to serialize > mc_send_command() calls > > 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. > What would be a reasonable max time for holding a spinlock here? > > > > + 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/