[PATCH iwl-net v1] igb: Fix not clearing TimeSync interrupts for 82580

[PATCH iwl-net v1] igb: Fix not clearing TimeSync interrupts for 82580

[Vinicius Costa Gomes]

It was reported that 82580 NICs have a hardware bug that makes it
necessary to write into the TSICR (TimeSync Interrupt Cause) register
to clear it.

Add a conditional so only for 82580 we write into the TSICR register,
so we don't risk losing events for other models.

This (partially) reverts commit ee14cc9ea19b ("igb: Fix missing time sync events").

Fixes: ee14cc9ea19b ("igb: Fix missing time sync events")
Reported-by: Daiwei Li <daiweili@gmail.com>
Closes: https://lore.kernel.org/intel-wired-lan/CAN0jFd1kO0MMtOh8N2Ztxn6f7vvDKp2h507sMryobkBKe=xk=w@mail.gmail.com/
Signed-off-by: Vinicius Costa Gomes <vinicius.gomes@intel.com>
---

@Daiwei Li, I don't have a 82580 handy, please confirm that the patch
fixes the issue you are having.

 drivers/net/ethernet/intel/igb/igb_main.c | 27 ++++++++++++++++++-----
 1 file changed, 22 insertions(+), 5 deletions(-)

diff --git a/drivers/net/ethernet/intel/igb/igb_main.c b/drivers/net/ethernet/intel/igb/igb_main.c
index 11be39f435f3..edb34f67ae03 100644
--- a/drivers/net/ethernet/intel/igb/igb_main.c
+++ b/drivers/net/ethernet/intel/igb/igb_main.c
@@ -6960,31 +6960,48 @@ static void igb_extts(struct igb_adapter *adapter, int tsintr_tt)
 static void igb_tsync_interrupt(struct igb_adapter *adapter)
 {
 	struct e1000_hw *hw = &adapter->hw;
-	u32 tsicr = rd32(E1000_TSICR);
+	u32 ack = 0, tsicr = rd32(E1000_TSICR);
 	struct ptp_clock_event event;
 
 	if (tsicr & TSINTR_SYS_WRAP) {
 		event.type = PTP_CLOCK_PPS;
 		if (adapter->ptp_caps.pps)
 			ptp_clock_event(adapter->ptp_clock, &event);
+		ack |= TSINTR_SYS_WRAP;
 	}
 
 	if (tsicr & E1000_TSICR_TXTS) {
 		/* retrieve hardware timestamp */
 		schedule_work(&adapter->ptp_tx_work);
+		ack |= E1000_TSICR_TXTS;
 	}
 
-	if (tsicr & TSINTR_TT0)
+	if (tsicr & TSINTR_TT0) {
 		igb_perout(adapter, 0);
+		ack |= TSINTR_TT0;
+	}
 
-	if (tsicr & TSINTR_TT1)
+	if (tsicr & TSINTR_TT1) {
 		igb_perout(adapter, 1);
+		ack |= TSINTR_TT1;
+	}
 
-	if (tsicr & TSINTR_AUTT0)
+	if (tsicr & TSINTR_AUTT0) {
 		igb_extts(adapter, 0);
+		ack |= TSINTR_AUTT0;
+	}
 
-	if (tsicr & TSINTR_AUTT1)
+	if (tsicr & TSINTR_AUTT1) {
 		igb_extts(adapter, 1);
+		ack |= TSINTR_AUTT1;
+	}
+
+	if (hw->mac.type == e1000_82580) {
+		/* 82580 has a hardware bug that requires a explicit
+		 * write to clear the TimeSync interrupt cause.
+		 */
+		wr32(E1000_TSICR, ack);
+	}
 }
 
 static irqreturn_t igb_msix_other(int irq, void *data)
-- 
2.45.2

Re: [Intel-wired-lan] [PATCH iwl-net v1] igb: Fix not clearing TimeSync interrupts for 82580

[Paul Menzel]

Dear Vinicius,


Thank you for the patch.


Am 10.08.24 um 02:23 schrieb Vinicius Costa Gomes:
> It was reported that 82580 NICs have a hardware bug that makes it
> necessary to write into the TSICR (TimeSync Interrupt Cause) register
> to clear it.

Were you able to verify that report by checking against some errata? Is 
Intel aware of the problem?

> Add a conditional so only for 82580 we write into the TSICR register,
> so we don't risk losing events for other models.
> 
> This (partially) reverts commit ee14cc9ea19b ("igb: Fix missing time sync events").
> 
> Fixes: ee14cc9ea19b ("igb: Fix missing time sync events")
> Reported-by: Daiwei Li <daiweili@gmail.com>
> Closes: https://lore.kernel.org/intel-wired-lan/CAN0jFd1kO0MMtOh8N2Ztxn6f7vvDKp2h507sMryobkBKe=xk=w@mail.gmail.com/
> Signed-off-by: Vinicius Costa Gomes <vinicius.gomes@intel.com>
> ---
> 
> @Daiwei Li, I don't have a 82580 handy, please confirm that the patch
> fixes the issue you are having.

Please also add a description of the test case, and maybe the PCI vendor 
and device code of your network device.

>   drivers/net/ethernet/intel/igb/igb_main.c | 27 ++++++++++++++++++-----
>   1 file changed, 22 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/net/ethernet/intel/igb/igb_main.c b/drivers/net/ethernet/intel/igb/igb_main.c
> index 11be39f435f3..edb34f67ae03 100644
> --- a/drivers/net/ethernet/intel/igb/igb_main.c
> +++ b/drivers/net/ethernet/intel/igb/igb_main.c
> @@ -6960,31 +6960,48 @@ static void igb_extts(struct igb_adapter *adapter, int tsintr_tt)
>   static void igb_tsync_interrupt(struct igb_adapter *adapter)
>   {
>   	struct e1000_hw *hw = &adapter->hw;
> -	u32 tsicr = rd32(E1000_TSICR);
> +	u32 ack = 0, tsicr = rd32(E1000_TSICR);
>   	struct ptp_clock_event event;
>   
>   	if (tsicr & TSINTR_SYS_WRAP) {
>   		event.type = PTP_CLOCK_PPS;
>   		if (adapter->ptp_caps.pps)
>   			ptp_clock_event(adapter->ptp_clock, &event);
> +		ack |= TSINTR_SYS_WRAP;
>   	}
>   
>   	if (tsicr & E1000_TSICR_TXTS) {
>   		/* retrieve hardware timestamp */
>   		schedule_work(&adapter->ptp_tx_work);
> +		ack |= E1000_TSICR_TXTS;
>   	}
>   
> -	if (tsicr & TSINTR_TT0)
> +	if (tsicr & TSINTR_TT0) {
>   		igb_perout(adapter, 0);
> +		ack |= TSINTR_TT0;
> +	}
>   
> -	if (tsicr & TSINTR_TT1)
> +	if (tsicr & TSINTR_TT1) {
>   		igb_perout(adapter, 1);
> +		ack |= TSINTR_TT1;
> +	}
>   
> -	if (tsicr & TSINTR_AUTT0)
> +	if (tsicr & TSINTR_AUTT0) {
>   		igb_extts(adapter, 0);
> +		ack |= TSINTR_AUTT0;
> +	}
>   
> -	if (tsicr & TSINTR_AUTT1)
> +	if (tsicr & TSINTR_AUTT1) {
>   		igb_extts(adapter, 1);
> +		ack |= TSINTR_AUTT1;
> +	}
> +
> +	if (hw->mac.type == e1000_82580) {
> +		/* 82580 has a hardware bug that requires a explicit

a*n*

> +		 * write to clear the TimeSync interrupt cause.
> +		 */
> +		wr32(E1000_TSICR, ack);
> +	}

Is there a nicer way to write this, so `ack` is only assigned in case 
for the 82580?

>   }
>   
>   static irqreturn_t igb_msix_other(int irq, void *data)


Kind regards,

Paul

Re: [PATCH iwl-net v1] igb: Fix not clearing TimeSync interrupts for 82580

[Richard Cochran]

On Fri, Aug 09, 2024 at 05:23:02PM -0700, Vinicius Costa Gomes wrote:
> It was reported that 82580 NICs have a hardware bug that makes it
> necessary to write into the TSICR (TimeSync Interrupt Cause) register
> to clear it.

Bug, or was it a feature?

Or IOW, maybe i210 changed the semantics of the TSICR?

And what about the 82576?

Thanks,
Richard

Re: [PATCH iwl-net v1] igb: Fix not clearing TimeSync interrupts for 82580

[Daiwei Li]

> @Daiwei Li, I don't have a 82580 handy, please confirm that the patch
fixes the issue you are having.

Thank you for the patch! I can confirm it fixes my issue. Below I offer a
patch that also works in response to Paul's feedback.

> Please also add a description of the test case

I am running ptp4l to serve PTP to a client device attached to the NIC.
To test, I am rebuilding igb.ko and reloading it.
Without this patch, I see repeatedly in the output of ptp4l:

> timed out while polling for tx timestamp increasing tx_timestamp_timeout or
> increasing kworker priority may correct this issue, but a driver bug likely
> causes it

as well as my client device failing to sync time.

> and maybe the PCI vendor and device code of your network device.

% lspci -nn | grep Network
17:00.0 Ethernet controller [0200]: Intel Corporation 82580 Gigabit
Network Connection [8086:150e] (rev 01)
17:00.1 Ethernet controller [0200]: Intel Corporation 82580 Gigabit
Network Connection [8086:150e] (rev 01)
17:00.2 Ethernet controller [0200]: Intel Corporation 82580 Gigabit
Network Connection [8086:150e] (rev 01)
17:00.3 Ethernet controller [0200]: Intel Corporation 82580 Gigabit
Network Connection [8086:150e] (rev 01)

> Bug, or was it a feature?

According to https://lore.kernel.org/all/CDCB8BE0.1EC2C%25matthew.vick@intel.com/
it was a bug. It looks like the datasheet was not updated to
acknowledge this bug:
https://www.intel.com/content/www/us/en/content-details/333167/intel-82580-eb-82580-db-gbe-controller-datasheet.html
(section 8.17.28.1).

> Is there a nicer way to write this, so `ack` is only assigned in case
> for the 82580?

diff --git a/drivers/net/ethernet/intel/igb/igb_main.c
b/drivers/net/ethernet/intel/igb/igb_main.c
index ada42ba63549..87ec1258e22a 100644
--- a/drivers/net/ethernet/intel/igb/igb_main.c
+++ b/drivers/net/ethernet/intel/igb/igb_main.c
@@ -6986,6 +6986,10 @@ static void igb_tsync_interrupt(struct
igb_adapter *adapter)
        struct e1000_hw *hw = &adapter->hw;
        u32 tsicr = rd32(E1000_TSICR);
        struct ptp_clock_event event;
+       const u32 mask = (TSINTR_SYS_WRAP | E1000_TSICR_TXTS |
+                          TSINTR_TT0 | TSINTR_TT1 |
+                          TSINTR_AUTT0 | TSINTR_AUTT1);
+

        if (tsicr & TSINTR_SYS_WRAP) {
                event.type = PTP_CLOCK_PPS;
@@ -7009,6 +7013,13 @@ static void igb_tsync_interrupt(struct
igb_adapter *adapter)

        if (tsicr & TSINTR_AUTT1)
                igb_extts(adapter, 1);
+
+       if (hw->mac.type == e1000_82580) {
+               /* 82580 has a hardware bug that requires a explicit
+                * write to clear the TimeSync interrupt cause.
+                */
+               wr32(E1000_TSICR, tsicr & mask);
+       }
 }
On Fri, Aug 9, 2024 at 10:04 PM Richard Cochran
<richardcochran@gmail.com> wrote:
>
> On Fri, Aug 09, 2024 at 05:23:02PM -0700, Vinicius Costa Gomes wrote:
> > It was reported that 82580 NICs have a hardware bug that makes it
> > necessary to write into the TSICR (TimeSync Interrupt Cause) register
> > to clear it.
>
> Bug, or was it a feature?
>
> Or IOW, maybe i210 changed the semantics of the TSICR?
>
> And what about the 82576?
>
> Thanks,
> Richard

Re: [PATCH iwl-net v1] igb: Fix not clearing TimeSync interrupts for 82580

[Vinicius Costa Gomes]

Hi,

Daiwei Li <daiweili@gmail.com> writes:

>> @Daiwei Li, I don't have a 82580 handy, please confirm that the patch
> fixes the issue you are having.
>
> Thank you for the patch! I can confirm it fixes my issue. Below I offer a
> patch that also works in response to Paul's feedback.
>

Your patch looks better than mine. I would suggest for you to go ahead
and propose yours for inclusion.

>> Please also add a description of the test case
>
> I am running ptp4l to serve PTP to a client device attached to the NIC.
> To test, I am rebuilding igb.ko and reloading it.
> Without this patch, I see repeatedly in the output of ptp4l:
>
>> timed out while polling for tx timestamp increasing tx_timestamp_timeout or
>> increasing kworker priority may correct this issue, but a driver bug likely
>> causes it
>
> as well as my client device failing to sync time.
>
>> and maybe the PCI vendor and device code of your network device.
>
> % lspci -nn | grep Network
> 17:00.0 Ethernet controller [0200]: Intel Corporation 82580 Gigabit
> Network Connection [8086:150e] (rev 01)
> 17:00.1 Ethernet controller [0200]: Intel Corporation 82580 Gigabit
> Network Connection [8086:150e] (rev 01)
> 17:00.2 Ethernet controller [0200]: Intel Corporation 82580 Gigabit
> Network Connection [8086:150e] (rev 01)
> 17:00.3 Ethernet controller [0200]: Intel Corporation 82580 Gigabit
> Network Connection [8086:150e] (rev 01)
>
>> Bug, or was it a feature?
>
> According to https://lore.kernel.org/all/CDCB8BE0.1EC2C%25matthew.vick@intel.com/
> it was a bug. It looks like the datasheet was not updated to
> acknowledge this bug:
> https://www.intel.com/content/www/us/en/content-details/333167/intel-82580-eb-82580-db-gbe-controller-datasheet.html
> (section 8.17.28.1).
>
>> Is there a nicer way to write this, so `ack` is only assigned in case
>> for the 82580?
>
> diff --git a/drivers/net/ethernet/intel/igb/igb_main.c
> b/drivers/net/ethernet/intel/igb/igb_main.c
> index ada42ba63549..87ec1258e22a 100644
> --- a/drivers/net/ethernet/intel/igb/igb_main.c
> +++ b/drivers/net/ethernet/intel/igb/igb_main.c
> @@ -6986,6 +6986,10 @@ static void igb_tsync_interrupt(struct
> igb_adapter *adapter)
>         struct e1000_hw *hw = &adapter->hw;
>         u32 tsicr = rd32(E1000_TSICR);
>         struct ptp_clock_event event;
> +       const u32 mask = (TSINTR_SYS_WRAP | E1000_TSICR_TXTS |
> +                          TSINTR_TT0 | TSINTR_TT1 |
> +                          TSINTR_AUTT0 | TSINTR_AUTT1);
> +
>
>         if (tsicr & TSINTR_SYS_WRAP) {
>                 event.type = PTP_CLOCK_PPS;
> @@ -7009,6 +7013,13 @@ static void igb_tsync_interrupt(struct
> igb_adapter *adapter)
>
>         if (tsicr & TSINTR_AUTT1)
>                 igb_extts(adapter, 1);
> +
> +       if (hw->mac.type == e1000_82580) {
> +               /* 82580 has a hardware bug that requires a explicit
> +                * write to clear the TimeSync interrupt cause.
> +                */
> +               wr32(E1000_TSICR, tsicr & mask);

Yeah, I should have thought about that, that writing '1' into an
interrupr that is cleared should be fine.

> +       }
>  }
> On Fri, Aug 9, 2024 at 10:04 PM Richard Cochran
> <richardcochran@gmail.com> wrote:
>>
>> On Fri, Aug 09, 2024 at 05:23:02PM -0700, Vinicius Costa Gomes wrote:
>> > It was reported that 82580 NICs have a hardware bug that makes it
>> > necessary to write into the TSICR (TimeSync Interrupt Cause) register
>> > to clear it.
>>
>> Bug, or was it a feature?
>>
>> Or IOW, maybe i210 changed the semantics of the TSICR?
>>
>> And what about the 82576?
>>
>> Thanks,
>> Richard

-- 
Vinicius

[PATCH iwl-net v2] igb: Fix not clearing TimeSync interrupts for 82580

[Daiwei Li]

82580 NICs have a hardware bug that makes it
necessary to write into the TSICR (TimeSync Interrupt Cause) register
to clear it:
https://lore.kernel.org/all/CDCB8BE0.1EC2C%25matthew.vick@intel.com/

Add a conditional so only for 82580 we write into the TSICR register,
so we don't risk losing events for other models.

This (partially) reverts commit ee14cc9ea19b ("igb: Fix missing time sync events").

Fixes: ee14cc9ea19b ("igb: Fix missing time sync events")
Closes: https://lore.kernel.org/intel-wired-lan/CAN0jFd1kO0MMtOh8N2Ztxn6f7vvDKp2h507sMryobkBKe=xk=w@mail.gmail.com/
Tested-by: Daiwei Li <daiweili@google.com>
Signed-off-by: Daiwei Li <daiweili@google.com>
---

@Vinicius Gomes, this is my first time submitting a Linux kernel patch,
so apologies if I missed any part of the procedure (e.g. this is
currently on top of 6.7.12, the kernel I am running; should I be
rebasing on inline?). Also, is there any way to annotate the patch
to give you credit for the original change?

 drivers/net/ethernet/intel/igb/igb_main.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/drivers/net/ethernet/intel/igb/igb_main.c b/drivers/net/ethernet/intel/igb/igb_main.c
index ada42ba63549..1210ddc5d81e 100644
--- a/drivers/net/ethernet/intel/igb/igb_main.c
+++ b/drivers/net/ethernet/intel/igb/igb_main.c
@@ -6986,6 +6986,16 @@ static void igb_tsync_interrupt(struct igb_adapter *adapter)
 	struct e1000_hw *hw = &adapter->hw;
 	u32 tsicr = rd32(E1000_TSICR);
 	struct ptp_clock_event event;
+	const u32 mask = (TSINTR_SYS_WRAP | E1000_TSICR_TXTS |
+			  TSINTR_TT0 | TSINTR_TT1 |
+			  TSINTR_AUTT0 | TSINTR_AUTT1);
+
+	if (hw->mac.type == e1000_82580) {
+		/* 82580 has a hardware bug that requires a explicit
+		 * write to clear the TimeSync interrupt cause.
+		 */
+		wr32(E1000_TSICR, tsicr & mask);
+	}
 
 	if (tsicr & TSINTR_SYS_WRAP) {
 		event.type = PTP_CLOCK_PPS;
-- 
2.46.0.76.ge559c4bf1a-goog

Re: [PATCH iwl-net v2] igb: Fix not clearing TimeSync interrupts for 82580

[Vinicius Costa Gomes]

Daiwei Li <daiweili@google.com> writes:

> 82580 NICs have a hardware bug that makes it
> necessary to write into the TSICR (TimeSync Interrupt Cause) register
> to clear it:
> https://lore.kernel.org/all/CDCB8BE0.1EC2C%25matthew.vick@intel.com/
>
> Add a conditional so only for 82580 we write into the TSICR register,
> so we don't risk losing events for other models.

Please add some information in the commit message about how to reproduce
the issue, as Paul suggested.

>
> This (partially) reverts commit ee14cc9ea19b ("igb: Fix missing time sync events").
>
> Fixes: ee14cc9ea19b ("igb: Fix missing time sync events")
> Closes: https://lore.kernel.org/intel-wired-lan/CAN0jFd1kO0MMtOh8N2Ztxn6f7vvDKp2h507sMryobkBKe=xk=w@mail.gmail.com/
> Tested-by: Daiwei Li <daiweili@google.com>
> Signed-off-by: Daiwei Li <daiweili@google.com>
> ---
>
> @Vinicius Gomes, this is my first time submitting a Linux kernel patch,
> so apologies if I missed any part of the procedure (e.g. this is
> currently on top of 6.7.12, the kernel I am running; should I be
> rebasing on inline?). Also, is there any way to annotate the patch
> to give you credit for the original change?

Your submission format looks fine. Just a couple details:
 - No need for setting in-reply-to (or something like it);
 
 - For this particular patch, you got lucky and it applies cleanly
 against current tip, but for future submissions, for intel-wired-lan
 and patches intended for the stable tree, please rebase against:

 https://git.kernel.org/pub/scm/linux/kernel/git/tnguy/net-queue.git/

For credits, you can add something like:

Suggested-by: Vinicius Costa Gomes <vinicius.gomes@intel.com>

>
>  drivers/net/ethernet/intel/igb/igb_main.c | 10 ++++++++++
>  1 file changed, 10 insertions(+)
>
> diff --git a/drivers/net/ethernet/intel/igb/igb_main.c b/drivers/net/ethernet/intel/igb/igb_main.c
> index ada42ba63549..1210ddc5d81e 100644
> --- a/drivers/net/ethernet/intel/igb/igb_main.c
> +++ b/drivers/net/ethernet/intel/igb/igb_main.c
> @@ -6986,6 +6986,16 @@ static void igb_tsync_interrupt(struct igb_adapter *adapter)
>  	struct e1000_hw *hw = &adapter->hw;
>  	u32 tsicr = rd32(E1000_TSICR);
>  	struct ptp_clock_event event;
> +	const u32 mask = (TSINTR_SYS_WRAP | E1000_TSICR_TXTS |
> +			  TSINTR_TT0 | TSINTR_TT1 |
> +			  TSINTR_AUTT0 | TSINTR_AUTT1);
> +

Please move the declaration of 'mask' up, to follow the convention, the
"reverse christmas tree" rule. Or separate the attribution from the
declaration.

> +	if (hw->mac.type == e1000_82580) {
> +		/* 82580 has a hardware bug that requires a explicit

And as pointed by Paul, "*an* explicit".

> +		 * write to clear the TimeSync interrupt cause.
> +		 */
> +		wr32(E1000_TSICR, tsicr & mask);
> +	}
>  
>  	if (tsicr & TSINTR_SYS_WRAP) {
>  		event.type = PTP_CLOCK_PPS;
> -- 
> 2.46.0.76.ge559c4bf1a-goog
>

-- 
Vinicius

Re: [PATCH iwl-net v2] igb: Fix not clearing TimeSync interrupts for 82580

[Daiwei Li]

Thank you for the review! I've sent out another patch that hopefully
addresses the comments.


On Tue, Aug 13, 2024 at 3:26 PM Vinicius Costa Gomes
<vinicius.gomes@intel.com> wrote:
>
> Daiwei Li <daiweili@google.com> writes:
>
> > 82580 NICs have a hardware bug that makes it
> > necessary to write into the TSICR (TimeSync Interrupt Cause) register
> > to clear it:
> > https://lore.kernel.org/all/CDCB8BE0.1EC2C%25matthew.vick@intel.com/
> >
> > Add a conditional so only for 82580 we write into the TSICR register,
> > so we don't risk losing events for other models.
>
> Please add some information in the commit message about how to reproduce
> the issue, as Paul suggested.
>
> >
> > This (partially) reverts commit ee14cc9ea19b ("igb: Fix missing time sync events").
> >
> > Fixes: ee14cc9ea19b ("igb: Fix missing time sync events")
> > Closes: https://lore.kernel.org/intel-wired-lan/CAN0jFd1kO0MMtOh8N2Ztxn6f7vvDKp2h507sMryobkBKe=xk=w@mail.gmail.com/
> > Tested-by: Daiwei Li <daiweili@google.com>
> > Signed-off-by: Daiwei Li <daiweili@google.com>
> > ---
> >
> > @Vinicius Gomes, this is my first time submitting a Linux kernel patch,
> > so apologies if I missed any part of the procedure (e.g. this is
> > currently on top of 6.7.12, the kernel I am running; should I be
> > rebasing on inline?). Also, is there any way to annotate the patch
> > to give you credit for the original change?
>
> Your submission format looks fine. Just a couple details:
>  - No need for setting in-reply-to (or something like it);
>
>  - For this particular patch, you got lucky and it applies cleanly
>  against current tip, but for future submissions, for intel-wired-lan
>  and patches intended for the stable tree, please rebase against:
>
>  https://git.kernel.org/pub/scm/linux/kernel/git/tnguy/net-queue.git/
>
> For credits, you can add something like:
>
> Suggested-by: Vinicius Costa Gomes <vinicius.gomes@intel.com>
>
> >
> >  drivers/net/ethernet/intel/igb/igb_main.c | 10 ++++++++++
> >  1 file changed, 10 insertions(+)
> >
> > diff --git a/drivers/net/ethernet/intel/igb/igb_main.c b/drivers/net/ethernet/intel/igb/igb_main.c
> > index ada42ba63549..1210ddc5d81e 100644
> > --- a/drivers/net/ethernet/intel/igb/igb_main.c
> > +++ b/drivers/net/ethernet/intel/igb/igb_main.c
> > @@ -6986,6 +6986,16 @@ static void igb_tsync_interrupt(struct igb_adapter *adapter)
> >       struct e1000_hw *hw = &adapter->hw;
> >       u32 tsicr = rd32(E1000_TSICR);
> >       struct ptp_clock_event event;
> > +     const u32 mask = (TSINTR_SYS_WRAP | E1000_TSICR_TXTS |
> > +                       TSINTR_TT0 | TSINTR_TT1 |
> > +                       TSINTR_AUTT0 | TSINTR_AUTT1);
> > +
>
> Please move the declaration of 'mask' up, to follow the convention, the
> "reverse christmas tree" rule. Or separate the attribution from the
> declaration.
>
> > +     if (hw->mac.type == e1000_82580) {
> > +             /* 82580 has a hardware bug that requires a explicit
>
> And as pointed by Paul, "*an* explicit".
>
> > +              * write to clear the TimeSync interrupt cause.
> > +              */
> > +             wr32(E1000_TSICR, tsicr & mask);
> > +     }
> >
> >       if (tsicr & TSINTR_SYS_WRAP) {
> >               event.type = PTP_CLOCK_PPS;
> > --
> > 2.46.0.76.ge559c4bf1a-goog
> >
>
> --
> Vinicius



-- 
Daiwei Li
Software Engineer
Mobile: 415-736-8670
waymo.com

Re: [PATCH iwl-net v1] igb: Fix not clearing TimeSync interrupts for 82580

[Ratheesh Kannoth]

On 2024-08-10 at 05:53:02, Vinicius Costa Gomes (vinicius.gomes@intel.com) wrote:
> @@ -6960,31 +6960,48 @@ static void igb_extts(struct igb_adapter *adapter, int tsintr_tt)
>  static void igb_tsync_interrupt(struct igb_adapter *adapter)
>  {
>  	struct e1000_hw *hw = &adapter->hw;
> -	u32 tsicr = rd32(E1000_TSICR);
> +	u32 ack = 0, tsicr = rd32(E1000_TSICR);
nit: reverse xmas tree.

Re: [PATCH iwl-net v1] igb: Fix not clearing TimeSync interrupts for 82580

[Jakub Kicinski]

On Tue, 13 Aug 2024 09:42:53 +0530 Ratheesh Kannoth wrote:
> On 2024-08-10 at 05:53:02, Vinicius Costa Gomes (vinicius.gomes@intel.com) wrote:
> > @@ -6960,31 +6960,48 @@ static void igb_extts(struct igb_adapter *adapter, int tsintr_tt)
> >  static void igb_tsync_interrupt(struct igb_adapter *adapter)
> >  {
> >  	struct e1000_hw *hw = &adapter->hw;
> > -	u32 tsicr = rd32(E1000_TSICR);
> > +	u32 ack = 0, tsicr = rd32(E1000_TSICR);  
> nit: reverse xmas tree.

Please read the last paragraph below and disseminate this information
among your colleagues at Marvell.

  Reviewer guidance
  -----------------
  
  Reviewing other people's patches on the list is highly encouraged,
  regardless of the level of expertise. For general guidance and
  helpful tips please see :ref:`development_advancedtopics_reviews`.
  
  It's safe to assume that netdev maintainers know the community and
  the level of expertise of the reviewers. The reviewers should not be
  concerned about their comments impeding or derailing the patch flow.
  
  Less experienced reviewers are highly encouraged to do more in-depth
  review of submissions and not focus exclusively on trivial or
  subjective matters like code formatting, tags etc.
  
See: https://www.kernel.org/doc/html/next/process/maintainer-netdev.html#reviewer-guidance