> -----Original Message----- > From: Dan Carpenter [mailto:dan.carpenter@xxxxxxxxxx] > Sent: Thursday, April 30, 2015 7:59 AM > To: Rivera Jose-B46482 > Cc: gregkh@xxxxxxxxxxxxxxxxxxx; arnd@xxxxxxxx; > devel@xxxxxxxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx; Yoder Stuart- > B08248; Sharma Bhupesh-B45370; agraf@xxxxxxx; 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 _______________________________________________ devel mailing list devel@xxxxxxxxxxxxxxxxxxxxxx http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
