Here's another patch to be applied after the previous two. I have a
feeling you won't like it, but I'm sending it just for the record. It
makes rndis_host query the device for the MTU and update net->mtu etc.
accordingly. The Axim reports 8050, the Wizard reports 1500, so it
depends on the device what it's set to by default. (I haven't tried
changing the default, not sure if the OID is read-write.)
Doesn't seem to improve the IN performance much, but for OUT it
results in 269262 bytes/s instead of 244783 bytes/s, meaning a
performance boost of ~9%.
This is still only about 50% of the OUT performance delivered by the
Windows driver. My theory here is that this is because we don't batch
multiple packets per transfer and the devices are optimized for
infrequent big transfers instead of frequent smaller ones.
Ole André
On 12/16/06, Ole André Vadla Ravnås <[EMAIL PROTECTED]> wrote:
Hi again,
Here's two patches against the latest revision.
The first patch restores the rx_urb_size, which makes my devices work
again and results in quite acceptable performance for device to host
transfers:
$ pcp ":/My Documents/nightwish - angels fall first - angels fall
first.mp3" "foo.mp3"
File copy of 5385245 bytes took 0 minutes and 8 seconds, that's 673155 bytes/s.
This is equivalent to the Windows driver, if not faster (the fastest I
got there was 625 kB/s, though that was testing against an FTP server
on the device instead of the native RPC API and thus not very
scientifically correct).
The second patch makes rndis_host work with PXA27x-based devices.
(Time to open those champagne bottles! :D)
The fix is simply not setting an alternate setting -- SET_INTERFACE
makes these devices behave strangely ("hanging" the status and bulk
endpoints), leaving this out makes it all work as it should.
Cheers,
Ole André
On 12/16/06, Ole André Vadla Ravnås <[EMAIL PROTECTED]> wrote:
> On 12/15/06, David Brownell <[EMAIL PROTECTED]> wrote:
> > On Wednesday 13 December 2006 2:17 pm, Ole André Vadla Ravnås wrote:
> (...)
> > > After debugging this issue further I discovered that RNDIS_MSG_INIT's
> > > MaxTransferSize, sent by the host, is the value that
> > > usbnet.rx_urb_size should be set to. If this is set to something lower
> > > the device will (obviously) prepare larger URB payloads than what we
> > > expect, and thus we'll fetch partial ones. This results in horrible
> > > performance and makes the device stop responding after attempting a
> > > few transfers.
> > >
> > > Patch (attached): linux-2.6.19-git-rndis_host-rx_usb_size-fix.patch
> >
> > This patch looks wrong. Instead, how about this theory: the peripheral
> > is using the "pad with zeroes to end of packet" option on packets it
> > sends, but the rndis_host code wasn't expecting that. The reason your
> > patch worked was because it was creating HUGE rx buffers, which also
> > coincidntally happened to allow that padding.
> >
> > I've updated the patch to allow padded RX buffers like that.
>
> Well, I tried with the revised patch and that seems to have broken it
> completely.
> My theory here is that the MaxTransferSize you specify in the
> RNDIS_MSG_INIT is the maximum size you're able to read (IN) in one
> transfer (one URB in the USB context). My devices here (HTC Wizard,
> HTC TyTN and Dell Axim X51) all batch multiple RNDIS_MSG_PACKET
> messages per transfer. So, what happens if you generate sufficient
> traffic from the device to the host is that the device batches
> multiple packets in each transfer, typically 13-14 kB worth of
> packets. When you do one read with the buffer-size set to for instance
> 8 kB, the device will (obviously) give you the first 8 kB, and the
> rest in the next read. So, by stating 16 kB in MaxTransferSize we
> oblige to have an rx buffer at least that big. And when sending URBs
> to the device, we should never do bigger (OUT) transfers than what the
> device reports as MaxTransferSize in the RNDIS_MSG_INIT reply.
>
> Ole André
>
Index: rndis_host.c
===================================================================
--- rndis_host.c (revision 2705)
+++ rndis_host.c (revision 2707)
@@ -254,6 +254,7 @@
* of that mess as possible.
*/
#define OID_802_3_PERMANENT_ADDRESS ccpu2(0x01010101)
+#define OID_GEN_MAXIMUM_FRAME_SIZE ccpu2(0x00010106)
#define OID_GEN_CURRENT_PACKET_FILTER ccpu2(0x0001010e)
/*
@@ -388,6 +389,71 @@
return -ETIMEDOUT;
}
+/*
+ * rndis_query:
+ *
+ * Performs a query for #oid along with 0 or more bytes of
+ * payload as specified by #in_len. If #reply_len is not set
+ * to -1 then the reply length is checked against this value,
+ * resulting in an error if it doesn't match.
+ *
+ * NOTE: Adding a payload exactly or greater than the size of
+ * the expected response payload is an evident requirement MSFT
+ * added for ActiveSync.
+ * The only exception is for OIDs that return a variably sized
+ * response, in which case no payload should be added.
+ * This undocumented (and nonsensical) issue was found by sniffing
+ * protocol requests from the ActiveSync 4.1 Windows driver.
+ */
+static int rndis_query(struct usbnet *dev, struct usb_interface *intf,
+ void *buf, u32 oid, u32 in_len,
+ void **reply, int *reply_len)
+{
+ int retval;
+ union {
+ void *buf;
+ struct rndis_msg_hdr *header;
+ struct rndis_query *get;
+ struct rndis_query_c *get_c;
+ } u;
+ u32 off, len;
+
+ u.buf = buf;
+
+ memset(u.get, 0, sizeof *u.get + in_len);
+ u.get->msg_type = RNDIS_MSG_QUERY;
+ u.get->msg_len = ccpu2(sizeof *u.get + in_len);
+ u.get->oid = oid;
+ u.get->len = ccpu2(in_len);
+ u.get->offset = ccpu2(20);
+
+ retval = rndis_command(dev, u.header);
+ if (unlikely(retval < 0)) {
+ dev_err(&intf->dev, "RNDIS_MSG_QUERY(0x%08x) failed, %d\n",
+ oid, retval);
+ return retval;
+ }
+
+ off = le32_to_cpu(u.get_c->offset);
+ len = le32_to_cpu(u.get_c->len);
+ if (unlikely((8 + off + len) > CONTROL_BUFFER_SIZE))
+ goto response_error;
+
+ if (*reply_len != -1 && len != *reply_len)
+ goto response_error;
+
+ *reply = (unsigned char *) &u.get_c->request_id + off;
+ *reply_len = len;
+
+ return retval;
+
+response_error:
+ dev_err(&intf->dev, "RNDIS_MSG_QUERY(0x%08x) failed because of an "
+ "invalid response - off %d len %d\n",
+ oid, off, len);
+ return -EDOM;
+}
+
static int rndis_bind(struct usbnet *dev, struct usb_interface *intf)
{
int retval;
@@ -402,7 +468,9 @@
struct rndis_set *set;
struct rndis_set_c *set_c;
} u;
- u32 tmp;
+ u32 tmp, *dwp;
+ int reply_len;
+ unsigned char *bp;
/* we can't rely on i/o from stack working, or stack allocation */
u.buf = kmalloc(CONTROL_BUFFER_SIZE, GFP_KERNEL);
@@ -456,35 +524,25 @@
dev->hard_mtu, tmp, dev->rx_urb_size,
1 << le32_to_cpu(u.init_c->packet_alignment));
- /* Get designated host ethernet address.
- *
- * Adding a payload exactly the same size as the expected response
- * payload is an evident requirement MSFT added for ActiveSync.
- * This undocumented (and nonsensical) issue was found by sniffing
- * protocol requests from the ActiveSync 4.1 Windows driver.
- */
- memset(u.get, 0, sizeof *u.get + 48);
- u.get->msg_type = RNDIS_MSG_QUERY;
- u.get->msg_len = ccpu2(sizeof *u.get + 48);
- u.get->oid = OID_802_3_PERMANENT_ADDRESS;
- u.get->len = ccpu2(48);
- u.get->offset = ccpu2(20);
+ /* Get designated host ethernet address. */
+ reply_len = ETH_ALEN;
+ retval = rndis_query(dev, intf, u.buf, OID_802_3_PERMANENT_ADDRESS,
+ 48, (void **) &bp, &reply_len);
+ if (unlikely(retval < 0))
+ goto fail;
- retval = rndis_command(dev, u.header);
- if (unlikely(retval < 0)) {
- dev_err(&intf->dev, "rndis get ethaddr, %d\n", retval);
+ memcpy(net->dev_addr, bp, ETH_ALEN);
+
+ /* Get the preferred MTU. */
+ reply_len = 4;
+ retval = rndis_query(dev, intf, u.buf, OID_GEN_MAXIMUM_FRAME_SIZE, 4,
+ (void **) &dwp, &reply_len);
+ if (unlikely(retval < 0))
goto fail;
- }
- tmp = le32_to_cpu(u.get_c->offset);
- if (unlikely((tmp + 8) > (CONTROL_BUFFER_SIZE - ETH_ALEN)
- || u.get_c->len != ccpu2(ETH_ALEN))) {
- dev_err(&intf->dev, "rndis ethaddr off %d len %d ?\n",
- tmp, le32_to_cpu(u.get_c->len));
- retval = -EDOM;
- goto fail;
- }
- memcpy(net->dev_addr, tmp + (char *)&u.get_c->request_id, ETH_ALEN);
+ net->mtu = le32_to_cpu(*dwp);
+ dev->hard_mtu = net->mtu + net->hard_header_len;
+
/* set a nonzero filter to enable data transfers */
memset(u.set, 0, sizeof *u.set);
u.set->msg_type = RNDIS_MSG_SET;
-------------------------------------------------------------------------
Take Surveys. Earn Cash. Influence the Future of IT
Join SourceForge.net's Techsay panel and you'll get the chance to share your
opinions on IT & business topics through brief surveys - and earn cash
http://www.techsay.com/default.php?page=join.php&p=sourceforge&CID=DEVDEV
_______________________________________________
linux-usb-devel@lists.sourceforge.net
To unsubscribe, use the last form field at:
https://lists.sourceforge.net/lists/listinfo/linux-usb-devel
