Vladimir Oltean <olte...@gmail.com> writes: > On Mon, Jul 06, 2020 at 06:21:59PM +0300, Sergey Organov wrote: >> Vladimir Oltean <olte...@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-olte...@gmail.com/ >> >> Yes, it sounds exactly like that! >> > > Cool. Join the club! You were lucky though, in your case it was pretty > evident where the problem might be, so you were already on your way even > though you didn't know exactly what was going on. > > Towards the point that you brought up in that thread: > >> Could somebody please help me implement (or point me to) proper fix to >> reliably use external PHY to timestamp network packets? > > We do it like this: > - DSA: If there is a timestamping switch stacked on top of a > timestamping Ethernet MAC, the switch hijacks the .ndo_do_ioctl of the > host port, and you are supposed to use the PTP clock of the switch, > through the .ndo_do_ioctl of its own (virtual) net devices. This > approach works without changing any code in each individual Ethernet > MAC driver. > - PHY: The Ethernet MAC driver needs to be kind enough to check whether > the PHY supports hw timestamping, and pass this ioctl to that PHY > while making sure it doesn't do anything stupid in the meanwhile, like > also acting upon that timestamping request itself. > > Both are finicky in their own ways. There is no real way for the user to > select which PHC they want to use. The assumption is that you'd always > want to use the outermost one, and that things in the kernel side always > collaborate towards that end.
Makes sense, -- thanks for clarification! Indeed, if somebody connected that external thingy, chances are high it was made for a purpose. > >> 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....@osv.gnss.ru/ >> >> Basically, you get PHY response when you ask for capabilities, but then >> MAC executes ioctl() request for corresponding configuration! >> >> [...] >> > > Yup, sure, _but_ my point is: PHY timestamping is not supposed to work > unless you do that phy_has_hwtstamp dance in .ndo_do_ioctl and pass it > to the PHY driver. Whereas, timestamping on a DSA switch is supposed to > just work. So, the double-TX-timestamp fix is common for both DSA and > PHY timestamping, and it should be a separate patch that goes to David's > "net" tree and has an according Fixes: tag for the stable people to pick > it up. Then, the PHY timestamping patch is technically a new feature, > because the driver wasn't looking at the PHY's ability to perform PTP > timestamping, and now it does. So that part is a patch for "net-next". Ah, thanks, now it makes sense! I simply was not aware of the DSA (whatever it is) you've mentioned above. I'll then make these 2 changes separate in v2 indeed, though I'm not aware about Fixes: tag and if I should do something about it. Any clues? Thanks, -- Sergey
