| Message ID | 1429710210-7291-1-git-send-email-ivecera@redhat.com |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | David Miller |
| Headers | show |
On Wed, 2015-04-22 at 15:43 +0200, Ivan Vecera wrote: > The driver unlike other drivers does not log link state changes. Why add all the speed stuff, why not add a query instead? I think it'd be simpler to add a line like: status = be_cmd_link_status_query(adapter, &speed, &status, 0); before emitting speed/link_status Question for the emulex folk: Is the dom argument to link_status_query necessary? It's currently always 0. and trivially: > diff --git a/drivers/net/ethernet/emulex/benet/be_main.c b/drivers/net/ethernet/emulex/benet/be_main.c [] > @@ -649,7 +649,8 @@ static struct rtnl_link_stats64 *be_get_stats64(struct net_device *netdev, > return stats; > } > > -void be_link_status_update(struct be_adapter *adapter, u8 link_status) > +void be_link_status_update(struct be_adapter *adapter, u8 link_status, > + u16 speed) > { > struct net_device *netdev = adapter->netdev; > > @@ -658,10 +659,18 @@ void be_link_status_update(struct be_adapter *adapter, u8 link_status) > adapter->flags |= BE_FLAGS_LINK_STATUS_INIT; > } > > - if (link_status) > + if (link_status) { > + if (speed) > + /* Print speed only when it is known */ > + netdev_info(netdev, "Link is Up at %d Mbps\n", speed); signed/unsigned mismatch. %u, speed -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 04/22/2015 04:07 PM, Joe Perches wrote: > On Wed, 2015-04-22 at 15:43 +0200, Ivan Vecera wrote: >> >The driver unlike other drivers does not log link state changes. > Why add all the speed stuff, why not add a query instead? > > I think it'd be simpler to add a line like: > status = be_cmd_link_status_query(adapter, &speed, &status, 0); > before emitting speed/link_status The func be_link_status_update() is also called from be_async_link_state_process() and I'm not sure if it is possible to call be_cmd_link_status_query() from its context. Ivan -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
From: Joe Perches <joe@perches.com> Date: Wed, 22 Apr 2015 07:07:54 -0700 > On Wed, 2015-04-22 at 15:43 +0200, Ivan Vecera wrote: >> @@ -658,10 +659,18 @@ void be_link_status_update(struct be_adapter *adapter, u8 link_status) >> adapter->flags |= BE_FLAGS_LINK_STATUS_INIT; >> } >> >> - if (link_status) >> + if (link_status) { >> + if (speed) >> + /* Print speed only when it is known */ >> + netdev_info(netdev, "Link is Up at %d Mbps\n", speed); > > signed/unsigned mismatch. %u, speed Indeed, please fix this up and resubmit, thanks. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> -----Original Message----- > From: Ivan Vecera [mailto:ivecera@redhat.com] > > The driver unlike other drivers does not log link state changes. > > v2: added current link speed to log message > Ivan, I disagree with the v2 change. I think your original intention was just to log a message when the link goes up or down asynchronously (i.e., without any user intervention.) After alerting the user, if the user wants to know other link properties like speed, duplex etc then the ethtool cmd needs to be used; there is no need to log a message with those details. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> -----Original Message----- > From: Joe Perches [mailto:joe@perches.com] > > On Wed, 2015-04-22 at 15:43 +0200, Ivan Vecera wrote: > > The driver unlike other drivers does not log link state changes. > ... > > Question for the emulex folk: > > Is the dom argument to link_status_query necessary? > It's currently always 0. It's needed only if the PF wants to query the "link_status" of a VF, which it doesn't do currently... Yes, its un-necessary and can be removed (in a separate patch I guess :-)). thanks, -Sathya -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 04/23/2015 08:31 AM, Sathya Perla wrote: >> -----Original Message----- >> From: Ivan Vecera [mailto:ivecera@redhat.com] >> >> The driver unlike other drivers does not log link state changes. >> >> v2: added current link speed to log message >> > Ivan, I disagree with the v2 change. I think your original intention > was just to log a message when the link goes up or down > asynchronously (i.e., without any user intervention.) > After alerting the user, if the user wants to know other link > properties like speed, duplex etc then the ethtool cmd needs > to be used; there is no need to log a message with those details. Yes, this was my original intention... to see these async link events in the system log. In this case the v1 should be used. Ivan -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 04/23/2015 08:31 AM, Sathya Perla wrote: >> -----Original Message----- >> From: Ivan Vecera [mailto:ivecera@redhat.com] >> >> The driver unlike other drivers does not log link state changes. >> >> v2: added current link speed to log message >> > Ivan, I disagree with the v2 change. I think your original intention > was just to log a message when the link goes up or down > asynchronously (i.e., without any user intervention.) > After alerting the user, if the user wants to know other link > properties like speed, duplex etc then the ethtool cmd needs > to be used; there is no need to log a message with those details. > > -- > To unsubscribe from this list: send the line "unsubscribe netdev" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > Dave, could we apply the v1 WRT Sathya's comment? Thanks, Ivan -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
From: Ivan Vecera <ivecera@redhat.com> Date: Tue, 28 Apr 2015 16:32:37 +0200 > On 04/23/2015 08:31 AM, Sathya Perla wrote: >>> -----Original Message----- >>> From: Ivan Vecera [mailto:ivecera@redhat.com] >>> >>> The driver unlike other drivers does not log link state changes. >>> >>> v2: added current link speed to log message >>> >> Ivan, I disagree with the v2 change. I think your original intention >> was just to log a message when the link goes up or down >> asynchronously (i.e., without any user intervention.) >> After alerting the user, if the user wants to know other link >> properties like speed, duplex etc then the ethtool cmd needs >> to be used; there is no need to log a message with those details. >> >> -- >> To unsubscribe from this list: send the line "unsubscribe netdev" in >> the body of a message to majordomo@vger.kernel.org >> More majordomo info at http://vger.kernel.org/majordomo-info.html >> > Dave, could we apply the v1 WRT Sathya's comment? Patches should be resubmitted freshly when people want me to do something like this. Thanks. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 04/28/2015 06:44 PM, David Miller wrote: > From: Ivan Vecera <ivecera@redhat.com> > Date: Tue, 28 Apr 2015 16:32:37 +0200 > >> On 04/23/2015 08:31 AM, Sathya Perla wrote: >>>> -----Original Message----- >>>> From: Ivan Vecera [mailto:ivecera@redhat.com] >>>> >>>> The driver unlike other drivers does not log link state changes. >>>> >>>> v2: added current link speed to log message >>>> >>> Ivan, I disagree with the v2 change. I think your original intention >>> was just to log a message when the link goes up or down >>> asynchronously (i.e., without any user intervention.) >>> After alerting the user, if the user wants to know other link >>> properties like speed, duplex etc then the ethtool cmd needs >>> to be used; there is no need to log a message with those details. >>> >>> -- >>> To unsubscribe from this list: send the line "unsubscribe netdev" in >>> the body of a message to majordomo@vger.kernel.org >>> More majordomo info at http://vger.kernel.org/majordomo-info.html >>> >> Dave, could we apply the v1 WRT Sathya's comment? > > Patches should be resubmitted freshly when people want me to do something > like this. > > Thanks. > OK... will repost v1. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/net/ethernet/emulex/benet/be.h b/drivers/net/ethernet/emulex/benet/be.h index 1bf1cdc..f5409d9 100644 --- a/drivers/net/ethernet/emulex/benet/be.h +++ b/drivers/net/ethernet/emulex/benet/be.h @@ -796,7 +796,8 @@ static inline void be_clear_all_error(struct be_adapter *adapter) void be_cq_notify(struct be_adapter *adapter, u16 qid, bool arm, u16 num_popped); -void be_link_status_update(struct be_adapter *adapter, u8 link_status); +void be_link_status_update(struct be_adapter *adapter, u8 link_status, + u16 speed); void be_parse_stats(struct be_adapter *adapter); int be_load_fw(struct be_adapter *adapter, u8 *func); bool be_is_wol_supported(struct be_adapter *adapter); diff --git a/drivers/net/ethernet/emulex/benet/be_cmds.c b/drivers/net/ethernet/emulex/benet/be_cmds.c index fb140fa..60381eb 100644 --- a/drivers/net/ethernet/emulex/benet/be_cmds.c +++ b/drivers/net/ethernet/emulex/benet/be_cmds.c @@ -262,7 +262,8 @@ static void be_async_link_state_process(struct be_adapter *adapter, */ if (adapter->flags & BE_FLAGS_LINK_STATUS_INIT) be_link_status_update(adapter, - evt->port_link_status & LINK_STATUS_MASK); + evt->port_link_status & LINK_STATUS_MASK, + 0); } static void be_async_port_misconfig_event_process(struct be_adapter *adapter, diff --git a/drivers/net/ethernet/emulex/benet/be_ethtool.c b/drivers/net/ethernet/emulex/benet/be_ethtool.c index b765c24..831db95 100644 --- a/drivers/net/ethernet/emulex/benet/be_ethtool.c +++ b/drivers/net/ethernet/emulex/benet/be_ethtool.c @@ -611,7 +611,7 @@ static int be_get_settings(struct net_device *netdev, struct ethtool_cmd *ecmd) status = be_cmd_link_status_query(adapter, &link_speed, &link_status, 0); if (!status) - be_link_status_update(adapter, link_status); + be_link_status_update(adapter, link_status, link_speed); ethtool_cmd_speed_set(ecmd, link_speed); status = be_cmd_get_phy_info(adapter); diff --git a/drivers/net/ethernet/emulex/benet/be_main.c b/drivers/net/ethernet/emulex/benet/be_main.c index fb0bc3c..d3bfac9 100644 --- a/drivers/net/ethernet/emulex/benet/be_main.c +++ b/drivers/net/ethernet/emulex/benet/be_main.c @@ -649,7 +649,8 @@ static struct rtnl_link_stats64 *be_get_stats64(struct net_device *netdev, return stats; } -void be_link_status_update(struct be_adapter *adapter, u8 link_status) +void be_link_status_update(struct be_adapter *adapter, u8 link_status, + u16 speed) { struct net_device *netdev = adapter->netdev; @@ -658,10 +659,18 @@ void be_link_status_update(struct be_adapter *adapter, u8 link_status) adapter->flags |= BE_FLAGS_LINK_STATUS_INIT; } - if (link_status) + if (link_status) { + if (speed) + /* Print speed only when it is known */ + netdev_info(netdev, "Link is Up at %d Mbps\n", speed); + else + netdev_info(netdev, "Link is Up"); + netif_carrier_on(netdev); - else + } else { + netdev_info(netdev, "Link is Down\n"); netif_carrier_off(netdev); + } } static void be_tx_stats_update(struct be_tx_obj *txo, struct sk_buff *skb) @@ -3241,6 +3250,7 @@ static int be_open(struct net_device *netdev) struct be_eq_obj *eqo; struct be_rx_obj *rxo; struct be_tx_obj *txo; + u16 speed; u8 link_status; int status, i; @@ -3267,9 +3277,9 @@ static int be_open(struct net_device *netdev) } adapter->flags |= BE_FLAGS_NAPI_ENABLED; - status = be_cmd_link_status_query(adapter, NULL, &link_status, 0); + status = be_cmd_link_status_query(adapter, &speed, &link_status, 0); if (!status) - be_link_status_update(adapter, link_status); + be_link_status_update(adapter, link_status, speed); netif_tx_start_all_queues(netdev); be_roce_dev_open(adapter);
The driver unlike other drivers does not log link state changes. v2: added current link speed to log message Cc: Sathya Perla <sathya.perla@emulex.com> Cc: Subbu Seetharaman <subbu.seetharaman@emulex.com> Cc: Ajit Khaparde <ajit.khaparde@emulex.com> Signed-off-by: Ivan Vecera <ivecera@redhat.com> --- drivers/net/ethernet/emulex/benet/be.h | 3 ++- drivers/net/ethernet/emulex/benet/be_cmds.c | 3 ++- drivers/net/ethernet/emulex/benet/be_ethtool.c | 2 +- drivers/net/ethernet/emulex/benet/be_main.c | 20 +++++++++++++++----- 4 files changed, 20 insertions(+), 8 deletions(-)