Re: [PATCH] mmc: omap_hsmmc: Fix conditional locking

[Adrian Hunter <adrian.hunter@nokia.com>]

Thomas Gleixner wrote:
> Conditional locking on (!in_interrupt()) is broken by design and there
> is no reason to keep the host->irq_lock across the call to
> mmc_request_done(). Also the host->protect_card magic hack does not
> depend on the context

Card protect does depend on not being invoked in the middle of a request.
For example, it could result in a open-ended read being started but never
stopped.

in_interrupt() can be replaced with a new flag 'in_request' that is set
in omap_hsmmc_request() and cleared after mmc_request_done()

Also, as you allude to in another email, the spinlock, while preventing an
oops, does not stop the unwanted interrupt, which will cause problems.

I can make the necessary changes but it will be a few days before
I have the time.

> 
> Fix the mess by dropping host->irq_lock before calling
> mmc_request_done().
> 
> Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
> 
> diff --git a/drivers/mmc/host/omap_hsmmc.c b/drivers/mmc/host/omap_hsmmc.c
> index 4b23225..99a3383 100644
> --- a/drivers/mmc/host/omap_hsmmc.c
> +++ b/drivers/mmc/host/omap_hsmmc.c
> @@ -468,14 +468,6 @@ omap_hsmmc_start_command(struct omap_hsmmc_host *host, struct mmc_command *cmd,
>  	if (host->use_dma)
>  		cmdreg |= DMA_EN;
>  
> -	/*
> -	 * In an interrupt context (i.e. STOP command), the spinlock is unlocked
> -	 * by the interrupt handler, otherwise (i.e. for a new request) it is
> -	 * unlocked here.
> -	 */
> -	if (!in_interrupt())
> -		spin_unlock_irqrestore(&host->irq_lock, host->flags);
> -
>  	OMAP_HSMMC_WRITE(host->base, ARG, cmd->arg);
>  	OMAP_HSMMC_WRITE(host->base, CMD, cmdreg);
>  }
> @@ -506,7 +498,9 @@ omap_hsmmc_xfer_done(struct omap_hsmmc_host *host, struct mmc_data *data)
>  		}
>  
>  		host->mrq = NULL;
> +		spin_unlock(&host->irq_lock);
>  		mmc_request_done(host->mmc, mrq);
> +		spin_lock(&host->irq_lock);
>  		return;
>  	}
>  
> @@ -523,7 +517,9 @@ omap_hsmmc_xfer_done(struct omap_hsmmc_host *host, struct mmc_data *data)
>  
>  	if (!data->stop) {
>  		host->mrq = NULL;
> +		spin_unlock(&host->irq_lock);
>  		mmc_request_done(host->mmc, data->mrq);
> +		spin_lock(&host->irq_lock);
>  		return;
>  	}
>  	omap_hsmmc_start_command(host, data->stop, NULL);
> @@ -551,7 +547,9 @@ omap_hsmmc_cmd_done(struct omap_hsmmc_host *host, struct mmc_command *cmd)
>  	}
>  	if ((host->data == NULL && !host->response_busy) || cmd->error) {
>  		host->mrq = NULL;
> +		spin_unlock(&host->irq_lock);
>  		mmc_request_done(host->mmc, cmd->mrq);
> +		spin_lock(&host->irq_lock);
>  	}
>  }
>  
> @@ -1077,37 +1075,31 @@ static void omap_hsmmc_request(struct mmc_host *mmc, struct mmc_request *req)
>  	struct omap_hsmmc_host *host = mmc_priv(mmc);
>  	int err;
>  
> +	spin_lock_irqsave(&host->irq_lock, host->flags);
>  	/*
> -	 * Prevent races with the interrupt handler because of unexpected
> -	 * interrupts, but not if we are already in interrupt context i.e.
> -	 * retries.
> +	 * Protect the card from I/O if there is a possibility
> +	 * it can be removed.
>  	 */
> -	if (!in_interrupt()) {
> -		spin_lock_irqsave(&host->irq_lock, host->flags);
> -		/*
> -		 * Protect the card from I/O if there is a possibility
> -		 * it can be removed.
> -		 */
> -		if (host->protect_card) {
> -			if (host->reqs_blocked < 3) {
> -				/*
> -				 * Ensure the controller is left in a consistent
> -				 * state by resetting the command and data state
> -				 * machines.
> -				 */
> -				omap_hsmmc_reset_controller_fsm(host, SRD);
> -				omap_hsmmc_reset_controller_fsm(host, SRC);
> -				host->reqs_blocked += 1;
> -			}
> -			req->cmd->error = -EBADF;
> -			if (req->data)
> -				req->data->error = -EBADF;
> -			spin_unlock_irqrestore(&host->irq_lock, host->flags);
> -			mmc_request_done(mmc, req);
> -			return;
> -		} else if (host->reqs_blocked)
> -			host->reqs_blocked = 0;
> -	}
> +	if (host->protect_card) {
> +		if (host->reqs_blocked < 3) {
> +			/*
> +			 * Ensure the controller is left in a consistent
> +			 * state by resetting the command and data state
> +			 * machines.
> +			 */
> +			omap_hsmmc_reset_controller_fsm(host, SRD);
> +			omap_hsmmc_reset_controller_fsm(host, SRC);
> +			host->reqs_blocked += 1;
> +		}
> +		req->cmd->error = -EBADF;
> +		if (req->data)
> +			req->data->error = -EBADF;
> +		spin_unlock_irqrestore(&host->irq_lock, host->flags);
> +		mmc_request_done(mmc, req);
> +		return;
> +	} else if (host->reqs_blocked)
> +		host->reqs_blocked = 0;
> +
>  	WARN_ON(host->mrq != NULL);
>  	host->mrq = req;
>  	err = omap_hsmmc_prepare_data(host, req);
> @@ -1116,13 +1108,13 @@ static void omap_hsmmc_request(struct mmc_host *mmc, struct mmc_request *req)
>  		if (req->data)
>  			req->data->error = err;
>  		host->mrq = NULL;
> -		if (!in_interrupt())
> -			spin_unlock_irqrestore(&host->irq_lock, host->flags);
> +		spin_unlock_irqrestore(&host->irq_lock, host->flags);
>  		mmc_request_done(mmc, req);
>  		return;
>  	}
>  
>  	omap_hsmmc_start_command(host, req->cmd, req->data);
> +	spin_unlock_irqrestore(&host->irq_lock, host->flags);
>  }
>  
>  /* Routine to configure clock values. Exposed API to core */
> --
> To unsubscribe from this list: send the line "unsubscribe linux-omap" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 
>