| Message ID | 1308756376-9920-1-git-send-email-marius@kotsbak.com |
|---|---|
| State | Accepted, archived |
| Delegated to: | David Miller |
| Headers | show |
From: "Marius B. Kotsbak" <marius.kotsbak@gmail.com> Date: Wed, 22 Jun 2011 17:26:16 +0200 > -Support for big endian. > -Do not use USB buffers at the stack. > -Safer/more efficient code for local constants. > > Signed-off-by: Marius B. Kotsbak <marius@kotsbak.com> Applied, thanks. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hello. On 22-06-2011 19:26, Marius B. Kotsbak wrote: > -Support for big endian. > -Do not use USB buffers at the stack. > -Safer/more efficient code for local constants. > Signed-off-by: Marius B. Kotsbak<marius@kotsbak.com> > --- > drivers/net/usb/kalmia.c | 40 ++++++++++++++++++++++++---------------- > 1 files changed, 24 insertions(+), 16 deletions(-) > diff --git a/drivers/net/usb/kalmia.c b/drivers/net/usb/kalmia.c > index d965fb1..d4edeb2 100644 > --- a/drivers/net/usb/kalmia.c > +++ b/drivers/net/usb/kalmia.c > @@ -100,27 +100,35 @@ kalmia_send_init_packet(struct usbnet *dev, u8 *init_msg, u8 init_msg_len, > static int > kalmia_init_and_get_ethernet_addr(struct usbnet *dev, u8 *ethernet_addr) > { > - char init_msg_1[] = > + const static char init_msg_1[] = > { 0x57, 0x50, 0x04, 0x00, 0x00, 0x00, 0x00, 0x20, 0x00, 0x00, > 0x00, 0x00 }; > - char init_msg_2[] = > + const static char init_msg_2[] = > { 0x57, 0x50, 0x04, 0x00, 0x00, 0x00, 0x00, 0x02, 0x00, 0xf4, > 0x00, 0x00 }; Actually 'const' alone should've been enough for the variable to be placed in the data section ISO stack. > - char receive_buf[28]; > + const static int buflen = 28; Why declare it at all, when it's used only once? > + char *usb_buf; > int status; > > - status = kalmia_send_init_packet(dev, init_msg_1, sizeof(init_msg_1) > - / sizeof(init_msg_1[0]), receive_buf, 24); > + usb_buf = kmalloc(buflen, GFP_DMA | GFP_KERNEL); > + if (!usb_buf) > + return -ENOMEM; > + > + memcpy(usb_buf, init_msg_1, 12); s/12/sizeof(init_msg_1)/ > + status = kalmia_send_init_packet(dev, usb_buf, sizeof(init_msg_1) > + / sizeof(init_msg_1[0]), usb_buf, 24); There's ARRAY_SIZE() macro to replace: sizeof(init_msg_1) / sizeof(init_msg_1[0]) and why not use just sizeof(init_msg_1)? > if (status != 0) > return status; > > - status = kalmia_send_init_packet(dev, init_msg_2, sizeof(init_msg_2) > - / sizeof(init_msg_2[0]), receive_buf, 28); > + memcpy(usb_buf, init_msg_2, 12); s/12/sizeof(init_msg_2)/ > + status = kalmia_send_init_packet(dev, usb_buf, sizeof(init_msg_2) > + / sizeof(init_msg_2[0]), usb_buf, 28); The same comment here about: sizeof(init_msg_2) / sizeof(init_msg_2[0]) WBR, Sergei -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, 23 Jun 2011, Sergei Shtylyov wrote: > Hello. > > On 22-06-2011 19:26, Marius B. Kotsbak wrote: > > > -Support for big endian. > > -Do not use USB buffers at the stack. > > -Safer/more efficient code for local constants. > > > Signed-off-by: Marius B. Kotsbak<marius@kotsbak.com> > > --- > > drivers/net/usb/kalmia.c | 40 ++++++++++++++++++++++++---------------- > > 1 files changed, 24 insertions(+), 16 deletions(-) > > > diff --git a/drivers/net/usb/kalmia.c b/drivers/net/usb/kalmia.c > > index d965fb1..d4edeb2 100644 > > --- a/drivers/net/usb/kalmia.c > > +++ b/drivers/net/usb/kalmia.c > > @@ -100,27 +100,35 @@ kalmia_send_init_packet(struct usbnet *dev, u8 *init_msg, u8 init_msg_len, > > static int > > kalmia_init_and_get_ethernet_addr(struct usbnet *dev, u8 *ethernet_addr) > > { > > - char init_msg_1[] = > > + const static char init_msg_1[] = > > { 0x57, 0x50, 0x04, 0x00, 0x00, 0x00, 0x00, 0x20, 0x00, 0x00, > > 0x00, 0x00 }; > > - char init_msg_2[] = > > + const static char init_msg_2[] = > > { 0x57, 0x50, 0x04, 0x00, 0x00, 0x00, 0x00, 0x02, 0x00, 0xf4, > > 0x00, 0x00 }; > > Actually 'const' alone should've been enough for the variable to be placed > in the data section ISO stack. I don't understand what "ISO stack" means here. I would think that "const" might or might not cause the array to be allocated statically. All it really tells the compiler is that the function will not change the contents of the array. To be certain that the array is allocated statically (not on the stack), it's best to use "static". Alan Stern -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Alan Stern wrote: >>> -Support for big endian. >>> -Do not use USB buffers at the stack. >>> -Safer/more efficient code for local constants. >>> Signed-off-by: Marius B. Kotsbak<marius@kotsbak.com> >>> --- >>> drivers/net/usb/kalmia.c | 40 ++++++++++++++++++++++++---------------- >>> 1 files changed, 24 insertions(+), 16 deletions(-) >>> diff --git a/drivers/net/usb/kalmia.c b/drivers/net/usb/kalmia.c >>> index d965fb1..d4edeb2 100644 >>> --- a/drivers/net/usb/kalmia.c >>> +++ b/drivers/net/usb/kalmia.c >>> @@ -100,27 +100,35 @@ kalmia_send_init_packet(struct usbnet *dev, u8 *init_msg, u8 init_msg_len, >>> static int >>> kalmia_init_and_get_ethernet_addr(struct usbnet *dev, u8 *ethernet_addr) >>> { >>> - char init_msg_1[] = >>> + const static char init_msg_1[] = >>> { 0x57, 0x50, 0x04, 0x00, 0x00, 0x00, 0x00, 0x20, 0x00, 0x00, >>> 0x00, 0x00 }; >>> - char init_msg_2[] = >>> + const static char init_msg_2[] = >>> { 0x57, 0x50, 0x04, 0x00, 0x00, 0x00, 0x00, 0x02, 0x00, 0xf4, >>> 0x00, 0x00 }; >> Actually 'const' alone should've been enough for the variable to be placed >> in the data section ISO stack. > I don't understand what "ISO stack" means here. ISO == instead of. > I would think that "const" might or might not cause the array to be > allocated statically. At least with gcc, it will be allocated statically. > Alan Stern WBR, Sergei -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Den 23. juni 2011 12:46, skrev Sergei Shtylyov: > Hello. > > On 22-06-2011 19:26, Marius B. Kotsbak wrote: > >> -Support for big endian. >> -Do not use USB buffers at the stack. >> -Safer/more efficient code for local constants. > >> Signed-off-by: Marius B. Kotsbak<marius@kotsbak.com> >> --- >> drivers/net/usb/kalmia.c | 40 >> ++++++++++++++++++++++++---------------- >> 1 files changed, 24 insertions(+), 16 deletions(-) > > >> - char receive_buf[28]; >> + const static int buflen = 28; > > Why declare it at all, when it's used only once? > >> + char *usb_buf; >> int status; >> >> - status = kalmia_send_init_packet(dev, init_msg_1, >> sizeof(init_msg_1) >> - / sizeof(init_msg_1[0]), receive_buf, 24); >> + usb_buf = kmalloc(buflen, GFP_DMA | GFP_KERNEL); >> + if (!usb_buf) >> + return -ENOMEM; >> + >> + memcpy(usb_buf, init_msg_1, 12); > > s/12/sizeof(init_msg_1)/ > >> + status = kalmia_send_init_packet(dev, usb_buf, sizeof(init_msg_1) >> + / sizeof(init_msg_1[0]), usb_buf, 24); > > There's ARRAY_SIZE() macro to replace: > > sizeof(init_msg_1) / sizeof(init_msg_1[0]) > > and why not use just sizeof(init_msg_1)? > >> if (status != 0) >> return status; >> >> - status = kalmia_send_init_packet(dev, init_msg_2, >> sizeof(init_msg_2) >> - / sizeof(init_msg_2[0]), receive_buf, 28); >> + memcpy(usb_buf, init_msg_2, 12); > > s/12/sizeof(init_msg_2)/ > >> + status = kalmia_send_init_packet(dev, usb_buf, sizeof(init_msg_2) >> + / sizeof(init_msg_2[0]), usb_buf, 28); > > The same comment here about: > > sizeof(init_msg_2) / sizeof(init_msg_2[0]) > Thanks for the tips. I know some parts of the code are a bit ugly, but the primary goal was to get it working despite the quirky state of the current modem firmware. I have noted it for later fixing. -- Marius -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Am Donnerstag, 23. Juni 2011, 16:55:12 schrieb Sergei Shtylyov: > >> Actually 'const' alone should've been enough for the variable to be placed > >> in the data section ISO stack. > > > I don't understand what "ISO stack" means here. > > ISO == instead of. That doesn't matter. If you are sufficiently unlucky it will land on a page boundary. You must not do DMA on the data section either. Regards Oliver -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hello. On 24-06-2011 9:42, Oliver Neukum wrote: >>>> Actually 'const' alone should've been enough for the variable to be placed >>>> in the data section ISO stack. >>> I don't understand what "ISO stack" means here. >> ISO == instead of. > That doesn't matter. If you are sufficiently unlucky it will land on > a page boundary. You must not do DMA on the data section either. Who said it was going to do DMA on the data section? The arrays got copied to heap-allocated buffer before DMA. But allocating those arrays on stack was not optimal anyway. > Regards > Oliver > WBR, Sergei -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/net/usb/kalmia.c b/drivers/net/usb/kalmia.c index d965fb1..d4edeb2 100644 --- a/drivers/net/usb/kalmia.c +++ b/drivers/net/usb/kalmia.c @@ -100,27 +100,35 @@ kalmia_send_init_packet(struct usbnet *dev, u8 *init_msg, u8 init_msg_len, static int kalmia_init_and_get_ethernet_addr(struct usbnet *dev, u8 *ethernet_addr) { - char init_msg_1[] = + const static char init_msg_1[] = { 0x57, 0x50, 0x04, 0x00, 0x00, 0x00, 0x00, 0x20, 0x00, 0x00, 0x00, 0x00 }; - char init_msg_2[] = + const static char init_msg_2[] = { 0x57, 0x50, 0x04, 0x00, 0x00, 0x00, 0x00, 0x02, 0x00, 0xf4, 0x00, 0x00 }; - char receive_buf[28]; + const static int buflen = 28; + char *usb_buf; int status; - status = kalmia_send_init_packet(dev, init_msg_1, sizeof(init_msg_1) - / sizeof(init_msg_1[0]), receive_buf, 24); + usb_buf = kmalloc(buflen, GFP_DMA | GFP_KERNEL); + if (!usb_buf) + return -ENOMEM; + + memcpy(usb_buf, init_msg_1, 12); + status = kalmia_send_init_packet(dev, usb_buf, sizeof(init_msg_1) + / sizeof(init_msg_1[0]), usb_buf, 24); if (status != 0) return status; - status = kalmia_send_init_packet(dev, init_msg_2, sizeof(init_msg_2) - / sizeof(init_msg_2[0]), receive_buf, 28); + memcpy(usb_buf, init_msg_2, 12); + status = kalmia_send_init_packet(dev, usb_buf, sizeof(init_msg_2) + / sizeof(init_msg_2[0]), usb_buf, 28); if (status != 0) return status; - memcpy(ethernet_addr, receive_buf + 10, ETH_ALEN); + memcpy(ethernet_addr, usb_buf + 10, ETH_ALEN); + kfree(usb_buf); return status; } @@ -190,7 +198,8 @@ kalmia_tx_fixup(struct usbnet *dev, struct sk_buff *skb, gfp_t flags) dev_kfree_skb_any(skb); skb = skb2; - done: header_start = skb_push(skb, KALMIA_HEADER_LENGTH); +done: + header_start = skb_push(skb, KALMIA_HEADER_LENGTH); ether_type_1 = header_start[KALMIA_HEADER_LENGTH + 12]; ether_type_2 = header_start[KALMIA_HEADER_LENGTH + 13]; @@ -201,9 +210,8 @@ kalmia_tx_fixup(struct usbnet *dev, struct sk_buff *skb, gfp_t flags) header_start[0] = 0x57; header_start[1] = 0x44; content_len = skb->len - KALMIA_HEADER_LENGTH; - header_start[2] = (content_len & 0xff); /* low byte */ - header_start[3] = (content_len >> 8); /* high byte */ + put_unaligned_le16(content_len, &header_start[2]); header_start[4] = ether_type_1; header_start[5] = ether_type_2; @@ -231,13 +239,13 @@ kalmia_rx_fixup(struct usbnet *dev, struct sk_buff *skb) * Our task here is to strip off framing, leaving skb with one * data frame for the usbnet framework code to process. */ - const u8 HEADER_END_OF_USB_PACKET[] = + const static u8 HEADER_END_OF_USB_PACKET[] = { 0x57, 0x5a, 0x00, 0x00, 0x08, 0x00 }; - const u8 EXPECTED_UNKNOWN_HEADER_1[] = + const static u8 EXPECTED_UNKNOWN_HEADER_1[] = { 0x57, 0x43, 0x1e, 0x00, 0x15, 0x02 }; - const u8 EXPECTED_UNKNOWN_HEADER_2[] = + const static u8 EXPECTED_UNKNOWN_HEADER_2[] = { 0x57, 0x50, 0x0e, 0x00, 0x00, 0x00 }; - u8 i = 0; + int i = 0; /* incomplete header? */ if (skb->len < KALMIA_HEADER_LENGTH) @@ -285,7 +293,7 @@ kalmia_rx_fixup(struct usbnet *dev, struct sk_buff *skb) /* subtract start header and end header */ usb_packet_length = skb->len - (2 * KALMIA_HEADER_LENGTH); - ether_packet_length = header_start[2] + (header_start[3] << 8); + ether_packet_length = get_unaligned_le16(&header_start[2]); skb_pull(skb, KALMIA_HEADER_LENGTH); /* Some small packets misses end marker */
-Support for big endian. -Do not use USB buffers at the stack. -Safer/more efficient code for local constants. Signed-off-by: Marius B. Kotsbak <marius@kotsbak.com> --- drivers/net/usb/kalmia.c | 40 ++++++++++++++++++++++++---------------- 1 files changed, 24 insertions(+), 16 deletions(-)