Messages in this thread | | | From | Sergey Organov <> | Subject | Re: [PATCH 1/5] net: fec: properly support external PTP PHY for hardware time stamping | Date | Mon, 06 Jul 2020 21:33:30 +0300 |
| |
Vladimir Oltean <olteanv@gmail.com> writes:
> On Mon, Jul 06, 2020 at 06:21:59PM +0300, Sergey Organov wrote: >> 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! >> > > 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.fsf@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
| |