| Message ID | 5456B7B3.60401@gmail.com |
|---|---|
| State | Not Applicable |
| Headers | show |
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 -- To unsubscribe from this list: send the line "unsubscribe linux-gpio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe linux-gpio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line "unsubscribe linux-gpio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe linux-gpio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe linux-gpio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe linux-gpio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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)