| Message ID | 20200717161027.1408240-4-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: > These are some questions I had while trying to explain the behavior of > some drivers with respect to software timestamping. Answered with the > help of Richard Cochran. > > Signed-off-by: Vladimir Oltean <olteanv@gmail.com> > --- > Documentation/networking/timestamping.rst | 26 +++++++++++++++++++++++ > 1 file changed, 26 insertions(+) > > diff --git a/Documentation/networking/timestamping.rst b/Documentation/networking/timestamping.rst > index 4004c5d2771d..e01ec01179fe 100644 > --- a/Documentation/networking/timestamping.rst > +++ b/Documentation/networking/timestamping.rst > @@ -791,3 +791,29 @@ 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. > + > +3.4 Frequently asked questions > +------------------------------ > + > +Q: When should drivers set SKBTX_IN_PROGRESS? > +^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ > + > +When the interface they represent offers both ``SOF_TIMESTAMPING_TX_HARDWARE`` > +and ``SOF_TIMESTAMPING_TX_SOFTWARE``. > +Originally, the network stack could deliver either a hardware or a software > +time stamp, but not both. This flag prevents software timestamp delivery. > +This restriction was eventually lifted via the ``SOF_TIMESTAMPING_OPT_TX_SWHW`` > +option, but still the original behavior is preserved as the default. > + So, this implies that we set this only if both are supported? I thought the intention was to set this flag whenever we start a HW timestamp. > +Q: Should drivers that don't offer SOF_TIMESTAMPING_TX_SOFTWARE call skb_tx_timestamp()? > +^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ > + > +The ``skb_clone_tx_timestamp()`` function from its body helps with propagation > +of TX timestamps from PTP PHYs, and the required placement of this call is the > +same as for software TX timestamping. > +Additionally, since PTP is broken on ports with timestamping PHYs and unmet > +requirements, the consequence is that any driver which may be ever coupled to > +a timestamping-capable PHY in ``netdev->phydev`` should call at least > +``skb_clone_tx_timestamp()``. However, calling the higher-level > +``skb_tx_timestamp()`` instead achieves the same purpose, but also offers > +additional compliance to ``SOF_TIMESTAMPING_TX_SOFTWARE``. > This makes sense. Thanks, Jake
On Fri, Jul 17, 2020 at 04:12:07PM -0700, Jacob Keller wrote: > > > On 7/17/2020 9:10 AM, Vladimir Oltean wrote: > > These are some questions I had while trying to explain the behavior of > > some drivers with respect to software timestamping. Answered with the > > help of Richard Cochran. > > > > Signed-off-by: Vladimir Oltean <olteanv@gmail.com> > > --- > > Documentation/networking/timestamping.rst | 26 +++++++++++++++++++++++ > > 1 file changed, 26 insertions(+) > > > > diff --git a/Documentation/networking/timestamping.rst b/Documentation/networking/timestamping.rst > > index 4004c5d2771d..e01ec01179fe 100644 > > --- a/Documentation/networking/timestamping.rst > > +++ b/Documentation/networking/timestamping.rst > > @@ -791,3 +791,29 @@ 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. > > + > > +3.4 Frequently asked questions > > +------------------------------ > > + > > +Q: When should drivers set SKBTX_IN_PROGRESS? > > +^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ > > + > > +When the interface they represent offers both ``SOF_TIMESTAMPING_TX_HARDWARE`` > > +and ``SOF_TIMESTAMPING_TX_SOFTWARE``. > > +Originally, the network stack could deliver either a hardware or a software > > +time stamp, but not both. This flag prevents software timestamp delivery. > > +This restriction was eventually lifted via the ``SOF_TIMESTAMPING_OPT_TX_SWHW`` > > +option, but still the original behavior is preserved as the default. > > + > > So, this implies that we set this only if both are supported? I thought > the intention was to set this flag whenever we start a HW timestamp. > It's only _required_ when SOF_TIMESTAMPING_TX_SOFTWARE is used, it seems. I had also thought of setting 'SKBTX_IN_PROGRESS' as good practice, but there are many situations where it can do more harm than good. > > +Q: Should drivers that don't offer SOF_TIMESTAMPING_TX_SOFTWARE call skb_tx_timestamp()? > > +^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ > > + > > +The ``skb_clone_tx_timestamp()`` function from its body helps with propagation > > +of TX timestamps from PTP PHYs, and the required placement of this call is the > > +same as for software TX timestamping. > > +Additionally, since PTP is broken on ports with timestamping PHYs and unmet > > +requirements, the consequence is that any driver which may be ever coupled to > > +a timestamping-capable PHY in ``netdev->phydev`` should call at least > > +``skb_clone_tx_timestamp()``. However, calling the higher-level > > +``skb_tx_timestamp()`` instead achieves the same purpose, but also offers > > +additional compliance to ``SOF_TIMESTAMPING_TX_SOFTWARE``. > > > > This makes sense. > > Thanks, > Jake Thanks, -Vladimir
On 7/18/2020 4:35 AM, Vladimir Oltean wrote: > On Fri, Jul 17, 2020 at 04:12:07PM -0700, Jacob Keller wrote: >> On 7/17/2020 9:10 AM, Vladimir Oltean wrote: >>> +When the interface they represent offers both ``SOF_TIMESTAMPING_TX_HARDWARE`` >>> +and ``SOF_TIMESTAMPING_TX_SOFTWARE``. >>> +Originally, the network stack could deliver either a hardware or a software >>> +time stamp, but not both. This flag prevents software timestamp delivery. >>> +This restriction was eventually lifted via the ``SOF_TIMESTAMPING_OPT_TX_SWHW`` >>> +option, but still the original behavior is preserved as the default. >>> + >> >> So, this implies that we set this only if both are supported? I thought >> the intention was to set this flag whenever we start a HW timestamp. >> > > It's only _required_ when SOF_TIMESTAMPING_TX_SOFTWARE is used, it > seems. I had also thought of setting 'SKBTX_IN_PROGRESS' as good > practice, but there are many situations where it can do more harm than > good. > I guess I've only ever implemented a driver with software timestamping enabled as an option. What sort of issues arise when you have this set? I'm guessing that it's some configuration of stacked devices as in the other cases? If the issue can't be fixed I'd at least like more explanation here, since the prevailing convention is that we set this flag, so understanding when and why it's problematic would be useful. Thanks, Jake
On Mon, Jul 20, 2020 at 11:54:30AM -0700, Jacob Keller wrote: > On 7/18/2020 4:35 AM, Vladimir Oltean wrote: > > On Fri, Jul 17, 2020 at 04:12:07PM -0700, Jacob Keller wrote: > >> On 7/17/2020 9:10 AM, Vladimir Oltean wrote: > >>> +When the interface they represent offers both ``SOF_TIMESTAMPING_TX_HARDWARE`` > >>> +and ``SOF_TIMESTAMPING_TX_SOFTWARE``. > >>> +Originally, the network stack could deliver either a hardware or a software > >>> +time stamp, but not both. This flag prevents software timestamp delivery. > >>> +This restriction was eventually lifted via the ``SOF_TIMESTAMPING_OPT_TX_SWHW`` > >>> +option, but still the original behavior is preserved as the default. > >>> + > >> > >> So, this implies that we set this only if both are supported? I thought > >> the intention was to set this flag whenever we start a HW timestamp. > >> > > > > It's only _required_ when SOF_TIMESTAMPING_TX_SOFTWARE is used, it > > seems. I had also thought of setting 'SKBTX_IN_PROGRESS' as good > > practice, but there are many situations where it can do more harm than > > good. > > > > I guess I've only ever implemented a driver with software timestamping > enabled as an option. What sort of issues arise when you have this set? > I'm guessing that it's some configuration of stacked devices as in the > other cases? If the issue can't be fixed I'd at least like more > explanation here, since the prevailing convention is that we set this > flag, so understanding when and why it's problematic would be useful. > > Thanks, > Jake Yes, the problematic cases have to do with stacked PHCs (DSA, PHY). The pattern is that: - DSA sets SKBTX_IN_PROGRESS - calls dev_queue_xmit towards the MAC driver - MAC driver sees SKBTX_IN_PROGRESS, thinks it's the one who set it - MAC driver delivers TX timestamp - DSA ends poll or receives TX interrupt, collects its timestamp, and delivers a second TX timestamp In fact this is explained in a bit more detail in the current timestamping.rst file. Not only are there existing in-tree drivers that do that (and various subtle variations of it), but new code also has this tendency to take shortcuts and interpret any SKBTX_IN_PROGRESS flag set as being set locally. Good thing it's caught during review most of the time these days. It's an error-prone design. On the DSA front, 1 driver sets this flag (sja1105) and 3 don't (felix, mv88e6xxx, hellcreek). The driver who had trouble because of this flag? sja1105. On the PHY front, 2 drivers set this flag (mscc_phy, dp83640) and 1 doesn't (ptp_ines). The driver who had trouble? dp83640. So it's very far from obvious that setting this flag is 'the prevailing convention'. For a MAC driver, that might well be, but for DSA/PHY, there seem to be risks associated with doing that, and driver writers should know what they're signing up for. -Vladimir
On 7/20/2020 2:05 PM, Vladimir Oltean wrote: > On Mon, Jul 20, 2020 at 11:54:30AM -0700, Jacob Keller wrote: >> On 7/18/2020 4:35 AM, Vladimir Oltean wrote: >>> On Fri, Jul 17, 2020 at 04:12:07PM -0700, Jacob Keller wrote: >>>> On 7/17/2020 9:10 AM, Vladimir Oltean wrote: >>>>> +When the interface they represent offers both ``SOF_TIMESTAMPING_TX_HARDWARE`` >>>>> +and ``SOF_TIMESTAMPING_TX_SOFTWARE``. >>>>> +Originally, the network stack could deliver either a hardware or a software >>>>> +time stamp, but not both. This flag prevents software timestamp delivery. >>>>> +This restriction was eventually lifted via the ``SOF_TIMESTAMPING_OPT_TX_SWHW`` >>>>> +option, but still the original behavior is preserved as the default. >>>>> + >>>> >>>> So, this implies that we set this only if both are supported? I thought >>>> the intention was to set this flag whenever we start a HW timestamp. >>>> >>> >>> It's only _required_ when SOF_TIMESTAMPING_TX_SOFTWARE is used, it >>> seems. I had also thought of setting 'SKBTX_IN_PROGRESS' as good >>> practice, but there are many situations where it can do more harm than >>> good. >>> >> >> I guess I've only ever implemented a driver with software timestamping >> enabled as an option. What sort of issues arise when you have this set? >> I'm guessing that it's some configuration of stacked devices as in the >> other cases? If the issue can't be fixed I'd at least like more >> explanation here, since the prevailing convention is that we set this >> flag, so understanding when and why it's problematic would be useful. >> >> Thanks, >> Jake > > Yes, the problematic cases have to do with stacked PHCs (DSA, PHY). The > pattern is that: > - DSA sets SKBTX_IN_PROGRESS > - calls dev_queue_xmit towards the MAC driver > - MAC driver sees SKBTX_IN_PROGRESS, thinks it's the one who set it > - MAC driver delivers TX timestamp > - DSA ends poll or receives TX interrupt, collects its timestamp, and > delivers a second TX timestamp > In fact this is explained in a bit more detail in the current > timestamping.rst file. > Not only are there existing in-tree drivers that do that (and various > subtle variations of it), but new code also has this tendency to take > shortcuts and interpret any SKBTX_IN_PROGRESS flag set as being set > locally. Good thing it's caught during review most of the time these > days. It's an error-prone design. > On the DSA front, 1 driver sets this flag (sja1105) and 3 don't (felix, > mv88e6xxx, hellcreek). The driver who had trouble because of this flag? > sja1105. > On the PHY front, 2 drivers set this flag (mscc_phy, dp83640) and 1 > doesn't (ptp_ines). The driver who had trouble? dp83640. > So it's very far from obvious that setting this flag is 'the prevailing > convention'. For a MAC driver, that might well be, but for DSA/PHY, > there seem to be risks associated with doing that, and driver writers > should know what they're signing up for. > Perhaps the issue is that the MAC driver using SKBTX_IN_PROGRESS as the mechanism for telling if it should deliver a timestamp. Shouldn't they be relying on SKBTX_HW_TSTAMP for the "please timestamp" notification, and then using their own mechanism for forwarding that timestamp once it's complete? I see a handful of drivers do rely on checking this, but I think that's the real bug here. > -Vladimir >
On Mon, Jul 20, 2020 at 02:45:03PM -0700, Jacob Keller wrote: > > > On 7/20/2020 2:05 PM, Vladimir Oltean wrote: > > On Mon, Jul 20, 2020 at 11:54:30AM -0700, Jacob Keller wrote: > >> On 7/18/2020 4:35 AM, Vladimir Oltean wrote: > >>> On Fri, Jul 17, 2020 at 04:12:07PM -0700, Jacob Keller wrote: > >>>> On 7/17/2020 9:10 AM, Vladimir Oltean wrote: > >>>>> +When the interface they represent offers both ``SOF_TIMESTAMPING_TX_HARDWARE`` > >>>>> +and ``SOF_TIMESTAMPING_TX_SOFTWARE``. > >>>>> +Originally, the network stack could deliver either a hardware or a software > >>>>> +time stamp, but not both. This flag prevents software timestamp delivery. > >>>>> +This restriction was eventually lifted via the ``SOF_TIMESTAMPING_OPT_TX_SWHW`` > >>>>> +option, but still the original behavior is preserved as the default. > >>>>> + > >>>> > >>>> So, this implies that we set this only if both are supported? I thought > >>>> the intention was to set this flag whenever we start a HW timestamp. > >>>> > >>> > >>> It's only _required_ when SOF_TIMESTAMPING_TX_SOFTWARE is used, it > >>> seems. I had also thought of setting 'SKBTX_IN_PROGRESS' as good > >>> practice, but there are many situations where it can do more harm than > >>> good. > >>> > >> > >> I guess I've only ever implemented a driver with software timestamping > >> enabled as an option. What sort of issues arise when you have this set? > >> I'm guessing that it's some configuration of stacked devices as in the > >> other cases? If the issue can't be fixed I'd at least like more > >> explanation here, since the prevailing convention is that we set this > >> flag, so understanding when and why it's problematic would be useful. > >> > >> Thanks, > >> Jake > > > > Yes, the problematic cases have to do with stacked PHCs (DSA, PHY). The > > pattern is that: > > - DSA sets SKBTX_IN_PROGRESS > > - calls dev_queue_xmit towards the MAC driver > > - MAC driver sees SKBTX_IN_PROGRESS, thinks it's the one who set it > > - MAC driver delivers TX timestamp > > - DSA ends poll or receives TX interrupt, collects its timestamp, and > > delivers a second TX timestamp > > In fact this is explained in a bit more detail in the current > > timestamping.rst file. > > Not only are there existing in-tree drivers that do that (and various > > subtle variations of it), but new code also has this tendency to take > > shortcuts and interpret any SKBTX_IN_PROGRESS flag set as being set > > locally. Good thing it's caught during review most of the time these > > days. It's an error-prone design. > > On the DSA front, 1 driver sets this flag (sja1105) and 3 don't (felix, > > mv88e6xxx, hellcreek). The driver who had trouble because of this flag? > > sja1105. > > On the PHY front, 2 drivers set this flag (mscc_phy, dp83640) and 1 > > doesn't (ptp_ines). The driver who had trouble? dp83640. > > So it's very far from obvious that setting this flag is 'the prevailing > > convention'. For a MAC driver, that might well be, but for DSA/PHY, > > there seem to be risks associated with doing that, and driver writers > > should know what they're signing up for. > > > > Perhaps the issue is that the MAC driver using SKBTX_IN_PROGRESS as the > mechanism for telling if it should deliver a timestamp. Shouldn't they > be relying on SKBTX_HW_TSTAMP for the "please timestamp" notification, > and then using their own mechanism for forwarding that timestamp once > it's complete? > > I see a handful of drivers do rely on checking this, but I think that's > the real bug here. > > > -Vladimir > > Yes, indeed, a lot of them are exclusively checking "skb_shinfo(skb)->tx_flags & SKBTX_IN_PROGRESS", without any further verification that they have hardware timestamping enabled in the first place, a lot more than I remembered. Some of the occurrences are actually new. I think at least part of the reason why this keeps going on is that there aren't any hard and fast rules that say you shouldn't do it. When there isn't even a convincing percentage of DSA/PHY drivers that do set SKBTX_HW_TSTAMP, the chances are pretty low that you'll get a stacked PHC driver that sets the flag, plus a MAC driver that checks for it incorrectly. So people tend to ignore this case. Even though, if stacked DSA drivers started supporting software TX timestamping (which is not unlikely, given the fact that this would also give you compatibility with PHY timestamping), I'm sure things would change, because more people would become aware of the issue once mv88e6xxx starts getting affected. What I've been trying to do is at least try to get people (especially people who have a lot of XP with 1588 drivers) to agree on a common set of guidelines that are explicitly written down. I think that's step #1. -Vladimir
On Tue, Jul 21, 2020 at 12:05:18AM +0300, Vladimir Oltean wrote: > > Yes, the problematic cases have to do with stacked PHCs (DSA, PHY). The > pattern is that: > - DSA sets SKBTX_IN_PROGRESS Nit: DSA should _not_ set this bit (but a PHY/MII device should). > - calls dev_queue_xmit towards the MAC driver > - MAC driver sees SKBTX_IN_PROGRESS, thinks it's the one who set it > - MAC driver delivers TX timestamp > - DSA ends poll or receives TX interrupt, collects its timestamp, and > delivers a second TX timestamp > So it's very far from obvious that setting this flag is 'the prevailing > convention'. For a MAC driver, that might well be, but for DSA/PHY, > there seem to be risks associated with doing that, and driver writers > should know what they're signing up for. The flag only exists to prevent the stack from delivering SW time stamps when HW time stamps are active. If an interface doesn't provide SW time stamps (like DSA interfaces), then there is no need to set the flag. For MAC and PHY/MII time stamping, they must co-operate, meaning that the MAC driver must be prepared to deal with the fact that the PHY/MII might set this flag. Many MAC drivers don't do this correctly, but there are very few PHY/MII actually in use, and so very few authors of MAC drivers have paid attention to this. Thanks, Richard
On Tue, Jul 21, 2020 at 01:13:14AM +0300, Vladimir Oltean wrote: > I think at least part of the reason why this keeps going on is that > there aren't any hard and fast rules that say you shouldn't do it. When > there isn't even a convincing percentage of DSA/PHY drivers that do set > SKBTX_HW_TSTAMP, the chances are pretty low that you'll get a stacked > PHC driver that sets the flag, plus a MAC driver that checks for it > incorrectly. So people tend to ignore this case. Right. > Even though, if stacked > DSA drivers started supporting software TX timestamping (which is not > unlikely, given the fact that this would also give you compatibility > with PHY timestamping), I'm sure things would change, because more > people would become aware of the issue once mv88e6xxx starts getting > affected. I really can't see the utility in having a SW time stamp from a DSA interface. The whole point of DSA time stamping is to get the ingress and egress time of frames on the external ports, in order to correct for the residence time within the switch. Thanks, Richard
On 7/20/2020 3:13 PM, Vladimir Oltean wrote:> > Yes, indeed, a lot of them are exclusively checking > "skb_shinfo(skb)->tx_flags & SKBTX_IN_PROGRESS", without any further > verification that they have hardware timestamping enabled in the first > place, a lot more than I remembered. Some of the occurrences are > actually new. > > I think at least part of the reason why this keeps going on is that > there aren't any hard and fast rules that say you shouldn't do it. When > there isn't even a convincing percentage of DSA/PHY drivers that do set > SKBTX_HW_TSTAMP, the chances are pretty low that you'll get a stacked > PHC driver that sets the flag, plus a MAC driver that checks for it > incorrectly. So people tend to ignore this case. Even though, if stacked > DSA drivers started supporting software TX timestamping (which is not > unlikely, given the fact that this would also give you compatibility > with PHY timestamping), I'm sure things would change, because more > people would become aware of the issue once mv88e6xxx starts getting > affected. > > What I've been trying to do is at least try to get people (especially > people who have a lot of XP with 1588 drivers) to agree on a common set > of guidelines that are explicitly written down. I think that's step #1. > > -Vladimir > Right. I think the guideline should be: This flag indicates to the stack whether or not a hardware Tx timestamp has been started. It's primary purpose is to prevent sending software timestamps if a hw timestamp is provided. 1) set the flag whenever you start a tx timestamp 2) do not assume you are the only driver that will set the flag for a given skb. Use a separate mechanism to decide if that skb is supposed to have a timestamp.
On Mon, Jul 20, 2020 at 05:21:50PM -0700, Richard Cochran wrote: > On Tue, Jul 21, 2020 at 01:13:14AM +0300, Vladimir Oltean wrote: > > I think at least part of the reason why this keeps going on is that > > there aren't any hard and fast rules that say you shouldn't do it. When > > there isn't even a convincing percentage of DSA/PHY drivers that do set > > SKBTX_HW_TSTAMP, the chances are pretty low that you'll get a stacked > > PHC driver that sets the flag, plus a MAC driver that checks for it > > incorrectly. So people tend to ignore this case. > > Right. > > > Even though, if stacked > > DSA drivers started supporting software TX timestamping (which is not > > unlikely, given the fact that this would also give you compatibility > > with PHY timestamping), I'm sure things would change, because more > > people would become aware of the issue once mv88e6xxx starts getting > > affected. > > I really can't see the utility in having a SW time stamp from a DSA > interface. The whole point of DSA time stamping is to get the ingress > and egress time of frames on the external ports, in order to correct > for the residence time within the switch. > > Thanks, > Richard I understand where this is coming from. The DSA software data path is the mirror image of the hardware data path: first the net device corresponding to the switch port, then the net device corresponding to the host port, then the physical host port, then the physical switch port. So, just as hardware timestamping makes the most sense on the outermost PHC, software timestamping makes the most sense on the innermost driver, the last frontier before the packet leaves software hands. That is clear. But I feel that going as far as saying that 'DSA shouldn't set SKBTX_IN_PROGRESS because it already offers hardware timestamping' is wrong. A software timestamp provided by a DSA net device is just as valuable (or not, depending on your needs) as a software timestamp provided by any other net device. For example, to the people doing TCP time stamping, this software timestamp is just 'the driver timestamp', so it makes perfect sense to have it just where it is: in DSA. Not only that, but we shouldn't completely rule out the idea of software TX timestamps in DSA _and_ in the host interface for the same packet, either, since that could form the basis for some nice benchmarking. Not only that, but with PHY timestamping, one popular way of handling TX hardware timestamps from a PHY is to call skb_tx_timestamp(). It is nonsense to me, and counterproductive, to end up having that in the code, but not claim SOF_TIMESTAMPING_TX_SOFTWARE support. And PHY timestamping with DSA is not a contradiction in terms by any means, on the contrary, it makes just as much sense as PHY timestamping in general. So I think the position of "just don't have software timestamping code in DSA and you'll be fine" won't be getting us anywhere. Either you can or you can't, and there isn't anything absurd about it, so sooner or later somebody will want to do it. The rules surrounding it, however, are far from being ready, or clear. Am I missing something? Thanks, -Vladimir
On Tue, Jul 21, 2020 at 10:51:27PM +0300, Vladimir Oltean wrote: > So I think the position of "just don't have software timestamping code > in DSA and you'll be fine" won't be getting us anywhere. Either you can > or you can't, and there isn't anything absurd about it, so sooner or > later somebody will want to do it. The rules surrounding it, however, > are far from being ready, or clear. > > Am I missing something? I'm just trying to make things easy for you, as the author of DSA drivers. There is no need to set skb flags that have no purpose within the stack. Nobody is demanding software time stamps from any DSA devices yet, and so I don't see the point in solving a problem that doesn't exist. I'm sorry if the "rules" are not clear, but if you look around the kernel internals, you will be hard pressed to find perfectly documented rules anywhere! Thanks, Richard
On Tue, Jul 21, 2020 at 08:25:53PM -0700, Richard Cochran wrote: > On Tue, Jul 21, 2020 at 10:51:27PM +0300, Vladimir Oltean wrote: > > So I think the position of "just don't have software timestamping code > > in DSA and you'll be fine" won't be getting us anywhere. Either you can > > or you can't, and there isn't anything absurd about it, so sooner or > > later somebody will want to do it. The rules surrounding it, however, > > are far from being ready, or clear. > > > > Am I missing something? > > I'm just trying to make things easy for you, as the author of DSA > drivers. There is no need to set skb flags that have no purpose > within the stack. > > Nobody is demanding software time stamps from any DSA devices yet, and > so I don't see the point in solving a problem that doesn't exist. > > I'm sorry if the "rules" are not clear, but if you look around the > kernel internals, you will be hard pressed to find perfectly > documented rules anywhere! > > Thanks, > Richard Could we perhaps take a step back and see what can be improved about the documentation updates? Thanks, -Vladimir
diff --git a/Documentation/networking/timestamping.rst b/Documentation/networking/timestamping.rst index 4004c5d2771d..e01ec01179fe 100644 --- a/Documentation/networking/timestamping.rst +++ b/Documentation/networking/timestamping.rst @@ -791,3 +791,29 @@ 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. + +3.4 Frequently asked questions +------------------------------ + +Q: When should drivers set SKBTX_IN_PROGRESS? +^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + +When the interface they represent offers both ``SOF_TIMESTAMPING_TX_HARDWARE`` +and ``SOF_TIMESTAMPING_TX_SOFTWARE``. +Originally, the network stack could deliver either a hardware or a software +time stamp, but not both. This flag prevents software timestamp delivery. +This restriction was eventually lifted via the ``SOF_TIMESTAMPING_OPT_TX_SWHW`` +option, but still the original behavior is preserved as the default. + +Q: Should drivers that don't offer SOF_TIMESTAMPING_TX_SOFTWARE call skb_tx_timestamp()? +^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + +The ``skb_clone_tx_timestamp()`` function from its body helps with propagation +of TX timestamps from PTP PHYs, and the required placement of this call is the +same as for software TX timestamping. +Additionally, since PTP is broken on ports with timestamping PHYs and unmet +requirements, the consequence is that any driver which may be ever coupled to +a timestamping-capable PHY in ``netdev->phydev`` should call at least +``skb_clone_tx_timestamp()``. However, calling the higher-level +``skb_tx_timestamp()`` instead achieves the same purpose, but also offers +additional compliance to ``SOF_TIMESTAMPING_TX_SOFTWARE``.
These are some questions I had while trying to explain the behavior of some drivers with respect to software timestamping. Answered with the help of Richard Cochran. Signed-off-by: Vladimir Oltean <olteanv@gmail.com> --- Documentation/networking/timestamping.rst | 26 +++++++++++++++++++++++ 1 file changed, 26 insertions(+)