diff mbox series

[next-queue,v5,3/4] igc: Enable PCIe PTM

Message ID 20210605002356.3996853-4-vinicius.gomes@intel.com (mailing list archive)
State Superseded
Delegated to: Bjorn Helgaas
Headers show
Series igc: Add support for PCIe PTM | expand

Commit Message

Vinicius Costa Gomes June 5, 2021, 12:23 a.m. UTC
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(+)

Comments

Paul Menzel June 5, 2021, 6:42 a.m. UTC | #1
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
Vinicius Costa Gomes June 8, 2021, 7:02 p.m. UTC | #2
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,
Paul Menzel June 9, 2021, 6:24 a.m. UTC | #3
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
Vinicius Costa Gomes June 9, 2021, 8:08 p.m. UTC | #4
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,
Paul Menzel June 9, 2021, 9:26 p.m. UTC | #5
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
Vinicius Costa Gomes June 9, 2021, 11:07 p.m. UTC | #6
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,
Bjorn Helgaas June 9, 2021, 11:20 p.m. UTC | #7
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
Vinicius Costa Gomes June 10, 2021, 12:04 a.m. UTC | #8
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 mbox series

Patch

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;