From: Holger Hans Peter Freyther zecke@selfish.org
This still needs to receive some more testing (untested on Windows right now) and also consideration how/if we want to upgrade this in the field. If we consider reflashing the DFU part safe enough. I would argue that there is still SAMBA to fix things up if someone breaks the bootloader.
Holger Hans Peter Freyther (1): usb: Do not send ZLP when we have filled the window
firmware/src/dfu/dfu.c | 46 ++++++++++++++++---------------- firmware/src/dfu/dfu.h | 2 +- firmware/src/os/pcd_enumerate.c | 54 +++++++++++++++++++++------------------ 3 files changed, 53 insertions(+), 49 deletions(-)
From: Holger Hans Peter Freyther zecke@selfish.org
Only send the ZLP if we send less data than was required/asked for by the host and it is a multiple of the bMaxPacketSize0 (which is hardcoded to 8 right now).
This is completing the change done in fe88b83e80df8be0351ff38ee6 to fix SIMtrace attached to OSX and not regress on windows.
Introduce another parameter to udp_ep0_send_data to specify the window size (wLength) or if not available the default from USB 2.0 specification. --- firmware/src/dfu/dfu.c | 46 ++++++++++++++++---------------- firmware/src/dfu/dfu.h | 2 +- firmware/src/os/pcd_enumerate.c | 54 +++++++++++++++++++++------------------ 3 files changed, 53 insertions(+), 49 deletions(-)
diff --git a/firmware/src/dfu/dfu.c b/firmware/src/dfu/dfu.c index 5d6865c..9d1ad10 100644 --- a/firmware/src/dfu/dfu.c +++ b/firmware/src/dfu/dfu.c @@ -96,13 +96,15 @@ static void __dfufunc udp_init(void) static void __dfufunc udp_ep0_send_zlp(void);
/* Send Data through the control endpoint */ -static void __dfufunc udp_ep0_send_data(const char *pData, u_int32_t length) +static void __dfufunc udp_ep0_send_data(const char *pData, u_int32_t length, + u_int32_t window_length) { AT91PS_UDP pUdp = AT91C_BASE_UDP; - u_int32_t cpt = 0, len_remain = length; + u_int32_t cpt = 0, len_remain; AT91_REG csr;
- DEBUGE("send_data: %u bytes ", length); + len_remain = MIN(length, window_length); + DEBUGE("send_data: %u/%u bytes ", length, window_length);
do { cpt = MIN(len_remain, 8); @@ -135,7 +137,7 @@ static void __dfufunc udp_ep0_send_data(const char *pData, u_int32_t length) while (pUdp->UDP_CSR[0] & AT91C_UDP_TXCOMP) ; }
- if ((length % 8) == 0) { + if ((length % 8) == 0 && length < window_length) { /* if the length is a multiple of the EP size, we need * to send another ZLP (zero-length packet) to tell the * host the transfer has completed. */ @@ -366,7 +368,7 @@ static __dfufunc int handle_upload(u_int16_t __unused val, u_int16_t len) first_download = 1; }
- udp_ep0_send_data((char *)ptr, len); + udp_ep0_send_data((char *)ptr, len, len); ptr+= len;
return len; @@ -409,7 +411,7 @@ static __dfufunc void handle_getstatus(void) dstat.iString = 0; /* FIXME: set dstat.bwPollTimeout */
- udp_ep0_send_data((char *)&dstat, sizeof(dstat)); + udp_ep0_send_data((char *)&dstat, sizeof(dstat), sizeof(dstat)); }
static void __dfufunc handle_getstate(void) @@ -417,7 +419,7 @@ static void __dfufunc handle_getstate(void) u_int8_t u8 = dfu_state; DEBUGE("getstate ");
- udp_ep0_send_data((char *)&u8, sizeof(u8)); + udp_ep0_send_data((char *)&u8, sizeof(u8), sizeof(u8)); }
/* callback function for DFU requests */ @@ -806,15 +808,13 @@ static __dfufunc void dfu_udp_ep0_handler(void) /* Return Device Descriptor */ udp_ep0_send_data((const char *) &dfu_dev_descriptor, - MIN(sizeof(dfu_dev_descriptor), - wLength)); + sizeof(dfu_dev_descriptor), wLength); break; case USB_DT_CONFIG: /* Return Configuration Descriptor */ udp_ep0_send_data((const char *) &dfu_cfg_descriptor, - MIN(sizeof(dfu_cfg_descriptor), - wLength)); + sizeof(dfu_cfg_descriptor), wLength); break; case USB_DT_STRING: /* Return String Descriptor */ @@ -825,14 +825,12 @@ static __dfufunc void dfu_udp_ep0_handler(void) DEBUGE("bLength=%u, wLength=%u ", usb_strings[desc_index]->bLength, wLength); udp_ep0_send_data((const char *) usb_strings[desc_index], - MIN(usb_strings[desc_index]->bLength, - wLength)); + usb_strings[desc_index]->bLength, wLength); break; case USB_DT_CS_DEVICE: /* Return Function descriptor */ udp_ep0_send_data((const char *) &dfu_cfg_descriptor.func_dfu, - MIN(sizeof(dfu_cfg_descriptor.func_dfu), - wLength)); + sizeof(dfu_cfg_descriptor.func_dfu), wLength); break; default: udp_ep0_send_stall(); @@ -867,18 +865,20 @@ static __dfufunc void dfu_udp_ep0_handler(void) break; case STD_GET_CONFIGURATION: DEBUGE("GET_CONFIG "); - udp_ep0_send_data((char *)&(cur_config), - sizeof(cur_config)); + /* Table 9.4 wLength One */ + udp_ep0_send_data((char *)&(cur_config), sizeof(cur_config), 1); break; case STD_GET_STATUS_ZERO: DEBUGE("GET_STATUS_ZERO "); wStatus = 0; - udp_ep0_send_data((char *)&wStatus, sizeof(wStatus)); + /* Table 9.4 wLength Two */ + udp_ep0_send_data((char *)&wStatus, sizeof(wStatus), 2); break; case STD_GET_STATUS_INTERFACE: DEBUGE("GET_STATUS_INTERFACE "); wStatus = 0; - udp_ep0_send_data((char *)&wStatus, sizeof(wStatus)); + /* Table 9.4 wLength Two */ + udp_ep0_send_data((char *)&wStatus, sizeof(wStatus), 2); break; case STD_GET_STATUS_ENDPOINT: DEBUGE("GET_STATUS_ENDPOINT(EPidx=%u) ", wIndex&0x0f); @@ -887,14 +887,14 @@ static __dfufunc void dfu_udp_ep0_handler(void) if ((pUDP->UDP_GLBSTATE & AT91C_UDP_CONFG) && (wIndex == 0)) { wStatus = (pUDP->UDP_CSR[wIndex] & AT91C_UDP_EPEDS) ? 0 : 1; - udp_ep0_send_data((char *)&wStatus, - sizeof(wStatus)); + /* Table 9.4 wLength Two */ + udp_ep0_send_data((char *)&wStatus, sizeof(wStatus), 2); } else if ((pUDP->UDP_GLBSTATE & AT91C_UDP_FADDEN) && (wIndex == 0)) { wStatus = (pUDP->UDP_CSR[wIndex] & AT91C_UDP_EPEDS) ? 0 : 1; - udp_ep0_send_data((char *)&wStatus, - sizeof(wStatus)); + /* Table 9.4 wLength Two */ + udp_ep0_send_data((char *)&wStatus, sizeof(wStatus), 2); } else udp_ep0_send_stall(); break; diff --git a/firmware/src/dfu/dfu.h b/firmware/src/dfu/dfu.h index c898197..27c63b5 100644 --- a/firmware/src/dfu/dfu.h +++ b/firmware/src/dfu/dfu.h @@ -124,7 +124,7 @@ struct _dfu_desc {
struct dfuapi { void (*udp_init)(void); - void (*ep0_send_data)(const char *data, u_int32_t len); + void (*ep0_send_data)(const char *data, u_int32_t len, u_int32_t wlen); void (*ep0_send_zlp)(void); void (*ep0_send_stall)(void); int (*dfu_ep0_handler)(u_int8_t req_type, u_int8_t req, diff --git a/firmware/src/os/pcd_enumerate.c b/firmware/src/os/pcd_enumerate.c index c360d37..3a7397f 100644 --- a/firmware/src/os/pcd_enumerate.c +++ b/firmware/src/os/pcd_enumerate.c @@ -531,12 +531,12 @@ static void udp_ep0_handler(void) if (*dfu->dfu_state != DFU_STATE_appIDLE) udp_ep0_send_data((const char *) dfu->dfu_dev_descriptor, - MIN(dfu->dfu_dev_descriptor->bLength, - wLength)); + dfu->dfu_dev_descriptor->bLength, + wLength); else #endif udp_ep0_send_data((const char *) &dev_descriptor, - MIN(sizeof(dev_descriptor), wLength)); + sizeof(dev_descriptor), wLength); break; case USB_DT_CONFIG: /* Return Configuration Descriptor */ @@ -544,12 +544,12 @@ static void udp_ep0_handler(void) if (*dfu->dfu_state != DFU_STATE_appIDLE) udp_ep0_send_data((const char *) dfu->dfu_cfg_descriptor, - MIN(dfu->dfu_cfg_descriptor->ucfg.wTotalLength, - wLength)); + dfu->dfu_cfg_descriptor->ucfg.wTotalLength, + wLength); else #endif udp_ep0_send_data((const char *) &cfg_descriptor, - MIN(sizeof(cfg_descriptor), wLength)); + sizeof(cfg_descriptor), wLength); break; case USB_DT_STRING: #ifdef CONFIG_USB_STRING @@ -559,8 +559,8 @@ static void udp_ep0_handler(void) DEBUGE("bLength=%u, wLength=%u\n", usb_strings[desc_index]->bLength, wLength); udp_ep0_send_data((const char *) usb_strings[desc_index], - MIN(usb_strings[desc_index]->bLength, - wLength)); + usb_strings[desc_index]->bLength, + wLength); #else goto out_stall; #endif @@ -568,7 +568,7 @@ static void udp_ep0_handler(void) case USB_DT_CS_DEVICE: /* Return Function descriptor */ udp_ep0_send_data((const char *) &dfu->dfu_cfg_descriptor->func_dfu, - MIN(sizeof(dfu->dfu_cfg_descriptor->func_dfu), wLength)); + sizeof(dfu->dfu_cfg_descriptor->func_dfu), wLength); break; case USB_DT_INTERFACE: /* Return Interface descriptor */ @@ -578,27 +578,27 @@ static void udp_ep0_handler(void) case 0: udp_ep0_send_data((const char *) &cfg_descriptor.uif, - MIN(sizeof(cfg_descriptor.uif), - wLength)); + sizeof(cfg_descriptor.uif), + wLength); break; #ifdef CONFIG_DFU case 1: udp_ep0_send_data((const char *) &cfg_descriptor.uif_dfu[0], - MIN(sizeof(cfg_descriptor.uif_dfu[0]), - wLength)); + sizeof(cfg_descriptor.uif_dfu[0]), + wLength); break; case 2: udp_ep0_send_data((const char *) &cfg_descriptor.uif_dfu[1], - MIN(sizeof(cfg_descriptor.uif_dfu[1]), - wLength)); + sizeof(cfg_descriptor.uif_dfu[1]), + wLength); break; case 3: udp_ep0_send_data((const char *) &cfg_descriptor.uif_dfu[2], - MIN(sizeof(cfg_descriptor.uif_dfu[2]), - wLength)); + sizeof(cfg_descriptor.uif_dfu[2]), + wLength); break; #endif default: @@ -679,8 +679,9 @@ static void udp_ep0_handler(void) switch (upcd.state) { case USB_STATE_ADDRESS: case USB_STATE_CONFIGURED: + /* Table 9.4 wLength One */ udp_ep0_send_data((char *)&(upcd.cur_config), - sizeof(upcd.cur_config)); + sizeof(upcd.cur_config), 1); break; default: goto out_stall; @@ -691,13 +692,15 @@ static void udp_ep0_handler(void) DEBUGE("GET_INTERFACE "); if (upcd.state != USB_STATE_CONFIGURED) goto out_stall; + /* Table 9.4 wLength One */ udp_ep0_send_data((char *)&(upcd.cur_altsett), - sizeof(upcd.cur_altsett)); + sizeof(upcd.cur_altsett), 1); break; case STD_GET_STATUS_ZERO: DEBUGE("GET_STATUS_ZERO "); wStatus = 0; - udp_ep0_send_data((char *)&wStatus, sizeof(wStatus)); + /* Table 9.4 wLength Two */ + udp_ep0_send_data((char *)&wStatus, sizeof(wStatus), 2); break; case STD_GET_STATUS_INTERFACE: DEBUGE("GET_STATUS_INTERFACE "); @@ -705,7 +708,8 @@ static void udp_ep0_handler(void) (upcd.state == USB_STATE_ADDRESS && wIndex != 0)) goto out_stall; wStatus = 0; - udp_ep0_send_data((char *)&wStatus, sizeof(wStatus)); + /* Table 9.4 wLength Two */ + udp_ep0_send_data((char *)&wStatus, sizeof(wStatus), 2); break; case STD_GET_STATUS_ENDPOINT: DEBUGE("GET_STATUS_ENDPOINT(EPidx=%u) ", wIndex&0x0f); @@ -717,14 +721,14 @@ static void udp_ep0_handler(void) if ((pUDP->UDP_GLBSTATE & AT91C_UDP_CONFG) && (wIndex <= 3)) { wStatus = (pUDP->UDP_CSR[wIndex] & AT91C_UDP_EPEDS) ? 0 : 1; - udp_ep0_send_data((char *)&wStatus, - sizeof(wStatus)); + /* Table 9.4 wLength Two */ + udp_ep0_send_data((char *)&wStatus, sizeof(wStatus), 2); } else if ((pUDP->UDP_GLBSTATE & AT91C_UDP_FADDEN) && (wIndex == 0)) { wStatus = (pUDP->UDP_CSR[wIndex] & AT91C_UDP_EPEDS) ? 0 : 1; - udp_ep0_send_data((char *)&wStatus, - sizeof(wStatus)); + /* Table 9.4 wLength Two */ + udp_ep0_send_data((char *)&wStatus, sizeof(wStatus), 2); } else goto out_stall; break;
On 11/09/2011 11:22 PM, Holger Hans Peter Freyther wrote:
From: Holger Hans Peter Freyther zecke@selfish.org
I enabled debug in dfu and added another debug statement. It appears to do the right thing. I have attached the device to Windows running in KVM but I think that a native windows will behave differently (bus reset, etc). I will try to test with a native windows, please don't apply the patch yet.
main(76): entering main (idle) loop send_data: 18/64 bytes stopped by status out send_data: 18/18 bytes NO ZLP 18 18 send_data: 68/9 bytes NO ZLP 68 9 send_data: 68/66 bytes NO ZLP 68 66 send_data: 4/255 bytes NO ZLP 4 255 send_data: 72/255 bytes set_txpktrdy_zlp send_data: 100/255 bytes NO ZLP 100 255 send_data: 62/255 bytes NO ZLP 62 255 send_data: 54/255 bytes NO ZLP 54 255 send_data: 94/255 bytes NO ZLP 94 255 send_data: 92/255 bytes NO ZLP 92 255 send_data: 58/255 bytes NO ZLP 58 255 send_data: 1/1 bytes NO ZLP 1 1 send_data: 18/64 bytes stopped by status out send_data: 18/18 bytes NO ZLP 18 18 send_data: 68/66 bytes NO ZLP 68 66 send_data: 18/64 bytes NO ZLP 18 64 send_data: 18/64 bytes stopped by status out send_data: 18/18 bytes NO ZLP 18 18 send_data: 68/66 bytes NO ZLP 68 66 send_data: 18/18 bytes NO ZLP 18 18 send_data: 68/255 bytes NO ZLP 68 255 send_data: 4/255 bytes NO ZLP 4 255 send_data: 72/255 bytes set_txpktrdy_zlp
On 11/10/2011 09:20 PM, Holger Hans Peter Freyther wrote:
On 11/09/2011 11:22 PM, Holger Hans Peter Freyther wrote:
From: Holger Hans Peter Freyther zecke@selfish.org
Hi again,
i tested this patch by connecting the device to Mac OS X 10.6, Windows7 in qemu/kvm (with -usb) and Dieter tested it by attaching yo his device. The upgrade procedure is not tested yet but should be:
1.) flash the new DFU 2.) reboot with bootloader button pressed 3.) flash the new main_simtrace.bin
Hi Holger,
On Wed, Nov 09, 2011 at 11:22:13PM +0100, Holger Hans Peter Freyther wrote:
Only send the ZLP if we send less data than was required/asked for by the host and it is a multiple of the bMaxPacketSize0 (which is hardcoded to 8 right now).
I've finally merged this.
That should basically mean we shouldn't need the os x hack anymore?
Cheers, Lukas
Sent from my iPhone
On Dec 14, 2011, at 3:31 PM, Harald Welte laforge@gnumonks.org wrote:
Hi Holger,
On Wed, Nov 09, 2011 at 11:22:13PM +0100, Holger Hans Peter Freyther wrote:
Only send the ZLP if we send less data than was required/asked for by the host and it is a multiple of the bMaxPacketSize0 (which is hardcoded to 8 right now).
I've finally merged this.
--
- Harald Welte laforge@gnumonks.org http://laforge.gnumonks.org/
============================================================================ "Privacy in residential applications is a desirable marketing option." (ETSI EN 300 175-7 Ch. A6)
On 12/14/2011 08:09 PM, Lukas Kuzmiak wrote:
That should basically mean we shouldn't need the os x hack anymore?
right. feel free to test the dfu upgrade path.
1.) reflash the dfu part 2.) reflash the main part 3.) reset/reboot
harald proposed to add ABI versioning to payloads, it might be a 30 minute work and I think I will attempt to find some time for it.
Hi Holger,
I've just cloned the latest git, compiled with no patches, reflashed the whole thing and can confirm it works with no problem on mac os x lion (macbook air).
Cheers, Lukas
On Wed, Dec 14, 2011 at 8:15 PM, Holger Hans Peter Freyther < holger@freyther.de> wrote:
On 12/14/2011 08:09 PM, Lukas Kuzmiak wrote:
That should basically mean we shouldn't need the os x hack anymore?
right. feel free to test the dfu upgrade path.
1.) reflash the dfu part 2.) reflash the main part 3.) reset/reboot
harald proposed to add ABI versioning to payloads, it might be a 30 minute work and I think I will attempt to find some time for it.
Is there a compiled version of this new firmware somewhere ?
Cheers,
Sylvain
Haven't noticed any, but I can upload you one I've compiled if u want.
Cheers, Lukas
Sent from my iPhone
On Dec 17, 2011, at 9:47 PM, Sylvain Munaut 246tnt@gmail.com wrote:
Is there a compiled version of this new firmware somewhere ?
Cheers,
Sylvain
Is there a compiled version of this new firmware somewhere ?
Haven't noticed any, but I can upload you one I've compiled if u want.
Yes, I think you could put it in the wikipage http://bb.osmocom.org/trac/wiki/SIMtrace/Firmware with the others.
(I think both main and dfu part are needed right ?)
Cheers,
Sylvain
On 12/17/2011 10:00 PM, Sylvain Munaut wrote:
Is there a compiled version of this new firmware somewhere ?
Haven't noticed any, but I can upload you one I've compiled if u want.
Yes, I think you could put it in the wikipage http://bb.osmocom.org/trac/wiki/SIMtrace/Firmware with the others.
Hi,
I would prefer to not upload non-tagged versions. I think there is one patch from Kevin that is not merged yet.
holger
-
Harald Welte -
Holger Hans Peter Freyther -
Lukas Kuzmiak -
Sylvain Munaut