| Message ID | 20211104133204.19757-8-martin.kaistra@linutronix.de (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | Add PTP support for BCM53128 switch | expand |
On Thu, Nov 04, 2021 at 02:32:01PM +0100, Martin Kaistra wrote: > +static int b53_set_hwtstamp_config(struct b53_device *dev, int port, > + struct hwtstamp_config *config) > +{ > + struct b53_port_hwtstamp *ps = &dev->ports[port].port_hwtstamp; > + bool tstamp_enable = false; > + > + clear_bit_unlock(B53_HWTSTAMP_ENABLED, &ps->state); > + > + /* Reserved for future extensions */ > + if (config->flags) > + return -EINVAL; > + > + switch (config->tx_type) { > + case HWTSTAMP_TX_ON: > + tstamp_enable = true; > + break; > + case HWTSTAMP_TX_OFF: > + tstamp_enable = false; > + break; > + default: > + return -ERANGE; > + } > + > + switch (config->rx_filter) { > + case HWTSTAMP_FILTER_NONE: > + tstamp_enable = false; > + break; > + case HWTSTAMP_FILTER_PTP_V2_L2_EVENT: > + case HWTSTAMP_FILTER_PTP_V2_L2_SYNC: > + case HWTSTAMP_FILTER_PTP_V2_L2_DELAY_REQ: > + case HWTSTAMP_FILTER_PTP_V2_EVENT: > + case HWTSTAMP_FILTER_PTP_V2_SYNC: > + case HWTSTAMP_FILTER_PTP_V2_DELAY_REQ: This is incorrect. HWTSTAMP_FILTER_PTP_V2_EVENT includes support for UDP/IPv4 and UDP/IPv6. Driver should return error here. > + case HWTSTAMP_FILTER_ALL: > + config->rx_filter = HWTSTAMP_FILTER_PTP_V2_L2_EVENT; > + break; > + default: > + return -ERANGE; > + } Thanks, Richard
Am 04.11.21 um 18:42 schrieb Richard Cochran: > On Thu, Nov 04, 2021 at 02:32:01PM +0100, Martin Kaistra wrote: >> +static int b53_set_hwtstamp_config(struct b53_device *dev, int port, >> + struct hwtstamp_config *config) >> +{ >> + struct b53_port_hwtstamp *ps = &dev->ports[port].port_hwtstamp; >> + bool tstamp_enable = false; >> + >> + clear_bit_unlock(B53_HWTSTAMP_ENABLED, &ps->state); >> + >> + /* Reserved for future extensions */ >> + if (config->flags) >> + return -EINVAL; >> + >> + switch (config->tx_type) { >> + case HWTSTAMP_TX_ON: >> + tstamp_enable = true; >> + break; >> + case HWTSTAMP_TX_OFF: >> + tstamp_enable = false; >> + break; >> + default: >> + return -ERANGE; >> + } >> + >> + switch (config->rx_filter) { >> + case HWTSTAMP_FILTER_NONE: >> + tstamp_enable = false; >> + break; >> + case HWTSTAMP_FILTER_PTP_V2_L2_EVENT: >> + case HWTSTAMP_FILTER_PTP_V2_L2_SYNC: >> + case HWTSTAMP_FILTER_PTP_V2_L2_DELAY_REQ: >> + case HWTSTAMP_FILTER_PTP_V2_EVENT: >> + case HWTSTAMP_FILTER_PTP_V2_SYNC: >> + case HWTSTAMP_FILTER_PTP_V2_DELAY_REQ: > > This is incorrect. HWTSTAMP_FILTER_PTP_V2_EVENT includes support for > UDP/IPv4 and UDP/IPv6. Driver should return error here. Ok, then I will remove HWTSTAMP_FILTER_PTP_V2_(EVENT|SYNC|DELAY_REQ) from this list, what about HWTSTAMP_FILTER_ALL? > >> + case HWTSTAMP_FILTER_ALL: >> + config->rx_filter = HWTSTAMP_FILTER_PTP_V2_L2_EVENT; >> + break; >> + default: >> + return -ERANGE; >> + } > > Thanks, > Richard >
On Fri, Nov 05, 2021 at 02:38:01PM +0100, Martin Kaistra wrote: > Ok, then I will remove HWTSTAMP_FILTER_PTP_V2_(EVENT|SYNC|DELAY_REQ) from > this list, what about HWTSTAMP_FILTER_ALL? AKK means time stamp every received frame, so your driver should return an error in this case as well. Thanks, Richard
On Fri, Nov 05, 2021 at 07:13:19AM -0700, Richard Cochran wrote: > On Fri, Nov 05, 2021 at 02:38:01PM +0100, Martin Kaistra wrote: > > Ok, then I will remove HWTSTAMP_FILTER_PTP_V2_(EVENT|SYNC|DELAY_REQ) from > > this list, what about HWTSTAMP_FILTER_ALL? > > AKK means time stamp every received frame, so your driver should s/AKK/ALL/ > return an error in this case as well. > > Thanks, > Richard >
On Fri, Nov 05, 2021 at 07:13:19AM -0700, Richard Cochran wrote: > On Fri, Nov 05, 2021 at 02:38:01PM +0100, Martin Kaistra wrote: > > Ok, then I will remove HWTSTAMP_FILTER_PTP_V2_(EVENT|SYNC|DELAY_REQ) from > > this list, what about HWTSTAMP_FILTER_ALL? > > AKK means time stamp every received frame, so your driver should > return an error in this case as well. What is the expected convention exactly? There are other drivers that downgrade the user application's request to what they support, and at least ptp4l does not error out, it just prints a warning.
On Fri, 5 Nov 2021 16:28:33 +0200 Vladimir Oltean wrote: > On Fri, Nov 05, 2021 at 07:13:19AM -0700, Richard Cochran wrote: > > On Fri, Nov 05, 2021 at 02:38:01PM +0100, Martin Kaistra wrote: > > > Ok, then I will remove HWTSTAMP_FILTER_PTP_V2_(EVENT|SYNC|DELAY_REQ) from > > > this list, what about HWTSTAMP_FILTER_ALL? > > > > AKK means time stamp every received frame, so your driver should > > return an error in this case as well. > > What is the expected convention exactly? There are other drivers that > downgrade the user application's request to what they support, and at > least ptp4l does not error out, it just prints a warning. Which is sad because that's one of the best documented parts of our API: Desired behavior is passed into the kernel and to a specific device by calling ioctl(SIOCSHWTSTAMP) with a pointer to a struct ifreq whose ifr_data points to a struct hwtstamp_config. The tx_type and rx_filter are hints to the driver what it is expected to do. If the requested fine-grained filtering for incoming packets is not supported, the driver may time stamp more than just the requested types of packets. Drivers are free to use a more permissive configuration than the requested configuration. It is expected that drivers should only implement directly the most generic mode that can be supported. For example if the hardware can support HWTSTAMP_FILTER_V2_EVENT, then it should generally always upscale HWTSTAMP_FILTER_V2_L2_SYNC_MESSAGE, and so forth, as HWTSTAMP_FILTER_V2_EVENT is more generic (and more useful to applications). A driver which supports hardware time stamping shall update the struct with the actual, possibly more permissive configuration. If the requested packets cannot be time stamped, then nothing should be changed and ERANGE shall be returned (in contrast to EINVAL, which indicates that SIOCSHWTSTAMP is not supported at all). https://www.kernel.org/doc/html/latest/networking/timestamping.html#hardware-timestamping-configuration-siocshwtstamp-and-siocghwtstamp
On Fri, Nov 05, 2021 at 08:09:39AM -0700, Jakub Kicinski wrote: > On Fri, 5 Nov 2021 16:28:33 +0200 Vladimir Oltean wrote: > > On Fri, Nov 05, 2021 at 07:13:19AM -0700, Richard Cochran wrote: > > > On Fri, Nov 05, 2021 at 02:38:01PM +0100, Martin Kaistra wrote: > > > > Ok, then I will remove HWTSTAMP_FILTER_PTP_V2_(EVENT|SYNC|DELAY_REQ) from > > > > this list, what about HWTSTAMP_FILTER_ALL? > > > > > > AKK means time stamp every received frame, so your driver should > > > return an error in this case as well. > > > > What is the expected convention exactly? There are other drivers that > > downgrade the user application's request to what they support, and at > > least ptp4l does not error out, it just prints a warning. > > Which is sad because that's one of the best documented parts of our API: > > Desired behavior is passed into the kernel and to a specific device by > calling ioctl(SIOCSHWTSTAMP) with a pointer to a struct ifreq whose > ifr_data points to a struct hwtstamp_config. The tx_type and > rx_filter are hints to the driver what it is expected to do. If > the requested fine-grained filtering for incoming packets is not > supported, the driver may time stamp more than just the requested types > of packets. > > Drivers are free to use a more permissive configuration than the requested > configuration. It is expected that drivers should only implement directly the > most generic mode that can be supported. For example if the hardware can > support HWTSTAMP_FILTER_V2_EVENT, then it should generally always upscale > HWTSTAMP_FILTER_V2_L2_SYNC_MESSAGE, and so forth, as HWTSTAMP_FILTER_V2_EVENT > is more generic (and more useful to applications). > > A driver which supports hardware time stamping shall update the struct > with the actual, possibly more permissive configuration. If the > requested packets cannot be time stamped, then nothing should be > changed and ERANGE shall be returned (in contrast to EINVAL, which > indicates that SIOCSHWTSTAMP is not supported at all). > > https://www.kernel.org/doc/html/latest/networking/timestamping.html#hardware-timestamping-configuration-siocshwtstamp-and-siocghwtstamp Yeah, sorry, I've been all over that documentation file for the past few days, but I missed that section. "that's one of the best documented parts of our API" is a nice euphemism for all the SO_TIMESTAMPING flags :)
On Fri, Nov 05, 2021 at 04:28:33PM +0200, Vladimir Oltean wrote: > What is the expected convention exactly? There are other drivers that > downgrade the user application's request to what they support, and at > least ptp4l does not error out, it just prints a warning. Drivers may upgrade, but they may not downgrade. Which drivers downgrade? We need to fix those buggy drivers. Thanks, Richard
On Fri, Nov 05, 2021 at 05:18:04PM -0700, Richard Cochran wrote: > On Fri, Nov 05, 2021 at 04:28:33PM +0200, Vladimir Oltean wrote: > > What is the expected convention exactly? There are other drivers that > > downgrade the user application's request to what they support, and at > > least ptp4l does not error out, it just prints a warning. > > Drivers may upgrade, but they may not downgrade. > > Which drivers downgrade? We need to fix those buggy drivers. > > Thanks, > Richard Just a quick example https://elixir.bootlin.com/linux/v5.15/source/drivers/net/ethernet/mscc/ocelot.c#L1178 I haven't studied the whole tree, but I'm sure there are many more.
On Sat, Nov 06, 2021 at 02:36:06AM +0200, Vladimir Oltean wrote: > On Fri, Nov 05, 2021 at 05:18:04PM -0700, Richard Cochran wrote: > > On Fri, Nov 05, 2021 at 04:28:33PM +0200, Vladimir Oltean wrote: > > > What is the expected convention exactly? There are other drivers that > > > downgrade the user application's request to what they support, and at > > > least ptp4l does not error out, it just prints a warning. > > > > Drivers may upgrade, but they may not downgrade. > > > > Which drivers downgrade? We need to fix those buggy drivers. > > > > Thanks, > > Richard > > Just a quick example > https://elixir.bootlin.com/linux/v5.15/source/drivers/net/ethernet/mscc/ocelot.c#L1178 switch (cfg.rx_filter) { case HWTSTAMP_FILTER_NONE: break; case HWTSTAMP_FILTER_ALL: case HWTSTAMP_FILTER_SOME: case HWTSTAMP_FILTER_PTP_V1_L4_EVENT: case HWTSTAMP_FILTER_PTP_V1_L4_SYNC: case HWTSTAMP_FILTER_PTP_V1_L4_DELAY_REQ: case HWTSTAMP_FILTER_NTP_ALL: case HWTSTAMP_FILTER_PTP_V2_L4_EVENT: case HWTSTAMP_FILTER_PTP_V2_L4_SYNC: case HWTSTAMP_FILTER_PTP_V2_L4_DELAY_REQ: case HWTSTAMP_FILTER_PTP_V2_L2_EVENT: case HWTSTAMP_FILTER_PTP_V2_L2_SYNC: case HWTSTAMP_FILTER_PTP_V2_L2_DELAY_REQ: case HWTSTAMP_FILTER_PTP_V2_EVENT: case HWTSTAMP_FILTER_PTP_V2_SYNC: case HWTSTAMP_FILTER_PTP_V2_DELAY_REQ: cfg.rx_filter = HWTSTAMP_FILTER_PTP_V2_EVENT; break; default: mutex_unlock(&ocelot->ptp_lock); return -ERANGE; } That is essentially an upgrade to HWTSTAMP_FILTER_PTP_V2_EVENT. The change from ALL to HWTSTAMP_FILTER_PTP_V2_EVENT is probably a simple oversight, and the driver can be easily fixed. Thanks, Richard
On Sun, Nov 07, 2021 at 06:05:34AM -0800, Richard Cochran wrote: > On Sat, Nov 06, 2021 at 02:36:06AM +0200, Vladimir Oltean wrote: > > On Fri, Nov 05, 2021 at 05:18:04PM -0700, Richard Cochran wrote: > > > On Fri, Nov 05, 2021 at 04:28:33PM +0200, Vladimir Oltean wrote: > > > > What is the expected convention exactly? There are other drivers that > > > > downgrade the user application's request to what they support, and at > > > > least ptp4l does not error out, it just prints a warning. > > > > > > Drivers may upgrade, but they may not downgrade. > > > > > > Which drivers downgrade? We need to fix those buggy drivers. > > > > > > Thanks, > > > Richard > > > > Just a quick example > > https://elixir.bootlin.com/linux/v5.15/source/drivers/net/ethernet/mscc/ocelot.c#L1178 > > switch (cfg.rx_filter) { > case HWTSTAMP_FILTER_NONE: > break; > case HWTSTAMP_FILTER_ALL: > case HWTSTAMP_FILTER_SOME: > case HWTSTAMP_FILTER_PTP_V1_L4_EVENT: > case HWTSTAMP_FILTER_PTP_V1_L4_SYNC: > case HWTSTAMP_FILTER_PTP_V1_L4_DELAY_REQ: > case HWTSTAMP_FILTER_NTP_ALL: > case HWTSTAMP_FILTER_PTP_V2_L4_EVENT: > case HWTSTAMP_FILTER_PTP_V2_L4_SYNC: > case HWTSTAMP_FILTER_PTP_V2_L4_DELAY_REQ: > case HWTSTAMP_FILTER_PTP_V2_L2_EVENT: > case HWTSTAMP_FILTER_PTP_V2_L2_SYNC: > case HWTSTAMP_FILTER_PTP_V2_L2_DELAY_REQ: > case HWTSTAMP_FILTER_PTP_V2_EVENT: > case HWTSTAMP_FILTER_PTP_V2_SYNC: > case HWTSTAMP_FILTER_PTP_V2_DELAY_REQ: > cfg.rx_filter = HWTSTAMP_FILTER_PTP_V2_EVENT; > break; > default: > mutex_unlock(&ocelot->ptp_lock); > return -ERANGE; > } > > That is essentially an upgrade to HWTSTAMP_FILTER_PTP_V2_EVENT. The > change from ALL to HWTSTAMP_FILTER_PTP_V2_EVENT is probably a simple > oversight, and the driver can be easily fixed. > > Thanks, > Richard It's essentially the same pattern as what Martin is introducing for b53.
On Sun, Nov 07, 2021 at 04:27:03PM +0200, Vladimir Oltean wrote: > On Sun, Nov 07, 2021 at 06:05:34AM -0800, Richard Cochran wrote: > > switch (cfg.rx_filter) { > > case HWTSTAMP_FILTER_NONE: > > break; > > case HWTSTAMP_FILTER_ALL: > > case HWTSTAMP_FILTER_SOME: > > case HWTSTAMP_FILTER_PTP_V1_L4_EVENT: > > case HWTSTAMP_FILTER_PTP_V1_L4_SYNC: > > case HWTSTAMP_FILTER_PTP_V1_L4_DELAY_REQ: > > case HWTSTAMP_FILTER_NTP_ALL: > > case HWTSTAMP_FILTER_PTP_V2_L4_EVENT: > > case HWTSTAMP_FILTER_PTP_V2_L4_SYNC: > > case HWTSTAMP_FILTER_PTP_V2_L4_DELAY_REQ: > > case HWTSTAMP_FILTER_PTP_V2_L2_EVENT: > > case HWTSTAMP_FILTER_PTP_V2_L2_SYNC: > > case HWTSTAMP_FILTER_PTP_V2_L2_DELAY_REQ: > > case HWTSTAMP_FILTER_PTP_V2_EVENT: > > case HWTSTAMP_FILTER_PTP_V2_SYNC: > > case HWTSTAMP_FILTER_PTP_V2_DELAY_REQ: > > cfg.rx_filter = HWTSTAMP_FILTER_PTP_V2_EVENT; > > break; > > default: > > mutex_unlock(&ocelot->ptp_lock); > > return -ERANGE; > > } > > > > That is essentially an upgrade to HWTSTAMP_FILTER_PTP_V2_EVENT. The > > change from ALL to HWTSTAMP_FILTER_PTP_V2_EVENT is probably a simple > > oversight, and the driver can be easily fixed. > > > > Thanks, > > Richard > > It's essentially the same pattern as what Martin is introducing for b53. Uh, no it isn't. The present patch has: + case HWTSTAMP_FILTER_PTP_V2_L2_EVENT: + case HWTSTAMP_FILTER_PTP_V2_L2_SYNC: + case HWTSTAMP_FILTER_PTP_V2_L2_DELAY_REQ: + case HWTSTAMP_FILTER_PTP_V2_EVENT: + case HWTSTAMP_FILTER_PTP_V2_SYNC: + case HWTSTAMP_FILTER_PTP_V2_DELAY_REQ: + case HWTSTAMP_FILTER_ALL: + config->rx_filter = HWTSTAMP_FILTER_PTP_V2_L2_EVENT; There is an important difference between HWTSTAMP_FILTER_PTP_V2_L2_EVENT and HWTSTAMP_FILTER_PTP_V2_EVENT Notice the "L2" in there. Thanks, Richard
On Mon, Nov 08, 2021 at 06:48:24AM -0800, Richard Cochran wrote: > On Sun, Nov 07, 2021 at 04:27:03PM +0200, Vladimir Oltean wrote: > > On Sun, Nov 07, 2021 at 06:05:34AM -0800, Richard Cochran wrote: > > > switch (cfg.rx_filter) { > > > case HWTSTAMP_FILTER_NONE: > > > break; > > > case HWTSTAMP_FILTER_ALL: > > > case HWTSTAMP_FILTER_SOME: > > > case HWTSTAMP_FILTER_PTP_V1_L4_EVENT: > > > case HWTSTAMP_FILTER_PTP_V1_L4_SYNC: > > > case HWTSTAMP_FILTER_PTP_V1_L4_DELAY_REQ: > > > case HWTSTAMP_FILTER_NTP_ALL: > > > case HWTSTAMP_FILTER_PTP_V2_L4_EVENT: > > > case HWTSTAMP_FILTER_PTP_V2_L4_SYNC: > > > case HWTSTAMP_FILTER_PTP_V2_L4_DELAY_REQ: > > > case HWTSTAMP_FILTER_PTP_V2_L2_EVENT: > > > case HWTSTAMP_FILTER_PTP_V2_L2_SYNC: > > > case HWTSTAMP_FILTER_PTP_V2_L2_DELAY_REQ: > > > case HWTSTAMP_FILTER_PTP_V2_EVENT: > > > case HWTSTAMP_FILTER_PTP_V2_SYNC: > > > case HWTSTAMP_FILTER_PTP_V2_DELAY_REQ: > > > cfg.rx_filter = HWTSTAMP_FILTER_PTP_V2_EVENT; > > > break; > > > default: > > > mutex_unlock(&ocelot->ptp_lock); > > > return -ERANGE; > > > } > > > > > > That is essentially an upgrade to HWTSTAMP_FILTER_PTP_V2_EVENT. The > > > change from ALL to HWTSTAMP_FILTER_PTP_V2_EVENT is probably a simple > > > oversight, and the driver can be easily fixed. > > > > > > Thanks, > > > Richard > > > > It's essentially the same pattern as what Martin is introducing for b53. > > Uh, no it isn't. The present patch has: > > + case HWTSTAMP_FILTER_PTP_V2_L2_EVENT: > + case HWTSTAMP_FILTER_PTP_V2_L2_SYNC: > + case HWTSTAMP_FILTER_PTP_V2_L2_DELAY_REQ: > + case HWTSTAMP_FILTER_PTP_V2_EVENT: > + case HWTSTAMP_FILTER_PTP_V2_SYNC: > + case HWTSTAMP_FILTER_PTP_V2_DELAY_REQ: > + case HWTSTAMP_FILTER_ALL: > + config->rx_filter = HWTSTAMP_FILTER_PTP_V2_L2_EVENT; > > There is an important difference between > HWTSTAMP_FILTER_PTP_V2_L2_EVENT and HWTSTAMP_FILTER_PTP_V2_EVENT > > Notice the "L2" in there. Richard, when the request is PTP_V2_EVENT and the response is PTP_V2_L2_EVENT, is that an upgrade or a downgrade? PTP_V2_EVENT also includes PTP_V2_L4_EVENT.
On Thu Nov 25 2021, Vladimir Oltean wrote: > On Mon, Nov 08, 2021 at 06:48:24AM -0800, Richard Cochran wrote: >> On Sun, Nov 07, 2021 at 04:27:03PM +0200, Vladimir Oltean wrote: >> > On Sun, Nov 07, 2021 at 06:05:34AM -0800, Richard Cochran wrote: >> > > switch (cfg.rx_filter) { >> > > case HWTSTAMP_FILTER_NONE: >> > > break; >> > > case HWTSTAMP_FILTER_ALL: >> > > case HWTSTAMP_FILTER_SOME: >> > > case HWTSTAMP_FILTER_PTP_V1_L4_EVENT: >> > > case HWTSTAMP_FILTER_PTP_V1_L4_SYNC: >> > > case HWTSTAMP_FILTER_PTP_V1_L4_DELAY_REQ: >> > > case HWTSTAMP_FILTER_NTP_ALL: >> > > case HWTSTAMP_FILTER_PTP_V2_L4_EVENT: >> > > case HWTSTAMP_FILTER_PTP_V2_L4_SYNC: >> > > case HWTSTAMP_FILTER_PTP_V2_L4_DELAY_REQ: >> > > case HWTSTAMP_FILTER_PTP_V2_L2_EVENT: >> > > case HWTSTAMP_FILTER_PTP_V2_L2_SYNC: >> > > case HWTSTAMP_FILTER_PTP_V2_L2_DELAY_REQ: >> > > case HWTSTAMP_FILTER_PTP_V2_EVENT: >> > > case HWTSTAMP_FILTER_PTP_V2_SYNC: >> > > case HWTSTAMP_FILTER_PTP_V2_DELAY_REQ: >> > > cfg.rx_filter = HWTSTAMP_FILTER_PTP_V2_EVENT; >> > > break; >> > > default: >> > > mutex_unlock(&ocelot->ptp_lock); >> > > return -ERANGE; >> > > } >> > > >> > > That is essentially an upgrade to HWTSTAMP_FILTER_PTP_V2_EVENT. The >> > > change from ALL to HWTSTAMP_FILTER_PTP_V2_EVENT is probably a simple >> > > oversight, and the driver can be easily fixed. >> > > >> > > Thanks, >> > > Richard >> > >> > It's essentially the same pattern as what Martin is introducing for b53. >> >> Uh, no it isn't. The present patch has: >> >> + case HWTSTAMP_FILTER_PTP_V2_L2_EVENT: >> + case HWTSTAMP_FILTER_PTP_V2_L2_SYNC: >> + case HWTSTAMP_FILTER_PTP_V2_L2_DELAY_REQ: >> + case HWTSTAMP_FILTER_PTP_V2_EVENT: >> + case HWTSTAMP_FILTER_PTP_V2_SYNC: >> + case HWTSTAMP_FILTER_PTP_V2_DELAY_REQ: >> + case HWTSTAMP_FILTER_ALL: >> + config->rx_filter = HWTSTAMP_FILTER_PTP_V2_L2_EVENT; >> >> There is an important difference between >> HWTSTAMP_FILTER_PTP_V2_L2_EVENT and HWTSTAMP_FILTER_PTP_V2_EVENT >> >> Notice the "L2" in there. > > Richard, when the request is PTP_V2_EVENT and the response is > PTP_V2_L2_EVENT, is that an upgrade or a downgrade? It is a downgrade, isn't it? > PTP_V2_EVENT also includes PTP_V2_L4_EVENT. Yes, exactly. Thanks, Kurt
On Fri, Nov 26, 2021 at 09:42:32AM +0100, Kurt Kanzenbach wrote: > On Thu Nov 25 2021, Vladimir Oltean wrote: > > Richard, when the request is PTP_V2_EVENT and the response is > > PTP_V2_L2_EVENT, is that an upgrade or a downgrade? > > It is a downgrade, isn't it? Yes. "Any kind of PTP Event" is a superset of "Any Layer-2 Event". When userland asks for "any kind", then it wants to run PTP over IPv4, IPv6, or Layer2, maybe even more than one at the same time. If the driver changes that to Layer2 only, then the PTP possibilities have been downgraded. Thanks, Richard
On Fri, 26 Nov 2021 at 18:31, Richard Cochran <richardcochran@gmail.com> wrote: > > On Fri, Nov 26, 2021 at 09:42:32AM +0100, Kurt Kanzenbach wrote: > > On Thu Nov 25 2021, Vladimir Oltean wrote: > > > Richard, when the request is PTP_V2_EVENT and the response is > > > PTP_V2_L2_EVENT, is that an upgrade or a downgrade? > > > > It is a downgrade, isn't it? > > Yes. "Any kind of PTP Event" is a superset of "Any Layer-2 Event". > > When userland asks for "any kind", then it wants to run PTP over IPv4, > IPv6, or Layer2, maybe even more than one at the same time. If the > driver changes that to Layer2 only, then the PTP possibilities have > been downgraded. Well, when I said that it's essentially the same pattern, this is what I was talking about. The b53 driver downgrades everything and the kitchen sink to HWTSTAMP_FILTER_PTP_V2_L2_EVENT, the ocelot driver to HWTSTAMP_FILTER_PTP_V2_EVENT, and both are buggy for the same reason. I don't see why you mention that there is an important difference between HWTSTAMP_FILTER_PTP_V2_L2_EVENT and HWTSTAMP_FILTER_PTP_V2_EVENT. I know there is, but the _pattern_ is the same. I'm still missing something obvious, aren't I?
On Fri, Nov 26, 2021 at 06:42:57PM +0200, Vladimir Oltean wrote:
> I'm still missing something obvious, aren't I?
You said there are "many more" drivers with this bug, but I'm saying
that most drivers correctly upgrade the ioctl request.
So far we have b53 and ocelot doing the buggy downgrade. I guess it
will require a tree wide audit to discover the "many more"...
Thanks,
Richard
On Fri, Nov 26, 2021 at 09:03:48AM -0800, Richard Cochran wrote: > On Fri, Nov 26, 2021 at 06:42:57PM +0200, Vladimir Oltean wrote: > > I'm still missing something obvious, aren't I? > > You said there are "many more" drivers with this bug, but I'm saying > that most drivers correctly upgrade the ioctl request. > > So far we have b53 and ocelot doing the buggy downgrade. I guess it > will require a tree wide audit to discover the "many more"... Ah, yes, I assure you that there are many more drivers doing wacky stuff, for example sja1105 will take any RX filter that isn't NONE, and then reports it back as PTP_V2_L2_EVENT. https://elixir.bootlin.com/linux/latest/source/drivers/net/dsa/sja1105/sja1105_ptp.c#L89 Somehow at this stage I don't even want to know about any other drivers, since I might feel the urge to patch them and I don't really have the necessary free time for that right now :D
diff --git a/drivers/net/dsa/b53/b53_common.c b/drivers/net/dsa/b53/b53_common.c index 56a9de89b38b..3e7e5f83cc84 100644 --- a/drivers/net/dsa/b53/b53_common.c +++ b/drivers/net/dsa/b53/b53_common.c @@ -2302,6 +2302,8 @@ static const struct dsa_switch_ops b53_switch_ops = { .get_ts_info = b53_get_ts_info, .port_rxtstamp = b53_port_rxtstamp, .port_txtstamp = b53_port_txtstamp, + .port_hwtstamp_set = b53_port_hwtstamp_set, + .port_hwtstamp_get = b53_port_hwtstamp_get, }; struct b53_chip_data { diff --git a/drivers/net/dsa/b53/b53_ptp.c b/drivers/net/dsa/b53/b53_ptp.c index 7cb4d1c9d6f7..a0a91134d2d8 100644 --- a/drivers/net/dsa/b53/b53_ptp.c +++ b/drivers/net/dsa/b53/b53_ptp.c @@ -263,12 +263,100 @@ int b53_get_ts_info(struct dsa_switch *ds, int port, info->so_timestamping = SOF_TIMESTAMPING_TX_HARDWARE | SOF_TIMESTAMPING_RX_HARDWARE | SOF_TIMESTAMPING_RAW_HARDWARE; - info->tx_types = BIT(HWTSTAMP_TX_OFF); - info->rx_filters = BIT(HWTSTAMP_FILTER_NONE); + info->tx_types = BIT(HWTSTAMP_TX_ON); + info->rx_filters = BIT(HWTSTAMP_FILTER_PTP_V2_L2_EVENT); return 0; } +static int b53_set_hwtstamp_config(struct b53_device *dev, int port, + struct hwtstamp_config *config) +{ + struct b53_port_hwtstamp *ps = &dev->ports[port].port_hwtstamp; + bool tstamp_enable = false; + + clear_bit_unlock(B53_HWTSTAMP_ENABLED, &ps->state); + + /* Reserved for future extensions */ + if (config->flags) + return -EINVAL; + + switch (config->tx_type) { + case HWTSTAMP_TX_ON: + tstamp_enable = true; + break; + case HWTSTAMP_TX_OFF: + tstamp_enable = false; + break; + default: + return -ERANGE; + } + + switch (config->rx_filter) { + case HWTSTAMP_FILTER_NONE: + tstamp_enable = false; + break; + case HWTSTAMP_FILTER_PTP_V2_L2_EVENT: + case HWTSTAMP_FILTER_PTP_V2_L2_SYNC: + case HWTSTAMP_FILTER_PTP_V2_L2_DELAY_REQ: + case HWTSTAMP_FILTER_PTP_V2_EVENT: + case HWTSTAMP_FILTER_PTP_V2_SYNC: + case HWTSTAMP_FILTER_PTP_V2_DELAY_REQ: + case HWTSTAMP_FILTER_ALL: + config->rx_filter = HWTSTAMP_FILTER_PTP_V2_L2_EVENT; + break; + default: + return -ERANGE; + } + + if (ps->tx_skb) { + dev_kfree_skb_any(ps->tx_skb); + ps->tx_skb = NULL; + } + clear_bit(B53_HWTSTAMP_TX_IN_PROGRESS, &ps->state); + + if (tstamp_enable) + set_bit(B53_HWTSTAMP_ENABLED, &ps->state); + + return 0; +} + +int b53_port_hwtstamp_set(struct dsa_switch *ds, int port, struct ifreq *ifr) +{ + struct b53_device *dev = ds->priv; + struct b53_port_hwtstamp *ps; + struct hwtstamp_config config; + int err; + + ps = &dev->ports[port].port_hwtstamp; + + if (copy_from_user(&config, ifr->ifr_data, sizeof(config))) + return -EFAULT; + + err = b53_set_hwtstamp_config(dev, port, &config); + if (err) + return err; + + /* Save the chosen configuration to be returned later */ + memcpy(&ps->tstamp_config, &config, sizeof(config)); + + return copy_to_user(ifr->ifr_data, &config, sizeof(config)) ? -EFAULT : + 0; +} + +int b53_port_hwtstamp_get(struct dsa_switch *ds, int port, struct ifreq *ifr) +{ + struct b53_device *dev = ds->priv; + struct b53_port_hwtstamp *ps; + struct hwtstamp_config *config; + + ps = &dev->ports[port].port_hwtstamp; + config = &ps->tstamp_config; + + return copy_to_user(ifr->ifr_data, config, sizeof(*config)) ? -EFAULT : + 0; +} + void b53_ptp_exit(struct b53_device *dev) { cancel_delayed_work_sync(&dev->overflow_work); diff --git a/drivers/net/dsa/b53/b53_ptp.h b/drivers/net/dsa/b53/b53_ptp.h index c76e3f4018d0..51146d451361 100644 --- a/drivers/net/dsa/b53/b53_ptp.h +++ b/drivers/net/dsa/b53/b53_ptp.h @@ -17,6 +17,8 @@ int b53_ptp_init(struct b53_device *dev); void b53_ptp_exit(struct b53_device *dev); int b53_get_ts_info(struct dsa_switch *ds, int port, struct ethtool_ts_info *info); +int b53_port_hwtstamp_set(struct dsa_switch *ds, int port, struct ifreq *ifr); +int b53_port_hwtstamp_get(struct dsa_switch *ds, int port, struct ifreq *ifr); bool b53_port_rxtstamp(struct dsa_switch *ds, int port, struct sk_buff *skb, unsigned int type); void b53_port_txtstamp(struct dsa_switch *ds, int port, struct sk_buff *skb); @@ -38,6 +40,18 @@ static inline int b53_get_ts_info(struct dsa_switch *ds, int port, return -EOPNOTSUPP; } +static inline int b53_port_hwtstamp_set(struct dsa_switch *ds, int port, + struct ifreq *ifr) +{ + return -EOPNOTSUPP; +} + +static inline int b53_port_hwtstamp_get(struct dsa_switch *ds, int port, + struct ifreq *ifr) +{ + return -EOPNOTSUPP; +} + static inline bool b53_port_rxtstamp(struct dsa_switch *ds, int port, struct sk_buff *skb, unsigned int type) {