Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <0946c513-867d-0ddc-a632-b03fea2cc921@intel.com>
Date:   Tue, 17 Jan 2023 16:07:14 -0800
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:102.0) Gecko/20100101
 Thunderbird/102.6.1
Subject: Re: [PATCH net] net: macb: fix PTP TX timestamp failure due to packet
 padding
Content-Language: en-US
To:     Robert Hancock <robert.hancock@calian.com>,
        Nicolas Ferre <nicolas.ferre@microchip.com>,
        Claudiu Beznea <claudiu.beznea@microchip.com>,
        "David S. Miller" <davem@davemloft.net>,
        Eric Dumazet <edumazet@google.com>,
        Jakub Kicinski <kuba@kernel.org>,
        Paolo Abeni <pabeni@redhat.com>,
        "Richard Cochran" <richardcochran@gmail.com>
CC:     <netdev@vger.kernel.org>, <linux-kernel@vger.kernel.org>
References: <20230116214133.1834364-1-robert.hancock@calian.com>
From:   Jacob Keller <jacob.e.keller@intel.com>
In-Reply-To: <20230116214133.1834364-1-robert.hancock@calian.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: 7bit
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?OXdSUmRZVVZ0bGhFOG5lSjZ0M2xKRUxjaHhoTXpXUVZzSWczS1RhSThhYjBQ?=
 =?utf-8?B?NUxZcjZaSTVEU0tLT2pRL0d1Q3Fkdjc4ZjRmWmFFZHZ1ZXE4T1dMN0FNSTk0?=
 =?utf-8?B?aU0zRWEyS1ljcjlUMEl4bkdwT3RRc2tzRjJpTW9yMHhpNGlZRlpTNFJVTkdJ?=
 =?utf-8?B?YTkvS3NGUHpjbW43Z3VHaGxXc1VrejZnNUJ2UklNM1R2YXhvbXlaSmM5Q2tC?=
 =?utf-8?B?dE9rUlRlNG01TzdXd3pUbExQSUU2SHFFRzI0NTNuaFRLS0NLbkJVWUtWVDBh?=
 =?utf-8?B?ZjNxQmRmdHlNS0ttQ1JidkNOU2ZoaU5JcUZ5V1hQeitLNEFTWE5mbWtrMHZU?=
 =?utf-8?B?WFQwNDVYeGFrbWJWSXlVeHNSOWJ6RWtGcWQvSENqdm8xeENuMlppOEs0VFlw?=
 =?utf-8?B?Tmk5TXZkYkpZcG0rNCs3YTMvdWdydU03NnMyL0lEWkFtcCtUYmM0NmdSOW1D?=
 =?utf-8?B?UnB1Qjlsak8wVXFKY0Q1cXdrMjhTWGJrZk5HOHBEV1I1VHlwR1Z4S0tLbXdJ?=
 =?utf-8?B?d1BDN2hITGJkQlpYR1N3RC9xcUFsT0srdXVIL0tkWHN3WndHTnloS1F3UWsz?=
 =?utf-8?B?UVd1U2hzTVIrRVg2RC9qQytGaGZKY3RBZmVpdFBha2YzeDhqWWpEeWdxMGhS?=
 =?utf-8?B?ZmxKZTJncjdmb0NuQkV1Zm5ETU5tczNUYkRweTI1eVZHdS83MXV4c1M2d1FZ?=
 =?utf-8?B?MjQ4dFova0Y5UE9yU0NYVk1MekEzZE1Bam94elVnTlhYVmhYV1lUakpCc3N0?=
 =?utf-8?B?L09EblVJeXduVTFSZEpteFM5Q1ZwQVIrTzFRMUFpeGVlckU5eTdjMytBcW56?=
 =?utf-8?B?WGZwbkNLUnMzMDNML1dBU2E4eSt0b2NMeU1ZdDhuVHRYbTMwZmJ2OUZQUjl5?=
 =?utf-8?B?M3h4dWhIVzE4bUdIUkgvaGZ0aUZPcTZqelRIMTFqcWVkZEZ1V0J3OGJEVmFo?=
 =?utf-8?B?NVVqT2pxSGx2WW82SWhrNlJyRTd5bnF0NGF6aDhobTVwMHZ0UGJwcVY3Z1p0?=
 =?utf-8?B?UUNLSnM5SENqZWw4blZKd0RQVG9NNi9wNytObXZFQ0JSM3ljRTNLMWVKcjRP?=
 =?utf-8?B?Z3RNTkJDa1BDTkxSNVNWVzExWUV5ZVBSSm9sN2hyeCtXK1BmSjNRL2szeHgw?=
 =?utf-8?B?RXdHaFJZMEdFQjduRXVRNUlBb2VHaEFpcStMd2NKOEtLUUJsUVRGWXZwN05I?=
 =?utf-8?B?TEFLYWI5TDc1dEM2cEVxbXZuQUlEdWs3UXlGOVMydVNtS3J3RFYra0tPcnBo?=
 =?utf-8?B?cnh5azRkYThieHVIZG83NWdxbGM4U1F0VzRNSnA0a0UvcXZhTlpwWFZYaVhW?=
 =?utf-8?B?Qi9CbVFtU1g5bFZVMzJiOXVNaFBFUHJSOThxT3d4Y293N3hoT2tCRHlqRXN1?=
 =?utf-8?B?K3Z3Y2drWG5EdFAxM01hbnVaUnAwWmlYbThuNysxcHozSWM4SGwzaEp3NTlS?=
 =?utf-8?B?akMxQzFVejBrTTFWZVUrQU45OGhrN3pwSnJacHRaaFIvTExXZjMvaXVyWTZS?=
 =?utf-8?B?VEhKakc4N0ZHRFRhZUlpd1pZdWFxb2hNcFJ3K2RRR21mMnR6WWd6WlV6dmZO?=
 =?utf-8?B?YkhodmxvWUNtd2FBdGlpM3dVd0puYkRodW5NYjVRNkRMZTFGaUFYNGg2SzhX?=
 =?utf-8?B?NW9oZU0zc2RIL3pQQmdUSEpWSTFnd3NSVTc4QXd1WmVsTVZXNVpucEUxcDBT?=
 =?utf-8?B?eGxGZE44TjYyc1hsMUpEMTIvWUJQanRvTHFJSzJrc3pYV1Jtd2xieU1nRUJ4?=
 =?utf-8?B?bXhSL0VCYm1vb29XMEhSWjZPeVVRUDdueDFWSU1MVUZZa1UxYU1qZHFkay9P?=
 =?utf-8?B?T0JlL1FpUGpqOWhkVXYvZzdQLzNhaXdibEUramRSSUloRytYbnU2Y0RobFhv?=
 =?utf-8?B?RlZkSlJZR2MrYUFvb1dxWTViZjBQWW1zdk0xRzZDWjZ6WHdJOExMOHIxVkhR?=
 =?utf-8?B?K3lkUHJYMGs5amE2VUVGT09HenI3MjFIVkoycmZ0bVJmWEMrM0xUZ3E4UVdE?=
 =?utf-8?B?cVlXV0FheVd6bDZIdnFsVHBwYkRaYmlPRVZ1TDRlLy9EVVl6aVdTeUJudFhM?=
 =?utf-8?B?SFptY3VBam5pU1pEUWZCNXFReGY2ajFHYmMyMnpiKzlpcW04RlpVYVhLNDJv?=
 =?utf-8?B?cU1PT0EzMk42cnV0cHZKWktVM3Y2MWd5eHV4cW5wRmtLeEJsNytEaEhPSnBT?=
 =?utf-8?B?NXc9PQ==?=
X-MS-Exchange-CrossTenant-Network-Message-Id: 55db633c-7b6a-4016-a2ab-08daf8e7f576
X-MS-Exchange-CrossTenant-AuthSource: CO1PR11MB5089.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 18 Jan 2023 00:07:17.3484
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 46c98d88-e344-4ed4-8496-4ed7712e255d
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: 5FKYzKdP3P1U/RI/h7uFrSkzNzUjHACBtizoa/+g4igBUBhBV0MIzmKE9ovx/Rq/F85+JdDAL7eNAjMp72Smnap12CDy6eAe5ckX5FhYIwU=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: SJ0PR11MB4974
X-OriginatorOrg: intel.com
X-Spam-Status: No, score=-4.5 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH,
        DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,NICE_REPLY_A,
        RCVD_IN_DNSWL_MED,SPF_HELO_NONE,SPF_NONE autolearn=ham
        autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on
        lindbergh.monkeyblade.net
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org


On 1/16/2023 1:41 PM, Robert Hancock wrote:
> 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>
> ---

Makes sense and is a nice cleanup.

Reviewed-by: Jacob Keller <jacob.e.keller@intel.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;