lists.openwall.net   lists  /  announce  owl-users  owl-dev  john-users  john-dev  passwdqc-users  yescrypt  popa3d-users  /  oss-security  kernel-hardening  musl  sabotage  tlsify  passwords  /  crypt-dev  xvendor  /  Bugtraq  Full-Disclosure  linux-kernel  linux-netdev  linux-ext4  PHC 
Open Source and information security mailing list archives
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date:	Tue, 2 Mar 2010 19:37:42 -0600
From:	"Madhusudhan" <madhu.cr@...com>
To:	"'Thomas Gleixner'" <tglx@...utronix.de>,
	"'LKML'" <linux-kernel@...r.kernel.org>
Cc:	<linux-omap@...r.kernel.org>, <linux-mmc@...r.kernel.org>
Subject: RE: [PATCH] mmc: omap_hsmmc: Fix conditional locking



> -----Original Message-----
> From: linux-mmc-owner@...r.kernel.org [mailto:linux-mmc-
> owner@...r.kernel.org] On Behalf Of Thomas Gleixner
> Sent: Monday, March 01, 2010 1:02 PM
> To: LKML
> Cc: linux-omap@...r.kernel.org; linux-mmc@...r.kernel.org
> Subject: [PATCH] mmc: omap_hsmmc: Fix conditional locking
> 
> 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
> 

Can you please elaborate why the existing logic is broken?

It locks at the new request and unlocks just before issuing the cmd. Further
IRQ handler has these calls hence the !in_interrupt check.

How does this patch improve that? In fact with your patch for a data
transfer cmd there are several lock-unlock calls.

> Fix the mess by dropping host->irq_lock before calling
> mmc_request_done().
> 
> Signed-off-by: Thomas Gleixner <tglx@...utronix.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-mmc" in
> the body of a message to majordomo@...r.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

Powered by blists - more mailing lists