regression v4.16 on Nokia N900:/dev/input/event6 aka AV Jack support disappeared

Hi!

In v4.16, AV jack support disappeared.

This one is suspect:

commit 7be4b5dc7ffa9499ac6ef33a5ffa9ff43f9b7057

Author: Andrew F. Davis <[email protected]>
Date:   Wed Nov 29 11:13:59 2017 -0600

ARM: dts: omap3-n900: Fix the audio CODEC's reset pin

The correct DT property for specifying a GPIO used for reset
is "reset-gpios", fix this here.

Fixes 14e3e295b2b9 ("ARM: dts: omap3-n900: Add TLV320AIC3X

support")

Signed-off-by Andrew F. Davis <[email protected]>

Signed-off-by Tony Lindgren <[email protected]>

How was it tested? It reverts polarity of reset pin, but sound/soc/codecs/tlv320aic3x.c treats those as aliases:

ret = of_get_named_gpio(np, "reset-gpios", 0);

 if (ret >= 0) {
    aic3x->gpio_reset = ret;
 } else {
 ret = of_get_named_gpio(np, "gpio-reset", 0);
 if (ret > 0) {
    dev_warn(&i2c->dev, "Using deprecated property \"gpio-r\eset\", please update your DT");
    aic3x->gpio_reset = ret;


									Pavel

JFYI: This issues is tracked in the regression reports for Linux 4.16
(http://bit.ly/lnxregrep416 ) with this id:

Linux-Regression-ID: lr#4b650f

Hi!

Quoted 3 lines

JFYI: This issues is tracked in the regression reports for Linux 4.16
(http://bit.ly/lnxregrep416 ) with this id:

Quoted 1 line

Linux-Regression-ID: lr#4b650f

Ok, so it seems that issue is bigger: whole sound subsystem does not work. /proc/asound/cards is empty.

7e6127c1240ed569cdda2a67c8f03836f9f28c05 seems to be bad already.

I tried to revert sound/soc changes, and sound is broken, too. Nasty

Quoted 10 lines · Pavel Machek

Hi!

Quoted 4 lines

JFYI: This issues is tracked in the regression reports for Linux 4.16
(http://bit.ly/lnxregrep416 ) with this id:

Linux-Regression-ID: lr#4b650f

Ok, so it seems that issue is bigger: whole sound subsystem does not work. /proc/asound/cards is empty.

Quoted 3 lines

7e6127c1240ed569cdda2a67c8f03836f9f28c05 seems to be bad already.

I tried to revert sound/soc changes, and sound is broken, too. Nasty

dmesg log?

Quoted 8 lines

Quoted 8 lines

Quoted 4 lines

JFYI: This issues is tracked in the regression reports for Linux 4.16
(http://bit.ly/lnxregrep416 ) with this id:

Linux-Regression-ID: lr#4b650f

Ok, so it seems that issue is bigger: whole sound subsystem does not work. /proc/asound/cards is empty.

Quoted 6 lines

Quoted 3 lines

7e6127c1240ed569cdda2a67c8f03836f9f28c05 seems to be bad already.

I tried to revert sound/soc changes, and sound is broken, too. Nasty

dmesg log?

Partial dmesg is at:
https://github.com/pavelmachek/missy/blob/master/db/phone/nokia/n900/pavel/2018.1291171648263/dmesg.out

I should be able to get full one...

I did git bisect, and the winner seems to be:

pavel@duo:/data/l/linux-n900$ git bisect bad
c85823390215e52d68d3826df92a447ed31e5c80 is the first bad commit
commit c85823390215e52d68d3826df92a447ed31e5c80
Author: Linus Walleij <[email protected]>
Date:   Wed Dec 27 16:37:44 2017 +0100

I did git bisect, and the winner seems to be:

pavel at duo:/data/l/linux-n900$ git bisect bad c85823390215e52d68d3826df92a447ed31e5c80 is the first bad commit commit c85823390215e52d68d3826df92a447ed31e5c80

Author: Linus Walleij <[email protected]>
Date:   Wed Dec 27 16:37:44 2017 +0100

gpio: of: Support SPI nonstandard GPIO properties

Quoted 8 lines · Pavel Machek

I did git bisect, and the winner seems to be:

pavel@duo:/data/l/linux-n900$ git bisect bad
c85823390215e52d68d3826df92a447ed31e5c80 is the first bad commit
commit c85823390215e52d68d3826df92a447ed31e5c80
Author: Linus Walleij <[email protected]>
Date:   Wed Dec 27 16:37:44 2017 +0100

Quoted 1 line

gpio: of: Support SPI nonstandard GPIO properties

I have fixes queued for this, tried to send a pull request yesterday but it turns out the fixes need fixing... OK I'm onto it anyways.

Linux-Regression-ID: lr#4b650f

Quoted 12 lines · Linus Walleij

Quoted 9 lines · Pavel Machek

I did git bisect, and the winner seems to be:

pavel at duo:/data/l/linux-n900$ git bisect bad c85823390215e52d68d3826df92a447ed31e5c80 is the first bad commit commit c85823390215e52d68d3826df92a447ed31e5c80

Author: Linus Walleij <[email protected]>
Date:   Wed Dec 27 16:37:44 2017 +0100

gpio: of: Support SPI nonstandard GPIO properties

Quoted 2 lines

I have fixes queued for this, tried to send a pull request yesterday but it turns out the fixes need fixing... OK I'm onto it anyways.

Do you have any updates? Is there way to fix dts so that this does not trigger on N900?

If this is taking longer to fix, should c85823390215 be reverted in the meantime? It does not seem particulary important/urgent...

Quoted 15 lines · Pavel Machek

Linux-Regression-ID: lr#4b650f

Quoted 12 lines · Linus Walleij

Quoted 9 lines · Pavel Machek

I did git bisect, and the winner seems to be:

pavel at duo:/data/l/linux-n900$ git bisect bad c85823390215e52d68d3826df92a447ed31e5c80 is the first bad commit commit c85823390215e52d68d3826df92a447ed31e5c80

Author: Linus Walleij <[email protected]>
Date:   Wed Dec 27 16:37:44 2017 +0100

gpio: of: Support SPI nonstandard GPIO properties

Quoted 8 lines

Quoted 2 lines

I have fixes queued for this, tried to send a pull request yesterday but it turns out the fixes need fixing... OK I'm onto it anyways.

Do you have any updates? Is there way to fix dts so that this does not trigger on N900?

If this is taking longer to fix, should c85823390215 be reverted in the meantime? It does not seem particulary important/urgent...

No patience between the v4.16 release candidates eh ;)

commit 6662ae6af82df10259a70c7569b4c12ea7f3ba93 ("gpiolib: Keep returning EPROBE_DEFER when we should")

and

commit ce27fb2c56db6ccfe8099343bb4afdab15e77e7b ("gpio: Handle deferred probing in of_find_gpio() properly")

that are both in Torvalds' tree since yesterday should be fixing this, I think? Did you try just using the upstream HEAD?

Quoted 35 lines · Linus Walleij

Quoted 15 lines · Pavel Machek

Linux-Regression-ID: lr#4b650f

Quoted 12 lines · Linus Walleij

Quoted 9 lines · Pavel Machek

I did git bisect, and the winner seems to be:

pavel at duo:/data/l/linux-n900$ git bisect bad c85823390215e52d68d3826df92a447ed31e5c80 is the first bad commit commit c85823390215e52d68d3826df92a447ed31e5c80

Author: Linus Walleij <[email protected]>
Date:   Wed Dec 27 16:37:44 2017 +0100

gpio: of: Support SPI nonstandard GPIO properties

Quoted 8 lines

Quoted 2 lines

I have fixes queued for this, tried to send a pull request yesterday but it turns out the fixes need fixing... OK I'm onto it anyways.

Do you have any updates? Is there way to fix dts so that this does not trigger on N900?

If this is taking longer to fix, should c85823390215 be reverted in the meantime? It does not seem particulary important/urgent...

No patience between the v4.16 release candidates eh ;)

commit 6662ae6af82df10259a70c7569b4c12ea7f3ba93 ("gpiolib: Keep returning EPROBE_DEFER when we should")

and

commit ce27fb2c56db6ccfe8099343bb4afdab15e77e7b ("gpio: Handle deferred probing in of_find_gpio() properly")

Quoted 2 lines

that are both in Torvalds' tree since yesterday should be fixing this, I think? Did you try just using the upstream HEAD?

After I spent hours bisecting, I was kind of assuming you'd cc me on merge request or something like that... so that I could test it before going mainline.

Which of those should fix it?

I tested today's mainline, and... sound fails, different way:

pavel at n900:~$ cat /proc/asound/cards
0 [RX51 ]: RX-51 - RX-51

                       RX-51
pavel at n900:~$ cd g/tui/ofone/
pavel at n900:~/g/tui/ofone$ cd
pavel at n900:~$ festival --tts
                       ahoj
                       ^D
                       uname -a

(Festival is hung).

Quoted 2 lines

Ok, so this code looks pretty crazy to me: I tried removing the "of_find_spi_gpio" part, and audio started working.

Hmm. Looks like audio is working w/o any changes, too. Not sure why festival hung on me before.

Quoted 5 lines

Quoted 2 lines

Ok, so this code looks pretty crazy to me: I tried removing the "of_find_spi_gpio" part, and audio started working.

Hmm. Looks like audio is working w/o any changes, too. Not sure why festival hung on me before.

Does it mean that mainline is working find for you or do we need to look deeper into the problem in the OF lookup?

Quoted 6 lines

Quoted 5 lines

Quoted 2 lines

Ok, so this code looks pretty crazy to me: I tried removing the "of_find_spi_gpio" part, and audio started working.

Hmm. Looks like audio is working w/o any changes, too. Not sure why festival hung on me before.

Quoted 3 lines

Does it mean that mainline is working find for you or do we need to look deeper into the problem in the OF lookup?

It now works for me.

(But take a look at the patch I sent -- I believe you are still overwriting return values in a bad way, and code is quite confusing).

Quoted 2 lines

If this is taking longer to fix, should c85823390215 be reverted in the meantime? It does not seem particulary important/urgent...

No patience between the v4.16 release candidates eh ;)

commit 6662ae6af82df10259a70c7569b4c12ea7f3ba93 ("gpiolib: Keep returning EPROBE_DEFER when we should")

and

commit ce27fb2c56db6ccfe8099343bb4afdab15e77e7b ("gpio: Handle deferred probing in of_find_gpio() properly")

that are both in Torvalds' tree since yesterday should be fixing this, I think? Did you try just using the upstream HEAD?

Hi!

Quoted 10 lines

Quoted 2 lines

If this is taking longer to fix, should c85823390215 be reverted in the meantime? It does not seem particulary important/urgent...

No patience between the v4.16 release candidates eh ;)

commit 6662ae6af82df10259a70c7569b4c12ea7f3ba93 ("gpiolib: Keep returning EPROBE_DEFER when we should")

and

Quoted 5 lines

commit ce27fb2c56db6ccfe8099343bb4afdab15e77e7b ("gpio: Handle deferred probing in of_find_gpio() properly")

that are both in Torvalds' tree since yesterday should be fixing this, I think? Did you try just using the upstream HEAD?

Ok, so this code looks pretty crazy to me: I tried removing the "of_find_spi_gpio" part, and audio started working.

Hi!

Quoted 8 lines

Quoted 2 lines

If this is taking longer to fix, should c85823390215 be reverted in the meantime? It does not seem particulary important/urgent...

No patience between the v4.16 release candidates eh ;)

commit 6662ae6af82df10259a70c7569b4c12ea7f3ba93 ("gpiolib: Keep returning EPROBE_DEFER when we should")

Quoted 7 lines

and

commit ce27fb2c56db6ccfe8099343bb4afdab15e77e7b ("gpio: Handle deferred probing in of_find_gpio() properly")

that are both in Torvalds' tree since yesterday should be fixing this, I think? Did you try just using the upstream HEAD?

Ok, so this code looks pretty crazy to me: I tried removing the "of_find_spi_gpio" part, and audio started working.

What is going on with the ()s around == s? You made me look up C operator precedence.

Hmm, and it is also wrong, right? It turns any error code into ENOENT, as it tries to do the "special handling".

                 *                                                                      
                 * This means we don't need to look any further for                     
                 * alternate name conventions, and we should really                     
                 * preserve the return code for our user to be able to                  
                 * retry probing later.                                                 
                 */
                if (IS_ERR(desc) && PTR_ERR(desc) == -EPROBE_DEFER)
                        return desc;

                if (!IS_ERR(desc) || (PTR_ERR(desc) != -ENOENT))
                        break;
        }

        /* Special handling for SPI GPIOs if used */
        if (IS_ERR(desc))
                desc = of_find_spi_gpio(dev, con_id, &of_flags);

        /* Special handling for regulator GPIOs if used */
        if (IS_ERR(desc) && PTR_ERR(desc) != -EPROBE_DEFER)
                desc = of_find_regulator_gpio(dev, con_id, &of_flags);

Something like this?

drivers/gpio/gpiolib-of.c +0 -5

index 84e5a9d..f0fab26 100644
@@ -241,29 +241,17 @@ struct gpio_desc *of_find_gpio(struct device *dev, const char *con_id,
 
 		desc = of_get_named_gpiod_flags(dev->of_node, prop_name, idx,
 						&of_flags);
-		/*
-		 * -EPROBE_DEFER in our case means that we found a
-		 * valid GPIO property, but no controller has been
-		 * registered so far.
-		 *

drivers/gpio/gpiolib-of.c +2 -2

 			break;
 	}
 
 	/* Special handling for SPI GPIOs if used */
-	if (IS_ERR(desc))
+	if (IS_ERR(desc) && PTR_ERR(desc) == -ENOENT)
 		desc = of_find_spi_gpio(dev, con_id, &of_flags);
 
 	/* Special handling for regulator GPIOs if used */
-	if (IS_ERR(desc) && PTR_ERR(desc) != -EPROBE_DEFER)
+	if (IS_ERR(desc) && PTR_ERR(desc) == -ENOENT)
 		desc = of_find_regulator_gpio(dev, con_id, &of_flags);
 
 	if (IS_ERR(desc))

Quoted 71 lines · Pavel Machek

Hi!

Quoted 8 lines

Quoted 2 lines

If this is taking longer to fix, should c85823390215 be reverted in the meantime? It does not seem particulary important/urgent...

No patience between the v4.16 release candidates eh ;)

commit 6662ae6af82df10259a70c7569b4c12ea7f3ba93 ("gpiolib: Keep returning EPROBE_DEFER when we should")

Quoted 7 lines

and

commit ce27fb2c56db6ccfe8099343bb4afdab15e77e7b ("gpio: Handle deferred probing in of_find_gpio() properly")

that are both in Torvalds' tree since yesterday should be fixing this, I think? Did you try just using the upstream HEAD?

Ok, so this code looks pretty crazy to me: I tried removing the "of_find_spi_gpio" part, and audio started working.

What is going on with the ()s around == s? You made me look up C operator precedence.

Hmm, and it is also wrong, right? It turns any error code into ENOENT, as it tries to do the "special handling".

                 *                                                                      
                 * This means we don't need to look any further for                     
                 * alternate name conventions, and we should really                     
                 * preserve the return code for our user to be able to                  
                 * retry probing later.                                                 
                 */
                if (IS_ERR(desc) && PTR_ERR(desc) == -EPROBE_DEFER)
                        return desc;

                if (!IS_ERR(desc) || (PTR_ERR(desc) != -ENOENT))
                        break;
        }

        /* Special handling for SPI GPIOs if used */
        if (IS_ERR(desc))
                desc = of_find_spi_gpio(dev, con_id, &of_flags);

        /* Special handling for regulator GPIOs if used */
        if (IS_ERR(desc) && PTR_ERR(desc) != -EPROBE_DEFER)
                desc = of_find_regulator_gpio(dev, con_id, &of_flags);

Something like this?

drivers/gpio/gpiolib-of.c +0 -12

index 84e5a9d..f0fab26 100644
@@ -241,29 +241,17 @@ struct gpio_desc *of_find_gpio(struct device *dev, const char *con_id,
 
 		desc = of_get_named_gpiod_flags(dev->of_node, prop_name, idx,
 						&of_flags);
-		/*
-		 * -EPROBE_DEFER in our case means that we found a
-		 * valid GPIO property, but no controller has been
-		 * registered so far.
-		 *
-		 * This means we don't need to look any further for
-		 * alternate name conventions, and we should really
-		 * preserve the return code for our user to be able to
-		 * retry probing later.
-		 */
-		if (IS_ERR(desc) && PTR_ERR(desc) == -EPROBE_DEFER)
-			return desc;
 

drivers/gpio/gpiolib-of.c struct gpio_desc *of_find_gpio(struct device *dev, const char *con_id, · line 245

Quoted 2 lines

drivers/gpio/gpiolib-of.c +1 -1

-		if (!IS_ERR(desc) || (PTR_ERR(desc) != -ENOENT))
+		if (!IS_ERR(desc) || PTR_ERR(desc) != -ENOENT)

I rather like the () so one doesn't always have to look up C operator precedence to verify..

Quoted 9 lines

drivers/gpio/gpiolib-of.c +1 -1

 			break;
 	}
 
 	/* Special handling for SPI GPIOs if used */
-	if (IS_ERR(desc))
+	if (IS_ERR(desc) && PTR_ERR(desc) == -ENOENT)
 		desc = of_find_spi_gpio(dev, con_id, &of_flags);
 

drivers/gpio/gpiolib-of.c struct gpio_desc *of_find_gpio(struct device *dev, const char *con_id, · line 257

Quoted 7 lines

drivers/gpio/gpiolib-of.c +1 -1

 	/* Special handling for regulator GPIOs if used */
-	if (IS_ERR(desc) && PTR_ERR(desc) != -EPROBE_DEFER)
+	if (IS_ERR(desc) && PTR_ERR(desc) == -ENOENT)
 		desc = of_find_regulator_gpio(dev, con_id, &of_flags);
 
 	if (IS_ERR(desc))

Quoted 21 lines

drivers/gpio/gpiolib-of.c +0 -12

index 84e5a9d..f0fab26 100644
@@ -241,29 +241,17 @@ struct gpio_desc *of_find_gpio(struct device *dev, const char *con_id,
 
 		desc = of_get_named_gpiod_flags(dev->of_node, prop_name, idx,
 						&of_flags);
-		/*
-		 * -EPROBE_DEFER in our case means that we found a
-		 * valid GPIO property, but no controller has been
-		 * registered so far.
-		 *
-		 * This means we don't need to look any further for
-		 * alternate name conventions, and we should really
-		 * preserve the return code for our user to be able to
-		 * retry probing later.
-		 */
-		if (IS_ERR(desc) && PTR_ERR(desc) == -EPROBE_DEFER)
-			return desc;
 

drivers/gpio/gpiolib-of.c struct gpio_desc *of_find_gpio(struct device *dev, const char *con_id, · line 245

Quoted 2 lines

drivers/gpio/gpiolib-of.c +1 -1

-		if (!IS_ERR(desc) || (PTR_ERR(desc) != -ENOENT))
+		if (!IS_ERR(desc) || PTR_ERR(desc) != -ENOENT)

I rather like the () so one doesn't always have to look up C operator precedence to verify..

drivers/gpio/gpiolib-of.c struct gpio_desc *of_find_gpio(struct device *dev, const char *con_id, · line 250

Quoted 8 lines

drivers/gpio/gpiolib-of.c +1 -1

 			break;
 	}
 
 	/* Special handling for SPI GPIOs if used */
-	if (IS_ERR(desc))
+	if (IS_ERR(desc) && PTR_ERR(desc) == -ENOENT)

Quoted 21 lines · Andrew F. Davis

Quoted 21 lines

drivers/gpio/gpiolib-of.c +0 -12

index 84e5a9d..f0fab26 100644
@@ -241,29 +241,17 @@ struct gpio_desc *of_find_gpio(struct device *dev, const char *con_id,
 
 		desc = of_get_named_gpiod_flags(dev->of_node, prop_name, idx,
 						&of_flags);
-		/*
-		 * -EPROBE_DEFER in our case means that we found a
-		 * valid GPIO property, but no controller has been
-		 * registered so far.
-		 *
-		 * This means we don't need to look any further for
-		 * alternate name conventions, and we should really
-		 * preserve the return code for our user to be able to
-		 * retry probing later.
-		 */
-		if (IS_ERR(desc) && PTR_ERR(desc) == -EPROBE_DEFER)
-			return desc;
 

Quoted 6 lines

drivers/gpio/gpiolib-of.c struct gpio_desc *of_find_gpio(struct device *dev, const char *con_id, · line 245

Quoted 2 lines

drivers/gpio/gpiolib-of.c +1 -1

-		if (!IS_ERR(desc) || (PTR_ERR(desc) != -ENOENT))
+		if (!IS_ERR(desc) || PTR_ERR(desc) != -ENOENT)

I rather like the () so one doesn't always have to look up C operator precedence to verify..

Too many make it impossible to see which close paren ties up with which open paren. I've spent way too long in the past reformatting code, where people think that () are a good thing, to work out what the comparison is actually doing before then rewriting the damn thing with less () and in a much clearer way. I'm now convinced that unnecessary () are a very bad thing as they severely harm readability as test complexity increases.

Quoted 8 lines

drivers/gpio/gpiolib-of.c struct gpio_desc *of_find_gpio(struct device *dev, const char *con_id, · line 250

Quoted 8 lines

drivers/gpio/gpiolib-of.c +1 -1

 			break;
 	}
 
 	/* Special handling for SPI GPIOs if used */
-	if (IS_ERR(desc))
+	if (IS_ERR(desc) && PTR_ERR(desc) == -ENOENT)

These can be simplified to:

	if (desc == ERR_PTR(-ENOENT))

since error pointers are unique - ERR_PTR(-ENOENT) is what was returned as an error-pointer, and if IS_ERR(ERR_PTR(-ENOMENT)) etc were false, we'd have big problems as it'd mean we couldn't detect error pointers!

As an added bonus, they don't involve any operator precedence questions either.

Hi!

In v4.16, AV jack support disappeared.

This one is suspect:

commit 7be4b5dc7ffa9499ac6ef33a5ffa9ff43f9b7057

Author: Andrew F. Davis <[email protected]>
Date:   Wed Nov 29 11:13:59 2017 -0600

ARM: dts: omap3-n900: Fix the audio CODEC's reset pin

The correct DT property for specifying a GPIO used for reset
is "reset-gpios", fix this here.

Fixes 14e3e295b2b9 ("ARM: dts: omap3-n900: Add TLV320AIC3X

support")

Signed-off-by Andrew F. Davis <[email protected]>

Signed-off-by Tony Lindgren <[email protected]>

How was it tested? It reverts polarity of reset pin, but sound/soc/codecs/tlv320aic3x.c treats those as aliases:

ret = of_get_named_gpio(np, "reset-gpios", 0);

 if (ret >= 0) {
    aic3x->gpio_reset = ret;
 } else {
 ret = of_get_named_gpio(np, "gpio-reset", 0);
 if (ret > 0) {
    dev_warn(&i2c->dev, "Using deprecated property \"gpio-r\eset\", please update your DT");
    aic3x->gpio_reset = ret;


									Pavel