| Message ID | 20210605002356.3996853-4-vinicius.gomes@intel.com |
|---|---|
| State | Changes Requested |
| 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,
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,
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,
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,
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"); + pci_set_master(pdev); err = -ENOMEM;
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(+)