[PATCH 0/4] CDC Phonet function fixes

[PATCH 0/4] CDC Phonet function fixes

[Rémi Denis-Courmont]

	Hello,

Here are a bunch of fixes for the Phonet USB gadget function network
interface driver. Comments welcome.

Rémi Denis-Courmont (4):
      f_phonet: dev_kfree_skb instead of dev_kfree_skb_any in TX callback
      f_phonet: no need to check for carrier - scheduler does it internally
      f_phonet: fix memory allocation sizes
      f_phonet: use page-sized rather than MTU-sized RX buffers

 drivers/usb/gadget/f_phonet.c |   95 +++++++++++++++++++++++++----------------
 1 files changed, 58 insertions(+), 37 deletions(-)

-- 
Rémi Denis-Courmont
Nokia Devices R&D, Maemo Software, Helsinki

[PATCH 1/4] f_phonet: dev_kfree_skb instead of dev_kfree_skb_any in TX callback

[Rémi Denis-Courmont]

From: Rémi Denis-Courmont <remi.denis-courmont@nokia.com>

Network device TX is never run in IRQ context, and skb is freed outside
of the IRQ-disabling spin lock. So checking for IRQ was a waste of time
here.

Signed-off-by: Rémi Denis-Courmont <remi.denis-courmont@nokia.com>
---
 drivers/usb/gadget/f_phonet.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/drivers/usb/gadget/f_phonet.c b/drivers/usb/gadget/f_phonet.c
index c1abeb8..54451ee 100644
--- a/drivers/usb/gadget/f_phonet.c
+++ b/drivers/usb/gadget/f_phonet.c
@@ -255,7 +255,7 @@ out_unlock:
 	spin_unlock_irqrestore(&port->lock, flags);
 out:
 	if (unlikely(skb)) {
-		dev_kfree_skb_any(skb);
+		dev_kfree_skb(skb);
 		dev->stats.tx_dropped++;
 	}
 	return 0;
-- 
1.5.6.5

[PATCH 3/4] f_phonet: fix inverted memory allocation sizes

[Rémi Denis-Courmont]

From: Rémi Denis-Courmont <remi.denis-courmont@nokia.com>

Signed-off-by: Rémi Denis-Courmont <remi.denis-courmont@nokia.com>
---
 drivers/usb/gadget/f_phonet.c |    9 ++++-----
 1 files changed, 4 insertions(+), 5 deletions(-)

diff --git a/drivers/usb/gadget/f_phonet.c b/drivers/usb/gadget/f_phonet.c
index dff7bae..25e711f 100644
--- a/drivers/usb/gadget/f_phonet.c
+++ b/drivers/usb/gadget/f_phonet.c
@@ -570,9 +570,10 @@ static struct net_device *dev;
 int __init phonet_bind_config(struct usb_configuration *c)
 {
 	struct f_phonet *fp;
-	int err;
+	int err, size;
 
-	fp = kzalloc(sizeof(*fp), GFP_KERNEL);
+	size = sizeof(*fp) + (phonet_rxq_size * sizeof(struct usb_request *));
+	fp = kzalloc(size, GFP_KERNEL);
 	if (!fp)
 		return -ENOMEM;
 
@@ -597,9 +598,7 @@ int __init gphonet_setup(struct usb_gadget *gadget)
 
 	/* Create net device */
 	BUG_ON(dev);
-	dev = alloc_netdev(sizeof(*port)
-		+ (phonet_rxq_size * sizeof(struct usb_request *)),
-				"upnlink%d", pn_net_setup);
+	dev = alloc_netdev(sizeof(*port), "upnlink%d", pn_net_setup);
 	if (!dev)
 		return -ENOMEM;
 
-- 
1.5.6.5

[PATCH 4/4] f_phonet: use page-sized rather than MTU-sized RX buffers

[Rémi Denis-Courmont]

From: Rémi Denis-Courmont <remi.denis-courmont@nokia.com>

Instead of a large (physically) linear buffer, we generate a paged
sk_buff, so no extra memory copy is involved.

This removes high-order allocations and saves quite a bit of locked
memory (MTU is 65543 bytes, was padded to 128 kilo-bytes). This also
limits overcharging of the receiver socket buffer space.

Signed-off-by: Rémi Denis-Courmont <remi.denis-courmont@nokia.com>
---
 drivers/usb/gadget/f_phonet.c |   76 +++++++++++++++++++++++++++-------------
 1 files changed, 51 insertions(+), 25 deletions(-)

diff --git a/drivers/usb/gadget/f_phonet.c b/drivers/usb/gadget/f_phonet.c
index 25e711f..51235f2 100644
--- a/drivers/usb/gadget/f_phonet.c
+++ b/drivers/usb/gadget/f_phonet.c
@@ -35,6 +35,10 @@
 #include "u_phonet.h"
 
 #define PN_MEDIA_USB	0x1B
+#define MAXPACKET	512
+#if (PAGE_SIZE % MAXPACKET)
+#error MAXPACKET must divide PAGE_SIZE!
+#endif
 
 /*-------------------------------------------------------------------------*/
 
@@ -45,6 +49,10 @@ struct phonet_port {
 
 struct f_phonet {
 	struct usb_function		function;
+	struct {
+		struct sk_buff		*skb;
+		spinlock_t		lock;
+	} rx;
 	struct net_device		*dev;
 	struct usb_ep			*in_ep, *out_ep;
 
@@ -138,7 +146,7 @@ pn_hs_sink_desc = {
 
 	.bEndpointAddress =	USB_DIR_OUT,
 	.bmAttributes =		USB_ENDPOINT_XFER_BULK,
-	.wMaxPacketSize =	cpu_to_le16(512),
+	.wMaxPacketSize =	cpu_to_le16(MAXPACKET),
 };
 
 static struct usb_endpoint_descriptor
@@ -308,21 +316,21 @@ static void pn_net_setup(struct net_device *dev)
 static int
 pn_rx_submit(struct f_phonet *fp, struct usb_request *req, gfp_t gfp_flags)
 {
-	struct sk_buff *skb;
-	const size_t size = fp->dev->mtu;
+	struct net_device *dev = fp->dev;
+	struct page *page;
 	int err;
 
-	skb = alloc_skb(size, gfp_flags);
-	if (!skb)
+	page = __netdev_alloc_page(dev, gfp_flags);
+	if (!page)
 		return -ENOMEM;
 
-	req->buf = skb->data;
-	req->length = size;
-	req->context = skb;
+	req->buf = page_address(page);
+	req->length = PAGE_SIZE;
+	req->context = page;
 
 	err = usb_ep_queue(fp->out_ep, req, gfp_flags);
 	if (unlikely(err))
-		dev_kfree_skb_any(skb);
+		netdev_free_page(dev, page);
 	return err;
 }
 
@@ -330,25 +338,37 @@ static void pn_rx_complete(struct usb_ep *ep, struct usb_request *req)
 {
 	struct f_phonet *fp = ep->driver_data;
 	struct net_device *dev = fp->dev;
-	struct sk_buff *skb = req->context;
+	struct page *page = req->context;
+	struct sk_buff *skb;
+	unsigned long flags;
 	int status = req->status;
 
 	switch (status) {
 	case 0:
-		if (unlikely(!netif_running(dev)))
-			break;
-		if (unlikely(req->actual < 1))
+		spin_lock_irqsave(&fp->rx.lock, flags);
+		skb = fp->rx.skb;
+		if (!skb)
+			skb = fp->rx.skb = netdev_alloc_skb(dev, 12);
+		if (req->actual < req->length) /* Last fragment */
+			fp->rx.skb = NULL;
+		spin_unlock_irqrestore(&fp->rx.lock, flags);
+
+		if (unlikely(!skb))
 			break;
-		skb_put(skb, req->actual);
-		skb->protocol = htons(ETH_P_PHONET);
-		skb_reset_mac_header(skb);
-		__skb_pull(skb, 1);
-		skb->dev = dev;
-		dev->stats.rx_packets++;
-		dev->stats.rx_bytes += skb->len;
-
-		netif_rx(skb);
-		skb = NULL;
+		skb_add_rx_frag(skb, skb_shinfo(skb)->nr_frags, page, 0,
+				req->actual);
+		page = NULL;
+
+		if (req->actual < req->length) { /* Last fragment */
+			skb->protocol = htons(ETH_P_PHONET);
+			skb_reset_mac_header(skb);
+			pskb_pull(skb, 1);
+			skb->dev = dev;
+			dev->stats.rx_packets++;
+			dev->stats.rx_bytes += skb->len;
+
+			netif_rx(skb);
+		}
 		break;
 
 	/* Do not resubmit in these cases: */
@@ -366,8 +386,8 @@ static void pn_rx_complete(struct usb_ep *ep, struct usb_request *req)
 		break;
 	}
 
-	if (skb)
-		dev_kfree_skb_any(skb);
+	if (page)
+		netdev_free_page(dev, page);
 	if (req)
 		pn_rx_submit(fp, req, GFP_ATOMIC);
 }
@@ -386,6 +407,10 @@ static void __pn_reset(struct usb_function *f)
 
 	usb_ep_disable(fp->out_ep);
 	usb_ep_disable(fp->in_ep);
+	if (fp->rx.skb) {
+		dev_kfree_skb_irq(fp->rx.skb);
+		fp->rx.skb = NULL;
+	}
 }
 
 static int pn_set_alt(struct usb_function *f, unsigned intf, unsigned alt)
@@ -584,6 +609,7 @@ int __init phonet_bind_config(struct usb_configuration *c)
 	fp->function.set_alt = pn_set_alt;
 	fp->function.get_alt = pn_get_alt;
 	fp->function.disable = pn_disconnect;
+	spin_lock_init(&fp->rx.lock);
 
 	err = usb_add_function(c, &fp->function);
 	if (err)
-- 
1.5.6.5

[PATCH 2/4] f_phonet: no need to check for carrier - scheduler does it internally

[Rémi Denis-Courmont]

From: Rémi Denis-Courmont <remi.denis-courmont-xNZwKgViW5gAvxtiuMwx3w@public.gmane.org>

Signed-off-by: Rémi Denis-Courmont <remi.denis-courmont-xNZwKgViW5gAvxtiuMwx3w@public.gmane.org>
---
 drivers/usb/gadget/f_phonet.c |    8 ++------
 1 files changed, 2 insertions(+), 6 deletions(-)

diff --git a/drivers/usb/gadget/f_phonet.c b/drivers/usb/gadget/f_phonet.c
index 54451ee..dff7bae 100644
--- a/drivers/usb/gadget/f_phonet.c
+++ b/drivers/usb/gadget/f_phonet.c
@@ -188,8 +188,7 @@ static struct usb_descriptor_header *hs_pn_function[] = {
 
 static int pn_net_open(struct net_device *dev)
 {
-	if (netif_carrier_ok(dev))
-		netif_wake_queue(dev);
+	netif_wake_queue(dev);
 	return 0;
 }
 
@@ -219,8 +218,7 @@ static void pn_tx_complete(struct usb_ep *ep, struct usb_request *req)
 	}
 
 	dev_kfree_skb_any(skb);
-	if (netif_carrier_ok(dev))
-		netif_wake_queue(dev);
+	netif_wake_queue(dev);
 }
 
 static int pn_net_xmit(struct sk_buff *skb, struct net_device *dev)
@@ -427,8 +425,6 @@ static int pn_set_alt(struct usb_function *f, unsigned intf, unsigned alt)
 			fp->in_ep->driver_data = fp;
 
 			netif_carrier_on(dev);
-			if (netif_running(dev))
-				netif_wake_queue(dev);
 			for (i = 0; i < phonet_rxq_size; i++)
 				pn_rx_submit(fp, fp->out_reqv[i], GFP_ATOMIC);
 		}
-- 
1.5.6.5

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 0/4] CDC Phonet function fixes

[David Brownell]

On Monday 06 April 2009, Rémi Denis-Courmont wrote:
> Here are a bunch of fixes for the Phonet USB gadget function network
> interface driver. Comments welcome.

I seem to have missed any patch for this "Phonet" code,
and so I have no way to evaluate bug fixes to it.  It's
not in any standard USB patch queue.

Got a URL for those patches?  Maybe you should just
submit the drivers/usb/gadget/f_phonet.c code, and
something using it, with these patches pre-merged...

- Dave

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 0/4] CDC Phonet function fixes

[Rémi Denis-Courmont]

	Hello,

(My corporate alter ego now on vacation)

Le lundi 6 avril 2009 21:53:46 David Brownell, vous avez écrit :
> On Monday 06 April 2009, Rémi Denis-Courmont wrote:
> > Here are a bunch of fixes for the Phonet USB gadget function network
> > interface driver. Comments welcome.
>
> I seem to have missed any patch for this "Phonet" code,
> and so I have no way to evaluate bug fixes to it.  It's
> not in any standard USB patch queue.

Those are there: 
http://kerneltrap.org/index.php?q=mailarchive/linux-netdev/2008/12/17/4438934/thread

AFAIK, they are already in the mainline, though the code is not used by any 
published gadget driver that I am aware of (paraphrase for "dead [code]").

> Got a URL for those patches?  Maybe you should just
> submit the drivers/usb/gadget/f_phonet.c code, and
> something using it, with these patches pre-merged...

It seems I've been overly stingy on the Cc:s in December. I would rather 
not -on top of that- bother davem/gregkh/whoever with a merge conflict just 
for the sake of The Process or cosmetics.

Sorry for the inconvenience.

-- 
Rémi Denis-Courmont
http://www.remlab.net/