From: Ben Dooks <ben.dooks@codethink.co.uk> To: David Laight <David.Laight@ACULAB.COM>, "netdev@vger.kernel.org" <netdev@vger.kernel.org> Cc: "oneukum@suse.com" <oneukum@suse.com>, "davem@davemloft.net" <davem@davemloft.net>, "linux-usb@vger.kernel.org" <linux-usb@vger.kernel.org>, "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>, "linux-kernel@lists.codethink.co.uk" <linux-kernel@lists.codethink.co.uk> Subject: Re: [PATCH 2/4] usbnet: smsc95xx: align tx-buffer to word Date: Tue, 2 Oct 2018 14:35:59 +0100 [thread overview] Message-ID: <ee3ad7ff-3110-67ff-a4a4-3e647f311dbd@codethink.co.uk> (raw) In-Reply-To: <59988ed22559410881addfecf58335eb@AcuMS.aculab.com> On 02/10/18 14:19, David Laight wrote: > From: Ben Dooks >> Sent: 02 October 2018 10:27 >> >> The tegra driver requires alignment of the buffer, so try and >> make this better by pushing the buffer start back to an word >> aligned address. At the worst this makes memcpy() easier as >> it is word aligned, at best it makes sure the usb can directly >> map the buffer. >> >> Signed-off-by: Ben Dooks <ben.dooks@codethink.co.uk> >> [todo - make this configurable] >> --- >> drivers/net/usb/Kconfig | 12 ++++++++++++ >> drivers/net/usb/smsc95xx.c | 22 ++++++++++++++++++++-- >> 2 files changed, 32 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/net/usb/Kconfig b/drivers/net/usb/Kconfig > ... >> +static bool align_tx = IS_ENABLED(CONFIG_USB_NET_SMSC95XX_TXALIGN); >> +module_param(align_tx, bool, 0644); >> +MODULE_PARM_DESC(align_tx, "Align TX buffers to word boundaries"); > > DM doesn't like module parameters. > >> static bool turbo_mode = IS_ENABLED(CONFIG_USB_NET_SMSC95XX_TURBO); >> module_param(turbo_mode, bool, 0644); >> MODULE_PARM_DESC(turbo_mode, "Enable multiple frames per Rx transaction"); >> @@ -2005,10 +2009,18 @@ static struct sk_buff *smsc95xx_tx_fixup(struct usbnet *dev, >> bool csum = skb->ip_summed == CHECKSUM_PARTIAL; >> int overhead = csum ? SMSC95XX_TX_OVERHEAD_CSUM : SMSC95XX_TX_OVERHEAD; >> u32 tx_cmd_a, tx_cmd_b; >> + u32 data_len; >> + uintptr_t align = 0; >> >> /* We do not advertise SG, so skbs should be already linearized */ >> BUG_ON(skb_shinfo(skb)->nr_frags); >> >> + if (IS_ENABLED(CONFIG_USB_NET_SMSC95XX_TXALIGN) && align_tx) { >> + align = (uintptr_t)skb->data & 3; >> + if (align) >> + overhead += 4 - align; > > Better to calculate the pad size once: > align = (-(long)skb->data) & 3; > should do it - and you can unconditionally add it in. > >> + } >> + >> /* Make writable and expand header space by overhead if required */ >> if (skb_cow_head(skb, overhead)) { >> /* Must deallocate here as returning NULL to indicate error >> @@ -2037,16 +2049,22 @@ static struct sk_buff *smsc95xx_tx_fixup(struct usbnet *dev, >> } >> } >> >> + data_len = skb->len; >> + if (align) >> + skb_push(skb, 4 - align); >> + >> skb_push(skb, 4); > > You don't want to call skb_push() twice. > IIRC really horrid things happen if the data has to be copied. > (Actually what happens to the alignment in that case??) > And there is another skb_push() below.... The driver does it /multiple/ times depending on the path used. Is it wise to try and make a separate patch to skb_push() once and also move the tx_cmd_a and tx_cmd_b bit to a single point? >> - tx_cmd_b = (u32)(skb->len - 4); >> + tx_cmd_b = (u32)(data_len); > > You don't need the cast here at all (if it was ever needed). > Actually you don't need the new 'data_len' variable. > Just set tx_cmd_b earlier. > > ... > > David > > - > Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK > Registration No: 1397386 (Wales) > > -- Ben Dooks http://www.codethink.co.uk/ Senior Engineer Codethink - Providing Genius https://www.codethink.co.uk/privacy.html
WARNING: multiple messages have this Message-ID (diff)
From: Ben Dooks <ben.dooks@codethink.co.uk> To: David Laight <David.Laight@ACULAB.COM>, "netdev@vger.kernel.org" <netdev@vger.kernel.org> Cc: "oneukum@suse.com" <oneukum@suse.com>, "davem@davemloft.net" <davem@davemloft.net>, "linux-usb@vger.kernel.org" <linux-usb@vger.kernel.org>, "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>, "linux-kernel@lists.codethink.co.uk" <linux-kernel@lists.codethink.co.uk> Subject: [2/4] usbnet: smsc95xx: align tx-buffer to word Date: Tue, 2 Oct 2018 14:35:59 +0100 [thread overview] Message-ID: <ee3ad7ff-3110-67ff-a4a4-3e647f311dbd@codethink.co.uk> (raw) On 02/10/18 14:19, David Laight wrote: > From: Ben Dooks >> Sent: 02 October 2018 10:27 >> >> The tegra driver requires alignment of the buffer, so try and >> make this better by pushing the buffer start back to an word >> aligned address. At the worst this makes memcpy() easier as >> it is word aligned, at best it makes sure the usb can directly >> map the buffer. >> >> Signed-off-by: Ben Dooks <ben.dooks@codethink.co.uk> >> [todo - make this configurable] >> --- >> drivers/net/usb/Kconfig | 12 ++++++++++++ >> drivers/net/usb/smsc95xx.c | 22 ++++++++++++++++++++-- >> 2 files changed, 32 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/net/usb/Kconfig b/drivers/net/usb/Kconfig > ... >> +static bool align_tx = IS_ENABLED(CONFIG_USB_NET_SMSC95XX_TXALIGN); >> +module_param(align_tx, bool, 0644); >> +MODULE_PARM_DESC(align_tx, "Align TX buffers to word boundaries"); > > DM doesn't like module parameters. > >> static bool turbo_mode = IS_ENABLED(CONFIG_USB_NET_SMSC95XX_TURBO); >> module_param(turbo_mode, bool, 0644); >> MODULE_PARM_DESC(turbo_mode, "Enable multiple frames per Rx transaction"); >> @@ -2005,10 +2009,18 @@ static struct sk_buff *smsc95xx_tx_fixup(struct usbnet *dev, >> bool csum = skb->ip_summed == CHECKSUM_PARTIAL; >> int overhead = csum ? SMSC95XX_TX_OVERHEAD_CSUM : SMSC95XX_TX_OVERHEAD; >> u32 tx_cmd_a, tx_cmd_b; >> + u32 data_len; >> + uintptr_t align = 0; >> >> /* We do not advertise SG, so skbs should be already linearized */ >> BUG_ON(skb_shinfo(skb)->nr_frags); >> >> + if (IS_ENABLED(CONFIG_USB_NET_SMSC95XX_TXALIGN) && align_tx) { >> + align = (uintptr_t)skb->data & 3; >> + if (align) >> + overhead += 4 - align; > > Better to calculate the pad size once: > align = (-(long)skb->data) & 3; > should do it - and you can unconditionally add it in. > >> + } >> + >> /* Make writable and expand header space by overhead if required */ >> if (skb_cow_head(skb, overhead)) { >> /* Must deallocate here as returning NULL to indicate error >> @@ -2037,16 +2049,22 @@ static struct sk_buff *smsc95xx_tx_fixup(struct usbnet *dev, >> } >> } >> >> + data_len = skb->len; >> + if (align) >> + skb_push(skb, 4 - align); >> + >> skb_push(skb, 4); > > You don't want to call skb_push() twice. > IIRC really horrid things happen if the data has to be copied. > (Actually what happens to the alignment in that case??) > And there is another skb_push() below.... The driver does it /multiple/ times depending on the path used. Is it wise to try and make a separate patch to skb_push() once and also move the tx_cmd_a and tx_cmd_b bit to a single point? >> - tx_cmd_b = (u32)(skb->len - 4); >> + tx_cmd_b = (u32)(data_len); > > You don't need the cast here at all (if it was ever needed). > Actually you don't need the new 'data_len' variable. > Just set tx_cmd_b earlier. > > ... > > David > > - > Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK > Registration No: 1397386 (Wales) > >
next prev parent reply other threads:[~2018-10-02 13:43 UTC|newest] Thread overview: 41+ messages / expand[flat|nested] mbox.gz Atom feed top 2018-10-02 9:26 SMSC95XX driver updates Ben Dooks 2018-10-02 9:26 ` [PATCH 1/4] usbnet: smsc95xx: add kconfig for turbo mode Ben Dooks 2018-10-02 9:26 ` [1/4] " Ben Dooks 2018-10-02 12:46 ` [PATCH 1/4] " Andrew Lunn 2018-10-02 12:46 ` [1/4] " Andrew Lunn 2018-10-02 9:26 ` [PATCH 2/4] usbnet: smsc95xx: align tx-buffer to word Ben Dooks 2018-10-02 9:26 ` [2/4] " Ben Dooks 2018-10-02 13:19 ` [PATCH 2/4] " David Laight 2018-10-02 13:19 ` [2/4] " David Laight 2018-10-02 13:19 ` [PATCH 2/4] " David Laight 2018-10-02 13:35 ` Ben Dooks [this message] 2018-10-02 13:35 ` [2/4] " Ben Dooks 2018-10-02 13:35 ` [PATCH 2/4] " Ben Dooks 2018-10-02 16:56 ` [PATCH] usbnet: smsc95xx: simplify tx_fixup code Ben Dooks 2018-10-02 16:56 ` Ben Dooks 2018-10-03 13:36 ` [PATCH] " David Laight 2018-10-03 13:36 ` David Laight 2018-10-03 13:36 ` [PATCH] " David Laight 2018-10-03 16:25 ` Ben Dooks 2018-10-03 16:25 ` Ben Dooks 2018-10-05 21:24 ` [PATCH] " David Miller 2018-10-05 21:24 ` David Miller 2018-10-06 11:27 ` [PATCH] " Ben Dooks 2018-10-06 11:27 ` Ben Dooks 2018-10-06 17:28 ` [PATCH] " David Miller 2018-10-06 17:28 ` David Miller 2018-10-08 8:41 ` [PATCH] " David Laight 2018-10-08 8:41 ` David Laight 2018-10-08 8:41 ` [PATCH] " David Laight 2018-10-04 16:53 ` [Linux-kernel] [PATCH 2/4] usbnet: smsc95xx: align tx-buffer to word Ben Hutchings 2018-10-04 16:53 ` [2/4] " Ben Hutchings 2018-10-04 16:53 ` [PATCH 2/4] " Ben Hutchings 2018-10-02 9:26 ` [PATCH 3/4] usbnet: smsc95xx: check for csum being in last four bytes Ben Dooks 2018-10-02 9:26 ` [3/4] " Ben Dooks 2018-10-02 9:45 ` [PATCH 3/4] " Sergei Shtylyov 2018-10-02 9:45 ` [3/4] " Sergei Shtylyov 2018-10-04 16:55 ` [Linux-kernel] [PATCH 3/4] " Ben Hutchings 2018-10-04 16:55 ` [3/4] " Ben Hutchings 2018-10-04 16:55 ` [PATCH 3/4] " Ben Hutchings 2018-10-02 9:26 ` [PATCH 4/4] usbnet: smsc95xx: fix rx packet alignment Ben Dooks 2018-10-02 9:26 ` [4/4] " Ben Dooks
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=ee3ad7ff-3110-67ff-a4a4-3e647f311dbd@codethink.co.uk \ --to=ben.dooks@codethink.co.uk \ --cc=David.Laight@ACULAB.COM \ --cc=davem@davemloft.net \ --cc=linux-kernel@lists.codethink.co.uk \ --cc=linux-kernel@vger.kernel.org \ --cc=linux-usb@vger.kernel.org \ --cc=netdev@vger.kernel.org \ --cc=oneukum@suse.com \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.