| Message ID | 20250107025135.15167-3-johndale@cisco.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | enic: Set link speed only after link up | expand |
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
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
>> 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>
>> 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.
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>
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);