Re: [Intel-wired-lan] [PATCH v2] e1000: The 'const' qualifier has been added where applicable to enhance code safety and prevent unintended modifications.

2025-03-04 Thread Przemek Kitszel 

On 3/3/25 21:47, joaomboni wrote:

Signed-off-by: Joao Bonifacio 


it will be good to use imperative mood in the Subject,
and add one more paragraph, like:

Subject: e1000: mark global variables const where possible

Next paragraph:
Mark global variables const, so unintended modification would not be
possible.


---
  drivers/net/ethernet/intel/e1000/e1000_main.c | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/intel/e1000/e1000_main.c 
b/drivers/net/ethernet/intel/e1000/e1000_main.c
index 3f089c3d47b2..96bc85f09aaf 100644
--- a/drivers/net/ethernet/intel/e1000/e1000_main.c
+++ b/drivers/net/ethernet/intel/e1000/e1000_main.c
@@ -9,7 +9,7 @@
  #include 
  
  char e1000_driver_name[] = "e1000";


your commit message suggests that you add const "everywhere", but it
seems that there are other candidates, like the one above

PS. You have to wait 24h before posting next revision.


-static char e1000_driver_string[] = "Intel(R) PRO/1000 Network Driver";
+static const char e1000_driver_string[] = "Intel(R) PRO/1000 Network Driver";
  static const char e1000_copyright[] = "Copyright (c) 1999-2006 Intel 
Corporation.";
  
  /* e1000_pci_tbl - PCI Device ID Table

Re: [Intel-wired-lan] [PATCH iwl-next v5 09/15] ixgbe: add E610 functions getting PBA and FW ver info

2025-03-04 Thread R, Bharath 
> -Original Message-
> From: Intel-wired-lan  On Behalf Of
> Jedrzej Jagielski
> Sent: Friday, February 21, 2025 5:21 PM
> To: intel-wired-...@lists.osuosl.org
> Cc: Nguyen, Anthony L ;
> net...@vger.kernel.org; ho...@kernel.org; j...@nvidia.com; Jagielski, Jedrzej
> ; Polchlopek, Mateusz
> ; Wegrzyn, Stefan
> 
> Subject: [Intel-wired-lan] [PATCH iwl-next v5 09/15] ixgbe: add E610
> functions getting PBA and FW ver info
> 
> Introduce 2 E610 specific callbacks implementations:
> -ixgbe_start_hw_e610() which expands the regular .start_hw callback with
> getting FW version information
> -ixgbe_read_pba_string_e610() which gets Product Board Assembly string
> 
> Extend EEPROM ops with new .read_pba_string in order to distinguish generic
> one and the E610 one.
> 
> Reviewed-by: Mateusz Polchlopek 
> Co-developed-by: Stefan Wegrzyn 
> Signed-off-by: Stefan Wegrzyn 
> Signed-off-by: Jedrzej Jagielski 
> ---
>  .../ethernet/intel/ixgbe/devlink/devlink.c|   5 +-
>  .../net/ethernet/intel/ixgbe/ixgbe_82598.c|   1 +
>  .../net/ethernet/intel/ixgbe/ixgbe_82599.c|   1 +
>  .../net/ethernet/intel/ixgbe/ixgbe_common.c   |   1 +
>  drivers/net/ethernet/intel/ixgbe/ixgbe_e610.c | 181 +-
>  drivers/net/ethernet/intel/ixgbe/ixgbe_main.c |   2 +-
>  drivers/net/ethernet/intel/ixgbe/ixgbe_type.h |   2 +
>  .../ethernet/intel/ixgbe/ixgbe_type_e610.h|   1 +
>  drivers/net/ethernet/intel/ixgbe/ixgbe_x540.c |   1 +
>  drivers/net/ethernet/intel/ixgbe/ixgbe_x550.c |   1 +
>  10 files changed, 190 insertions(+), 6 deletions(-)
> 

Tested-by: Bharath R

Re: [Intel-wired-lan] [iwl-net v3 1/5] virtchnl: make proto and filter action count unsigned

2025-03-04 Thread Szapar-Mudlaw, Martyna 




On 3/4/2025 12:15 PM, Paul Menzel wrote:

Dear Jan, dear Martina,


Thank you for the patch.

Am 04.03.25 um 12:08 schrieb Martyna Szapar-Mudlaw:

From: Jan Glaza 

The count field in virtchnl_proto_hdrs and virtchnl_filter_action_set
should never be negative while still being valid. Changing it from
int to u32 ensures proper handling of values in virtchnl messages in
driverrs and prevents unintended behavior.
In its current signed form, a negative count does not trigger
an error in ice driver but instead results in it being treated as 0.
This can lead to unexpected outcomes when processing messages.
By using u32, any invalid values will correctly trigger -EINVAL,
making error detection more robust.

Fixes: 1f7ea1cd6a374 ("ice: Enable FDIR Configure for AVF")
Reviewed-by: Jedrzej Jagielski 
Reviewed-by: Simon Horman 
Signed-off-by: Jan Glaza 
Signed-off-by: Martyna Szapar-Mudlaw mud...@linux.intel.com>

---
  include/linux/avf/virtchnl.h | 4 ++--
  1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/include/linux/avf/virtchnl.h b/include/linux/avf/virtchnl.h
index 4811b9a14604..cf0afa60e4a7 100644
--- a/include/linux/avf/virtchnl.h
+++ b/include/linux/avf/virtchnl.h
@@ -1343,7 +1343,7 @@ struct virtchnl_proto_hdrs {
   * 2 - from the second inner layer
   * 
   **/
-    int count; /* the proto layers must < VIRTCHNL_MAX_NUM_PROTO_HDRS */
+    u32 count; /* the proto layers must < VIRTCHNL_MAX_NUM_PROTO_HDRS */


Why limit the length, and not use unsigned int?



u32 range is completely sufficient for number of proto hdrs (as said: 
"the proto layers must < VIRTCHNL_MAX_NUM_PROTO_HDRS") and I believe it 
is recommended to use fixed sized variables where possible



  union {
  struct virtchnl_proto_hdr
  proto_hdr[VIRTCHNL_MAX_NUM_PROTO_HDRS];
@@ -1395,7 +1395,7 @@ VIRTCHNL_CHECK_STRUCT_LEN(36, 
virtchnl_filter_action);

  struct virtchnl_filter_action_set {
  /* action number must be less then VIRTCHNL_MAX_NUM_ACTIONS */
-    int count;
+    u32 count;
  struct virtchnl_filter_action actions[VIRTCHNL_MAX_NUM_ACTIONS];
  };



Kind regards,

Paul

Re: [Intel-wired-lan] [iwl-net v3 1/5] virtchnl: make proto and filter action count unsigned

2025-03-04 Thread Paul Menzel 

Dear Martyna,


Thank you for your quick reply.

Am 04.03.25 um 12:45 schrieb Szapar-Mudlaw, Martyna:


On 3/4/2025 12:15 PM, Paul Menzel wrote:



Am 04.03.25 um 12:08 schrieb Martyna Szapar-Mudlaw:

From: Jan Glaza 

The count field in virtchnl_proto_hdrs and virtchnl_filter_action_set
should never be negative while still being valid. Changing it from
int to u32 ensures proper handling of values in virtchnl messages in
driverrs and prevents unintended behavior.
In its current signed form, a negative count does not trigger
an error in ice driver but instead results in it being treated as 0.
This can lead to unexpected outcomes when processing messages.
By using u32, any invalid values will correctly trigger -EINVAL,
making error detection more robust.

Fixes: 1f7ea1cd6a374 ("ice: Enable FDIR Configure for AVF")
Reviewed-by: Jedrzej Jagielski 
Reviewed-by: Simon Horman 
Signed-off-by: Jan Glaza 
Signed-off-by: Martyna Szapar-Mudlaw 
---
  include/linux/avf/virtchnl.h | 4 ++--
  1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/include/linux/avf/virtchnl.h b/include/linux/avf/virtchnl.h
index 4811b9a14604..cf0afa60e4a7 100644
--- a/include/linux/avf/virtchnl.h
+++ b/include/linux/avf/virtchnl.h
@@ -1343,7 +1343,7 @@ struct virtchnl_proto_hdrs {
   * 2 - from the second inner layer
   * 
   **/
-    int count; /* the proto layers must < VIRTCHNL_MAX_NUM_PROTO_HDRS */
+    u32 count; /* the proto layers must < VIRTCHNL_MAX_NUM_PROTO_HDRS */


Why limit the length, and not use unsigned int?


u32 range is completely sufficient for number of proto hdrs (as said: 
"the proto layers must < VIRTCHNL_MAX_NUM_PROTO_HDRS") and I believe it 
is recommended to use fixed sized variables where possible


Do you have a pointer to the recommendation? I heard the opposite, that 
fixed length is only useful for register writes. Otherwise, you should 
use the “generic” types [1].



  union {
  struct virtchnl_proto_hdr
  proto_hdr[VIRTCHNL_MAX_NUM_PROTO_HDRS];
@@ -1395,7 +1395,7 @@ VIRTCHNL_CHECK_STRUCT_LEN(36, virtchnl_filter_action);
  struct virtchnl_filter_action_set {
  /* action number must be less then VIRTCHNL_MAX_NUM_ACTIONS */
-    int count;
+    u32 count;
  struct virtchnl_filter_action actions[VIRTCHNL_MAX_NUM_ACTIONS];
  };


Kind regards,

Paul


[1]: https://notabs.org/coding/smallIntsBigPenalty.htm

Re: [Intel-wired-lan] [PATCH iwl-next v7 5/9] igc: Add support for frame preemption verification

2025-03-04 Thread Vladimir Oltean 
On Mon, Mar 03, 2025 at 05:26:54AM -0500, Faizal Rahim wrote:
> +static inline bool igc_fpe_is_verify_or_response(union igc_adv_rx_desc 
> *rx_desc,
> +  unsigned int size)
> +{
> + u32 status_error = le32_to_cpu(rx_desc->wb.upper.status_error);
> + int smd;
> +
> + smd = FIELD_GET(IGC_RXDADV_STAT_SMD_TYPE_MASK, status_error);
> +
> + return (smd == IGC_RXD_STAT_SMD_TYPE_V || smd == 
> IGC_RXD_STAT_SMD_TYPE_R) &&
> + size == SMD_FRAME_SIZE;
> +}

The NIC should explicitly not respond to frames which have an SMD-V but
are not "verify" mPackets (7 octets of 0x55 + 1 octet SMD-V + 60 octets
of 0x00 + mCRC - as per 802.3 definitions). Similarly, it should only
treat SMD-R frames which contain 7 octets of 0x55 + 1 octet SMD-R + 60
octets of 0x00 + mCRC as "respond" mPackets, and only advance its
verification state machine based on those.

Specifically, it doesn't look like you are ensuring the packet payload
contains 60 octets of zeroes. Is this something that the hardware
already does for you, or is it something that needs further validation
and differentiation in software?

Re: [Intel-wired-lan] [PATCH iwl-next v1] ice: refactor the Tx scheduler feature

2025-03-04 Thread Szapar-Mudlaw, Martyna 




On 3/3/2025 10:54 AM, Simon Horman wrote:

On Wed, Feb 26, 2025 at 12:33:56PM +0100, Mateusz Polchlopek wrote:

Embed ice_get_tx_topo_user_sel() inside the only caller:
ice_devlink_tx_sched_layers_get().
Instead of jump from the wrapper to the function that does "get" operation
it does "get" itself.

Remove unnecessary comment and make usage of str_enabled_disabled()
in ice_init_tx_topology().


Hi Mateusz,

These changes seem reasonable to me.
But I wonder if they could be motivated in the commit message.

What I mean is, the commit message explains what has been done.
But I think it should explain why it has been done.


Hi Simon,

I'm replying on behalf of Mateusz since he's on leave, and we didn't 
want to keep this issue waiting too long.

Would such extended commit message make sense and address your concerns?

"Simplify the code by eliminating an unnecessary wrapper function. 
Previously, ice_devlink_tx_sched_layers_get() acted as a thin wrapper 
around ice_get_tx_topo_user_sel(), adding no real value but increasing 
code complexity. Since both functions were only used once, the wrapper 
was redundant and contributed approximately 20 lines of unnecessary 
code. By directly calling ice_get_tx_topo_user_sel(), improve 
readability and reduce function jumps, without altering functionality.
Also remove unnecessary comment and make usage of str_enabled_disabled() 
in ice_init_tx_topology()."


Thank you,
Martyna




Suggested-by: Marcin Szycik 
Reviewed-by: Michal Swiatkowski 
Reviewed-by: Jedrzej Jagielski 
Reviewed-by: Przemek Kitszel 
Reviewed-by: Aleksandr Loktionov 
Signed-off-by: Mateusz Polchlopek 


...

Re: [Intel-wired-lan] [iwl-net v3 1/5] virtchnl: make proto and filter action count unsigned

2025-03-04 Thread Szapar-Mudlaw, Martyna 




On 3/4/2025 12:51 PM, Paul Menzel wrote:

Dear Martyna,


Thank you for your quick reply.

Am 04.03.25 um 12:45 schrieb Szapar-Mudlaw, Martyna:


On 3/4/2025 12:15 PM, Paul Menzel wrote:



Am 04.03.25 um 12:08 schrieb Martyna Szapar-Mudlaw:

From: Jan Glaza 

The count field in virtchnl_proto_hdrs and virtchnl_filter_action_set
should never be negative while still being valid. Changing it from
int to u32 ensures proper handling of values in virtchnl messages in
driverrs and prevents unintended behavior.
In its current signed form, a negative count does not trigger
an error in ice driver but instead results in it being treated as 0.
This can lead to unexpected outcomes when processing messages.
By using u32, any invalid values will correctly trigger -EINVAL,
making error detection more robust.

Fixes: 1f7ea1cd6a374 ("ice: Enable FDIR Configure for AVF")
Reviewed-by: Jedrzej Jagielski 
Reviewed-by: Simon Horman 
Signed-off-by: Jan Glaza 
Signed-off-by: Martyna Szapar-Mudlaw mud...@linux.intel.com>

---
  include/linux/avf/virtchnl.h | 4 ++--
  1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/include/linux/avf/virtchnl.h b/include/linux/avf/ 
virtchnl.h

index 4811b9a14604..cf0afa60e4a7 100644
--- a/include/linux/avf/virtchnl.h
+++ b/include/linux/avf/virtchnl.h
@@ -1343,7 +1343,7 @@ struct virtchnl_proto_hdrs {
   * 2 - from the second inner layer
   * 
   **/
-    int count; /* the proto layers must < 
VIRTCHNL_MAX_NUM_PROTO_HDRS */
+    u32 count; /* the proto layers must < 
VIRTCHNL_MAX_NUM_PROTO_HDRS */


Why limit the length, and not use unsigned int?


u32 range is completely sufficient for number of proto hdrs (as said: 
"the proto layers must < VIRTCHNL_MAX_NUM_PROTO_HDRS") and I believe 
it is recommended to use fixed sized variables where possible


Do you have a pointer to the recommendation? I heard the opposite, that 
fixed length is only useful for register writes. Otherwise, you should 
use the “generic” types [1].


Thanks for sharing the source and your perspective, you are right, as a 
general rule, using generic types is preferred - I actually learned 
something new from this.
That said, I still believe there are exceptions, and in this case, using 
u32 is the right choice. When dealing with protocols or data formats 
using a fixed-width type makes sense.
Additionally, throughout this file, we consistently use u32/u16 for 
similar cases, so also here we're keeping it aligned with the existing 
codebase.

Thank you for your review and appreciate the discussion on best practices.

Regards,
Martyna




  union {
  struct virtchnl_proto_hdr
  proto_hdr[VIRTCHNL_MAX_NUM_PROTO_HDRS];
@@ -1395,7 +1395,7 @@ VIRTCHNL_CHECK_STRUCT_LEN(36, 
virtchnl_filter_action);

  struct virtchnl_filter_action_set {
  /* action number must be less then VIRTCHNL_MAX_NUM_ACTIONS */
-    int count;
+    u32 count;
  struct virtchnl_filter_action actions[VIRTCHNL_MAX_NUM_ACTIONS];
  };


Kind regards,

Paul


[1]: https://notabs.org/coding/smallIntsBigPenalty.htm

Re: [Intel-wired-lan] [PATCH] e1000e: Link flap workaround option for false IRP events

2025-03-04 Thread Andrew Lunn 
> > > However, that does not really help explain how this helps prevent an
> > > interrupt. I assume playing with EEE settings was also played
> > > with. Not that is register appears to have anything to do with EEE!
> > > 
> > I don't think we did tried those - it was never suggested that I can recall 
> > (the original debug started 6 months+ ago). I don't know fully what testing 
> > Intel did in their lab once the issue was reproduced there.
> > 
> > If you have any particular recommendations we can try that - with a note 
> > that we have to run a soak for ~1 week to have confidence if a change made 
> > a difference (the issue can reproduce between 1 to 2 days).
> 
> Personally I doubt that it is related to EEE since there was no real link
> flap.

I tend to agree. Despite the group of registers being called LPI, it
appears this one has nothing to do with LPI? It would probably been
better to have it in page 776, General Registers, but that region is
full.

> > > I don't follow what you are saying here. As far as i can see, the
> > > interrupt handler will triggers a read of the BMCR to determine the
> > > link status. It should not matter if there is a spurious interrupt,
> > > the BMCR should report the truth. So does BMCR actually indicate the
> > > link did go down? I also see there is the usual misunderstanding with
> > > how BMCR is latching. It should not be read twice, processed once, it
> > > should be processed each time, otherwise you miss quick link down/up
> > > events.
> > > 
> > > > We communicated that this solution is not likely to be accepted to the
> > > > kernel as is, and the initial responses on the mailing list demonstrate 
> > > > the
> > > > pushback.
> > > 
> > > What it has done is start a discussion towards an acceptable
> > > solution. Which is a good thing. But at the moment, the discussion
> > > does not have sufficient details.
> > > 
> > > Please could somebody describe the chain of events which results in
> > > the link down, and subsequent link up. Is the interrupt spurious, or
> > > does BMCR really indicate the link went down and up again?
> > > 
> > 
> > I'm fairly certain there is no actual link bounce but I don't know the 
> > reason for the interrupt or why it was triggered.
> > 
> > Vitaly, do you have a way of getting these answers from the Intel team that 
> > worked on this? I don't think I'll be able to get any answers, 
> > unfortunately.
> 
> You are correct, from what we saw there was no real link flap there. Only a
> false link status change interrupt.
 
So if BMCR shows no state change, why is the driver doing anything?

I really would like to understand the chain of events. Once we
understand the chain of events, we can probably come up with a change
somewhere in the chain to break it, so the spurious interrupt is
ignored.

Andrew

Re: [Intel-wired-lan] [PATCH iwl-next v1 1/3] ice: remove SW side band access workaround for E825

2025-03-04 Thread Nitka, Grzegorz 
> -Original Message-
> From: Paul Menzel 
> Sent: Friday, February 21, 2025 10:16 PM
> To: Nitka, Grzegorz ; Kolacinski, Karol
> 
> Cc: intel-wired-...@lists.osuosl.org; net...@vger.kernel.org; Kitszel,
> Przemyslaw ; Michal Swiatkowski
> 
> Subject: Re: [Intel-wired-lan] [PATCH iwl-next v1 1/3] ice: remove SW side
> band access workaround for E825
> 
> Dear Grzegorz, dear Karol,
> 
> 
> Thank you for your patch.
> 
> Am 21.02.25 um 13:31 schrieb Grzegorz Nitka:
> > From: Karol Kolacinski 
> >
> > Due to the bug in FW/NVM autoload mechanism (wrong default
> > SB_REM_DEV_CTL register settings), the access to peer PHY and CGU
> > clients was disabled by default.
> 
> I’d add a blank line between the paragraphs.
> 
> > As the workaround solution, the register value was overwritten by the
> > driver at the probe or reset handling.
> > Remove workaround as it's not needed anymore. The fix in autoload
> > procedure has been provided with NVM 3.80 version.
> 
> Is this compatible with Linux’ no regression policy? People might only
> update the Linux kernel and not the firmware.
> 
> How did you test this, and how can others test this?
> 
> > Reviewed-by: Michal Swiatkowski 
> > Reviewed-by: Przemek Kitszel 
> > Signed-off-by: Karol Kolacinski 
> > Signed-off-by: Grzegorz Nitka 
> > ---
> >   drivers/net/ethernet/intel/ice/ice_ptp_hw.c | 23 -
> >   1 file changed, 23 deletions(-)
> 
> 
> Kind regards,
> 
> Paul
> 
> 

Dear Paul, first of all thank you for your review and apologize for late
response (simply, I was out previous week).

Removing that workaround was a conscious decision.
Rationale for that is, that the 'problematic' workaround was provided
in very early product development stage (~ year ago).  Now, especially
when E825-C product was just officially announced, the customer shall
use official, approved SW ingredients.

Here are more details on this:
- introduction to E825-C devices was provided in kernel 6.6, to allow
  selected customers for early E825-C enablement. At that time E825-C
  product family was in early phase (kind of Alpha), and one of the
  consequences, from today's perspective,  is that it included 'unwanted'
  workarounds like this.

- this change applies to E825-C products only, which is SoC product, not
  a regular NIC card.  What I'd like to emphasize here, it requires even more
  customer support from Intel company in terms of providing matching
  SW stack (like BIOS, firmware, drivers etc.).
  
I see two possibilities now:
1) if the regression policy you mentioned is inviolable, keep the workaround
   in the kernel code like it is today. Maybe we could add NVM version checker
   and apply register updates for older NVMs only.
   But, in my opinion, it would lead to keeping a dead code. There shouldn't be
   official FW (I hope I won't regret these words) on the market which requires
   this workaround.
  
2) remove the workaround like it's implemented in this patch and improve
   commit message to clarify all potential doubts/questions from the user
   perspective.

What's your thoughts?

Kind regards

Grzegorz

> > diff --git a/drivers/net/ethernet/intel/ice/ice_ptp_hw.c
> b/drivers/net/ethernet/intel/ice/ice_ptp_hw.c
> > index 89bb8461284a..a5df081ffc19 100644
> > --- a/drivers/net/ethernet/intel/ice/ice_ptp_hw.c
> > +++ b/drivers/net/ethernet/intel/ice/ice_ptp_hw.c
> > @@ -2630,25 +2630,6 @@ int ice_start_phy_timer_eth56g(struct ice_hw
> *hw, u8 port)
> > return 0;
> >   }
> >
> > -/**
> > - * ice_sb_access_ena_eth56g - Enable SB devices (PHY and others) access
> > - * @hw: pointer to HW struct
> > - * @enable: Enable or disable access
> > - *
> > - * Enable sideband devices (PHY and others) access.
> > - */
> > -static void ice_sb_access_ena_eth56g(struct ice_hw *hw, bool enable)
> > -{
> > -   u32 val = rd32(hw, PF_SB_REM_DEV_CTL);
> > -
> > -   if (enable)
> > -   val |= BIT(eth56g_phy_0) | BIT(cgu) | BIT(eth56g_phy_1);
> > -   else
> > -   val &= ~(BIT(eth56g_phy_0) | BIT(cgu) | BIT(eth56g_phy_1));
> > -
> > -   wr32(hw, PF_SB_REM_DEV_CTL, val);
> > -}
> > -
> >   /**
> >* ice_ptp_init_phc_e825 - Perform E825 specific PHC initialization
> >* @hw: pointer to HW struct
> > @@ -2659,8 +2640,6 @@ static void ice_sb_access_ena_eth56g(struct
> ice_hw *hw, bool enable)
> >*/
> >   static int ice_ptp_init_phc_e825(struct ice_hw *hw)
> >   {
> > -   ice_sb_access_ena_eth56g(hw, true);
> > -
> > /* Initialize the Clock Generation Unit */
> > return ice_init_cgu_e82x(hw);
> >   }
> > @@ -2747,8 +2726,6 @@ static void ice_ptp_init_phy_e825(struct ice_hw
> *hw)
> > params->num_phys = 2;
> > ptp->ports_per_phy = 4;
> > ptp->num_lports = params->num_phys * ptp->ports_per_phy;
> > -
> > -   ice_sb_access_ena_eth56g(hw, true);
> >   }
> >
> >   /* E822 family functions

Re: [Intel-wired-lan] [PATCH net-next v9 2/6] net: ena: use napi's aRFS rmap notifers

2025-03-04 Thread Ahmed Zaki 

[RE-SEND] I just realized I sent this only to iwl, sorry for spamming.


On 2025-03-03 10:11 a.m., Arinzon, David wrote:

Use the core's rmap notifiers and delete our own.

Acked-by: David Arinzon 
Signed-off-by: Ahmed Zaki 
---
  drivers/net/ethernet/amazon/ena/ena_netdev.c | 43 +---
  1 file changed, 1 insertion(+), 42 deletions(-)

diff --git a/drivers/net/ethernet/amazon/ena/ena_netdev.c
b/drivers/net/ethernet/amazon/ena/ena_netdev.c
index c1295dfad0d0..6aab85a7c60a 100644
--- a/drivers/net/ethernet/amazon/ena/ena_netdev.c
+++ b/drivers/net/ethernet/amazon/ena/ena_netdev.c
@@ -5,9 +5,6 @@

  #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt

-#ifdef CONFIG_RFS_ACCEL
-#include 
-#endif /* CONFIG_RFS_ACCEL */
  #include 
  #include 
  #include 
@@ -162,30 +159,6 @@ int ena_xmit_common(struct ena_adapter
*adapter,
 return 0;
  }

-static int ena_init_rx_cpu_rmap(struct ena_adapter *adapter) -{ -#ifdef
CONFIG_RFS_ACCEL
-   u32 i;
-   int rc;
-
-   adapter->netdev->rx_cpu_rmap = alloc_irq_cpu_rmap(adapter-

num_io_queues);

-   if (!adapter->netdev->rx_cpu_rmap)
-   return -ENOMEM;
-   for (i = 0; i < adapter->num_io_queues; i++) {
-   int irq_idx = ENA_IO_IRQ_IDX(i);
-
-   rc = irq_cpu_rmap_add(adapter->netdev->rx_cpu_rmap,
- pci_irq_vector(adapter->pdev, irq_idx));
-   if (rc) {
-   free_irq_cpu_rmap(adapter->netdev->rx_cpu_rmap);
-   adapter->netdev->rx_cpu_rmap = NULL;
-   return rc;
-   }
-   }
-#endif /* CONFIG_RFS_ACCEL */
-   return 0;
-}
-
  static void ena_init_io_rings_common(struct ena_adapter *adapter,
  struct ena_ring *ring, u16 qid)  { @@ 
-1596,7 +1569,7 @@
static int ena_enable_msix(struct ena_adapter *adapter)
 adapter->num_io_queues = irq_cnt - ENA_ADMIN_MSIX_VEC;
 }

-   if (ena_init_rx_cpu_rmap(adapter))
+   if (netif_enable_cpu_rmap(adapter->netdev,
+ adapter->num_io_queues))
 netif_warn(adapter, probe, adapter->netdev,
"Failed to map IRQs to CPUs\n");

@@ -1742,13 +1715,6 @@ static void ena_free_io_irq(struct ena_adapter
*adapter)
 struct ena_irq *irq;
 int i;

-#ifdef CONFIG_RFS_ACCEL
-   if (adapter->msix_vecs >= 1) {
-   free_irq_cpu_rmap(adapter->netdev->rx_cpu_rmap);
-   adapter->netdev->rx_cpu_rmap = NULL;
-   }
-#endif /* CONFIG_RFS_ACCEL */
-
 for (i = ENA_IO_IRQ_FIRST_IDX; i <
ENA_MAX_MSIX_VEC(io_queue_count); i++) {
 irq = &adapter->irq_tbl[i];
 irq_set_affinity_hint(irq->vector, NULL); @@ -4131,13 +4097,6 
@@
static void __ena_shutoff(struct pci_dev *pdev, bool shutdown)
 ena_dev = adapter->ena_dev;
 netdev = adapter->netdev;

-#ifdef CONFIG_RFS_ACCEL
-   if ((adapter->msix_vecs >= 1) && (netdev->rx_cpu_rmap)) {
-   free_irq_cpu_rmap(netdev->rx_cpu_rmap);
-   netdev->rx_cpu_rmap = NULL;
-   }
-
-#endif /* CONFIG_RFS_ACCEL */
 /* Make sure timer and reset routine won't be called after
  * freeing device resources.
  */
--
2.43.0


Hi Ahmed,

After the merging of this patch, I see the below stack trace when the IRQs are 
freed.
It can be reproduced by unloading and loading the driver using `modprobe -r 
ena; modprobe ena` (happens during unload)

Based on the patchset and the changes to other drivers, I think there's a 
missing call to the function
that releases the affinity notifier (The warn is in 
https://web.git.kernel.org/pub/scm/linux/kernel/git/netdev/net-next.git/tree/kernel/irq/manage.c#n2031)

I saw in the intel code in the patchset that ` netif_napi_set_irq(, -1);` 
is added?

After adding the code snippet I don't see this anymore, but I want to 
understand whether it's the right call by design.


Yes, in ena_down() the IRQs are freed before napis are deleted (where 
IRQ notifiers are released). The code below is fine (and is better IMO) 
but you can also delete napis then free IRQs.





@@ -1716,8 +1716,11 @@ static void ena_free_io_irq(struct ena_adapter *adapter)
 int i;

 for (i = ENA_IO_IRQ_FIRST_IDX; i < ENA_MAX_MSIX_VEC(io_queue_count); 
i++) {
+   struct ena_napi *ena_napi;
+
 irq = &adapter->irq_tbl[i];
 irq_set_affinity_hint(irq->vector, NULL);
+   ena_napi = (struct ena_napi *)irq->data;
+   netif_napi_set_irq(&ena_napi->napi, -1);
 free_irq(irq->vector, irq->data);
 }
  }

[  484.544586]  ? __warn+0x84/0x130
[  484.544843]  ? free_irq+0x5c/0x70
[  484.545105]  ? report_bug+0x18a/0x1a0
[  484.545390]  ? handle_bug+0x53/0x90
[  484.545664]  ? exc_invalid_op+0x14/0x70
[  484.545959]  ? asm_exc_invalid_op+0x16/0x20
[  484.546279]  ? free_irq+0x5c/0x70
[  484.546545]  ? free_irq+0x10/

Re: [Intel-wired-lan] [PATCH] e1000e: Link flap workaround option for false IRP events

2025-03-04 Thread Mark Pearson 
Thanks Vitaly,

On Tue, Mar 4, 2025, at 5:48 AM, Lifshits, Vitaly wrote:
> On 3/3/2025 5:34 AM, Mark Pearson wrote:
>> Hi Andrew,
>> 
>> On Sun, Mar 2, 2025, at 11:13 AM, Andrew Lunn wrote:
>>> On Sun, Mar 02, 2025 at 03:09:35PM +0200, Lifshits, Vitaly wrote:


 Hi Mark,

> Hi Andrew
>
> On Thu, Feb 27, 2025, at 11:07 AM, Andrew Lunn wrote:
> + e1e_rphy(hw, PHY_REG(772, 26), &phy_data);

 Please add some #define for these magic numbers, so we have some idea
 what PHY register you are actually reading. That in itself might help
 explain how the workaround actually works.

>>>
>>> I don't know what this register does I'm afraid - that's Intel 
>>> knowledge and has not been shared.
>>
>> What PHY is it? Often it is just a COTS PHY, and the datasheet might
>> be available.
>>
>> Given your setup description, pause seems like the obvious thing to
>> check. When trying to debug this, did you look at pause settings?
>> Knowing what this register is might also point towards pause, or
>> something totally different.
>>
>>  Andrew
>
> For the PHY - do you know a way of determining this easily? I can reach 
> out to the platform team but that will take some time. I'm not seeing 
> anything in the kernel logs, but if there's a recommended way of 
> confirming that would be appreciated.

 The PHY is I219 PHY.
 The datasheet is indeed accessible to the public:
 https://cdrdv2-public.intel.com/612523/ethernet-connection-i219-datasheet.pdf
>>>
>>> Thanks for the link.
>>>
>>> So it is reading page 772, register 26. Page 772 is all about LPI. So
>>> we can have a #define for that. Register 26 is Memories Power. So we
>>> can also have an #define for that.
>> 
>> Yep - I'll look to add this.
>> 
>>>
>>> However, that does not really help explain how this helps prevent an
>>> interrupt. I assume playing with EEE settings was also played
>>> with. Not that is register appears to have anything to do with EEE!
>>>
>> I don't think we did tried those - it was never suggested that I can recall 
>> (the original debug started 6 months+ ago). I don't know fully what testing 
>> Intel did in their lab once the issue was reproduced there.
>> 
>> If you have any particular recommendations we can try that - with a note 
>> that we have to run a soak for ~1 week to have confidence if a change made a 
>> difference (the issue can reproduce between 1 to 2 days).
>
> Personally I doubt that it is related to EEE since there was no real 
> link flap.
>
> I suggest to try replacing the register read for a short delay or 
> reading the PHY STATUS register instead.
>

Ack - we'll try that, and collect some other debug registers in the process.
Will update with findings - this may take a while :)

Thanks
Mark

Re: [Intel-wired-lan] [PATCH iwl-next v3] igc: Change Tx mode for MQPRIO offloading

2025-03-04 Thread Simon Horman 
On Mon, Mar 03, 2025 at 10:16:33AM +0100, Kurt Kanzenbach wrote:
> The current MQPRIO offload implementation uses the legacy TSN Tx mode. In
> this mode the hardware uses four packet buffers and considers queue
> priorities.
> 
> In order to harmonize the TAPRIO implementation with MQPRIO, switch to the
> regular TSN Tx mode. This mode also uses four packet buffers and considers
> queue priorities. In addition to the legacy mode, transmission is always
> coupled to Qbv. The driver already has mechanisms to use a dummy schedule
> of 1 second with all gates open for ETF. Simply use this for MQPRIO too.
> 
> This reduces code and makes it easier to add support for frame preemption
> later.
> 
> While at it limit the netdev_tc calls to MQPRIO only.

Hi Kurt,

Can this part be broken out into a separate patch?
It seems so to me, but perhaps I'm missing something.

The reason that I ask is that this appears to be a good portion of the
change, and doing so would make the code changes for main part of the
patch, as per the description prior to the line above, clearer IMHO.

> 
> Tested on i225 with real time application using high priority queue, iperf3
> using low priority queue and network TAP device.
> 
> Signed-off-by: Kurt Kanzenbach 

...

Re: [Intel-wired-lan] [PATCH net-next v9 2/6] net: ena: use napi's aRFS rmap notifers

2025-03-04 Thread Arinzon, David 
> [RE-SEND] I just realized I sent this only to iwl, sorry for spamming.
> 
> 
> On 2025-03-03 10:11 a.m., Arinzon, David wrote:
> >> Use the core's rmap notifiers and delete our own.
> >>
> >> Acked-by: David Arinzon 
> >> Signed-off-by: Ahmed Zaki 
> >> ---
> >>   drivers/net/ethernet/amazon/ena/ena_netdev.c | 43 +---
> >>   1 file changed, 1 insertion(+), 42 deletions(-)
> >>
> >> diff --git a/drivers/net/ethernet/amazon/ena/ena_netdev.c
> >> b/drivers/net/ethernet/amazon/ena/ena_netdev.c
> >> index c1295dfad0d0..6aab85a7c60a 100644
> >> --- a/drivers/net/ethernet/amazon/ena/ena_netdev.c
> >> +++ b/drivers/net/ethernet/amazon/ena/ena_netdev.c
> >> @@ -5,9 +5,6 @@
> >>
> >>   #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
> >>
> >> -#ifdef CONFIG_RFS_ACCEL
> >> -#include 
> >> -#endif /* CONFIG_RFS_ACCEL */
> >>   #include 
> >>   #include 
> >>   #include 
> >> @@ -162,30 +159,6 @@ int ena_xmit_common(struct ena_adapter
> *adapter,
> >>  return 0;
> >>   }
> >>
> >> -static int ena_init_rx_cpu_rmap(struct ena_adapter *adapter) -{
> >> -#ifdef CONFIG_RFS_ACCEL
> >> -   u32 i;
> >> -   int rc;
> >> -
> >> -   adapter->netdev->rx_cpu_rmap = alloc_irq_cpu_rmap(adapter-
> >>> num_io_queues);
> >> -   if (!adapter->netdev->rx_cpu_rmap)
> >> -   return -ENOMEM;
> >> -   for (i = 0; i < adapter->num_io_queues; i++) {
> >> -   int irq_idx = ENA_IO_IRQ_IDX(i);
> >> -
> >> -   rc = irq_cpu_rmap_add(adapter->netdev->rx_cpu_rmap,
> >> - pci_irq_vector(adapter->pdev, 
> >> irq_idx));
> >> -   if (rc) {
> >> -   free_irq_cpu_rmap(adapter->netdev->rx_cpu_rmap);
> >> -   adapter->netdev->rx_cpu_rmap = NULL;
> >> -   return rc;
> >> -   }
> >> -   }
> >> -#endif /* CONFIG_RFS_ACCEL */
> >> -   return 0;
> >> -}
> >> -
> >>   static void ena_init_io_rings_common(struct ena_adapter *adapter,
> >>   struct ena_ring *ring, u16 qid)
> >> { @@ -1596,7 +1569,7 @@ static int ena_enable_msix(struct ena_adapter
> *adapter)
> >>  adapter->num_io_queues = irq_cnt - ENA_ADMIN_MSIX_VEC;
> >>  }
> >>
> >> -   if (ena_init_rx_cpu_rmap(adapter))
> >> +   if (netif_enable_cpu_rmap(adapter->netdev,
> >> + adapter->num_io_queues))
> >>  netif_warn(adapter, probe, adapter->netdev,
> >> "Failed to map IRQs to CPUs\n");
> >>
> >> @@ -1742,13 +1715,6 @@ static void ena_free_io_irq(struct ena_adapter
> >> *adapter)
> >>  struct ena_irq *irq;
> >>  int i;
> >>
> >> -#ifdef CONFIG_RFS_ACCEL
> >> -   if (adapter->msix_vecs >= 1) {
> >> -   free_irq_cpu_rmap(adapter->netdev->rx_cpu_rmap);
> >> -   adapter->netdev->rx_cpu_rmap = NULL;
> >> -   }
> >> -#endif /* CONFIG_RFS_ACCEL */
> >> -
> >>  for (i = ENA_IO_IRQ_FIRST_IDX; i <
> >> ENA_MAX_MSIX_VEC(io_queue_count); i++) {
> >>  irq = &adapter->irq_tbl[i];
> >>  irq_set_affinity_hint(irq->vector, NULL); @@
> >> -4131,13 +4097,6 @@ static void __ena_shutoff(struct pci_dev *pdev,
> bool shutdown)
> >>  ena_dev = adapter->ena_dev;
> >>  netdev = adapter->netdev;
> >>
> >> -#ifdef CONFIG_RFS_ACCEL
> >> -   if ((adapter->msix_vecs >= 1) && (netdev->rx_cpu_rmap)) {
> >> -   free_irq_cpu_rmap(netdev->rx_cpu_rmap);
> >> -   netdev->rx_cpu_rmap = NULL;
> >> -   }
> >> -
> >> -#endif /* CONFIG_RFS_ACCEL */
> >>  /* Make sure timer and reset routine won't be called after
> >>   * freeing device resources.
> >>   */
> >> --
> >> 2.43.0
> >
> > Hi Ahmed,
> >
> > After the merging of this patch, I see the below stack trace when the IRQs
> are freed.
> > It can be reproduced by unloading and loading the driver using
> > `modprobe -r ena; modprobe ena` (happens during unload)
> >
> > Based on the patchset and the changes to other drivers, I think
> > there's a missing call to the function that releases the affinity
> > notifier (The warn is in
> > https://web.git.kernel.org/pub/scm/linux/kernel/git/netdev/net-next.gi
> > t/tree/kernel/irq/manage.c#n2031)
> >
> > I saw in the intel code in the patchset that ` netif_napi_set_irq(, 
> > -1);`
> is added?
> >
> > After adding the code snippet I don't see this anymore, but I want to
> understand whether it's the right call by design.
> 
> Yes, in ena_down() the IRQs are freed before napis are deleted (where IRQ
> notifiers are released). The code below is fine (and is better IMO) but you
> can also delete napis then free IRQs.
> 
> 

Thanks for the clarification. Some book-keeping, as this change fixes the issue.
The need to use `netif_napi_set_irq` was introduced in 
https://lore.kernel.org/netdev/20241002001331.65444-2-jdam...@fastly.com/,
But, technically, there was not need to use the call with the

Re: [Intel-wired-lan] [PATCH] e1000e: Link flap workaround option for false IRP events

2025-03-04 Thread Lifshits, Vitaly 




On 3/3/2025 5:34 AM, Mark Pearson wrote:

Hi Andrew,

On Sun, Mar 2, 2025, at 11:13 AM, Andrew Lunn wrote:

On Sun, Mar 02, 2025 at 03:09:35PM +0200, Lifshits, Vitaly wrote:



Hi Mark,


Hi Andrew

On Thu, Feb 27, 2025, at 11:07 AM, Andrew Lunn wrote:

+   e1e_rphy(hw, PHY_REG(772, 26), &phy_data);


Please add some #define for these magic numbers, so we have some idea
what PHY register you are actually reading. That in itself might help
explain how the workaround actually works.



I don't know what this register does I'm afraid - that's Intel knowledge and 
has not been shared.


What PHY is it? Often it is just a COTS PHY, and the datasheet might
be available.

Given your setup description, pause seems like the obvious thing to
check. When trying to debug this, did you look at pause settings?
Knowing what this register is might also point towards pause, or
something totally different.

Andrew


For the PHY - do you know a way of determining this easily? I can reach out to 
the platform team but that will take some time. I'm not seeing anything in the 
kernel logs, but if there's a recommended way of confirming that would be 
appreciated.


The PHY is I219 PHY.
The datasheet is indeed accessible to the public:
https://cdrdv2-public.intel.com/612523/ethernet-connection-i219-datasheet.pdf


Thanks for the link.

So it is reading page 772, register 26. Page 772 is all about LPI. So
we can have a #define for that. Register 26 is Memories Power. So we
can also have an #define for that.


Yep - I'll look to add this.



However, that does not really help explain how this helps prevent an
interrupt. I assume playing with EEE settings was also played
with. Not that is register appears to have anything to do with EEE!


I don't think we did tried those - it was never suggested that I can recall 
(the original debug started 6 months+ ago). I don't know fully what testing 
Intel did in their lab once the issue was reproduced there.

If you have any particular recommendations we can try that - with a note that 
we have to run a soak for ~1 week to have confidence if a change made a 
difference (the issue can reproduce between 1 to 2 days).


Personally I doubt that it is related to EEE since there was no real 
link flap.


I suggest to try replacing the register read for a short delay or 
reading the PHY STATUS register instead.





Reading this register was suggested for debug purposes to understand if
there is some misconfiguration. We did not find any misconfiguration.
The issue as we discovered was a link status change interrupt caused the
CSME to reset the adapter causing the link flap.

We were unable to determine what causes the link status change interrupt in
the first place. As stated in the comment, it was only ever observed on
Lenovo P5/P7systems and we couldn't ever reproduce on other systems. The
reproduction in our lab was on a P5 system as well.


Regarding the suggested workaround, there isn’t a clear understanding why it
works. We suspect that reading a PHY register is probably prevents the CSME
from resetting the PHY when it handles the LSC interrupt it gets. However,
it can also be a matter of slight timing variations.


I don't follow what you are saying here. As far as i can see, the
interrupt handler will triggers a read of the BMCR to determine the
link status. It should not matter if there is a spurious interrupt,
the BMCR should report the truth. So does BMCR actually indicate the
link did go down? I also see there is the usual misunderstanding with
how BMCR is latching. It should not be read twice, processed once, it
should be processed each time, otherwise you miss quick link down/up
events.


We communicated that this solution is not likely to be accepted to the
kernel as is, and the initial responses on the mailing list demonstrate the
pushback.


What it has done is start a discussion towards an acceptable
solution. Which is a good thing. But at the moment, the discussion
does not have sufficient details.

Please could somebody describe the chain of events which results in
the link down, and subsequent link up. Is the interrupt spurious, or
does BMCR really indicate the link went down and up again?



I'm fairly certain there is no actual link bounce but I don't know the reason 
for the interrupt or why it was triggered.

Vitaly, do you have a way of getting these answers from the Intel team that 
worked on this? I don't think I'll be able to get any answers, unfortunately.


You are correct, from what we saw there was no real link flap there. 
Only a false link status change interrupt.





On a different topic, I suggest removing the part of the comment below:
* Intel unable to determine root cause.
The issue went through joint debug by Intel and Lenovo, and no obvious spec
violations by either party were found. There doesn’t seem to be value in
including this information in the comments of upstream code.


I partially agree. Assuming the root cause i

Re: [Intel-wired-lan] [PATCH v2] e1000: The 'const' qualifier has been added where applicable to enhance code safety and prevent unintended modifications.

2025-03-04 Thread Loktionov, Aleksandr 


> -Original Message-
> From: Intel-wired-lan  On Behalf Of
> Przemek Kitszel
> Sent: Tuesday, March 4, 2025 9:28 AM
> To: joaomboni 
> Cc: Nguyen, Anthony L ;
> andrew+net...@lunn.ch; da...@davemloft.net; eduma...@google.com;
> k...@kernel.org; pab...@redhat.com; intel-wired-...@lists.osuosl.org;
> net...@vger.kernel.org; linux-ker...@vger.kernel.org
> Subject: Re: [Intel-wired-lan] [PATCH v2] e1000: The 'const' qualifier has 
> been
> added where applicable to enhance code safety and prevent unintended
> modifications.
> 
> On 3/3/25 21:47, joaomboni wrote:
> > Signed-off-by: Joao Bonifacio 
> 
> it will be good to use imperative mood in the Subject, and add one more
> paragraph, like:
> 
> Subject: e1000: mark global variables const where possible
> 
I'd suggest 'fix' 
e1000: fix global variables const where possible
But anyway the change is useful and so small and well-understandable.

> Next paragraph:
> Mark global variables const, so unintended modification would not be
> possible.
> 
> > ---
> >   drivers/net/ethernet/intel/e1000/e1000_main.c | 2 +-
> >   1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/net/ethernet/intel/e1000/e1000_main.c
> > b/drivers/net/ethernet/intel/e1000/e1000_main.c
> > index 3f089c3d47b2..96bc85f09aaf 100644
> > --- a/drivers/net/ethernet/intel/e1000/e1000_main.c
> > +++ b/drivers/net/ethernet/intel/e1000/e1000_main.c
> > @@ -9,7 +9,7 @@
> >   #include 
> >
> >   char e1000_driver_name[] = "e1000";
> 
> your commit message suggests that you add const "everywhere", but it seems
> that there are other candidates, like the one above
> 
> PS. You have to wait 24h before posting next revision.
> 
> > -static char e1000_driver_string[] = "Intel(R) PRO/1000 Network
> > Driver";
> > +static const char e1000_driver_string[] = "Intel(R) PRO/1000 Network
> > +Driver";
> >   static const char e1000_copyright[] = "Copyright (c) 1999-2006 Intel
> > Corporation.";
> >
> >   /* e1000_pci_tbl - PCI Device ID Table

Re: [Intel-wired-lan] [PATCH v2] e1000: The 'const' qualifier has been added where applicable to enhance code safety and prevent unintended modifications.

2025-03-04 Thread Loktionov, Aleksandr 



> -Original Message-
> From: Intel-wired-lan  On Behalf Of
> joaomboni
> Sent: Monday, March 3, 2025 9:48 PM
> To: Nguyen, Anthony L ; Kitszel, Przemyslaw
> ; andrew+net...@lunn.ch;
> da...@davemloft.net; eduma...@google.com; k...@kernel.org;
> pab...@redhat.com
> Cc: intel-wired-...@lists.osuosl.org; net...@vger.kernel.org; linux-
> ker...@vger.kernel.org; joaomboni 
> Subject: [Intel-wired-lan] [PATCH v2] e1000: The 'const' qualifier has been
> added where applicable to enhance code safety and prevent unintended
> modifications.
> 
> Signed-off-by: Joao Bonifacio 
Good!
Reviewed-by: Aleksandr Loktionov 

> ---
>  drivers/net/ethernet/intel/e1000/e1000_main.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/net/ethernet/intel/e1000/e1000_main.c
> b/drivers/net/ethernet/intel/e1000/e1000_main.c
> index 3f089c3d47b2..96bc85f09aaf 100644
> --- a/drivers/net/ethernet/intel/e1000/e1000_main.c
> +++ b/drivers/net/ethernet/intel/e1000/e1000_main.c
> @@ -9,7 +9,7 @@
>  #include 
> 
>  char e1000_driver_name[] = "e1000";
> -static char e1000_driver_string[] = "Intel(R) PRO/1000 Network Driver";
> +static const char e1000_driver_string[] = "Intel(R) PRO/1000 Network
> Driver";
>  static const char e1000_copyright[] = "Copyright (c) 1999-2006 Intel
> Corporation.";
> 
>  /* e1000_pci_tbl - PCI Device ID Table
> --
> 2.48.1

[Intel-wired-lan] [iwl-net v3 2/5] ice: stop truncating queue ids when checking

2025-03-04 Thread Martyna Szapar-Mudlaw 
From: Jan Glaza 

Queue IDs can be up to 4096, fix invalid check to stop
truncating IDs to 8 bits.

Fixes: bf93bf791cec8 ("ice: introduce ice_virtchnl.c and ice_virtchnl.h")
Reviewed-by: Aleksandr Loktionov 
Reviewed-by: Jedrzej Jagielski 
Reviewed-by: Simon Horman 
Signed-off-by: Jan Glaza 
Signed-off-by: Martyna Szapar-Mudlaw 
---
 drivers/net/ethernet/intel/ice/ice_virtchnl.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/intel/ice/ice_virtchnl.c 
b/drivers/net/ethernet/intel/ice/ice_virtchnl.c
index b6285433307c..343f2b4b0dc5 100644
--- a/drivers/net/ethernet/intel/ice/ice_virtchnl.c
+++ b/drivers/net/ethernet/intel/ice/ice_virtchnl.c
@@ -565,7 +565,7 @@ bool ice_vc_isvalid_vsi_id(struct ice_vf *vf, u16 vsi_id)
  *
  * check for the valid queue ID
  */
-static bool ice_vc_isvalid_q_id(struct ice_vsi *vsi, u8 qid)
+static bool ice_vc_isvalid_q_id(struct ice_vsi *vsi, u16 qid)
 {
/* allocated Tx and Rx queues should be always equal for VF VSI */
return qid < vsi->alloc_txq;
-- 
2.47.0

[Intel-wired-lan] [iwl-net v3 3/5] ice: validate queue quanta parameters to prevent OOB access

2025-03-04 Thread Martyna Szapar-Mudlaw 
From: Jan Glaza 

Add queue wraparound prevention in quanta configuration.
Ensure end_qid does not overflow by validating start_qid and num_queues.

Fixes: 015307754a19 ("ice: Support VF queue rate limit and quanta size 
configuration")
Reviewed-by: Jedrzej Jagielski 
Reviewed-by: Simon Horman 
Signed-off-by: Jan Glaza 
Signed-off-by: Martyna Szapar-Mudlaw 
---
 drivers/net/ethernet/intel/ice/ice_virtchnl.c | 13 +
 1 file changed, 9 insertions(+), 4 deletions(-)

diff --git a/drivers/net/ethernet/intel/ice/ice_virtchnl.c 
b/drivers/net/ethernet/intel/ice/ice_virtchnl.c
index 343f2b4b0dc5..adb1bf12542f 100644
--- a/drivers/net/ethernet/intel/ice/ice_virtchnl.c
+++ b/drivers/net/ethernet/intel/ice/ice_virtchnl.c
@@ -1903,13 +1903,21 @@ static int ice_vc_cfg_q_bw(struct ice_vf *vf, u8 *msg)
  */
 static int ice_vc_cfg_q_quanta(struct ice_vf *vf, u8 *msg)
 {
+   u16 quanta_prof_id, quanta_size, start_qid, num_queues, end_qid, i;
enum virtchnl_status_code v_ret = VIRTCHNL_STATUS_SUCCESS;
-   u16 quanta_prof_id, quanta_size, start_qid, end_qid, i;
struct virtchnl_quanta_cfg *qquanta =
(struct virtchnl_quanta_cfg *)msg;
struct ice_vsi *vsi;
int ret;
 
+   start_qid = qquanta->queue_select.start_queue_id;
+   num_queues = qquanta->queue_select.num_queues;
+
+   if (check_add_overflow(start_qid, num_queues, &end_qid)) {
+   v_ret = VIRTCHNL_STATUS_ERR_PARAM;
+   goto err;
+   }
+
if (!test_bit(ICE_VF_STATE_ACTIVE, vf->vf_states)) {
v_ret = VIRTCHNL_STATUS_ERR_PARAM;
goto err;
@@ -1921,8 +1929,6 @@ static int ice_vc_cfg_q_quanta(struct ice_vf *vf, u8 *msg)
goto err;
}
 
-   end_qid = qquanta->queue_select.start_queue_id +
- qquanta->queue_select.num_queues;
if (end_qid > ICE_MAX_RSS_QS_PER_VF ||
end_qid > min_t(u16, vsi->alloc_txq, vsi->alloc_rxq)) {
dev_err(ice_pf_to_dev(vf->pf), "VF-%d trying to configure more 
than allocated number of queues: %d\n",
@@ -1951,7 +1957,6 @@ static int ice_vc_cfg_q_quanta(struct ice_vf *vf, u8 *msg)
goto err;
}
 
-   start_qid = qquanta->queue_select.start_queue_id;
for (i = start_qid; i < end_qid; i++)
vsi->tx_rings[i]->quanta_prof_id = quanta_prof_id;
 
-- 
2.47.0

[Intel-wired-lan] [iwl-net v3 1/5] virtchnl: make proto and filter action count unsigned

2025-03-04 Thread Martyna Szapar-Mudlaw 
From: Jan Glaza 

The count field in virtchnl_proto_hdrs and virtchnl_filter_action_set
should never be negative while still being valid. Changing it from
int to u32 ensures proper handling of values in virtchnl messages in
driverrs and prevents unintended behavior.
In its current signed form, a negative count does not trigger
an error in ice driver but instead results in it being treated as 0.
This can lead to unexpected outcomes when processing messages.
By using u32, any invalid values will correctly trigger -EINVAL,
making error detection more robust.

Fixes: 1f7ea1cd6a374 ("ice: Enable FDIR Configure for AVF")
Reviewed-by: Jedrzej Jagielski 
Reviewed-by: Simon Horman 
Signed-off-by: Jan Glaza 
Signed-off-by: Martyna Szapar-Mudlaw 
---
 include/linux/avf/virtchnl.h | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/include/linux/avf/virtchnl.h b/include/linux/avf/virtchnl.h
index 4811b9a14604..cf0afa60e4a7 100644
--- a/include/linux/avf/virtchnl.h
+++ b/include/linux/avf/virtchnl.h
@@ -1343,7 +1343,7 @@ struct virtchnl_proto_hdrs {
 * 2 - from the second inner layer
 * 
 **/
-   int count; /* the proto layers must < VIRTCHNL_MAX_NUM_PROTO_HDRS */
+   u32 count; /* the proto layers must < VIRTCHNL_MAX_NUM_PROTO_HDRS */
union {
struct virtchnl_proto_hdr
proto_hdr[VIRTCHNL_MAX_NUM_PROTO_HDRS];
@@ -1395,7 +1395,7 @@ VIRTCHNL_CHECK_STRUCT_LEN(36, virtchnl_filter_action);
 
 struct virtchnl_filter_action_set {
/* action number must be less then VIRTCHNL_MAX_NUM_ACTIONS */
-   int count;
+   u32 count;
struct virtchnl_filter_action actions[VIRTCHNL_MAX_NUM_ACTIONS];
 };
 
-- 
2.47.0

[Intel-wired-lan] [iwl-net v3 0/5] ice: fix validation issues in virtchnl parameters

2025-03-04 Thread Martyna Szapar-Mudlaw 
This patch series addresses validation issues in the virtchnl interface
of the ice driver. These fixes correct improper value checking,
ensuring that the driver can properly handle and reject invalid inputs
from potentially malicious VFs. By fixing validation mechanisms,
these patches strictly enforce existing constraints to prevent
out-of-bounds scenarios, making the system more robust against incorrect
or unexpected data.

---

v3 -> v2:
removed redundant check and fixed kfree being called on uninitialized var in 5. 
patch

v2 -> v1:
attached Mateusz's related patch
rephrase some commit messages to indicate that this are fixes and should target 
net

--- 

Jan Glaza (3):
  virtchnl: make proto and filter action count unsigned
  ice: stop truncating queue ids when checking
  ice: validate queue quanta parameters to prevent OOB access

Lukasz Czapnik (1):
  ice: fix input validation for virtchnl BW

Mateusz Polchlopek (1):
  ice: fix using untrusted value of pkt_len in ice_vc_fdir_parse_raw()

 drivers/net/ethernet/intel/ice/ice_virtchnl.c | 39 +++
 .../ethernet/intel/ice/ice_virtchnl_fdir.c| 24 +++-
 include/linux/avf/virtchnl.h  |  4 +-
 3 files changed, 48 insertions(+), 19 deletions(-)

-- 
2.47.0

[Intel-wired-lan] [iwl-net v3 4/5] ice: fix input validation for virtchnl BW

2025-03-04 Thread Martyna Szapar-Mudlaw 
From: Lukasz Czapnik 

Add missing validation of tc and queue id values sent by a VF in
ice_vc_cfg_q_bw().
Additionally fixed logged value in the warning message,
where max_tx_rate was incorrectly referenced instead of min_tx_rate.
Also correct error handling in this function by properly exiting
when invalid configuration is detected.

Fixes: 015307754a19 ("ice: Support VF queue rate limit and quanta size 
configuration")
Reviewed-by: Jedrzej Jagielski 
Reviewed-by: Simon Horman 
Signed-off-by: Lukasz Czapnik 
Co-developed-by: Martyna Szapar-Mudlaw 
Signed-off-by: Martyna Szapar-Mudlaw 
---
 drivers/net/ethernet/intel/ice/ice_virtchnl.c | 24 ---
 1 file changed, 21 insertions(+), 3 deletions(-)

diff --git a/drivers/net/ethernet/intel/ice/ice_virtchnl.c 
b/drivers/net/ethernet/intel/ice/ice_virtchnl.c
index adb1bf12542f..824ef849b0ea 100644
--- a/drivers/net/ethernet/intel/ice/ice_virtchnl.c
+++ b/drivers/net/ethernet/intel/ice/ice_virtchnl.c
@@ -1865,15 +1865,33 @@ static int ice_vc_cfg_q_bw(struct ice_vf *vf, u8 *msg)
 
for (i = 0; i < qbw->num_queues; i++) {
if (qbw->cfg[i].shaper.peak != 0 && vf->max_tx_rate != 0 &&
-   qbw->cfg[i].shaper.peak > vf->max_tx_rate)
+   qbw->cfg[i].shaper.peak > vf->max_tx_rate) {
dev_warn(ice_pf_to_dev(vf->pf), "The maximum queue %d 
rate limit configuration may not take effect because the maximum TX rate for 
VF-%d is %d\n",
 qbw->cfg[i].queue_id, vf->vf_id,
 vf->max_tx_rate);
+   v_ret = VIRTCHNL_STATUS_ERR_PARAM;
+   goto err;
+   }
if (qbw->cfg[i].shaper.committed != 0 && vf->min_tx_rate != 0 &&
-   qbw->cfg[i].shaper.committed < vf->min_tx_rate)
+   qbw->cfg[i].shaper.committed < vf->min_tx_rate) {
dev_warn(ice_pf_to_dev(vf->pf), "The minimum queue %d 
rate limit configuration may not take effect because the minimum TX rate for 
VF-%d is %d\n",
 qbw->cfg[i].queue_id, vf->vf_id,
-vf->max_tx_rate);
+vf->min_tx_rate);
+   v_ret = VIRTCHNL_STATUS_ERR_PARAM;
+   goto err;
+   }
+   if (qbw->cfg[i].queue_id > vf->num_vf_qs) {
+   dev_warn(ice_pf_to_dev(vf->pf), "VF-%d trying to 
configure invalid queue_id\n",
+vf->vf_id);
+   v_ret = VIRTCHNL_STATUS_ERR_PARAM;
+   goto err;
+   }
+   if (qbw->cfg[i].tc >= ICE_MAX_TRAFFIC_CLASS) {
+   dev_warn(ice_pf_to_dev(vf->pf), "VF-%d trying to 
configure a traffic class higher than allowed\n",
+vf->vf_id);
+   v_ret = VIRTCHNL_STATUS_ERR_PARAM;
+   goto err;
+   }
}
 
for (i = 0; i < qbw->num_queues; i++) {
-- 
2.47.0

Re: [Intel-wired-lan] [iwl-net v3 1/5] virtchnl: make proto and filter action count unsigned

2025-03-04 Thread Paul Menzel 

Dear Jan, dear Martina,


Thank you for the patch.

Am 04.03.25 um 12:08 schrieb Martyna Szapar-Mudlaw:

From: Jan Glaza 

The count field in virtchnl_proto_hdrs and virtchnl_filter_action_set
should never be negative while still being valid. Changing it from
int to u32 ensures proper handling of values in virtchnl messages in
driverrs and prevents unintended behavior.
In its current signed form, a negative count does not trigger
an error in ice driver but instead results in it being treated as 0.
This can lead to unexpected outcomes when processing messages.
By using u32, any invalid values will correctly trigger -EINVAL,
making error detection more robust.

Fixes: 1f7ea1cd6a374 ("ice: Enable FDIR Configure for AVF")
Reviewed-by: Jedrzej Jagielski 
Reviewed-by: Simon Horman 
Signed-off-by: Jan Glaza 
Signed-off-by: Martyna Szapar-Mudlaw 
---
  include/linux/avf/virtchnl.h | 4 ++--
  1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/include/linux/avf/virtchnl.h b/include/linux/avf/virtchnl.h
index 4811b9a14604..cf0afa60e4a7 100644
--- a/include/linux/avf/virtchnl.h
+++ b/include/linux/avf/virtchnl.h
@@ -1343,7 +1343,7 @@ struct virtchnl_proto_hdrs {
 * 2 - from the second inner layer
 * 
 **/
-   int count; /* the proto layers must < VIRTCHNL_MAX_NUM_PROTO_HDRS */
+   u32 count; /* the proto layers must < VIRTCHNL_MAX_NUM_PROTO_HDRS */


Why limit the length, and not use unsigned int?


union {
struct virtchnl_proto_hdr
proto_hdr[VIRTCHNL_MAX_NUM_PROTO_HDRS];
@@ -1395,7 +1395,7 @@ VIRTCHNL_CHECK_STRUCT_LEN(36, virtchnl_filter_action);
  
  struct virtchnl_filter_action_set {

/* action number must be less then VIRTCHNL_MAX_NUM_ACTIONS */
-   int count;
+   u32 count;
struct virtchnl_filter_action actions[VIRTCHNL_MAX_NUM_ACTIONS];
  };



Kind regards,

Paul

[Intel-wired-lan] [iwl-net v3 5/5] ice: fix using untrusted value of pkt_len in ice_vc_fdir_parse_raw()

2025-03-04 Thread Martyna Szapar-Mudlaw 
From: Mateusz Polchlopek 

Fix using the untrusted value of proto->raw.pkt_len in function
ice_vc_fdir_parse_raw() by verifying if it does not exceed the
VIRTCHNL_MAX_SIZE_RAW_PACKET value.

Fixes: 99f419df8a5c ("ice: enable FDIR filters from raw binary patterns for 
VFs")
Reviewed-by: Przemek Kitszel 
Signed-off-by: Mateusz Polchlopek 
Signed-off-by: Martyna Szapar-Mudlaw 
---
 .../ethernet/intel/ice/ice_virtchnl_fdir.c| 24 ---
 1 file changed, 15 insertions(+), 9 deletions(-)

diff --git a/drivers/net/ethernet/intel/ice/ice_virtchnl_fdir.c 
b/drivers/net/ethernet/intel/ice/ice_virtchnl_fdir.c
index 14e3f0f89c78..9be4bd717512 100644
--- a/drivers/net/ethernet/intel/ice/ice_virtchnl_fdir.c
+++ b/drivers/net/ethernet/intel/ice/ice_virtchnl_fdir.c
@@ -832,21 +832,27 @@ ice_vc_fdir_parse_raw(struct ice_vf *vf,
  struct virtchnl_proto_hdrs *proto,
  struct virtchnl_fdir_fltr_conf *conf)
 {
-   u8 *pkt_buf, *msk_buf __free(kfree);
+   u8 *pkt_buf, *msk_buf __free(kfree) = NULL;
struct ice_parser_result rslt;
struct ice_pf *pf = vf->pf;
+   u16 pkt_len, udp_port = 0;
struct ice_parser *psr;
int status = -ENOMEM;
struct ice_hw *hw;
-   u16 udp_port = 0;
 
-   pkt_buf = kzalloc(proto->raw.pkt_len, GFP_KERNEL);
-   msk_buf = kzalloc(proto->raw.pkt_len, GFP_KERNEL);
+   pkt_len = proto->raw.pkt_len;
+
+   if (!pkt_len || pkt_len > VIRTCHNL_MAX_SIZE_RAW_PACKET)
+   return -EINVAL;
+
+   pkt_buf = kzalloc(pkt_len, GFP_KERNEL);
+   msk_buf = kzalloc(pkt_len, GFP_KERNEL);
+
if (!pkt_buf || !msk_buf)
goto err_mem_alloc;
 
-   memcpy(pkt_buf, proto->raw.spec, proto->raw.pkt_len);
-   memcpy(msk_buf, proto->raw.mask, proto->raw.pkt_len);
+   memcpy(pkt_buf, proto->raw.spec, pkt_len);
+   memcpy(msk_buf, proto->raw.mask, pkt_len);
 
hw = &pf->hw;
 
@@ -862,7 +868,7 @@ ice_vc_fdir_parse_raw(struct ice_vf *vf,
if (ice_get_open_tunnel_port(hw, &udp_port, TNL_VXLAN))
ice_parser_vxlan_tunnel_set(psr, udp_port, true);
 
-   status = ice_parser_run(psr, pkt_buf, proto->raw.pkt_len, &rslt);
+   status = ice_parser_run(psr, pkt_buf, pkt_len, &rslt);
if (status)
goto err_parser_destroy;
 
@@ -876,7 +882,7 @@ ice_vc_fdir_parse_raw(struct ice_vf *vf,
}
 
status = ice_parser_profile_init(&rslt, pkt_buf, msk_buf,
-proto->raw.pkt_len, ICE_BLK_FD,
+pkt_len, ICE_BLK_FD,
 conf->prof);
if (status)
goto err_parser_profile_init;
@@ -885,7 +891,7 @@ ice_vc_fdir_parse_raw(struct ice_vf *vf,
ice_parser_profile_dump(hw, conf->prof);
 
/* Store raw flow info into @conf */
-   conf->pkt_len = proto->raw.pkt_len;
+   conf->pkt_len = pkt_len;
conf->pkt_buf = pkt_buf;
conf->parser_ena = true;
 
-- 
2.47.0

Re: [Intel-wired-lan] e1000e/I219-LM: Speed negotiation problems

2025-03-04 Thread Lifshits, Vitaly 




On 3/3/2025 5:30 PM, Paul Menzel wrote:

Dear Linux folks,


On a Dell OptiPlex 7000/0F1HC1, BIOS 1.8.2 12/08/2022 with

     00:1f.6 Ethernet controller [0200]: Intel Corporation Ethernet 
Connection (17) I219-LM [8086:1a1c] (rev 11)


and Dell OptiPlex 7071/097YXY, BIOS 1.1.2 08/29/2019 with

     00:1f.6 Ethernet controller [0200]: Intel Corporation Ethernet 
Connection (7) I219-LM [8086:15bb] (rev 10)


we recently observed that randomly auto-negotiation would only result in 
a speed of 100 Mb/s or once even 10 Mb/s. (Wake-on-LAN is enabled for 
the device, although it does not work for the Dell OptiPlex 7000.)


Today, the Dell OptiPlex 7071 booted and only came up with 10 Mb/s. Only 
after re-plugging the cable in both plugs (device and in the ground), 
and applying contact spray, it negotiated a speed of 1000 Gb/s. Then 
rebooting, it negotiated a speed 1000 Gb/s, but than a minute later, the 
link went down, and, 22 seconds later, it came back with 100 Mb/s.


     Mar 03 15:35:35 sighup.molgen.mpg.de kernel: Linux version 
6.12.11.mx64.479 (r...@lucy.molgen.mpg.de) (gcc (GCC) 12.3.0, GNU ld 
(GNU Binutils) 2.41) #1 SMP PREEMPT_DYNAMIC Fri Jan 24 13:30:47 CET 2025

     […]
     Mar 03 15:35:40 sighup.molgen.mpg.de kernel: e1000e :00:1f.6 
net00: NIC Link is Up 1000 Mbps Full Duplex, Flow Control: Rx/Tx

     […]
     Mar 03 15:37:31 sighup.molgen.mpg.de kernel: e1000e :00:1f.6 
net00: NIC Link is Down

     […]
     Mar 03 15:37:53 sighup.molgen.mpg.de kernel: e1000e :00:1f.6 
net00: NIC Link is Up 100 Mbps Full Duplex, Flow Control: Rx/Tx


Then I enabled dynamic debug messages

     echo "module e1000e +flmpt" | sudo tee 
/sys/kernel/debug/dynamic_debug/control


and tried to force negotiating a speed of 1 Gb/s:

     sudo ethtool -s net00 autoneg on speed 1000 duplex full

The Linux kernel messages are attached:

     2025-03-03T15:39:00+01:00 sighup  sudo:  pmenzel : TTY=pts/0 ; 
PWD=/home/pmenzel ; USER=root ; COMMAND=/usr/bin/tee 
/sys/kernel/debug/dynamic_debug/control

     […]
     2025-03-03T15:39:08+01:00 sighup  sudo:  pmenzel : TTY=pts/0 ; 
PWD=/home/pmenzel ; USER=root ; COMMAND=/usr/sbin/ethtool -s net00 
autoneg on speed 1000 duplex full

     […]
     2025-03-03T15:39:08.768806+01:00 sighup kernel: [  217.278630] 
[1168] e1000e:e1000_reset_hw_ich8lan:4778: e1000e :00:1f.6 net00: 
Masking off all interrupts

     […]
     2025-03-03T15:39:08.834765+01:00 sighup kernel: [  217.344955] 
[1168] e1000e:e1000e_setup_copper_link:1216: e1000e :00:1f.6 net00: 
Unable to establish link!!!

     […]
     2025-03-03T15:39:08.840771+01:00 sighup kernel: [  217.350769] 
[1168] e1000e:__e1000_write_phy_reg_hv:2963: e1000e :00:1f.6 net00: 
writing PHY page 769 (or 0x6020 shifted) reg 0x14


Later the link went down again with dynamic debug enabled. It came back 
22 seconds later:


     [ 2848.096035] [2031] e1000e:__e1000_read_phy_reg_hv:2839: e1000e 
:00:1f.6 net00: reading PHY page 0 (or 0x0 shifted) reg 0x1
     [ 2848.096121] [2031] e1000e:__e1000_read_phy_reg_hv:2839: e1000e 
:00:1f.6 net00: reading PHY page 0 (or 0x0 shifted) reg 0x1

     [ 2848.096200] e1000e :00:1f.6 net00: NIC Link is Down
     […]
     [ 2906.442227] [2031] e1000e:__e1000_read_phy_reg_hv:2839: e1000e 
:00:1f.6 net00: reading PHY page 0 (or 0x0 shifted) reg 0xf
     [ 2906.442299] [2031] 
e1000e:e1000e_get_speed_and_duplex_copper:1321: e1000e :00:1f.6 
net00: 1000 Mbps, Full Duplex
     [ 2906.442308] e1000e :00:1f.6 net00: NIC Link is Up 1000 Mbps 
Full Duplex, Flow Control: Rx/Tx


Can you deduce anything from the logs? Of course it could be floor 
connector or the cable. I don’t want to change too much in the setup 
though.



Kind regards,

Paul


PS: Output of `ethtool`:

```
@sighup:~$ sudo ethtool net00
Settings for net00:
 Supported ports: [ TP ]
 Supported link modes:   10baseT/Half 10baseT/Full
     100baseT/Half 100baseT/Full
     1000baseT/Full
 Supported pause frame use: Symmetric Receive-only
 Supports auto-negotiation: Yes
 Supported FEC modes: Not reported
 Advertised link modes:  1000baseT/Full
 Advertised pause frame use: Symmetric Receive-only
 Advertised auto-negotiation: Yes
 Advertised FEC modes: Not reported
 Link partner advertised link modes:  10baseT/Half 10baseT/Full
  100baseT/Half 100baseT/Full
  1000baseT/Half 1000baseT/Full
 Link partner advertised pause frame use: Symmetric Receive-only
 Link partner advertised auto-negotiation: Yes
 Link partner advertised FEC modes: Not reported
 Speed: 1000Mb/s
 Duplex: Full
 Auto-negotiation: on
 Port: Twisted Pair
 PHYAD: 1
 Transceiver: internal
 MDI-X: on (auto)
 Supports Wake-on: pumbg
 Wake-on: g
     Current message level: 0x0007 (7)
    drv prob

Re: [Intel-wired-lan] [PATCH iwl-next v7 1/9] net: ethtool: mm: extract stmmac verification logic into common library

2025-03-04 Thread Vladimir Oltean 
On Mon, Mar 03, 2025 at 05:26:50AM -0500, Faizal Rahim wrote:
> +/**
> + * ethtool_mmsv_link_state_handle() - Inform MAC Merge Software Verification
> + * of link state changes
> + * @mmsv: MAC Merge Software Verification state
> + * @up: True if device carrier is up and able to pass verification packets
> + *
> + * Calling context is expected to be from a task, interrupts enabled.
> + */
> +void ethtool_mmsv_link_state_handle(struct ethtool_mmsv *mmsv, bool up)
> +{
> + unsigned long flags;
> +
> + ethtool_mmsv_stop(mmsv);
> +
> + spin_lock_irqsave(&mmsv->lock, flags);
> +
> + if (up && mmsv->pmac_enabled) {
> + /* VERIFY process requires pMAC enabled when NIC comes up */
> + ethtool_mmsv_configure_pmac(mmsv, true);
> +
> + /* New link => maybe new partner => new verification process */
> + ethtool_mmsv_apply(mmsv);
> + } else {
> + /* Reset the reported verification state while the link is down 
> */
> + if (mmsv->verify_enabled)
> + mmsv->status = ETHTOOL_MM_VERIFY_STATUS_INITIAL;

As requested in v5, please make this a separate patch. It represents a
functional change for stmmac, and I don't want somebody who bisects it
to find a code movement commit and search for a needle through a haystack.

> +
> + /* No link or pMAC not enabled */
> + ethtool_mmsv_configure_pmac(mmsv, false);
> + ethtool_mmsv_configure_tx(mmsv, false);
> + }
> +
> + spin_unlock_irqrestore(&mmsv->lock, flags);
> +}
> +EXPORT_SYMBOL_GPL(ethtool_mmsv_link_state_handle);
> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_ethtool.c 
> b/drivers/net/ethernet/stmicro/stmmac/stmmac_ethtool.c
> index 918a32f8fda8..25533d6a3175 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_ethtool.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_ethtool.c
> @@ -1210,37 +1210,17 @@ static int stmmac_get_mm(struct net_device *ndev,
>struct ethtool_mm_state *state)
>  {
>   struct stmmac_priv *priv = netdev_priv(ndev);
> - unsigned long flags;
>   u32 frag_size;
>  
>   if (!stmmac_fpe_supported(priv))
>   return -EOPNOTSUPP;
>  
> - spin_lock_irqsave(&priv->fpe_cfg.lock, flags);
> + ethtool_mmsv_get_mm(&priv->fpe_cfg.mmsv, state);
>  
> - state->max_verify_time = STMMAC_FPE_MM_MAX_VERIFY_TIME_MS;
> - state->verify_enabled = priv->fpe_cfg.verify_enabled;
> - state->pmac_enabled = priv->fpe_cfg.pmac_enabled;
> - state->verify_time = priv->fpe_cfg.verify_time;
> - state->tx_enabled = priv->fpe_cfg.tx_enabled;
> - state->verify_status = priv->fpe_cfg.status;
>   state->rx_min_frag_size = ETH_ZLEN;
> -
> - /* FPE active if common tx_enabled and
> -  * (verification success or disabled(forced))
> -  */
> - if (state->tx_enabled &&
> - (state->verify_status == ETHTOOL_MM_VERIFY_STATUS_SUCCEEDED ||
> -  state->verify_status == ETHTOOL_MM_VERIFY_STATUS_DISABLED))
> - state->tx_active = true;
> - else
> - state->tx_active = false;
> -
>   frag_size = stmmac_fpe_get_add_frag_size(priv);
>   state->tx_min_frag_size = ethtool_mm_frag_size_add_to_min(frag_size);
>  
> - spin_unlock_irqrestore(&priv->fpe_cfg.lock, flags);
> -
>   return 0;
>  }
>  
> @@ -1248,8 +1228,6 @@ static int stmmac_set_mm(struct net_device *ndev, 
> struct ethtool_mm_cfg *cfg,
>struct netlink_ext_ack *extack)
>  {
>   struct stmmac_priv *priv = netdev_priv(ndev);
> - struct stmmac_fpe_cfg *fpe_cfg = &priv->fpe_cfg;
> - unsigned long flags;
>   u32 frag_size;
>   int err;
>  
> @@ -1258,23 +1236,8 @@ static int stmmac_set_mm(struct net_device *ndev, 
> struct ethtool_mm_cfg *cfg,
>   if (err)
>   return err;
>  
> - /* Wait for the verification that's currently in progress to finish */
> - timer_shutdown_sync(&fpe_cfg->verify_timer);
> -
> - spin_lock_irqsave(&fpe_cfg->lock, flags);
> -
> - fpe_cfg->verify_enabled = cfg->verify_enabled;
> - fpe_cfg->pmac_enabled = cfg->pmac_enabled;
> - fpe_cfg->verify_time = cfg->verify_time;
> - fpe_cfg->tx_enabled = cfg->tx_enabled;
> -
> - if (!cfg->verify_enabled)
> - fpe_cfg->status = ETHTOOL_MM_VERIFY_STATUS_DISABLED;
> -
>   stmmac_fpe_set_add_frag_size(priv, frag_size);

This is another change in behavior which unfortunately I am noticing
only now: stmmac_fpe_set_add_frag_size() and stmmac_fpe_get_add_frag_size()
used to execute under &fpe_cfg->lock, and now run outside of what
eventually became the mmsv->lock (but still under rtnl_lock() protection).

I am not seeing a need for the additional fragment size to be covered
by the mmsv->lock (the mmsv has no business with this parameter), and
rtnl_lock() should be sufficient to serialize stmmac_get_mm() with
stmmac_set_mm(). So I guess I

Re: [Intel-wired-lan] [PATCH] e1000e: Link flap workaround option for false IRP events

2025-03-04 Thread Andrew Lunn 
> > I suggest to try replacing the register read for a short delay or 
> > reading the PHY STATUS register instead.
> >
> 
> Ack - we'll try that, and collect some other debug registers in the process.
> Will update with findings - this may take a while :)

Please be careful with which register you choice. Because the link
status bit in BMSR is latching, you should not be reading it and
discarding the result.

Reading register 2 or 3 should be totally safe.

Another thing to keep in mind, you cannot unconditionally read a paged
register in this particular PHY, because the e1000e is used with a
number of different PHYs. That register does not exist in other PHYs,
and the action of selecting the page performs a register write, which
for some other PHY could be destructive. So i would suggest you keep
to registers defined in 802.3 C22.

Andrew

Re: [Intel-wired-lan] [PATCH iwl-next v1 3/3] ice: enable timesync operation on 2xNAC E825 devices

2025-03-04 Thread Nitka, Grzegorz 
> -Original Message-
> From: Simon Horman 
> Sent: Tuesday, February 25, 2025 4:05 PM
> To: Nitka, Grzegorz 
> Cc: intel-wired-...@lists.osuosl.org; net...@vger.kernel.org; Kolacinski,
> Karol ; Kitszel, Przemyslaw
> 
> Subject: Re: [PATCH iwl-next v1 3/3] ice: enable timesync operation on
> 2xNAC E825 devices
> 
> On Fri, Feb 21, 2025 at 01:31:23PM +0100, Grzegorz Nitka wrote:
> > From: Karol Kolacinski 
> >
> > According to the E825C specification, SBQ address for ports on a single
> > complex is device 2 for PHY 0 and device 13 for PHY1.
> > For accessing ports on a dual complex E825C (so called 2xNAC mode),
> > the driver should use destination device 2 (referred as phy_0) for
> > the current complex PHY and device 13 (referred as phy_0_peer) for
> > peer complex PHY.
> >
> > Differentiate SBQ destination device by checking if current PF port
> > number is on the same PHY as target port number.
> >
> > Adjust 'ice_get_lane_number' function to provide unique port number for
> > ports from PHY1 in 'dual' mode config (by adding fixed offset for PHY1
> > ports). Cache this value in ice_hw struct.
> >
> > Introduce ice_get_primary_hw wrapper to get access to timesync register
> > not available from second NAC.
> >
> > Reviewed-by: Przemek Kitszel 
> > Signed-off-by: Karol Kolacinski 
> > Co-developed-by: Grzegorz Nitka 
> > Signed-off-by: Grzegorz Nitka 
> > ---
> >  drivers/net/ethernet/intel/ice/ice.h| 60 -
> >  drivers/net/ethernet/intel/ice/ice_common.c |  6 ++-
> >  drivers/net/ethernet/intel/ice/ice_ptp.c| 49 -
> >  drivers/net/ethernet/intel/ice/ice_ptp_hw.c | 39 +++---
> >  drivers/net/ethernet/intel/ice/ice_ptp_hw.h |  5 --
> >  drivers/net/ethernet/intel/ice/ice_type.h   |  1 +
> >  6 files changed, 134 insertions(+), 26 deletions(-)
> >
> > diff --git a/drivers/net/ethernet/intel/ice/ice.h
> b/drivers/net/ethernet/intel/ice/ice.h
> > index 53b990e2e850..d80ab48402f1 100644
> > --- a/drivers/net/ethernet/intel/ice/ice.h
> > +++ b/drivers/net/ethernet/intel/ice/ice.h
> > @@ -193,8 +193,6 @@
> >
> >  #define ice_pf_to_dev(pf) (&((pf)->pdev->dev))
> >
> > -#define ice_pf_src_tmr_owned(pf) ((pf)-
> >hw.func_caps.ts_func_info.src_tmr_owned)
> > -
> >  enum ice_feature {
> > ICE_F_DSCP,
> > ICE_F_PHY_RCLK,
> > @@ -1049,4 +1047,62 @@ static inline void ice_clear_rdma_cap(struct
> ice_pf *pf)
> >  }
> >
> >  extern const struct xdp_metadata_ops ice_xdp_md_ops;
> > +
> > +/**
> > + * ice_is_dual - Check if given config is multi-NAC
> > + * @hw: pointer to HW structure
> > + *
> > + * Return: true if the device is running in mutli-NAC (Network
> > + * Acceleration Complex) configuration variant, false otherwise
> > + * (always false for non-E825 devices).
> > + */
> > +static inline bool ice_is_dual(struct ice_hw *hw)
> > +{
> > +   return hw->mac_type == ICE_MAC_GENERIC_3K_E825 &&
> > +  (hw->dev_caps.nac_topo.mode & ICE_NAC_TOPO_DUAL_M);
> > +}
> 
> Thanks for the complete Kernel doc, and not adding unnecessary () around
> the value returned. Overall these helpers seem nice and clean to me.
> 
> ...
> 
> > diff --git a/drivers/net/ethernet/intel/ice/ice_common.c
> b/drivers/net/ethernet/intel/ice/ice_common.c
> > index ed7ef8608cbb..4ff4c99d0872 100644
> > --- a/drivers/net/ethernet/intel/ice/ice_common.c
> > +++ b/drivers/net/ethernet/intel/ice/ice_common.c
> > @@ -1135,6 +1135,8 @@ int ice_init_hw(struct ice_hw *hw)
> > }
> > }
> >
> > +   hw->lane_num = ice_get_phy_lane_number(hw);
> > +
> 
> Unfortunately there seems to be a bit of a problem here:
> 
> The type of hw->lane number is u8.
> But ice_get_phy_lane_number returns an int,
> which may be a negative error value...
> 
> ...
> 
> > diff --git a/drivers/net/ethernet/intel/ice/ice_ptp.c
> b/drivers/net/ethernet/intel/ice/ice_ptp.c
> 
> ...

Hello Simon,

Thanks for your review and apologize for a late response (I was OOO last
week).
Yes, this is actually a good catch!
I have no idea how it happened, most likely my typo when backporting
the patch from our reference code. hw->lane_num is declared as 's8' there.
To be fixed in v2 (along with doc update from your other comment).

> 
> > @@ -3177,17 +3203,16 @@ void ice_ptp_init(struct ice_pf *pf)
> >  {
> > struct ice_ptp *ptp = &pf->ptp;
> > struct ice_hw *hw = &pf->hw;
> > -   int lane_num, err;
> > +   int err;
> >
> > ptp->state = ICE_PTP_INITIALIZING;
> >
> > -   lane_num = ice_get_phy_lane_number(hw);
> > -   if (lane_num < 0) {
> > -   err = lane_num;
> > +   if (hw->lane_num < 0) {
> 
> ... which is checked here.
> 
> But because hw->lane_num is unsigned, this condition is always false.
> 
> FWIIW, I think it is nice that that hw->lane is a u8.
> But error handling seems broken here.
> 
> > +   err = hw->lane_num;
> > goto err_exit;
> > }
> > +   ptp->port.port_num = hw->lane_num;
> >
> > -   ptp->port.port_num = (u8)lane_num;
> > ice_ptp_init_hw(hw);