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

[Dan Carpenter]

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@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/