* [2/4] usbnet: smsc95xx: align tx-buffer to word
@ 2018-10-02 13:19 ` David Laight
0 siblings, 0 replies; 41+ messages in thread
From: David Laight @ 2018-10-02 13:19 UTC (permalink / raw)
To: 'Ben Dooks', netdev
Cc: oneukum, davem, linux-usb, linux-kernel, linux-kernel
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....
> - 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)
^ permalink raw reply [flat|nested] 41+ messages in thread* RE: [PATCH 2/4] usbnet: smsc95xx: align tx-buffer to word
@ 2018-10-02 13:19 ` David Laight
0 siblings, 0 replies; 41+ messages in thread
From: David Laight @ 2018-10-02 13:19 UTC (permalink / raw)
To: 'Ben Dooks', netdev
Cc: oneukum, davem, linux-usb, linux-kernel, linux-kernel
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....
> - 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)
^ permalink raw reply [flat|nested] 41+ messages in thread* Re: [PATCH 2/4] usbnet: smsc95xx: align tx-buffer to word
2018-10-02 13:19 ` [PATCH 2/4] " David Laight
(?)
@ 2018-10-02 13:35 ` Ben Dooks
-1 siblings, 0 replies; 41+ messages in thread
From: Ben Dooks @ 2018-10-02 13:35 UTC (permalink / raw)
To: David Laight, netdev
Cc: oneukum, davem, linux-usb, linux-kernel, linux-kernel
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
^ permalink raw reply [flat|nested] 41+ messages in thread* [2/4] usbnet: smsc95xx: align tx-buffer to word
@ 2018-10-02 13:35 ` Ben Dooks
0 siblings, 0 replies; 41+ messages in thread
From: Ben Dooks @ 2018-10-02 13:35 UTC (permalink / raw)
To: David Laight, netdev
Cc: oneukum, davem, linux-usb, linux-kernel, linux-kernel
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)
>
>
^ permalink raw reply [flat|nested] 41+ messages in thread* Re: [PATCH 2/4] usbnet: smsc95xx: align tx-buffer to word
@ 2018-10-02 13:35 ` Ben Dooks
0 siblings, 0 replies; 41+ messages in thread
From: Ben Dooks @ 2018-10-02 13:35 UTC (permalink / raw)
To: David Laight, netdev
Cc: oneukum, davem, linux-usb, linux-kernel, linux-kernel
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
^ permalink raw reply [flat|nested] 41+ messages in thread
* [PATCH] usbnet: smsc95xx: simplify tx_fixup code
@ 2018-10-02 16:56 ` Ben Dooks
0 siblings, 0 replies; 41+ messages in thread
From: Ben Dooks @ 2018-10-02 16:56 UTC (permalink / raw)
To: netdev
Cc: David.Laight, oneukum, davem, linux-usb, linux-kernel,
linux-kernel, Ben Dooks
The smsc95xx_tx_fixup is doing multiple calls to skb_push() to
put an 8-byte command header onto the packet. It would be easier
to do one skb_push() and then copy the data in once the push is
done.
Signed-off-by: Ben Dooks <ben.dooks@codethink.co.uk>
---
drivers/net/usb/smsc95xx.c | 25 +++++++++++++------------
1 file changed, 13 insertions(+), 12 deletions(-)
diff --git a/drivers/net/usb/smsc95xx.c b/drivers/net/usb/smsc95xx.c
index cb19aea139d3..813ab93ee2c3 100644
--- a/drivers/net/usb/smsc95xx.c
+++ b/drivers/net/usb/smsc95xx.c
@@ -2006,6 +2006,7 @@ 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;
+ void *ptr;
/* We do not advertise SG, so skbs should be already linearized */
BUG_ON(skb_shinfo(skb)->nr_frags);
@@ -2019,6 +2020,9 @@ static struct sk_buff *smsc95xx_tx_fixup(struct usbnet *dev,
return NULL;
}
+ tx_cmd_b = (u32)skb->len;
+ tx_cmd_a = tx_cmd_b | TX_CMD_A_FIRST_SEG_ | TX_CMD_A_LAST_SEG_;
+
if (csum) {
if (skb->len <= 45) {
/* workaround - hardware tx checksum does not work
@@ -2035,21 +2039,18 @@ static struct sk_buff *smsc95xx_tx_fixup(struct usbnet *dev,
skb_push(skb, 4);
cpu_to_le32s(&csum_preamble);
memcpy(skb->data, &csum_preamble, 4);
+
+ tx_cmd_a += 4;
+ tx_cmd_b += 4;
+ tx_cmd_b |= TX_CMD_B_CSUM_ENABLE;
}
}
- skb_push(skb, 4);
- tx_cmd_b = (u32)(skb->len - 4);
- if (csum)
- tx_cmd_b |= TX_CMD_B_CSUM_ENABLE;
- cpu_to_le32s(&tx_cmd_b);
- memcpy(skb->data, &tx_cmd_b, 4);
-
- skb_push(skb, 4);
- tx_cmd_a = (u32)(skb->len - 8) | TX_CMD_A_FIRST_SEG_ |
- TX_CMD_A_LAST_SEG_;
- cpu_to_le32s(&tx_cmd_a);
- memcpy(skb->data, &tx_cmd_a, 4);
+ ptr = skb_push(skb, 8);
+ tx_cmd_a = cpu_to_le32(tx_cmd_a);
+ tx_cmd_b = cpu_to_le32(tx_cmd_b);
+ memcpy(ptr, &tx_cmd_a, 4);
+ memcpy(ptr+4, &tx_cmd_b, 4);
return skb;
}
--
2.19.0
^ permalink raw reply related [flat|nested] 41+ messages in thread* usbnet: smsc95xx: simplify tx_fixup code
@ 2018-10-02 16:56 ` Ben Dooks
0 siblings, 0 replies; 41+ messages in thread
From: Ben Dooks @ 2018-10-02 16:56 UTC (permalink / raw)
To: netdev
Cc: David.Laight, oneukum, davem, linux-usb, linux-kernel,
linux-kernel, Ben Dooks
The smsc95xx_tx_fixup is doing multiple calls to skb_push() to
put an 8-byte command header onto the packet. It would be easier
to do one skb_push() and then copy the data in once the push is
done.
Signed-off-by: Ben Dooks <ben.dooks@codethink.co.uk>
---
drivers/net/usb/smsc95xx.c | 25 +++++++++++++------------
1 file changed, 13 insertions(+), 12 deletions(-)
diff --git a/drivers/net/usb/smsc95xx.c b/drivers/net/usb/smsc95xx.c
index cb19aea139d3..813ab93ee2c3 100644
--- a/drivers/net/usb/smsc95xx.c
+++ b/drivers/net/usb/smsc95xx.c
@@ -2006,6 +2006,7 @@ 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;
+ void *ptr;
/* We do not advertise SG, so skbs should be already linearized */
BUG_ON(skb_shinfo(skb)->nr_frags);
@@ -2019,6 +2020,9 @@ static struct sk_buff *smsc95xx_tx_fixup(struct usbnet *dev,
return NULL;
}
+ tx_cmd_b = (u32)skb->len;
+ tx_cmd_a = tx_cmd_b | TX_CMD_A_FIRST_SEG_ | TX_CMD_A_LAST_SEG_;
+
if (csum) {
if (skb->len <= 45) {
/* workaround - hardware tx checksum does not work
@@ -2035,21 +2039,18 @@ static struct sk_buff *smsc95xx_tx_fixup(struct usbnet *dev,
skb_push(skb, 4);
cpu_to_le32s(&csum_preamble);
memcpy(skb->data, &csum_preamble, 4);
+
+ tx_cmd_a += 4;
+ tx_cmd_b += 4;
+ tx_cmd_b |= TX_CMD_B_CSUM_ENABLE;
}
}
- skb_push(skb, 4);
- tx_cmd_b = (u32)(skb->len - 4);
- if (csum)
- tx_cmd_b |= TX_CMD_B_CSUM_ENABLE;
- cpu_to_le32s(&tx_cmd_b);
- memcpy(skb->data, &tx_cmd_b, 4);
-
- skb_push(skb, 4);
- tx_cmd_a = (u32)(skb->len - 8) | TX_CMD_A_FIRST_SEG_ |
- TX_CMD_A_LAST_SEG_;
- cpu_to_le32s(&tx_cmd_a);
- memcpy(skb->data, &tx_cmd_a, 4);
+ ptr = skb_push(skb, 8);
+ tx_cmd_a = cpu_to_le32(tx_cmd_a);
+ tx_cmd_b = cpu_to_le32(tx_cmd_b);
+ memcpy(ptr, &tx_cmd_a, 4);
+ memcpy(ptr+4, &tx_cmd_b, 4);
return skb;
}
^ permalink raw reply related [flat|nested] 41+ messages in thread* RE: [PATCH] usbnet: smsc95xx: simplify tx_fixup code
2018-10-02 16:56 ` Ben Dooks
(?)
@ 2018-10-03 13:36 ` David Laight
-1 siblings, 0 replies; 41+ messages in thread
From: David Laight @ 2018-10-03 13:36 UTC (permalink / raw)
To: 'Ben Dooks', netdev
Cc: oneukum, davem, linux-usb, linux-kernel, linux-kernel
From: Ben Dooks
> Sent: 02 October 2018 17:56
>
> The smsc95xx_tx_fixup is doing multiple calls to skb_push() to
> put an 8-byte command header onto the packet. It would be easier
> to do one skb_push() and then copy the data in once the push is
> done.
>
> Signed-off-by: Ben Dooks <ben.dooks@codethink.co.uk>
> ---
> drivers/net/usb/smsc95xx.c | 25 +++++++++++++------------
> 1 file changed, 13 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/net/usb/smsc95xx.c b/drivers/net/usb/smsc95xx.c
> index cb19aea139d3..813ab93ee2c3 100644
> --- a/drivers/net/usb/smsc95xx.c
> +++ b/drivers/net/usb/smsc95xx.c
> @@ -2006,6 +2006,7 @@ 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;
> + void *ptr;
It might be useful to define a structure for the header.
You might need to find the 'store unaligned 32bit word' macro though.
(Actually that will probably be better than the memcpy() which might
end up doing memory-memory copies rather than storing the register.)
Although if/when you add the tx alignment that won't be needed because the
header will be aligned.
> /* We do not advertise SG, so skbs should be already linearized */
> BUG_ON(skb_shinfo(skb)->nr_frags);
> @@ -2019,6 +2020,9 @@ static struct sk_buff *smsc95xx_tx_fixup(struct usbnet *dev,
> return NULL;
> }
>
> + tx_cmd_b = (u32)skb->len;
> + tx_cmd_a = tx_cmd_b | TX_CMD_A_FIRST_SEG_ | TX_CMD_A_LAST_SEG_;
> +
> if (csum) {
> if (skb->len <= 45) {
> /* workaround - hardware tx checksum does not work
> @@ -2035,21 +2039,18 @@ static struct sk_buff *smsc95xx_tx_fixup(struct usbnet *dev,
> skb_push(skb, 4);
> cpu_to_le32s(&csum_preamble);
Not related, but csum_preamble = cpu_to_le32(csum_preamble) is likely to
generate better code (at least for some architectures).
David
-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)
^ permalink raw reply [flat|nested] 41+ messages in thread* usbnet: smsc95xx: simplify tx_fixup code
@ 2018-10-03 13:36 ` David Laight
0 siblings, 0 replies; 41+ messages in thread
From: David Laight @ 2018-10-03 13:36 UTC (permalink / raw)
To: 'Ben Dooks', netdev
Cc: oneukum, davem, linux-usb, linux-kernel, linux-kernel
From: Ben Dooks
> Sent: 02 October 2018 17:56
>
> The smsc95xx_tx_fixup is doing multiple calls to skb_push() to
> put an 8-byte command header onto the packet. It would be easier
> to do one skb_push() and then copy the data in once the push is
> done.
>
> Signed-off-by: Ben Dooks <ben.dooks@codethink.co.uk>
> ---
> drivers/net/usb/smsc95xx.c | 25 +++++++++++++------------
> 1 file changed, 13 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/net/usb/smsc95xx.c b/drivers/net/usb/smsc95xx.c
> index cb19aea139d3..813ab93ee2c3 100644
> --- a/drivers/net/usb/smsc95xx.c
> +++ b/drivers/net/usb/smsc95xx.c
> @@ -2006,6 +2006,7 @@ 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;
> + void *ptr;
It might be useful to define a structure for the header.
You might need to find the 'store unaligned 32bit word' macro though.
(Actually that will probably be better than the memcpy() which might
end up doing memory-memory copies rather than storing the register.)
Although if/when you add the tx alignment that won't be needed because the
header will be aligned.
> /* We do not advertise SG, so skbs should be already linearized */
> BUG_ON(skb_shinfo(skb)->nr_frags);
> @@ -2019,6 +2020,9 @@ static struct sk_buff *smsc95xx_tx_fixup(struct usbnet *dev,
> return NULL;
> }
>
> + tx_cmd_b = (u32)skb->len;
> + tx_cmd_a = tx_cmd_b | TX_CMD_A_FIRST_SEG_ | TX_CMD_A_LAST_SEG_;
> +
> if (csum) {
> if (skb->len <= 45) {
> /* workaround - hardware tx checksum does not work
> @@ -2035,21 +2039,18 @@ static struct sk_buff *smsc95xx_tx_fixup(struct usbnet *dev,
> skb_push(skb, 4);
> cpu_to_le32s(&csum_preamble);
Not related, but csum_preamble = cpu_to_le32(csum_preamble) is likely to
generate better code (at least for some architectures).
David
-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)
^ permalink raw reply [flat|nested] 41+ messages in thread* RE: [PATCH] usbnet: smsc95xx: simplify tx_fixup code
@ 2018-10-03 13:36 ` David Laight
0 siblings, 0 replies; 41+ messages in thread
From: David Laight @ 2018-10-03 13:36 UTC (permalink / raw)
To: 'Ben Dooks', netdev
Cc: oneukum, davem, linux-usb, linux-kernel, linux-kernel
From: Ben Dooks
> Sent: 02 October 2018 17:56
>
> The smsc95xx_tx_fixup is doing multiple calls to skb_push() to
> put an 8-byte command header onto the packet. It would be easier
> to do one skb_push() and then copy the data in once the push is
> done.
>
> Signed-off-by: Ben Dooks <ben.dooks@codethink.co.uk>
> ---
> drivers/net/usb/smsc95xx.c | 25 +++++++++++++------------
> 1 file changed, 13 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/net/usb/smsc95xx.c b/drivers/net/usb/smsc95xx.c
> index cb19aea139d3..813ab93ee2c3 100644
> --- a/drivers/net/usb/smsc95xx.c
> +++ b/drivers/net/usb/smsc95xx.c
> @@ -2006,6 +2006,7 @@ 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;
> + void *ptr;
It might be useful to define a structure for the header.
You might need to find the 'store unaligned 32bit word' macro though.
(Actually that will probably be better than the memcpy() which might
end up doing memory-memory copies rather than storing the register.)
Although if/when you add the tx alignment that won't be needed because the
header will be aligned.
> /* We do not advertise SG, so skbs should be already linearized */
> BUG_ON(skb_shinfo(skb)->nr_frags);
> @@ -2019,6 +2020,9 @@ static struct sk_buff *smsc95xx_tx_fixup(struct usbnet *dev,
> return NULL;
> }
>
> + tx_cmd_b = (u32)skb->len;
> + tx_cmd_a = tx_cmd_b | TX_CMD_A_FIRST_SEG_ | TX_CMD_A_LAST_SEG_;
> +
> if (csum) {
> if (skb->len <= 45) {
> /* workaround - hardware tx checksum does not work
> @@ -2035,21 +2039,18 @@ static struct sk_buff *smsc95xx_tx_fixup(struct usbnet *dev,
> skb_push(skb, 4);
> cpu_to_le32s(&csum_preamble);
Not related, but csum_preamble = cpu_to_le32(csum_preamble) is likely to
generate better code (at least for some architectures).
David
-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)
^ permalink raw reply [flat|nested] 41+ messages in thread* RE: [PATCH] usbnet: smsc95xx: simplify tx_fixup code
@ 2018-10-03 16:25 ` Ben Dooks
0 siblings, 0 replies; 41+ messages in thread
From: Ben Dooks @ 2018-10-03 16:25 UTC (permalink / raw)
To: David Laight
Cc: netdev, oneukum, davem, linux-usb, linux-kernel, linux-kernel
On 2018-10-03 14:36, David Laight wrote:
> From: Ben Dooks
>> Sent: 02 October 2018 17:56
>>
>> The smsc95xx_tx_fixup is doing multiple calls to skb_push() to
>> put an 8-byte command header onto the packet. It would be easier
>> to do one skb_push() and then copy the data in once the push is
>> done.
>>
>> Signed-off-by: Ben Dooks <ben.dooks@codethink.co.uk>
>> ---
>> drivers/net/usb/smsc95xx.c | 25 +++++++++++++------------
>> 1 file changed, 13 insertions(+), 12 deletions(-)
>>
>> diff --git a/drivers/net/usb/smsc95xx.c b/drivers/net/usb/smsc95xx.c
>> index cb19aea139d3..813ab93ee2c3 100644
>> --- a/drivers/net/usb/smsc95xx.c
>> +++ b/drivers/net/usb/smsc95xx.c
>> @@ -2006,6 +2006,7 @@ 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;
>> + void *ptr;
>
> It might be useful to define a structure for the header.
> You might need to find the 'store unaligned 32bit word' macro though.
> (Actually that will probably be better than the memcpy() which might
> end up doing memory-memory copies rather than storing the register.)
> Although if/when you add the tx alignment that won't be needed because
> the
> header will be aligned.
Ok, might be worth doing.
I did try to do a "u32 tx_cmd[2]" but the code generated ended up
storing
stuff onto the stack before copying into the packet. I agree that
possibly
going to the "put_unaligned" function might be nicer too.
If we did enable tx-align all the time then we'd not have to care about
the
alignment, but I didn't want to do that if possible as that would end up
sending up to 3 bytes extra per packet.
I am trying not too do too many changes at one time to allow roll back.
>> /* We do not advertise SG, so skbs should be already linearized */
>> BUG_ON(skb_shinfo(skb)->nr_frags);
>> @@ -2019,6 +2020,9 @@ static struct sk_buff *smsc95xx_tx_fixup(struct
>> usbnet *dev,
>> return NULL;
>> }
>>
>> + tx_cmd_b = (u32)skb->len;
>> + tx_cmd_a = tx_cmd_b | TX_CMD_A_FIRST_SEG_ | TX_CMD_A_LAST_SEG_;
>> +
>> if (csum) {
>> if (skb->len <= 45) {
>> /* workaround - hardware tx checksum does not work
>> @@ -2035,21 +2039,18 @@ static struct sk_buff
>> *smsc95xx_tx_fixup(struct usbnet *dev,
>> skb_push(skb, 4);
>> cpu_to_le32s(&csum_preamble);
>
> Not related, but csum_preamble = cpu_to_le32(csum_preamble) is likely
> to
> generate better code (at least for some architectures).
>
> David
>
> -
> Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes,
> MK1 1PT, UK
> Registration No: 1397386 (Wales)
^ permalink raw reply [flat|nested] 41+ messages in thread* usbnet: smsc95xx: simplify tx_fixup code
@ 2018-10-03 16:25 ` Ben Dooks
0 siblings, 0 replies; 41+ messages in thread
From: Ben Dooks @ 2018-10-03 16:25 UTC (permalink / raw)
To: David Laight
Cc: netdev, oneukum, davem, linux-usb, linux-kernel, linux-kernel
On 2018-10-03 14:36, David Laight wrote:
> From: Ben Dooks
>> Sent: 02 October 2018 17:56
>>
>> The smsc95xx_tx_fixup is doing multiple calls to skb_push() to
>> put an 8-byte command header onto the packet. It would be easier
>> to do one skb_push() and then copy the data in once the push is
>> done.
>>
>> Signed-off-by: Ben Dooks <ben.dooks@codethink.co.uk>
>> ---
>> drivers/net/usb/smsc95xx.c | 25 +++++++++++++------------
>> 1 file changed, 13 insertions(+), 12 deletions(-)
>>
>> diff --git a/drivers/net/usb/smsc95xx.c b/drivers/net/usb/smsc95xx.c
>> index cb19aea139d3..813ab93ee2c3 100644
>> --- a/drivers/net/usb/smsc95xx.c
>> +++ b/drivers/net/usb/smsc95xx.c
>> @@ -2006,6 +2006,7 @@ 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;
>> + void *ptr;
>
> It might be useful to define a structure for the header.
> You might need to find the 'store unaligned 32bit word' macro though.
> (Actually that will probably be better than the memcpy() which might
> end up doing memory-memory copies rather than storing the register.)
> Although if/when you add the tx alignment that won't be needed because
> the
> header will be aligned.
Ok, might be worth doing.
I did try to do a "u32 tx_cmd[2]" but the code generated ended up
storing
stuff onto the stack before copying into the packet. I agree that
possibly
going to the "put_unaligned" function might be nicer too.
If we did enable tx-align all the time then we'd not have to care about
the
alignment, but I didn't want to do that if possible as that would end up
sending up to 3 bytes extra per packet.
I am trying not too do too many changes at one time to allow roll back.
>> /* We do not advertise SG, so skbs should be already linearized */
>> BUG_ON(skb_shinfo(skb)->nr_frags);
>> @@ -2019,6 +2020,9 @@ static struct sk_buff *smsc95xx_tx_fixup(struct
>> usbnet *dev,
>> return NULL;
>> }
>>
>> + tx_cmd_b = (u32)skb->len;
>> + tx_cmd_a = tx_cmd_b | TX_CMD_A_FIRST_SEG_ | TX_CMD_A_LAST_SEG_;
>> +
>> if (csum) {
>> if (skb->len <= 45) {
>> /* workaround - hardware tx checksum does not work
>> @@ -2035,21 +2039,18 @@ static struct sk_buff
>> *smsc95xx_tx_fixup(struct usbnet *dev,
>> skb_push(skb, 4);
>> cpu_to_le32s(&csum_preamble);
>
> Not related, but csum_preamble = cpu_to_le32(csum_preamble) is likely
> to
> generate better code (at least for some architectures).
>
> David
>
> -
> Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes,
> MK1 1PT, UK
> Registration No: 1397386 (Wales)
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH] usbnet: smsc95xx: simplify tx_fixup code
@ 2018-10-05 21:24 ` David Miller
0 siblings, 0 replies; 41+ messages in thread
From: David Miller @ 2018-10-05 21:24 UTC (permalink / raw)
To: ben.dooks
Cc: netdev, David.Laight, oneukum, linux-usb, linux-kernel, linux-kernel
From: Ben Dooks <ben.dooks@codethink.co.uk>
Date: Tue, 2 Oct 2018 17:56:02 +0100
> - memcpy(skb->data, &tx_cmd_a, 4);
> + ptr = skb_push(skb, 8);
> + tx_cmd_a = cpu_to_le32(tx_cmd_a);
> + tx_cmd_b = cpu_to_le32(tx_cmd_b);
> + memcpy(ptr, &tx_cmd_a, 4);
> + memcpy(ptr+4, &tx_cmd_b, 4);
Even a memcpy() through a void pointer does not guarantee that gcc will
not emit word sized loads and stores.
You must use the get_unaligned()/put_unaligned() facilities to do this
properly.
I also agree that making a proper type and structure instead of using
a void pointer would be better.
^ permalink raw reply [flat|nested] 41+ messages in thread
* usbnet: smsc95xx: simplify tx_fixup code
@ 2018-10-05 21:24 ` David Miller
0 siblings, 0 replies; 41+ messages in thread
From: David Miller @ 2018-10-05 21:24 UTC (permalink / raw)
To: ben.dooks
Cc: netdev, David.Laight, oneukum, linux-usb, linux-kernel, linux-kernel
From: Ben Dooks <ben.dooks@codethink.co.uk>
Date: Tue, 2 Oct 2018 17:56:02 +0100
> - memcpy(skb->data, &tx_cmd_a, 4);
> + ptr = skb_push(skb, 8);
> + tx_cmd_a = cpu_to_le32(tx_cmd_a);
> + tx_cmd_b = cpu_to_le32(tx_cmd_b);
> + memcpy(ptr, &tx_cmd_a, 4);
> + memcpy(ptr+4, &tx_cmd_b, 4);
Even a memcpy() through a void pointer does not guarantee that gcc will
not emit word sized loads and stores.
You must use the get_unaligned()/put_unaligned() facilities to do this
properly.
I also agree that making a proper type and structure instead of using
a void pointer would be better.
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH] usbnet: smsc95xx: simplify tx_fixup code
@ 2018-10-06 11:27 ` Ben Dooks
0 siblings, 0 replies; 41+ messages in thread
From: Ben Dooks @ 2018-10-06 11:27 UTC (permalink / raw)
To: David Miller
Cc: netdev, David.Laight, oneukum, linux-usb, linux-kernel, linux-kernel
On 2018-10-05 22:24, David Miller wrote:
> From: Ben Dooks <ben.dooks@codethink.co.uk>
> Date: Tue, 2 Oct 2018 17:56:02 +0100
>
>> - memcpy(skb->data, &tx_cmd_a, 4);
>> + ptr = skb_push(skb, 8);
>> + tx_cmd_a = cpu_to_le32(tx_cmd_a);
>> + tx_cmd_b = cpu_to_le32(tx_cmd_b);
>> + memcpy(ptr, &tx_cmd_a, 4);
>> + memcpy(ptr+4, &tx_cmd_b, 4);
>
> Even a memcpy() through a void pointer does not guarantee that gcc will
> not emit word sized loads and stores.
>
> You must use the get_unaligned()/put_unaligned() facilities to do this
> properly.
Thanks, got a new version of the series just being tested with this.
Should it go into the original, or as a separate change?
>
> I also agree that making a proper type and structure instead of using
> a void pointer would be better.
^ permalink raw reply [flat|nested] 41+ messages in thread
* usbnet: smsc95xx: simplify tx_fixup code
@ 2018-10-06 11:27 ` Ben Dooks
0 siblings, 0 replies; 41+ messages in thread
From: Ben Dooks @ 2018-10-06 11:27 UTC (permalink / raw)
To: David Miller
Cc: netdev, David.Laight, oneukum, linux-usb, linux-kernel, linux-kernel
On 2018-10-05 22:24, David Miller wrote:
> From: Ben Dooks <ben.dooks@codethink.co.uk>
> Date: Tue, 2 Oct 2018 17:56:02 +0100
>
>> - memcpy(skb->data, &tx_cmd_a, 4);
>> + ptr = skb_push(skb, 8);
>> + tx_cmd_a = cpu_to_le32(tx_cmd_a);
>> + tx_cmd_b = cpu_to_le32(tx_cmd_b);
>> + memcpy(ptr, &tx_cmd_a, 4);
>> + memcpy(ptr+4, &tx_cmd_b, 4);
>
> Even a memcpy() through a void pointer does not guarantee that gcc will
> not emit word sized loads and stores.
>
> You must use the get_unaligned()/put_unaligned() facilities to do this
> properly.
Thanks, got a new version of the series just being tested with this.
Should it go into the original, or as a separate change?
>
> I also agree that making a proper type and structure instead of using
> a void pointer would be better.
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH] usbnet: smsc95xx: simplify tx_fixup code
@ 2018-10-06 17:28 ` David Miller
0 siblings, 0 replies; 41+ messages in thread
From: David Miller @ 2018-10-06 17:28 UTC (permalink / raw)
To: ben.dooks
Cc: netdev, David.Laight, oneukum, linux-usb, linux-kernel, linux-kernel
From: Ben Dooks <ben.dooks@codethink.co.uk>
Date: Sat, 06 Oct 2018 12:27:27 +0100
> Thanks, got a new version of the series just being tested with this.
> Should it go into the original, or as a separate change?
Into the original.
^ permalink raw reply [flat|nested] 41+ messages in thread
* usbnet: smsc95xx: simplify tx_fixup code
@ 2018-10-06 17:28 ` David Miller
0 siblings, 0 replies; 41+ messages in thread
From: David Miller @ 2018-10-06 17:28 UTC (permalink / raw)
To: ben.dooks
Cc: netdev, David.Laight, oneukum, linux-usb, linux-kernel, linux-kernel
From: Ben Dooks <ben.dooks@codethink.co.uk>
Date: Sat, 06 Oct 2018 12:27:27 +0100
> Thanks, got a new version of the series just being tested with this.
> Should it go into the original, or as a separate change?
Into the original.
^ permalink raw reply [flat|nested] 41+ messages in thread
* RE: [PATCH] usbnet: smsc95xx: simplify tx_fixup code
2018-10-05 21:24 ` David Miller
(?)
@ 2018-10-08 8:41 ` David Laight
-1 siblings, 0 replies; 41+ messages in thread
From: David Laight @ 2018-10-08 8:41 UTC (permalink / raw)
To: 'David Miller', ben.dooks
Cc: netdev, oneukum, linux-usb, linux-kernel, linux-kernel
From: David Miller
> Sent: 05 October 2018 22:24
>
> From: Ben Dooks <ben.dooks@codethink.co.uk>
> Date: Tue, 2 Oct 2018 17:56:02 +0100
>
> > - memcpy(skb->data, &tx_cmd_a, 4);
> > + ptr = skb_push(skb, 8);
> > + tx_cmd_a = cpu_to_le32(tx_cmd_a);
> > + tx_cmd_b = cpu_to_le32(tx_cmd_b);
> > + memcpy(ptr, &tx_cmd_a, 4);
> > + memcpy(ptr+4, &tx_cmd_b, 4);
>
> Even a memcpy() through a void pointer does not guarantee that gcc will
> not emit word sized loads and stores.
True, but only if gcc can 'see' something that would require the
pointer be aligned.
In this case the void pointer comes from an external function
so is fine.
> You must use the get_unaligned()/put_unaligned() facilities to do this
> properly.
>
> I also agree that making a proper type and structure instead of using
> a void pointer would be better.
The structure would need to be marked 'packed' - since its alignment
isn't guaranteed.
Then you don't need to use put_unaligned().
If it wasn't 'packed' then gcc would implement
memcpy(&hdr->tx_cmd_a, &tx_cmd_a, 4) using an aligned write.
David
-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)
^ permalink raw reply [flat|nested] 41+ messages in thread
* usbnet: smsc95xx: simplify tx_fixup code
@ 2018-10-08 8:41 ` David Laight
0 siblings, 0 replies; 41+ messages in thread
From: David Laight @ 2018-10-08 8:41 UTC (permalink / raw)
To: 'David Miller', ben.dooks
Cc: netdev, oneukum, linux-usb, linux-kernel, linux-kernel
From: David Miller
> Sent: 05 October 2018 22:24
>
> From: Ben Dooks <ben.dooks@codethink.co.uk>
> Date: Tue, 2 Oct 2018 17:56:02 +0100
>
> > - memcpy(skb->data, &tx_cmd_a, 4);
> > + ptr = skb_push(skb, 8);
> > + tx_cmd_a = cpu_to_le32(tx_cmd_a);
> > + tx_cmd_b = cpu_to_le32(tx_cmd_b);
> > + memcpy(ptr, &tx_cmd_a, 4);
> > + memcpy(ptr+4, &tx_cmd_b, 4);
>
> Even a memcpy() through a void pointer does not guarantee that gcc will
> not emit word sized loads and stores.
True, but only if gcc can 'see' something that would require the
pointer be aligned.
In this case the void pointer comes from an external function
so is fine.
> You must use the get_unaligned()/put_unaligned() facilities to do this
> properly.
>
> I also agree that making a proper type and structure instead of using
> a void pointer would be better.
The structure would need to be marked 'packed' - since its alignment
isn't guaranteed.
Then you don't need to use put_unaligned().
If it wasn't 'packed' then gcc would implement
memcpy(&hdr->tx_cmd_a, &tx_cmd_a, 4) using an aligned write.
David
-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)
^ permalink raw reply [flat|nested] 41+ messages in thread
* RE: [PATCH] usbnet: smsc95xx: simplify tx_fixup code
@ 2018-10-08 8:41 ` David Laight
0 siblings, 0 replies; 41+ messages in thread
From: David Laight @ 2018-10-08 8:41 UTC (permalink / raw)
To: 'David Miller', ben.dooks
Cc: netdev, oneukum, linux-usb, linux-kernel, linux-kernel
From: David Miller
> Sent: 05 October 2018 22:24
>
> From: Ben Dooks <ben.dooks@codethink.co.uk>
> Date: Tue, 2 Oct 2018 17:56:02 +0100
>
> > - memcpy(skb->data, &tx_cmd_a, 4);
> > + ptr = skb_push(skb, 8);
> > + tx_cmd_a = cpu_to_le32(tx_cmd_a);
> > + tx_cmd_b = cpu_to_le32(tx_cmd_b);
> > + memcpy(ptr, &tx_cmd_a, 4);
> > + memcpy(ptr+4, &tx_cmd_b, 4);
>
> Even a memcpy() through a void pointer does not guarantee that gcc will
> not emit word sized loads and stores.
True, but only if gcc can 'see' something that would require the
pointer be aligned.
In this case the void pointer comes from an external function
so is fine.
> You must use the get_unaligned()/put_unaligned() facilities to do this
> properly.
>
> I also agree that making a proper type and structure instead of using
> a void pointer would be better.
The structure would need to be marked 'packed' - since its alignment
isn't guaranteed.
Then you don't need to use put_unaligned().
If it wasn't 'packed' then gcc would implement
memcpy(&hdr->tx_cmd_a, &tx_cmd_a, 4) using an aligned write.
David
-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)
^ permalink raw reply [flat|nested] 41+ messages in thread