Messages in this thread |  | | Date | Wed, 03 Oct 2018 17:25:48 +0100 | From | Ben Dooks <> | Subject | RE: [PATCH] usbnet: smsc95xx: simplify tx_fixup code |
| |
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)
|  |