Hello Jacob, On Mon, 29 Jul 2024 11:08:01 -0700 Jacob Keller <jacob.e.keller@xxxxxxxxx> wrote: Sorry for answering it so late. I was a bit busy. > >>> --- a/net/core/timestamping.c > >>> +++ b/net/core/timestamping.c > >>> @@ -25,7 +25,8 @@ void skb_clone_tx_timestamp(struct sk_buff *skb) > >>> struct sk_buff *clone; > >>> unsigned int type; > >>> > >>> - if (!skb->sk) > >>> + if (!skb->sk || !skb->dev || > >>> + !phy_is_default_hwtstamp(skb->dev->phydev)) > >> > >> I don't follow why this check is added and its not calling something > >> like "phy_is_current_hwtstamp"? I guess because we don't yet have a way > >> to select between MAC/PHY at this point in the series? Ok. > > > > skb_clone_tx_timestamp is only used for PHY timestamping so we should do > > nothing if the default PTP is the MAC. > > > > I guess my misunderstanding is what about the case where user selects > PHY timestamping with the netlink command? Then it would still need to > do the skb_clone_tx_timestamp even though its not the default? Or does > phy_is_default_hwtstamp take that into account? In which case it would > make more sense to name it phy_is_current_hwtstamp. > > Either way this is mostly bikeshedding and probably just some > misunderstanding in my reading of the code. In fact the phy_is_default_hwtstamp() is only needed in case of no netlink command used. As you can see in patch 8, we call it only if dev->hwtstamp is null which mean that a netlink command has been sent. Regards, -- Köry Maincent, Bootlin Embedded Linux and kernel engineering https://bootlin.com
