Re: [PATCH 1/5] net: fec: properly support external PTP PHY for hardware time stamping

[Sergey Organov <sorganov@gmail.com>]

Vladimir Oltean <olteanv@gmail.com> writes:

> Hi Sergey,
>
> On Mon, Jul 06, 2020 at 05:26:12PM +0300, Sergey Organov wrote:
>> When external PTP-aware PHY is in use, it's that PHY that is to time
>> stamp network packets, and it's that PHY where configuration requests
>> of time stamping features are to be routed.
>> 
>> To achieve these goals:
>> 
>> 1. Make sure we don't time stamp packets when external PTP PHY is in use
>> 
>> 2. Make sure we redirect ioctl() related to time stamping of Ethernet
>>    packets to connected PTP PHY rather than handle them ourselves

[...]

>> diff --git a/drivers/net/ethernet/freescale/fec_main.c b/drivers/net/ethernet/freescale/fec_main.c
>> index 2d0d313..995ea2e 100644
>> --- a/drivers/net/ethernet/freescale/fec_main.c
>> +++ b/drivers/net/ethernet/freescale/fec_main.c
>> @@ -1298,7 +1298,11 @@ fec_enet_tx_queue(struct net_device *ndev, u16 queue_id)
>>  			ndev->stats.tx_bytes += skb->len;
>>  		}
>>  
>> +		/* It could be external PHY that had set SKBTX_IN_PROGRESS, so
>> +		 * we still need to check it's we who are to time stamp
>> +		 */
>>  		if (unlikely(skb_shinfo(skb)->tx_flags & SKBTX_IN_PROGRESS) &&
>> +		    unlikely(fep->hwts_tx_en) &&
>
> I think this could qualify as a pretty significant fix in its own right,
> that should go to stable trees. Right now, this patch appears pretty
> easy to overlook.
>
> Is this the same situation as what is being described here for the
> gianfar driver?
>
> https://patchwork.ozlabs.org/project/netdev/patch/20191227004435.21692-2-olteanv@gmail.com/

Yes, it sounds exactly like that!

However, I'd insist that the second part of the patch is as important.
Please refer to my original post for the description of nasty confusion
the second part of the patch fixes:

https://lore.kernel.org/netdev/87r1uqtybr.fsf@osv.gnss.ru/

Basically, you get PHY response when you ask for capabilities, but then
MAC executes ioctl() request for corresponding configuration!

[...]

Thanks,
-- Sergey