[PATCH/RFC] mmc: sh_mmcif: Add exclusion between cmd and interrupt

[PATCH/RFC] mmc: sh_mmcif: Add exclusion between cmd and interrupt

[Yoshihiro Kaneko]

From: Kouichi Tomita <kouichi.tomita.yn@renesas.com>

A command end interrupt should not be processed between command issue
and setting of wait_for flag. It expects already the flag to be set.
Therefore the exclusive control was added.

Signed-off-by: Kouichi Tomita <kouichi.tomita.yn@renesas.com>
Signed-off-by: Yoshihiro Kaneko <ykaneko0929@gmail.com>
---

This patch is based on next branch of Chris Ball's mmc tree.

 drivers/mmc/host/sh_mmcif.c | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

diff --git a/drivers/mmc/host/sh_mmcif.c b/drivers/mmc/host/sh_mmcif.c
index 7d9d6a3..e5d0b42 100644
--- a/drivers/mmc/host/sh_mmcif.c
+++ b/drivers/mmc/host/sh_mmcif.c
@@ -875,6 +875,7 @@ static void sh_mmcif_start_cmd(struct sh_mmcif_host *host,
 	struct mmc_command *cmd = mrq->cmd;
 	u32 opc = cmd->opcode;
 	u32 mask;
+	unsigned long flags;
 
 	switch (opc) {
 	/* response busy check */
@@ -909,10 +910,12 @@ static void sh_mmcif_start_cmd(struct sh_mmcif_host *host,
 	/* set arg */
 	sh_mmcif_writel(host->addr, MMCIF_CE_ARG, cmd->arg);
 	/* set cmd */
+	spin_lock_irqsave(&host->lock, flags);
 	sh_mmcif_writel(host->addr, MMCIF_CE_CMD_SET, opc);
 
 	host->wait_for = MMCIF_WAIT_FOR_CMD;
 	schedule_delayed_work(&host->timeout_work, host->timeout);
+	spin_unlock_irqrestore(&host->lock, flags);
 }
 
 static void sh_mmcif_stop_cmd(struct sh_mmcif_host *host,
@@ -1171,6 +1174,12 @@ static irqreturn_t sh_mmcif_irqt(int irq, void *dev_id)
 	struct sh_mmcif_host *host = dev_id;
 	struct mmc_request *mrq;
 	bool wait = false;
+	unsigned long flags;
+	int wait_work;
+
+	spin_lock_irqsave(&host->lock, flags);
+	wait_work = host->wait_for;
+	spin_unlock_irqrestore(&host->lock, flags);
 
 	cancel_delayed_work_sync(&host->timeout_work);
 
@@ -1188,7 +1197,7 @@ static irqreturn_t sh_mmcif_irqt(int irq, void *dev_id)
 	 * All handlers return true, if processing continues, and false, if the
 	 * request has to be completed - successfully or not
 	 */
-	switch (host->wait_for) {
+	switch (wait_work) {
 	case MMCIF_WAIT_FOR_REQUEST:
 		/* We're too late, the timeout has already kicked in */
 		mutex_unlock(&host->thread_lock);
-- 
1.9.1

Re: [PATCH/RFC] mmc: sh_mmcif: Add exclusion between cmd and interrupt

[Sergei Shtylyov]

Hello.

On 02/15/2015 05:46 PM, Yoshihiro Kaneko wrote:

> From: Kouichi Tomita <kouichi.tomita.yn@renesas.com>

> A command end interrupt should not be processed between command issue
> and setting of wait_for flag. It expects already the flag to be set.
> Therefore the exclusive control was added.

> Signed-off-by: Kouichi Tomita <kouichi.tomita.yn@renesas.com>
> Signed-off-by: Yoshihiro Kaneko <ykaneko0929@gmail.com>
> ---

> This patch is based on next branch of Chris Ball's mmc tree.

>   drivers/mmc/host/sh_mmcif.c | 11 ++++++++++-
>   1 file changed, 10 insertions(+), 1 deletion(-)

> diff --git a/drivers/mmc/host/sh_mmcif.c b/drivers/mmc/host/sh_mmcif.c
> index 7d9d6a3..e5d0b42 100644
> --- a/drivers/mmc/host/sh_mmcif.c
> +++ b/drivers/mmc/host/sh_mmcif.c
[...]
> @@ -1171,6 +1174,12 @@ static irqreturn_t sh_mmcif_irqt(int irq, void *dev_id)
>   	struct sh_mmcif_host *host = dev_id;
>   	struct mmc_request *mrq;
>   	bool wait = false;
> +	unsigned long flags;
> +	int wait_work;
> +
> +	spin_lock_irqsave(&host->lock, flags);
> +	wait_work = host->wait_for;
> +	spin_unlock_irqrestore(&host->lock, flags);

    Locking don't seem to have much sense here, as the read is already atomic.

WBR, Sergei

Re: [PATCH/RFC] mmc: sh_mmcif: Add exclusion between cmd and interrupt

[Yoshihiro Kaneko]

Hello Sergei,

2015-02-16 1:02 GMT+09:00 Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>:
> Hello.
>
> On 02/15/2015 05:46 PM, Yoshihiro Kaneko wrote:
>
>> From: Kouichi Tomita <kouichi.tomita.yn@renesas.com>
>
>
>> A command end interrupt should not be processed between command issue
>> and setting of wait_for flag. It expects already the flag to be set.
>> Therefore the exclusive control was added.
>
>
>> Signed-off-by: Kouichi Tomita <kouichi.tomita.yn@renesas.com>
>> Signed-off-by: Yoshihiro Kaneko <ykaneko0929@gmail.com>
>> ---
>
>
>> This patch is based on next branch of Chris Ball's mmc tree.
>
>
>>   drivers/mmc/host/sh_mmcif.c | 11 ++++++++++-
>>   1 file changed, 10 insertions(+), 1 deletion(-)
>
>
>> diff --git a/drivers/mmc/host/sh_mmcif.c b/drivers/mmc/host/sh_mmcif.c
>> index 7d9d6a3..e5d0b42 100644
>> --- a/drivers/mmc/host/sh_mmcif.c
>> +++ b/drivers/mmc/host/sh_mmcif.c
>
> [...]
>>
>> @@ -1171,6 +1174,12 @@ static irqreturn_t sh_mmcif_irqt(int irq, void
>> *dev_id)
>>         struct sh_mmcif_host *host = dev_id;
>>         struct mmc_request *mrq;
>>         bool wait = false;
>> +       unsigned long flags;
>> +       int wait_work;
>> +
>> +       spin_lock_irqsave(&host->lock, flags);
>> +       wait_work = host->wait_for;
>> +       spin_unlock_irqrestore(&host->lock, flags);
>
>
>    Locking don't seem to have much sense here, as the read is already
> atomic.

Thank you for your review.
I agree with you and I will remove this locking.


Thanks,
Kaneko

>
> WBR, Sergei
>

Re: [PATCH/RFC] mmc: sh_mmcif: Add exclusion between cmd and interrupt

[Simon Horman]

Hi all and in particular Sergei,

elsewhere in this thread Sergei indicated that he felt that
the spin lock around reading host->wait_for in sh_mmcif_irqt() doesn't
"seem to have much sense here, as the read is already atomic."

My initial reaction was that remark was correct. However, Kaneko-san
has done some further analysis which I would now like to share. My
apologies if in advance if I have misinterpreted that analysis.

On Sun, Feb 15, 2015 at 11:46:46PM +0900, Yoshihiro Kaneko wrote:
> From: Kouichi Tomita <kouichi.tomita.yn@renesas.com>
> 
> A command end interrupt should not be processed between command issue
> and setting of wait_for flag. It expects already the flag to be set.
> Therefore the exclusive control was added.
> 
> Signed-off-by: Kouichi Tomita <kouichi.tomita.yn@renesas.com>
> Signed-off-by: Yoshihiro Kaneko <ykaneko0929@gmail.com>
> ---
> 
> This patch is based on next branch of Chris Ball's mmc tree.
> 
>  drivers/mmc/host/sh_mmcif.c | 11 ++++++++++-
>  1 file changed, 10 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/mmc/host/sh_mmcif.c b/drivers/mmc/host/sh_mmcif.c
> index 7d9d6a3..e5d0b42 100644
> --- a/drivers/mmc/host/sh_mmcif.c
> +++ b/drivers/mmc/host/sh_mmcif.c
> @@ -875,6 +875,7 @@ static void sh_mmcif_start_cmd(struct sh_mmcif_host *host,
>  	struct mmc_command *cmd = mrq->cmd;
>  	u32 opc = cmd->opcode;
>  	u32 mask;
> +	unsigned long flags;
>  
>  	switch (opc) {
>  	/* response busy check */
> @@ -909,10 +910,12 @@ static void sh_mmcif_start_cmd(struct sh_mmcif_host *host,
>  	/* set arg */
>  	sh_mmcif_writel(host->addr, MMCIF_CE_ARG, cmd->arg);
>  	/* set cmd */

We could get an interrupt here.

> +	spin_lock_irqsave(&host->lock, flags);
>  	sh_mmcif_writel(host->addr, MMCIF_CE_CMD_SET, opc);
>  
>  	host->wait_for = MMCIF_WAIT_FOR_CMD;
>  	schedule_delayed_work(&host->timeout_work, host->timeout);
> +	spin_unlock_irqrestore(&host->lock, flags);
>  }
>  
>  static void sh_mmcif_stop_cmd(struct sh_mmcif_host *host,
> @@ -1171,6 +1174,12 @@ static irqreturn_t sh_mmcif_irqt(int irq, void *dev_id)
>  	struct sh_mmcif_host *host = dev_id;
>  	struct mmc_request *mrq;
>  	bool wait = false;
> +	unsigned long flags;
> +	int wait_work;
> +
> +	spin_lock_irqsave(&host->lock, flags);
> +	wait_work = host->wait_for;
> +	spin_unlock_irqrestore(&host->lock, flags);
>  
>  	cancel_delayed_work_sync(&host->timeout_work);

And without the locking that this patch proposes if an interrupt did occur
at the point noted above then we may cancel work that has not yet been
scheduled. And furthermore the work will subsequently be scheduled and
never canceled.

> @@ -1188,7 +1197,7 @@ static irqreturn_t sh_mmcif_irqt(int irq, void *dev_id)
>  	 * All handlers return true, if processing continues, and false, if the
>  	 * request has to be completed - successfully or not
>  	 */
> -	switch (host->wait_for) {
> +	switch (wait_work) {

And without the locking proposed by this patch if an interrupt occurred
at the point annotated above then host->wait_for may be read before it
is set by sh_mmcif_start_cmd().

>  	case MMCIF_WAIT_FOR_REQUEST:
>  		/* We're too late, the timeout has already kicked in */
>  		mutex_unlock(&host->thread_lock);

Re: [PATCH/RFC] mmc: sh_mmcif: Add exclusion between cmd and interrupt

[Ulf Hansson]

On 15 February 2015 at 15:46, Yoshihiro Kaneko <ykaneko0929@gmail.com> wrote:
> From: Kouichi Tomita <kouichi.tomita.yn@renesas.com>
>
> A command end interrupt should not be processed between command issue
> and setting of wait_for flag. It expects already the flag to be set.
> Therefore the exclusive control was added.
>
> Signed-off-by: Kouichi Tomita <kouichi.tomita.yn@renesas.com>
> Signed-off-by: Yoshihiro Kaneko <ykaneko0929@gmail.com>

I followed the discussion for this patch, I decided to apply it as is, thanks!

Kind regards
Uffe

> ---
>
> This patch is based on next branch of Chris Ball's mmc tree.
>
>  drivers/mmc/host/sh_mmcif.c | 11 ++++++++++-
>  1 file changed, 10 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/mmc/host/sh_mmcif.c b/drivers/mmc/host/sh_mmcif.c
> index 7d9d6a3..e5d0b42 100644
> --- a/drivers/mmc/host/sh_mmcif.c
> +++ b/drivers/mmc/host/sh_mmcif.c
> @@ -875,6 +875,7 @@ static void sh_mmcif_start_cmd(struct sh_mmcif_host *host,
>         struct mmc_command *cmd = mrq->cmd;
>         u32 opc = cmd->opcode;
>         u32 mask;
> +       unsigned long flags;
>
>         switch (opc) {
>         /* response busy check */
> @@ -909,10 +910,12 @@ static void sh_mmcif_start_cmd(struct sh_mmcif_host *host,
>         /* set arg */
>         sh_mmcif_writel(host->addr, MMCIF_CE_ARG, cmd->arg);
>         /* set cmd */
> +       spin_lock_irqsave(&host->lock, flags);
>         sh_mmcif_writel(host->addr, MMCIF_CE_CMD_SET, opc);
>
>         host->wait_for = MMCIF_WAIT_FOR_CMD;
>         schedule_delayed_work(&host->timeout_work, host->timeout);
> +       spin_unlock_irqrestore(&host->lock, flags);
>  }
>
>  static void sh_mmcif_stop_cmd(struct sh_mmcif_host *host,
> @@ -1171,6 +1174,12 @@ static irqreturn_t sh_mmcif_irqt(int irq, void *dev_id)
>         struct sh_mmcif_host *host = dev_id;
>         struct mmc_request *mrq;
>         bool wait = false;
> +       unsigned long flags;
> +       int wait_work;
> +
> +       spin_lock_irqsave(&host->lock, flags);
> +       wait_work = host->wait_for;
> +       spin_unlock_irqrestore(&host->lock, flags);
>
>         cancel_delayed_work_sync(&host->timeout_work);
>
> @@ -1188,7 +1197,7 @@ static irqreturn_t sh_mmcif_irqt(int irq, void *dev_id)
>          * All handlers return true, if processing continues, and false, if the
>          * request has to be completed - successfully or not
>          */
> -       switch (host->wait_for) {
> +       switch (wait_work) {
>         case MMCIF_WAIT_FOR_REQUEST:
>                 /* We're too late, the timeout has already kicked in */
>                 mutex_unlock(&host->thread_lock);
> --
> 1.9.1
>