[Regression Resend] mmc: mx28: sd card detection broken since 3.18-rc1

[Regression Resend] mmc: mx28: sd card detection broken since 3.18-rc1

[Stefan Wahren]

Hi,

i was testing Linux Kernel 3.18-rc2 with my i.MX28 board (I2SE Duckbill) and ran
into the problem that the sd card isn't detected from the Kernel at booting
(driver: mxs-mmc.c). That results in a endless wait for the root partition

Here are the relevant messages (bad case):

[...]
[    1.501883] mxs-mmc 80010000.ssp: initialized
[    1.521203] TCP: cubic registered
[    1.530850] NET: Registered protocol family 10
[    1.548469] mip6: Mobile IPv6
[    1.551698] sit: IPv6 over IPv4 tunneling driver
[    1.566016] ip6_gre: GRE over IPv6 tunneling driver
[    1.575831] NET: Registered protocol family 17
[    1.581640] bridge: automatic filtering via arp/ip/ip6tables has been
deprecated. Update your scripts to load br_netfilter if you need this.
[    1.595635] Key type dns_resolver registered
[    1.604302] registered taskstats version 1
[    1.618188] stmp3xxx-rtc 80056000.rtc: setting system clock to 1970-01-01
00:00:03 UTC (3)
[    1.675580] Waiting for root device /dev/mmcblk0p3...

In Linux Kernel 3.17 that problem didn't exist (good case):

[...]
[    1.546857] mxs-mmc 80010000.ssp: initialized
[    1.576363] TCP: cubic registered
[    1.588856] NET: Registered protocol family 10
[    1.608208] mmc0: host does not support reading read-only switch. assuming
write-enable.
[    1.616927] mip6: Mobile IPv6
[    1.620028] sit: IPv6 over IPv4 tunneling driver
[    1.629900] mmc0: new high speed SDHC card at address 0007
[    1.642901] ip6_gre: GRE over IPv6 tunneling driver
[    1.652047] mmcblk0: mmc0:0007 SD16G 14.6 GiB
[    1.662108] NET: Registered protocol family 17
[    1.678091]  mmcblk0: p1 p2 p3
[...]

I've have bisected the problem to this commit:

commit 89168b48991537bec2573b3b6a8841df74465b12
Author: Linus Walleij <linus.walleij@linaro.org>
Date:   Thu Oct 2 09:08:46 2014 +0200

    mmc: core: restore detect line inversion semantics
   
    commit 98e90de99a0c43bd434da814c882c4332441871e
    "mmc: host: switch OF parser to use gpio descriptors"
    switched the semantic behaviour of card detect and read
    only flags such that the inversion capability flag would
    only be set if inversion was explicitly specified in the
    device tree, in the hopes that no-one was using double
    inversion.
   
    It turns out that the XOR:ing between the explicit
    inversion was indeed in use, so we need to restore the
    old semantics where both ways of inversion are checked
    and the end result XOR:ed.
   
    Reported-by: Javier Martinez Canillas <javier@dowhile0.org>
    Tested-by: Javier Martinez Canillas <javier@dowhile0.org>
    Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
    Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>

Kernel command line: -e noinitrd console=ttyAMA0,115200 root=/dev/mmcblk0p3 rw
rootwait

It looks to me that the patch didn't fix all host controller.

BR Stefan

[Regression Resend] mmc: mx28: sd card detection broken since 3.18-rc1

[Kristina Martšenko]

On 01/11/14 23:40, Stefan Wahren wrote:
> Hi,
> 
> i was testing Linux Kernel 3.18-rc2 with my i.MX28 board (I2SE Duckbill) and ran
> into the problem that the sd card isn't detected from the Kernel at booting
> (driver: mxs-mmc.c). That results in a endless wait for the root partition
> 
> Here are the relevant messages (bad case):
> 
> [...]
> [    1.501883] mxs-mmc 80010000.ssp: initialized
> [    1.521203] TCP: cubic registered
> [    1.530850] NET: Registered protocol family 10
> [    1.548469] mip6: Mobile IPv6
> [    1.551698] sit: IPv6 over IPv4 tunneling driver
> [    1.566016] ip6_gre: GRE over IPv6 tunneling driver
> [    1.575831] NET: Registered protocol family 17
> [    1.581640] bridge: automatic filtering via arp/ip/ip6tables has been
> deprecated. Update your scripts to load br_netfilter if you need this.
> [    1.595635] Key type dns_resolver registered
> [    1.604302] registered taskstats version 1
> [    1.618188] stmp3xxx-rtc 80056000.rtc: setting system clock to 1970-01-01
> 00:00:03 UTC (3)
> [    1.675580] Waiting for root device /dev/mmcblk0p3...
> 
> In Linux Kernel 3.17 that problem didn't exist (good case):
> 
> [...]
> [    1.546857] mxs-mmc 80010000.ssp: initialized
> [    1.576363] TCP: cubic registered
> [    1.588856] NET: Registered protocol family 10
> [    1.608208] mmc0: host does not support reading read-only switch. assuming
> write-enable.
> [    1.616927] mip6: Mobile IPv6
> [    1.620028] sit: IPv6 over IPv4 tunneling driver
> [    1.629900] mmc0: new high speed SDHC card at address 0007
> [    1.642901] ip6_gre: GRE over IPv6 tunneling driver
> [    1.652047] mmcblk0: mmc0:0007 SD16G 14.6 GiB
> [    1.662108] NET: Registered protocol family 17
> [    1.678091]  mmcblk0: p1 p2 p3
> [...]
> 
> I've have bisected the problem to this commit:
> 
> commit 89168b48991537bec2573b3b6a8841df74465b12
> Author: Linus Walleij <linus.walleij@linaro.org>
> Date:   Thu Oct 2 09:08:46 2014 +0200
> 
>     mmc: core: restore detect line inversion semantics
>    
>     commit 98e90de99a0c43bd434da814c882c4332441871e
>     "mmc: host: switch OF parser to use gpio descriptors"
>     switched the semantic behaviour of card detect and read
>     only flags such that the inversion capability flag would
>     only be set if inversion was explicitly specified in the
>     device tree, in the hopes that no-one was using double
>     inversion.
>    
>     It turns out that the XOR:ing between the explicit
>     inversion was indeed in use, so we need to restore the
>     old semantics where both ways of inversion are checked
>     and the end result XOR:ed.
>    
>     Reported-by: Javier Martinez Canillas <javier@dowhile0.org>
>     Tested-by: Javier Martinez Canillas <javier@dowhile0.org>
>     Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
>     Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
> 
> Kernel command line: -e noinitrd console=ttyAMA0,115200 root=/dev/mmcblk0p3 rw
> rootwait
> 
> It looks to me that the patch didn't fix all host controller.

I ran into this issue as well. Seems that a card-detect flag 
(MMC_CAP2_CD_ACTIVE_HIGH) can currently be set based on an 
uninitialized variable, which can lead to the card being reported as 
not present. This patch fixes it for me:

diff --git a/drivers/mmc/core/host.c b/drivers/mmc/core/host.c
index 03c53b72a2d6..f0e187682d3b 100644
--- a/drivers/mmc/core/host.c
+++ b/drivers/mmc/core/host.c
@@ -311,7 +311,7 @@ int mmc_of_parse(struct mmc_host *host)
 	struct device_node *np;
 	u32 bus_width;
 	int len, ret;
-	bool cap_invert, gpio_invert;
+	bool cap_invert, gpio_invert = false;
 
 	if (!host->parent || !host->parent->of_node)
 		return 0;
@@ -401,6 +401,7 @@ int mmc_of_parse(struct mmc_host *host)
 	else
 		cap_invert = false;
 
+	gpio_invert = false;
 	ret = mmc_gpiod_request_ro(host, "wp", 0, false, 0, &gpio_invert);
 	if (ret) {
 		if (ret == -EPROBE_DEFER)

Let me know if this also fixes it for you, and I'll send in a proper 
patch.

Thanks,
Kristina

[Regression Resend] mmc: mx28: sd card detection broken since 3.18-rc1

[Fabio Estevam]

Hi Kristina,

On Sun, Nov 2, 2014 at 9:01 PM, Kristina Mart?enko
<kristina.martsenko@gmail.com> wrote:

> I ran into this issue as well. Seems that a card-detect flag
> (MMC_CAP2_CD_ACTIVE_HIGH) can currently be set based on an
> uninitialized variable, which can lead to the card being reported as
> not present. This patch fixes it for me:

Your patch fixes the card detection issue on mx28.

When you submit the patch formally, you can add my:

Tested-by: Fabio Estevam <fabio.estevam@freescale.com>

Thanks

[Regression Resend] mmc: mx28: sd card detection broken since 3.18-rc1

[Stefan Wahren]

Hello Kristina,

Am 03.11.2014 um 00:01 schrieb Kristina Mart?enko:
> [...]
> I ran into this issue as well. Seems that a card-detect flag 
> (MMC_CAP2_CD_ACTIVE_HIGH) can currently be set based on an 
> uninitialized variable, which can lead to the card being reported as 
> not present. This patch fixes it for me:
>
> diff --git a/drivers/mmc/core/host.c b/drivers/mmc/core/host.c
> index 03c53b72a2d6..f0e187682d3b 100644
> --- a/drivers/mmc/core/host.c
> +++ b/drivers/mmc/core/host.c
> @@ -311,7 +311,7 @@ int mmc_of_parse(struct mmc_host *host)
>  	struct device_node *np;
>  	u32 bus_width;
>  	int len, ret;
> -	bool cap_invert, gpio_invert;
> +	bool cap_invert, gpio_invert = false;
>  
>  	if (!host->parent || !host->parent->of_node)
>  		return 0;
> @@ -401,6 +401,7 @@ int mmc_of_parse(struct mmc_host *host)
>  	else
>  		cap_invert = false;
>  
> +	gpio_invert = false;
>  	ret = mmc_gpiod_request_ro(host, "wp", 0, false, 0, &gpio_invert);
>  	if (ret) {
>  		if (ret == -EPROBE_DEFER)
>
> Let me know if this also fixes it for you, and I'll send in a proper 
> patch.

the patch works for me too. You can also add:

Tested-by: Stefan Wahren <stefan.wahren@i2se.com>

> Thanks,
> Kristina

Thanks a lot
Stefan

[Regression Resend] mmc: mx28: sd card detection broken since 3.18-rc1

[Michael Heimpold]

Hi,

Am Montag, 3. November 2014, 01:01:07 schrieben Sie:
> On 01/11/14 23:40, Stefan Wahren wrote:
> > Hi,
> > 
> > i was testing Linux Kernel 3.18-rc2 with my i.MX28 board (I2SE Duckbill) and ran
> > into the problem that the sd card isn't detected from the Kernel at booting
> > (driver: mxs-mmc.c). That results in a endless wait for the root partition
> > 
> > Here are the relevant messages (bad case):
> > 
> > [...]
> > [    1.501883] mxs-mmc 80010000.ssp: initialized
> > [    1.521203] TCP: cubic registered
> > [    1.530850] NET: Registered protocol family 10
> > [    1.548469] mip6: Mobile IPv6
> > [    1.551698] sit: IPv6 over IPv4 tunneling driver
> > [    1.566016] ip6_gre: GRE over IPv6 tunneling driver
> > [    1.575831] NET: Registered protocol family 17
> > [    1.581640] bridge: automatic filtering via arp/ip/ip6tables has been
> > deprecated. Update your scripts to load br_netfilter if you need this.
> > [    1.595635] Key type dns_resolver registered
> > [    1.604302] registered taskstats version 1
> > [    1.618188] stmp3xxx-rtc 80056000.rtc: setting system clock to 1970-01-01
> > 00:00:03 UTC (3)
> > [    1.675580] Waiting for root device /dev/mmcblk0p3...
> > 
> > In Linux Kernel 3.17 that problem didn't exist (good case):
> > 
> > [...]
> > [    1.546857] mxs-mmc 80010000.ssp: initialized
> > [    1.576363] TCP: cubic registered
> > [    1.588856] NET: Registered protocol family 10
> > [    1.608208] mmc0: host does not support reading read-only switch. assuming
> > write-enable.
> > [    1.616927] mip6: Mobile IPv6
> > [    1.620028] sit: IPv6 over IPv4 tunneling driver
> > [    1.629900] mmc0: new high speed SDHC card at address 0007
> > [    1.642901] ip6_gre: GRE over IPv6 tunneling driver
> > [    1.652047] mmcblk0: mmc0:0007 SD16G 14.6 GiB
> > [    1.662108] NET: Registered protocol family 17
> > [    1.678091]  mmcblk0: p1 p2 p3
> > [...]
> > 
> > I've have bisected the problem to this commit:
> > 
> > commit 89168b48991537bec2573b3b6a8841df74465b12
> > Author: Linus Walleij <linus.walleij@linaro.org>
> > Date:   Thu Oct 2 09:08:46 2014 +0200
> > 
> >     mmc: core: restore detect line inversion semantics
> >    
> >     commit 98e90de99a0c43bd434da814c882c4332441871e
> >     "mmc: host: switch OF parser to use gpio descriptors"
> >     switched the semantic behaviour of card detect and read
> >     only flags such that the inversion capability flag would
> >     only be set if inversion was explicitly specified in the
> >     device tree, in the hopes that no-one was using double
> >     inversion.
> >    
> >     It turns out that the XOR:ing between the explicit
> >     inversion was indeed in use, so we need to restore the
> >     old semantics where both ways of inversion are checked
> >     and the end result XOR:ed.
> >    
> >     Reported-by: Javier Martinez Canillas <javier@dowhile0.org>
> >     Tested-by: Javier Martinez Canillas <javier@dowhile0.org>
> >     Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
> >     Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
> > 
> > Kernel command line: -e noinitrd console=ttyAMA0,115200 root=/dev/mmcblk0p3 rw
> > rootwait
> > 
> > It looks to me that the patch didn't fix all host controller.
> 
> I ran into this issue as well. Seems that a card-detect flag 
> (MMC_CAP2_CD_ACTIVE_HIGH) can currently be set based on an 
> uninitialized variable, which can lead to the card being reported as 
> not present. This patch fixes it for me:
> 
> diff --git a/drivers/mmc/core/host.c b/drivers/mmc/core/host.c
> index 03c53b72a2d6..f0e187682d3b 100644
> --- a/drivers/mmc/core/host.c
> +++ b/drivers/mmc/core/host.c
> @@ -311,7 +311,7 @@ int mmc_of_parse(struct mmc_host *host)
>  	struct device_node *np;
>  	u32 bus_width;
>  	int len, ret;
> -	bool cap_invert, gpio_invert;
> +	bool cap_invert, gpio_invert = false;
>  

sorry, but I don't understand how your patch fixes the problem.

First use of the gpio_invert bool is in line 370/371 within mmc_gpiod_request_cd
(re-wrapped into a single line here for better reading):

-snip-
ret = mmc_gpiod_request_cd(host, "cd", 0, true,  0, &gpio_invert);
-snap-

A pointer to the bool is passed, and inside mmc_gpiod_request_cd (drivers/mmc/core/slot-gpio.c,
line 322), always a value is assigned:

-snip-
if (gpio_invert)
                *gpio_invert = !gpiod_is_active_low(desc);
-snap-

So returning to mmc_of_parse, the bool should always have an initialized value.
Apart from some error handling, the bool is used immediately in the xor expression
and results in setting MMC_CAP2_CD_ACTIVE_HIGH bit, or not.

I also cannot see a code path, where gpio_invert is used without a call to mmc_gpiod_request_cd.

Would be nice, if you could point me to what I'm missing.

>  	if (!host->parent || !host->parent->of_node)
>  		return 0;
> @@ -401,6 +401,7 @@ int mmc_of_parse(struct mmc_host *host)
>  	else
>  		cap_invert = false;
>  
> +	gpio_invert = false;
>  	ret = mmc_gpiod_request_ro(host, "wp", 0, false, 0, &gpio_invert);

Same here. The functions always assigns a value when a pointer is given.
(And this change is unrelated to the reporters problem, so should be fixed with a
dedicated patch.)


Thanks,
Michael


>  	if (ret) {
>  		if (ret == -EPROBE_DEFER)
> 
> Let me know if this also fixes it for you, and I'll send in a proper 
> patch.
> 
> Thanks,
> Kristina
> --
> To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
> the body of a message to majordomo at vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

[Regression Resend] mmc: mx28: sd card detection broken since 3.18-rc1

[Kristina Martšenko]

On 03/11/14 22:49, Michael Heimpold wrote:
> Hi,

Hi Michael,

> Am Montag, 3. November 2014, 01:01:07 schrieben Sie:
>> On 01/11/14 23:40, Stefan Wahren wrote:
>>> Hi,
>>>
>>> i was testing Linux Kernel 3.18-rc2 with my i.MX28 board (I2SE Duckbill) and ran
>>> into the problem that the sd card isn't detected from the Kernel at booting
>>> (driver: mxs-mmc.c). That results in a endless wait for the root partition
>>
>> I ran into this issue as well. Seems that a card-detect flag 
>> (MMC_CAP2_CD_ACTIVE_HIGH) can currently be set based on an 
>> uninitialized variable, which can lead to the card being reported as 
>> not present. This patch fixes it for me:
>>
>> diff --git a/drivers/mmc/core/host.c b/drivers/mmc/core/host.c
>> index 03c53b72a2d6..f0e187682d3b 100644
>> --- a/drivers/mmc/core/host.c
>> +++ b/drivers/mmc/core/host.c
>> @@ -311,7 +311,7 @@ int mmc_of_parse(struct mmc_host *host)
>>  	struct device_node *np;
>>  	u32 bus_width;
>>  	int len, ret;
>> -	bool cap_invert, gpio_invert;
>> +	bool cap_invert, gpio_invert = false;
>>  
> 
> sorry, but I don't understand how your patch fixes the problem.
> 
> First use of the gpio_invert bool is in line 370/371 within mmc_gpiod_request_cd
> (re-wrapped into a single line here for better reading):
> 
> -snip-
> ret = mmc_gpiod_request_cd(host, "cd", 0, true,  0, &gpio_invert);
> -snap-
> 
> A pointer to the bool is passed, and inside mmc_gpiod_request_cd (drivers/mmc/core/slot-gpio.c,
> line 322), always a value is assigned:
> 
> -snip-
> if (gpio_invert)
>                 *gpio_invert = !gpiod_is_active_low(desc);
> -snap-
> 
> So returning to mmc_of_parse, the bool should always have an initialized value.
> Apart from some error handling, the bool is used immediately in the xor expression
> and results in setting MMC_CAP2_CD_ACTIVE_HIGH bit, or not.
> 
> I also cannot see a code path, where gpio_invert is used without a call to mmc_gpiod_request_cd.
> 
> Would be nice, if you could point me to what I'm missing.

mmc_gpiod_request_cd can return without ever reaching the line where 
the value is assigned to gpio_invert:

desc = devm_gpiod_get_index(host->parent, con_id, idx, GPIOD_IN);
if (IS_ERR(desc))
	return PTR_ERR(desc);

This can happen when the host controller doesn't use a GPIO for card 
detection (but instead uses a dedicated pin). In this case 
devm_gpiod_get_index will return -ENOENT.

>>  	if (!host->parent || !host->parent->of_node)
>>  		return 0;
>> @@ -401,6 +401,7 @@ int mmc_of_parse(struct mmc_host *host)
>>  	else
>>  		cap_invert = false;
>>  
>> +	gpio_invert = false;
>>  	ret = mmc_gpiod_request_ro(host, "wp", 0, false, 0, &gpio_invert);
> 
> Same here. The functions always assigns a value when a pointer is given.

Same thing, the function can return before gpio_invert is ever assigned 
to.

> (And this change is unrelated to the reporters problem, so should be fixed with a
> dedicated patch.)

Since both flags are set wrong for the exact same reason, and they're 
both regressions in 3.18, I thought it would make sense to fix them 
both in one patch. (Especially since I think it's cleaner to split the 
gpio_invert variable into cd_gpio_invert and ro_gpio_invert, and I was 
going to do that in the patch I'd send.) Let me know if you still think 
I should send separate patches for them.

Thanks for the review,
Kristina

[Regression Resend] mmc: mx28: sd card detection broken since 3.18-rc1

[Michael Heimpold]

Hi Kristina,

Am Montag, 3. November 2014, 23:50:53 schrieb Kristina Mart?enko:
> On 03/11/14 22:49, Michael Heimpold wrote:
> > Hi,
> 
> Hi Michael,
> 
> > Am Montag, 3. November 2014, 01:01:07 schrieben Sie:
> >> On 01/11/14 23:40, Stefan Wahren wrote:
> >>> Hi,
> >>>
> >>> i was testing Linux Kernel 3.18-rc2 with my i.MX28 board (I2SE Duckbill) and ran
> >>> into the problem that the sd card isn't detected from the Kernel at booting
> >>> (driver: mxs-mmc.c). That results in a endless wait for the root partition
> >>
> >> I ran into this issue as well. Seems that a card-detect flag 
> >> (MMC_CAP2_CD_ACTIVE_HIGH) can currently be set based on an 
> >> uninitialized variable, which can lead to the card being reported as 
> >> not present. This patch fixes it for me:
> >>
> >> diff --git a/drivers/mmc/core/host.c b/drivers/mmc/core/host.c
> >> index 03c53b72a2d6..f0e187682d3b 100644
> >> --- a/drivers/mmc/core/host.c
> >> +++ b/drivers/mmc/core/host.c
> >> @@ -311,7 +311,7 @@ int mmc_of_parse(struct mmc_host *host)
> >>  	struct device_node *np;
> >>  	u32 bus_width;
> >>  	int len, ret;
> >> -	bool cap_invert, gpio_invert;
> >> +	bool cap_invert, gpio_invert = false;
> >>  
> > 
> > sorry, but I don't understand how your patch fixes the problem.
> > 
> > First use of the gpio_invert bool is in line 370/371 within mmc_gpiod_request_cd
> > (re-wrapped into a single line here for better reading):
> > 
> > -snip-
> > ret = mmc_gpiod_request_cd(host, "cd", 0, true,  0, &gpio_invert);
> > -snap-
> > 
> > A pointer to the bool is passed, and inside mmc_gpiod_request_cd (drivers/mmc/core/slot-gpio.c,
> > line 322), always a value is assigned:
> > 
> > -snip-
> > if (gpio_invert)
> >                 *gpio_invert = !gpiod_is_active_low(desc);
> > -snap-
> > 
> > So returning to mmc_of_parse, the bool should always have an initialized value.
> > Apart from some error handling, the bool is used immediately in the xor expression
> > and results in setting MMC_CAP2_CD_ACTIVE_HIGH bit, or not.
> > 
> > I also cannot see a code path, where gpio_invert is used without a call to mmc_gpiod_request_cd.
> > 
> > Would be nice, if you could point me to what I'm missing.
> 
> mmc_gpiod_request_cd can return without ever reaching the line where 
> the value is assigned to gpio_invert:
> 
> desc = devm_gpiod_get_index(host->parent, con_id, idx, GPIOD_IN);
> if (IS_ERR(desc))
> 	return PTR_ERR(desc);
> 
> This can happen when the host controller doesn't use a GPIO for card 
> detection (but instead uses a dedicated pin). In this case 
> devm_gpiod_get_index will return -ENOENT.
> 

ah, I see - thank you for taking the time to explain.

> >>  	if (!host->parent || !host->parent->of_node)
> >>  		return 0;
> >> @@ -401,6 +401,7 @@ int mmc_of_parse(struct mmc_host *host)
> >>  	else
> >>  		cap_invert = false;
> >>  
> >> +	gpio_invert = false;
> >>  	ret = mmc_gpiod_request_ro(host, "wp", 0, false, 0, &gpio_invert);
> > 
> > Same here. The functions always assigns a value when a pointer is given.
> 
> Same thing, the function can return before gpio_invert is ever assigned 
> to.
> 
> > (And this change is unrelated to the reporters problem, so should be fixed with a
> > dedicated patch.)
> 
> Since both flags are set wrong for the exact same reason, and they're 
> both regressions in 3.18, I thought it would make sense to fix them 
> both in one patch. (Especially since I think it's cleaner to split the 
> gpio_invert variable into cd_gpio_invert and ro_gpio_invert, and I was 
> going to do that in the patch I'd send.) Let me know if you still think 
> I should send separate patches for them.
 
Splitting the variable sounds good to me and will improve readability. In this case
(and with proper commit message) I think it's really possible to combine both fixes
in one patch.

Thanks,
Michael

[Regression Resend] mmc: mx28: sd card detection broken since 3.18-rc1

[Linus Walleij]

On Mon, Nov 3, 2014 at 12:01 AM, Kristina Mart?enko
<kristina.martsenko@gmail.com> wrote:

> I ran into this issue as well. Seems that a card-detect flag
> (MMC_CAP2_CD_ACTIVE_HIGH) can currently be set based on an
> uninitialized variable, which can lead to the card being reported as
> not present. This patch fixes it for me:
>
> diff --git a/drivers/mmc/core/host.c b/drivers/mmc/core/host.c
> index 03c53b72a2d6..f0e187682d3b 100644
> --- a/drivers/mmc/core/host.c
> +++ b/drivers/mmc/core/host.c
> @@ -311,7 +311,7 @@ int mmc_of_parse(struct mmc_host *host)
>         struct device_node *np;
>         u32 bus_width;
>         int len, ret;
> -       bool cap_invert, gpio_invert;
> +       bool cap_invert, gpio_invert = false;
>
>         if (!host->parent || !host->parent->of_node)
>                 return 0;
> @@ -401,6 +401,7 @@ int mmc_of_parse(struct mmc_host *host)
>         else
>                 cap_invert = false;
>
> +       gpio_invert = false;
>         ret = mmc_gpiod_request_ro(host, "wp", 0, false, 0, &gpio_invert);
>         if (ret) {
>                 if (ret == -EPROBE_DEFER)
>
> Let me know if this also fixes it for you, and I'll send in a proper
> patch.

Argh how could I make this stupid mistake :(

Reviewed-by: Linus Walleij <linus.walleij@linaro.org>

Yours,
Linus Walleij

[PATCH] mmc: core: fix card detection regression

[Kristina Martšenko]

Since commit 89168b489915 ("mmc: core: restore detect line inversion
semantics"), the SD card on i.MX28 (and possibly other) devices isn't
detected and booting stops at:

[    4.120617] Waiting for root device /dev/mmcblk0p3...

This is caused by the MMC_CAP2_CD_ACTIVE_HIGH flag being set incorrectly
when the host controller doesn't use a GPIO for card detection (but
instead uses a dedicated pin). In this case mmc_gpiod_request_cd() will
return before assigning to the gpio_invert variable, leaving the
variable uninitialized. The variable then gets used to set the flag.
This patch fixes the issue by making sure gpio_invert is set to false
when a GPIO isn't used. After this patch, i.MX28 boots fine.

The MMC_CAP2_RO_ACTIVE_HIGH (write protect) flag is also set incorrectly
for the exact same reason (it uses the same uninitialized variable), so
this patch fixes that too.

Fixes: 89168b489915 ("mmc: core: restore detect line inversion semantics")
Reported-by: Stefan Wahren <stefan.wahren@i2se.com>
Signed-off-by: Kristina Mart?enko <kristina.martsenko@gmail.com>
---
This is a different version of the previous patch [1], so I haven't
included the Reviewed-by and Tested-by's. Would be nice if someone could
test this version as well.

[1] http://thread.gmane.org/gmane.linux.kernel.gpio/4609/focus=29576

 drivers/mmc/core/host.c | 21 ++++++++-------------
 1 file changed, 8 insertions(+), 13 deletions(-)

diff --git a/drivers/mmc/core/host.c b/drivers/mmc/core/host.c
index 03c53b72a2d6..270d58a4c43d 100644
--- a/drivers/mmc/core/host.c
+++ b/drivers/mmc/core/host.c
@@ -311,7 +311,8 @@ int mmc_of_parse(struct mmc_host *host)
 	struct device_node *np;
 	u32 bus_width;
 	int len, ret;
-	bool cap_invert, gpio_invert;
+	bool cd_cap_invert, cd_gpio_invert = false;
+	bool ro_cap_invert, ro_gpio_invert = false;
 
 	if (!host->parent || !host->parent->of_node)
 		return 0;
@@ -359,16 +360,13 @@ int mmc_of_parse(struct mmc_host *host)
 	if (of_find_property(np, "non-removable", &len)) {
 		host->caps |= MMC_CAP_NONREMOVABLE;
 	} else {
-		if (of_property_read_bool(np, "cd-inverted"))
-			cap_invert = true;
-		else
-			cap_invert = false;
+		cd_cap_invert = of_property_read_bool(np, "cd-inverted");
 
 		if (of_find_property(np, "broken-cd", &len))
 			host->caps |= MMC_CAP_NEEDS_POLL;
 
 		ret = mmc_gpiod_request_cd(host, "cd", 0, true,
-					   0, &gpio_invert);
+					   0, &cd_gpio_invert);
 		if (ret) {
 			if (ret == -EPROBE_DEFER)
 				return ret;
@@ -391,17 +389,14 @@ int mmc_of_parse(struct mmc_host *host)
 		 * both inverted, the end result is that the CD line is
 		 * not inverted.
 		 */
-		if (cap_invert ^ gpio_invert)
+		if (cd_cap_invert ^ cd_gpio_invert)
 			host->caps2 |= MMC_CAP2_CD_ACTIVE_HIGH;
 	}
 
 	/* Parse Write Protection */
-	if (of_property_read_bool(np, "wp-inverted"))
-		cap_invert = true;
-	else
-		cap_invert = false;
+	ro_cap_invert = of_property_read_bool(np, "wp-inverted");
 
-	ret = mmc_gpiod_request_ro(host, "wp", 0, false, 0, &gpio_invert);
+	ret = mmc_gpiod_request_ro(host, "wp", 0, false, 0, &ro_gpio_invert);
 	if (ret) {
 		if (ret == -EPROBE_DEFER)
 			goto out;
@@ -414,7 +409,7 @@ int mmc_of_parse(struct mmc_host *host)
 		dev_info(host->parent, "Got WP GPIO\n");
 
 	/* See the comment on CD inversion above */
-	if (cap_invert ^ gpio_invert)
+	if (ro_cap_invert ^ ro_gpio_invert)
 		host->caps2 |= MMC_CAP2_RO_ACTIVE_HIGH;
 
 	if (of_find_property(np, "cap-sd-highspeed", &len))
-- 
2.0.2

[PATCH] mmc: core: fix card detection regression

[Fabio Estevam]

Hi Kristina,

On Tue, Nov 4, 2014 at 10:22 PM, Kristina Mart?enko
<kristina.martsenko@gmail.com> wrote:
> Since commit 89168b489915 ("mmc: core: restore detect line inversion
> semantics"), the SD card on i.MX28 (and possibly other) devices isn't
> detected and booting stops at:
>
> [    4.120617] Waiting for root device /dev/mmcblk0p3...
>
> This is caused by the MMC_CAP2_CD_ACTIVE_HIGH flag being set incorrectly
> when the host controller doesn't use a GPIO for card detection (but
> instead uses a dedicated pin). In this case mmc_gpiod_request_cd() will
> return before assigning to the gpio_invert variable, leaving the
> variable uninitialized. The variable then gets used to set the flag.
> This patch fixes the issue by making sure gpio_invert is set to false
> when a GPIO isn't used. After this patch, i.MX28 boots fine.
>
> The MMC_CAP2_RO_ACTIVE_HIGH (write protect) flag is also set incorrectly
> for the exact same reason (it uses the same uninitialized variable), so
> this patch fixes that too.
>
> Fixes: 89168b489915 ("mmc: core: restore detect line inversion semantics")
> Reported-by: Stefan Wahren <stefan.wahren@i2se.com>
> Signed-off-by: Kristina Mart?enko <kristina.martsenko@gmail.com>

This fixes mx28 card detection, thanks!

Tested-by: Fabio Estevam <fabio.estevam@freescale.com>

[PATCH] mmc: core: fix card detection regression

[Ulf Hansson]

On 5 November 2014 01:22, Kristina Mart?enko
<kristina.martsenko@gmail.com> wrote:
> Since commit 89168b489915 ("mmc: core: restore detect line inversion
> semantics"), the SD card on i.MX28 (and possibly other) devices isn't
> detected and booting stops at:
>
> [    4.120617] Waiting for root device /dev/mmcblk0p3...
>
> This is caused by the MMC_CAP2_CD_ACTIVE_HIGH flag being set incorrectly
> when the host controller doesn't use a GPIO for card detection (but
> instead uses a dedicated pin). In this case mmc_gpiod_request_cd() will
> return before assigning to the gpio_invert variable, leaving the
> variable uninitialized. The variable then gets used to set the flag.
> This patch fixes the issue by making sure gpio_invert is set to false
> when a GPIO isn't used. After this patch, i.MX28 boots fine.
>
> The MMC_CAP2_RO_ACTIVE_HIGH (write protect) flag is also set incorrectly
> for the exact same reason (it uses the same uninitialized variable), so
> this patch fixes that too.
>
> Fixes: 89168b489915 ("mmc: core: restore detect line inversion semantics")
> Reported-by: Stefan Wahren <stefan.wahren@i2se.com>
> Signed-off-by: Kristina Mart?enko <kristina.martsenko@gmail.com>

Thanks! Applied for fixes.

Kind regards
Uffe