Message ID | 20200717161027.1408240-3-olteanv@gmail.com |
---|---|
State | Changes Requested |
Delegated to: | David Miller |
Headers | show |
Series | Document more PTP timestamping known quirks | expand |
On 7/17/2020 9:10 AM, Vladimir Oltean wrote: > Document the fact that Ethernet PHY timestamping has a fundamentally > flawed corner case (which in fact hits the majority of networking > drivers): a PHY for which its host MAC driver doesn't forward the > phy_mii_ioctl for timestamping is still going to be presented to user > space as functional. > > Fixing this inconsistency would require moving the phy_has_tsinfo() > check inside all MAC drivers which are capable of PHY timestamping, to > be in harmony with the existing design for phy_has_hwtstamp() checks. > Instead of doing that, document the preferable solution which is that > offending MAC drivers be fixed instead. This statement feels weird. Aren't you documenting that the preferable solution is? I.e. "Document this preferable solution: Fix the offending MAC driver" Or am I misunderstanding what the issue is here? > > Signed-off-by: Vladimir Oltean <olteanv@gmail.com> > --- > Documentation/networking/timestamping.rst | 37 +++++++++++++++++++++++ > 1 file changed, 37 insertions(+) > > diff --git a/Documentation/networking/timestamping.rst b/Documentation/networking/timestamping.rst > index 9a1f4cb4ce9e..4004c5d2771d 100644 > --- a/Documentation/networking/timestamping.rst > +++ b/Documentation/networking/timestamping.rst > @@ -754,3 +754,40 @@ check in their "TX confirmation" portion, not only for > that PTP timestamping is not enabled for anything other than the outermost PHC, > this enhanced check will avoid delivering a duplicated TX timestamp to user > space. > + > +Another known limitation is the design of the ``__ethtool_get_ts_info`` > +function:: > + > + int __ethtool_get_ts_info(struct net_device *dev, struct ethtool_ts_info *info) > + { > + const struct ethtool_ops *ops = dev->ethtool_ops; > + struct phy_device *phydev = dev->phydev; > + > + memset(info, 0, sizeof(*info)); > + info->cmd = ETHTOOL_GET_TS_INFO; > + > + if (phy_has_tsinfo(phydev)) > + return phy_ts_info(phydev, info); > + if (ops->get_ts_info) > + return ops->get_ts_info(dev, info); > + > + info->so_timestamping = SOF_TIMESTAMPING_RX_SOFTWARE | > + SOF_TIMESTAMPING_SOFTWARE; > + info->phc_index = -1; > + > + return 0; > + } > + > +Because the generic function searches first for the timestamping capabilities > +of the attached PHY, and returns them directly without consulting the MAC > +driver, no checking is being done whether the requirements described in `3.2.2 > +Ethernet PHYs`_ are implemented or not. Therefore, if the MAC driver does not > +satisfy the requirements for PHY timestamping, and > +``CONFIG_NETWORK_PHY_TIMESTAMPING`` is enabled, then a non-functional PHC index > +(the one corresponding to the PHY) will be reported to user space, via > +``ethtool -T``. > + > +The correct solution to this problem is to implement the PHY timestamping > +requirements in the MAC driver found broken, and submit as a bug fix patch to > +netdev@vger.kernel.org. See :ref:`Documentation/process/stable-kernel-rules.rst > +<stable_kernel_rules>` for more details. >
On Fri, Jul 17, 2020 at 04:08:03PM -0700, Jacob Keller wrote: > > > On 7/17/2020 9:10 AM, Vladimir Oltean wrote: > > Document the fact that Ethernet PHY timestamping has a fundamentally > > flawed corner case (which in fact hits the majority of networking > > drivers): a PHY for which its host MAC driver doesn't forward the > > phy_mii_ioctl for timestamping is still going to be presented to user > > space as functional. > > > > Fixing this inconsistency would require moving the phy_has_tsinfo() > > check inside all MAC drivers which are capable of PHY timestamping, to > > be in harmony with the existing design for phy_has_hwtstamp() checks. > > Instead of doing that, document the preferable solution which is that > > offending MAC drivers be fixed instead. > > This statement feels weird. Aren't you documenting that the preferable > solution is? I.e. "Document this preferable solution: Fix the offending > MAC driver" > > Or am I misunderstanding what the issue is here? > You're right, it looks like I wasn't thinking in full sentences at that particular time of day. > > > > Signed-off-by: Vladimir Oltean <olteanv@gmail.com> > > --- > > Documentation/networking/timestamping.rst | 37 +++++++++++++++++++++++ > > 1 file changed, 37 insertions(+) > > > > diff --git a/Documentation/networking/timestamping.rst b/Documentation/networking/timestamping.rst > > index 9a1f4cb4ce9e..4004c5d2771d 100644 > > --- a/Documentation/networking/timestamping.rst > > +++ b/Documentation/networking/timestamping.rst > > @@ -754,3 +754,40 @@ check in their "TX confirmation" portion, not only for > > that PTP timestamping is not enabled for anything other than the outermost PHC, > > this enhanced check will avoid delivering a duplicated TX timestamp to user > > space. > > + > > +Another known limitation is the design of the ``__ethtool_get_ts_info`` > > +function:: > > + > > + int __ethtool_get_ts_info(struct net_device *dev, struct ethtool_ts_info *info) > > + { > > + const struct ethtool_ops *ops = dev->ethtool_ops; > > + struct phy_device *phydev = dev->phydev; > > + > > + memset(info, 0, sizeof(*info)); > > + info->cmd = ETHTOOL_GET_TS_INFO; > > + > > + if (phy_has_tsinfo(phydev)) > > + return phy_ts_info(phydev, info); > > + if (ops->get_ts_info) > > + return ops->get_ts_info(dev, info); > > + > > + info->so_timestamping = SOF_TIMESTAMPING_RX_SOFTWARE | > > + SOF_TIMESTAMPING_SOFTWARE; > > + info->phc_index = -1; > > + > > + return 0; > > + } > > + > > +Because the generic function searches first for the timestamping capabilities > > +of the attached PHY, and returns them directly without consulting the MAC > > +driver, no checking is being done whether the requirements described in `3.2.2 > > +Ethernet PHYs`_ are implemented or not. Therefore, if the MAC driver does not > > +satisfy the requirements for PHY timestamping, and > > +``CONFIG_NETWORK_PHY_TIMESTAMPING`` is enabled, then a non-functional PHC index > > +(the one corresponding to the PHY) will be reported to user space, via > > +``ethtool -T``. > > + > > +The correct solution to this problem is to implement the PHY timestamping > > +requirements in the MAC driver found broken, and submit as a bug fix patch to > > +netdev@vger.kernel.org. See :ref:`Documentation/process/stable-kernel-rules.rst > > +<stable_kernel_rules>` for more details. > > Thanks, -Vladimir
diff --git a/Documentation/networking/timestamping.rst b/Documentation/networking/timestamping.rst index 9a1f4cb4ce9e..4004c5d2771d 100644 --- a/Documentation/networking/timestamping.rst +++ b/Documentation/networking/timestamping.rst @@ -754,3 +754,40 @@ check in their "TX confirmation" portion, not only for that PTP timestamping is not enabled for anything other than the outermost PHC, this enhanced check will avoid delivering a duplicated TX timestamp to user space. + +Another known limitation is the design of the ``__ethtool_get_ts_info`` +function:: + + int __ethtool_get_ts_info(struct net_device *dev, struct ethtool_ts_info *info) + { + const struct ethtool_ops *ops = dev->ethtool_ops; + struct phy_device *phydev = dev->phydev; + + memset(info, 0, sizeof(*info)); + info->cmd = ETHTOOL_GET_TS_INFO; + + if (phy_has_tsinfo(phydev)) + return phy_ts_info(phydev, info); + if (ops->get_ts_info) + return ops->get_ts_info(dev, info); + + info->so_timestamping = SOF_TIMESTAMPING_RX_SOFTWARE | + SOF_TIMESTAMPING_SOFTWARE; + info->phc_index = -1; + + return 0; + } + +Because the generic function searches first for the timestamping capabilities +of the attached PHY, and returns them directly without consulting the MAC +driver, no checking is being done whether the requirements described in `3.2.2 +Ethernet PHYs`_ are implemented or not. Therefore, if the MAC driver does not +satisfy the requirements for PHY timestamping, and +``CONFIG_NETWORK_PHY_TIMESTAMPING`` is enabled, then a non-functional PHC index +(the one corresponding to the PHY) will be reported to user space, via +``ethtool -T``. + +The correct solution to this problem is to implement the PHY timestamping +requirements in the MAC driver found broken, and submit as a bug fix patch to +netdev@vger.kernel.org. See :ref:`Documentation/process/stable-kernel-rules.rst +<stable_kernel_rules>` for more details.
Document the fact that Ethernet PHY timestamping has a fundamentally flawed corner case (which in fact hits the majority of networking drivers): a PHY for which its host MAC driver doesn't forward the phy_mii_ioctl for timestamping is still going to be presented to user space as functional. Fixing this inconsistency would require moving the phy_has_tsinfo() check inside all MAC drivers which are capable of PHY timestamping, to be in harmony with the existing design for phy_has_hwtstamp() checks. Instead of doing that, document the preferable solution which is that offending MAC drivers be fixed instead. Signed-off-by: Vladimir Oltean <olteanv@gmail.com> --- Documentation/networking/timestamping.rst | 37 +++++++++++++++++++++++ 1 file changed, 37 insertions(+)