| Message ID | 20200702084733.2032531-4-sr@denx.de |
|---|---|
| State | Superseded |
| Delegated to: | Marek Vasut |
| Headers | show |
| Series | [v1,1/4] usb: xhci: Add missing endian conversions (cpu_to_leXX / leXX_to_cpu) | expand |
Hi Stefan, On Thu, Jul 2, 2020 at 4:47 PM Stefan Roese <sr@denx.de> wrote: > > These values are already swapped to CPU endianess, so swapping them Can you please add more details as to when these values are swapped? I assume this is inside usb_select_config() which is called before this function is called? But I wonder how this code ever worked on ARM/x86? > again is a bug. Let's remove the swap here instead. > > Signed-off-by: Stefan Roese <sr@denx.de> > Cc: Bin Meng <bmeng.cn@gmail.com> > Cc: Marek Vasut <marex@denx.de> > --- > > drivers/usb/host/usb-uclass.c | 8 ++++---- > 1 file changed, 4 insertions(+), 4 deletions(-) > > diff --git a/drivers/usb/host/usb-uclass.c b/drivers/usb/host/usb-uclass.c > index cb79dfbbd5..aa08d4ffc6 100644 > --- a/drivers/usb/host/usb-uclass.c > +++ b/drivers/usb/host/usb-uclass.c > @@ -416,21 +416,21 @@ static int usb_match_device(const struct usb_device_descriptor *desc, > const struct usb_device_id *id) > { > if ((id->match_flags & USB_DEVICE_ID_MATCH_VENDOR) && > - id->idVendor != le16_to_cpu(desc->idVendor)) > + id->idVendor != desc->idVendor) > return 0; > > if ((id->match_flags & USB_DEVICE_ID_MATCH_PRODUCT) && > - id->idProduct != le16_to_cpu(desc->idProduct)) > + id->idProduct != desc->idProduct) > return 0; > > /* No need to test id->bcdDevice_lo != 0, since 0 is never > greater than any unsigned number. */ > if ((id->match_flags & USB_DEVICE_ID_MATCH_DEV_LO) && > - (id->bcdDevice_lo > le16_to_cpu(desc->bcdDevice))) > + (id->bcdDevice_lo > desc->bcdDevice)) > return 0; > > if ((id->match_flags & USB_DEVICE_ID_MATCH_DEV_HI) && > - (id->bcdDevice_hi < le16_to_cpu(desc->bcdDevice))) > + (id->bcdDevice_hi < desc->bcdDevice)) > return 0; > > if ((id->match_flags & USB_DEVICE_ID_MATCH_DEV_CLASS) && Reviewed-by: Bin Meng <bmeng.cn@gmail.com> Regards, Bin
On 17.07.20 07:33, Bin Meng wrote: > Hi Stefan, > > On Thu, Jul 2, 2020 at 4:47 PM Stefan Roese <sr@denx.de> wrote: >> >> These values are already swapped to CPU endianess, so swapping them > > Can you please add more details as to when these values are swapped? I > assume this is inside usb_select_config() which is called before this > function is called? Yes. Its swapped exactly here: int usb_select_config(struct usb_device *dev) { unsigned char *tmpbuf = NULL; int err; err = get_descriptor_len(dev, USB_DT_DEVICE_SIZE, USB_DT_DEVICE_SIZE); if (err) return err; /* correct le values */ le16_to_cpus(&dev->descriptor.bcdUSB); le16_to_cpus(&dev->descriptor.idVendor); le16_to_cpus(&dev->descriptor.idProduct); le16_to_cpus(&dev->descriptor.bcdDevice); > But I wonder how this code ever worked on ARM/x86? On ARM/x86, the little-endian swapping is a no-op. So it doesn't matter if you swap once or twice or... ;) >> again is a bug. Let's remove the swap here instead. >> >> Signed-off-by: Stefan Roese <sr@denx.de> >> Cc: Bin Meng <bmeng.cn@gmail.com> >> Cc: Marek Vasut <marex@denx.de> >> --- >> >> drivers/usb/host/usb-uclass.c | 8 ++++---- >> 1 file changed, 4 insertions(+), 4 deletions(-) >> >> diff --git a/drivers/usb/host/usb-uclass.c b/drivers/usb/host/usb-uclass.c >> index cb79dfbbd5..aa08d4ffc6 100644 >> --- a/drivers/usb/host/usb-uclass.c >> +++ b/drivers/usb/host/usb-uclass.c >> @@ -416,21 +416,21 @@ static int usb_match_device(const struct usb_device_descriptor *desc, >> const struct usb_device_id *id) >> { >> if ((id->match_flags & USB_DEVICE_ID_MATCH_VENDOR) && >> - id->idVendor != le16_to_cpu(desc->idVendor)) >> + id->idVendor != desc->idVendor) >> return 0; >> >> if ((id->match_flags & USB_DEVICE_ID_MATCH_PRODUCT) && >> - id->idProduct != le16_to_cpu(desc->idProduct)) >> + id->idProduct != desc->idProduct) >> return 0; >> >> /* No need to test id->bcdDevice_lo != 0, since 0 is never >> greater than any unsigned number. */ >> if ((id->match_flags & USB_DEVICE_ID_MATCH_DEV_LO) && >> - (id->bcdDevice_lo > le16_to_cpu(desc->bcdDevice))) >> + (id->bcdDevice_lo > desc->bcdDevice)) >> return 0; >> >> if ((id->match_flags & USB_DEVICE_ID_MATCH_DEV_HI) && >> - (id->bcdDevice_hi < le16_to_cpu(desc->bcdDevice))) >> + (id->bcdDevice_hi < desc->bcdDevice)) >> return 0; >> >> if ((id->match_flags & USB_DEVICE_ID_MATCH_DEV_CLASS) && > > Reviewed-by: Bin Meng <bmeng.cn@gmail.com> > > Regards, > Bin Thanks, Stefan
Hi Stefan, On Fri, Jul 17, 2020 at 6:08 PM Stefan Roese <sr@denx.de> wrote: > > On 17.07.20 07:33, Bin Meng wrote: > > Hi Stefan, > > > > On Thu, Jul 2, 2020 at 4:47 PM Stefan Roese <sr@denx.de> wrote: > >> > >> These values are already swapped to CPU endianess, so swapping them > > > > Can you please add more details as to when these values are swapped? I > > assume this is inside usb_select_config() which is called before this > > function is called? > > Yes. Its swapped exactly here: > > int usb_select_config(struct usb_device *dev) > { > unsigned char *tmpbuf = NULL; > int err; > > err = get_descriptor_len(dev, USB_DT_DEVICE_SIZE, USB_DT_DEVICE_SIZE); > if (err) > return err; > > /* correct le values */ > le16_to_cpus(&dev->descriptor.bcdUSB); > le16_to_cpus(&dev->descriptor.idVendor); > le16_to_cpus(&dev->descriptor.idProduct); > le16_to_cpus(&dev->descriptor.bcdDevice); > > > But I wonder how this code ever worked on ARM/x86? > > On ARM/x86, the little-endian swapping is a no-op. So it doesn't matter > if you swap once or twice or... ;) > Ah, yes! I misread this as an unconditional swap ... > >> again is a bug. Let's remove the swap here instead. > >> > >> Signed-off-by: Stefan Roese <sr@denx.de> > >> Cc: Bin Meng <bmeng.cn@gmail.com> > >> Cc: Marek Vasut <marex@denx.de> > >> --- > >> > >> drivers/usb/host/usb-uclass.c | 8 ++++---- > >> 1 file changed, 4 insertions(+), 4 deletions(-) > >> Regards, Bin
On Thu, Jul 2, 2020 at 4:47 PM Stefan Roese <sr@denx.de> wrote: > > These values are already swapped to CPU endianess, so swapping them > again is a bug. Let's remove the swap here instead. > > Signed-off-by: Stefan Roese <sr@denx.de> > Cc: Bin Meng <bmeng.cn@gmail.com> > Cc: Marek Vasut <marex@denx.de> > --- > > drivers/usb/host/usb-uclass.c | 8 ++++---- > 1 file changed, 4 insertions(+), 4 deletions(-) > Tested-by: Bin Meng <bmeng.cn@gmail.com>
diff --git a/drivers/usb/host/usb-uclass.c b/drivers/usb/host/usb-uclass.c index cb79dfbbd5..aa08d4ffc6 100644 --- a/drivers/usb/host/usb-uclass.c +++ b/drivers/usb/host/usb-uclass.c @@ -416,21 +416,21 @@ static int usb_match_device(const struct usb_device_descriptor *desc, const struct usb_device_id *id) { if ((id->match_flags & USB_DEVICE_ID_MATCH_VENDOR) && - id->idVendor != le16_to_cpu(desc->idVendor)) + id->idVendor != desc->idVendor) return 0; if ((id->match_flags & USB_DEVICE_ID_MATCH_PRODUCT) && - id->idProduct != le16_to_cpu(desc->idProduct)) + id->idProduct != desc->idProduct) return 0; /* No need to test id->bcdDevice_lo != 0, since 0 is never greater than any unsigned number. */ if ((id->match_flags & USB_DEVICE_ID_MATCH_DEV_LO) && - (id->bcdDevice_lo > le16_to_cpu(desc->bcdDevice))) + (id->bcdDevice_lo > desc->bcdDevice)) return 0; if ((id->match_flags & USB_DEVICE_ID_MATCH_DEV_HI) && - (id->bcdDevice_hi < le16_to_cpu(desc->bcdDevice))) + (id->bcdDevice_hi < desc->bcdDevice)) return 0; if ((id->match_flags & USB_DEVICE_ID_MATCH_DEV_CLASS) &&
These values are already swapped to CPU endianess, so swapping them again is a bug. Let's remove the swap here instead. Signed-off-by: Stefan Roese <sr@denx.de> Cc: Bin Meng <bmeng.cn@gmail.com> Cc: Marek Vasut <marex@denx.de> --- drivers/usb/host/usb-uclass.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)