Messages in this thread Patch in this message | ![/](/images/icornerl.gif) | | From | <> | Subject | Re: [PATCH net] net: macb: fix PTP TX timestamp failure due to packet padding | Date | Wed, 18 Jan 2023 09:39:44 +0000 |
| |
On 16.01.2023 23:41, Robert Hancock wrote: > EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe > > PTP TX timestamp handling was observed to be broken with this driver > when using the raw Layer 2 PTP encapsulation. ptp4l was not receiving > the expected TX timestamp after transmitting a packet, causing it to > enter a failure state. > > The problem appears to be due to the way that the driver pads packets > which are smaller than the Ethernet minimum of 60 bytes. If headroom > space was available in the SKB, this caused the driver to move the data > back to utilize it. However, this appears to cause other data references > in the SKB to become inconsistent. In particular, this caused the > ptp_one_step_sync function to later (in the TX completion path) falsely > detect the packet as a one-step SYNC packet, even when it was not, which > caused the TX timestamp to not be processed when it should be. > > Using the headroom for this purpose seems like an unnecessary complexity > as this is not a hot path in the driver, and in most cases it appears > that there is sufficient tailroom to not require using the headroom > anyway. Remove this usage of headroom to prevent this inconsistency from > occurring and causing other problems. > > Fixes: 653e92a9175e ("net: macb: add support for padding and fcs computation") > Signed-off-by: Robert Hancock <robert.hancock@calian.com>
Reviewed-by: Claudiu Beznea <claudiu.beznea@microchip.com>
> --- > drivers/net/ethernet/cadence/macb_main.c | 9 +-------- > 1 file changed, 1 insertion(+), 8 deletions(-) > > diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c > index 95667b979fab..72e42820713d 100644 > --- a/drivers/net/ethernet/cadence/macb_main.c > +++ b/drivers/net/ethernet/cadence/macb_main.c > @@ -2187,7 +2187,6 @@ static int macb_pad_and_fcs(struct sk_buff **skb, struct net_device *ndev) > bool cloned = skb_cloned(*skb) || skb_header_cloned(*skb) || > skb_is_nonlinear(*skb); > int padlen = ETH_ZLEN - (*skb)->len; > - int headroom = skb_headroom(*skb); > int tailroom = skb_tailroom(*skb); > struct sk_buff *nskb; > u32 fcs; > @@ -2201,9 +2200,6 @@ static int macb_pad_and_fcs(struct sk_buff **skb, struct net_device *ndev) > /* FCS could be appeded to tailroom. */ > if (tailroom >= ETH_FCS_LEN) > goto add_fcs; > - /* FCS could be appeded by moving data to headroom. */ > - else if (!cloned && headroom + tailroom >= ETH_FCS_LEN) > - padlen = 0; > /* No room for FCS, need to reallocate skb. */ > else > padlen = ETH_FCS_LEN; > @@ -2212,10 +2208,7 @@ static int macb_pad_and_fcs(struct sk_buff **skb, struct net_device *ndev) > padlen += ETH_FCS_LEN; > } > > - if (!cloned && headroom + tailroom >= padlen) { > - (*skb)->data = memmove((*skb)->head, (*skb)->data, (*skb)->len); > - skb_set_tail_pointer(*skb, (*skb)->len); > - } else { > + if (cloned || tailroom < padlen) { > nskb = skb_copy_expand(*skb, 0, padlen, GFP_ATOMIC); > if (!nskb) > return -ENOMEM; > -- > 2.39.0 >
| ![\](/images/icornerr.gif) |