[PATCH net-next 0/2] enic: Set link speed only after link up

[PATCH net-next 0/2] enic: Set link speed only after link up

[John Daley]

This is a scaled down patch set that only contains the independent
link speed fix which was part of the patch set titled:
    enic: Use Page Pool API for receiving packets

John Daley (2):
  enic: Move RX coalescing set function
  enic: Obtain the Link speed only after the link comes up

 drivers/net/ethernet/cisco/enic/enic_main.c | 64 ++++++++++-----------
 1 file changed, 32 insertions(+), 32 deletions(-)

-- 
2.35.2

[PATCH net-next 1/2] enic: Move RX coalescing set function

[John Daley]

Move the function used for setting the RX coalescing range to before
the function that checks the link status. It needs to be called from
there instead of from the probe function.

There is no functional change.

Co-developed-by: Nelson Escobar <neescoba@cisco.com>
Signed-off-by: Nelson Escobar <neescoba@cisco.com>
Co-developed-by: Satish Kharat <satishkh@cisco.com>
Signed-off-by: Satish Kharat <satishkh@cisco.com>
Signed-off-by: John Daley <johndale@cisco.com>
---
 drivers/net/ethernet/cisco/enic/enic_main.c | 60 ++++++++++-----------
 1 file changed, 30 insertions(+), 30 deletions(-)

diff --git a/drivers/net/ethernet/cisco/enic/enic_main.c b/drivers/net/ethernet/cisco/enic/enic_main.c
index 9913952ccb42..957efe73e41a 100644
--- a/drivers/net/ethernet/cisco/enic/enic_main.c
+++ b/drivers/net/ethernet/cisco/enic/enic_main.c
@@ -428,6 +428,36 @@ static void enic_mtu_check(struct enic *enic)
 	}
 }
 
+static void enic_set_rx_coal_setting(struct enic *enic)
+{
+	unsigned int speed;
+	int index = -1;
+	struct enic_rx_coal *rx_coal = &enic->rx_coalesce_setting;
+
+	/* 1. Read the link speed from fw
+	 * 2. Pick the default range for the speed
+	 * 3. Update it in enic->rx_coalesce_setting
+	 */
+	speed = vnic_dev_port_speed(enic->vdev);
+	if (speed > ENIC_LINK_SPEED_10G)
+		index = ENIC_LINK_40G_INDEX;
+	else if (speed > ENIC_LINK_SPEED_4G)
+		index = ENIC_LINK_10G_INDEX;
+	else
+		index = ENIC_LINK_4G_INDEX;
+
+	rx_coal->small_pkt_range_start = mod_range[index].small_pkt_range_start;
+	rx_coal->large_pkt_range_start = mod_range[index].large_pkt_range_start;
+	rx_coal->range_end = ENIC_RX_COALESCE_RANGE_END;
+
+	/* Start with the value provided by UCSM */
+	for (index = 0; index < enic->rq_count; index++)
+		enic->cq[index].cur_rx_coal_timeval =
+				enic->config.intr_timer_usec;
+
+	rx_coal->use_adaptive_rx_coalesce = 1;
+}
+
 static void enic_link_check(struct enic *enic)
 {
 	int link_status = vnic_dev_link_status(enic->vdev);
@@ -1901,36 +1931,6 @@ static void enic_synchronize_irqs(struct enic *enic)
 	}
 }
 
-static void enic_set_rx_coal_setting(struct enic *enic)
-{
-	unsigned int speed;
-	int index = -1;
-	struct enic_rx_coal *rx_coal = &enic->rx_coalesce_setting;
-
-	/* 1. Read the link speed from fw
-	 * 2. Pick the default range for the speed
-	 * 3. Update it in enic->rx_coalesce_setting
-	 */
-	speed = vnic_dev_port_speed(enic->vdev);
-	if (ENIC_LINK_SPEED_10G < speed)
-		index = ENIC_LINK_40G_INDEX;
-	else if (ENIC_LINK_SPEED_4G < speed)
-		index = ENIC_LINK_10G_INDEX;
-	else
-		index = ENIC_LINK_4G_INDEX;
-
-	rx_coal->small_pkt_range_start = mod_range[index].small_pkt_range_start;
-	rx_coal->large_pkt_range_start = mod_range[index].large_pkt_range_start;
-	rx_coal->range_end = ENIC_RX_COALESCE_RANGE_END;
-
-	/* Start with the value provided by UCSM */
-	for (index = 0; index < enic->rq_count; index++)
-		enic->cq[index].cur_rx_coal_timeval =
-				enic->config.intr_timer_usec;
-
-	rx_coal->use_adaptive_rx_coalesce = 1;
-}
-
 static int enic_dev_notify_set(struct enic *enic)
 {
 	int err;
-- 
2.35.2

[PATCH net-next 2/2] enic: Obtain the Link speed only after the link comes up

[John Daley]

The link speed is obtained in the RX adaptive coalescing function. It
was being called at probe time when the link may not be up. Change the
call to run after the Link comes up.

The impact of not getting the correct link speed was that the low end of
the adaptive interrupt range was always being set to 0 which could have
caused a slight increase in the number of RX interrupts.

Co-developed-by: Nelson Escobar <neescoba@cisco.com>
Signed-off-by: Nelson Escobar <neescoba@cisco.com>
Co-developed-by: Satish Kharat <satishkh@cisco.com>
Signed-off-by: Satish Kharat <satishkh@cisco.com>
Signed-off-by: John Daley <johndale@cisco.com>
---
 drivers/net/ethernet/cisco/enic/enic_main.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/cisco/enic/enic_main.c b/drivers/net/ethernet/cisco/enic/enic_main.c
index 957efe73e41a..49f6cab01ed5 100644
--- a/drivers/net/ethernet/cisco/enic/enic_main.c
+++ b/drivers/net/ethernet/cisco/enic/enic_main.c
@@ -109,7 +109,7 @@ static struct enic_intr_mod_table mod_table[ENIC_MAX_COALESCE_TIMERS + 1] = {
 static struct enic_intr_mod_range mod_range[ENIC_MAX_LINK_SPEEDS] = {
 	{0,  0}, /* 0  - 4  Gbps */
 	{0,  3}, /* 4  - 10 Gbps */
-	{3,  6}, /* 10 - 40 Gbps */
+	{3,  6}, /* 10+ Gbps */
 };
 
 static void enic_init_affinity_hint(struct enic *enic)
@@ -466,6 +466,7 @@ static void enic_link_check(struct enic *enic)
 	if (link_status && !carrier_ok) {
 		netdev_info(enic->netdev, "Link UP\n");
 		netif_carrier_on(enic->netdev);
+		enic_set_rx_coal_setting(enic);
 	} else if (!link_status && carrier_ok) {
 		netdev_info(enic->netdev, "Link DOWN\n");
 		netif_carrier_off(enic->netdev);
@@ -3063,7 +3064,6 @@ static int enic_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
 	timer_setup(&enic->notify_timer, enic_notify_timer, 0);
 
 	enic_rfs_flw_tbl_init(enic);
-	enic_set_rx_coal_setting(enic);
 	INIT_WORK(&enic->reset, enic_reset);
 	INIT_WORK(&enic->tx_hang_reset, enic_tx_hang_reset);
 	INIT_WORK(&enic->change_mtu_work, enic_change_mtu_work);
-- 
2.35.2

Re: [PATCH net-next 1/2] enic: Move RX coalescing set function

[Michal Swiatkowski]

On Mon, Jan 06, 2025 at 06:51:34PM -0800, John Daley wrote:
> Move the function used for setting the RX coalescing range to before
> the function that checks the link status. It needs to be called from
> there instead of from the probe function.
> 
> There is no functional change.
> 
> Co-developed-by: Nelson Escobar <neescoba@cisco.com>
> Signed-off-by: Nelson Escobar <neescoba@cisco.com>
> Co-developed-by: Satish Kharat <satishkh@cisco.com>
> Signed-off-by: Satish Kharat <satishkh@cisco.com>
> Signed-off-by: John Daley <johndale@cisco.com>
> ---
>  drivers/net/ethernet/cisco/enic/enic_main.c | 60 ++++++++++-----------
>  1 file changed, 30 insertions(+), 30 deletions(-)
> 
> diff --git a/drivers/net/ethernet/cisco/enic/enic_main.c b/drivers/net/ethernet/cisco/enic/enic_main.c
> index 9913952ccb42..957efe73e41a 100644
> --- a/drivers/net/ethernet/cisco/enic/enic_main.c
> +++ b/drivers/net/ethernet/cisco/enic/enic_main.c
> @@ -428,6 +428,36 @@ static void enic_mtu_check(struct enic *enic)
>  	}
>  }
>  
> +static void enic_set_rx_coal_setting(struct enic *enic)
> +{
> +	unsigned int speed;
> +	int index = -1;
> +	struct enic_rx_coal *rx_coal = &enic->rx_coalesce_setting;
> +
> +	/* 1. Read the link speed from fw
> +	 * 2. Pick the default range for the speed
> +	 * 3. Update it in enic->rx_coalesce_setting
> +	 */
> +	speed = vnic_dev_port_speed(enic->vdev);
> +	if (speed > ENIC_LINK_SPEED_10G)
> +		index = ENIC_LINK_40G_INDEX;
> +	else if (speed > ENIC_LINK_SPEED_4G)
> +		index = ENIC_LINK_10G_INDEX;
> +	else
> +		index = ENIC_LINK_4G_INDEX;
> +
> +	rx_coal->small_pkt_range_start = mod_range[index].small_pkt_range_start;
> +	rx_coal->large_pkt_range_start = mod_range[index].large_pkt_range_start;
> +	rx_coal->range_end = ENIC_RX_COALESCE_RANGE_END;
> +
> +	/* Start with the value provided by UCSM */
> +	for (index = 0; index < enic->rq_count; index++)
> +		enic->cq[index].cur_rx_coal_timeval =
> +				enic->config.intr_timer_usec;
> +
> +	rx_coal->use_adaptive_rx_coalesce = 1;
> +}
> +
>  static void enic_link_check(struct enic *enic)
>  {
>  	int link_status = vnic_dev_link_status(enic->vdev);
> @@ -1901,36 +1931,6 @@ static void enic_synchronize_irqs(struct enic *enic)
>  	}
>  }
>  
> -static void enic_set_rx_coal_setting(struct enic *enic)
> -{
> -	unsigned int speed;
> -	int index = -1;
> -	struct enic_rx_coal *rx_coal = &enic->rx_coalesce_setting;
> -
> -	/* 1. Read the link speed from fw
> -	 * 2. Pick the default range for the speed
> -	 * 3. Update it in enic->rx_coalesce_setting
> -	 */
> -	speed = vnic_dev_port_speed(enic->vdev);
> -	if (ENIC_LINK_SPEED_10G < speed)
> -		index = ENIC_LINK_40G_INDEX;
> -	else if (ENIC_LINK_SPEED_4G < speed)
> -		index = ENIC_LINK_10G_INDEX;
> -	else
> -		index = ENIC_LINK_4G_INDEX;
> -
> -	rx_coal->small_pkt_range_start = mod_range[index].small_pkt_range_start;
> -	rx_coal->large_pkt_range_start = mod_range[index].large_pkt_range_start;
> -	rx_coal->range_end = ENIC_RX_COALESCE_RANGE_END;
> -
> -	/* Start with the value provided by UCSM */
> -	for (index = 0; index < enic->rq_count; index++)
> -		enic->cq[index].cur_rx_coal_timeval =
> -				enic->config.intr_timer_usec;
> -
> -	rx_coal->use_adaptive_rx_coalesce = 1;
> -}
> -
>  static int enic_dev_notify_set(struct enic *enic)
>  {
>  	int err;
> -- 
> 2.35.2

Reviewed-by: Michal Swiatkowski <michal.swiatkowski@linux.intel.com>

Re: [PATCH net-next 2/2] enic: Obtain the Link speed only after the link comes up

[Michal Swiatkowski]

On Mon, Jan 06, 2025 at 06:51:35PM -0800, John Daley wrote:
> The link speed is obtained in the RX adaptive coalescing function. It
> was being called at probe time when the link may not be up. Change the
> call to run after the Link comes up.
> 
> The impact of not getting the correct link speed was that the low end of
> the adaptive interrupt range was always being set to 0 which could have
> caused a slight increase in the number of RX interrupts.
> 
> Co-developed-by: Nelson Escobar <neescoba@cisco.com>
> Signed-off-by: Nelson Escobar <neescoba@cisco.com>
> Co-developed-by: Satish Kharat <satishkh@cisco.com>
> Signed-off-by: Satish Kharat <satishkh@cisco.com>
> Signed-off-by: John Daley <johndale@cisco.com>
> ---
>  drivers/net/ethernet/cisco/enic/enic_main.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/ethernet/cisco/enic/enic_main.c b/drivers/net/ethernet/cisco/enic/enic_main.c
> index 957efe73e41a..49f6cab01ed5 100644
> --- a/drivers/net/ethernet/cisco/enic/enic_main.c
> +++ b/drivers/net/ethernet/cisco/enic/enic_main.c
> @@ -109,7 +109,7 @@ static struct enic_intr_mod_table mod_table[ENIC_MAX_COALESCE_TIMERS + 1] = {
>  static struct enic_intr_mod_range mod_range[ENIC_MAX_LINK_SPEEDS] = {
>  	{0,  0}, /* 0  - 4  Gbps */
>  	{0,  3}, /* 4  - 10 Gbps */
> -	{3,  6}, /* 10 - 40 Gbps */
> +	{3,  6}, /* 10+ Gbps */

Is this on purpose? You didn't mention anything about speed range in
commit message. Just wondering, patch looks fine, thanks.

Reviewed-by: Michal Swiatkowski <michal.swiatkowski@linux.intel.com>

>  };
>  
>  static void enic_init_affinity_hint(struct enic *enic)
> @@ -466,6 +466,7 @@ static void enic_link_check(struct enic *enic)
>  	if (link_status && !carrier_ok) {
>  		netdev_info(enic->netdev, "Link UP\n");
>  		netif_carrier_on(enic->netdev);
> +		enic_set_rx_coal_setting(enic);
>  	} else if (!link_status && carrier_ok) {
>  		netdev_info(enic->netdev, "Link DOWN\n");
>  		netif_carrier_off(enic->netdev);
> @@ -3063,7 +3064,6 @@ static int enic_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
>  	timer_setup(&enic->notify_timer, enic_notify_timer, 0);
>  
>  	enic_rfs_flw_tbl_init(enic);
> -	enic_set_rx_coal_setting(enic);
>  	INIT_WORK(&enic->reset, enic_reset);
>  	INIT_WORK(&enic->tx_hang_reset, enic_tx_hang_reset);
>  	INIT_WORK(&enic->change_mtu_work, enic_change_mtu_work);
> -- 
> 2.35.2

Re: [PATCH net-next 1/2] enic: Move RX coalescing set function

[Andrew Lunn]

On Mon, Jan 06, 2025 at 06:51:34PM -0800, John Daley wrote:
> Move the function used for setting the RX coalescing range to before
> the function that checks the link status. It needs to be called from
> there instead of from the probe function.
> 
> There is no functional change.
> 
> Co-developed-by: Nelson Escobar <neescoba@cisco.com>
> Signed-off-by: Nelson Escobar <neescoba@cisco.com>
> Co-developed-by: Satish Kharat <satishkh@cisco.com>
> Signed-off-by: Satish Kharat <satishkh@cisco.com>
> Signed-off-by: John Daley <johndale@cisco.com>

Reviewed-by: Andrew Lunn <andrew@lunn.ch>

    Andrew

Re: [PATCH net-next 2/2] enic: Obtain the Link speed only after the link comes up

[Andrew Lunn]

On Mon, Jan 06, 2025 at 06:51:35PM -0800, John Daley wrote:
> The link speed is obtained in the RX adaptive coalescing function. It
> was being called at probe time when the link may not be up. Change the
> call to run after the Link comes up.
> 
> The impact of not getting the correct link speed was that the low end of
> the adaptive interrupt range was always being set to 0 which could have
> caused a slight increase in the number of RX interrupts.
> 
> Co-developed-by: Nelson Escobar <neescoba@cisco.com>
> Signed-off-by: Nelson Escobar <neescoba@cisco.com>
> Co-developed-by: Satish Kharat <satishkh@cisco.com>
> Signed-off-by: Satish Kharat <satishkh@cisco.com>
> Signed-off-by: John Daley <johndale@cisco.com>
> ---
>  drivers/net/ethernet/cisco/enic/enic_main.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/ethernet/cisco/enic/enic_main.c b/drivers/net/ethernet/cisco/enic/enic_main.c
> index 957efe73e41a..49f6cab01ed5 100644
> --- a/drivers/net/ethernet/cisco/enic/enic_main.c
> +++ b/drivers/net/ethernet/cisco/enic/enic_main.c
> @@ -109,7 +109,7 @@ static struct enic_intr_mod_table mod_table[ENIC_MAX_COALESCE_TIMERS + 1] = {
>  static struct enic_intr_mod_range mod_range[ENIC_MAX_LINK_SPEEDS] = {
>  	{0,  0}, /* 0  - 4  Gbps */
>  	{0,  3}, /* 4  - 10 Gbps */
> -	{3,  6}, /* 10 - 40 Gbps */
> +	{3,  6}, /* 10+ Gbps */
>  };

So we still have this second change, which is not explained in the
commit message, and probably should be in a patch of its own.


    Andrew

---
pw-bot: cr

Re: [PATCH net-next 2/2] enic: Obtain the Link speed only after the link comes up

[John Daley]

>> diff --git a/drivers/net/ethernet/cisco/enic/enic_main.c b/drivers/net/ethernet/cisco/enic/enic_main.c
>> index 957efe73e41a..49f6cab01ed5 100644
>> --- a/drivers/net/ethernet/cisco/enic/enic_main.c
>> +++ b/drivers/net/ethernet/cisco/enic/enic_main.c
>> @@ -109,7 +109,7 @@ static struct enic_intr_mod_table mod_table[ENIC_MAX_COALESCE_TIMERS + 1] = {
>>  static struct enic_intr_mod_range mod_range[ENIC_MAX_LINK_SPEEDS] = {
>>  	{0,  0}, /* 0  - 4  Gbps */
>>  	{0,  3}, /* 4  - 10 Gbps */
>> -	{3,  6}, /* 10 - 40 Gbps */
>> +	{3,  6}, /* 10+ Gbps */
>
>Is this on purpose? You didn't mention anything about speed range in
>commit message. Just wondering, patch looks fine, thanks.

I changed the comment on purpose since it applies to adapters way past
40 Gbps nowdays. I should have mentioned the change. Thanks for the
reveiw.
>
>Reviewed-by: Michal Swiatkowski <michal.swiatkowski@linux.intel.com>

Re: [PATCH net-next 2/2] enic: Obtain the Link speed only after the link comes up

[John Daley]

>> The link speed is obtained in the RX adaptive coalescing function. It
>> was being called at probe time when the link may not be up. Change the
>> call to run after the Link comes up.
>> 
>> The impact of not getting the correct link speed was that the low end of
>> the adaptive interrupt range was always being set to 0 which could have
>> caused a slight increase in the number of RX interrupts.
>> 
>> Co-developed-by: Nelson Escobar <neescoba@cisco.com>
>> Signed-off-by: Nelson Escobar <neescoba@cisco.com>
>> Co-developed-by: Satish Kharat <satishkh@cisco.com>
>> Signed-off-by: Satish Kharat <satishkh@cisco.com>
>> Signed-off-by: John Daley <johndale@cisco.com>
>> ---
>>  drivers/net/ethernet/cisco/enic/enic_main.c | 4 ++--
>>  1 file changed, 2 insertions(+), 2 deletions(-)
>> 
>> diff --git a/drivers/net/ethernet/cisco/enic/enic_main.c b/drivers/net/ethernet/cisco/enic/enic_main.c
>> index 957efe73e41a..49f6cab01ed5 100644
>> --- a/drivers/net/ethernet/cisco/enic/enic_main.c
>> +++ b/drivers/net/ethernet/cisco/enic/enic_main.c
>> @@ -109,7 +109,7 @@ static struct enic_intr_mod_table mod_table[ENIC_MAX_COALESCE_TIMERS + 1] = {
>>  static struct enic_intr_mod_range mod_range[ENIC_MAX_LINK_SPEEDS] = {
>>  	{0,  0}, /* 0  - 4  Gbps */
>>  	{0,  3}, /* 4  - 10 Gbps */
>> -	{3,  6}, /* 10 - 40 Gbps */
>> +	{3,  6}, /* 10+ Gbps */
>>  };
>
>So we still have this second change, which is not explained in the
>commit message, and probably should be in a patch of its own.

OK, I did a v2 with the typo fix as its own patch in this set of link related
patches. Thanks.

Re: [PATCH net-next 2/2] enic: Obtain the Link speed only after the link comes up

[Michal Swiatkowski]

On Tue, Jan 07, 2025 at 11:53:04AM -0800, John Daley wrote:
> >> diff --git a/drivers/net/ethernet/cisco/enic/enic_main.c b/drivers/net/ethernet/cisco/enic/enic_main.c
> >> index 957efe73e41a..49f6cab01ed5 100644
> >> --- a/drivers/net/ethernet/cisco/enic/enic_main.c
> >> +++ b/drivers/net/ethernet/cisco/enic/enic_main.c
> >> @@ -109,7 +109,7 @@ static struct enic_intr_mod_table mod_table[ENIC_MAX_COALESCE_TIMERS + 1] = {
> >>  static struct enic_intr_mod_range mod_range[ENIC_MAX_LINK_SPEEDS] = {
> >>  	{0,  0}, /* 0  - 4  Gbps */
> >>  	{0,  3}, /* 4  - 10 Gbps */
> >> -	{3,  6}, /* 10 - 40 Gbps */
> >> +	{3,  6}, /* 10+ Gbps */
> >
> >Is this on purpose? You didn't mention anything about speed range in
> >commit message. Just wondering, patch looks fine, thanks.
> 
> I changed the comment on purpose since it applies to adapters way past
> 40 Gbps nowdays. I should have mentioned the change. Thanks for the
> reveiw.

Reasonable, thanks

> >
> >Reviewed-by: Michal Swiatkowski <michal.swiatkowski@linux.intel.com>