RE: [PATCH 6/7] staging: fsl-mc: Add locking to serialize mc_send_command() calls

[Jose Rivera]

> -----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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/