> -----Original Message-----
> From: Dan Carpenter [mailto:dan.carpen...@oracle.com]
> Sent: Thursday, April 30, 2015 7:59 AM
> To: Rivera Jose-B46482
> Cc: gre...@linuxfoundation.org; a...@arndb.de;
> de...@driverdev.osuosl.org; linux-ker...@vger.kernel.org; Yoder Stuart-
> B08248; Sharma Bhupesh-B45370; ag...@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
_______________________________________________
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
