[RFC PATCH 0/6] cdc_ncm: some basic refactoring

[RFC PATCH 0/6] cdc_ncm: some basic refactoring

[Enrico Mioso]

Here is some work I did to move in the direction of refactoring the cdc_ncm
module, to allow for future further changes.
This is work in progress (WIP): and I know some changes are controversial. In
particular, the modification you can see related to cdc_ncm.h header file is
useless at this point. But actually it can be useful for learning from your
opinions: is this the right thing to do?
Thank in advance to anyone reviewing this code.
The final objective would be to have the driver accumulate frames as it does
now, but in a skb queue (sk_buff_head), and then generating the NCM frame
itself only when needed (queue full / flueshed).
I need assistance / help: the logic seems hard to follow for me (is it me or
is it little bit convoluted?).
Thank you for any help, waiting for your comments.
Note: please CC me as I am not subscribed to this list.


Enrico Mioso (6):
  cdc_ncm: factor out skb allocation and finalizzing
  cdc_ncm: be more precise in comments for cdc_ncm_prepare_skb_ncm16
  cdc_ncm: add needed members to cdc_ncm_ctx for refactoring
  cdc_ncm: update specs URL
  cdc_ncm: fix typo
  cdc_ncm: factor out NDP preparation and frame linking

 drivers/net/usb/cdc_ncm.c   | 74 ++++++++++++++++++++++++++++++++-------------
 include/linux/usb/cdc_ncm.h |  1 +
 2 files changed, 54 insertions(+), 21 deletions(-)

-- 
2.2.1

[RFC PATCH 1/6] cdc_ncm: factor out skb allocation and finalizzing

[Enrico Mioso]

This might help when we will refactor the code to prepare frames only when we
have enough data, allowing us to re-order the position of different NCM
fields.

Signed-Off-By: Enrico Mioso <mrkiko.rs@gmail.com>
---
 drivers/net/usb/cdc_ncm.c | 46 +++++++++++++++++++++++++++++++++++-----------
 1 file changed, 35 insertions(+), 11 deletions(-)

diff --git a/drivers/net/usb/cdc_ncm.c b/drivers/net/usb/cdc_ncm.c
index 389a3a4..48fee7a 100644
--- a/drivers/net/usb/cdc_ncm.c
+++ b/drivers/net/usb/cdc_ncm.c
@@ -1014,6 +1014,36 @@ static struct usb_cdc_ncm_ndp16 *cdc_ncm_ndp16(struct cdc_ncm_ctx *ctx, struct s
 	return ndp16;
 }
 
+/* Allocate new SKB for use with 16-bit NCM according to actual TX/RX
+	settings */
+struct sk_buff *
+cdc_ncm_prepare_skb_ncm16(struct sk_buff *skb, struct cdc_ncm_ctx *ctx)
+{
+	struct usb_cdc_ncm_nth16 *nth16;
+	
+	skb = alloc_skb(ctx->tx_max, GFP_ATOMIC);
+	if (skb == NULL)
+			goto exit_no_mem;
+		
+	/* fill out the initial 16-bit NTB header */
+	nth16 = (struct usb_cdc_ncm_nth16 *)memset(skb_put(skb, sizeof(struct usb_cdc_ncm_nth16)), 0, sizeof(struct usb_cdc_ncm_nth16));
+	nth16->dwSignature = cpu_to_le32(USB_CDC_NCM_NTH16_SIGN);
+	nth16->wHeaderLength = cpu_to_le16(sizeof(struct usb_cdc_ncm_nth16));
+	nth16->wSequence = cpu_to_le16(ctx->tx_seq++);
+
+exit_no_mem:
+	return skb;
+}
+
+/* Adjust 16-bit NCM header frame length */
+struct usb_cdc_ncm_nth16 *
+cdc_ncm_finalize_nth16(struct usb_cdc_ncm_nth16 *nth16, struct sk_buff *skb)
+{
+	nth16 = (struct usb_cdc_ncm_nth16 *)skb->data;
+	nth16->wBlockLength = cpu_to_le16(skb->len);
+	return nth16;
+}
+
 struct sk_buff *
 cdc_ncm_fill_tx_frame(struct usbnet *dev, struct sk_buff *skb, __le32 sign)
 {
@@ -1037,7 +1067,7 @@ cdc_ncm_fill_tx_frame(struct usbnet *dev, struct sk_buff *skb, __le32 sign)
 
 	/* allocate a new OUT skb */
 	if (!skb_out) {
-		skb_out = alloc_skb(ctx->tx_max, GFP_ATOMIC);
+		skb_out = cdc_ncm_prepare_skb_ncm16(skb_out, ctx);
 		if (skb_out == NULL) {
 			if (skb != NULL) {
 				dev_kfree_skb_any(skb);
@@ -1045,11 +1075,6 @@ cdc_ncm_fill_tx_frame(struct usbnet *dev, struct sk_buff *skb, __le32 sign)
 			}
 			goto exit_no_skb;
 		}
-		/* fill out the initial 16-bit NTB header */
-		nth16 = (struct usb_cdc_ncm_nth16 *)memset(skb_put(skb_out, sizeof(struct usb_cdc_ncm_nth16)), 0, sizeof(struct usb_cdc_ncm_nth16));
-		nth16->dwSignature = cpu_to_le32(USB_CDC_NCM_NTH16_SIGN);
-		nth16->wHeaderLength = cpu_to_le16(sizeof(struct usb_cdc_ncm_nth16));
-		nth16->wSequence = cpu_to_le16(ctx->tx_seq++);
 
 		/* count total number of frames in this NTB */
 		ctx->tx_curr_frame_num = 0;
@@ -1165,11 +1190,10 @@ cdc_ncm_fill_tx_frame(struct usbnet *dev, struct sk_buff *skb, __le32 sign)
 		       ctx->tx_max - skb_out->len);
 	else if (skb_out->len < ctx->tx_max && (skb_out->len % dev->maxpacket) == 0)
 		*skb_put(skb_out, 1) = 0;	/* force short packet */
-
-	/* set final frame length */
-	nth16 = (struct usb_cdc_ncm_nth16 *)skb_out->data;
-	nth16->wBlockLength = cpu_to_le16(skb_out->len);
-
+	
+	/* Finalize packet */
+	nth16 = cdc_ncm_finalize_nth16(nth16, skb_out);
+	
 	/* return skb */
 	ctx->tx_curr_skb = NULL;
 	dev->net->stats.tx_packets += ctx->tx_curr_frame_num;
-- 
2.2.1

[RFC PATCH 2/6] cdc_ncm: be more precise in comments for cdc_ncm_prepare_skb_ncm16

[Enrico Mioso]

The function might return NULL: callers must be prepared to this possibility.

Signed-Off-By: Enrico Mioso <mrkiko.rs@gmail.com>
---
 drivers/net/usb/cdc_ncm.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/usb/cdc_ncm.c b/drivers/net/usb/cdc_ncm.c
index 48fee7a..bcd9437 100644
--- a/drivers/net/usb/cdc_ncm.c
+++ b/drivers/net/usb/cdc_ncm.c
@@ -1015,7 +1015,7 @@ static struct usb_cdc_ncm_ndp16 *cdc_ncm_ndp16(struct cdc_ncm_ctx *ctx, struct s
 }
 
 /* Allocate new SKB for use with 16-bit NCM according to actual TX/RX
-	settings */
+	settings; returns NULL in case of errors */
 struct sk_buff *
 cdc_ncm_prepare_skb_ncm16(struct sk_buff *skb, struct cdc_ncm_ctx *ctx)
 {
-- 
2.2.1

[RFC PATCH 3/6] cdc_ncm: add needed members to cdc_ncm_ctx for refactoring

[Enrico Mioso]

We intend to use something like a TX queue - so prepare the driver's header
file for this.

Signed-Off-By: Enrico Mioso <mrkiko.rs@gmail.com>
---
 include/linux/usb/cdc_ncm.h | 1 +
 1 file changed, 1 insertion(+)

diff --git a/include/linux/usb/cdc_ncm.h b/include/linux/usb/cdc_ncm.h
index 7c9b484..6710555 100644
--- a/include/linux/usb/cdc_ncm.h
+++ b/include/linux/usb/cdc_ncm.h
@@ -100,6 +100,7 @@ struct cdc_ncm_ctx {
 	struct sk_buff *tx_curr_skb;
 	struct sk_buff *tx_rem_skb;
 	__le32 tx_rem_sign;
+	struct sk_buff_head tx_skb_queue;
 
 	spinlock_t mtx;
 	atomic_t stop;
-- 
2.2.1

[RFC PATCH 4/6] cdc_ncm: update specs URL

[Enrico Mioso]

Just in case there is the need to reference docs.

Signed-Off-By: Enrico Mioso <mrkiko.rs@gmail.com>
---
 drivers/net/usb/cdc_ncm.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/usb/cdc_ncm.c b/drivers/net/usb/cdc_ncm.c
index bcd9437..0970991 100644
--- a/drivers/net/usb/cdc_ncm.c
+++ b/drivers/net/usb/cdc_ncm.c
@@ -6,7 +6,7 @@
  * Original author: Hans Petter Selasky <hans.petter.selasky@stericsson.com>
  *
  * USB Host Driver for Network Control Model (NCM)
- * http://www.usb.org/developers/devclass_docs/NCM10.zip
+ * http://www.usb.org/developers/docs/devclass_docs/NCM10_012011.zip
  *
  * The NCM encoding, decoding and initialization logic
  * derives from FreeBSD 8.x. if_cdce.c and if_cdcereg.h
-- 
2.2.1

[RFC PATCH 5/6] cdc_ncm: fix typo

[Enrico Mioso]

It probably was "within".

Signed-Off-By: Enrico Mioso <mrkiko.rs@gmail.com>
---
 drivers/net/usb/cdc_ncm.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/usb/cdc_ncm.c b/drivers/net/usb/cdc_ncm.c
index 0970991..babfaee 100644
--- a/drivers/net/usb/cdc_ncm.c
+++ b/drivers/net/usb/cdc_ncm.c
@@ -1123,7 +1123,7 @@ cdc_ncm_fill_tx_frame(struct usbnet *dev, struct sk_buff *skb, __le32 sign)
 			break;
 		}
 
-		/* calculate frame number withing this NDP */
+		/* calculate frame number within this NDP */
 		ndplen = le16_to_cpu(ndp16->wLength);
 		index = (ndplen - sizeof(struct usb_cdc_ncm_ndp16)) / sizeof(struct usb_cdc_ncm_dpe16) - 1;
 
-- 
2.2.1

[RFC PATCH 6/6] cdc_ncm: factor out NDP preparation and frame linking

[Enrico Mioso]

To some extent, factor out NDP preparation and frame linking.
This is the change I like less - but I plan to revisit this once (in case) we
change the way the entire cdc_ncm_fill_tx_frame works.

Signed-Off-By: Enrico Mioso <mrkiko.rs@gmail.com>
---
 drivers/net/usb/cdc_ncm.c | 24 ++++++++++++++++--------
 1 file changed, 16 insertions(+), 8 deletions(-)

diff --git a/drivers/net/usb/cdc_ncm.c b/drivers/net/usb/cdc_ncm.c
index babfaee..b21aab8 100644
--- a/drivers/net/usb/cdc_ncm.c
+++ b/drivers/net/usb/cdc_ncm.c
@@ -1044,6 +1044,20 @@ cdc_ncm_finalize_nth16(struct usb_cdc_ncm_nth16 *nth16, struct sk_buff *skb)
 	return nth16;
 }
 
+u16
+cdc_ncm_calculate_ndp16(struct usb_cdc_ncm_ndp16 *ndp16, struct sk_buff *skb, struct sk_buff *next_skb) {
+	u16 ndplen, index;
+	
+	ndplen = le16_to_cpu(ndp16->wLength);
+	index = (ndplen - sizeof(struct usb_cdc_ncm_ndp16)) / sizeof(struct usb_cdc_ncm_dpe16) - 1;
+
+	/* OK, add this skb */
+	ndp16->dpe16[index].wDatagramLength = cpu_to_le16(skb->len);
+	ndp16->dpe16[index].wDatagramIndex = cpu_to_le16(next_skb->len);
+	ndp16->wLength = cpu_to_le16(ndplen + sizeof(struct usb_cdc_ncm_dpe16));
+	return index;
+}
+
 struct sk_buff *
 cdc_ncm_fill_tx_frame(struct usbnet *dev, struct sk_buff *skb, __le32 sign)
 {
@@ -1051,7 +1065,7 @@ cdc_ncm_fill_tx_frame(struct usbnet *dev, struct sk_buff *skb, __le32 sign)
 	struct usb_cdc_ncm_nth16 *nth16;
 	struct usb_cdc_ncm_ndp16 *ndp16;
 	struct sk_buff *skb_out;
-	u16 n = 0, index, ndplen;
+	u16 n = 0, index;
 	u8 ready2send = 0;
 
 	/* if there is a remaining skb, it gets priority */
@@ -1124,13 +1138,7 @@ cdc_ncm_fill_tx_frame(struct usbnet *dev, struct sk_buff *skb, __le32 sign)
 		}
 
 		/* calculate frame number within this NDP */
-		ndplen = le16_to_cpu(ndp16->wLength);
-		index = (ndplen - sizeof(struct usb_cdc_ncm_ndp16)) / sizeof(struct usb_cdc_ncm_dpe16) - 1;
-
-		/* OK, add this skb */
-		ndp16->dpe16[index].wDatagramLength = cpu_to_le16(skb->len);
-		ndp16->dpe16[index].wDatagramIndex = cpu_to_le16(skb_out->len);
-		ndp16->wLength = cpu_to_le16(ndplen + sizeof(struct usb_cdc_ncm_dpe16));
+		index = cdc_ncm_calculate_ndp16(ndp16, skb, skb_out);
 		memcpy(skb_put(skb_out, skb->len), skb->data, skb->len);
 		ctx->tx_curr_frame_payload += skb->len;	/* count real tx payload data */
 		dev_kfree_skb_any(skb);
-- 
2.2.1

Re: [RFC PATCH 2/6] cdc_ncm: be more precise in comments for cdc_ncm_prepare_skb_ncm16

[Sergei Shtylyov]

Hello.

On 12/29/2014 1:09 PM, Enrico Mioso wrote:

> The function might return NULL: callers must be prepared to this possibility.

> Signed-Off-By: Enrico Mioso <mrkiko.rs@gmail.com>
> ---
>   drivers/net/usb/cdc_ncm.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)

> diff --git a/drivers/net/usb/cdc_ncm.c b/drivers/net/usb/cdc_ncm.c
> index 48fee7a..bcd9437 100644
> --- a/drivers/net/usb/cdc_ncm.c
> +++ b/drivers/net/usb/cdc_ncm.c
> @@ -1015,7 +1015,7 @@ static struct usb_cdc_ncm_ndp16 *cdc_ncm_ndp16(struct cdc_ncm_ctx *ctx, struct s
>   }
>
>   /* Allocate new SKB for use with 16-bit NCM according to actual TX/RX
> -	settings */
> +	settings; returns NULL in case of errors */

    BTW, the preferred style for the multi-line comments in the networking 
code is:

/* bla
  * bla
  */

WBR, Sergei

Re: [RFC PATCH 2/6] cdc_ncm: be more precise in comments for cdc_ncm_prepare_skb_ncm16

[Enrico Mioso]

Thank you Sergei: for your time and review. I'll be careful to this.
Wish you best regards too - and good new year!

Enrico



On Mon, 29 Dec 2014, Sergei Shtylyov wrote:

> Date: Mon, 29 Dec 2014 14:29:40
> From: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>
> To: Enrico Mioso <mrkiko.rs@gmail.com>, netdev@vger.kernel.org
> Subject: Re: [RFC PATCH 2/6] cdc_ncm: be more precise in comments for
>     cdc_ncm_prepare_skb_ncm16
> 
> Hello.
>
> On 12/29/2014 1:09 PM, Enrico Mioso wrote:
>
>> The function might return NULL: callers must be prepared to this 
>> possibility.
>
>> Signed-Off-By: Enrico Mioso <mrkiko.rs@gmail.com>
>> ---
>>   drivers/net/usb/cdc_ncm.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>
>> diff --git a/drivers/net/usb/cdc_ncm.c b/drivers/net/usb/cdc_ncm.c
>> index 48fee7a..bcd9437 100644
>> --- a/drivers/net/usb/cdc_ncm.c
>> +++ b/drivers/net/usb/cdc_ncm.c
>> @@ -1015,7 +1015,7 @@ static struct usb_cdc_ncm_ndp16 *cdc_ncm_ndp16(struct 
>> cdc_ncm_ctx *ctx, struct s
>>   }
>>
>>   /* Allocate new SKB for use with 16-bit NCM according to actual TX/RX
>> -	settings */
>> +	settings; returns NULL in case of errors */
>
>   BTW, the preferred style for the multi-line comments in the networking 
> code is:
>
> /* bla
> * bla
> */
>
> WBR, Sergei
>
>