* Re: [PATCH/RFC] mmc: sh_mmcif: Add exclusion between cmd and interrupt
2015-02-15 14:46 [PATCH/RFC] mmc: sh_mmcif: Add exclusion between cmd and interrupt Yoshihiro Kaneko
@ 2015-02-15 16:02 ` Sergei Shtylyov
2015-02-25 13:21 ` Yoshihiro Kaneko
2015-03-02 0:22 ` Simon Horman
2015-03-05 14:54 ` Ulf Hansson
2 siblings, 1 reply; 5+ messages in thread
From: Sergei Shtylyov @ 2015-02-15 16:02 UTC (permalink / raw)
To: Yoshihiro Kaneko, linux-mmc
Cc: Chris Ball, Ulf Hansson, Simon Horman, Magnus Damm,
Kuninori Morimoto, linux-sh
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
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH/RFC] mmc: sh_mmcif: Add exclusion between cmd and interrupt
2015-02-15 16:02 ` Sergei Shtylyov
@ 2015-02-25 13:21 ` Yoshihiro Kaneko
0 siblings, 0 replies; 5+ messages in thread
From: Yoshihiro Kaneko @ 2015-02-25 13:21 UTC (permalink / raw)
To: Sergei Shtylyov
Cc: linux-mmc, Chris Ball, Ulf Hansson, Simon Horman, Magnus Damm,
Kuninori Morimoto, Linux-sh list
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
>
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH/RFC] mmc: sh_mmcif: Add exclusion between cmd and interrupt
2015-02-15 14:46 [PATCH/RFC] mmc: sh_mmcif: Add exclusion between cmd and interrupt Yoshihiro Kaneko
2015-02-15 16:02 ` Sergei Shtylyov
@ 2015-03-02 0:22 ` Simon Horman
2015-03-05 14:54 ` Ulf Hansson
2 siblings, 0 replies; 5+ messages in thread
From: Simon Horman @ 2015-03-02 0:22 UTC (permalink / raw)
To: Yoshihiro Kaneko, Sergei Shtylyov
Cc: linux-mmc, Chris Ball, Ulf Hansson, Magnus Damm,
Kuninori Morimoto, linux-sh
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);
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH/RFC] mmc: sh_mmcif: Add exclusion between cmd and interrupt
2015-02-15 14:46 [PATCH/RFC] mmc: sh_mmcif: Add exclusion between cmd and interrupt Yoshihiro Kaneko
2015-02-15 16:02 ` Sergei Shtylyov
2015-03-02 0:22 ` Simon Horman
@ 2015-03-05 14:54 ` Ulf Hansson
2 siblings, 0 replies; 5+ messages in thread
From: Ulf Hansson @ 2015-03-05 14:54 UTC (permalink / raw)
To: Yoshihiro Kaneko
Cc: linux-mmc, Chris Ball, Simon Horman, Magnus Damm,
Kuninori Morimoto, Linux-sh list
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
>
^ permalink raw reply [flat|nested] 5+ messages in thread