[PATCH v2] mmc: sdhci: fix incorrect command used in tuning

[PATCH v2] mmc: sdhci: fix incorrect command used in tuning

[Aaron Lu]

V2:
Fix for SDIO case: both SD and SDIO cards use cmd19 while eMMC use cmd21.

V1:
For SD hosts using retuning mode 1, when retuning timer expired, it will
need to do retuning in sdhci_request before processing the actual
request. But the retuning command is fixed: cmd19 for SD card and cmd21
for eMMC card, so we can't use the original request's command to do the
tuning.

And since the tuning command depends on the card type atteched to the
host, we will need to know the card type to use the correct tuning
command.

Cc: stable <stable@vger.kernel.org> [3.3+]
Signed-off-by: Aaron Lu <aaron.lu@amd.com>
---
 drivers/mmc/host/sdhci.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
index f76736b..4e53e6b 100644
--- a/drivers/mmc/host/sdhci.c
+++ b/drivers/mmc/host/sdhci.c
@@ -27,6 +27,7 @@
 
 #include <linux/mmc/mmc.h>
 #include <linux/mmc/host.h>
+#include <linux/mmc/card.h>
 
 #include "sdhci.h"
 
@@ -1245,6 +1246,7 @@ static void sdhci_request(struct mmc_host *mmc, struct mmc_request *mrq)
 	struct sdhci_host *host;
 	bool present;
 	unsigned long flags;
+	u32 tuning_opcode;
 
 	host = mmc_priv(mmc);
 
@@ -1292,8 +1294,12 @@ static void sdhci_request(struct mmc_host *mmc, struct mmc_request *mrq)
 		 */
 		if ((host->flags & SDHCI_NEEDS_RETUNING) &&
 		    !(present_state & (SDHCI_DOING_WRITE | SDHCI_DOING_READ))) {
+			/* eMMC uses cmd21 while sd and sdio use cmd19 */
+			tuning_opcode = mmc->card->type == MMC_TYPE_MMC ?
+				MMC_SEND_TUNING_BLOCK_HS200 :
+				MMC_SEND_TUNING_BLOCK;
 			spin_unlock_irqrestore(&host->lock, flags);
-			sdhci_execute_tuning(mmc, mrq->cmd->opcode);
+			sdhci_execute_tuning(mmc, tuning_opcode);
 			spin_lock_irqsave(&host->lock, flags);
 
 			/* Restore original mmc_request structure */
-- 
1.7.11.1.3.g4c8a9db

Re: [PATCH v2] mmc: sdhci: fix incorrect command used in tuning

[Girish K S]

On 3 July 2012 14:57, Aaron Lu <aaron.lu@amd.com> wrote:
> V2:
> Fix for SDIO case: both SD and SDIO cards use cmd19 while eMMC use cmd21.
>
> V1:
> For SD hosts using retuning mode 1, when retuning timer expired, it will
> need to do retuning in sdhci_request before processing the actual
> request. But the retuning command is fixed: cmd19 for SD card and cmd21
> for eMMC card, so we can't use the original request's command to do the
> tuning.
>
> And since the tuning command depends on the card type atteched to the
> host, we will need to know the card type to use the correct tuning
> command.
>
> Cc: stable <stable@vger.kernel.org> [3.3+]
> Signed-off-by: Aaron Lu <aaron.lu@amd.com>
> ---
>  drivers/mmc/host/sdhci.c | 8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
> index f76736b..4e53e6b 100644
> --- a/drivers/mmc/host/sdhci.c
> +++ b/drivers/mmc/host/sdhci.c
> @@ -27,6 +27,7 @@
>
>  #include <linux/mmc/mmc.h>
>  #include <linux/mmc/host.h>
> +#include <linux/mmc/card.h>
>
>  #include "sdhci.h"
>
> @@ -1245,6 +1246,7 @@ static void sdhci_request(struct mmc_host *mmc, struct mmc_request *mrq)
>         struct sdhci_host *host;
>         bool present;
>         unsigned long flags;
> +       u32 tuning_opcode;
>
>         host = mmc_priv(mmc);
>
> @@ -1292,8 +1294,12 @@ static void sdhci_request(struct mmc_host *mmc, struct mmc_request *mrq)
>                  */
>                 if ((host->flags & SDHCI_NEEDS_RETUNING) &&
>                     !(present_state & (SDHCI_DOING_WRITE | SDHCI_DOING_READ))) {
> +                       /* eMMC uses cmd21 while sd and sdio use cmd19 */
> +                       tuning_opcode = mmc->card->type == MMC_TYPE_MMC ?
> +                               MMC_SEND_TUNING_BLOCK_HS200 :
> +                               MMC_SEND_TUNING_BLOCK;
>                         spin_unlock_irqrestore(&host->lock, flags);
> -                       sdhci_execute_tuning(mmc, mrq->cmd->opcode);
> +                       sdhci_execute_tuning(mmc, tuning_opcode);
dont you think the previous implementation does the same. It is
already handled by introducing the 2nd parameter.
>                         spin_lock_irqsave(&host->lock, flags);
>
>                         /* Restore original mmc_request structure */
> --
> 1.7.11.1.3.g4c8a9db
>
>

Re: [PATCH v2] mmc: sdhci: fix incorrect command used in tuning

[Aaron Lu]

Hi,

On Tue, Jul 03, 2012 at 03:28:28PM +0530, Girish K S wrote:
> On 3 July 2012 14:57, Aaron Lu <aaron.lu@amd.com> wrote:
> > V2:
> > Fix for SDIO case: both SD and SDIO cards use cmd19 while eMMC use cmd21.
> >
> > V1:
> > For SD hosts using retuning mode 1, when retuning timer expired, it will
> > need to do retuning in sdhci_request before processing the actual
> > request. But the retuning command is fixed: cmd19 for SD card and cmd21
> > for eMMC card, so we can't use the original request's command to do the
> > tuning.
> >
> > And since the tuning command depends on the card type atteched to the
> > host, we will need to know the card type to use the correct tuning
> > command.
> >
> > Cc: stable <stable@vger.kernel.org> [3.3+]
> > Signed-off-by: Aaron Lu <aaron.lu@amd.com>
> > ---
> >  drivers/mmc/host/sdhci.c | 8 +++++++-
> >  1 file changed, 7 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
> > index f76736b..4e53e6b 100644
> > --- a/drivers/mmc/host/sdhci.c
> > +++ b/drivers/mmc/host/sdhci.c
> > @@ -27,6 +27,7 @@
> >
> >  #include <linux/mmc/mmc.h>
> >  #include <linux/mmc/host.h>
> > +#include <linux/mmc/card.h>
> >
> >  #include "sdhci.h"
> >
> > @@ -1245,6 +1246,7 @@ static void sdhci_request(struct mmc_host *mmc, struct mmc_request *mrq)
> >         struct sdhci_host *host;
> >         bool present;
> >         unsigned long flags;
> > +       u32 tuning_opcode;
> >
> >         host = mmc_priv(mmc);
> >
> > @@ -1292,8 +1294,12 @@ static void sdhci_request(struct mmc_host *mmc, struct mmc_request *mrq)
> >                  */
> >                 if ((host->flags & SDHCI_NEEDS_RETUNING) &&
> >                     !(present_state & (SDHCI_DOING_WRITE | SDHCI_DOING_READ))) {
> > +                       /* eMMC uses cmd21 while sd and sdio use cmd19 */
> > +                       tuning_opcode = mmc->card->type == MMC_TYPE_MMC ?
> > +                               MMC_SEND_TUNING_BLOCK_HS200 :
> > +                               MMC_SEND_TUNING_BLOCK;
> >                         spin_unlock_irqrestore(&host->lock, flags);
> > -                       sdhci_execute_tuning(mmc, mrq->cmd->opcode);
> > +                       sdhci_execute_tuning(mmc, tuning_opcode);
> dont you think the previous implementation does the same. It is
> already handled by introducing the 2nd parameter.

Suppose the following scenario:
mmc_start_request (e.g. mrq->cmd->opcode is 18 for this call)
  -> host->ops->request
  (sdhci's retuning timer expired, the flag SDHCI_NEEDS_RETUNING is set)
    -> sdhci_request
      -> sdhci_execute_tuning will be called before processing the
actual request due to retuning's requirement, but with the wrong command
opcode(cmd18) instead of cmd19 for sd/sdio or cmd21 for emmc.

The problem is with retuning, for normal explicit calls of
sdhci_execute_tuning, there is no problem with the code. But when
retuning is required, sdhci_execute_retuning will be executed implicitly
to the above layer and we have to use the right tuning command instead of
the current processing command, which can be any of the valid sd/sdio/mmc
commands.

-Aaron

> >                         spin_lock_irqsave(&host->lock, flags);
> >
> >                         /* Restore original mmc_request structure */
> > --
> > 1.7.11.1.3.g4c8a9db
> >
> >

Re: [PATCH v2] mmc: sdhci: fix incorrect command used in tuning

[Girish K S]

On 3 July 2012 17:01, Aaron Lu <aaron.lu@amd.com> wrote:
> Hi,
>
> On Tue, Jul 03, 2012 at 03:28:28PM +0530, Girish K S wrote:
>> On 3 July 2012 14:57, Aaron Lu <aaron.lu@amd.com> wrote:
>> > V2:
>> > Fix for SDIO case: both SD and SDIO cards use cmd19 while eMMC use cmd21.
>> >
>> > V1:
>> > For SD hosts using retuning mode 1, when retuning timer expired, it will
>> > need to do retuning in sdhci_request before processing the actual
>> > request. But the retuning command is fixed: cmd19 for SD card and cmd21
>> > for eMMC card, so we can't use the original request's command to do the
>> > tuning.
>> >
>> > And since the tuning command depends on the card type atteched to the
>> > host, we will need to know the card type to use the correct tuning
>> > command.
>> >
>> > Cc: stable <stable@vger.kernel.org> [3.3+]
>> > Signed-off-by: Aaron Lu <aaron.lu@amd.com>
>> > ---
>> >  drivers/mmc/host/sdhci.c | 8 +++++++-
>> >  1 file changed, 7 insertions(+), 1 deletion(-)
>> >
>> > diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
>> > index f76736b..4e53e6b 100644
>> > --- a/drivers/mmc/host/sdhci.c
>> > +++ b/drivers/mmc/host/sdhci.c
>> > @@ -27,6 +27,7 @@
>> >
>> >  #include <linux/mmc/mmc.h>
>> >  #include <linux/mmc/host.h>
>> > +#include <linux/mmc/card.h>
>> >
>> >  #include "sdhci.h"
>> >
>> > @@ -1245,6 +1246,7 @@ static void sdhci_request(struct mmc_host *mmc, struct mmc_request *mrq)
>> >         struct sdhci_host *host;
>> >         bool present;
>> >         unsigned long flags;
>> > +       u32 tuning_opcode;
>> >
>> >         host = mmc_priv(mmc);
>> >
>> > @@ -1292,8 +1294,12 @@ static void sdhci_request(struct mmc_host *mmc, struct mmc_request *mrq)
>> >                  */
>> >                 if ((host->flags & SDHCI_NEEDS_RETUNING) &&
>> >                     !(present_state & (SDHCI_DOING_WRITE | SDHCI_DOING_READ))) {
>> > +                       /* eMMC uses cmd21 while sd and sdio use cmd19 */
>> > +                       tuning_opcode = mmc->card->type == MMC_TYPE_MMC ?
>> > +                               MMC_SEND_TUNING_BLOCK_HS200 :
>> > +                               MMC_SEND_TUNING_BLOCK;
>> >                         spin_unlock_irqrestore(&host->lock, flags);
>> > -                       sdhci_execute_tuning(mmc, mrq->cmd->opcode);
>> > +                       sdhci_execute_tuning(mmc, tuning_opcode);
>> dont you think the previous implementation does the same. It is
>> already handled by introducing the 2nd parameter.
>
> Suppose the following scenario:
> mmc_start_request (e.g. mrq->cmd->opcode is 18 for this call)
>   -> host->ops->request
>   (sdhci's retuning timer expired, the flag SDHCI_NEEDS_RETUNING is set)
>     -> sdhci_request
>       -> sdhci_execute_tuning will be called before processing the
> actual request due to retuning's requirement, but with the wrong command
> opcode(cmd18) instead of cmd19 for sd/sdio or cmd21 for emmc.
>
> The problem is with retuning, for normal explicit calls of
> sdhci_execute_tuning, there is no problem with the code. But when
> retuning is required, sdhci_execute_retuning will be executed implicitly
> to the above layer and we have to use the right tuning command instead of
> the current processing command, which can be any of the valid sd/sdio/mmc
> commands.
Thanks for the explanation. is it possible to make a separate local
function for this. Since mmc_host_ops has a member .execute_tuning
which takes 2 parameters that are called from respective sd/mmc/sdio
card files. there might be conflict
>
> -Aaron
>
>> >                         spin_lock_irqsave(&host->lock, flags);
>> >
>> >                         /* Restore original mmc_request structure */
>> > --
>> > 1.7.11.1.3.g4c8a9db
>> >
>> >

Re: [PATCH v2] mmc: sdhci: fix incorrect command used in tuning

[Girish K S]

On 3 July 2012 17:27, Girish K S <girish.shivananjappa@linaro.org> wrote:
> On 3 July 2012 17:01, Aaron Lu <aaron.lu@amd.com> wrote:
>> Hi,
>>
>> On Tue, Jul 03, 2012 at 03:28:28PM +0530, Girish K S wrote:
>>> On 3 July 2012 14:57, Aaron Lu <aaron.lu@amd.com> wrote:
>>> > V2:
>>> > Fix for SDIO case: both SD and SDIO cards use cmd19 while eMMC use cmd21.
>>> >
>>> > V1:
>>> > For SD hosts using retuning mode 1, when retuning timer expired, it will
>>> > need to do retuning in sdhci_request before processing the actual
>>> > request. But the retuning command is fixed: cmd19 for SD card and cmd21
>>> > for eMMC card, so we can't use the original request's command to do the
>>> > tuning.
>>> >
>>> > And since the tuning command depends on the card type atteched to the
>>> > host, we will need to know the card type to use the correct tuning
>>> > command.
>>> >
>>> > Cc: stable <stable@vger.kernel.org> [3.3+]
>>> > Signed-off-by: Aaron Lu <aaron.lu@amd.com>
>>> > ---
>>> >  drivers/mmc/host/sdhci.c | 8 +++++++-
>>> >  1 file changed, 7 insertions(+), 1 deletion(-)
>>> >
>>> > diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
>>> > index f76736b..4e53e6b 100644
>>> > --- a/drivers/mmc/host/sdhci.c
>>> > +++ b/drivers/mmc/host/sdhci.c
>>> > @@ -27,6 +27,7 @@
>>> >
>>> >  #include <linux/mmc/mmc.h>
>>> >  #include <linux/mmc/host.h>
>>> > +#include <linux/mmc/card.h>
>>> >
>>> >  #include "sdhci.h"
>>> >
>>> > @@ -1245,6 +1246,7 @@ static void sdhci_request(struct mmc_host *mmc, struct mmc_request *mrq)
>>> >         struct sdhci_host *host;
>>> >         bool present;
>>> >         unsigned long flags;
>>> > +       u32 tuning_opcode;
>>> >
>>> >         host = mmc_priv(mmc);
>>> >
>>> > @@ -1292,8 +1294,12 @@ static void sdhci_request(struct mmc_host *mmc, struct mmc_request *mrq)
>>> >                  */
>>> >                 if ((host->flags & SDHCI_NEEDS_RETUNING) &&
>>> >                     !(present_state & (SDHCI_DOING_WRITE | SDHCI_DOING_READ))) {
>>> > +                       /* eMMC uses cmd21 while sd and sdio use cmd19 */
>>> > +                       tuning_opcode = mmc->card->type == MMC_TYPE_MMC ?
>>> > +                               MMC_SEND_TUNING_BLOCK_HS200 :
>>> > +                               MMC_SEND_TUNING_BLOCK;
>>> >                         spin_unlock_irqrestore(&host->lock, flags);
>>> > -                       sdhci_execute_tuning(mmc, mrq->cmd->opcode);
>>> > +                       sdhci_execute_tuning(mmc, tuning_opcode);
>>> dont you think the previous implementation does the same. It is
>>> already handled by introducing the 2nd parameter.
>>
>> Suppose the following scenario:
>> mmc_start_request (e.g. mrq->cmd->opcode is 18 for this call)
>>   -> host->ops->request
>>   (sdhci's retuning timer expired, the flag SDHCI_NEEDS_RETUNING is set)
>>     -> sdhci_request
>>       -> sdhci_execute_tuning will be called before processing the
>> actual request due to retuning's requirement, but with the wrong command
>> opcode(cmd18) instead of cmd19 for sd/sdio or cmd21 for emmc.
>>
>> The problem is with retuning, for normal explicit calls of
>> sdhci_execute_tuning, there is no problem with the code. But when
>> retuning is required, sdhci_execute_retuning will be executed implicitly
>> to the above layer and we have to use the right tuning command instead of
>> the current processing command, which can be any of the valid sd/sdio/mmc
>> commands.
> Thanks for the explanation. is it possible to make a separate local
> function for this. Since mmc_host_ops has a member .execute_tuning
> which takes 2 parameters that are called from respective sd/mmc/sdio
> card files. there might be conflict
sorry igonre it

>>
>> -Aaron
>>
>>> >                         spin_lock_irqsave(&host->lock, flags);
>>> >
>>> >                         /* Restore original mmc_request structure */
>>> > --
>>> > 1.7.11.1.3.g4c8a9db
>>> >
>>> >

Re: [PATCH v2] mmc: sdhci: fix incorrect command used in tuning

[Girish K S]

On 3 July 2012 17:27, Girish K S <girish.shivananjappa@linaro.org> wrote:
> On 3 July 2012 17:01, Aaron Lu <aaron.lu@amd.com> wrote:
>> Hi,
>>
>> On Tue, Jul 03, 2012 at 03:28:28PM +0530, Girish K S wrote:
>>> On 3 July 2012 14:57, Aaron Lu <aaron.lu@amd.com> wrote:
>>> > V2:
>>> > Fix for SDIO case: both SD and SDIO cards use cmd19 while eMMC use cmd21.
>>> >
>>> > V1:
>>> > For SD hosts using retuning mode 1, when retuning timer expired, it will
>>> > need to do retuning in sdhci_request before processing the actual
>>> > request. But the retuning command is fixed: cmd19 for SD card and cmd21
>>> > for eMMC card, so we can't use the original request's command to do the
>>> > tuning.
>>> >
>>> > And since the tuning command depends on the card type atteched to the
>>> > host, we will need to know the card type to use the correct tuning
>>> > command.
>>> >
>>> > Cc: stable <stable@vger.kernel.org> [3.3+]
>>> > Signed-off-by: Aaron Lu <aaron.lu@amd.com>
>>> > ---
>>> >  drivers/mmc/host/sdhci.c | 8 +++++++-
>>> >  1 file changed, 7 insertions(+), 1 deletion(-)
>>> >
>>> > diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
>>> > index f76736b..4e53e6b 100644
>>> > --- a/drivers/mmc/host/sdhci.c
>>> > +++ b/drivers/mmc/host/sdhci.c
>>> > @@ -27,6 +27,7 @@
>>> >
>>> >  #include <linux/mmc/mmc.h>
>>> >  #include <linux/mmc/host.h>
>>> > +#include <linux/mmc/card.h>
>>> >
>>> >  #include "sdhci.h"
>>> >
>>> > @@ -1245,6 +1246,7 @@ static void sdhci_request(struct mmc_host *mmc, struct mmc_request *mrq)
>>> >         struct sdhci_host *host;
>>> >         bool present;
>>> >         unsigned long flags;
>>> > +       u32 tuning_opcode;
>>> >
>>> >         host = mmc_priv(mmc);
>>> >
>>> > @@ -1292,8 +1294,12 @@ static void sdhci_request(struct mmc_host *mmc, struct mmc_request *mrq)
>>> >                  */
>>> >                 if ((host->flags & SDHCI_NEEDS_RETUNING) &&
>>> >                     !(present_state & (SDHCI_DOING_WRITE | SDHCI_DOING_READ))) {
>>> > +                       /* eMMC uses cmd21 while sd and sdio use cmd19 */
>>> > +                       tuning_opcode = mmc->card->type == MMC_TYPE_MMC ?
>>> > +                               MMC_SEND_TUNING_BLOCK_HS200 :
>>> > +                               MMC_SEND_TUNING_BLOCK;
>>> >                         spin_unlock_irqrestore(&host->lock, flags);
>>> > -                       sdhci_execute_tuning(mmc, mrq->cmd->opcode);
>>> > +                       sdhci_execute_tuning(mmc, tuning_opcode);
>>> dont you think the previous implementation does the same. It is
>>> already handled by introducing the 2nd parameter.
>>
>> Suppose the following scenario:
>> mmc_start_request (e.g. mrq->cmd->opcode is 18 for this call)
>>   -> host->ops->request
>>   (sdhci's retuning timer expired, the flag SDHCI_NEEDS_RETUNING is set)
>>     -> sdhci_request
>>       -> sdhci_execute_tuning will be called before processing the
>> actual request due to retuning's requirement, but with the wrong command
>> opcode(cmd18) instead of cmd19 for sd/sdio or cmd21 for emmc.
>>
>> The problem is with retuning, for normal explicit calls of
>> sdhci_execute_tuning, there is no problem with the code. But when
>> retuning is required, sdhci_execute_retuning will be executed implicitly
>> to the above layer and we have to use the right tuning command instead of
>> the current processing command, which can be any of the valid sd/sdio/mmc
>> commands.
> Thanks for the explanation. is it possible to make a separate local
> function for this. Since mmc_host_ops has a member .execute_tuning
> which takes 2 parameters that are called from respective sd/mmc/sdio
> card files. there might be conflict
Sorry i just read it wrongly (function has only 1 param).
>>
>> -Aaron
>>
>>> >                         spin_lock_irqsave(&host->lock, flags);
>>> >
>>> >                         /* Restore original mmc_request structure */
>>> > --
>>> > 1.7.11.1.3.g4c8a9db
>>> >
>>> >

Re: [PATCH v2] mmc: sdhci: fix incorrect command used in tuning

[Aaron Lu]

Hi,

On Tue, Jul 03, 2012 at 05:32:36PM +0530, Girish K S wrote:
> >>> > @@ -1292,8 +1294,12 @@ static void sdhci_request(struct mmc_host *mmc, struct mmc_request *mrq)
> >>> >                  */
> >>> >                 if ((host->flags & SDHCI_NEEDS_RETUNING) &&
> >>> >                     !(present_state & (SDHCI_DOING_WRITE | SDHCI_DOING_READ))) {
> >>> > +                       /* eMMC uses cmd21 while sd and sdio use cmd19 */
> >>> > +                       tuning_opcode = mmc->card->type == MMC_TYPE_MMC ?
> >>> > +                               MMC_SEND_TUNING_BLOCK_HS200 :
> >>> > +                               MMC_SEND_TUNING_BLOCK;
> >>> >                         spin_unlock_irqrestore(&host->lock, flags);
> >>> > -                       sdhci_execute_tuning(mmc, mrq->cmd->opcode);
> >>> > +                       sdhci_execute_tuning(mmc, tuning_opcode);
> >>> dont you think the previous implementation does the same. It is
> >>> already handled by introducing the 2nd parameter.
> >>
> >> Suppose the following scenario:
> >> mmc_start_request (e.g. mrq->cmd->opcode is 18 for this call)
> >>   -> host->ops->request
> >>   (sdhci's retuning timer expired, the flag SDHCI_NEEDS_RETUNING is set)
> >>     -> sdhci_request
> >>       -> sdhci_execute_tuning will be called before processing the
> >> actual request due to retuning's requirement, but with the wrong command
> >> opcode(cmd18) instead of cmd19 for sd/sdio or cmd21 for emmc.
> >>
> >> The problem is with retuning, for normal explicit calls of
> >> sdhci_execute_tuning, there is no problem with the code. But when
> >> retuning is required, sdhci_execute_retuning will be executed implicitly
> >> to the above layer and we have to use the right tuning command instead of
> >> the current processing command, which can be any of the valid sd/sdio/mmc
> >> commands.
> > Thanks for the explanation. is it possible to make a separate local
> > function for this. Since mmc_host_ops has a member .execute_tuning
> > which takes 2 parameters that are called from respective sd/mmc/sdio
> > card files. there might be conflict
> Sorry i just read it wrongly (function has only 1 param).

So do you have any other problems with this patch?
If not, can I have your ack on this patch?

Thanks,
Aaron

RE: [PATCH v2] mmc: sdhci: fix incorrect command used in tuning

[Philip Rakity]

Reviewed-by philip Rakity prakity@marvell.com
________________________________________
From: Aaron Lu [aaron.lu@amd.com]
Sent: Tuesday, July 03, 2012 2:27 AM
To: Girish K S; Subhash Jadavani; Philip Rakity; Chris Ball
Cc: linux-mmc@vger.kernel.org; Aaron Lu; Aaron Lu; stable; "[3.3+]"@domain.invalid
Subject: [PATCH v2] mmc: sdhci: fix incorrect command used in tuning

V2:
Fix for SDIO case: both SD and SDIO cards use cmd19 while eMMC use cmd21.

V1:
For SD hosts using retuning mode 1, when retuning timer expired, it will
need to do retuning in sdhci_request before processing the actual
request. But the retuning command is fixed: cmd19 for SD card and cmd21
for eMMC card, so we can't use the original request's command to do the
tuning.

And since the tuning command depends on the card type atteched to the
host, we will need to know the card type to use the correct tuning
command.

Cc: stable <stable@vger.kernel.org> [3.3+]
Signed-off-by: Aaron Lu <aaron.lu@amd.com>
---
 drivers/mmc/host/sdhci.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
index f76736b..4e53e6b 100644
--- a/drivers/mmc/host/sdhci.c
+++ b/drivers/mmc/host/sdhci.c
@@ -27,6 +27,7 @@

 #include <linux/mmc/mmc.h>
 #include <linux/mmc/host.h>
+#include <linux/mmc/card.h>

 #include "sdhci.h"

@@ -1245,6 +1246,7 @@ static void sdhci_request(struct mmc_host *mmc, struct mmc_request *mrq)
        struct sdhci_host *host;
        bool present;
        unsigned long flags;
+       u32 tuning_opcode;

        host = mmc_priv(mmc);

@@ -1292,8 +1294,12 @@ static void sdhci_request(struct mmc_host *mmc, struct mmc_request *mrq)
                 */
                if ((host->flags & SDHCI_NEEDS_RETUNING) &&
                    !(present_state & (SDHCI_DOING_WRITE | SDHCI_DOING_READ))) {
+                       /* eMMC uses cmd21 while sd and sdio use cmd19 */
+                       tuning_opcode = mmc->card->type == MMC_TYPE_MMC ?
+                               MMC_SEND_TUNING_BLOCK_HS200 :
+                               MMC_SEND_TUNING_BLOCK;
                        spin_unlock_irqrestore(&host->lock, flags);
-                       sdhci_execute_tuning(mmc, mrq->cmd->opcode);
+                       sdhci_execute_tuning(mmc, tuning_opcode);
                        spin_lock_irqsave(&host->lock, flags);

                        /* Restore original mmc_request structure */
--
1.7.11.1.3.g4c8a9db

Re: [PATCH v2] mmc: sdhci: fix incorrect command used in tuning

[Chris Ball]

Hi,

On Tue, Jul 03 2012, Aaron Lu wrote:
> V2:
> Fix for SDIO case: both SD and SDIO cards use cmd19 while eMMC use cmd21.
>
> V1:
> For SD hosts using retuning mode 1, when retuning timer expired, it will
> need to do retuning in sdhci_request before processing the actual
> request. But the retuning command is fixed: cmd19 for SD card and cmd21
> for eMMC card, so we can't use the original request's command to do the
> tuning.
>
> And since the tuning command depends on the card type atteched to the
> host, we will need to know the card type to use the correct tuning
> command.
>
> Cc: stable <stable@vger.kernel.org> [3.3+]
> Signed-off-by: Aaron Lu <aaron.lu@amd.com>
> ---

Thanks, pushed to mmc-next for 3.6 with Philip's Reviewed-by.

For next time, please put the patch changelog at this point in the patch
(after ---) so that it doesn't show up in the recorded commit message.

Also, please don't e-mail the patch to stable@ before it has entered
mainline.  They'll pick it up from the CC in the commit message when
Linus merges it, they don't want a copy of the patch before then.

Thanks!

- Chris.
-- 
Chris Ball   <cjb@laptop.org>   <http://printf.net/>
One Laptop Per Child

Re: [PATCH v2] mmc: sdhci: fix incorrect command used in tuning

[Aaron Lu]

Hi Chris,

On Tue, Jul 03, 2012 at 08:37:33PM -0400, Chris Ball wrote:
> Hi,
> 
> On Tue, Jul 03 2012, Aaron Lu wrote:
> > V2:
> > Fix for SDIO case: both SD and SDIO cards use cmd19 while eMMC use cmd21.
> >
> > V1:
> > For SD hosts using retuning mode 1, when retuning timer expired, it will
> > need to do retuning in sdhci_request before processing the actual
> > request. But the retuning command is fixed: cmd19 for SD card and cmd21
> > for eMMC card, so we can't use the original request's command to do the
> > tuning.
> >
> > And since the tuning command depends on the card type atteched to the
> > host, we will need to know the card type to use the correct tuning
> > command.
> >
> > Cc: stable <stable@vger.kernel.org> [3.3+]
> > Signed-off-by: Aaron Lu <aaron.lu@amd.com>
> > ---
> 
> Thanks, pushed to mmc-next for 3.6 with Philip's Reviewed-by.
> 
> For next time, please put the patch changelog at this point in the patch
> (after ---) so that it doesn't show up in the recorded commit message.
> 
> Also, please don't e-mail the patch to stable@ before it has entered
> mainline.  They'll pick it up from the CC in the commit message when
> Linus merges it, they don't want a copy of the patch before then.

OK, thanks for pointing this out, will follow this in the future.

-Aaron

> 
> Thanks!
> 
> - Chris.
> -- 
> Chris Ball   <cjb@laptop.org>   <http://printf.net/>
> One Laptop Per Child
> --
> To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Re: [PATCH v2] mmc: sdhci: fix incorrect command used in tuning

[Chris Ball]

Hi Aaron,

On Tue, Jul 03 2012, Aaron Lu wrote:
>  		 */
>  		if ((host->flags & SDHCI_NEEDS_RETUNING) &&
>  		    !(present_state & (SDHCI_DOING_WRITE | SDHCI_DOING_READ))) {
> +			/* eMMC uses cmd21 while sd and sdio use cmd19 */
> +			tuning_opcode = mmc->card->type == MMC_TYPE_MMC ?
> +				MMC_SEND_TUNING_BLOCK_HS200 :
> +				MMC_SEND_TUNING_BLOCK;

This is causing a NULL deref crash here when run on host controllers
with no card inserted -- mmc->card is NULL, as you'd expect, yet it's
dereferenced anyway.  Maybe the system you tested it on only has an
eMMC, so you never noticed that it crashes if there's no card present?
Or maybe it's abnormal for host controllers to set SDHCI_NEEDS_RETUNING
when there's no card present?

In any case, this has hit 3.[345]-stable now, so we're causing crashes
for people who weren't seeing a crash before they pulled -stable.  :(
The patch below just checks mmc->card before dereferencing it -- does
this look like the correct fix to you?


Subject: [PATCH] mmc: sdhci: Fix NULL dereference in sdhci_request() tuning code

Commit 473b095a72a9 ("mmc: sdhci: fix incorrect command used in tuning")
introduced a NULL dereference if an SD 3.0 host controller raises the
SDHCI_NEEDS_TUNING flag while no card is inserted.

Signed-off-by: Chris Ball <cjb@laptop.org>
---
 drivers/mmc/host/sdhci.c | 23 +++++++++++++----------
 1 file changed, 13 insertions(+), 10 deletions(-)

diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
index 07a5346..2e1175d 100644
--- a/drivers/mmc/host/sdhci.c
+++ b/drivers/mmc/host/sdhci.c
@@ -1294,16 +1294,19 @@ static void sdhci_request(struct mmc_host *mmc, struct mmc_request *mrq)
 		 */
 		if ((host->flags & SDHCI_NEEDS_RETUNING) &&
 		    !(present_state & (SDHCI_DOING_WRITE | SDHCI_DOING_READ))) {
-			/* eMMC uses cmd21 while sd and sdio use cmd19 */
-			tuning_opcode = mmc->card->type == MMC_TYPE_MMC ?
-				MMC_SEND_TUNING_BLOCK_HS200 :
-				MMC_SEND_TUNING_BLOCK;
-			spin_unlock_irqrestore(&host->lock, flags);
-			sdhci_execute_tuning(mmc, tuning_opcode);
-			spin_lock_irqsave(&host->lock, flags);
-
-			/* Restore original mmc_request structure */
-			host->mrq = mrq;
+			if (mmc->card) {
+				/* eMMC uses cmd21 but sd and sdio use cmd19 */
+				tuning_opcode =
+					mmc->card->type == MMC_TYPE_MMC ?
+					MMC_SEND_TUNING_BLOCK_HS200 :
+					MMC_SEND_TUNING_BLOCK;
+				spin_unlock_irqrestore(&host->lock, flags);
+				sdhci_execute_tuning(mmc, tuning_opcode);
+				spin_lock_irqsave(&host->lock, flags);
+
+				/* Restore original mmc_request structure */
+				host->mrq = mrq;
+			}
 		}
 
 		if (mrq->sbc && !(host->flags & SDHCI_AUTO_CMD23))
-- 
Chris Ball   <cjb@laptop.org>   <http://printf.net/>
One Laptop Per Child

Re: [PATCH v2] mmc: sdhci: fix incorrect command used in tuning

[Chris Ball]

Hi Aaron,

On Mon, Nov 05 2012, Chris Ball wrote:
> On Tue, Jul 03 2012, Aaron Lu wrote:
>>  		 */
>>  		if ((host->flags & SDHCI_NEEDS_RETUNING) &&
>>  		    !(present_state & (SDHCI_DOING_WRITE | SDHCI_DOING_READ))) {
>> +			/* eMMC uses cmd21 while sd and sdio use cmd19 */
>> +			tuning_opcode = mmc->card->type == MMC_TYPE_MMC ?
>> +				MMC_SEND_TUNING_BLOCK_HS200 :
>> +				MMC_SEND_TUNING_BLOCK;
>
> This is causing a NULL deref crash here when run on host controllers
> with no card inserted -- mmc->card is NULL, as you'd expect, yet it's
> dereferenced anyway.  Maybe the system you tested it on only has an
> eMMC, so you never noticed that it crashes if there's no card present?
> Or maybe it's abnormal for host controllers to set SDHCI_NEEDS_RETUNING
> when there's no card present?

I'm going to send my fix to Linus (and hence stable@) tomorrow morning,
since it's important to remove this crash.  Let me know what you think
when you get a chance.

Thanks,

- Chris.
-- 
Chris Ball   <cjb@laptop.org>   <http://printf.net/>
One Laptop Per Child