| Message ID | 1498211673-18715-4-git-send-email-bmeng.cn@gmail.com |
|---|---|
| State | Superseded |
| Delegated to: | Marek Vasut |
| Headers | show |
Hi, On Fri, Jun 23, 2017 at 5:54 PM, Bin Meng <bmeng.cn@gmail.com> wrote: > The only work we need do in usb_setup_descriptor() is to initialize > dev->descriptor.bMaxPacketSize0, when do_read is false. Other steps > are the same as do_read being true. > > While we are here, update the comment block to be within 80 cols. > > Signed-off-by: Bin Meng <bmeng.cn@gmail.com> > --- > > common/usb.c | 40 ++++++++++++++++++---------------------- > 1 file changed, 18 insertions(+), 22 deletions(-) > > diff --git a/common/usb.c b/common/usb.c > index 15e1e4c..293d968 100644 > --- a/common/usb.c > +++ b/common/usb.c > @@ -962,37 +962,33 @@ static int usb_setup_descriptor(struct usb_device *dev, bool do_read) > * some more, or keeps on retransmitting the 8 byte header. > */ > > - if (dev->speed == USB_SPEED_LOW) { > - dev->descriptor.bMaxPacketSize0 = 8; > - dev->maxpacketsize = PACKET_SIZE_8; > - } else { > - dev->descriptor.bMaxPacketSize0 = 64; > - dev->maxpacketsize = PACKET_SIZE_64; > - } > - dev->epmaxpacketin[0] = dev->descriptor.bMaxPacketSize0; > - dev->epmaxpacketout[0] = dev->descriptor.bMaxPacketSize0; > - > if (do_read) { > int err; > > /* > - * Validate we've received only at least 8 bytes, not that we've > - * received the entire descriptor. The reasoning is: > - * - The code only uses fields in the first 8 bytes, so that's all we > - * need to have fetched at this stage. > - * - The smallest maxpacket size is 8 bytes. Before we know the actual > - * maxpacket the device uses, the USB controller may only accept a > - * single packet. Consequently we are only guaranteed to receive 1 > - * packet (at least 8 bytes) even in a non-error case. > + * Validate we've received only at least 8 bytes, not that > + * we've received the entire descriptor. The reasoning is: > + * - The code only uses fields in the first 8 bytes, so that's > + * all we need to have fetched at this stage. > + * - The smallest maxpacket size is 8 bytes. Before we know > + * the actual maxpacket the device uses, the USB controller > + * may only accept a single packet. Consequently we are only > + * guaranteed to receive 1 packet (at least 8 bytes) even in > + * a non-error case. > * > - * At least the DWC2 controller needs to be programmed with the number > - * of packets in addition to the number of bytes. A request for 64 > - * bytes of data with the maxpacket guessed as 64 (above) yields a > - * request for 1 packet. > + * At least the DWC2 controller needs to be programmed with > + * the number of packets in addition to the number of bytes. > + * A request for 64 bytes of data with the maxpacket guessed > + * as 64 (above) yields a request for 1 packet. > */ > err = get_descriptor_len(dev, 64, 8); > if (err) > return err; > + } else { > + if (dev->speed == USB_SPEED_LOW) > + dev->descriptor.bMaxPacketSize0 = 8; > + else > + dev->descriptor.bMaxPacketSize0 = 64; > } > > dev->epmaxpacketin[0] = dev->descriptor.bMaxPacketSize0; > -- For some reason that I don't understand, this patch breaks EHCI. dev->maxpacketsize is equal to zero with this patch, which leads to a 'divide error' exception in ehci_submit_async(). Not sure if anyone has some hints? For now, I will just drop this patch in v2. This needs be further revisited. Regards, Bin
Hi, On Fri, 30 Jun 2017 11:49:56 +0800 Bin Meng wrote: > Hi, > > On Fri, Jun 23, 2017 at 5:54 PM, Bin Meng <bmeng.cn@gmail.com> wrote: > > The only work we need do in usb_setup_descriptor() is to initialize > > dev->descriptor.bMaxPacketSize0, when do_read is false. Other steps > > are the same as do_read being true. > > > > While we are here, update the comment block to be within 80 cols. > > > > Signed-off-by: Bin Meng <bmeng.cn@gmail.com> > > --- > > > > common/usb.c | 40 ++++++++++++++++++---------------------- > > 1 file changed, 18 insertions(+), 22 deletions(-) > > > > diff --git a/common/usb.c b/common/usb.c > > index 15e1e4c..293d968 100644 > > --- a/common/usb.c > > +++ b/common/usb.c > > @@ -962,37 +962,33 @@ static int usb_setup_descriptor(struct usb_device *dev, bool do_read) > > * some more, or keeps on retransmitting the 8 byte header. > > */ > > > > - if (dev->speed == USB_SPEED_LOW) { > > - dev->descriptor.bMaxPacketSize0 = 8; > > - dev->maxpacketsize = PACKET_SIZE_8; > > - } else { > > - dev->descriptor.bMaxPacketSize0 = 64; > > - dev->maxpacketsize = PACKET_SIZE_64; > > - } > > - dev->epmaxpacketin[0] = dev->descriptor.bMaxPacketSize0; > > - dev->epmaxpacketout[0] = dev->descriptor.bMaxPacketSize0; > > - > > if (do_read) { > > int err; > > > > /* > > - * Validate we've received only at least 8 bytes, not that we've > > - * received the entire descriptor. The reasoning is: > > - * - The code only uses fields in the first 8 bytes, so that's all we > > - * need to have fetched at this stage. > > - * - The smallest maxpacket size is 8 bytes. Before we know the actual > > - * maxpacket the device uses, the USB controller may only accept a > > - * single packet. Consequently we are only guaranteed to receive 1 > > - * packet (at least 8 bytes) even in a non-error case. > > + * Validate we've received only at least 8 bytes, not that > > + * we've received the entire descriptor. The reasoning is: > > + * - The code only uses fields in the first 8 bytes, so that's > > + * all we need to have fetched at this stage. > > + * - The smallest maxpacket size is 8 bytes. Before we know > > + * the actual maxpacket the device uses, the USB controller > > + * may only accept a single packet. Consequently we are only > > + * guaranteed to receive 1 packet (at least 8 bytes) even in > > + * a non-error case. > > * > > - * At least the DWC2 controller needs to be programmed with the number > > - * of packets in addition to the number of bytes. A request for 64 > > - * bytes of data with the maxpacket guessed as 64 (above) yields a > > - * request for 1 packet. > > + * At least the DWC2 controller needs to be programmed with > > + * the number of packets in addition to the number of bytes. > > + * A request for 64 bytes of data with the maxpacket guessed > > + * as 64 (above) yields a request for 1 packet. > > */ > > err = get_descriptor_len(dev, 64, 8); > > if (err) > > return err; > > + } else { > > + if (dev->speed == USB_SPEED_LOW) > > + dev->descriptor.bMaxPacketSize0 = 8; > > + else > > + dev->descriptor.bMaxPacketSize0 = 64; > > } > > > > dev->epmaxpacketin[0] = dev->descriptor.bMaxPacketSize0; > > -- > > For some reason that I don't understand, this patch breaks EHCI. > dev->maxpacketsize is equal to zero with this patch, which leads to a > 'divide error' exception in ehci_submit_async(). Not sure if anyone > has some hints? > In the do_read case the dev->descriptor.bMaxPacketSize0 is not set up. Lothar Waßmann
Hi Lothar, On Fri, Jun 30, 2017 at 2:15 PM, Lothar Waßmann <LW@karo-electronics.de> wrote: > Hi, > > On Fri, 30 Jun 2017 11:49:56 +0800 Bin Meng wrote: >> Hi, >> >> On Fri, Jun 23, 2017 at 5:54 PM, Bin Meng <bmeng.cn@gmail.com> wrote: >> > The only work we need do in usb_setup_descriptor() is to initialize >> > dev->descriptor.bMaxPacketSize0, when do_read is false. Other steps >> > are the same as do_read being true. >> > >> > While we are here, update the comment block to be within 80 cols. >> > >> > Signed-off-by: Bin Meng <bmeng.cn@gmail.com> >> > --- >> > >> > common/usb.c | 40 ++++++++++++++++++---------------------- >> > 1 file changed, 18 insertions(+), 22 deletions(-) >> > >> > diff --git a/common/usb.c b/common/usb.c >> > index 15e1e4c..293d968 100644 >> > --- a/common/usb.c >> > +++ b/common/usb.c >> > @@ -962,37 +962,33 @@ static int usb_setup_descriptor(struct usb_device *dev, bool do_read) >> > * some more, or keeps on retransmitting the 8 byte header. >> > */ >> > >> > - if (dev->speed == USB_SPEED_LOW) { >> > - dev->descriptor.bMaxPacketSize0 = 8; >> > - dev->maxpacketsize = PACKET_SIZE_8; >> > - } else { >> > - dev->descriptor.bMaxPacketSize0 = 64; >> > - dev->maxpacketsize = PACKET_SIZE_64; >> > - } >> > - dev->epmaxpacketin[0] = dev->descriptor.bMaxPacketSize0; >> > - dev->epmaxpacketout[0] = dev->descriptor.bMaxPacketSize0; >> > - >> > if (do_read) { >> > int err; >> > >> > /* >> > - * Validate we've received only at least 8 bytes, not that we've >> > - * received the entire descriptor. The reasoning is: >> > - * - The code only uses fields in the first 8 bytes, so that's all we >> > - * need to have fetched at this stage. >> > - * - The smallest maxpacket size is 8 bytes. Before we know the actual >> > - * maxpacket the device uses, the USB controller may only accept a >> > - * single packet. Consequently we are only guaranteed to receive 1 >> > - * packet (at least 8 bytes) even in a non-error case. >> > + * Validate we've received only at least 8 bytes, not that >> > + * we've received the entire descriptor. The reasoning is: >> > + * - The code only uses fields in the first 8 bytes, so that's >> > + * all we need to have fetched at this stage. >> > + * - The smallest maxpacket size is 8 bytes. Before we know >> > + * the actual maxpacket the device uses, the USB controller >> > + * may only accept a single packet. Consequently we are only >> > + * guaranteed to receive 1 packet (at least 8 bytes) even in >> > + * a non-error case. >> > * >> > - * At least the DWC2 controller needs to be programmed with the number >> > - * of packets in addition to the number of bytes. A request for 64 >> > - * bytes of data with the maxpacket guessed as 64 (above) yields a >> > - * request for 1 packet. >> > + * At least the DWC2 controller needs to be programmed with >> > + * the number of packets in addition to the number of bytes. >> > + * A request for 64 bytes of data with the maxpacket guessed >> > + * as 64 (above) yields a request for 1 packet. >> > */ >> > err = get_descriptor_len(dev, 64, 8); >> > if (err) >> > return err; >> > + } else { >> > + if (dev->speed == USB_SPEED_LOW) >> > + dev->descriptor.bMaxPacketSize0 = 8; >> > + else >> > + dev->descriptor.bMaxPacketSize0 = 64; >> > } >> > >> > dev->epmaxpacketin[0] = dev->descriptor.bMaxPacketSize0; >> > -- >> >> For some reason that I don't understand, this patch breaks EHCI. >> dev->maxpacketsize is equal to zero with this patch, which leads to a >> 'divide error' exception in ehci_submit_async(). Not sure if anyone >> has some hints? >> > In the do_read case the dev->descriptor.bMaxPacketSize0 is not set up. > In that case, dev->descriptor.bMaxPacketSize0 was read from device. Am I missing anything? Regards, Bin
On Freitag, 30. Juni 2017 05:49:56 CEST Bin Meng wrote: > Hi, > > On Fri, Jun 23, 2017 at 5:54 PM, Bin Meng <bmeng.cn@gmail.com> wrote: > > The only work we need do in usb_setup_descriptor() is to initialize > > dev->descriptor.bMaxPacketSize0, when do_read is false. Other steps > > are the same as do_read being true. > > > > While we are here, update the comment block to be within 80 cols. > > > > Signed-off-by: Bin Meng <bmeng.cn@gmail.com> > > --- > > > > common/usb.c | 40 ++++++++++++++++++---------------------- > > 1 file changed, 18 insertions(+), 22 deletions(-) > > > > diff --git a/common/usb.c b/common/usb.c > > index 15e1e4c..293d968 100644 > > --- a/common/usb.c > > +++ b/common/usb.c > > @@ -962,37 +962,33 @@ static int usb_setup_descriptor(struct usb_device > > *dev, bool do_read)> > > * some more, or keeps on retransmitting the 8 byte header. > > */ > > > > - if (dev->speed == USB_SPEED_LOW) { > > - dev->descriptor.bMaxPacketSize0 = 8; > > - dev->maxpacketsize = PACKET_SIZE_8; > > - } else { > > - dev->descriptor.bMaxPacketSize0 = 64; > > - dev->maxpacketsize = PACKET_SIZE_64; > > - } > > - dev->epmaxpacketin[0] = dev->descriptor.bMaxPacketSize0; > > - dev->epmaxpacketout[0] = dev->descriptor.bMaxPacketSize0; > > - You remove the initialization of dev->maxpacketsize here ... > > > > if (do_read) { > > [...] > > > > */ > > > > err = get_descriptor_len(dev, 64, 8); > > if (err) > > > > return err; ... and probably return early here. Can you add some debug output to get_descriptor_len(...) (len, expected len, return code)? > > > > + } else { > > + if (dev->speed == USB_SPEED_LOW) > > + dev->descriptor.bMaxPacketSize0 = 8; > > + else > > + dev->descriptor.bMaxPacketSize0 = 64; > > > > } > > > > dev->epmaxpacketin[0] = dev->descriptor.bMaxPacketSize0; > > > > -- > > For some reason that I don't understand, this patch breaks EHCI. > dev->maxpacketsize is equal to zero with this patch, which leads to a > 'divide error' exception in ehci_submit_async(). Not sure if anyone > has some hints? > > For now, I will just drop this patch in v2. This needs be further revisited. Kind regards, Stefan
Hi Stefan, On Sun, Jul 2, 2017 at 1:29 AM, <stefan.bruens@rwth-aachen.de> wrote: > On Freitag, 30. Juni 2017 05:49:56 CEST Bin Meng wrote: >> Hi, >> >> On Fri, Jun 23, 2017 at 5:54 PM, Bin Meng <bmeng.cn@gmail.com> wrote: >> > The only work we need do in usb_setup_descriptor() is to initialize >> > dev->descriptor.bMaxPacketSize0, when do_read is false. Other steps >> > are the same as do_read being true. >> > >> > While we are here, update the comment block to be within 80 cols. >> > >> > Signed-off-by: Bin Meng <bmeng.cn@gmail.com> >> > --- >> > >> > common/usb.c | 40 ++++++++++++++++++---------------------- >> > 1 file changed, 18 insertions(+), 22 deletions(-) >> > >> > diff --git a/common/usb.c b/common/usb.c >> > index 15e1e4c..293d968 100644 >> > --- a/common/usb.c >> > +++ b/common/usb.c >> > @@ -962,37 +962,33 @@ static int usb_setup_descriptor(struct usb_device >> > *dev, bool do_read)> >> > * some more, or keeps on retransmitting the 8 byte header. >> > */ >> > >> > - if (dev->speed == USB_SPEED_LOW) { >> > - dev->descriptor.bMaxPacketSize0 = 8; >> > - dev->maxpacketsize = PACKET_SIZE_8; >> > - } else { >> > - dev->descriptor.bMaxPacketSize0 = 64; >> > - dev->maxpacketsize = PACKET_SIZE_64; >> > - } >> > - dev->epmaxpacketin[0] = dev->descriptor.bMaxPacketSize0; >> > - dev->epmaxpacketout[0] = dev->descriptor.bMaxPacketSize0; >> > - > > You remove the initialization of dev->maxpacketsize here ... > >> > >> > if (do_read) { >> > > [...] >> > >> > */ >> > >> > err = get_descriptor_len(dev, 64, 8); >> > if (err) >> > >> > return err; > > ... and probably return early here. Can you add some debug output to > get_descriptor_len(...) (len, expected len, return code)? > The get_descriptor_len() was successful, so it is not caused by the "if (err)" branch. >> > >> > + } else { >> > + if (dev->speed == USB_SPEED_LOW) >> > + dev->descriptor.bMaxPacketSize0 = 8; >> > + else >> > + dev->descriptor.bMaxPacketSize0 = 64; >> > >> > } >> > >> > dev->epmaxpacketin[0] = dev->descriptor.bMaxPacketSize0; >> > >> > -- >> >> For some reason that I don't understand, this patch breaks EHCI. >> dev->maxpacketsize is equal to zero with this patch, which leads to a >> 'divide error' exception in ehci_submit_async(). Not sure if anyone >> has some hints? >> >> For now, I will just drop this patch in v2. This needs be further revisited. > Regards, Bin
diff --git a/common/usb.c b/common/usb.c index 15e1e4c..293d968 100644 --- a/common/usb.c +++ b/common/usb.c @@ -962,37 +962,33 @@ static int usb_setup_descriptor(struct usb_device *dev, bool do_read) * some more, or keeps on retransmitting the 8 byte header. */ - if (dev->speed == USB_SPEED_LOW) { - dev->descriptor.bMaxPacketSize0 = 8; - dev->maxpacketsize = PACKET_SIZE_8; - } else { - dev->descriptor.bMaxPacketSize0 = 64; - dev->maxpacketsize = PACKET_SIZE_64; - } - dev->epmaxpacketin[0] = dev->descriptor.bMaxPacketSize0; - dev->epmaxpacketout[0] = dev->descriptor.bMaxPacketSize0; - if (do_read) { int err; /* - * Validate we've received only at least 8 bytes, not that we've - * received the entire descriptor. The reasoning is: - * - The code only uses fields in the first 8 bytes, so that's all we - * need to have fetched at this stage. - * - The smallest maxpacket size is 8 bytes. Before we know the actual - * maxpacket the device uses, the USB controller may only accept a - * single packet. Consequently we are only guaranteed to receive 1 - * packet (at least 8 bytes) even in a non-error case. + * Validate we've received only at least 8 bytes, not that + * we've received the entire descriptor. The reasoning is: + * - The code only uses fields in the first 8 bytes, so that's + * all we need to have fetched at this stage. + * - The smallest maxpacket size is 8 bytes. Before we know + * the actual maxpacket the device uses, the USB controller + * may only accept a single packet. Consequently we are only + * guaranteed to receive 1 packet (at least 8 bytes) even in + * a non-error case. * - * At least the DWC2 controller needs to be programmed with the number - * of packets in addition to the number of bytes. A request for 64 - * bytes of data with the maxpacket guessed as 64 (above) yields a - * request for 1 packet. + * At least the DWC2 controller needs to be programmed with + * the number of packets in addition to the number of bytes. + * A request for 64 bytes of data with the maxpacket guessed + * as 64 (above) yields a request for 1 packet. */ err = get_descriptor_len(dev, 64, 8); if (err) return err; + } else { + if (dev->speed == USB_SPEED_LOW) + dev->descriptor.bMaxPacketSize0 = 8; + else + dev->descriptor.bMaxPacketSize0 = 64; } dev->epmaxpacketin[0] = dev->descriptor.bMaxPacketSize0;
The only work we need do in usb_setup_descriptor() is to initialize dev->descriptor.bMaxPacketSize0, when do_read is false. Other steps are the same as do_read being true. While we are here, update the comment block to be within 80 cols. Signed-off-by: Bin Meng <bmeng.cn@gmail.com> --- common/usb.c | 40 ++++++++++++++++++---------------------- 1 file changed, 18 insertions(+), 22 deletions(-)