| Message ID | 20210605002356.3996853-1-vinicius.gomes@intel.com |
|---|---|
| Headers | show |
| Series | igc: Add support for PCIe PTM | expand |
Dear Vinicius, Am 05.06.21 um 02:23 schrieb Vinicius Costa Gomes: > Enables PCIe PTM (Precision Time Measurement) support in the igc > driver. Notifies the PCI devices that PCIe PTM should be enabled. > > PCIe PTM is similar protocol to PTP (Precision Time Protocol) running > in the PCIe fabric, it allows devices to report time measurements from > their internal clocks and the correlation with the PCIe root clock. > > The i225 NIC exposes some registers that expose those time > measurements, those registers will be used, in later patches, to > implement the PTP_SYS_OFFSET_PRECISE ioctl(). > > Signed-off-by: Vinicius Costa Gomes <vinicius.gomes@intel.com> > --- > drivers/net/ethernet/intel/igc/igc_main.c | 6 ++++++ > 1 file changed, 6 insertions(+) > > diff --git a/drivers/net/ethernet/intel/igc/igc_main.c b/drivers/net/ethernet/intel/igc/igc_main.c > index a05e6d8ec660..f23d0303e53b 100644 > --- a/drivers/net/ethernet/intel/igc/igc_main.c > +++ b/drivers/net/ethernet/intel/igc/igc_main.c > @@ -12,6 +12,8 @@ > #include <net/pkt_sched.h> > #include <linux/bpf_trace.h> > #include <net/xdp_sock_drv.h> > +#include <linux/pci.h> > + > #include <net/ipv6.h> > > #include "igc.h" > @@ -5864,6 +5866,10 @@ static int igc_probe(struct pci_dev *pdev, > > pci_enable_pcie_error_reporting(pdev); > > + err = pci_enable_ptm(pdev, NULL); > + if (err < 0) > + dev_err(&pdev->dev, "PTM not supported\n"); > + Sorry, if I am missing something, but do all devices supported by this driver support PTM or only the i225 NIC? In that case, it wouldn’t be an error for a device not supporting PTM, would it? > pci_set_master(pdev); > > err = -ENOMEM; > Kind regards, Paul
Hi Paul, Paul Menzel <pmenzel@molgen.mpg.de> writes: > Dear Vinicius, > > > Am 05.06.21 um 02:23 schrieb Vinicius Costa Gomes: >> Enables PCIe PTM (Precision Time Measurement) support in the igc >> driver. Notifies the PCI devices that PCIe PTM should be enabled. >> >> PCIe PTM is similar protocol to PTP (Precision Time Protocol) running >> in the PCIe fabric, it allows devices to report time measurements from >> their internal clocks and the correlation with the PCIe root clock. >> >> The i225 NIC exposes some registers that expose those time >> measurements, those registers will be used, in later patches, to >> implement the PTP_SYS_OFFSET_PRECISE ioctl(). >> >> Signed-off-by: Vinicius Costa Gomes <vinicius.gomes@intel.com> >> --- >> drivers/net/ethernet/intel/igc/igc_main.c | 6 ++++++ >> 1 file changed, 6 insertions(+) >> >> diff --git a/drivers/net/ethernet/intel/igc/igc_main.c b/drivers/net/ethernet/intel/igc/igc_main.c >> index a05e6d8ec660..f23d0303e53b 100644 >> --- a/drivers/net/ethernet/intel/igc/igc_main.c >> +++ b/drivers/net/ethernet/intel/igc/igc_main.c >> @@ -12,6 +12,8 @@ >> #include <net/pkt_sched.h> >> #include <linux/bpf_trace.h> >> #include <net/xdp_sock_drv.h> >> +#include <linux/pci.h> >> + >> #include <net/ipv6.h> >> >> #include "igc.h" >> @@ -5864,6 +5866,10 @@ static int igc_probe(struct pci_dev *pdev, >> >> pci_enable_pcie_error_reporting(pdev); >> >> + err = pci_enable_ptm(pdev, NULL); >> + if (err < 0) >> + dev_err(&pdev->dev, "PTM not supported\n"); >> + > > Sorry, if I am missing something, but do all devices supported by this > driver support PTM or only the i225 NIC? In that case, it wouldn’t be an > error for a device not supporting PTM, would it? That was a very good question. I had to talk with the hardware folks. All the devices supported by the igc driver should support PTM. And just to be clear, the reason that I am not returning an error here is that PTM could not be supported by the host system (think PCI controller). > >> pci_set_master(pdev); >> >> err = -ENOMEM; >> > > > Kind regards, > > Paul Cheers, -- Vinicius
Dear Vinicius, Am 08.06.21 um 21:02 schrieb Vinicius Costa Gomes: > Paul Menzel writes: >> Am 05.06.21 um 02:23 schrieb Vinicius Costa Gomes: >>> Enables PCIe PTM (Precision Time Measurement) support in the igc >>> driver. Notifies the PCI devices that PCIe PTM should be enabled. >>> >>> PCIe PTM is similar protocol to PTP (Precision Time Protocol) running >>> in the PCIe fabric, it allows devices to report time measurements from >>> their internal clocks and the correlation with the PCIe root clock. >>> >>> The i225 NIC exposes some registers that expose those time >>> measurements, those registers will be used, in later patches, to >>> implement the PTP_SYS_OFFSET_PRECISE ioctl(). >>> >>> Signed-off-by: Vinicius Costa Gomes <vinicius.gomes@intel.com> >>> --- >>> drivers/net/ethernet/intel/igc/igc_main.c | 6 ++++++ >>> 1 file changed, 6 insertions(+) >>> >>> diff --git a/drivers/net/ethernet/intel/igc/igc_main.c b/drivers/net/ethernet/intel/igc/igc_main.c >>> index a05e6d8ec660..f23d0303e53b 100644 >>> --- a/drivers/net/ethernet/intel/igc/igc_main.c >>> +++ b/drivers/net/ethernet/intel/igc/igc_main.c >>> @@ -12,6 +12,8 @@ >>> #include <net/pkt_sched.h> >>> #include <linux/bpf_trace.h> >>> #include <net/xdp_sock_drv.h> >>> +#include <linux/pci.h> >>> + >>> #include <net/ipv6.h> >>> >>> #include "igc.h" >>> @@ -5864,6 +5866,10 @@ static int igc_probe(struct pci_dev *pdev, >>> >>> pci_enable_pcie_error_reporting(pdev); >>> >>> + err = pci_enable_ptm(pdev, NULL); >>> + if (err < 0) >>> + dev_err(&pdev->dev, "PTM not supported\n"); >>> + >> >> Sorry, if I am missing something, but do all devices supported by this >> driver support PTM or only the i225 NIC? In that case, it wouldn’t be an >> error for a device not supporting PTM, would it? > > That was a very good question. I had to talk with the hardware folks. > All the devices supported by the igc driver should support PTM. Thank you for checking that, that is valuable information. > And just to be clear, the reason that I am not returning an error here > is that PTM could not be supported by the host system (think PCI > controller). I just checked `pci_enable_ptm()` and on success it calls `pci_ptm_info()` logging a message: pci_info(dev, "PTM enabled%s, %s granularity\n", dev->ptm_root ? " (root)" : "", clock_desc); Was that present on your system with your patch? Please add that to the commit message. Regarding my comment, I did not mean returning an error but the log *level* of the message. So, `dmesg --level err` would show that message. But if there are PCI controllers not supporting that, it’s not an error, but a warning at most. So, I’d use: dev_warn(&pdev->dev, "PTM not supported by PCI bus/controller (pci_enable_ptm() failed)\n"); >>> pci_set_master(pdev); >>> >>> err = -ENOMEM; Kind regards, Paul
Paul Menzel <pmenzel@molgen.mpg.de> writes: > Dear Vinicius, > > > Am 08.06.21 um 21:02 schrieb Vinicius Costa Gomes: > >> Paul Menzel writes: > >>> Am 05.06.21 um 02:23 schrieb Vinicius Costa Gomes: >>>> Enables PCIe PTM (Precision Time Measurement) support in the igc >>>> driver. Notifies the PCI devices that PCIe PTM should be enabled. >>>> >>>> PCIe PTM is similar protocol to PTP (Precision Time Protocol) running >>>> in the PCIe fabric, it allows devices to report time measurements from >>>> their internal clocks and the correlation with the PCIe root clock. >>>> >>>> The i225 NIC exposes some registers that expose those time >>>> measurements, those registers will be used, in later patches, to >>>> implement the PTP_SYS_OFFSET_PRECISE ioctl(). >>>> >>>> Signed-off-by: Vinicius Costa Gomes <vinicius.gomes@intel.com> >>>> --- >>>> drivers/net/ethernet/intel/igc/igc_main.c | 6 ++++++ >>>> 1 file changed, 6 insertions(+) >>>> >>>> diff --git a/drivers/net/ethernet/intel/igc/igc_main.c b/drivers/net/ethernet/intel/igc/igc_main.c >>>> index a05e6d8ec660..f23d0303e53b 100644 >>>> --- a/drivers/net/ethernet/intel/igc/igc_main.c >>>> +++ b/drivers/net/ethernet/intel/igc/igc_main.c >>>> @@ -12,6 +12,8 @@ >>>> #include <net/pkt_sched.h> >>>> #include <linux/bpf_trace.h> >>>> #include <net/xdp_sock_drv.h> >>>> +#include <linux/pci.h> >>>> + >>>> #include <net/ipv6.h> >>>> >>>> #include "igc.h" >>>> @@ -5864,6 +5866,10 @@ static int igc_probe(struct pci_dev *pdev, >>>> >>>> pci_enable_pcie_error_reporting(pdev); >>>> >>>> + err = pci_enable_ptm(pdev, NULL); >>>> + if (err < 0) >>>> + dev_err(&pdev->dev, "PTM not supported\n"); >>>> + >>> >>> Sorry, if I am missing something, but do all devices supported by this >>> driver support PTM or only the i225 NIC? In that case, it wouldn’t be an >>> error for a device not supporting PTM, would it? >> >> That was a very good question. I had to talk with the hardware folks. >> All the devices supported by the igc driver should support PTM. > > Thank you for checking that, that is valuable information. > >> And just to be clear, the reason that I am not returning an error here >> is that PTM could not be supported by the host system (think PCI >> controller). > > I just checked `pci_enable_ptm()` and on success it calls > `pci_ptm_info()` logging a message: > > pci_info(dev, "PTM enabled%s, %s granularity\n", > dev->ptm_root ? " (root)" : "", clock_desc); > > Was that present on your system with your patch? Please add that to the > commit message. Yes, with my patches applied I can see this message on my systems. Sure, will add this to the commit message. > > Regarding my comment, I did not mean returning an error but the log > *level* of the message. So, `dmesg --level err` would show that message. > But if there are PCI controllers not supporting that, it’s not an error, > but a warning at most. So, I’d use: > > dev_warn(&pdev->dev, "PTM not supported by PCI bus/controller > (pci_enable_ptm() failed)\n"); I will use you suggestion for the message, but I think that warn is a bit too much, info or notice seem to be better. Cheers, -- Vinicius
Dear Vinicius, Am 09.06.21 um 22:08 schrieb Vinicius Costa Gomes: > Paul Menzel writes: >> Am 08.06.21 um 21:02 schrieb Vinicius Costa Gomes: >> >>> Paul Menzel writes: >> >>>> Am 05.06.21 um 02:23 schrieb Vinicius Costa Gomes: >>>>> Enables PCIe PTM (Precision Time Measurement) support in the igc >>>>> driver. Notifies the PCI devices that PCIe PTM should be enabled. >>>>> >>>>> PCIe PTM is similar protocol to PTP (Precision Time Protocol) running >>>>> in the PCIe fabric, it allows devices to report time measurements from >>>>> their internal clocks and the correlation with the PCIe root clock. >>>>> >>>>> The i225 NIC exposes some registers that expose those time >>>>> measurements, those registers will be used, in later patches, to >>>>> implement the PTP_SYS_OFFSET_PRECISE ioctl(). >>>>> >>>>> Signed-off-by: Vinicius Costa Gomes <vinicius.gomes@intel.com> >>>>> --- >>>>> drivers/net/ethernet/intel/igc/igc_main.c | 6 ++++++ >>>>> 1 file changed, 6 insertions(+) >>>>> >>>>> diff --git a/drivers/net/ethernet/intel/igc/igc_main.c b/drivers/net/ethernet/intel/igc/igc_main.c >>>>> index a05e6d8ec660..f23d0303e53b 100644 >>>>> --- a/drivers/net/ethernet/intel/igc/igc_main.c >>>>> +++ b/drivers/net/ethernet/intel/igc/igc_main.c >>>>> @@ -12,6 +12,8 @@ >>>>> #include <net/pkt_sched.h> >>>>> #include <linux/bpf_trace.h> >>>>> #include <net/xdp_sock_drv.h> >>>>> +#include <linux/pci.h> >>>>> + >>>>> #include <net/ipv6.h> >>>>> >>>>> #include "igc.h" >>>>> @@ -5864,6 +5866,10 @@ static int igc_probe(struct pci_dev *pdev, >>>>> >>>>> pci_enable_pcie_error_reporting(pdev); >>>>> >>>>> + err = pci_enable_ptm(pdev, NULL); >>>>> + if (err < 0) >>>>> + dev_err(&pdev->dev, "PTM not supported\n"); >>>>> + >>>> >>>> Sorry, if I am missing something, but do all devices supported by this >>>> driver support PTM or only the i225 NIC? In that case, it wouldn’t be an >>>> error for a device not supporting PTM, would it? >>> >>> That was a very good question. I had to talk with the hardware folks. >>> All the devices supported by the igc driver should support PTM. >> >> Thank you for checking that, that is valuable information. >> >>> And just to be clear, the reason that I am not returning an error here >>> is that PTM could not be supported by the host system (think PCI >>> controller). >> >> I just checked `pci_enable_ptm()` and on success it calls >> `pci_ptm_info()` logging a message: >> >> pci_info(dev, "PTM enabled%s, %s granularity\n", >> dev->ptm_root ? " (root)" : "", clock_desc); >> >> Was that present on your system with your patch? Please add that to the >> commit message. > > Yes, with my patches applied I can see this message on my systems. > > Sure, will add this to the commit message. > >> Regarding my comment, I did not mean returning an error but the log >> *level* of the message. So, `dmesg --level err` would show that message. >> But if there are PCI controllers not supporting that, it’s not an error, >> but a warning at most. So, I’d use: >> >> dev_warn(&pdev->dev, "PTM not supported by PCI bus/controller >> (pci_enable_ptm() failed)\n"); > > I will use you suggestion for the message, but I think that warn is a > bit too much, info or notice seem to be better. I do not know, if modern PCI(e)(?) controllers normally support PTM or not. If recent controllers should support it, then a warning would be warranted, otherwise a notice. Kind regards, Paul
Hi Paul, >> >>> Regarding my comment, I did not mean returning an error but the log >>> *level* of the message. So, `dmesg --level err` would show that message. >>> But if there are PCI controllers not supporting that, it’s not an error, >>> but a warning at most. So, I’d use: >>> >>> dev_warn(&pdev->dev, "PTM not supported by PCI bus/controller >>> (pci_enable_ptm() failed)\n"); >> >> I will use you suggestion for the message, but I think that warn is a >> bit too much, info or notice seem to be better. > > I do not know, if modern PCI(e)(?) controllers normally support PTM or > not. If recent controllers should support it, then a warning would be > warranted, otherwise a notice. > From the Intel side, it seems that it's been supported for a few years. So, fair enough, let's go with a warn. Cheers, -- Vinicius
On Wed, Jun 09, 2021 at 04:07:20PM -0700, Vinicius Costa Gomes wrote: > Hi Paul, > > >> > >>> Regarding my comment, I did not mean returning an error but the log > >>> *level* of the message. So, `dmesg --level err` would show that message. > >>> But if there are PCI controllers not supporting that, it’s not an error, > >>> but a warning at most. So, I’d use: > >>> > >>> dev_warn(&pdev->dev, "PTM not supported by PCI bus/controller > >>> (pci_enable_ptm() failed)\n"); > >> > >> I will use you suggestion for the message, but I think that warn is a > >> bit too much, info or notice seem to be better. > > > > I do not know, if modern PCI(e)(?) controllers normally support PTM or > > not. If recent controllers should support it, then a warning would be > > warranted, otherwise a notice. > > From the Intel side, it seems that it's been supported for a few years. > So, fair enough, let's go with a warn. I'm not sure about this. I think "warning" messages interrupt distro graphical boot scenarios and cause user complaints. In this case, there is nothing broken and the user can do nothing about it; it's merely a piece of missing optional functionality. So I think "info" is a more appropriate level. Bjorn
Bjorn Helgaas <helgaas@kernel.org> writes: > On Wed, Jun 09, 2021 at 04:07:20PM -0700, Vinicius Costa Gomes wrote: >> Hi Paul, >> >> >> >> >>> Regarding my comment, I did not mean returning an error but the log >> >>> *level* of the message. So, `dmesg --level err` would show that message. >> >>> But if there are PCI controllers not supporting that, it’s not an error, >> >>> but a warning at most. So, I’d use: >> >>> >> >>> dev_warn(&pdev->dev, "PTM not supported by PCI bus/controller >> >>> (pci_enable_ptm() failed)\n"); >> >> >> >> I will use you suggestion for the message, but I think that warn is a >> >> bit too much, info or notice seem to be better. >> > >> > I do not know, if modern PCI(e)(?) controllers normally support PTM or >> > not. If recent controllers should support it, then a warning would be >> > warranted, otherwise a notice. >> >> From the Intel side, it seems that it's been supported for a few years. >> So, fair enough, let's go with a warn. > > I'm not sure about this. I think "warning" messages interrupt distro > graphical boot scenarios and cause user complaints. In this case, > there is nothing broken and the user can do nothing about it; it's > merely a piece of missing optional functionality. So I think "info" > is a more appropriate level. Good point. "info" it is, then. Cheers, -- Vinicius
Hi, Changes from v4: - Improved commit messages (Bjorn Helgaas); Changes from v3: - More descriptive commit messages and comments (Bjorn Helgaas); - Added a pcie_ptm_enabled() helper (Bjorn Helgaas); Changes from v2: - Now the PTM timestamps are retrieved synchronously with the ioctl(); - Fixed some typos in constants; - The IGC_PTM_STAT register is write-1-to-clear, document this more clearly; Changes from v1: - This now should cross compile better, convert_art_ns_to_tsc() will only be used if CONFIG_X86_TSC is enabled; - PCIe PTM errors reported by the NIC are logged and PTM cycles are restarted in case an error is detected; Original cover letter (lightly edited): This adds support for PCIe PTM (Precision Time Measurement) to the igc driver. PCIe PTM allows the NIC and Host clocks to be compared more precisely, improving the clock synchronization accuracy. Patch 1/4 reverts a commit that made pci_enable_ptm() private to the PCI subsystem, reverting makes it possible for it to be called from the drivers. Patch 2/4 adds the pcie_ptm_enabled() helper. Patch 3/4 calls pci_enable_ptm() from the igc driver. Patch 4/4 implements the PCIe PTM support. It adds a workqueue that reads the PTM registers periodically and collects the information so a subsequent call to getcrosststamp() has all the timestamps needed. Some questions are raised (also pointed out in the commit message): 1. Using convert_art_ns_to_tsc() is too x86 specific, there should be a common way to create a 'system_counterval_t' from a timestamp. 2. convert_art_ns_to_tsc() says that it should only be used when X86_FEATURE_TSC_KNOWN_FREQ is true, but during tests it works even when it returns false. Should that check be done? Cheers, Vinicius Costa Gomes (4): Revert "PCI: Make pci_enable_ptm() private" PCI: Add pcie_ptm_enabled() igc: Enable PCIe PTM igc: Add support for PTP getcrosststamp() drivers/net/ethernet/intel/igc/igc.h | 1 + drivers/net/ethernet/intel/igc/igc_defines.h | 31 ++++ drivers/net/ethernet/intel/igc/igc_main.c | 6 + drivers/net/ethernet/intel/igc/igc_ptp.c | 182 +++++++++++++++++++ drivers/net/ethernet/intel/igc/igc_regs.h | 23 +++ drivers/pci/pci.h | 3 - drivers/pci/pcie/ptm.c | 9 + include/linux/pci.h | 10 + 8 files changed, 262 insertions(+), 3 deletions(-)