* [PATCH v5 0/2] usb: dwc3: core: defer probe on ulpi_read_id timeout @ 2022-12-05 20:15 Ferry Toth 2022-12-05 20:15 ` [PATCH v5 1/2] usb: ulpi: defer ulpi_register " Ferry Toth 2022-12-05 20:15 ` [PATCH v5 2/2] usb: dwc3: core: defer probe on ulpi_read_id timeout Ferry Toth 0 siblings, 2 replies; 14+ messages in thread From: Ferry Toth @ 2022-12-05 20:15 UTC (permalink / raw) To: linux-usb, linux-kernel Cc: Heikki Krogerus, Greg Kroah-Hartman, Thinh Nguyen, Sean Anderson, Liu Shixin, Ferry Toth, Andrey Smirnov, Andy Shevchenko, Ferry Toth v5: - Fix the tag block (Andy) - Rephrasing, typo's (Andy, Thinh) v4: - Sent in correct version (Andy, Thinh) - Add details on reproducing on Intel Merrifield (Thinh) v3: - Correct commit message (Greg) - Add Fixes: and Cc: stable (Greg) v2: - Split into separate commits (Thinh) - Only defer probe on -ETIMEDOUT (Thinh) - Loose curly brackets (Heikki) Ferry Toth (2): usb: ulpi: defer ulpi_register on ulpi_read_id timeout usb: dwc3: core: defer probe on ulpi_read_id timeout drivers/usb/common/ulpi.c | 2 +- drivers/usb/dwc3/core.c | 7 ++++++- 2 files changed, 7 insertions(+), 2 deletions(-) -- 2.37.2 ^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH v5 1/2] usb: ulpi: defer ulpi_register on ulpi_read_id timeout 2022-12-05 20:15 [PATCH v5 0/2] usb: dwc3: core: defer probe on ulpi_read_id timeout Ferry Toth @ 2022-12-05 20:15 ` Ferry Toth 2022-12-07 11:33 ` Heikki Krogerus 2022-12-20 19:43 ` Guenter Roeck 2022-12-05 20:15 ` [PATCH v5 2/2] usb: dwc3: core: defer probe on ulpi_read_id timeout Ferry Toth 1 sibling, 2 replies; 14+ messages in thread From: Ferry Toth @ 2022-12-05 20:15 UTC (permalink / raw) To: linux-usb, linux-kernel Cc: Heikki Krogerus, Greg Kroah-Hartman, Thinh Nguyen, Sean Anderson, Liu Shixin, Ferry Toth, Andrey Smirnov, Andy Shevchenko, Ferry Toth, stable Since commit 0f0101719138 ("usb: dwc3: Don't switch OTG -> peripheral if extcon is present") Dual Role support on Intel Merrifield platform broke due to rearranging the call to dwc3_get_extcon(). It appears to be caused by ulpi_read_id() on the first test write failing with -ETIMEDOUT. Currently ulpi_read_id() expects to discover the phy via DT when the test write fails and returns 0 in that case, even if DT does not provide the phy. As a result usb probe completes without phy. Make ulpi_read_id() return -ETIMEDOUT to its user if the first test write fails. The user should then handle it appropriately. A follow up patch will make dwc3_core_init() set -EPROBE_DEFER in this case and bail out. Fixes: ef6a7bcfb01c ("usb: ulpi: Support device discovery via DT") Cc: stable@vger.kernel.org Signed-off-by: Ferry Toth <ftoth@exalondelft.nl> --- drivers/usb/common/ulpi.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/usb/common/ulpi.c b/drivers/usb/common/ulpi.c index d7c8461976ce..60e8174686a1 100644 --- a/drivers/usb/common/ulpi.c +++ b/drivers/usb/common/ulpi.c @@ -207,7 +207,7 @@ static int ulpi_read_id(struct ulpi *ulpi) /* Test the interface */ ret = ulpi_write(ulpi, ULPI_SCRATCH, 0xaa); if (ret < 0) - goto err; + return ret; ret = ulpi_read(ulpi, ULPI_SCRATCH); if (ret < 0) -- 2.37.2 ^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH v5 1/2] usb: ulpi: defer ulpi_register on ulpi_read_id timeout 2022-12-05 20:15 ` [PATCH v5 1/2] usb: ulpi: defer ulpi_register " Ferry Toth @ 2022-12-07 11:33 ` Heikki Krogerus 2022-12-20 19:43 ` Guenter Roeck 1 sibling, 0 replies; 14+ messages in thread From: Heikki Krogerus @ 2022-12-07 11:33 UTC (permalink / raw) To: Ferry Toth Cc: linux-usb, linux-kernel, Greg Kroah-Hartman, Thinh Nguyen, Sean Anderson, Liu Shixin, Ferry Toth, Andrey Smirnov, Andy Shevchenko, stable On Mon, Dec 05, 2022 at 09:15:26PM +0100, Ferry Toth wrote: > Since commit 0f0101719138 ("usb: dwc3: Don't switch OTG -> peripheral > if extcon is present") Dual Role support on Intel Merrifield platform > broke due to rearranging the call to dwc3_get_extcon(). > > It appears to be caused by ulpi_read_id() on the first test write failing > with -ETIMEDOUT. Currently ulpi_read_id() expects to discover the phy via > DT when the test write fails and returns 0 in that case, even if DT does not > provide the phy. As a result usb probe completes without phy. > > Make ulpi_read_id() return -ETIMEDOUT to its user if the first test write > fails. The user should then handle it appropriately. A follow up patch > will make dwc3_core_init() set -EPROBE_DEFER in this case and bail out. > > Fixes: ef6a7bcfb01c ("usb: ulpi: Support device discovery via DT") > Cc: stable@vger.kernel.org > Signed-off-by: Ferry Toth <ftoth@exalondelft.nl> Acked-by: Heikki Krogerus <heikki.krogerus@linux.intel.com> > --- > drivers/usb/common/ulpi.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/usb/common/ulpi.c b/drivers/usb/common/ulpi.c > index d7c8461976ce..60e8174686a1 100644 > --- a/drivers/usb/common/ulpi.c > +++ b/drivers/usb/common/ulpi.c > @@ -207,7 +207,7 @@ static int ulpi_read_id(struct ulpi *ulpi) > /* Test the interface */ > ret = ulpi_write(ulpi, ULPI_SCRATCH, 0xaa); > if (ret < 0) > - goto err; > + return ret; > > ret = ulpi_read(ulpi, ULPI_SCRATCH); > if (ret < 0) thanks, -- heikki ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v5 1/2] usb: ulpi: defer ulpi_register on ulpi_read_id timeout 2022-12-05 20:15 ` [PATCH v5 1/2] usb: ulpi: defer ulpi_register " Ferry Toth 2022-12-07 11:33 ` Heikki Krogerus @ 2022-12-20 19:43 ` Guenter Roeck 2022-12-21 10:07 ` Ferry Toth 2022-12-22 12:45 ` [PATCH v5 1/2] usb: ulpi: defer ulpi_register on ulpi_read_id timeout #forregzbot Thorsten Leemhuis 1 sibling, 2 replies; 14+ messages in thread From: Guenter Roeck @ 2022-12-20 19:43 UTC (permalink / raw) To: Ferry Toth Cc: linux-usb, linux-kernel, Heikki Krogerus, Greg Kroah-Hartman, Thinh Nguyen, Sean Anderson, Liu Shixin, Ferry Toth, Andrey Smirnov, Andy Shevchenko, stable On Mon, Dec 05, 2022 at 09:15:26PM +0100, Ferry Toth wrote: > Since commit 0f0101719138 ("usb: dwc3: Don't switch OTG -> peripheral > if extcon is present") Dual Role support on Intel Merrifield platform > broke due to rearranging the call to dwc3_get_extcon(). > > It appears to be caused by ulpi_read_id() on the first test write failing > with -ETIMEDOUT. Currently ulpi_read_id() expects to discover the phy via > DT when the test write fails and returns 0 in that case, even if DT does not > provide the phy. As a result usb probe completes without phy. > > Make ulpi_read_id() return -ETIMEDOUT to its user if the first test write > fails. The user should then handle it appropriately. A follow up patch > will make dwc3_core_init() set -EPROBE_DEFER in this case and bail out. > > Fixes: ef6a7bcfb01c ("usb: ulpi: Support device discovery via DT") > Cc: stable@vger.kernel.org > Signed-off-by: Ferry Toth <ftoth@exalondelft.nl> Hi, this patch results in some qemu test failures, specifically xilinx-zynq-a9 machine and zynq-zc702 as well as zynq-zed devicetree files, when trying to boot from USB drive. The log shows ci_hdrc ci_hdrc.0: failed to register ULPI interface ci_hdrc: probe of ci_hdrc.0 failed with error -110 and the USB interface does not instantiate. Reverting this patch fixes the problem. Bisect log is attached. A detailed log is available at https://kerneltests.org/builders/qemu-arm-v7-master/builds/484/steps/qemubuildcommand/logs/stdio Guenter --- # bad: [35f79d0e2c98ff6ecb9b5fc33113158dc7f7353c] Merge tag 'parisc-for-6.2-1' of git://git.kernel.org/pub/scm/linux/kernel/git/deller/parisc-linux # good: [830b3c68c1fb1e9176028d02ef86f3cf76aa2476] Linux 6.1 git bisect start 'HEAD' 'v6.1' # good: [90b12f423d3c8a89424c7bdde18e1923dfd0941e] Merge tag 'for-linus-6.2-1' of https://github.com/cminyard/linux-ipmi git bisect good 90b12f423d3c8a89424c7bdde18e1923dfd0941e # good: [c7020e1b346d5840e93b58cc4f2c67fc645d8df9] Merge tag 'pci-v6.2-changes' of git://git.kernel.org/pub/scm/linux/kernel/git/helgaas/pci git bisect good c7020e1b346d5840e93b58cc4f2c67fc645d8df9 # bad: [b83a7080d30032cf70832bc2bb04cc342e203b88] Merge tag 'staging-6.2-rc1' of git://git.kernel.org/pub/scm/linux/kernel/git/gregkh/staging git bisect bad b83a7080d30032cf70832bc2bb04cc342e203b88 # good: [057b40f43ce429a02e793adf3cfbf2446a19a38e] Merge tag 'acpi-6.2-rc1-2' of git://git.kernel.org/pub/scm/linux/kernel/git/rafael/linux-pm git bisect good 057b40f43ce429a02e793adf3cfbf2446a19a38e # good: [851f657a86421dded42b6175c6ea0f4f5e86af97] Merge tag '6.2-rc-smb3-client-fixes-part1' of git://git.samba.org/sfrench/cifs-2.6 git bisect good 851f657a86421dded42b6175c6ea0f4f5e86af97 # good: [fa205589d5e9fc2d1b2f8d31f665152da04160bc] staging: r8188eu: stop beacon processing if kmalloc fails git bisect good fa205589d5e9fc2d1b2f8d31f665152da04160bc # good: [4051a1c96e4883f3445cc8f239c214be622f4c6c] Merge tag 'thunderbolt-for-v6.2-rc1' of git://git.kernel.org/pub/scm/linux/kernel/git/westeri/thunderbolt into usb-next git bisect good 4051a1c96e4883f3445cc8f239c214be622f4c6c # good: [84e57d292203a45c96dbcb2e6be9dd80961d981a] Merge tag 'exfat-for-6.2-rc1' of git://git.kernel.org/pub/scm/linux/kernel/git/linkinjeon/exfat git bisect good 84e57d292203a45c96dbcb2e6be9dd80961d981a # good: [6f1f0ad910f73f5533b65e1748448d334e0ec697] usb: gadget: udc: drop obsolete dependencies on COMPILE_TEST git bisect good 6f1f0ad910f73f5533b65e1748448d334e0ec697 # good: [c7912f27dedd874d49eadf78b5b6fbfdec52c7c3] staging: rtl8192e: Fix spelling mistake "ContryIE" -> "CountryIE" git bisect good c7912f27dedd874d49eadf78b5b6fbfdec52c7c3 # bad: [63130462c919ece0ad0d9bb5a1f795ef8d79687e] usb: dwc3: core: defer probe on ulpi_read_id timeout git bisect bad 63130462c919ece0ad0d9bb5a1f795ef8d79687e # good: [38cea8e31e9ef143187135d714aed4d7bd18463c] dt-bindings: vendor-prefixes: add Genesys Logic git bisect good 38cea8e31e9ef143187135d714aed4d7bd18463c # good: [9bae996ffa28ac03b6d95382a2a082eb219e745a] usb: misc: onboard_usb_hub: add Genesys Logic GL850G hub support git bisect good 9bae996ffa28ac03b6d95382a2a082eb219e745a # bad: [8a7b31d545d3a15f0e6f5984ae16f0ca4fd76aac] usb: ulpi: defer ulpi_register on ulpi_read_id timeout git bisect bad 8a7b31d545d3a15f0e6f5984ae16f0ca4fd76aac # first bad commit: [8a7b31d545d3a15f0e6f5984ae16f0ca4fd76aac] usb: ulpi: defer ulpi_register on ulpi_read_id timeout ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v5 1/2] usb: ulpi: defer ulpi_register on ulpi_read_id timeout 2022-12-20 19:43 ` Guenter Roeck @ 2022-12-21 10:07 ` Ferry Toth 2022-12-21 12:41 ` Guenter Roeck 2022-12-22 12:45 ` [PATCH v5 1/2] usb: ulpi: defer ulpi_register on ulpi_read_id timeout #forregzbot Thorsten Leemhuis 1 sibling, 1 reply; 14+ messages in thread From: Ferry Toth @ 2022-12-21 10:07 UTC (permalink / raw) To: Guenter Roeck, Ferry Toth Cc: linux-usb, linux-kernel, Heikki Krogerus, Greg Kroah-Hartman, Thinh Nguyen, Sean Anderson, Liu Shixin, Andrey Smirnov, Andy Shevchenko, stable Hi, On 20-12-2022 20:43, Guenter Roeck wrote: > On Mon, Dec 05, 2022 at 09:15:26PM +0100, Ferry Toth wrote: >> Since commit 0f0101719138 ("usb: dwc3: Don't switch OTG -> peripheral >> if extcon is present") Dual Role support on Intel Merrifield platform >> broke due to rearranging the call to dwc3_get_extcon(). >> >> It appears to be caused by ulpi_read_id() on the first test write failing >> with -ETIMEDOUT. Currently ulpi_read_id() expects to discover the phy via >> DT when the test write fails and returns 0 in that case, even if DT does not >> provide the phy. As a result usb probe completes without phy. >> >> Make ulpi_read_id() return -ETIMEDOUT to its user if the first test write >> fails. The user should then handle it appropriately. A follow up patch >> will make dwc3_core_init() set -EPROBE_DEFER in this case and bail out. >> >> Fixes: ef6a7bcfb01c ("usb: ulpi: Support device discovery via DT") >> Cc: stable@vger.kernel.org >> Signed-off-by: Ferry Toth <ftoth@exalondelft.nl> > Hi, > > this patch results in some qemu test failures, specifically xilinx-zynq-a9 > machine and zynq-zc702 as well as zynq-zed devicetree files, when trying > to boot from USB drive. The log shows I'm not familiar with that platform. Does it use dt to discover the ulpi device? I'm guessing that the problem is actually caused by "usb: ulpi: defer ulpi_register on ulpi_read_id timeout". ulpi_read_id() now returns ETIMEDOUT due to the test write ulpi_write(ulpi, ULPI_SCRATCH, 0xaa) failing. Maybe we can create a fix by skipping the test write in case dt discovery is available and calling of_device_request_module() directly, instead of masking the timed out test write as it was before? > ci_hdrc ci_hdrc.0: failed to register ULPI interface > ci_hdrc: probe of ci_hdrc.0 failed with error -110 > > and the USB interface does not instantiate. Reverting this patch fixes > the problem. Bisect log is attached. > > A detailed log is available at > https://kerneltests.org/builders/qemu-arm-v7-master/builds/484/steps/qemubuildcommand/logs/stdio > > Guenter > > --- > # bad: [35f79d0e2c98ff6ecb9b5fc33113158dc7f7353c] Merge tag 'parisc-for-6.2-1' of git://git.kernel.org/pub/scm/linux/kernel/git/deller/parisc-linux > # good: [830b3c68c1fb1e9176028d02ef86f3cf76aa2476] Linux 6.1 > git bisect start 'HEAD' 'v6.1' > # good: [90b12f423d3c8a89424c7bdde18e1923dfd0941e] Merge tag 'for-linus-6.2-1' of https://github.com/cminyard/linux-ipmi > git bisect good 90b12f423d3c8a89424c7bdde18e1923dfd0941e > # good: [c7020e1b346d5840e93b58cc4f2c67fc645d8df9] Merge tag 'pci-v6.2-changes' of git://git.kernel.org/pub/scm/linux/kernel/git/helgaas/pci > git bisect good c7020e1b346d5840e93b58cc4f2c67fc645d8df9 > # bad: [b83a7080d30032cf70832bc2bb04cc342e203b88] Merge tag 'staging-6.2-rc1' of git://git.kernel.org/pub/scm/linux/kernel/git/gregkh/staging > git bisect bad b83a7080d30032cf70832bc2bb04cc342e203b88 > # good: [057b40f43ce429a02e793adf3cfbf2446a19a38e] Merge tag 'acpi-6.2-rc1-2' of git://git.kernel.org/pub/scm/linux/kernel/git/rafael/linux-pm > git bisect good 057b40f43ce429a02e793adf3cfbf2446a19a38e > # good: [851f657a86421dded42b6175c6ea0f4f5e86af97] Merge tag '6.2-rc-smb3-client-fixes-part1' of git://git.samba.org/sfrench/cifs-2.6 > git bisect good 851f657a86421dded42b6175c6ea0f4f5e86af97 > # good: [fa205589d5e9fc2d1b2f8d31f665152da04160bc] staging: r8188eu: stop beacon processing if kmalloc fails > git bisect good fa205589d5e9fc2d1b2f8d31f665152da04160bc > # good: [4051a1c96e4883f3445cc8f239c214be622f4c6c] Merge tag 'thunderbolt-for-v6.2-rc1' of git://git.kernel.org/pub/scm/linux/kernel/git/westeri/thunderbolt into usb-next > git bisect good 4051a1c96e4883f3445cc8f239c214be622f4c6c > # good: [84e57d292203a45c96dbcb2e6be9dd80961d981a] Merge tag 'exfat-for-6.2-rc1' of git://git.kernel.org/pub/scm/linux/kernel/git/linkinjeon/exfat > git bisect good 84e57d292203a45c96dbcb2e6be9dd80961d981a > # good: [6f1f0ad910f73f5533b65e1748448d334e0ec697] usb: gadget: udc: drop obsolete dependencies on COMPILE_TEST > git bisect good 6f1f0ad910f73f5533b65e1748448d334e0ec697 > # good: [c7912f27dedd874d49eadf78b5b6fbfdec52c7c3] staging: rtl8192e: Fix spelling mistake "ContryIE" -> "CountryIE" > git bisect good c7912f27dedd874d49eadf78b5b6fbfdec52c7c3 > # bad: [63130462c919ece0ad0d9bb5a1f795ef8d79687e] usb: dwc3: core: defer probe on ulpi_read_id timeout > git bisect bad 63130462c919ece0ad0d9bb5a1f795ef8d79687e > # good: [38cea8e31e9ef143187135d714aed4d7bd18463c] dt-bindings: vendor-prefixes: add Genesys Logic > git bisect good 38cea8e31e9ef143187135d714aed4d7bd18463c > # good: [9bae996ffa28ac03b6d95382a2a082eb219e745a] usb: misc: onboard_usb_hub: add Genesys Logic GL850G hub support > git bisect good 9bae996ffa28ac03b6d95382a2a082eb219e745a > # bad: [8a7b31d545d3a15f0e6f5984ae16f0ca4fd76aac] usb: ulpi: defer ulpi_register on ulpi_read_id timeout > git bisect bad 8a7b31d545d3a15f0e6f5984ae16f0ca4fd76aac > # first bad commit: [8a7b31d545d3a15f0e6f5984ae16f0ca4fd76aac] usb: ulpi: defer ulpi_register on ulpi_read_id timeout ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v5 1/2] usb: ulpi: defer ulpi_register on ulpi_read_id timeout 2022-12-21 10:07 ` Ferry Toth @ 2022-12-21 12:41 ` Guenter Roeck 2022-12-21 14:29 ` Ferry Toth 0 siblings, 1 reply; 14+ messages in thread From: Guenter Roeck @ 2022-12-21 12:41 UTC (permalink / raw) To: Ferry Toth Cc: Ferry Toth, linux-usb, linux-kernel, Heikki Krogerus, Greg Kroah-Hartman, Thinh Nguyen, Sean Anderson, Liu Shixin, Andrey Smirnov, Andy Shevchenko, stable On Wed, Dec 21, 2022 at 11:07:50AM +0100, Ferry Toth wrote: > Hi, > > On 20-12-2022 20:43, Guenter Roeck wrote: > > On Mon, Dec 05, 2022 at 09:15:26PM +0100, Ferry Toth wrote: > > > Since commit 0f0101719138 ("usb: dwc3: Don't switch OTG -> peripheral > > > if extcon is present") Dual Role support on Intel Merrifield platform > > > broke due to rearranging the call to dwc3_get_extcon(). > > > > > > It appears to be caused by ulpi_read_id() on the first test write failing > > > with -ETIMEDOUT. Currently ulpi_read_id() expects to discover the phy via > > > DT when the test write fails and returns 0 in that case, even if DT does not > > > provide the phy. As a result usb probe completes without phy. > > > > > > Make ulpi_read_id() return -ETIMEDOUT to its user if the first test write > > > fails. The user should then handle it appropriately. A follow up patch > > > will make dwc3_core_init() set -EPROBE_DEFER in this case and bail out. > > > > > > Fixes: ef6a7bcfb01c ("usb: ulpi: Support device discovery via DT") > > > Cc: stable@vger.kernel.org > > > Signed-off-by: Ferry Toth <ftoth@exalondelft.nl> > > Hi, > > > > this patch results in some qemu test failures, specifically xilinx-zynq-a9 > > machine and zynq-zc702 as well as zynq-zed devicetree files, when trying > > to boot from USB drive. The log shows > > I'm not familiar with that platform. Does it use dt to discover the ulpi > device? > The dt usb description includes usb_phy0: phy0 { compatible = "usb-nop-xceiv"; #phy-cells = <0>; }; ... &usb0 { status = "okay"; dr_mode = "host"; usb-phy = <&usb_phy0>; }; ... usb0: usb@e0002000 { compatible = "xlnx,zynq-usb-2.20a", "chipidea,usb2"; status = "disabled"; clocks = <&clkc 28>; interrupt-parent = <&intc>; interrupts = <0 21 4>; reg = <0xe0002000 0x1000>; phy_type = "ulpi"; }; The chipidea core initialization code includes if (!platdata->phy_mode) platdata->phy_mode = of_usb_get_phy_mode(dev->of_node); Does that mean that every chipidea based usb implementation specifying phy_type = "ulpi"; in their devicetree description will now fail, plus maybe others who determine the phy mode from devicetree ? > I'm guessing that the problem is actually caused by "usb: ulpi: defer > ulpi_register on ulpi_read_id timeout". > Confused. Isn't that this patch ? > ulpi_read_id() now returns ETIMEDOUT due to the test write ulpi_write(ulpi, > ULPI_SCRATCH, 0xaa) failing. > > Maybe we can create a fix by skipping the test write in case dt discovery > is available and calling of_device_request_module() directly, instead of > masking the timed out test write as it was before? > I have no idea. All I can see is that it appears that there was a reason for not returning an error if that test write failed. Thanks, Guenter > > ci_hdrc ci_hdrc.0: failed to register ULPI interface > > ci_hdrc: probe of ci_hdrc.0 failed with error -110 > > > > and the USB interface does not instantiate. Reverting this patch fixes > > the problem. Bisect log is attached. > > > > A detailed log is available at > > https://kerneltests.org/builders/qemu-arm-v7-master/builds/484/steps/qemubuildcommand/logs/stdio > > > > Guenter > > > > --- > > # bad: [35f79d0e2c98ff6ecb9b5fc33113158dc7f7353c] Merge tag 'parisc-for-6.2-1' of git://git.kernel.org/pub/scm/linux/kernel/git/deller/parisc-linux > > # good: [830b3c68c1fb1e9176028d02ef86f3cf76aa2476] Linux 6.1 > > git bisect start 'HEAD' 'v6.1' > > # good: [90b12f423d3c8a89424c7bdde18e1923dfd0941e] Merge tag 'for-linus-6.2-1' of https://github.com/cminyard/linux-ipmi > > git bisect good 90b12f423d3c8a89424c7bdde18e1923dfd0941e > > # good: [c7020e1b346d5840e93b58cc4f2c67fc645d8df9] Merge tag 'pci-v6.2-changes' of git://git.kernel.org/pub/scm/linux/kernel/git/helgaas/pci > > git bisect good c7020e1b346d5840e93b58cc4f2c67fc645d8df9 > > # bad: [b83a7080d30032cf70832bc2bb04cc342e203b88] Merge tag 'staging-6.2-rc1' of git://git.kernel.org/pub/scm/linux/kernel/git/gregkh/staging > > git bisect bad b83a7080d30032cf70832bc2bb04cc342e203b88 > > # good: [057b40f43ce429a02e793adf3cfbf2446a19a38e] Merge tag 'acpi-6.2-rc1-2' of git://git.kernel.org/pub/scm/linux/kernel/git/rafael/linux-pm > > git bisect good 057b40f43ce429a02e793adf3cfbf2446a19a38e > > # good: [851f657a86421dded42b6175c6ea0f4f5e86af97] Merge tag '6.2-rc-smb3-client-fixes-part1' of git://git.samba.org/sfrench/cifs-2.6 > > git bisect good 851f657a86421dded42b6175c6ea0f4f5e86af97 > > # good: [fa205589d5e9fc2d1b2f8d31f665152da04160bc] staging: r8188eu: stop beacon processing if kmalloc fails > > git bisect good fa205589d5e9fc2d1b2f8d31f665152da04160bc > > # good: [4051a1c96e4883f3445cc8f239c214be622f4c6c] Merge tag 'thunderbolt-for-v6.2-rc1' of git://git.kernel.org/pub/scm/linux/kernel/git/westeri/thunderbolt into usb-next > > git bisect good 4051a1c96e4883f3445cc8f239c214be622f4c6c > > # good: [84e57d292203a45c96dbcb2e6be9dd80961d981a] Merge tag 'exfat-for-6.2-rc1' of git://git.kernel.org/pub/scm/linux/kernel/git/linkinjeon/exfat > > git bisect good 84e57d292203a45c96dbcb2e6be9dd80961d981a > > # good: [6f1f0ad910f73f5533b65e1748448d334e0ec697] usb: gadget: udc: drop obsolete dependencies on COMPILE_TEST > > git bisect good 6f1f0ad910f73f5533b65e1748448d334e0ec697 > > # good: [c7912f27dedd874d49eadf78b5b6fbfdec52c7c3] staging: rtl8192e: Fix spelling mistake "ContryIE" -> "CountryIE" > > git bisect good c7912f27dedd874d49eadf78b5b6fbfdec52c7c3 > > # bad: [63130462c919ece0ad0d9bb5a1f795ef8d79687e] usb: dwc3: core: defer probe on ulpi_read_id timeout > > git bisect bad 63130462c919ece0ad0d9bb5a1f795ef8d79687e > > # good: [38cea8e31e9ef143187135d714aed4d7bd18463c] dt-bindings: vendor-prefixes: add Genesys Logic > > git bisect good 38cea8e31e9ef143187135d714aed4d7bd18463c > > # good: [9bae996ffa28ac03b6d95382a2a082eb219e745a] usb: misc: onboard_usb_hub: add Genesys Logic GL850G hub support > > git bisect good 9bae996ffa28ac03b6d95382a2a082eb219e745a > > # bad: [8a7b31d545d3a15f0e6f5984ae16f0ca4fd76aac] usb: ulpi: defer ulpi_register on ulpi_read_id timeout > > git bisect bad 8a7b31d545d3a15f0e6f5984ae16f0ca4fd76aac > > # first bad commit: [8a7b31d545d3a15f0e6f5984ae16f0ca4fd76aac] usb: ulpi: defer ulpi_register on ulpi_read_id timeout ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v5 1/2] usb: ulpi: defer ulpi_register on ulpi_read_id timeout 2022-12-21 12:41 ` Guenter Roeck @ 2022-12-21 14:29 ` Ferry Toth 2022-12-21 17:30 ` Guenter Roeck 0 siblings, 1 reply; 14+ messages in thread From: Ferry Toth @ 2022-12-21 14:29 UTC (permalink / raw) To: Guenter Roeck, Ferry Toth Cc: linux-usb, linux-kernel, Heikki Krogerus, Greg Kroah-Hartman, Thinh Nguyen, Sean Anderson, Liu Shixin, Andrey Smirnov, Andy Shevchenko, stable Hi On 21-12-2022 13:41, Guenter Roeck wrote: > On Wed, Dec 21, 2022 at 11:07:50AM +0100, Ferry Toth wrote: >> Hi, >> >> On 20-12-2022 20:43, Guenter Roeck wrote: >>> On Mon, Dec 05, 2022 at 09:15:26PM +0100, Ferry Toth wrote: >>>> Since commit 0f0101719138 ("usb: dwc3: Don't switch OTG -> peripheral >>>> if extcon is present") Dual Role support on Intel Merrifield platform >>>> broke due to rearranging the call to dwc3_get_extcon(). >>>> >>>> It appears to be caused by ulpi_read_id() on the first test write failing >>>> with -ETIMEDOUT. Currently ulpi_read_id() expects to discover the phy via >>>> DT when the test write fails and returns 0 in that case, even if DT does not >>>> provide the phy. As a result usb probe completes without phy. >>>> >>>> Make ulpi_read_id() return -ETIMEDOUT to its user if the first test write >>>> fails. The user should then handle it appropriately. A follow up patch >>>> will make dwc3_core_init() set -EPROBE_DEFER in this case and bail out. >>>> >>>> Fixes: ef6a7bcfb01c ("usb: ulpi: Support device discovery via DT") >>>> Cc: stable@vger.kernel.org >>>> Signed-off-by: Ferry Toth <ftoth@exalondelft.nl> >>> Hi, >>> >>> this patch results in some qemu test failures, specifically xilinx-zynq-a9 >>> machine and zynq-zc702 as well as zynq-zed devicetree files, when trying >>> to boot from USB drive. The log shows >> I'm not familiar with that platform. Does it use dt to discover the ulpi >> device? >> > The dt usb description includes > > usb_phy0: phy0 { > compatible = "usb-nop-xceiv"; > #phy-cells = <0>; > }; > > ... > > &usb0 { > status = "okay"; > dr_mode = "host"; > usb-phy = <&usb_phy0>; > }; > > ... > > usb0: usb@e0002000 { > compatible = "xlnx,zynq-usb-2.20a", "chipidea,usb2"; > status = "disabled"; > clocks = <&clkc 28>; > interrupt-parent = <&intc>; > interrupts = <0 21 4>; > reg = <0xe0002000 0x1000>; > phy_type = "ulpi"; > }; > > The chipidea core initialization code includes > > if (!platdata->phy_mode) > platdata->phy_mode = of_usb_get_phy_mode(dev->of_node); > > Does that mean that every chipidea based usb implementation specifying > phy_type = "ulpi"; > in their devicetree description will now fail, plus maybe others > who determine the phy mode from devicetree ? I don't think so. >> I'm guessing that the problem is actually caused by "usb: ulpi: defer >> ulpi_register on ulpi_read_id timeout". >> > Confused. Isn't that this patch ? Ehem. Yes. >> ulpi_read_id() now returns ETIMEDOUT due to the test write ulpi_write(ulpi, >> ULPI_SCRATCH, 0xaa) failing. >> >> Maybe we can create a fix by skipping the test write in case dt discovery >> is available and calling of_device_request_module() directly, instead of >> masking the timed out test write as it was before? >> > I have no idea. All I can see is that it appears that there was a reason > for not returning an error if that test write failed. It seems to have been a quick patch to solve a power sequencing issue: "The ULPI bus code supports native enumeration by reading the vendor ID and product ID registers at device creation time, but we can't be certain that those register reads will succeed if the phy is not powered up. To avoid any problems with reading the ID registers before the phy is powered we fallback to DT matching when the ID reads fail. If the ULPI spec had some generic power sequencing for these registers we could put that into the ULPI bus layer and power up the device before reading the ID registers. Unfortunately this doesn't exist and the power sequence is usually device specific. By having the device matched up with DT we can avoid this problem." But as is, the code now requires a DT when there is a power sequencing issue, which is wrong for Merrifield. It seems my patch breaks the OF path, by replacing that by a deferred probe. I'm thinking the correct way would be: - if present use DT - else if test write fails, defer probe - else enumeration by reading the vendor ID and product ID registers > > Thanks, > Guenter > >>> ci_hdrc ci_hdrc.0: failed to register ULPI interface >>> ci_hdrc: probe of ci_hdrc.0 failed with error -110 >>> >>> and the USB interface does not instantiate. Reverting this patch fixes >>> the problem. Bisect log is attached. >>> >>> A detailed log is available at >>> https://kerneltests.org/builders/qemu-arm-v7-master/builds/484/steps/qemubuildcommand/logs/stdio >>> >>> Guenter >>> >>> --- >>> # bad: [35f79d0e2c98ff6ecb9b5fc33113158dc7f7353c] Merge tag 'parisc-for-6.2-1' of git://git.kernel.org/pub/scm/linux/kernel/git/deller/parisc-linux >>> # good: [830b3c68c1fb1e9176028d02ef86f3cf76aa2476] Linux 6.1 >>> git bisect start 'HEAD' 'v6.1' >>> # good: [90b12f423d3c8a89424c7bdde18e1923dfd0941e] Merge tag 'for-linus-6.2-1' of https://github.com/cminyard/linux-ipmi >>> git bisect good 90b12f423d3c8a89424c7bdde18e1923dfd0941e >>> # good: [c7020e1b346d5840e93b58cc4f2c67fc645d8df9] Merge tag 'pci-v6.2-changes' of git://git.kernel.org/pub/scm/linux/kernel/git/helgaas/pci >>> git bisect good c7020e1b346d5840e93b58cc4f2c67fc645d8df9 >>> # bad: [b83a7080d30032cf70832bc2bb04cc342e203b88] Merge tag 'staging-6.2-rc1' of git://git.kernel.org/pub/scm/linux/kernel/git/gregkh/staging >>> git bisect bad b83a7080d30032cf70832bc2bb04cc342e203b88 >>> # good: [057b40f43ce429a02e793adf3cfbf2446a19a38e] Merge tag 'acpi-6.2-rc1-2' of git://git.kernel.org/pub/scm/linux/kernel/git/rafael/linux-pm >>> git bisect good 057b40f43ce429a02e793adf3cfbf2446a19a38e >>> # good: [851f657a86421dded42b6175c6ea0f4f5e86af97] Merge tag '6.2-rc-smb3-client-fixes-part1' of git://git.samba.org/sfrench/cifs-2.6 >>> git bisect good 851f657a86421dded42b6175c6ea0f4f5e86af97 >>> # good: [fa205589d5e9fc2d1b2f8d31f665152da04160bc] staging: r8188eu: stop beacon processing if kmalloc fails >>> git bisect good fa205589d5e9fc2d1b2f8d31f665152da04160bc >>> # good: [4051a1c96e4883f3445cc8f239c214be622f4c6c] Merge tag 'thunderbolt-for-v6.2-rc1' of git://git.kernel.org/pub/scm/linux/kernel/git/westeri/thunderbolt into usb-next >>> git bisect good 4051a1c96e4883f3445cc8f239c214be622f4c6c >>> # good: [84e57d292203a45c96dbcb2e6be9dd80961d981a] Merge tag 'exfat-for-6.2-rc1' of git://git.kernel.org/pub/scm/linux/kernel/git/linkinjeon/exfat >>> git bisect good 84e57d292203a45c96dbcb2e6be9dd80961d981a >>> # good: [6f1f0ad910f73f5533b65e1748448d334e0ec697] usb: gadget: udc: drop obsolete dependencies on COMPILE_TEST >>> git bisect good 6f1f0ad910f73f5533b65e1748448d334e0ec697 >>> # good: [c7912f27dedd874d49eadf78b5b6fbfdec52c7c3] staging: rtl8192e: Fix spelling mistake "ContryIE" -> "CountryIE" >>> git bisect good c7912f27dedd874d49eadf78b5b6fbfdec52c7c3 >>> # bad: [63130462c919ece0ad0d9bb5a1f795ef8d79687e] usb: dwc3: core: defer probe on ulpi_read_id timeout >>> git bisect bad 63130462c919ece0ad0d9bb5a1f795ef8d79687e >>> # good: [38cea8e31e9ef143187135d714aed4d7bd18463c] dt-bindings: vendor-prefixes: add Genesys Logic >>> git bisect good 38cea8e31e9ef143187135d714aed4d7bd18463c >>> # good: [9bae996ffa28ac03b6d95382a2a082eb219e745a] usb: misc: onboard_usb_hub: add Genesys Logic GL850G hub support >>> git bisect good 9bae996ffa28ac03b6d95382a2a082eb219e745a >>> # bad: [8a7b31d545d3a15f0e6f5984ae16f0ca4fd76aac] usb: ulpi: defer ulpi_register on ulpi_read_id timeout >>> git bisect bad 8a7b31d545d3a15f0e6f5984ae16f0ca4fd76aac >>> # first bad commit: [8a7b31d545d3a15f0e6f5984ae16f0ca4fd76aac] usb: ulpi: defer ulpi_register on ulpi_read_id timeout ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v5 1/2] usb: ulpi: defer ulpi_register on ulpi_read_id timeout 2022-12-21 14:29 ` Ferry Toth @ 2022-12-21 17:30 ` Guenter Roeck 2022-12-21 18:23 ` Greg Kroah-Hartman 2022-12-21 18:38 ` Ferry Toth 0 siblings, 2 replies; 14+ messages in thread From: Guenter Roeck @ 2022-12-21 17:30 UTC (permalink / raw) To: Ferry Toth Cc: linux-usb, linux-kernel, Heikki Krogerus, Greg Kroah-Hartman, Thinh Nguyen, Sean Anderson, Liu Shixin, Andrey Smirnov, Andy Shevchenko, stable On Wed, Dec 21, 2022 at 03:29:09PM +0100, Ferry Toth wrote: > Hi > > On 21-12-2022 13:41, Guenter Roeck wrote: > > On Wed, Dec 21, 2022 at 11:07:50AM +0100, Ferry Toth wrote: > > > Hi, > > > > > > On 20-12-2022 20:43, Guenter Roeck wrote: > > > > On Mon, Dec 05, 2022 at 09:15:26PM +0100, Ferry Toth wrote: > > > > > Since commit 0f0101719138 ("usb: dwc3: Don't switch OTG -> peripheral > > > > > if extcon is present") Dual Role support on Intel Merrifield platform > > > > > broke due to rearranging the call to dwc3_get_extcon(). > > > > > > > > > > It appears to be caused by ulpi_read_id() on the first test write failing > > > > > with -ETIMEDOUT. Currently ulpi_read_id() expects to discover the phy via > > > > > DT when the test write fails and returns 0 in that case, even if DT does not > > > > > provide the phy. As a result usb probe completes without phy. > > > > > > > > > > Make ulpi_read_id() return -ETIMEDOUT to its user if the first test write > > > > > fails. The user should then handle it appropriately. A follow up patch > > > > > will make dwc3_core_init() set -EPROBE_DEFER in this case and bail out. > > > > > > > > > > Fixes: ef6a7bcfb01c ("usb: ulpi: Support device discovery via DT") > > > > > Cc: stable@vger.kernel.org > > > > > Signed-off-by: Ferry Toth <ftoth@exalondelft.nl> > > > > Hi, > > > > > > > > this patch results in some qemu test failures, specifically xilinx-zynq-a9 > > > > machine and zynq-zc702 as well as zynq-zed devicetree files, when trying > > > > to boot from USB drive. The log shows > > > I'm not familiar with that platform. Does it use dt to discover the ulpi > > > device? > > > > > The dt usb description includes > > > > usb_phy0: phy0 { > > compatible = "usb-nop-xceiv"; > > #phy-cells = <0>; > > }; > > > > ... > > > > &usb0 { > > status = "okay"; > > dr_mode = "host"; > > usb-phy = <&usb_phy0>; > > }; > > > > ... > > > > usb0: usb@e0002000 { > > compatible = "xlnx,zynq-usb-2.20a", "chipidea,usb2"; > > status = "disabled"; > > clocks = <&clkc 28>; > > interrupt-parent = <&intc>; > > interrupts = <0 21 4>; > > reg = <0xe0002000 0x1000>; > > phy_type = "ulpi"; > > }; > > > > The chipidea core initialization code includes > > > > if (!platdata->phy_mode) > > platdata->phy_mode = of_usb_get_phy_mode(dev->of_node); > > > > Does that mean that every chipidea based usb implementation specifying > > phy_type = "ulpi"; > > in their devicetree description will now fail, plus maybe others > > who determine the phy mode from devicetree ? > I don't think so. > > > I'm guessing that the problem is actually caused by "usb: ulpi: defer > > > ulpi_register on ulpi_read_id timeout". > > > > > Confused. Isn't that this patch ? > Ehem. Yes. > > > ulpi_read_id() now returns ETIMEDOUT due to the test write ulpi_write(ulpi, > > > ULPI_SCRATCH, 0xaa) failing. > > > > > > Maybe we can create a fix by skipping the test write in case dt discovery > > > is available and calling of_device_request_module() directly, instead of > > > masking the timed out test write as it was before? > > > > > I have no idea. All I can see is that it appears that there was a reason > > for not returning an error if that test write failed. > > It seems to have been a quick patch to solve a power sequencing issue: > > "The ULPI bus code supports native enumeration by reading the > vendor ID and product ID registers at device creation time, but > we can't be certain that those register reads will succeed if the > phy is not powered up. To avoid any problems with reading the ID > registers before the phy is powered we fallback to DT matching > when the ID reads fail. > > If the ULPI spec had some generic power sequencing for these > registers we could put that into the ULPI bus layer and power up > the device before reading the ID registers. Unfortunately this > doesn't exist and the power sequence is usually device specific. > By having the device matched up with DT we can avoid this > problem." > > But as is, the code now requires a DT when there is a power > sequencing issue, which is wrong for Merrifield. It seems my patch > breaks the OF path, by replacing that by a deferred probe. > > I'm thinking the correct way would be: > - if present use DT > - else if test write fails, defer probe > - else enumeration by reading the vendor ID and product ID registers > I think this patch should be reverted until a better solution is found. After all, at this point it is effectively unknown if there are other users (besides devicetree) depending on ulpi_read_id() returning 0 if the communication with the device fails. Guenter ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v5 1/2] usb: ulpi: defer ulpi_register on ulpi_read_id timeout 2022-12-21 17:30 ` Guenter Roeck @ 2022-12-21 18:23 ` Greg Kroah-Hartman 2022-12-21 18:38 ` Ferry Toth 1 sibling, 0 replies; 14+ messages in thread From: Greg Kroah-Hartman @ 2022-12-21 18:23 UTC (permalink / raw) To: Guenter Roeck Cc: Ferry Toth, linux-usb, linux-kernel, Heikki Krogerus, Thinh Nguyen, Sean Anderson, Liu Shixin, Andrey Smirnov, Andy Shevchenko, stable On Wed, Dec 21, 2022 at 09:30:35AM -0800, Guenter Roeck wrote: > On Wed, Dec 21, 2022 at 03:29:09PM +0100, Ferry Toth wrote: > > Hi > > > > On 21-12-2022 13:41, Guenter Roeck wrote: > > > On Wed, Dec 21, 2022 at 11:07:50AM +0100, Ferry Toth wrote: > > > > Hi, > > > > > > > > On 20-12-2022 20:43, Guenter Roeck wrote: > > > > > On Mon, Dec 05, 2022 at 09:15:26PM +0100, Ferry Toth wrote: > > > > > > Since commit 0f0101719138 ("usb: dwc3: Don't switch OTG -> peripheral > > > > > > if extcon is present") Dual Role support on Intel Merrifield platform > > > > > > broke due to rearranging the call to dwc3_get_extcon(). > > > > > > > > > > > > It appears to be caused by ulpi_read_id() on the first test write failing > > > > > > with -ETIMEDOUT. Currently ulpi_read_id() expects to discover the phy via > > > > > > DT when the test write fails and returns 0 in that case, even if DT does not > > > > > > provide the phy. As a result usb probe completes without phy. > > > > > > > > > > > > Make ulpi_read_id() return -ETIMEDOUT to its user if the first test write > > > > > > fails. The user should then handle it appropriately. A follow up patch > > > > > > will make dwc3_core_init() set -EPROBE_DEFER in this case and bail out. > > > > > > > > > > > > Fixes: ef6a7bcfb01c ("usb: ulpi: Support device discovery via DT") > > > > > > Cc: stable@vger.kernel.org > > > > > > Signed-off-by: Ferry Toth <ftoth@exalondelft.nl> > > > > > Hi, > > > > > > > > > > this patch results in some qemu test failures, specifically xilinx-zynq-a9 > > > > > machine and zynq-zc702 as well as zynq-zed devicetree files, when trying > > > > > to boot from USB drive. The log shows > > > > I'm not familiar with that platform. Does it use dt to discover the ulpi > > > > device? > > > > > > > The dt usb description includes > > > > > > usb_phy0: phy0 { > > > compatible = "usb-nop-xceiv"; > > > #phy-cells = <0>; > > > }; > > > > > > ... > > > > > > &usb0 { > > > status = "okay"; > > > dr_mode = "host"; > > > usb-phy = <&usb_phy0>; > > > }; > > > > > > ... > > > > > > usb0: usb@e0002000 { > > > compatible = "xlnx,zynq-usb-2.20a", "chipidea,usb2"; > > > status = "disabled"; > > > clocks = <&clkc 28>; > > > interrupt-parent = <&intc>; > > > interrupts = <0 21 4>; > > > reg = <0xe0002000 0x1000>; > > > phy_type = "ulpi"; > > > }; > > > > > > The chipidea core initialization code includes > > > > > > if (!platdata->phy_mode) > > > platdata->phy_mode = of_usb_get_phy_mode(dev->of_node); > > > > > > Does that mean that every chipidea based usb implementation specifying > > > phy_type = "ulpi"; > > > in their devicetree description will now fail, plus maybe others > > > who determine the phy mode from devicetree ? > > I don't think so. > > > > I'm guessing that the problem is actually caused by "usb: ulpi: defer > > > > ulpi_register on ulpi_read_id timeout". > > > > > > > Confused. Isn't that this patch ? > > Ehem. Yes. > > > > ulpi_read_id() now returns ETIMEDOUT due to the test write ulpi_write(ulpi, > > > > ULPI_SCRATCH, 0xaa) failing. > > > > > > > > Maybe we can create a fix by skipping the test write in case dt discovery > > > > is available and calling of_device_request_module() directly, instead of > > > > masking the timed out test write as it was before? > > > > > > > I have no idea. All I can see is that it appears that there was a reason > > > for not returning an error if that test write failed. > > > > It seems to have been a quick patch to solve a power sequencing issue: > > > > "The ULPI bus code supports native enumeration by reading the > > vendor ID and product ID registers at device creation time, but > > we can't be certain that those register reads will succeed if the > > phy is not powered up. To avoid any problems with reading the ID > > registers before the phy is powered we fallback to DT matching > > when the ID reads fail. > > > > If the ULPI spec had some generic power sequencing for these > > registers we could put that into the ULPI bus layer and power up > > the device before reading the ID registers. Unfortunately this > > doesn't exist and the power sequence is usually device specific. > > By having the device matched up with DT we can avoid this > > problem." > > > > But as is, the code now requires a DT when there is a power > > sequencing issue, which is wrong for Merrifield. It seems my patch > > breaks the OF path, by replacing that by a deferred probe. > > > > I'm thinking the correct way would be: > > - if present use DT > > - else if test write fails, defer probe > > - else enumeration by reading the vendor ID and product ID registers > > > > I think this patch should be reverted until a better solution is found. > After all, at this point it is effectively unknown if there are other > users (besides devicetree) depending on ulpi_read_id() returning 0 if > the communication with the device fails. Yes, it should be reverted, can someone send me a patch that does so summarizing this thread? thanks, greg k-h ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v5 1/2] usb: ulpi: defer ulpi_register on ulpi_read_id timeout 2022-12-21 17:30 ` Guenter Roeck 2022-12-21 18:23 ` Greg Kroah-Hartman @ 2022-12-21 18:38 ` Ferry Toth 2022-12-21 18:48 ` Greg Kroah-Hartman 1 sibling, 1 reply; 14+ messages in thread From: Ferry Toth @ 2022-12-21 18:38 UTC (permalink / raw) To: Guenter Roeck Cc: linux-usb, linux-kernel, Heikki Krogerus, Greg Kroah-Hartman, Thinh Nguyen, Sean Anderson, Liu Shixin, Andrey Smirnov, Andy Shevchenko, stable Op 21-12-2022 om 18:30 schreef Guenter Roeck: > On Wed, Dec 21, 2022 at 03:29:09PM +0100, Ferry Toth wrote: >> Hi >> >> On 21-12-2022 13:41, Guenter Roeck wrote: >>> On Wed, Dec 21, 2022 at 11:07:50AM +0100, Ferry Toth wrote: >>>> Hi, >>>> >>>> On 20-12-2022 20:43, Guenter Roeck wrote: >>>>> On Mon, Dec 05, 2022 at 09:15:26PM +0100, Ferry Toth wrote: >>>>>> Since commit 0f0101719138 ("usb: dwc3: Don't switch OTG -> peripheral >>>>>> if extcon is present") Dual Role support on Intel Merrifield platform >>>>>> broke due to rearranging the call to dwc3_get_extcon(). >>>>>> >>>>>> It appears to be caused by ulpi_read_id() on the first test write failing >>>>>> with -ETIMEDOUT. Currently ulpi_read_id() expects to discover the phy via >>>>>> DT when the test write fails and returns 0 in that case, even if DT does not >>>>>> provide the phy. As a result usb probe completes without phy. >>>>>> >>>>>> Make ulpi_read_id() return -ETIMEDOUT to its user if the first test write >>>>>> fails. The user should then handle it appropriately. A follow up patch >>>>>> will make dwc3_core_init() set -EPROBE_DEFER in this case and bail out. >>>>>> >>>>>> Fixes: ef6a7bcfb01c ("usb: ulpi: Support device discovery via DT") >>>>>> Cc: stable@vger.kernel.org >>>>>> Signed-off-by: Ferry Toth <ftoth@exalondelft.nl> >>>>> Hi, >>>>> >>>>> this patch results in some qemu test failures, specifically xilinx-zynq-a9 >>>>> machine and zynq-zc702 as well as zynq-zed devicetree files, when trying >>>>> to boot from USB drive. The log shows >>>> I'm not familiar with that platform. Does it use dt to discover the ulpi >>>> device? >>>> >>> The dt usb description includes >>> >>> usb_phy0: phy0 { >>> compatible = "usb-nop-xceiv"; >>> #phy-cells = <0>; >>> }; >>> >>> ... >>> >>> &usb0 { >>> status = "okay"; >>> dr_mode = "host"; >>> usb-phy = <&usb_phy0>; >>> }; >>> >>> ... >>> >>> usb0: usb@e0002000 { >>> compatible = "xlnx,zynq-usb-2.20a", "chipidea,usb2"; >>> status = "disabled"; >>> clocks = <&clkc 28>; >>> interrupt-parent = <&intc>; >>> interrupts = <0 21 4>; >>> reg = <0xe0002000 0x1000>; >>> phy_type = "ulpi"; >>> }; >>> >>> The chipidea core initialization code includes >>> >>> if (!platdata->phy_mode) >>> platdata->phy_mode = of_usb_get_phy_mode(dev->of_node); >>> >>> Does that mean that every chipidea based usb implementation specifying >>> phy_type = "ulpi"; >>> in their devicetree description will now fail, plus maybe others >>> who determine the phy mode from devicetree ? >> I don't think so. >>>> I'm guessing that the problem is actually caused by "usb: ulpi: defer >>>> ulpi_register on ulpi_read_id timeout". >>>> >>> Confused. Isn't that this patch ? >> Ehem. Yes. >>>> ulpi_read_id() now returns ETIMEDOUT due to the test write ulpi_write(ulpi, >>>> ULPI_SCRATCH, 0xaa) failing. >>>> >>>> Maybe we can create a fix by skipping the test write in case dt discovery >>>> is available and calling of_device_request_module() directly, instead of >>>> masking the timed out test write as it was before? >>>> >>> I have no idea. All I can see is that it appears that there was a reason >>> for not returning an error if that test write failed. >> >> It seems to have been a quick patch to solve a power sequencing issue: >> >> "The ULPI bus code supports native enumeration by reading the >> vendor ID and product ID registers at device creation time, but >> we can't be certain that those register reads will succeed if the >> phy is not powered up. To avoid any problems with reading the ID >> registers before the phy is powered we fallback to DT matching >> when the ID reads fail. >> >> If the ULPI spec had some generic power sequencing for these >> registers we could put that into the ULPI bus layer and power up >> the device before reading the ID registers. Unfortunately this >> doesn't exist and the power sequence is usually device specific. >> By having the device matched up with DT we can avoid this >> problem." >> >> But as is, the code now requires a DT when there is a power >> sequencing issue, which is wrong for Merrifield. It seems my patch >> breaks the OF path, by replacing that by a deferred probe. >> >> I'm thinking the correct way would be: >> - if present use DT >> - else if test write fails, defer probe >> - else enumeration by reading the vendor ID and product ID registers >> > > I think this patch should be reverted until a better solution is found. > After all, at this point it is effectively unknown if there are other > users (besides devicetree) depending on ulpi_read_id() returning 0 if > the communication with the device fails. I don't see how any code could rely on not having DT and a timeout while attempting to enumerate and still expecting success (0). dwc3 in that case just assumes ulpi found and continues probe leading to non-functional dwc3 host. I think a fix should be relatively simple to find and resolve 2 bads. > Guenter ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v5 1/2] usb: ulpi: defer ulpi_register on ulpi_read_id timeout 2022-12-21 18:38 ` Ferry Toth @ 2022-12-21 18:48 ` Greg Kroah-Hartman 0 siblings, 0 replies; 14+ messages in thread From: Greg Kroah-Hartman @ 2022-12-21 18:48 UTC (permalink / raw) To: Ferry Toth Cc: Guenter Roeck, linux-usb, linux-kernel, Heikki Krogerus, Thinh Nguyen, Sean Anderson, Liu Shixin, Andrey Smirnov, Andy Shevchenko, stable On Wed, Dec 21, 2022 at 07:38:52PM +0100, Ferry Toth wrote: > Op 21-12-2022 om 18:30 schreef Guenter Roeck: > > On Wed, Dec 21, 2022 at 03:29:09PM +0100, Ferry Toth wrote: > > > Hi > > > > > > On 21-12-2022 13:41, Guenter Roeck wrote: > > > > On Wed, Dec 21, 2022 at 11:07:50AM +0100, Ferry Toth wrote: > > > > > Hi, > > > > > > > > > > On 20-12-2022 20:43, Guenter Roeck wrote: > > > > > > On Mon, Dec 05, 2022 at 09:15:26PM +0100, Ferry Toth wrote: > > > > > > > Since commit 0f0101719138 ("usb: dwc3: Don't switch OTG -> peripheral > > > > > > > if extcon is present") Dual Role support on Intel Merrifield platform > > > > > > > broke due to rearranging the call to dwc3_get_extcon(). > > > > > > > > > > > > > > It appears to be caused by ulpi_read_id() on the first test write failing > > > > > > > with -ETIMEDOUT. Currently ulpi_read_id() expects to discover the phy via > > > > > > > DT when the test write fails and returns 0 in that case, even if DT does not > > > > > > > provide the phy. As a result usb probe completes without phy. > > > > > > > > > > > > > > Make ulpi_read_id() return -ETIMEDOUT to its user if the first test write > > > > > > > fails. The user should then handle it appropriately. A follow up patch > > > > > > > will make dwc3_core_init() set -EPROBE_DEFER in this case and bail out. > > > > > > > > > > > > > > Fixes: ef6a7bcfb01c ("usb: ulpi: Support device discovery via DT") > > > > > > > Cc: stable@vger.kernel.org > > > > > > > Signed-off-by: Ferry Toth <ftoth@exalondelft.nl> > > > > > > Hi, > > > > > > > > > > > > this patch results in some qemu test failures, specifically xilinx-zynq-a9 > > > > > > machine and zynq-zc702 as well as zynq-zed devicetree files, when trying > > > > > > to boot from USB drive. The log shows > > > > > I'm not familiar with that platform. Does it use dt to discover the ulpi > > > > > device? > > > > > > > > > The dt usb description includes > > > > > > > > usb_phy0: phy0 { > > > > compatible = "usb-nop-xceiv"; > > > > #phy-cells = <0>; > > > > }; > > > > > > > > ... > > > > > > > > &usb0 { > > > > status = "okay"; > > > > dr_mode = "host"; > > > > usb-phy = <&usb_phy0>; > > > > }; > > > > > > > > ... > > > > > > > > usb0: usb@e0002000 { > > > > compatible = "xlnx,zynq-usb-2.20a", "chipidea,usb2"; > > > > status = "disabled"; > > > > clocks = <&clkc 28>; > > > > interrupt-parent = <&intc>; > > > > interrupts = <0 21 4>; > > > > reg = <0xe0002000 0x1000>; > > > > phy_type = "ulpi"; > > > > }; > > > > > > > > The chipidea core initialization code includes > > > > > > > > if (!platdata->phy_mode) > > > > platdata->phy_mode = of_usb_get_phy_mode(dev->of_node); > > > > > > > > Does that mean that every chipidea based usb implementation specifying > > > > phy_type = "ulpi"; > > > > in their devicetree description will now fail, plus maybe others > > > > who determine the phy mode from devicetree ? > > > I don't think so. > > > > > I'm guessing that the problem is actually caused by "usb: ulpi: defer > > > > > ulpi_register on ulpi_read_id timeout". > > > > > > > > > Confused. Isn't that this patch ? > > > Ehem. Yes. > > > > > ulpi_read_id() now returns ETIMEDOUT due to the test write ulpi_write(ulpi, > > > > > ULPI_SCRATCH, 0xaa) failing. > > > > > > > > > > Maybe we can create a fix by skipping the test write in case dt discovery > > > > > is available and calling of_device_request_module() directly, instead of > > > > > masking the timed out test write as it was before? > > > > > > > > > I have no idea. All I can see is that it appears that there was a reason > > > > for not returning an error if that test write failed. > > > > > > It seems to have been a quick patch to solve a power sequencing issue: > > > > > > "The ULPI bus code supports native enumeration by reading the > > > vendor ID and product ID registers at device creation time, but > > > we can't be certain that those register reads will succeed if the > > > phy is not powered up. To avoid any problems with reading the ID > > > registers before the phy is powered we fallback to DT matching > > > when the ID reads fail. > > > > > > If the ULPI spec had some generic power sequencing for these > > > registers we could put that into the ULPI bus layer and power up > > > the device before reading the ID registers. Unfortunately this > > > doesn't exist and the power sequence is usually device specific. > > > By having the device matched up with DT we can avoid this > > > problem." > > > > > > But as is, the code now requires a DT when there is a power > > > sequencing issue, which is wrong for Merrifield. It seems my patch > > > breaks the OF path, by replacing that by a deferred probe. > > > > > > I'm thinking the correct way would be: > > > - if present use DT > > > - else if test write fails, defer probe > > > - else enumeration by reading the vendor ID and product ID registers > > > > > > > I think this patch should be reverted until a better solution is found. > > After all, at this point it is effectively unknown if there are other > > users (besides devicetree) depending on ulpi_read_id() returning 0 if > > the communication with the device fails. > > I don't see how any code could rely on not having DT and a timeout while > attempting to enumerate and still expecting success (0). dwc3 in that case > just assumes ulpi found and continues probe leading to non-functional dwc3 > host. > I think a fix should be relatively simple to find and resolve 2 bads. Please send a revert now, as it obviously breaks things that were working, and then resubmit a new change based on what you have found to work properly for everyone. thanks, greg k-h ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v5 1/2] usb: ulpi: defer ulpi_register on ulpi_read_id timeout #forregzbot 2022-12-20 19:43 ` Guenter Roeck 2022-12-21 10:07 ` Ferry Toth @ 2022-12-22 12:45 ` Thorsten Leemhuis 1 sibling, 0 replies; 14+ messages in thread From: Thorsten Leemhuis @ 2022-12-22 12:45 UTC (permalink / raw) To: regressions; +Cc: linux-usb, linux-kernel, stable [Note: this mail contains only information for Linux kernel regression tracking. Mails like these contain '#forregzbot' in the subject to make then easy to spot and filter out. The author also tried to remove most or all individuals from the list of recipients to spare them the hassle.] On 20.12.22 20:43, Guenter Roeck wrote: > this patch results in some qemu test failures, specifically xilinx-zynq-a9 > machine and zynq-zc702 as well as zynq-zed devicetree files, when trying > to boot from USB drive. The log shows Thanks for the report. To be sure below issue doesn't fall through the cracks unnoticed, I'm adding it to regzbot, my Linux kernel regression tracking bot: #regzbot ^introduced 8a7b31d545d3 #regzbot title usb: ulpi: various qemu test failures #regzbot monitor: https://lore.kernel.org/all/20221221201805.19436-1-ftoth@exalondelft.nl/ #regzbot fix: Revert "usb: ulpi: defer ulpi_register on ulpi_read_id timeout" #regzbot ignore-activity Ciao, Thorsten (wearing his 'the Linux kernel's regression tracker' hat) P.S.: As the Linux kernel's regression tracker I deal with a lot of reports and sometimes miss something important when writing mails like this. If that's the case here, don't hesitate to tell me in a public reply, it's in everyone's interest to set the public record straight. ^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH v5 2/2] usb: dwc3: core: defer probe on ulpi_read_id timeout 2022-12-05 20:15 [PATCH v5 0/2] usb: dwc3: core: defer probe on ulpi_read_id timeout Ferry Toth 2022-12-05 20:15 ` [PATCH v5 1/2] usb: ulpi: defer ulpi_register " Ferry Toth @ 2022-12-05 20:15 ` Ferry Toth 2022-12-05 21:19 ` Thinh Nguyen 1 sibling, 1 reply; 14+ messages in thread From: Ferry Toth @ 2022-12-05 20:15 UTC (permalink / raw) To: linux-usb, linux-kernel Cc: Heikki Krogerus, Greg Kroah-Hartman, Thinh Nguyen, Sean Anderson, Liu Shixin, Ferry Toth, Andrey Smirnov, Andy Shevchenko, Ferry Toth, stable Since commit 0f0101719138 ("usb: dwc3: Don't switch OTG -> peripheral if extcon is present"), Dual Role support on Intel Merrifield platform broke due to rearranging the call to dwc3_get_extcon(). It appears to be caused by ulpi_read_id() masking the timeout on the first test write. In the past dwc3 probe continued by calling dwc3_core_soft_reset() followed by dwc3_get_extcon() which happend to return -EPROBE_DEFER. On deferred probe ulpi_read_id() finally succeeded. Due to above mentioned rearranging -EPROBE_DEFER is not returned and probe completes without phy. On Intel Merrifield the timeout on the first test write issue is reproducible but it is difficult to find the root cause. Using a mainline kernel and rootfs with buildroot ulpi_read_id() succeeds. As soon as adding ftrace / bootconfig to find out why, ulpi_read_id() fails and we can't analyze the flow. Using another rootfs ulpi_read_id() fails even without adding ftrace. We suspect the issue is some kind of timing / race, but merely retrying ulpi_read_id() does not resolve the issue. As we now changed ulpi_read_id() to return -ETIMEDOUT in this case, we need to handle the error by calling dwc3_core_soft_reset() and request -EPROBE_DEFER. On deferred probe ulpi_read_id() is retried and succeeds. Fixes: ef6a7bcfb01c ("usb: ulpi: Support device discovery via DT") Cc: stable@vger.kernel.org Signed-off-by: Ferry Toth <ftoth@exalondelft.nl> --- drivers/usb/dwc3/core.c | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c index 648f1c570021..2779f17bffaf 100644 --- a/drivers/usb/dwc3/core.c +++ b/drivers/usb/dwc3/core.c @@ -1106,8 +1106,13 @@ static int dwc3_core_init(struct dwc3 *dwc) if (!dwc->ulpi_ready) { ret = dwc3_core_ulpi_init(dwc); - if (ret) + if (ret) { + if (ret == -ETIMEDOUT) { + dwc3_core_soft_reset(dwc); + ret = -EPROBE_DEFER; + } goto err0; + } dwc->ulpi_ready = true; } -- 2.37.2 ^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH v5 2/2] usb: dwc3: core: defer probe on ulpi_read_id timeout 2022-12-05 20:15 ` [PATCH v5 2/2] usb: dwc3: core: defer probe on ulpi_read_id timeout Ferry Toth @ 2022-12-05 21:19 ` Thinh Nguyen 0 siblings, 0 replies; 14+ messages in thread From: Thinh Nguyen @ 2022-12-05 21:19 UTC (permalink / raw) To: Ferry Toth Cc: linux-usb, linux-kernel, Heikki Krogerus, Greg Kroah-Hartman, Thinh Nguyen, Sean Anderson, Liu Shixin, Ferry Toth, Andrey Smirnov, Andy Shevchenko, stable On Mon, Dec 05, 2022, Ferry Toth wrote: > Since commit 0f0101719138 ("usb: dwc3: Don't switch OTG -> peripheral > if extcon is present"), Dual Role support on Intel Merrifield platform > broke due to rearranging the call to dwc3_get_extcon(). > > It appears to be caused by ulpi_read_id() masking the timeout on the first > test write. In the past dwc3 probe continued by calling dwc3_core_soft_reset() > followed by dwc3_get_extcon() which happend to return -EPROBE_DEFER. > On deferred probe ulpi_read_id() finally succeeded. Due to above mentioned > rearranging -EPROBE_DEFER is not returned and probe completes without phy. > > On Intel Merrifield the timeout on the first test write issue is reproducible > but it is difficult to find the root cause. Using a mainline kernel and > rootfs with buildroot ulpi_read_id() succeeds. As soon as adding > ftrace / bootconfig to find out why, ulpi_read_id() fails and we can't > analyze the flow. Using another rootfs ulpi_read_id() fails even without > adding ftrace. We suspect the issue is some kind of timing / race, but > merely retrying ulpi_read_id() does not resolve the issue. > > As we now changed ulpi_read_id() to return -ETIMEDOUT in this case, we > need to handle the error by calling dwc3_core_soft_reset() and request > -EPROBE_DEFER. On deferred probe ulpi_read_id() is retried and succeeds. > > Fixes: ef6a7bcfb01c ("usb: ulpi: Support device discovery via DT") > Cc: stable@vger.kernel.org > Signed-off-by: Ferry Toth <ftoth@exalondelft.nl> > --- > drivers/usb/dwc3/core.c | 7 ++++++- > 1 file changed, 6 insertions(+), 1 deletion(-) > > diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c > index 648f1c570021..2779f17bffaf 100644 > --- a/drivers/usb/dwc3/core.c > +++ b/drivers/usb/dwc3/core.c > @@ -1106,8 +1106,13 @@ static int dwc3_core_init(struct dwc3 *dwc) > > if (!dwc->ulpi_ready) { > ret = dwc3_core_ulpi_init(dwc); > - if (ret) > + if (ret) { > + if (ret == -ETIMEDOUT) { > + dwc3_core_soft_reset(dwc); > + ret = -EPROBE_DEFER; > + } > goto err0; > + } > dwc->ulpi_ready = true; > } > > -- > 2.37.2 > Acked-by: Thinh Nguyen <Thinh.Nguyen@synopsys.com> Thanks, Thinh ^ permalink raw reply [flat|nested] 14+ messages in thread
end of thread, other threads:[~2022-12-22 12:45 UTC | newest] Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2022-12-05 20:15 [PATCH v5 0/2] usb: dwc3: core: defer probe on ulpi_read_id timeout Ferry Toth 2022-12-05 20:15 ` [PATCH v5 1/2] usb: ulpi: defer ulpi_register " Ferry Toth 2022-12-07 11:33 ` Heikki Krogerus 2022-12-20 19:43 ` Guenter Roeck 2022-12-21 10:07 ` Ferry Toth 2022-12-21 12:41 ` Guenter Roeck 2022-12-21 14:29 ` Ferry Toth 2022-12-21 17:30 ` Guenter Roeck 2022-12-21 18:23 ` Greg Kroah-Hartman 2022-12-21 18:38 ` Ferry Toth 2022-12-21 18:48 ` Greg Kroah-Hartman 2022-12-22 12:45 ` [PATCH v5 1/2] usb: ulpi: defer ulpi_register on ulpi_read_id timeout #forregzbot Thorsten Leemhuis 2022-12-05 20:15 ` [PATCH v5 2/2] usb: dwc3: core: defer probe on ulpi_read_id timeout Ferry Toth 2022-12-05 21:19 ` Thinh Nguyen
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).