Re: [PATCH v2 3/3] bootcount: Add driver model I2C driver
Hello Philip,
On 26.10.23 05:44, Heiko Schocher wrote:
> Hello Philip,
>
> On 20.10.23 11:02, Philip Richard Oberfichtner wrote:
>> This adds a generic I2C bootcounter adhering to driver model to replace
>> the previously removed legacy implementation.
>>
>> There is no change in functionality, it can be used on any I2C device.
>> The device tree configuration may look like this for example:
>>
>> bootcount {
>> compatible = "u-boot,bootcount-i2c";
>> i2cbcdev = <_rtc>;
>> offset = <0x11>;
>> };
>>
>> Signed-off-by: Philip Richard Oberfichtner
>> ---
>>
>> Changes in v2:
>> - Adaption of Kconfig help message
>> - Rename chip to bcdev
>> - Adapt probe to use i2c_get_chip_by_phandle()
>>
>> drivers/bootcount/Kconfig| 10 +++
>> drivers/bootcount/Makefile | 1 +
>> drivers/bootcount/bootcount_dm_i2c.c | 103 +++
>> 3 files changed, 114 insertions(+)
>> create mode 100644 drivers/bootcount/bootcount_dm_i2c.c
>
> Reviewed-by: Heiko Schocher
Your patch drops an checkpatch error:
"""
ERROR: Do not add common.h to files
#164: FILE: drivers/bootcount/bootcount_dm_i2c.c:10:
+#include
"""
Could you please check and fix?
Thanks!
bye,
Heiko
--
DENX Software Engineering GmbH, Managing Director: Erika Unter
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-52 Fax: +49-8142-66989-80 Email: h...@denx.de
Re: [RFC 01/13] cmd: bootefi: unfold do_bootefi_image()
Hi Ilias,
On Fri, Oct 27, 2023 at 03:23:07PM +0300, Ilias Apalodimas wrote:
> Akashi-san
>
> On Fri, 27 Oct 2023 at 04:00, Tom Rini wrote:
> >
> > On Fri, Oct 27, 2023 at 09:25:44AM +0900, AKASHI Takahiro wrote:
> > > On Thu, Oct 26, 2023 at 01:01:52PM +0200, Heinrich Schuchardt wrote:
> > > > On 10/26/23 07:30, AKASHI Takahiro wrote:
> > > > > Unfold do_bootefi_image() into do_bootefi() for the sake of the
> > > > > succeeding
> > > > > refactor work.
> > > > >
>
> I don't disagree with the patchset, but what the patch does is pretty
> obvious.
Yeah, that is what I tried to do in this patch series, i.e. each step
be as trivial as possible to ensure that the semantics is unchanged
while the code is being morphed.
> It would help a lot to briefly describe how the unfolding
> process helps the refactoring.
Not sure, but will add a few more words.
Thanks,
-Takahiro Akashi
> Thanks
> /Ilias
> > > > > Signed-off-by: AKASHI Takahiro
> > > > > ---
> > > > > cmd/bootefi.c | 101
> > > > > ++
> > > > > 1 file changed, 37 insertions(+), 64 deletions(-)
> > > > >
> > > > > diff --git a/cmd/bootefi.c b/cmd/bootefi.c
> > > > > index 20e5c94a33a4..1b28bf5a318d 100644
> > > > > --- a/cmd/bootefi.c
> > > > > +++ b/cmd/bootefi.c
> > > > > @@ -425,58 +425,6 @@ static int do_efibootmgr(void)
> > > > > return CMD_RET_SUCCESS;
> > > > > }
> > > > >
> > > > > -/**
> > > > > - * do_bootefi_image() - execute EFI binary
> > > > > - *
> > > > > - * Set up memory image for the binary to be loaded, prepare device
> > > > > path, and
> > > > > - * then call do_bootefi_exec() to execute it.
> > > > > - *
> > > > > - * @image_opt: string with image start address
> > > > > - * @size_opt:string with image size or NULL
> > > > > - * Return: status code
> > > > > - */
> > > > > -static int do_bootefi_image(const char *image_opt, const char
> > > > > *size_opt)
> > > > > -{
> > > > > - void *image_buf;
> > > > > - unsigned long addr, size;
> > > > > - efi_status_t ret;
> > > > > -
> > > > > -#ifdef CONFIG_CMD_BOOTEFI_HELLO
> > > > > - if (!strcmp(image_opt, "hello")) {
> > > > > - image_buf = __efi_helloworld_begin;
> > > > > - size = __efi_helloworld_end - __efi_helloworld_begin;
> > > > > - efi_clear_bootdev();
> > > > > - } else
> > > > > -#endif
> > > > > - {
> > > > > - addr = strtoul(image_opt, NULL, 16);
> > > > > - /* Check that a numeric value was passed */
> > > > > - if (!addr)
> > > > > - return CMD_RET_USAGE;
> > > > > - image_buf = map_sysmem(addr, 0);
> > > > > -
> > > > > - if (size_opt) {
> > > > > - size = strtoul(size_opt, NULL, 16);
> > > > > - if (!size)
> > > > > - return CMD_RET_USAGE;
> > > > > - efi_clear_bootdev();
> > > > > - } else {
> > > > > - if (image_buf != image_addr) {
> > > > > - log_err("No UEFI binary known at %s\n",
> > > > > - image_opt);
> > > > > - return CMD_RET_FAILURE;
> > > > > - }
> > > > > - size = image_size;
> > > > > - }
> > > > > - }
> > > > > - ret = efi_run_image(image_buf, size);
> > > > > -
> > > > > - if (ret != EFI_SUCCESS)
> > > > > - return CMD_RET_FAILURE;
> > > > > -
> > > > > - return CMD_RET_SUCCESS;
> > > > > -}
> > > > > -
> > > > > /**
> > > > >* efi_run_image() - run loaded UEFI image
> > > > >*
> > > > > @@ -648,8 +596,9 @@ static int do_bootefi(struct cmd_tbl *cmdtp, int
> > > > > flag, int argc,
> > > > > char *const argv[])
> > > > > {
> > > > > efi_status_t ret;
> > > > > - char *img_addr, *img_size, *str_copy, *pos;
> > > > > - void *fdt;
> > > > > + char *p;
> > > > > + void *fdt, *image_buf;
> > > > > + unsigned long addr, size;
> > > > >
> > > > > if (argc < 2)
> > > > > return CMD_RET_USAGE;
> > > > > @@ -684,18 +633,42 @@ static int do_bootefi(struct cmd_tbl *cmdtp,
> > > > > int flag, int argc,
> > > > > if (!strcmp(argv[1], "selftest"))
> > > > > return do_efi_selftest();
> > > > > #endif
> > > > > - str_copy = strdup(argv[1]);
> > > > > - if (!str_copy) {
> > > > > - log_err("Out of memory\n");
> > > > > - return CMD_RET_FAILURE;
> > > > > +
> > > > > +#ifdef CONFIG_CMD_BOOTEFI_HELLO
> > > >
> > > > I would prefer a normal if and let the linker sort it out.
> > >
> > > Here I focused on simply unfolding("moving") the function, keeping the
> > > code
> > > as same as possible. Any how the code is in a tentative form
> > > at this point and will be reshaped in later patches, using IS_ENABLED().
> > > Please look at the final code after applying the whole series.
> >
> > I also agree with this approach.
> >
> > --
> > Tom
[PATCH] dfu: mmc: Add support for exposing whole mmc device
Add support for exposing the whole mmc device by setting the 'size'
parameter to 0. This can be useful in case it is not clear what the
total device size is up front. Update the documentation accordingly.
Signed-off-by: Marek Vasut
---
Cc: Lukasz Majewski
Cc: Mattijs Korpershoek
Cc: Tom Rini
---
doc/usage/dfu.rst | 5 +
drivers/dfu/dfu_mmc.c | 10 ++
2 files changed, 15 insertions(+)
diff --git a/doc/usage/dfu.rst b/doc/usage/dfu.rst
index 68cacbbef66..8845a71df36 100644
--- a/doc/usage/dfu.rst
+++ b/doc/usage/dfu.rst
@@ -121,6 +121,11 @@ mmc
with
+offset
+is the offset in the device (hexadecimal without "0x")
+size
+is the size of the access area (hexadecimal without "0x")
+or 0 which means whole device
partid
being the GPT or DOS partition index,
num
diff --git a/drivers/dfu/dfu_mmc.c b/drivers/dfu/dfu_mmc.c
index cdb3c18b01d..12c54e90ef7 100644
--- a/drivers/dfu/dfu_mmc.c
+++ b/drivers/dfu/dfu_mmc.c
@@ -386,6 +386,16 @@ int dfu_fill_entity_mmc(struct dfu_entity *dfu, char
*devstr, char **argv, int a
dfu->data.mmc.lba_size = third_arg;
dfu->data.mmc.lba_blk_size = mmc->read_bl_len;
+ /*
+* In case the size is zero (i.e. mmc raw 0x10 0),
+* assume the user intends to use whole device.
+*/
+ if (third_arg == 0) {
+ struct blk_desc *blk_dev = mmc_get_blk_desc(mmc);
+
+ dfu->data.mmc.lba_size = blk_dev->lba;
+ }
+
/*
* Check for an extra entry at dfu_alt_info env variable
* specifying the mmc HW defined partition number
--
2.42.0
Re: [PATCH] fixup! usb: xhci: Guard all calls to xhci_wait_for_event
On Sun, Oct 29, 2023 at 2:33 PM Peter Robinson wrote: > > On Sun, Oct 29, 2023 at 9:25 PM Marek Vasut wrote: > > > > On 10/27/23 08:03, Hector Martin wrote: > > > On 27/10/2023 09.36, Marek Vasut wrote: > > >> On 10/27/23 01:26, Hector Martin wrote: > > >>> Gah, I should've paid more attention to that rebase. Here's a dumb > > >>> fixup for this patch. I'll squash it into a v2 if needed alongside > > >>> any other changes, or if it looks good feel free to apply/squash > > >>> it in directly. > > >>> > > >>> --- > > >>>drivers/usb/host/xhci-ring.c | 1 + > > >>>1 file changed, 1 insertion(+) > > >>> > > >>> diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c > > >>> index e2bd2e999a8e..7f2507be0cf0 100644 > > >>> --- a/drivers/usb/host/xhci-ring.c > > >>> +++ b/drivers/usb/host/xhci-ring.c > > >>> @@ -532,6 +532,7 @@ static void reset_ep(struct usb_device *udev, int > > >>> ep_index) > > >>> event = xhci_wait_for_event(ctrl, TRB_COMPLETION); > > >>> if (!event) > > >>> return; > > >>> + field = le32_to_cpu(event->trans_event.flags); > > >>> BUG_ON(TRB_TO_SLOT_ID(field) != udev->slot_id); > > >>> xhci_acknowledge_event(ctrl); > > >> > > >> Please squash, and add > > >> > > >> Reviewed-by: Marek Vasut > > >> > > >> Also, +CC Minda, > > >> > > >> there has been a similar fix to this one but with much more information > > >> about the failure, see: > > >> > > >> [PATCH v1] usb: xhci: Check return value of wait for TRB_TRANSFER event > > >> > > >> I think it would be useful to somehow include that information, so it > > >> wouldn't be lost. > > > > > > The root cause for *that* failure is what I fix in patch #5. From that > > > thread: > > > > > >> scanning bus xhci_pci for devices... WARN halted endpoint, queueing > > > URB anyway. > > > > > > Without #5 and without #1, that situation then leads to fireworks. > > > > > > A bunch of things are broken, which is why this is a series and not a > > > single patch. I have more fixes to timeout handling, mass storage, etc. > > > coming up after this. I fixed most of this in one long day of trying > > > random USB devices, so it's not so much subtle super specific problems I > > > could document the crashes for as just wide-ranging problems in the > > > u-boot USB stack. None of this is particularly hard to repro if you just > > > try a bunch of normal consumer USB devices, including things like USB > > > HDDs which take time to spin-up leading to timeouts etc. > > > > I think majority of users I can think of use USB mass storage devices, > > like USB pen drives, not so much HDDs as far as I can tell. > > We see a bunch of spinning rust users in Fedora, these sorts of > devices are used by a bunch of people that want to run up cheap home > NAS devices so from experience I'd say while not usual to be USB > sticks and some form of solid state storage, spinning disk isn't > unusual. What Peter said. A very common use case, even more so than USB flash drives, is for the consumers to plug in a USB HDD to their routers or home NAS, as a cheap and quick solution. I've seen this type of timeout failure happen quite often with large >= 3TB HDDs in USB enclosure. All the best, Tony > > > > The crash dumps > > > aren't particularly useful, it's the subtleties of the xHCI interactions > > > that are, but for that you need to add and enable a lot more debugging > > > (patch #8). > > > > The crash dumps are more for posterity, when someone searches for a > > problem they see. Search engines tend to pick those up and it might > > point those people in the right direction. > > > > Also, it is good to include information about what triggered the crash, > > e.g. which USB device on which hardware, in case this is needed in the > > future. > > > > > I think the main reason all this stuff is broken and we're finding out > > > now is that u-boot has traditionally been used in embedded devices with > > > fairly narrow use cases for USB > > > > Yes > > > > >, and now we're running it on > > > workstation-grade laptops and desktops and people are plugging in all > > > kinds of normal USB devices and expecting their bootloader not to freak > > > out with them (even though most of the time it doesn't need to talk to > > > them). It's really easy to run into this stuff when that's your use case. > > > > It is really appreciated to see people fix things like this stuff, thanks !
Re: [PATCH] fixup! usb: xhci: Guard all calls to xhci_wait_for_event
On Sun, Oct 29, 2023 at 9:25 PM Marek Vasut wrote: > > On 10/27/23 08:03, Hector Martin wrote: > > On 27/10/2023 09.36, Marek Vasut wrote: > >> On 10/27/23 01:26, Hector Martin wrote: > >>> Gah, I should've paid more attention to that rebase. Here's a dumb > >>> fixup for this patch. I'll squash it into a v2 if needed alongside > >>> any other changes, or if it looks good feel free to apply/squash > >>> it in directly. > >>> > >>> --- > >>>drivers/usb/host/xhci-ring.c | 1 + > >>>1 file changed, 1 insertion(+) > >>> > >>> diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c > >>> index e2bd2e999a8e..7f2507be0cf0 100644 > >>> --- a/drivers/usb/host/xhci-ring.c > >>> +++ b/drivers/usb/host/xhci-ring.c > >>> @@ -532,6 +532,7 @@ static void reset_ep(struct usb_device *udev, int > >>> ep_index) > >>> event = xhci_wait_for_event(ctrl, TRB_COMPLETION); > >>> if (!event) > >>> return; > >>> + field = le32_to_cpu(event->trans_event.flags); > >>> BUG_ON(TRB_TO_SLOT_ID(field) != udev->slot_id); > >>> xhci_acknowledge_event(ctrl); > >> > >> Please squash, and add > >> > >> Reviewed-by: Marek Vasut > >> > >> Also, +CC Minda, > >> > >> there has been a similar fix to this one but with much more information > >> about the failure, see: > >> > >> [PATCH v1] usb: xhci: Check return value of wait for TRB_TRANSFER event > >> > >> I think it would be useful to somehow include that information, so it > >> wouldn't be lost. > > > > The root cause for *that* failure is what I fix in patch #5. From that > > thread: > > > >> scanning bus xhci_pci for devices... WARN halted endpoint, queueing > > URB anyway. > > > > Without #5 and without #1, that situation then leads to fireworks. > > > > A bunch of things are broken, which is why this is a series and not a > > single patch. I have more fixes to timeout handling, mass storage, etc. > > coming up after this. I fixed most of this in one long day of trying > > random USB devices, so it's not so much subtle super specific problems I > > could document the crashes for as just wide-ranging problems in the > > u-boot USB stack. None of this is particularly hard to repro if you just > > try a bunch of normal consumer USB devices, including things like USB > > HDDs which take time to spin-up leading to timeouts etc. > > I think majority of users I can think of use USB mass storage devices, > like USB pen drives, not so much HDDs as far as I can tell. We see a bunch of spinning rust users in Fedora, these sorts of devices are used by a bunch of people that want to run up cheap home NAS devices so from experience I'd say while not usual to be USB sticks and some form of solid state storage, spinning disk isn't unusual. > > The crash dumps > > aren't particularly useful, it's the subtleties of the xHCI interactions > > that are, but for that you need to add and enable a lot more debugging > > (patch #8). > > The crash dumps are more for posterity, when someone searches for a > problem they see. Search engines tend to pick those up and it might > point those people in the right direction. > > Also, it is good to include information about what triggered the crash, > e.g. which USB device on which hardware, in case this is needed in the > future. > > > I think the main reason all this stuff is broken and we're finding out > > now is that u-boot has traditionally been used in embedded devices with > > fairly narrow use cases for USB > > Yes > > >, and now we're running it on > > workstation-grade laptops and desktops and people are plugging in all > > kinds of normal USB devices and expecting their bootloader not to freak > > out with them (even though most of the time it doesn't need to talk to > > them). It's really easy to run into this stuff when that's your use case. > > It is really appreciated to see people fix things like this stuff, thanks !
Re: [PATCH] fixup! usb: xhci: Guard all calls to xhci_wait_for_event
On 10/27/23 08:03, Hector Martin wrote: On 27/10/2023 09.36, Marek Vasut wrote: On 10/27/23 01:26, Hector Martin wrote: Gah, I should've paid more attention to that rebase. Here's a dumb fixup for this patch. I'll squash it into a v2 if needed alongside any other changes, or if it looks good feel free to apply/squash it in directly. --- drivers/usb/host/xhci-ring.c | 1 + 1 file changed, 1 insertion(+) diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c index e2bd2e999a8e..7f2507be0cf0 100644 --- a/drivers/usb/host/xhci-ring.c +++ b/drivers/usb/host/xhci-ring.c @@ -532,6 +532,7 @@ static void reset_ep(struct usb_device *udev, int ep_index) event = xhci_wait_for_event(ctrl, TRB_COMPLETION); if (!event) return; + field = le32_to_cpu(event->trans_event.flags); BUG_ON(TRB_TO_SLOT_ID(field) != udev->slot_id); xhci_acknowledge_event(ctrl); Please squash, and add Reviewed-by: Marek Vasut Also, +CC Minda, there has been a similar fix to this one but with much more information about the failure, see: [PATCH v1] usb: xhci: Check return value of wait for TRB_TRANSFER event I think it would be useful to somehow include that information, so it wouldn't be lost. The root cause for *that* failure is what I fix in patch #5. From that thread: scanning bus xhci_pci for devices... WARN halted endpoint, queueing URB anyway. Without #5 and without #1, that situation then leads to fireworks. A bunch of things are broken, which is why this is a series and not a single patch. I have more fixes to timeout handling, mass storage, etc. coming up after this. I fixed most of this in one long day of trying random USB devices, so it's not so much subtle super specific problems I could document the crashes for as just wide-ranging problems in the u-boot USB stack. None of this is particularly hard to repro if you just try a bunch of normal consumer USB devices, including things like USB HDDs which take time to spin-up leading to timeouts etc. I think majority of users I can think of use USB mass storage devices, like USB pen drives, not so much HDDs as far as I can tell. The crash dumps aren't particularly useful, it's the subtleties of the xHCI interactions that are, but for that you need to add and enable a lot more debugging (patch #8). The crash dumps are more for posterity, when someone searches for a problem they see. Search engines tend to pick those up and it might point those people in the right direction. Also, it is good to include information about what triggered the crash, e.g. which USB device on which hardware, in case this is needed in the future. I think the main reason all this stuff is broken and we're finding out now is that u-boot has traditionally been used in embedded devices with fairly narrow use cases for USB Yes , and now we're running it on workstation-grade laptops and desktops and people are plugging in all kinds of normal USB devices and expecting their bootloader not to freak out with them (even though most of the time it doesn't need to talk to them). It's really easy to run into this stuff when that's your use case. It is really appreciated to see people fix things like this stuff, thanks !
Re: [PATCH] usb: Ignore endpoints in non-zero altsettings
On 10/29/23 16:50, Hector Martin wrote:
On 29/10/2023 21.04, Marek Vasut wrote:
On 10/29/23 08:24, Hector Martin wrote:
We currently do not really handle altsettings properly, and no driver
uses them. Ignore the respective endpoint descriptors for secondary
altsettings, to avoid creating duplicate endpoint records in the
interface.
This will have to be revisited if/when we have a driver that needs
altsettings to work properly.
Signed-off-by: Hector Martin
---
common/usb.c | 9 +
1 file changed, 9 insertions(+)
diff --git a/common/usb.c b/common/usb.c
index aad13fd9c557..90f72fda00bc 100644
--- a/common/usb.c
+++ b/common/usb.c
@@ -463,6 +463,15 @@ static int usb_parse_config(struct usb_device *dev,
puts("Endpoint descriptor out of order!\n");
break;
}
+ if (if_desc->num_altsetting > 1) {
+ /*
+* Ignore altsettings, which can trigger
duplicate
+* endpoint errors below. Revisit this when some
+* driver actually needs altsettings with
differing
+* endpoint setups.
+*/
How do you trigger this error ?
Plug in a device with altsettings, like most sound cards or UVC devices.
Please add that to the commit message.
Re: [PATCH 4/4] usb: storage: Implement 64-bit LBA support
On 10/29/23 16:54, Hector Martin wrote:
On 29/10/2023 21.11, Marek Vasut wrote:
On 10/29/23 08:23, Hector Martin wrote:
This makes things work properly on devices with >= 2 TiB
capacity. If u-boot is built without CONFIG_SYS_64BIT_LBA,
the capacity will be clamped at 2^32 - 1 sectors.
Signed-off-by: Hector Martin
---
common/usb_storage.c | 132
---
1 file changed, 114 insertions(+), 18 deletions(-)
diff --git a/common/usb_storage.c b/common/usb_storage.c
index 95507ffbce48..3035f2ee9868 100644
--- a/common/usb_storage.c
+++ b/common/usb_storage.c
@@ -66,7 +66,7 @@
static const unsigned char us_direction[256/8] = {
0x28, 0x81, 0x14, 0x14, 0x20, 0x01, 0x90, 0x77,
0x0C, 0x20, 0x00, 0x04, 0x00, 0x00, 0x00, 0x00,
- 0x00, 0x00, 0x00, 0x00, 0x00, 0x01, 0x00, 0x01,
+ 0x00, 0x01, 0x00, 0x40, 0x00, 0x01, 0x00, 0x01,
What changed here ?
This is an incomplete bitmap specifying the data transfer direction for
every possible SCSI command ID. I'm adding the new commands I'm now using.
Can you please add a code comment here, so others wouldn't ponder about
this too ?
Re: [PATCH v2 0/8] USB fixes: xHCI error handling
On Sun, Oct 29, 2023 at 2:38 AM Hector Martin wrote: > > This series is the first of a few bundles of USB fixes we have been > carrying downstream on the Asahi U-Boot branch for a few months. > > Patches #1-#6 fix a series of related robustness issues. Certain > conditions related to endpoint stalls revealed a chain of bugs > throughout the stack that caused U-Boot to completely fail when > encountering some errors (and errors are a fact of life with USB). > > Example scenario: > - The device stalls a bulk endpoint due to an error > - The upper layer driver tries to use the endpoint again > - There is no endpoint stall clear wired up in the API, so for starters > this is doomed to fail (fix: #4) > - xHCI knows the endpoint is halted, but tries to queue the TRB anyway, > which can't work (fix: #5) > - Since the endpoint is halted nothing happens, so the transfer times > out. > - The timeout handling tries to abort the transfer > - abort_td() tries to stop the endpoint, but since it is already halted, > this results in a context state error. As the transfer never started, > there is no completion event, so xhci_wait_for_event() is waiting for > the wrong event type, and logs an error and returns NULL. (fix: #2) > - abort_td() crashes due to failing to guard against the NULL event > (fix: #1) > - Even after fixing all that, abort_td() would not handle the context > state error properly and BUG() (fix: #3). This also fixes a race > condition documented in the xHCI spec that could occur even in the > absence of all the other bugs. > > Patch #6 addresses a related robustness issue where > xhci_wait_for_event() panics on event timeouts other than for transfers. > While this is clearly an unexpected condition and indicates a bug > elsewhere, it's no reason to outright crash. USB is notoriously > impossible to get 100% right, and we can't afford to be breaking users' > systems at any sign of trouble. Error conditions should always be dealt > with as gracefully as possible (even if that results in a completely > broken USB controller, that is much preferable to aborting the boot > process entirely, especially on devices with non-USB storage and > keyboards where USB support is effectively optional for most users). > Since after patch #1 we now guard all callers to xhci_wait_for_event() > with at least trivial NULL checks, it's okay to return and continue in > case of timeouts. > > Patch #7 addresses an unrelated bug I ran into while debugging all this, > and patch #8 adds extra debug logs to make finding future issues less > painful. > > I believe this should fix this Fedora bug too, which is either an > instance of the above sequence of events, or (more likely, given the > difficulty reproducing) the race condition documented in xHCI 4.6.9: > > https://bugzilla.redhat.com/show_bug.cgi?id=2244305 > > Signed-off-by: Hector Martin > --- > Changes in v2: > - Squashed in a trivial fix for patch #1 > - Removed spurious blank line > - Added a longer description to the cover letter > - Link to v1: > https://lore.kernel.org/r/20231027-usb-fixes-1-v1-0-1c879bbcd...@marcan.st > > --- > Hector Martin (8): > usb: xhci: Guard all calls to xhci_wait_for_event > usb: xhci: Better error handling in abort_td() > usb: xhci: Allow context state errors when halting an endpoint > usb: xhci: Recover from halted bulk endpoints > usb: xhci: Fail on attempt to queue TRBs to a halted endpoint > usb: xhci: Do not panic on event timeouts > usb: xhci: Fix DMA address calculation in queue_trb > usb: xhci: Add more debugging > > drivers/usb/host/xhci-ring.c | 99 > > drivers/usb/host/xhci.c | 9 > include/usb/xhci.h | 2 + > 3 files changed, 92 insertions(+), 18 deletions(-) > --- > base-commit: 7c0d668103abae3ec14cd96d882ba20134bb4529 > change-id: 20231027-usb-fixes-1-83bfc7013012 > Series LGTM. Reviewed-by: Neal Gompa -- 真実はいつも一つ！/ Always, there's only one truth!
Re: [PATCH 4/4] usb: storage: Implement 64-bit LBA support
On 29/10/2023 21.11, Marek Vasut wrote:
> On 10/29/23 08:23, Hector Martin wrote:
>> This makes things work properly on devices with >= 2 TiB
>> capacity. If u-boot is built without CONFIG_SYS_64BIT_LBA,
>> the capacity will be clamped at 2^32 - 1 sectors.
>>
>> Signed-off-by: Hector Martin
>> ---
>> common/usb_storage.c | 132
>> ---
>> 1 file changed, 114 insertions(+), 18 deletions(-)
>>
>> diff --git a/common/usb_storage.c b/common/usb_storage.c
>> index 95507ffbce48..3035f2ee9868 100644
>> --- a/common/usb_storage.c
>> +++ b/common/usb_storage.c
>> @@ -66,7 +66,7 @@
>> static const unsigned char us_direction[256/8] = {
>> 0x28, 0x81, 0x14, 0x14, 0x20, 0x01, 0x90, 0x77,
>> 0x0C, 0x20, 0x00, 0x04, 0x00, 0x00, 0x00, 0x00,
>> -0x00, 0x00, 0x00, 0x00, 0x00, 0x01, 0x00, 0x01,
>> +0x00, 0x01, 0x00, 0x40, 0x00, 0x01, 0x00, 0x01,
>
> What changed here ?
This is an incomplete bitmap specifying the data transfer direction for
every possible SCSI command ID. I'm adding the new commands I'm now using.
>
>> 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00
>> };
>> #define US_DIRECTION(x) ((us_direction[x>>3] >> (x & 7)) & 1)
>> @@ -1073,6 +1073,27 @@ static int usb_read_capacity(struct scsi_cmd *srb,
>> struct us_data *ss)
>> return -1;
>> }
>
> [...]
>
>> +#ifdef CONFIG_SYS_64BIT_LBA
>
> Could you try and use CONFIG_IS_ENABLED() ?
Sure.
>
>> +if (capacity == 0x1) {
>> +if (usb_read_capacity64(pccb, ss) != 0) {
>> +puts("READ_CAP64 ERROR\n");
>> +} else {
>> +debug("Read Capacity 64 returns: 0x%08x, 0x%08x,
>> 0x%08x\n",
>> + cap[0], cap[1], cap[2]);
>> +capacity = be64_to_cpu(*(uint64_t *)cap) + 1;
>> +blksz = be32_to_cpu(cap[2]);
>> +}
>> +}
>> +#else
>> +/*
>> + * READ CAPACITY will return 0x when limited,
>> + * which wraps to 0 with the +1 above
>> + */
>> +if (!capacity) {
>> +puts("LBA exceeds 32 bits but 64-bit LBA is disabled.\n");
>> +capacity = ~0;
>> +}
>> #endif
>
>
- Hector
Re: [PATCH] usb: Ignore endpoints in non-zero altsettings
On 29/10/2023 21.04, Marek Vasut wrote:
> On 10/29/23 08:24, Hector Martin wrote:
>> We currently do not really handle altsettings properly, and no driver
>> uses them. Ignore the respective endpoint descriptors for secondary
>> altsettings, to avoid creating duplicate endpoint records in the
>> interface.
>>
>> This will have to be revisited if/when we have a driver that needs
>> altsettings to work properly.
>>
>> Signed-off-by: Hector Martin
>> ---
>> common/usb.c | 9 +
>> 1 file changed, 9 insertions(+)
>>
>> diff --git a/common/usb.c b/common/usb.c
>> index aad13fd9c557..90f72fda00bc 100644
>> --- a/common/usb.c
>> +++ b/common/usb.c
>> @@ -463,6 +463,15 @@ static int usb_parse_config(struct usb_device *dev,
>> puts("Endpoint descriptor out of order!\n");
>> break;
>> }
>> +if (if_desc->num_altsetting > 1) {
>> +/*
>> + * Ignore altsettings, which can trigger
>> duplicate
>> + * endpoint errors below. Revisit this when some
>> + * driver actually needs altsettings with
>> differing
>> + * endpoint setups.
>> + */
>
> How do you trigger this error ?
>
Plug in a device with altsettings, like most sound cards or UVC devices.
- Hector
Re: [PATCH v2 8/8] usb: xhci: Add more debugging
On 10/29/23 07:37, Hector Martin wrote:
A bunch of miscellaneous debug messages to aid in working out USB
issues.
Signed-off-by: Hector Martin
---
drivers/usb/host/xhci-ring.c | 29 ++---
1 file changed, 26 insertions(+), 3 deletions(-)
diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
index b60661fe05e7..dabe6cf86af2 100644
--- a/drivers/usb/host/xhci-ring.c
+++ b/drivers/usb/host/xhci-ring.c
@@ -214,6 +214,9 @@ static dma_addr_t queue_trb(struct xhci_ctrl *ctrl, struct
xhci_ring *ring,
addr = xhci_trb_virt_to_dma(ring->enq_seg, (union xhci_trb *)trb);
+ debug("trb @ %llx: %08x %08x %08x %08x\n", addr,
+ trb_fields[0], trb_fields[1], trb_fields[2], trb_fields[3]);
+
Could you please use dev_dbg() instead of debug() ?
That way you'd also get the device prefix in the output.
Re: [PATCH v2 7/8] usb: xhci: Fix DMA address calculation in queue_trb
On 10/29/23 07:37, Hector Martin wrote: We need to get the DMA address before incrementing the pointer, as that might move us onto another segment. Signed-off-by: Hector Martin Reviewed-by: Marek Vasut
Re: [PATCH v2 6/8] usb: xhci: Do not panic on event timeouts
On 10/29/23 07:37, Hector Martin wrote:
Now that we always check the return value, just return NULL on timeouts.
We can still log the error since this is a problem, but it's not reason
to panic.
Signed-off-by: Hector Martin
---
drivers/usb/host/xhci-ring.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
index a969eafdc8ee..ae0ab5744df0 100644
--- a/drivers/usb/host/xhci-ring.c
+++ b/drivers/usb/host/xhci-ring.c
@@ -494,8 +494,9 @@ union xhci_trb *xhci_wait_for_event(struct xhci_ctrl *ctrl,
trb_type expected)
if (expected == TRB_TRANSFER)
return NULL;
- printf("XHCI timeout on event type %d... cannot recover.\n", expected);
- BUG();
+ printf("XHCI timeout on event type %d...\n", expected);
+
+ return NULL;
Reviewed-by: Marek Vasut
Re: [PATCH v2 5/8] usb: xhci: Fail on attempt to queue TRBs to a halted endpoint
On 10/29/23 07:37, Hector Martin wrote:
This isn't going to work, don't pretend it will and then end up timing
out.
Signed-off-by: Hector Martin
---
drivers/usb/host/xhci-ring.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
index db8b8f200250..a969eafdc8ee 100644
--- a/drivers/usb/host/xhci-ring.c
+++ b/drivers/usb/host/xhci-ring.c
@@ -243,7 +243,8 @@ static int prepare_ring(struct xhci_ctrl *ctrl, struct
xhci_ring *ep_ring,
puts("WARN waiting for error on ep to be cleared\n");
return -EINVAL;
case EP_STATE_HALTED:
- puts("WARN halted endpoint, queueing URB anyway.\n");
+ puts("WARN endpoint is halted\n");
+ return -EINVAL;
Reviewed-by: Marek Vasut
Re: [PATCH v2 4/8] usb: xhci: Recover from halted bulk endpoints
On 10/29/23 07:37, Hector Martin wrote: There is currently no codepath to recover from this case. In principle we could require that the upper layer do this explicitly, but let's just do it in xHCI when the next bulk transfer is started, since that reasonably implies whatever caused the problem has been dealt with. Signed-off-by: Hector Martin Reviewed-by: Marek Vasut
Re: [PATCH v2 3/8] usb: xhci: Allow context state errors when halting an endpoint
On 10/29/23 07:37, Hector Martin wrote:
There is a race where an endpoint may halt by itself while we are trying
to halt it, which results in a context state error. See xHCI 4.6.9 which
mentions this case.
This also avoids BUGging when we attempt to stop an endpoint which was
already stopped to begin with, which is probably a bug elsewhere but
not a good reason to crash.
Signed-off-by: Hector Martin
---
drivers/usb/host/xhci-ring.c | 6 --
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
index d21e76e0bdb6..e02a6e300c4f 100644
--- a/drivers/usb/host/xhci-ring.c
+++ b/drivers/usb/host/xhci-ring.c
@@ -545,6 +545,7 @@ static void abort_td(struct usb_device *udev, int ep_index)
struct xhci_ctrl *ctrl = xhci_get_ctrl(udev);
struct xhci_ring *ring = ctrl->devs[udev->slot_id]->eps[ep_index].ring;
union xhci_trb *event;
+ xhci_comp_code comp;
trb_type type;
u64 addr;
u32 field;
@@ -573,10 +574,11 @@ static void abort_td(struct usb_device *udev, int
ep_index)
printf("abort_td: Expected a TRB_TRANSFER TRB first\n");
}
+ comp = GET_COMP_CODE(le32_to_cpu(event->event_cmd.status));
BUG_ON(type != TRB_COMPLETION ||
TRB_TO_SLOT_ID(le32_to_cpu(event->event_cmd.flags))
- != udev->slot_id || GET_COMP_CODE(le32_to_cpu(
- event->event_cmd.status)) != COMP_SUCCESS);
+ != udev->slot_id || (comp != COMP_SUCCESS && comp
+ != COMP_CTX_STATE));
Can you please, in the process, reformat this condition so it is more
readable ?
xhci_acknowledge_event(ctrl);
addr = xhci_trb_virt_to_dma(ring->enq_seg,
Reviewed-by: Marek Vasut
Re: [PATCH v2 2/8] usb: xhci: Better error handling in abort_td()
On 10/29/23 07:37, Hector Martin wrote: If the xHC has a problem with our STOP ENDPOINT command, it is likely to return a completion directly instead of first a transfer event for the in-progress transfer. Handle that more gracefully. We still BUG() on the error code, but at least we don't end up timing out on the event and ending up with unexpected event errors. Signed-off-by: Hector Martin Reviewed-by: Marek Vasut
Re: [PATCH v2 1/8] usb: xhci: Guard all calls to xhci_wait_for_event
On 10/29/23 07:37, Hector Martin wrote: xhci_wait_for_event returns NULL on timeout, so the caller always has to check for that. This addresses immediate explosions in this part of the code when timeouts happen, but not the root cause for the timeout. Signed-off-by: Hector Martin Reviewed-by: Marek Vasut
Re: [PATCH 2/2] usb: hub: Add missing reset recovery delay
On 10/29/23 08:09, Hector Martin wrote: Some devices like YubiKeys need more time before SET_ADDRESS. The spec says we need to wait 10ms. Signed-off-by: Hector Martin Reviewed-by: Marek Vasut
Re: [PATCH 1/2] usb: kbd: Ignore Yubikeys
On 10/29/23 08:09, Hector Martin wrote:
We currently only support one USB keyboard device, but some devices
emulate keyboards for other purposes. Most commonly, people run into
this with Yubikeys, so let's ignore those.
Even if we end up supporting multiple keyboards in the future, it's
safer to ignore known non-keyboard devices.
This is particularly important to avoid regressing some users, since
YubiKeys often *don't* work due to other bugs in the USB stack, but will
start to work once they are fixed.
Signed-off-by: Hector Martin
---
common/usb_kbd.c | 19 +++
1 file changed, 19 insertions(+)
diff --git a/common/usb_kbd.c b/common/usb_kbd.c
index 352d86fb2ece..e8c102c567e4 100644
--- a/common/usb_kbd.c
+++ b/common/usb_kbd.c
@@ -120,6 +120,15 @@ struct usb_kbd_pdata {
extern int __maybe_unused net_busy_flag;
+/*
+ * Since we only support one usbkbd device in the iomux,
+ * ignore common keyboard-emulating devices that aren't
+ * real keyboards.
+ */
+const uint16_t vid_blocklist[] = {
+ 0x1050, /* Yubico */
I wonder if it would be better to have this default, but make the list
configurable via environment variable too. This way, if users run into
more weird devices, they could just:
=> setenv vid_blocklist "0x1234,0x5678" ; saveenv ; usb reset
Re: [PATCH 4/4] usb: storage: Implement 64-bit LBA support
On 10/29/23 08:23, Hector Martin wrote:
This makes things work properly on devices with >= 2 TiB
capacity. If u-boot is built without CONFIG_SYS_64BIT_LBA,
the capacity will be clamped at 2^32 - 1 sectors.
Signed-off-by: Hector Martin
---
common/usb_storage.c | 132 ---
1 file changed, 114 insertions(+), 18 deletions(-)
diff --git a/common/usb_storage.c b/common/usb_storage.c
index 95507ffbce48..3035f2ee9868 100644
--- a/common/usb_storage.c
+++ b/common/usb_storage.c
@@ -66,7 +66,7 @@
static const unsigned char us_direction[256/8] = {
0x28, 0x81, 0x14, 0x14, 0x20, 0x01, 0x90, 0x77,
0x0C, 0x20, 0x00, 0x04, 0x00, 0x00, 0x00, 0x00,
- 0x00, 0x00, 0x00, 0x00, 0x00, 0x01, 0x00, 0x01,
+ 0x00, 0x01, 0x00, 0x40, 0x00, 0x01, 0x00, 0x01,
What changed here ?
0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00
};
#define US_DIRECTION(x) ((us_direction[x>>3] >> (x & 7)) & 1)
@@ -1073,6 +1073,27 @@ static int usb_read_capacity(struct scsi_cmd *srb,
struct us_data *ss)
return -1;
}
[...]
+#ifdef CONFIG_SYS_64BIT_LBA
Could you try and use CONFIG_IS_ENABLED() ?
+ if (capacity == 0x1) {
+ if (usb_read_capacity64(pccb, ss) != 0) {
+ puts("READ_CAP64 ERROR\n");
+ } else {
+ debug("Read Capacity 64 returns: 0x%08x, 0x%08x,
0x%08x\n",
+ cap[0], cap[1], cap[2]);
+ capacity = be64_to_cpu(*(uint64_t *)cap) + 1;
+ blksz = be32_to_cpu(cap[2]);
+ }
+ }
+#else
+ /*
+* READ CAPACITY will return 0x when limited,
+* which wraps to 0 with the +1 above
+*/
+ if (!capacity) {
+ puts("LBA exceeds 32 bits but 64-bit LBA is disabled.\n");
+ capacity = ~0;
+ }
#endif
Re: [PATCH 3/4] usb: storage: Use the correct CBW lengths
On 10/29/23 08:23, Hector Martin wrote: USB UFI uses fixed 12-byte commands (as does RBC, which is not supported), but SCSI does not have this limitation. Use the correct command block lengths depending on the subclass. Signed-off-by: Hector Martin Reviewed-by: Marek Vasut
Re: [PATCH 2/4] usb: storage: Increase read/write timeout
On 10/29/23 08:23, Hector Martin wrote: Some USB devices (like hard disks) can take a long time to initially respond to read/write requests. Explicitly specify a much longer timeout than normal. Signed-off-by: Hector Martin Can we make this configurable with e.g. env variable, similar to usb_pgood_delay , to avoid affecting existing users ?
Re: [PATCH 1/4] scsi: Fix a bunch of SCSI definitions.
On 10/29/23 08:23, Hector Martin wrote:
0x9e isn't Read Capacity, it's a service action and the read capacity
command is a subcommand.
READ16 is not 0x48, it's 0x88. 0x48 is SANITIZE and that sounds like we
might have been destroying data instead of reading data. No bueno.
Signed-off-by: Hector Martin
---
drivers/ata/ahci.c | 9 ++---
drivers/scsi/scsi.c | 4 ++--
include/scsi.h | 8 ++--
3 files changed, 14 insertions(+), 7 deletions(-)
diff --git a/drivers/ata/ahci.c b/drivers/ata/ahci.c
index 2062197afcd3..b252e9e525db 100644
--- a/drivers/ata/ahci.c
+++ b/drivers/ata/ahci.c
@@ -906,15 +906,18 @@ static int ahci_scsi_exec(struct udevice *dev, struct
scsi_cmd *pccb)
case SCSI_RD_CAPAC10:
ret = ata_scsiop_read_capacity10(uc_priv, pccb);
break;
- case SCSI_RD_CAPAC16:
- ret = ata_scsiop_read_capacity16(uc_priv, pccb);
- break;
case SCSI_TST_U_RDY:
ret = ata_scsiop_test_unit_ready(uc_priv, pccb);
break;
case SCSI_INQUIRY:
ret = ata_scsiop_inquiry(uc_priv, pccb);
break;
+ case SCSI_SRV_ACTION_IN:
+ if ((pccb->cmd[1] & 0x1f) == SCSI_SAI_RD_CAPAC16) {
+ ret = ata_scsiop_read_capacity16(uc_priv, pccb);
+ break;
+ }
Would it make sense to add an else branch here and report unknown
subcommand there ?
With that:
Reviewed-by: Marek Vasut
Re: [PATCH] usb: Ignore endpoints in non-zero altsettings
On 10/29/23 08:24, Hector Martin wrote:
We currently do not really handle altsettings properly, and no driver
uses them. Ignore the respective endpoint descriptors for secondary
altsettings, to avoid creating duplicate endpoint records in the
interface.
This will have to be revisited if/when we have a driver that needs
altsettings to work properly.
Signed-off-by: Hector Martin
---
common/usb.c | 9 +
1 file changed, 9 insertions(+)
diff --git a/common/usb.c b/common/usb.c
index aad13fd9c557..90f72fda00bc 100644
--- a/common/usb.c
+++ b/common/usb.c
@@ -463,6 +463,15 @@ static int usb_parse_config(struct usb_device *dev,
puts("Endpoint descriptor out of order!\n");
break;
}
+ if (if_desc->num_altsetting > 1) {
+ /*
+* Ignore altsettings, which can trigger
duplicate
+* endpoint errors below. Revisit this when some
+* driver actually needs altsettings with
differing
+* endpoint setups.
+*/
How do you trigger this error ?
Re: [PATCH 1/2] usb: Pass through timeout to drivers
On 10/29/23 08:36, Hector Martin wrote: The old USB code was interrupt-driven and just polled at the top level. This has been obsolete since interrupts were removed, which means the timeout support has been completely broken. Rip out the top-level polling and just pass through the timeout parameter to host controller drivers. Right now this is ignored in the individual drivers. Signed-off-by: Hector Martin Except for the negative timeout value question, which needs to be sorted out: Reviewed-by: Marek Vasut
Re: [PATCH 2/2] usb: xhci: Hook up timeouts
On 10/29/23 08:36, Hector Martin wrote: Now that the USB core passes through timeout info to the host controller, actually hook it up. Signed-off-by: Hector Martin Except for the negative timeout value question, which needs to be sorted out: Reviewed-by: Marek Vasut
Re: [PATCH 1/2] usb: Pass through timeout to drivers
On 10/29/23 08:36, Hector Martin wrote: The old USB code was interrupt-driven and just polled at the top level. This has been obsolete since interrupts were removed, which means the timeout support has been completely broken. Rip out the top-level polling and just pass through the timeout parameter to host controller drivers. Right now this is ignored in the individual drivers. Signed-off-by: Hector Martin --- common/usb.c| 21 ++--- drivers/usb/host/ehci-hcd.c | 5 +++-- drivers/usb/host/ohci-hcd.c | 5 +++-- drivers/usb/host/r8a66597-hcd.c | 5 +++-- drivers/usb/host/usb-sandbox.c | 6 -- drivers/usb/host/usb-uclass.c | 9 + drivers/usb/host/xhci.c | 5 +++-- include/usb.h | 10 ++ 8 files changed, 29 insertions(+), 37 deletions(-) [...] int (*bulk)(struct udevice *bus, struct usb_device *udev, - unsigned long pipe, void *buffer, int length); + unsigned long pipe, void *buffer, int length, + int timeout); /** * interrupt() - Send an interrupt message * Can timeout ever be negative value ?
Re: [PATCH 0/2] USB fixes: (Re)implement timeouts
On 10/29/23 08:36, Hector Martin wrote: A long time ago, the USB code was interrupt-driven and used top-level timeout handling. This has long been obsolete, and that code is just broken dead cruft. HC drivers instead hardcode timeouts today. We need to be able to specify timeouts explicitly to handle cases like USB hard disks spinning up, without having ridiculously long timeouts across the board (which would cause endless waiting when things go wrong anywhere else). So, it's time to rip out the old broken nonsense and actually pass through timeouts to USB host controller drivers, so they can be implemented properly. I like this. [...]
Re: [PATCH 0/2] USB fixes: (Re)implement timeouts
On Sun, Oct 29, 2023 at 3:36 AM Hector Martin wrote: > > A long time ago, the USB code was interrupt-driven and used top-level > timeout handling. This has long been obsolete, and that code is just > broken dead cruft. HC drivers instead hardcode timeouts today. > > We need to be able to specify timeouts explicitly to handle cases like > USB hard disks spinning up, without having ridiculously long timeouts > across the board (which would cause endless waiting when things go > wrong anywhere else). So, it's time to rip out the old broken nonsense > and actually pass through timeouts to USB host controller drivers, so > they can be implemented properly. > > This series adds the necessary top-level scaffolding for control/bulk > timeouts, and implements them in xHCI. I didn't bother with interrupt > transfers, since I figure those probably never need long timeouts > anyway. > > The platform I deal with only has xHCI, so I'll leave implementing this > for EHCI/OHCI to someone else if anyone cares :) > > This series needs to be applied after [1], since the xHCI changes depend > on changes made there. > > [1] > https://lore.kernel.org/u-boot/20231029-usb-fixes-1-v2-0-623533f63...@marcan.st/ > > Signed-off-by: Hector Martin > --- > Hector Martin (2): > usb: Pass through timeout to drivers > usb: xhci: Hook up timeouts > > common/usb.c| 21 ++--- > drivers/usb/host/ehci-hcd.c | 5 +++-- > drivers/usb/host/ohci-hcd.c | 5 +++-- > drivers/usb/host/r8a66597-hcd.c | 5 +++-- > drivers/usb/host/usb-sandbox.c | 6 -- > drivers/usb/host/usb-uclass.c | 9 + > drivers/usb/host/xhci-ring.c| 32 > drivers/usb/host/xhci.c | 28 > include/usb.h | 10 ++ > include/usb/xhci.h | 14 ++ > 10 files changed, 72 insertions(+), 63 deletions(-) > --- > base-commit: 3d5d748e4d66b98109669c05d0c473fe67795801 > change-id: 20231029-usb-fixes-5-ca87bbedb40c > Series LGTM. Reviewed-by: Neal Gompa -- 真実はいつも一つ！/ Always, there's only one truth!
Re: [PATCH] usb: Ignore endpoints in non-zero altsettings
On Sun, Oct 29, 2023 at 3:25 AM Hector Martin wrote:
>
> We currently do not really handle altsettings properly, and no driver
> uses them. Ignore the respective endpoint descriptors for secondary
> altsettings, to avoid creating duplicate endpoint records in the
> interface.
>
> This will have to be revisited if/when we have a driver that needs
> altsettings to work properly.
>
> Signed-off-by: Hector Martin
> ---
> common/usb.c | 9 +
> 1 file changed, 9 insertions(+)
>
> diff --git a/common/usb.c b/common/usb.c
> index aad13fd9c557..90f72fda00bc 100644
> --- a/common/usb.c
> +++ b/common/usb.c
> @@ -463,6 +463,15 @@ static int usb_parse_config(struct usb_device *dev,
> puts("Endpoint descriptor out of order!\n");
> break;
> }
> + if (if_desc->num_altsetting > 1) {
> + /*
> +* Ignore altsettings, which can trigger
> duplicate
> +* endpoint errors below. Revisit this when
> some
> +* driver actually needs altsettings with
> differing
> +* endpoint setups.
> +*/
> + break;
> + }
> epno = dev->config.if_desc[ifno].no_of_ep;
> if_desc = >config.if_desc[ifno];
> if (epno >= USB_MAXENDPOINTS) {
>
> ---
> base-commit: 8ad1c9c26f7740806a162818b790d4a72f515b7e
> change-id: 20231029-usb-fixes-4-ba6931acf217
>
Irritating, but makes sense.
Reviewed-by: Neal Gompa
--
真実はいつも一つ！/ Always, there's only one truth!
Re: [PATCH 0/4] USB fixes: Mass Storage bugs & 64bit support
On Sun, Oct 29, 2023 at 3:23 AM Hector Martin wrote: > > This series fixes some bugs in the USBMS driver and adds 64-bit LBA > support. This is required to make USB HDDs >=4TB work. > > Note that the increased timeout won't actually work right now, due to > broken handling in the underlying USB infrastructure. That will be fixed > in a follow-up series, which depends on [1] being applied first. The > USBMS part is logically stand-alone and can be applied in parallel > before that. > > [1] > https://lore.kernel.org/u-boot/20231029-usb-fixes-1-v2-0-623533f63...@marcan.st/ > > Signed-off-by: Hector Martin > --- > Hector Martin (4): > scsi: Fix a bunch of SCSI definitions. > usb: storage: Increase read/write timeout > usb: storage: Use the correct CBW lengths > usb: storage: Implement 64-bit LBA support > > common/usb_storage.c | 164 > ++- > drivers/ata/ahci.c | 9 ++- > drivers/scsi/scsi.c | 4 +- > include/scsi.h | 8 ++- > 4 files changed, 150 insertions(+), 35 deletions(-) > --- > base-commit: 8ad1c9c26f7740806a162818b790d4a72f515b7e > change-id: 20231029-usb-fixes-3-c72f829ba61b Series LGTM. Reviewed-by: Neal Gompa -- 真実はいつも一つ！/ Always, there's only one truth!
Re: [PATCH 0/2] USB fixes: Add missing timeout, ignore YubiKeys
On Sun, Oct 29, 2023 at 3:09 AM Hector Martin wrote: > > This mini series fixes one bug, but in the process makes YubiKeys work, > which then regresses people who have one *and* a USB keyboard, since we > only support a single keyboard device. > > Therefore patch #1 makes U-Boot ignore YubiKeys, so #2 does not > regress things. > > Signed-off-by: Hector Martin > --- > Hector Martin (2): > usb: kbd: Ignore Yubikeys > usb: hub: Add missing reset recovery delay > > common/usb_hub.c | 7 +++ > common/usb_kbd.c | 19 +++ > 2 files changed, 26 insertions(+) > --- > base-commit: 8ad1c9c26f7740806a162818b790d4a72f515b7e > change-id: 20231029-usb-fixes-2-976486d1603c > Series LGTM. Reviewed-by: Neal Gompa -- 真実はいつも一つ！/ Always, there's only one truth!
Re: [PATCH 2/3] tpm: Convert sandbox-focussed tests to C
On Sun, Oct 29, 2023 at 05:28:13PM +1300, Simon Glass wrote:
> Some of the Python tests are a pain because they don't reset the TPM
> state before each test. Driver model tests do this, so convert the
> tests to C.
>
> This means that these tests won't run on real hardware, but we have
> tests which do TPM init, so there is still enough coverage.
>
> Rename and update the Python tpm_init test to use 'tpm autostart',
> since this deals with starting up ready for the tests below.
>
> Signed-off-by: Simon Glass
I worry that we're removing maybe a few too many of the tests we can be
used on real HW (and QEMU, another real use case) in favor of just
testing them on sandbox. We can certainly have parallel sandbox-only
test paths.
[snip]
> @@ -92,46 +79,6 @@ def tpm2_sandbox_init(u_boot_console):
> if skip_test:
> pytest.skip('skip TPM device test')
>
> -@pytest.mark.buildconfigspec('cmd_tpm_v2')
> -def test_tpm2_sandbox_self_test_full(u_boot_console):
> -"""Execute a TPM2_SelfTest (full) command.
> -
> -Ask the TPM to perform all self tests to also enable full capabilities.
> -"""
> -if is_sandbox(u_boot_console):
> -u_boot_console.restart_uboot()
> -u_boot_console.run_command('tpm2 init')
> -output = u_boot_console.run_command('echo $?')
> -assert output.endswith('0')
> -
> -u_boot_console.run_command('tpm2 startup TPM2_SU_CLEAR')
> -output = u_boot_console.run_command('echo $?')
> -assert output.endswith('0')
> -
> -skip_test = u_boot_console.config.env.get('env__tpm_device_test_skip',
> False)
> -if skip_test:
> -pytest.skip('skip TPM device test')
> -u_boot_console.run_command('tpm2 self_test full')
> -output = u_boot_console.run_command('echo $?')
> -assert output.endswith('0')
> -
> -@pytest.mark.buildconfigspec('cmd_tpm_v2')
> -def test_tpm2_continue_self_test(u_boot_console):
> -"""Execute a TPM2_SelfTest (continued) command.
> -
> -Ask the TPM to finish its self tests (alternative to the full test) in
> order
> -to enter a fully operational state.
> -"""
> -
> -skip_test = u_boot_console.config.env.get('env__tpm_device_test_skip',
> False)
> -if skip_test:
> -pytest.skip('skip TPM device test')
> -if is_sandbox(u_boot_console):
> -tpm2_sandbox_init(u_boot_console)
> -u_boot_console.run_command('tpm2 self_test continue')
> -output = u_boot_console.run_command('echo $?')
> -assert output.endswith('0')
> -
I would think these are useful cases to check outside of sandbox. But
I'll let Ilias chime in as I'm just assuming.
--
Tom
signature.asc
Description: PGP signature
Re: [PATCH 01/15] spl: nand: Fix NULL-pointer dereference
On 10/28/23 23:48, Sean Anderson wrote:
spl_nand_fit_read unconditionally accesses load->priv. Ensure it is set.
Fixes: 00e180cc513 ("spl: nand: support loading i.MX container format file")
Fixes: 4620e8aabc1 ("spl: nand: support loading legacy image with payload
compressed")
Signed-off-by: Sean Anderson
---
common/spl/spl_nand.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/common/spl/spl_nand.c b/common/spl/spl_nand.c
index 07916bedbb9..a19236d9e6d 100644
--- a/common/spl/spl_nand.c
+++ b/common/spl/spl_nand.c
@@ -105,7 +105,7 @@ static int spl_nand_load_element(struct spl_image_info
*spl_image,
struct spl_load_info load;
load.dev = NULL;
- load.priv = NULL;
+ load.priv =
load.filename = NULL;
load.bl_len = bl_len;
load.read = spl_nand_fit_read;
@@ -116,7 +116,7 @@ static int spl_nand_load_element(struct spl_image_info
*spl_image,
debug("Found legacy image\n");
load.dev = NULL;
- load.priv = NULL;
+ load.priv =
load.filename = NULL;
load.bl_len = 1;
load.read = spl_nand_legacy_read;
Actually, since spl_nand_legacy_read doesn't reference priv, this second hunk is
unnecessary. Actually, spl_nand_legacy_read and spl_load_legacy_img are
technically buggy
since size/offset are supposed to be in units of bl_len. However, this
basically just
results in extra multiplies and divides, so I don't think it's desirable. I
actually have
a patch to convert everything to bytes (keeping alignment), so "fixing" this is
not
necessary for the moment.
--Sean
Re: [PATCH] mmc: Add SPL_MMC_PWRSEQ to fix link issue when building SPL
Hi,
On Mon Sep 25, 2023 at 11:55 PM CEST, Jonas Karlman wrote:
> With MMC_PWRSEQ enabled the following link issue may happen when
> building SPL and SPL_PWRSEQ is not enabled.
>
> aarch64-linux-gnu-ld.bfd: drivers/mmc/meson_gx_mmc.o: in function
> `meson_mmc_probe':
> drivers/mmc/meson_gx_mmc.c:295: undefined reference to `pwrseq_set_power'
>
> Fix this by adding a SPL_MMC_PWRSEQ Kconfig option used to enable mmc
> pwrseq support in SPL.
>
> Also add depends on DM_GPIO to fix following link issue:
>
> aarch64-linux-gnu-ld.bfd: drivers/mmc/mmc-pwrseq.o: in function
> `mmc_pwrseq_set_power':
> drivers/mmc/mmc-pwrseq.c:26: undefined reference to `gpio_request_by_name'
> aarch64-linux-gnu-ld.bfd: drivers/mmc/mmc-pwrseq.c:29: undefined reference
> to `dm_gpio_set_value'
> aarch64-linux-gnu-ld.bfd: drivers/mmc/mmc-pwrseq.c:31: undefined reference
> to `dm_gpio_set_value'
>
> Signed-off-by: Jonas Karlman
> ---
> drivers/mmc/Kconfig | 10 +-
> drivers/mmc/Makefile | 2 +-
> drivers/mmc/meson_gx_mmc.c| 2 +-
> drivers/mmc/rockchip_dw_mmc.c | 2 +-
> include/mmc.h | 4 ++--
> 5 files changed, 14 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/mmc/Kconfig b/drivers/mmc/Kconfig
> index de01b9687bad..a9931d39412d 100644
> --- a/drivers/mmc/Kconfig
> +++ b/drivers/mmc/Kconfig
> @@ -20,11 +20,19 @@ config MMC_WRITE
>
> config MMC_PWRSEQ
> bool "HW reset support for eMMC"
> - depends on PWRSEQ
> + depends on PWRSEQ && DM_GPIO
> help
> Ths select Hardware reset support aka pwrseq-emmc for eMMC
> devices.
>
> +config SPL_MMC_PWRSEQ
> + bool "HW reset support for eMMC in SPL"
> + depends on SPL_PWRSEQ && SPL_DM_GPIO
> + default y if MMC_PWRSEQ
> + help
> + Ths select Hardware reset support aka pwrseq-emmc for eMMC
nit: 'Ths'->'This'
Seems to be the same earlier, in MMC_PWRSEQ
> + devices in SPL.
> +
> config MMC_BROKEN_CD
> bool "Poll for broken card detection case"
> help
> diff --git a/drivers/mmc/Makefile b/drivers/mmc/Makefile
> index 2c65c4765ab2..0a79dd058bef 100644
> --- a/drivers/mmc/Makefile
> +++ b/drivers/mmc/Makefile
> @@ -11,7 +11,7 @@ obj-$(CONFIG_$(SPL_TPL_)BOOTSTD) += mmc_bootdev.o
> endif
>
> obj-$(CONFIG_$(SPL_TPL_)MMC_WRITE) += mmc_write.o
> -obj-$(CONFIG_MMC_PWRSEQ) += mmc-pwrseq.o
> +obj-$(CONFIG_$(SPL_)MMC_PWRSEQ) += mmc-pwrseq.o
> obj-$(CONFIG_MMC_SDHCI_ADMA_HELPERS) += sdhci-adma.o
>
> ifndef CONFIG_$(SPL_)BLK
> diff --git a/drivers/mmc/meson_gx_mmc.c b/drivers/mmc/meson_gx_mmc.c
> index fcf4f03d1e24..0825c0a2a838 100644
> --- a/drivers/mmc/meson_gx_mmc.c
> +++ b/drivers/mmc/meson_gx_mmc.c
> @@ -288,7 +288,7 @@ static int meson_mmc_probe(struct udevice *dev)
>
> mmc_set_clock(mmc, cfg->f_min, MMC_CLK_ENABLE);
>
> -#ifdef CONFIG_MMC_PWRSEQ
> +#if CONFIG_IS_ENABLED(MMC_PWRSEQ)
> /* Enable power if needed */
> ret = mmc_pwrseq_get_power(dev, cfg);
> if (!ret) {
> diff --git a/drivers/mmc/rockchip_dw_mmc.c b/drivers/mmc/rockchip_dw_mmc.c
> index 72c820ee6330..ad4529d6afa8 100644
> --- a/drivers/mmc/rockchip_dw_mmc.c
> +++ b/drivers/mmc/rockchip_dw_mmc.c
> @@ -145,7 +145,7 @@ static int rockchip_dwmmc_probe(struct udevice *dev)
>
> host->fifo_mode = priv->fifo_mode;
>
> -#ifdef CONFIG_MMC_PWRSEQ
> +#if CONFIG_IS_ENABLED(MMC_PWRSEQ)
> /* Enable power if needed */
> ret = mmc_pwrseq_get_power(dev, >cfg);
> if (!ret) {
> diff --git a/include/mmc.h b/include/mmc.h
> index 1022db3ffa7c..9aef31ea5deb 100644
> --- a/include/mmc.h
> +++ b/include/mmc.h
> @@ -590,7 +590,7 @@ struct mmc_config {
> uint f_max;
> uint b_max;
> unsigned char part_type;
> -#ifdef CONFIG_MMC_PWRSEQ
> +#if CONFIG_IS_ENABLED(MMC_PWRSEQ)
> struct udevice *pwr_dev;
> #endif
> };
> @@ -808,7 +808,7 @@ int mmc_deinit(struct mmc *mmc);
> */
> int mmc_of_parse(struct udevice *dev, struct mmc_config *cfg);
>
> -#ifdef CONFIG_MMC_PWRSEQ
> +#if CONFIG_IS_ENABLED(MMC_PWRSEQ)
> /**
> * mmc_pwrseq_get_power() - get a power device from device tree
> *
> --
> 2.42.0
Otherwise, looks good to me.
Acked-by: Ferass El Hafidi
Re: [PATCH 04/15] nand: spl_loaders: Only read enough pages to load the image
Hi
On Sun, Oct 29, 2023 at 4:48 AM Sean Anderson wrote:
>
> All other implementations of nand_spl_load_image only read as many pages as
> are necessary to load the image. However, nand_spl_loaders.c loads the full
> block. Align it with other load functions so that it is easier to
> determine how large of a load buffer we need.
>
> Signed-off-by: Sean Anderson
> ---
>
> drivers/mtd/nand/raw/nand_spl_loaders.c | 5 -
> 1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/mtd/nand/raw/nand_spl_loaders.c
> b/drivers/mtd/nand/raw/nand_spl_loaders.c
> index 8848cb27db9..f071b5b57f5 100644
> --- a/drivers/mtd/nand/raw/nand_spl_loaders.c
> +++ b/drivers/mtd/nand/raw/nand_spl_loaders.c
> @@ -12,8 +12,11 @@ int nand_spl_load_image(uint32_t offs, unsigned int size,
> void *dst)
> while (block <= lastblock) {
> if (!nand_is_bad_block(block)) {
> /* Skip bad blocks */
> - while (page < SYS_NAND_PAGE_COUNT) {
> + while (size && page < SYS_NAND_PAGE_COUNT) {
> nand_read_page(block, page, dst);
> +
> + size -= min(size, CONFIG_SYS_NAND_PAGE_SIZE -
> + page_offset);
> /*
> * When offs is not aligned to page address
> the
> * extra offset is copied to dst as well. Copy
> --
> 2.37.1
>
Reviewed-by: Michael Trimarchi
--
Michael Nazzareno Trimarchi
Co-Founder & Chief Executive Officer
M. +39 347 913 2170
mich...@amarulasolutions.com
__
Amarula Solutions BV
Joop Geesinkweg 125, 1114 AB, Amsterdam, NL
T. +31 (0)85 111 9172
i...@amarulasolutions.com
www.amarulasolutions.com
Re: [PATCH 02/15] nand: Don't dereference NULL manufacturer_desc
Hi
On Sun, Oct 29, 2023 at 4:48 AM Sean Anderson wrote:
>
> When no manufacturer is matched, manufacturer_desc is NULL. Avoid
> dereferencing it in that case.
>
> Fixes: 4e67c571252 ("mtd,ubi,ubifs: sync with linux v3.15")
> Signed-off-by: Sean Anderson
> ---
>
> drivers/mtd/nand/raw/nand_base.c | 7 ++-
> 1 file changed, 2 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/mtd/nand/raw/nand_base.c
> b/drivers/mtd/nand/raw/nand_base.c
> index 6b4adcf6bdc..44b6cb63a01 100644
> --- a/drivers/mtd/nand/raw/nand_base.c
> +++ b/drivers/mtd/nand/raw/nand_base.c
> @@ -4462,17 +4462,14 @@ ident_done:
> else if (chip->jedec_version)
> pr_info("%s %s\n", manufacturer_desc->name,
> chip->jedec_params.model);
> - else
> + else if (manufacturer_desc)
> pr_info("%s %s\n", manufacturer_desc->name, type->name);
> #else
> if (chip->jedec_version)
> pr_info("%s %s\n", manufacturer_desc->name,
> chip->jedec_params.model);
> - else
> + else if (manufacturer_desc)
> pr_info("%s %s\n", manufacturer_desc->name, type->name);
> -
> - pr_info("%s %s\n", manufacturer_desc->name,
> - type->name);
> #endif
>
> pr_info("%d MiB, %s, erase size: %d KiB, page size: %d, OOB size:
> %d\n",
> --
> 2.37.1
>
Reviewed-by: Michael Trimarchi
--
Michael Nazzareno Trimarchi
Co-Founder & Chief Executive Officer
M. +39 347 913 2170
mich...@amarulasolutions.com
__
Amarula Solutions BV
Joop Geesinkweg 125, 1114 AB, Amsterdam, NL
T. +31 (0)85 111 9172
i...@amarulasolutions.com
www.amarulasolutions.com
[RFC v2 2/2] rng: Provide a RNG based on the RISC-V Zkr ISA extension
The Zkr ISA extension (ratified Nov 2021) introduced the seed CSR. It
provides an interface to a physical entropy source.
A RNG driver based on the seed CSR is provided. It depends on
mseccfg.sseed being set in the SBI firmware.
Signed-off-by: Heinrich Schuchardt
---
v2:
Resume after exception if seed CSR cannot be read.
---
drivers/rng/Kconfig | 8 +++
drivers/rng/Makefile| 1 +
drivers/rng/riscv_zkr_rng.c | 116
3 files changed, 125 insertions(+)
create mode 100644 drivers/rng/riscv_zkr_rng.c
diff --git a/drivers/rng/Kconfig b/drivers/rng/Kconfig
index 994cc35b27..4f6e367169 100644
--- a/drivers/rng/Kconfig
+++ b/drivers/rng/Kconfig
@@ -48,6 +48,14 @@ config RNG_OPTEE
accessible to normal world but reserved and used by the OP-TEE
to avoid the weakness of a software PRNG.
+config RNG_RISCV_ZKR
+ bool "RISC-V Zkr random number generator"
+ depends on RISCV_SMODE
+ help
+ This driver provides a Random Number Generator based on the
+ Zkr RISC-V ISA extension which provides an interface to an
+ NIST SP 800-90B or BSI AIS-31 compliant physical entropy source.
+
config RNG_STM32
bool "Enable random number generator for STM32"
depends on ARCH_STM32 || ARCH_STM32MP
diff --git a/drivers/rng/Makefile b/drivers/rng/Makefile
index 47b323e61e..a5d3ca4130 100644
--- a/drivers/rng/Makefile
+++ b/drivers/rng/Makefile
@@ -10,6 +10,7 @@ obj-$(CONFIG_RNG_MSM) += msm_rng.o
obj-$(CONFIG_RNG_NPCM) += npcm_rng.o
obj-$(CONFIG_RNG_OPTEE) += optee_rng.o
obj-$(CONFIG_RNG_STM32) += stm32_rng.o
+obj-$(CONFIG_RNG_RISCV_ZKR) += riscv_zkr_rng.o
obj-$(CONFIG_RNG_ROCKCHIP) += rockchip_rng.o
obj-$(CONFIG_RNG_IPROC200) += iproc_rng200.o
obj-$(CONFIG_RNG_SMCCC_TRNG) += smccc_trng.o
diff --git a/drivers/rng/riscv_zkr_rng.c b/drivers/rng/riscv_zkr_rng.c
new file mode 100644
index 00..8c9e111e2e
--- /dev/null
+++ b/drivers/rng/riscv_zkr_rng.c
@@ -0,0 +1,116 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * The RISC-V Zkr extension provides CSR seed which provides access to a
+ * random number generator.
+ */
+
+#define LOG_CATEGORY UCLASS_RNG
+
+#include
+#include
+#include
+#include
+
+#define DRIVER_NAME "riscv_zkr"
+
+enum opst {
+ /** @BIST: built in self test running */
+ BIST = 0b00,
+ /** @WAIT: sufficient amount of entropy is not yet available */
+ WAIT = 0b01,
+ /** @ES16: 16bits of entropy available */
+ ES16 = 0b10,
+ /** @DEAD: unrecoverable self-test error */
+ DEAD = 0b11,
+};
+
+static unsigned long read_seed(void)
+{
+ unsigned long ret;
+
+ __asm__ __volatile__("csrrw %0, seed, x0" : "=r" (ret) : : "memory");
+
+ return ret;
+}
+
+static int riscv_zkr_read(struct udevice *dev, void *data, size_t len)
+{
+ u8 *ptr = data;
+
+ while (len) {
+ u32 val;
+
+ val = read_seed();
+
+ switch (val >> 30) {
+ case BIST:
+ continue;
+ case WAIT:
+ continue;
+ case ES16:
+ *ptr++ = val & 0xff;
+ if (--len) {
+ *ptr++ = val >> 8;
+ --len;
+ }
+ break;
+ case DEAD:
+ return -ENODEV;
+ }
+ }
+
+ return 0;
+}
+
+/**
+ * riscv_zkr_probe() - check if the seed register is available
+ *
+ * If the SBI software has not set mseccfg.sseed=1 or the Zkr
+ * extension is not available this probe function will result
+ * in an exception. Currently we cannot recover from this.
+ *
+ * @dev: RNG device
+ * Return: 0 if successfully probed
+ */
+static int riscv_zkr_probe(struct udevice *dev)
+{
+ struct resume_data resume;
+ int ret;
+ u32 val;
+
+ /* Check if reading seed leads to interrupt */
+ set_resume();
+ ret = setjmp(resume.jump);
+ if (ret)
+ log_debug("Exception %ld reading seed CSR\n", resume.code);
+ else
+ val = read_seed();
+ set_resume(NULL);
+ if (ret)
+ return -ENODEV;
+
+ do {
+ val = read_seed();
+ val >>= 30;
+ } while (val == BIST || val == WAIT);
+
+ if (val == DEAD)
+ return -ENODEV;
+
+ return 0;
+}
+
+static const struct dm_rng_ops riscv_zkr_ops = {
+ .read = riscv_zkr_read,
+};
+
+U_BOOT_DRIVER(riscv_zkr) = {
+ .name = DRIVER_NAME,
+ .id = UCLASS_RNG,
+ .ops = _zkr_ops,
+ .probe = riscv_zkr_probe,
+};
+
+U_BOOT_DRVINFO(cpu_riscv_zkr) = {
+ .name = DRIVER_NAME,
+};
--
2.40.1
[RFC v2 1/2] riscv: allow resume after exception
If CSRs like seed are readable by S-mode, may not be determinable by
S-mode. For safe driver probing allow to resume via a longjmp after an
exception.
Signed-off-by: Heinrich Schuchardt
---
v2:
new patch
---
arch/riscv/lib/interrupts.c | 13 +
include/interrupt.h | 22 ++
2 files changed, 35 insertions(+)
create mode 100644 include/interrupt.h
diff --git a/arch/riscv/lib/interrupts.c b/arch/riscv/lib/interrupts.c
index 02dbcfd423..a26ccc721f 100644
--- a/arch/riscv/lib/interrupts.c
+++ b/arch/riscv/lib/interrupts.c
@@ -12,6 +12,7 @@
#include
#include
#include
+#include
#include
#include
#include
@@ -21,6 +22,13 @@
DECLARE_GLOBAL_DATA_PTR;
+static struct resume_data *resume;
+
+void set_resume(struct resume_data *data)
+{
+ resume = data;
+}
+
static void show_efi_loaded_images(uintptr_t epc)
{
efi_print_image_infos((void *)epc);
@@ -105,6 +113,11 @@ static void _exit_trap(ulong code, ulong epc, ulong tval,
struct pt_regs *regs)
"Store/AMO page fault",
};
+ if (resume) {
+ resume->code = code;
+ longjmp(resume->jump, 1);
+ }
+
if (code < ARRAY_SIZE(exception_code))
printf("Unhandled exception: %s\n", exception_code[code]);
else
diff --git a/include/interrupt.h b/include/interrupt.h
new file mode 100644
index 00..1baa60bcf2
--- /dev/null
+++ b/include/interrupt.h
@@ -0,0 +1,22 @@
+/* SPDX-License-Identifier: GPL-2.0-or-later */
+
+#include
+
+/**
+ * struct resume_data - data for resume after interrupt
+ */
+struct resume_data {
+ /** @jump: longjmp buffer */
+ jmp_buf jump;
+ /** @code: exception code */
+ ulong code;
+};
+
+/**
+ * set_resume() - set longjmp buffer for resuming after interrupt
+ *
+ * When resuming the exception code will be returned in @data->code.
+ *
+ * @data: pointer to structure with longjmp address
+ */
+void set_resume(struct resume_data *data);
--
2.40.1
[RFC v2 0/2] rng: Provide a RNG based on the RISC-V Zkr ISA extension
The Zkr ISA extension (ratified Nov 2021) introduced the seed CSR. It provides an interface to a physical entropy source. A RNG driver based on the seed CSR is provided. It depends on mseccfg.sseed being set in the SBI firmware. If the seed CSR readable, is not determinable by S-mode without risking an exception. For safe driver probing allow to resume via a longjmp after an exception. As the driver depends on mseccfg.sseed=1 we should wait with merging the driver until a decision has been taken in the RISC-V PRS TG on prescribing this. Heinrich Schuchardt (2): riscv: allow resume after exception rng: Provide a RNG based on the RISC-V Zkr ISA extension arch/riscv/lib/interrupts.c | 13 drivers/rng/Kconfig | 8 +++ drivers/rng/Makefile| 1 + drivers/rng/riscv_zkr_rng.c | 116 include/interrupt.h | 22 +++ 5 files changed, 160 insertions(+) create mode 100644 drivers/rng/riscv_zkr_rng.c create mode 100644 include/interrupt.h -- 2.40.1
Re: [PATCH] Kconfig: Remove all default n/no options
Hi Michal,
thanks, built all m68k stuff, no issues.
Also tested on stmark2, all is ok.
Reviewed-by: Angelo Dureghello
On 25/10/23 9:25 AM, Michal Simek wrote:
Similar change was done by commit b4c2c151b14b ("Kconfig: Remove all
default n/no options") and again sync is required.
default n/no doesn't need to be specified. It is default option anyway.
Signed-off-by: Michal Simek
---
arch/arm/mach-imx/mxs/Kconfig | 2 --
arch/arm/mach-rockchip/Kconfig | 1 -
arch/arm/mach-snapdragon/Kconfig | 1 -
arch/arm/mach-sunxi/Kconfig| 1 -
arch/arm/mach-tegra/Kconfig| 1 -
arch/m68k/Kconfig | 1 -
arch/riscv/Kconfig | 1 -
board/asus/grouper/Kconfig | 2 --
board/asus/transformer-t30/Kconfig | 1 -
board/keymile/Kconfig | 3 ---
board/lg/x3-t30/Kconfig| 2 --
cmd/Kconfig| 3 ---
drivers/crypto/fsl/Kconfig | 1 -
drivers/memory/Kconfig | 1 -
drivers/misc/Kconfig | 1 -
drivers/mtd/nand/raw/Kconfig | 1 -
drivers/mtd/spi/Kconfig| 1 -
drivers/sm/Kconfig | 1 -
drivers/spi/Kconfig| 1 -
drivers/usb/host/Kconfig | 2 --
fs/ubifs/Kconfig | 1 -
21 files changed, 29 deletions(-)
diff --git a/arch/arm/mach-imx/mxs/Kconfig b/arch/arm/mach-imx/mxs/Kconfig
index ccce6a78caa2..d2e4205c5ce5 100644
--- a/arch/arm/mach-imx/mxs/Kconfig
+++ b/arch/arm/mach-imx/mxs/Kconfig
@@ -54,7 +54,6 @@ config SYS_SOC
config SPL_MXS_PMU_MINIMAL_VDD5V_CURRENT
bool "Force minimal current draw from VDD5V by MX28 PMU"
- default n
help
After setting this option, the current drawn from VDD5V
by the PMU is reduced to zero - the DCDC_BATT is used as
@@ -62,7 +61,6 @@ config SPL_MXS_PMU_MINIMAL_VDD5V_CURRENT
config SPL_MXS_PMU_DISABLE_BATT_CHARGE
bool "Disable Battery Charging in MX28 PMU"
- default n
config SPL_MXS_PMU_ENABLE_4P2_LINEAR_REGULATOR
bool "Enable the 4P2 linear regulator in MX28 PMU"
diff --git a/arch/arm/mach-rockchip/Kconfig b/arch/arm/mach-rockchip/Kconfig
index c43c185c17c5..a6c69c300d00 100644
--- a/arch/arm/mach-rockchip/Kconfig
+++ b/arch/arm/mach-rockchip/Kconfig
@@ -450,7 +450,6 @@ config ROCKCHIP_BOOT_MODE_REG
config ROCKCHIP_RK8XX_DISABLE_BOOT_ON_POWERON
bool "Disable device boot on power plug-in"
depends on PMIC_RK8XX
- default n
---help---
Say Y here to prevent the device from booting up because of a plug-in
event. When set, the device will boot briefly to determine why it was
diff --git a/arch/arm/mach-snapdragon/Kconfig b/arch/arm/mach-snapdragon/Kconfig
index 0e073045be54..2fc1521e2d30 100644
--- a/arch/arm/mach-snapdragon/Kconfig
+++ b/arch/arm/mach-snapdragon/Kconfig
@@ -14,7 +14,6 @@ config SPL_SYS_MALLOC_F_LEN
config SDM845
bool "Qualcomm Snapdragon 845 SoC"
- default n
select LINUX_KERNEL_IMAGE_HEADER
config LNX_KRNL_IMG_TEXT_OFFSET_BASE
diff --git a/arch/arm/mach-sunxi/Kconfig b/arch/arm/mach-sunxi/Kconfig
index 40ca7d7b3a99..a10e4c06b6a3 100644
--- a/arch/arm/mach-sunxi/Kconfig
+++ b/arch/arm/mach-sunxi/Kconfig
@@ -811,7 +811,6 @@ config AXP_GPIO
config AXP_DISABLE_BOOT_ON_POWERON
bool "Disable device boot on power plug-in"
depends on AXP209_POWER || AXP221_POWER || AXP809_POWER || AXP818_POWER
- default n
---help---
Say Y here to prevent the device from booting up because of a plug-in
event. When set, the device will boot into the SPL briefly to
diff --git a/arch/arm/mach-tegra/Kconfig b/arch/arm/mach-tegra/Kconfig
index 464bd0798f62..0e94b84fe657 100644
--- a/arch/arm/mach-tegra/Kconfig
+++ b/arch/arm/mach-tegra/Kconfig
@@ -18,7 +18,6 @@ config TEGRA_CLKRST
config TEGRA_CRYPTO
bool "Tegra AES128 crypto module"
select AES
- default n
config TEGRA_GP_PADCTRL
bool
diff --git a/arch/m68k/Kconfig b/arch/m68k/Kconfig
index 587edd50d7e8..b288c65e7fd1 100644
--- a/arch/m68k/Kconfig
+++ b/arch/m68k/Kconfig
@@ -200,7 +200,6 @@ source "board/sysam/stmark2/Kconfig"
config M68K_QEMU
bool "Build with workarounds for incomplete QEMU emulation"
- default n
help
QEMU 8.x currently does not implement RAMBAR accesses and
DMA timers. Enable this option for U-Boot CI purposes only
diff --git a/arch/riscv/Kconfig b/arch/riscv/Kconfig
index e291456530bd..8fc81fb284cd 100644
--- a/arch/riscv/Kconfig
+++ b/arch/riscv/Kconfig
@@ -68,7 +68,6 @@ config SPL_SYS_DCACHE_OFF
config SPL_ZERO_MEM_BEFORE_USE
bool "Zero memory before use"
depends on SPL
- default n
help
Zero stack/GD/malloc area in SPL before using them, this is needed for
Sifive core devices that uses L2 cache to store SPL.
diff --git a/board/asus/grouper/Kconfig
Re: [RFC 1/1] rng: Provide a RNG based on the RISC-V Zkr ISA extension
On 10/29/23 06:39, Chanho Park wrote:
Hi,
-Original Message-
From: U-Boot On Behalf Of Heinrich
Schuchardt
Sent: Sunday, October 29, 2023 8:26 AM
To: Rick Chen ; Leo
Cc: Sughosh Ganu ; u-boot@lists.denx.de; Heinrich
Schuchardt
Subject: [RFC 1/1] rng: Provide a RNG based on the RISC-V Zkr ISA
extension
The Zkr ISA extension (ratified Nov 2021) introduced the seed CSR. It
provides an interface to a physical entropy source.
A RNG driver based on the seed CSR is provided. It depends on
mseccfg.sseed being set in the SBI firmware.
Signed-off-by: Heinrich Schuchardt
This works fine on my qemu risv with your opensbi patch and KASLR has been
tested as well.
Feel free to add my reviewed/tested-by tag.
Reviewed-by: Chanho Park
Tested-by: Chanho Park
Best Regards,
Chanho Park
Thanks for reviewing.
I am currently looking into detecting if the seed register is readable
to avoid a possible exception if the sseed flag is not set or the Zkr
exception is not available.
Best regards
Heinrich
---
drivers/rng/Kconfig | 11
drivers/rng/Makefile| 1 +
drivers/rng/riscv_zkr_rng.c | 102
3 files changed, 114 insertions(+)
create mode 100644 drivers/rng/riscv_zkr_rng.c
diff --git a/drivers/rng/Kconfig b/drivers/rng/Kconfig
index 994cc35b27..f8f1d91ed2 100644
--- a/drivers/rng/Kconfig
+++ b/drivers/rng/Kconfig
@@ -48,6 +48,17 @@ config RNG_OPTEE
accessible to normal world but reserved and used by the OP-TEE
to avoid the weakness of a software PRNG.
+config RNG_RISCV_ZKR
+ bool "RISC-V Zkr random number generator"
+ depends on RISCV_SMODE
+ help
+ This driver provides a Random Number Generator based on the
+ Zkr RISC-V ISA extension which provides an interface to an
+ NIST SP 800-90B or BSI AIS-31 compliant physical entropy source.
+
+ Using this driver will lead to an exception if the M-mode
firmware
+ has not set mseccfg.sseed=1.
+
config RNG_STM32
bool "Enable random number generator for STM32"
depends on ARCH_STM32 || ARCH_STM32MP
diff --git a/drivers/rng/Makefile b/drivers/rng/Makefile
index 47b323e61e..a5d3ca4130 100644
--- a/drivers/rng/Makefile
+++ b/drivers/rng/Makefile
@@ -10,6 +10,7 @@ obj-$(CONFIG_RNG_MSM) += msm_rng.o
obj-$(CONFIG_RNG_NPCM) += npcm_rng.o
obj-$(CONFIG_RNG_OPTEE) += optee_rng.o
obj-$(CONFIG_RNG_STM32) += stm32_rng.o
+obj-$(CONFIG_RNG_RISCV_ZKR) += riscv_zkr_rng.o
obj-$(CONFIG_RNG_ROCKCHIP) += rockchip_rng.o
obj-$(CONFIG_RNG_IPROC200) += iproc_rng200.o
obj-$(CONFIG_RNG_SMCCC_TRNG) += smccc_trng.o
diff --git a/drivers/rng/riscv_zkr_rng.c b/drivers/rng/riscv_zkr_rng.c
new file mode 100644
index 00..f48ae35410
--- /dev/null
+++ b/drivers/rng/riscv_zkr_rng.c
@@ -0,0 +1,102 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * The RISC-V Zkr extension provides CSR seed which provides access to a
+ * random number generator.
+ */
+
+#define LOG_CATEGORY UCLASS_RNG
+
+#include
+#include
+#include
+
+#define DRIVER_NAME "riscv_zkr"
+
+enum opst {
+ /** @BIST: built in self test running */
+ BIST = 0b00,
+ /** @WAIT: sufficient amount of entropy is not yet available */
+ WAIT = 0b01,
+ /** @ES16: 16bits of entropy available */
+ ES16 = 0b10,
+ /** @DEAD: unrecoverable self-test error */
+ DEAD = 0b11,
+};
+
+static unsigned long read_seed(void)
+{
+ unsigned long ret;
+
+ __asm__ __volatile__("csrrw %0, seed, x0" : "=r" (ret) : :
"memory");
+
+ return ret;
+}
+
+static int riscv_zkr_read(struct udevice *dev, void *data, size_t len)
+{
+ u8 *ptr = data;
+
+ while (len) {
+ u32 val;
+
+ val = read_seed();
+
+ switch (val >> 30) {
+ case BIST:
+ continue;
+ case WAIT:
+ continue;
+ case ES16:
+ *ptr++ = val & 0xff;
+ if (--len) {
+ *ptr++ = val >> 8;
+ --len;
+ }
+ break;
+ case DEAD:
+ return -ENODEV;
+ }
+ }
+
+ return 0;
+}
+
+/**
+ * riscv_zkr_probe() - check if the seed register is available
+ *
+ * If the SBI software has not set mseccfg.sseed=1 or the Zkr
+ * extension is not available this probe function will result
+ * in an exception. Currently we cannot recover from this.
+ *
+ * @dev: RNG device
+ * Return: 0 if successfully probed
+ */
+static int riscv_zkr_probe(struct udevice *dev)
+{
+ u32 val;
+
+ do {
+ val = read_seed();
+ val >>= 30;
+ } while (val == BIST || val == WAIT);
+
+ if (val == DEAD)
+ return -ENODEV;
+
+ return 0;
+}
+
+static const struct dm_rng_ops riscv_zkr_ops = {
+
[PATCH 2/2] usb: xhci: Hook up timeouts
Now that the USB core passes through timeout info to the host
controller, actually hook it up.
Signed-off-by: Hector Martin
---
drivers/usb/host/xhci-ring.c | 32
drivers/usb/host/xhci.c | 23 +--
include/usb/xhci.h | 14 ++
3 files changed, 43 insertions(+), 26 deletions(-)
diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
index dabe6cf86af2..14c0c60e8524 100644
--- a/drivers/usb/host/xhci-ring.c
+++ b/drivers/usb/host/xhci-ring.c
@@ -463,11 +463,16 @@ static int event_ready(struct xhci_ctrl *ctrl)
* @param expected TRB type expected from Event TRB
* Return: pointer to event trb
*/
-union xhci_trb *xhci_wait_for_event(struct xhci_ctrl *ctrl, trb_type expected)
+union xhci_trb *xhci_wait_for_event(struct xhci_ctrl *ctrl, trb_type expected,
+ int timeout)
{
trb_type type;
unsigned long ts = get_timer(0);
+ /* Fallback in case someone passes in 0 */
+ if (!timeout)
+ timeout = XHCI_TIMEOUT;
+
do {
union xhci_trb *event = ctrl->event_ring->dequeue;
@@ -504,7 +509,7 @@ union xhci_trb *xhci_wait_for_event(struct xhci_ctrl *ctrl,
trb_type expected)
le32_to_cpu(event->generic.field[3]));
xhci_acknowledge_event(ctrl);
- } while (get_timer(ts) < XHCI_TIMEOUT);
+ } while (get_timer(ts) < timeout);
if (expected == TRB_TRANSFER)
return NULL;
@@ -528,7 +533,7 @@ static void reset_ep(struct usb_device *udev, int ep_index)
printf("Resetting EP %d...\n", ep_index);
xhci_queue_command(ctrl, 0, udev->slot_id, ep_index, TRB_RESET_EP);
- event = xhci_wait_for_event(ctrl, TRB_COMPLETION);
+ event = xhci_wait_for_event(ctrl, TRB_COMPLETION, XHCI_SYS_TIMEOUT);
if (!event)
return;
@@ -539,7 +544,7 @@ static void reset_ep(struct usb_device *udev, int ep_index)
addr = xhci_trb_virt_to_dma(ring->enq_seg,
(void *)((uintptr_t)ring->enqueue | ring->cycle_state));
xhci_queue_command(ctrl, addr, udev->slot_id, ep_index, TRB_SET_DEQ);
- event = xhci_wait_for_event(ctrl, TRB_COMPLETION);
+ event = xhci_wait_for_event(ctrl, TRB_COMPLETION, XHCI_SYS_TIMEOUT);
if (!event)
return;
@@ -569,7 +574,7 @@ static void abort_td(struct usb_device *udev, int ep_index)
xhci_queue_command(ctrl, 0, udev->slot_id, ep_index, TRB_STOP_RING);
- event = xhci_wait_for_event(ctrl, TRB_NONE);
+ event = xhci_wait_for_event(ctrl, TRB_NONE, XHCI_SYS_TIMEOUT);
if (!event)
return;
@@ -582,7 +587,8 @@ static void abort_td(struct usb_device *udev, int ep_index)
!= COMP_STOP)));
xhci_acknowledge_event(ctrl);
- event = xhci_wait_for_event(ctrl, TRB_COMPLETION);
+ event = xhci_wait_for_event(ctrl, TRB_COMPLETION,
+ XHCI_SYS_TIMEOUT);
if (!event)
return;
type = TRB_FIELD_TO_TYPE(le32_to_cpu(event->event_cmd.flags));
@@ -601,7 +607,7 @@ static void abort_td(struct usb_device *udev, int ep_index)
addr = xhci_trb_virt_to_dma(ring->enq_seg,
(void *)((uintptr_t)ring->enqueue | ring->cycle_state));
xhci_queue_command(ctrl, addr, udev->slot_id, ep_index, TRB_SET_DEQ);
- event = xhci_wait_for_event(ctrl, TRB_COMPLETION);
+ event = xhci_wait_for_event(ctrl, TRB_COMPLETION, XHCI_SYS_TIMEOUT);
if (!event)
return;
@@ -657,10 +663,11 @@ static void record_transfer_result(struct usb_device
*udev,
* @param pipe contains the DIR_IN or OUT , devnum
* @param length length of the buffer
* @param buffer buffer to be read/written based on the request
+ * @param timeout timeout in milliseconds
* Return: returns 0 if successful else -1 on failure
*/
int xhci_bulk_tx(struct usb_device *udev, unsigned long pipe,
- int length, void *buffer)
+ int length, void *buffer, int timeout)
{
int num_trbs = 0;
struct xhci_generic_trb *start_trb;
@@ -825,7 +832,7 @@ int xhci_bulk_tx(struct usb_device *udev, unsigned long
pipe,
giveback_first_trb(udev, ep_index, start_cycle, start_trb);
again:
- event = xhci_wait_for_event(ctrl, TRB_TRANSFER);
+ event = xhci_wait_for_event(ctrl, TRB_TRANSFER, timeout);
if (!event) {
debug("XHCI bulk transfer timed out, aborting...\n");
abort_td(udev, ep_index);
@@ -862,11 +869,12 @@ again:
* @param req request type
* @param length length of the buffer
* @param buffer buffer to be read/written based on the request
+ * @param timeout timeout in milliseconds
* Return: returns
[PATCH 1/2] usb: Pass through timeout to drivers
The old USB code was interrupt-driven and just polled at the top level.
This has been obsolete since interrupts were removed, which means the
timeout support has been completely broken.
Rip out the top-level polling and just pass through the timeout
parameter to host controller drivers. Right now this is ignored in the
individual drivers.
Signed-off-by: Hector Martin
---
common/usb.c| 21 ++---
drivers/usb/host/ehci-hcd.c | 5 +++--
drivers/usb/host/ohci-hcd.c | 5 +++--
drivers/usb/host/r8a66597-hcd.c | 5 +++--
drivers/usb/host/usb-sandbox.c | 6 --
drivers/usb/host/usb-uclass.c | 9 +
drivers/usb/host/xhci.c | 5 +++--
include/usb.h | 10 ++
8 files changed, 29 insertions(+), 37 deletions(-)
diff --git a/common/usb.c b/common/usb.c
index 836506dcd9e9..8d13c5899027 100644
--- a/common/usb.c
+++ b/common/usb.c
@@ -241,22 +241,10 @@ int usb_control_msg(struct usb_device *dev, unsigned int
pipe,
request, requesttype, value, index, size);
dev->status = USB_ST_NOT_PROC; /*not yet processed */
- err = submit_control_msg(dev, pipe, data, size, setup_packet);
+ err = submit_control_msg(dev, pipe, data, size, setup_packet, timeout);
if (err < 0)
return err;
- if (timeout == 0)
- return (int)size;
- /*
-* Wait for status to update until timeout expires, USB driver
-* interrupt handler may set the status when the USB operation has
-* been completed.
-*/
- while (timeout--) {
- if (!((volatile unsigned long)dev->status & USB_ST_NOT_PROC))
- break;
- mdelay(1);
- }
if (dev->status)
return -1;
@@ -275,13 +263,8 @@ int usb_bulk_msg(struct usb_device *dev, unsigned int pipe,
if (len < 0)
return -EINVAL;
dev->status = USB_ST_NOT_PROC; /*not yet processed */
- if (submit_bulk_msg(dev, pipe, data, len) < 0)
+ if (submit_bulk_msg(dev, pipe, data, len, timeout) < 0)
return -EIO;
- while (timeout--) {
- if (!((volatile unsigned long)dev->status & USB_ST_NOT_PROC))
- break;
- mdelay(1);
- }
*actual_length = dev->act_len;
if (dev->status == 0)
return 0;
diff --git a/drivers/usb/host/ehci-hcd.c b/drivers/usb/host/ehci-hcd.c
index 9839aa17492d..ad0a1b9d24b1 100644
--- a/drivers/usb/host/ehci-hcd.c
+++ b/drivers/usb/host/ehci-hcd.c
@@ -1627,7 +1627,7 @@ int usb_lock_async(struct usb_device *dev, int lock)
#if CONFIG_IS_ENABLED(DM_USB)
static int ehci_submit_control_msg(struct udevice *dev, struct usb_device
*udev,
unsigned long pipe, void *buffer, int length,
- struct devrequest *setup)
+ struct devrequest *setup, int timeout)
{
debug("%s: dev='%s', udev=%p, udev->dev='%s', portnr=%d\n", __func__,
dev->name, udev, udev->dev->name, udev->portnr);
@@ -1636,7 +1636,8 @@ static int ehci_submit_control_msg(struct udevice *dev,
struct usb_device *udev,
}
static int ehci_submit_bulk_msg(struct udevice *dev, struct usb_device *udev,
- unsigned long pipe, void *buffer, int length)
+ unsigned long pipe, void *buffer, int length,
+ int timeout)
{
debug("%s: dev='%s', udev=%p\n", __func__, dev->name, udev);
return _ehci_submit_bulk_msg(udev, pipe, buffer, length);
diff --git a/drivers/usb/host/ohci-hcd.c b/drivers/usb/host/ohci-hcd.c
index 3f4418198ccd..8dc1f6660077 100644
--- a/drivers/usb/host/ohci-hcd.c
+++ b/drivers/usb/host/ohci-hcd.c
@@ -2047,7 +2047,7 @@ int submit_control_msg(struct usb_device *dev, unsigned
long pipe,
#if CONFIG_IS_ENABLED(DM_USB)
static int ohci_submit_control_msg(struct udevice *dev, struct usb_device
*udev,
unsigned long pipe, void *buffer, int length,
- struct devrequest *setup)
+ struct devrequest *setup, int timeout)
{
ohci_t *ohci = dev_get_priv(usb_get_bus(dev));
@@ -2056,7 +2056,8 @@ static int ohci_submit_control_msg(struct udevice *dev,
struct usb_device *udev,
}
static int ohci_submit_bulk_msg(struct udevice *dev, struct usb_device *udev,
- unsigned long pipe, void *buffer, int length)
+ unsigned long pipe, void *buffer, int length,
+ int timeout)
{
ohci_t *ohci = dev_get_priv(usb_get_bus(dev));
diff --git a/drivers/usb/host/r8a66597-hcd.c b/drivers/usb/host/r8a66597-hcd.c
index 3ccbc16da379..0ac853dc558b 100644
--- a/drivers/usb/host/r8a66597-hcd.c
+++ b/drivers/usb/host/r8a66597-hcd.c
@@
[PATCH 0/2] USB fixes: (Re)implement timeouts
A long time ago, the USB code was interrupt-driven and used top-level timeout handling. This has long been obsolete, and that code is just broken dead cruft. HC drivers instead hardcode timeouts today. We need to be able to specify timeouts explicitly to handle cases like USB hard disks spinning up, without having ridiculously long timeouts across the board (which would cause endless waiting when things go wrong anywhere else). So, it's time to rip out the old broken nonsense and actually pass through timeouts to USB host controller drivers, so they can be implemented properly. This series adds the necessary top-level scaffolding for control/bulk timeouts, and implements them in xHCI. I didn't bother with interrupt transfers, since I figure those probably never need long timeouts anyway. The platform I deal with only has xHCI, so I'll leave implementing this for EHCI/OHCI to someone else if anyone cares :) This series needs to be applied after [1], since the xHCI changes depend on changes made there. [1] https://lore.kernel.org/u-boot/20231029-usb-fixes-1-v2-0-623533f63...@marcan.st/ Signed-off-by: Hector Martin --- Hector Martin (2): usb: Pass through timeout to drivers usb: xhci: Hook up timeouts common/usb.c| 21 ++--- drivers/usb/host/ehci-hcd.c | 5 +++-- drivers/usb/host/ohci-hcd.c | 5 +++-- drivers/usb/host/r8a66597-hcd.c | 5 +++-- drivers/usb/host/usb-sandbox.c | 6 -- drivers/usb/host/usb-uclass.c | 9 + drivers/usb/host/xhci-ring.c| 32 drivers/usb/host/xhci.c | 28 include/usb.h | 10 ++ include/usb/xhci.h | 14 ++ 10 files changed, 72 insertions(+), 63 deletions(-) --- base-commit: 3d5d748e4d66b98109669c05d0c473fe67795801 change-id: 20231029-usb-fixes-5-ca87bbedb40c Best regards, -- Hector Martin
[PATCH] usb: Ignore endpoints in non-zero altsettings
We currently do not really handle altsettings properly, and no driver
uses them. Ignore the respective endpoint descriptors for secondary
altsettings, to avoid creating duplicate endpoint records in the
interface.
This will have to be revisited if/when we have a driver that needs
altsettings to work properly.
Signed-off-by: Hector Martin
---
common/usb.c | 9 +
1 file changed, 9 insertions(+)
diff --git a/common/usb.c b/common/usb.c
index aad13fd9c557..90f72fda00bc 100644
--- a/common/usb.c
+++ b/common/usb.c
@@ -463,6 +463,15 @@ static int usb_parse_config(struct usb_device *dev,
puts("Endpoint descriptor out of order!\n");
break;
}
+ if (if_desc->num_altsetting > 1) {
+ /*
+* Ignore altsettings, which can trigger
duplicate
+* endpoint errors below. Revisit this when some
+* driver actually needs altsettings with
differing
+* endpoint setups.
+*/
+ break;
+ }
epno = dev->config.if_desc[ifno].no_of_ep;
if_desc = >config.if_desc[ifno];
if (epno >= USB_MAXENDPOINTS) {
---
base-commit: 8ad1c9c26f7740806a162818b790d4a72f515b7e
change-id: 20231029-usb-fixes-4-ba6931acf217
Best regards,
--
Hector Martin
[PATCH 4/4] usb: storage: Implement 64-bit LBA support
This makes things work properly on devices with >= 2 TiB
capacity. If u-boot is built without CONFIG_SYS_64BIT_LBA,
the capacity will be clamped at 2^32 - 1 sectors.
Signed-off-by: Hector Martin
---
common/usb_storage.c | 132 ---
1 file changed, 114 insertions(+), 18 deletions(-)
diff --git a/common/usb_storage.c b/common/usb_storage.c
index 95507ffbce48..3035f2ee9868 100644
--- a/common/usb_storage.c
+++ b/common/usb_storage.c
@@ -66,7 +66,7 @@
static const unsigned char us_direction[256/8] = {
0x28, 0x81, 0x14, 0x14, 0x20, 0x01, 0x90, 0x77,
0x0C, 0x20, 0x00, 0x04, 0x00, 0x00, 0x00, 0x00,
- 0x00, 0x00, 0x00, 0x00, 0x00, 0x01, 0x00, 0x01,
+ 0x00, 0x01, 0x00, 0x40, 0x00, 0x01, 0x00, 0x01,
0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00
};
#define US_DIRECTION(x) ((us_direction[x>>3] >> (x & 7)) & 1)
@@ -1073,6 +1073,27 @@ static int usb_read_capacity(struct scsi_cmd *srb,
struct us_data *ss)
return -1;
}
+#ifdef CONFIG_SYS_64BIT_LBA
+static int usb_read_capacity64(struct scsi_cmd *srb, struct us_data *ss)
+{
+ int retry;
+ /* XXX retries */
+ retry = 3;
+ do {
+ memset(>cmd[0], 0, 16);
+ srb->cmd[0] = SCSI_SRV_ACTION_IN;
+ srb->cmd[1] = (srb->lun << 5) | SCSI_SAI_RD_CAPAC16;
+ srb->cmd[13] = 32; /* Allocation length */
+ srb->datalen = 32;
+ srb->cmdlen = 16;
+ if (ss->transport(srb, ss) == USB_STOR_TRANSPORT_GOOD)
+ return 0;
+ } while (retry--);
+
+ return -1;
+}
+#endif
+
static int usb_read_10(struct scsi_cmd *srb, struct us_data *ss,
unsigned long start, unsigned short blocks)
{
@@ -1107,6 +1128,49 @@ static int usb_write_10(struct scsi_cmd *srb, struct
us_data *ss,
return ss->transport(srb, ss);
}
+#ifdef CONFIG_SYS_64BIT_LBA
+static int usb_read_16(struct scsi_cmd *srb, struct us_data *ss,
+ uint64_t start, unsigned short blocks)
+{
+ memset(>cmd[0], 0, 16);
+ srb->cmd[0] = SCSI_READ16;
+ srb->cmd[1] = srb->lun << 5;
+ srb->cmd[2] = ((unsigned char) (start >> 56)) & 0xff;
+ srb->cmd[3] = ((unsigned char) (start >> 48)) & 0xff;
+ srb->cmd[4] = ((unsigned char) (start >> 40)) & 0xff;
+ srb->cmd[5] = ((unsigned char) (start >> 32)) & 0xff;
+ srb->cmd[6] = ((unsigned char) (start >> 24)) & 0xff;
+ srb->cmd[7] = ((unsigned char) (start >> 16)) & 0xff;
+ srb->cmd[8] = ((unsigned char) (start >> 8)) & 0xff;
+ srb->cmd[9] = ((unsigned char) (start)) & 0xff;
+ srb->cmd[12] = ((unsigned char) (blocks >> 8)) & 0xff;
+ srb->cmd[13] = (unsigned char) blocks & 0xff;
+ srb->cmdlen = 16;
+ debug("read16: start %llx blocks %x\n", (long long)start, blocks);
+ return ss->transport(srb, ss);
+}
+
+static int usb_write_16(struct scsi_cmd *srb, struct us_data *ss,
+ uint64_t start, unsigned short blocks)
+{
+ memset(>cmd[0], 0, 16);
+ srb->cmd[0] = SCSI_WRITE16;
+ srb->cmd[1] = srb->lun << 5;
+ srb->cmd[2] = ((unsigned char) (start >> 56)) & 0xff;
+ srb->cmd[3] = ((unsigned char) (start >> 48)) & 0xff;
+ srb->cmd[4] = ((unsigned char) (start >> 40)) & 0xff;
+ srb->cmd[5] = ((unsigned char) (start >> 32)) & 0xff;
+ srb->cmd[6] = ((unsigned char) (start >> 24)) & 0xff;
+ srb->cmd[7] = ((unsigned char) (start >> 16)) & 0xff;
+ srb->cmd[8] = ((unsigned char) (start >> 8)) & 0xff;
+ srb->cmd[9] = ((unsigned char) (start)) & 0xff;
+ srb->cmd[12] = ((unsigned char) (blocks >> 8)) & 0xff;
+ srb->cmd[13] = (unsigned char) blocks & 0xff;
+ srb->cmdlen = 16;
+ debug("write16: start %llx blocks %x\n", (long long)start, blocks);
+ return ss->transport(srb, ss);
+}
+#endif
#ifdef CONFIG_USB_BIN_FIXUP
/*
@@ -1145,6 +1209,7 @@ static unsigned long usb_stor_read(struct blk_desc
*block_dev, lbaint_t blknr,
struct usb_device *udev;
struct us_data *ss;
int retry;
+ int ret;
struct scsi_cmd *srb = _ccb;
#if CONFIG_IS_ENABLED(BLK)
struct blk_desc *block_dev;
@@ -1190,7 +1255,13 @@ retry_it:
usb_show_progress();
srb->datalen = block_dev->blksz * smallblks;
srb->pdata = (unsigned char *)buf_addr;
- if (usb_read_10(srb, ss, start, smallblks)) {
+#ifdef CONFIG_SYS_64BIT_LBA
+ if (block_dev->lba > ((lbaint_t)0x1))
+ ret = usb_read_16(srb, ss, start, smallblks);
+ else
+#endif
+ ret = usb_read_10(srb, ss, start, smallblks);
+ if (ret) {
debug("Read ERROR\n");
ss->flags &= ~USB_READY;
usb_request_sense(srb, ss);
@@ -1228,6 +1299,7 @@ static unsigned long
[PATCH 3/4] usb: storage: Use the correct CBW lengths
USB UFI uses fixed 12-byte commands (as does RBC, which is not
supported), but SCSI does not have this limitation. Use the correct
command block lengths depending on the subclass.
Signed-off-by: Hector Martin
---
common/usb_storage.c | 22 ++
1 file changed, 14 insertions(+), 8 deletions(-)
diff --git a/common/usb_storage.c b/common/usb_storage.c
index 729ddbc75a48..95507ffbce48 100644
--- a/common/usb_storage.c
+++ b/common/usb_storage.c
@@ -107,6 +107,7 @@ struct us_data {
trans_reset transport_reset;/* reset routine */
trans_cmnd transport; /* transport routine */
unsigned short max_xfer_blk; /* maximum transfer blocks */
+ boolcmd12; /* use 12-byte commands
(RBC/UFI) */
};
#if !CONFIG_IS_ENABLED(BLK)
@@ -349,7 +350,7 @@ static void usb_show_srb(struct scsi_cmd *pccb)
{
int i;
printf("SRB: len %d datalen 0x%lX\n ", pccb->cmdlen, pccb->datalen);
- for (i = 0; i < 12; i++)
+ for (i = 0; i < pccb->cmdlen; i++)
printf("%02X ", pccb->cmd[i]);
printf("\n");
}
@@ -888,7 +889,7 @@ do_retry:
psrb->cmd[4] = 18;
psrb->datalen = 18;
psrb->pdata = >sense_buf[0];
- psrb->cmdlen = 12;
+ psrb->cmdlen = us->cmd12 ? 12 : 6;
/* issue the command */
result = usb_stor_CB_comdat(psrb, us);
debug("auto request returned %d\n", result);
@@ -989,7 +990,7 @@ static int usb_inquiry(struct scsi_cmd *srb, struct us_data
*ss)
srb->cmd[1] = srb->lun << 5;
srb->cmd[4] = 36;
srb->datalen = 36;
- srb->cmdlen = 12;
+ srb->cmdlen = ss->cmd12 ? 12 : 6;
i = ss->transport(srb, ss);
debug("inquiry returns %d\n", i);
if (i == 0)
@@ -1014,7 +1015,7 @@ static int usb_request_sense(struct scsi_cmd *srb, struct
us_data *ss)
srb->cmd[4] = 18;
srb->datalen = 18;
srb->pdata = >sense_buf[0];
- srb->cmdlen = 12;
+ srb->cmdlen = ss->cmd12 ? 12 : 6;
ss->transport(srb, ss);
debug("Request Sense returned %02X %02X %02X\n",
srb->sense_buf[2], srb->sense_buf[12],
@@ -1032,7 +1033,7 @@ static int usb_test_unit_ready(struct scsi_cmd *srb,
struct us_data *ss)
srb->cmd[0] = SCSI_TST_U_RDY;
srb->cmd[1] = srb->lun << 5;
srb->datalen = 0;
- srb->cmdlen = 12;
+ srb->cmdlen = ss->cmd12 ? 12 : 6;
if (ss->transport(srb, ss) == USB_STOR_TRANSPORT_GOOD) {
ss->flags |= USB_READY;
return 0;
@@ -1064,7 +1065,7 @@ static int usb_read_capacity(struct scsi_cmd *srb, struct
us_data *ss)
srb->cmd[0] = SCSI_RD_CAPAC;
srb->cmd[1] = srb->lun << 5;
srb->datalen = 8;
- srb->cmdlen = 12;
+ srb->cmdlen = ss->cmd12 ? 12 : 10;
if (ss->transport(srb, ss) == USB_STOR_TRANSPORT_GOOD)
return 0;
} while (retry--);
@@ -1084,7 +1085,7 @@ static int usb_read_10(struct scsi_cmd *srb, struct
us_data *ss,
srb->cmd[5] = ((unsigned char) (start)) & 0xff;
srb->cmd[7] = ((unsigned char) (blocks >> 8)) & 0xff;
srb->cmd[8] = (unsigned char) blocks & 0xff;
- srb->cmdlen = 12;
+ srb->cmdlen = ss->cmd12 ? 12 : 10;
debug("read10: start %lx blocks %x\n", start, blocks);
return ss->transport(srb, ss);
}
@@ -1101,7 +1102,7 @@ static int usb_write_10(struct scsi_cmd *srb, struct
us_data *ss,
srb->cmd[5] = ((unsigned char) (start)) & 0xff;
srb->cmd[7] = ((unsigned char) (blocks >> 8)) & 0xff;
srb->cmd[8] = (unsigned char) blocks & 0xff;
- srb->cmdlen = 12;
+ srb->cmdlen = ss->cmd12 ? 12 : 10;
debug("write10: start %lx blocks %x\n", start, blocks);
return ss->transport(srb, ss);
}
@@ -1407,6 +1408,11 @@ int usb_storage_probe(struct usb_device *dev, unsigned
int ifnum,
printf("Sorry, protocol %d not yet supported.\n", ss->subclass);
return 0;
}
+
+ /* UFI uses 12-byte commands (like RBC, unlike SCSI) */
+ if (ss->subclass == US_SC_UFI)
+ ss->cmd12 = true;
+
if (ss->ep_int) {
/* we had found an interrupt endpoint, prepare irq pipe
* set up the IRQ pipe and handler
--
2.41.0
[PATCH 1/4] scsi: Fix a bunch of SCSI definitions.
0x9e isn't Read Capacity, it's a service action and the read capacity
command is a subcommand.
READ16 is not 0x48, it's 0x88. 0x48 is SANITIZE and that sounds like we
might have been destroying data instead of reading data. No bueno.
Signed-off-by: Hector Martin
---
drivers/ata/ahci.c | 9 ++---
drivers/scsi/scsi.c | 4 ++--
include/scsi.h | 8 ++--
3 files changed, 14 insertions(+), 7 deletions(-)
diff --git a/drivers/ata/ahci.c b/drivers/ata/ahci.c
index 2062197afcd3..b252e9e525db 100644
--- a/drivers/ata/ahci.c
+++ b/drivers/ata/ahci.c
@@ -906,15 +906,18 @@ static int ahci_scsi_exec(struct udevice *dev, struct
scsi_cmd *pccb)
case SCSI_RD_CAPAC10:
ret = ata_scsiop_read_capacity10(uc_priv, pccb);
break;
- case SCSI_RD_CAPAC16:
- ret = ata_scsiop_read_capacity16(uc_priv, pccb);
- break;
case SCSI_TST_U_RDY:
ret = ata_scsiop_test_unit_ready(uc_priv, pccb);
break;
case SCSI_INQUIRY:
ret = ata_scsiop_inquiry(uc_priv, pccb);
break;
+ case SCSI_SRV_ACTION_IN:
+ if ((pccb->cmd[1] & 0x1f) == SCSI_SAI_RD_CAPAC16) {
+ ret = ata_scsiop_read_capacity16(uc_priv, pccb);
+ break;
+ }
+ /* Fallthrough */
default:
printf("Unsupport SCSI command 0x%02x\n", pccb->cmd[0]);
return -ENOTSUPP;
diff --git a/drivers/scsi/scsi.c b/drivers/scsi/scsi.c
index d7b33010e469..f2c828eb305e 100644
--- a/drivers/scsi/scsi.c
+++ b/drivers/scsi/scsi.c
@@ -380,8 +380,8 @@ static int scsi_read_capacity(struct udevice *dev, struct
scsi_cmd *pccb,
/* Read capacity (10) was insufficient. Use read capacity (16). */
memset(pccb->cmd, '\0', sizeof(pccb->cmd));
- pccb->cmd[0] = SCSI_RD_CAPAC16;
- pccb->cmd[1] = 0x10;
+ pccb->cmd[0] = SCSI_SRV_ACTION_IN;
+ pccb->cmd[1] = SCSI_SAI_RD_CAPAC16;
pccb->cmdlen = 16;
pccb->msgout[0] = SCSI_IDENTIFY; /* NOT USED */
diff --git a/include/scsi.h b/include/scsi.h
index b47c7463c1d6..89e268586477 100644
--- a/include/scsi.h
+++ b/include/scsi.h
@@ -141,10 +141,9 @@ struct scsi_cmd {
#define SCSI_MED_REMOVL0x1E/* Prevent/Allow medium Removal
(O) */
#define SCSI_READ6 0x08/* Read 6-byte (MANDATORY) */
#define SCSI_READ100x28/* Read 10-byte (MANDATORY) */
-#define SCSI_READ160x48
+#define SCSI_READ160x88/* Read 16-byte */
#define SCSI_RD_CAPAC 0x25/* Read Capacity (MANDATORY) */
#define SCSI_RD_CAPAC10SCSI_RD_CAPAC /* Read Capacity (10) */
-#define SCSI_RD_CAPAC160x9e/* Read Capacity (16) */
#define SCSI_RD_DEFECT 0x37/* Read Defect Data (O) */
#define SCSI_READ_LONG 0x3E/* Read Long (O) */
#define SCSI_REASS_BLK 0x07/* Reassign Blocks (O) */
@@ -158,15 +157,20 @@ struct scsi_cmd {
#define SCSI_SEEK100x2B/* Seek 10-Byte (O) */
#define SCSI_SEND_DIAG 0x1D/* Send Diagnostics (MANDATORY) */
#define SCSI_SET_LIMIT 0x33/* Set Limits (O) */
+#define SCSI_SRV_ACTION_IN 0x9E/* Service Action In */
+#define SCSI_SRV_ACTION_OUT0x9F/* Service Action Out */
#define SCSI_START_STP 0x1B/* Start/Stop Unit (O) */
#define SCSI_SYNC_CACHE0x35/* Synchronize Cache (O) */
#define SCSI_VERIFY0x2F/* Verify (O) */
#define SCSI_WRITE60x0A/* Write 6-Byte (MANDATORY) */
#define SCSI_WRITE10 0x2A/* Write 10-Byte (MANDATORY) */
+#define SCSI_WRITE16 0x8A/* Write 16-byte */
#define SCSI_WRT_VERIFY0x2E/* Write and Verify (O) */
#define SCSI_WRITE_LONG0x3F/* Write Long (O) */
#define SCSI_WRITE_SAME0x41/* Write Same (O) */
+#define SCSI_SAI_RD_CAPAC160x10/* Service Action: Read Capacity (16) */
+
/**
* struct scsi_plat - stores information about SCSI controller
*
--
2.41.0
[PATCH 2/4] usb: storage: Increase read/write timeout
Some USB devices (like hard disks) can take a long time to initially
respond to read/write requests. Explicitly specify a much longer timeout
than normal.
Signed-off-by: Hector Martin
---
common/usb_storage.c | 10 --
1 file changed, 8 insertions(+), 2 deletions(-)
diff --git a/common/usb_storage.c b/common/usb_storage.c
index c9e2d7343ce2..729ddbc75a48 100644
--- a/common/usb_storage.c
+++ b/common/usb_storage.c
@@ -53,6 +53,12 @@
#undef BBB_COMDAT_TRACE
#undef BBB_XPORT_TRACE
+/*
+ * Timeout for read/write transfers. This needs to be able to handle very slow
+ * devices, such as hard disks that are spinning up.
+ */
+#define US_XFER_TIMEOUT 15000
+
#include
/* direction table -- this indicates the direction of the data
* transfer for each command code -- a 1 indicates input
@@ -394,7 +400,7 @@ static int us_one_transfer(struct us_data *us, int pipe,
char *buf, int length)
11 - maxtry);
result = usb_bulk_msg(us->pusb_dev, pipe, buf,
this_xfer, ,
- USB_CNTL_TIMEOUT * 5);
+ US_XFER_TIMEOUT);
debug("bulk_msg returned %d xferred %d/%d\n",
result, partial, this_xfer);
if (us->pusb_dev->status != 0) {
@@ -743,7 +749,7 @@ static int usb_stor_BBB_transport(struct scsi_cmd *srb,
struct us_data *us)
pipe = pipeout;
result = usb_bulk_msg(us->pusb_dev, pipe, srb->pdata, srb->datalen,
- _actlen, USB_CNTL_TIMEOUT * 5);
+ _actlen, US_XFER_TIMEOUT);
/* special handling of STALL in DATA phase */
if ((result < 0) && (us->pusb_dev->status & USB_ST_STALLED)) {
debug("DATA:stall\n");
--
2.41.0
[PATCH 0/4] USB fixes: Mass Storage bugs & 64bit support
This series fixes some bugs in the USBMS driver and adds 64-bit LBA support. This is required to make USB HDDs >=4TB work. Note that the increased timeout won't actually work right now, due to broken handling in the underlying USB infrastructure. That will be fixed in a follow-up series, which depends on [1] being applied first. The USBMS part is logically stand-alone and can be applied in parallel before that. [1] https://lore.kernel.org/u-boot/20231029-usb-fixes-1-v2-0-623533f63...@marcan.st/ Signed-off-by: Hector Martin --- Hector Martin (4): scsi: Fix a bunch of SCSI definitions. usb: storage: Increase read/write timeout usb: storage: Use the correct CBW lengths usb: storage: Implement 64-bit LBA support common/usb_storage.c | 164 ++- drivers/ata/ahci.c | 9 ++- drivers/scsi/scsi.c | 4 +- include/scsi.h | 8 ++- 4 files changed, 150 insertions(+), 35 deletions(-) --- base-commit: 8ad1c9c26f7740806a162818b790d4a72f515b7e change-id: 20231029-usb-fixes-3-c72f829ba61b Best regards, -- Hector Martin
[PATCH 2/2] usb: hub: Add missing reset recovery delay
Some devices like YubiKeys need more time before SET_ADDRESS. The spec says we need to wait 10ms. Signed-off-by: Hector Martin --- common/usb_hub.c | 7 +++ 1 file changed, 7 insertions(+) diff --git a/common/usb_hub.c b/common/usb_hub.c index ba11a188ca64..858ada0f73be 100644 --- a/common/usb_hub.c +++ b/common/usb_hub.c @@ -391,6 +391,13 @@ int usb_hub_port_connect_change(struct usb_device *dev, int port) break; } + /* +* USB 2.0 7.1.7.5: devices must be able to accept a SetAddress() +* request (refer to Section 11.24.2 and Section 9.4 respectively) +* after the reset recovery time 10 ms +*/ + mdelay(10); + #if CONFIG_IS_ENABLED(DM_USB) struct udevice *child; -- 2.41.0
[PATCH 1/2] usb: kbd: Ignore Yubikeys
We currently only support one USB keyboard device, but some devices
emulate keyboards for other purposes. Most commonly, people run into
this with Yubikeys, so let's ignore those.
Even if we end up supporting multiple keyboards in the future, it's
safer to ignore known non-keyboard devices.
This is particularly important to avoid regressing some users, since
YubiKeys often *don't* work due to other bugs in the USB stack, but will
start to work once they are fixed.
Signed-off-by: Hector Martin
---
common/usb_kbd.c | 19 +++
1 file changed, 19 insertions(+)
diff --git a/common/usb_kbd.c b/common/usb_kbd.c
index 352d86fb2ece..e8c102c567e4 100644
--- a/common/usb_kbd.c
+++ b/common/usb_kbd.c
@@ -120,6 +120,15 @@ struct usb_kbd_pdata {
extern int __maybe_unused net_busy_flag;
+/*
+ * Since we only support one usbkbd device in the iomux,
+ * ignore common keyboard-emulating devices that aren't
+ * real keyboards.
+ */
+const uint16_t vid_blocklist[] = {
+ 0x1050, /* Yubico */
+};
+
/* The period of time between two calls of usb_kbd_testc(). */
static unsigned long kbd_testc_tms;
@@ -465,6 +474,7 @@ static int usb_kbd_probe_dev(struct usb_device *dev,
unsigned int ifnum)
struct usb_endpoint_descriptor *ep;
struct usb_kbd_pdata *data;
int epNum;
+ int i;
if (dev->descriptor.bNumConfigurations != 1)
return 0;
@@ -480,6 +490,15 @@ static int usb_kbd_probe_dev(struct usb_device *dev,
unsigned int ifnum)
if (iface->desc.bInterfaceProtocol != USB_PROT_HID_KEYBOARD)
return 0;
+ for (i = 0; i < ARRAY_SIZE(vid_blocklist); i++) {
+ if (dev->descriptor.idVendor == vid_blocklist[i]) {
+ printf("Ignoring keyboard device 0x%x:0x%x\n",
+ dev->descriptor.idVendor,
+ dev->descriptor.idProduct);
+ return 0;
+ }
+ }
+
for (epNum = 0; epNum < iface->desc.bNumEndpoints; epNum++) {
ep = >ep_desc[epNum];
--
2.41.0
[PATCH 0/2] USB fixes: Add missing timeout, ignore YubiKeys
This mini series fixes one bug, but in the process makes YubiKeys work, which then regresses people who have one *and* a USB keyboard, since we only support a single keyboard device. Therefore patch #1 makes U-Boot ignore YubiKeys, so #2 does not regress things. Signed-off-by: Hector Martin --- Hector Martin (2): usb: kbd: Ignore Yubikeys usb: hub: Add missing reset recovery delay common/usb_hub.c | 7 +++ common/usb_kbd.c | 19 +++ 2 files changed, 26 insertions(+) --- base-commit: 8ad1c9c26f7740806a162818b790d4a72f515b7e change-id: 20231029-usb-fixes-2-976486d1603c Best regards, -- Hector Martin
[PATCH v2 8/8] usb: xhci: Add more debugging
A bunch of miscellaneous debug messages to aid in working out USB
issues.
Signed-off-by: Hector Martin
---
drivers/usb/host/xhci-ring.c | 29 ++---
1 file changed, 26 insertions(+), 3 deletions(-)
diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
index b60661fe05e7..dabe6cf86af2 100644
--- a/drivers/usb/host/xhci-ring.c
+++ b/drivers/usb/host/xhci-ring.c
@@ -214,6 +214,9 @@ static dma_addr_t queue_trb(struct xhci_ctrl *ctrl, struct
xhci_ring *ring,
addr = xhci_trb_virt_to_dma(ring->enq_seg, (union xhci_trb *)trb);
+ debug("trb @ %llx: %08x %08x %08x %08x\n", addr,
+ trb_fields[0], trb_fields[1], trb_fields[2], trb_fields[3]);
+
inc_enq(ctrl, ring, more_trbs_coming);
return addr;
@@ -296,6 +299,8 @@ void xhci_queue_command(struct xhci_ctrl *ctrl, dma_addr_t
addr, u32 slot_id,
{
u32 fields[4];
+ debug("CMD: %llx 0x%x 0x%x %d\n", addr, slot_id, ep_index, cmd);
+
BUG_ON(prepare_ring(ctrl, ctrl->cmd_ring, EP_STATE_RUNNING));
fields[0] = lower_32_bits(addr);
@@ -471,8 +476,14 @@ union xhci_trb *xhci_wait_for_event(struct xhci_ctrl
*ctrl, trb_type expected)
type = TRB_FIELD_TO_TYPE(le32_to_cpu(event->event_cmd.flags));
if (type == expected ||
- (expected == TRB_NONE && type != TRB_PORT_STATUS))
+ (expected == TRB_NONE && type != TRB_PORT_STATUS)) {
+ debug("Event: %08x %08x %08x %08x\n",
+ le32_to_cpu(event->generic.field[0]),
+ le32_to_cpu(event->generic.field[1]),
+ le32_to_cpu(event->generic.field[2]),
+ le32_to_cpu(event->generic.field[3]));
return event;
+ }
if (type == TRB_PORT_STATUS)
/* TODO: remove this once enumeration has been reworked */
@@ -484,8 +495,9 @@ union xhci_trb *xhci_wait_for_event(struct xhci_ctrl *ctrl,
trb_type expected)
le32_to_cpu(event->generic.field[2])) !=
COMP_SUCCESS);
else
- printf("Unexpected XHCI event TRB, skipping... "
+ printf("Unexpected XHCI event TRB, expected %d... "
"(%08x %08x %08x %08x)\n",
+ expected,
le32_to_cpu(event->generic.field[0]),
le32_to_cpu(event->generic.field[1]),
le32_to_cpu(event->generic.field[2]),
@@ -602,10 +614,13 @@ static void abort_td(struct usb_device *udev, int
ep_index)
static void record_transfer_result(struct usb_device *udev,
union xhci_trb *event, int length)
{
+ xhci_comp_code code = GET_COMP_CODE(
+ le32_to_cpu(event->trans_event.transfer_len));
+
udev->act_len = min(length, length -
(int)EVENT_TRB_LEN(le32_to_cpu(event->trans_event.transfer_len)));
- switch (GET_COMP_CODE(le32_to_cpu(event->trans_event.transfer_len))) {
+ switch (code) {
case COMP_SUCCESS:
BUG_ON(udev->act_len != length);
/* fallthrough */
@@ -613,16 +628,23 @@ static void record_transfer_result(struct usb_device
*udev,
udev->status = 0;
break;
case COMP_STALL:
+ debug("Xfer STALL\n");
udev->status = USB_ST_STALLED;
break;
case COMP_DB_ERR:
+ debug("Xfer DB_ERR\n");
+ udev->status = USB_ST_BUF_ERR;
+ break;
case COMP_TRB_ERR:
+ debug("Xfer TRB_ERR\n");
udev->status = USB_ST_BUF_ERR;
break;
case COMP_BABBLE:
+ debug("Xfer BABBLE\n");
udev->status = USB_ST_BABBLE_DET;
break;
default:
+ debug("Xfer error: %d\n", code);
udev->status = 0x80; /* USB_ST_TOO_LAZY_TO_MAKE_A_NEW_MACRO */
}
}
@@ -1016,6 +1038,7 @@ int xhci_ctrl_tx(struct usb_device *udev, unsigned long
pipe,
record_transfer_result(udev, event, length);
xhci_acknowledge_event(ctrl);
if (udev->status == USB_ST_STALLED) {
+ debug("EP %d stalled\n", ep_index);
reset_ep(udev, ep_index);
return -EPIPE;
}
--
2.41.0
[PATCH v2 7/8] usb: xhci: Fix DMA address calculation in queue_trb
We need to get the DMA address before incrementing the pointer, as that
might move us onto another segment.
Signed-off-by: Hector Martin
---
drivers/usb/host/xhci-ring.c | 5 -
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
index ae0ab5744df0..b60661fe05e7 100644
--- a/drivers/usb/host/xhci-ring.c
+++ b/drivers/usb/host/xhci-ring.c
@@ -202,6 +202,7 @@ static dma_addr_t queue_trb(struct xhci_ctrl *ctrl, struct
xhci_ring *ring,
bool more_trbs_coming, unsigned int *trb_fields)
{
struct xhci_generic_trb *trb;
+ dma_addr_t addr;
int i;
trb = >enqueue->generic;
@@ -211,9 +212,11 @@ static dma_addr_t queue_trb(struct xhci_ctrl *ctrl, struct
xhci_ring *ring,
xhci_flush_cache((uintptr_t)trb, sizeof(struct xhci_generic_trb));
+ addr = xhci_trb_virt_to_dma(ring->enq_seg, (union xhci_trb *)trb);
+
inc_enq(ctrl, ring, more_trbs_coming);
- return xhci_trb_virt_to_dma(ring->enq_seg, (union xhci_trb *)trb);
+ return addr;
}
/**
--
2.41.0
[PATCH v2 6/8] usb: xhci: Do not panic on event timeouts
Now that we always check the return value, just return NULL on timeouts.
We can still log the error since this is a problem, but it's not reason
to panic.
Signed-off-by: Hector Martin
---
drivers/usb/host/xhci-ring.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
index a969eafdc8ee..ae0ab5744df0 100644
--- a/drivers/usb/host/xhci-ring.c
+++ b/drivers/usb/host/xhci-ring.c
@@ -494,8 +494,9 @@ union xhci_trb *xhci_wait_for_event(struct xhci_ctrl *ctrl,
trb_type expected)
if (expected == TRB_TRANSFER)
return NULL;
- printf("XHCI timeout on event type %d... cannot recover.\n", expected);
- BUG();
+ printf("XHCI timeout on event type %d...\n", expected);
+
+ return NULL;
}
/*
--
2.41.0
[PATCH v2 5/8] usb: xhci: Fail on attempt to queue TRBs to a halted endpoint
This isn't going to work, don't pretend it will and then end up timing
out.
Signed-off-by: Hector Martin
---
drivers/usb/host/xhci-ring.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
index db8b8f200250..a969eafdc8ee 100644
--- a/drivers/usb/host/xhci-ring.c
+++ b/drivers/usb/host/xhci-ring.c
@@ -243,7 +243,8 @@ static int prepare_ring(struct xhci_ctrl *ctrl, struct
xhci_ring *ep_ring,
puts("WARN waiting for error on ep to be cleared\n");
return -EINVAL;
case EP_STATE_HALTED:
- puts("WARN halted endpoint, queueing URB anyway.\n");
+ puts("WARN endpoint is halted\n");
+ return -EINVAL;
case EP_STATE_STOPPED:
case EP_STATE_RUNNING:
debug("EP STATE RUNNING.\n");
--
2.41.0
[PATCH v2 4/8] usb: xhci: Recover from halted bulk endpoints
There is currently no codepath to recover from this case. In principle we could require that the upper layer do this explicitly, but let's just do it in xHCI when the next bulk transfer is started, since that reasonably implies whatever caused the problem has been dealt with. Signed-off-by: Hector Martin --- drivers/usb/host/xhci-ring.c | 8 1 file changed, 8 insertions(+) diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c index e02a6e300c4f..db8b8f200250 100644 --- a/drivers/usb/host/xhci-ring.c +++ b/drivers/usb/host/xhci-ring.c @@ -671,6 +671,14 @@ int xhci_bulk_tx(struct usb_device *udev, unsigned long pipe, ep_ctx = xhci_get_ep_ctx(ctrl, virt_dev->out_ctx, ep_index); + /* +* If the endpoint was halted due to a prior error, resume it before +* the next transfer. It is the responsibility of the upper layer to +* have dealt with whatever caused the error. +*/ + if ((le32_to_cpu(ep_ctx->ep_info) & EP_STATE_MASK) == EP_STATE_HALTED) + reset_ep(udev, ep_index); + ring = virt_dev->eps[ep_index].ring; /* * How much data is (potentially) left before the 64KB boundary? -- 2.41.0
[PATCH v2 3/8] usb: xhci: Allow context state errors when halting an endpoint
There is a race where an endpoint may halt by itself while we are trying
to halt it, which results in a context state error. See xHCI 4.6.9 which
mentions this case.
This also avoids BUGging when we attempt to stop an endpoint which was
already stopped to begin with, which is probably a bug elsewhere but
not a good reason to crash.
Signed-off-by: Hector Martin
---
drivers/usb/host/xhci-ring.c | 6 --
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
index d21e76e0bdb6..e02a6e300c4f 100644
--- a/drivers/usb/host/xhci-ring.c
+++ b/drivers/usb/host/xhci-ring.c
@@ -545,6 +545,7 @@ static void abort_td(struct usb_device *udev, int ep_index)
struct xhci_ctrl *ctrl = xhci_get_ctrl(udev);
struct xhci_ring *ring = ctrl->devs[udev->slot_id]->eps[ep_index].ring;
union xhci_trb *event;
+ xhci_comp_code comp;
trb_type type;
u64 addr;
u32 field;
@@ -573,10 +574,11 @@ static void abort_td(struct usb_device *udev, int
ep_index)
printf("abort_td: Expected a TRB_TRANSFER TRB first\n");
}
+ comp = GET_COMP_CODE(le32_to_cpu(event->event_cmd.status));
BUG_ON(type != TRB_COMPLETION ||
TRB_TO_SLOT_ID(le32_to_cpu(event->event_cmd.flags))
- != udev->slot_id || GET_COMP_CODE(le32_to_cpu(
- event->event_cmd.status)) != COMP_SUCCESS);
+ != udev->slot_id || (comp != COMP_SUCCESS && comp
+ != COMP_CTX_STATE));
xhci_acknowledge_event(ctrl);
addr = xhci_trb_virt_to_dma(ring->enq_seg,
--
2.41.0
[PATCH v2 2/8] usb: xhci: Better error handling in abort_td()
If the xHC has a problem with our STOP ENDPOINT command, it is likely to
return a completion directly instead of first a transfer event for the
in-progress transfer. Handle that more gracefully.
We still BUG() on the error code, but at least we don't end up timing
out on the event and ending up with unexpected event errors.
Signed-off-by: Hector Martin
---
drivers/usb/host/xhci-ring.c | 34 ++
include/usb/xhci.h | 2 ++
2 files changed, 24 insertions(+), 12 deletions(-)
diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
index d0960812a47b..d21e76e0bdb6 100644
--- a/drivers/usb/host/xhci-ring.c
+++ b/drivers/usb/host/xhci-ring.c
@@ -466,7 +466,8 @@ union xhci_trb *xhci_wait_for_event(struct xhci_ctrl *ctrl,
trb_type expected)
continue;
type = TRB_FIELD_TO_TYPE(le32_to_cpu(event->event_cmd.flags));
- if (type == expected)
+ if (type == expected ||
+ (expected == TRB_NONE && type != TRB_PORT_STATUS))
return event;
if (type == TRB_PORT_STATUS)
@@ -544,27 +545,36 @@ static void abort_td(struct usb_device *udev, int
ep_index)
struct xhci_ctrl *ctrl = xhci_get_ctrl(udev);
struct xhci_ring *ring = ctrl->devs[udev->slot_id]->eps[ep_index].ring;
union xhci_trb *event;
+ trb_type type;
u64 addr;
u32 field;
xhci_queue_command(ctrl, 0, udev->slot_id, ep_index, TRB_STOP_RING);
- event = xhci_wait_for_event(ctrl, TRB_TRANSFER);
+ event = xhci_wait_for_event(ctrl, TRB_NONE);
if (!event)
return;
- field = le32_to_cpu(event->trans_event.flags);
- BUG_ON(TRB_TO_SLOT_ID(field) != udev->slot_id);
- BUG_ON(TRB_TO_EP_INDEX(field) != ep_index);
- BUG_ON(GET_COMP_CODE(le32_to_cpu(event->trans_event.transfer_len
- != COMP_STOP)));
- xhci_acknowledge_event(ctrl);
+ type = TRB_FIELD_TO_TYPE(le32_to_cpu(event->event_cmd.flags));
+ if (type == TRB_TRANSFER) {
+ field = le32_to_cpu(event->trans_event.flags);
+ BUG_ON(TRB_TO_SLOT_ID(field) != udev->slot_id);
+ BUG_ON(TRB_TO_EP_INDEX(field) != ep_index);
+ BUG_ON(GET_COMP_CODE(le32_to_cpu(event->trans_event.transfer_len
+ != COMP_STOP)));
+ xhci_acknowledge_event(ctrl);
- event = xhci_wait_for_event(ctrl, TRB_COMPLETION);
- if (!event)
- return;
+ event = xhci_wait_for_event(ctrl, TRB_COMPLETION);
+ if (!event)
+ return;
+ type = TRB_FIELD_TO_TYPE(le32_to_cpu(event->event_cmd.flags));
- BUG_ON(TRB_TO_SLOT_ID(le32_to_cpu(event->event_cmd.flags))
+ } else {
+ printf("abort_td: Expected a TRB_TRANSFER TRB first\n");
+ }
+
+ BUG_ON(type != TRB_COMPLETION ||
+ TRB_TO_SLOT_ID(le32_to_cpu(event->event_cmd.flags))
!= udev->slot_id || GET_COMP_CODE(le32_to_cpu(
event->event_cmd.status)) != COMP_SUCCESS);
xhci_acknowledge_event(ctrl);
diff --git a/include/usb/xhci.h b/include/usb/xhci.h
index 4a4ac10229ac..04d16a256bbd 100644
--- a/include/usb/xhci.h
+++ b/include/usb/xhci.h
@@ -901,6 +901,8 @@ union xhci_trb {
/* TRB type IDs */
typedef enum {
+ /* reserved, used as a software sentinel */
+ TRB_NONE = 0,
/* bulk, interrupt, isoc scatter/gather, and control data stage */
TRB_NORMAL = 1,
/* setup stage for control transfers */
--
2.41.0
[PATCH v2 1/8] usb: xhci: Guard all calls to xhci_wait_for_event
xhci_wait_for_event returns NULL on timeout, so the caller always has to
check for that. This addresses immediate explosions in this part
of the code when timeouts happen, but not the root cause for the
timeout.
Signed-off-by: Hector Martin
---
drivers/usb/host/xhci-ring.c | 15 +++
drivers/usb/host/xhci.c | 9 +
2 files changed, 24 insertions(+)
diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
index c8260cbdf94b..d0960812a47b 100644
--- a/drivers/usb/host/xhci-ring.c
+++ b/drivers/usb/host/xhci-ring.c
@@ -511,6 +511,9 @@ static void reset_ep(struct usb_device *udev, int ep_index)
printf("Resetting EP %d...\n", ep_index);
xhci_queue_command(ctrl, 0, udev->slot_id, ep_index, TRB_RESET_EP);
event = xhci_wait_for_event(ctrl, TRB_COMPLETION);
+ if (!event)
+ return;
+
field = le32_to_cpu(event->trans_event.flags);
BUG_ON(TRB_TO_SLOT_ID(field) != udev->slot_id);
xhci_acknowledge_event(ctrl);
@@ -519,6 +522,9 @@ static void reset_ep(struct usb_device *udev, int ep_index)
(void *)((uintptr_t)ring->enqueue | ring->cycle_state));
xhci_queue_command(ctrl, addr, udev->slot_id, ep_index, TRB_SET_DEQ);
event = xhci_wait_for_event(ctrl, TRB_COMPLETION);
+ if (!event)
+ return;
+
BUG_ON(TRB_TO_SLOT_ID(le32_to_cpu(event->event_cmd.flags))
!= udev->slot_id || GET_COMP_CODE(le32_to_cpu(
event->event_cmd.status)) != COMP_SUCCESS);
@@ -544,6 +550,9 @@ static void abort_td(struct usb_device *udev, int ep_index)
xhci_queue_command(ctrl, 0, udev->slot_id, ep_index, TRB_STOP_RING);
event = xhci_wait_for_event(ctrl, TRB_TRANSFER);
+ if (!event)
+ return;
+
field = le32_to_cpu(event->trans_event.flags);
BUG_ON(TRB_TO_SLOT_ID(field) != udev->slot_id);
BUG_ON(TRB_TO_EP_INDEX(field) != ep_index);
@@ -552,6 +561,9 @@ static void abort_td(struct usb_device *udev, int ep_index)
xhci_acknowledge_event(ctrl);
event = xhci_wait_for_event(ctrl, TRB_COMPLETION);
+ if (!event)
+ return;
+
BUG_ON(TRB_TO_SLOT_ID(le32_to_cpu(event->event_cmd.flags))
!= udev->slot_id || GET_COMP_CODE(le32_to_cpu(
event->event_cmd.status)) != COMP_SUCCESS);
@@ -561,6 +573,9 @@ static void abort_td(struct usb_device *udev, int ep_index)
(void *)((uintptr_t)ring->enqueue | ring->cycle_state));
xhci_queue_command(ctrl, addr, udev->slot_id, ep_index, TRB_SET_DEQ);
event = xhci_wait_for_event(ctrl, TRB_COMPLETION);
+ if (!event)
+ return;
+
BUG_ON(TRB_TO_SLOT_ID(le32_to_cpu(event->event_cmd.flags))
!= udev->slot_id || GET_COMP_CODE(le32_to_cpu(
event->event_cmd.status)) != COMP_SUCCESS);
diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
index 5cacf0769ec7..d13cbff9b372 100644
--- a/drivers/usb/host/xhci.c
+++ b/drivers/usb/host/xhci.c
@@ -451,6 +451,9 @@ static int xhci_configure_endpoints(struct usb_device
*udev, bool ctx_change)
xhci_queue_command(ctrl, in_ctx->dma, udev->slot_id, 0,
ctx_change ? TRB_EVAL_CONTEXT : TRB_CONFIG_EP);
event = xhci_wait_for_event(ctrl, TRB_COMPLETION);
+ if (!event)
+ return -ETIMEDOUT;
+
BUG_ON(TRB_TO_SLOT_ID(le32_to_cpu(event->event_cmd.flags))
!= udev->slot_id);
@@ -647,6 +650,9 @@ static int xhci_address_device(struct usb_device *udev, int
root_portnr)
xhci_queue_command(ctrl, virt_dev->in_ctx->dma,
slot_id, 0, TRB_ADDR_DEV);
event = xhci_wait_for_event(ctrl, TRB_COMPLETION);
+ if (!event)
+ return -ETIMEDOUT;
+
BUG_ON(TRB_TO_SLOT_ID(le32_to_cpu(event->event_cmd.flags)) != slot_id);
switch (GET_COMP_CODE(le32_to_cpu(event->event_cmd.status))) {
@@ -722,6 +728,9 @@ static int _xhci_alloc_device(struct usb_device *udev)
xhci_queue_command(ctrl, 0, 0, 0, TRB_ENABLE_SLOT);
event = xhci_wait_for_event(ctrl, TRB_COMPLETION);
+ if (!event)
+ return -ETIMEDOUT;
+
BUG_ON(GET_COMP_CODE(le32_to_cpu(event->event_cmd.status))
!= COMP_SUCCESS);
--
2.41.0
[PATCH v2 0/8] USB fixes: xHCI error handling
This series is the first of a few bundles of USB fixes we have been carrying downstream on the Asahi U-Boot branch for a few months. Patches #1-#6 fix a series of related robustness issues. Certain conditions related to endpoint stalls revealed a chain of bugs throughout the stack that caused U-Boot to completely fail when encountering some errors (and errors are a fact of life with USB). Example scenario: - The device stalls a bulk endpoint due to an error - The upper layer driver tries to use the endpoint again - There is no endpoint stall clear wired up in the API, so for starters this is doomed to fail (fix: #4) - xHCI knows the endpoint is halted, but tries to queue the TRB anyway, which can't work (fix: #5) - Since the endpoint is halted nothing happens, so the transfer times out. - The timeout handling tries to abort the transfer - abort_td() tries to stop the endpoint, but since it is already halted, this results in a context state error. As the transfer never started, there is no completion event, so xhci_wait_for_event() is waiting for the wrong event type, and logs an error and returns NULL. (fix: #2) - abort_td() crashes due to failing to guard against the NULL event (fix: #1) - Even after fixing all that, abort_td() would not handle the context state error properly and BUG() (fix: #3). This also fixes a race condition documented in the xHCI spec that could occur even in the absence of all the other bugs. Patch #6 addresses a related robustness issue where xhci_wait_for_event() panics on event timeouts other than for transfers. While this is clearly an unexpected condition and indicates a bug elsewhere, it's no reason to outright crash. USB is notoriously impossible to get 100% right, and we can't afford to be breaking users' systems at any sign of trouble. Error conditions should always be dealt with as gracefully as possible (even if that results in a completely broken USB controller, that is much preferable to aborting the boot process entirely, especially on devices with non-USB storage and keyboards where USB support is effectively optional for most users). Since after patch #1 we now guard all callers to xhci_wait_for_event() with at least trivial NULL checks, it's okay to return and continue in case of timeouts. Patch #7 addresses an unrelated bug I ran into while debugging all this, and patch #8 adds extra debug logs to make finding future issues less painful. I believe this should fix this Fedora bug too, which is either an instance of the above sequence of events, or (more likely, given the difficulty reproducing) the race condition documented in xHCI 4.6.9: https://bugzilla.redhat.com/show_bug.cgi?id=2244305 Signed-off-by: Hector Martin --- Changes in v2: - Squashed in a trivial fix for patch #1 - Removed spurious blank line - Added a longer description to the cover letter - Link to v1: https://lore.kernel.org/r/20231027-usb-fixes-1-v1-0-1c879bbcd...@marcan.st --- Hector Martin (8): usb: xhci: Guard all calls to xhci_wait_for_event usb: xhci: Better error handling in abort_td() usb: xhci: Allow context state errors when halting an endpoint usb: xhci: Recover from halted bulk endpoints usb: xhci: Fail on attempt to queue TRBs to a halted endpoint usb: xhci: Do not panic on event timeouts usb: xhci: Fix DMA address calculation in queue_trb usb: xhci: Add more debugging drivers/usb/host/xhci-ring.c | 99 drivers/usb/host/xhci.c | 9 include/usb/xhci.h | 2 + 3 files changed, 92 insertions(+), 18 deletions(-) --- base-commit: 7c0d668103abae3ec14cd96d882ba20134bb4529 change-id: 20231027-usb-fixes-1-83bfc7013012 Best regards, -- Hector Martin
