Re: [PATCH net-next v2] be2net: log link status
On 04/28/2015 06:44 PM, David Miller wrote: From: Ivan Vecera 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:ivec...@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 majord...@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 majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH net-next v2] be2net: log link status
From: Ivan Vecera 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:ivec...@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 majord...@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 majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH net-next v2] be2net: log link status
On 04/23/2015 08:31 AM, Sathya Perla wrote: -Original Message- From: Ivan Vecera [mailto:ivec...@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 majord...@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 majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH net-next v2] be2net: log link status
On 04/23/2015 08:31 AM, Sathya Perla wrote: -Original Message- From: Ivan Vecera [mailto:ivec...@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 majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH net-next v2] be2net: log link status
> -Original Message- > From: Joe Perches [mailto:j...@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 majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH net-next v2] be2net: log link status
> -Original Message- > From: Ivan Vecera [mailto:ivec...@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 majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH net-next v2] be2net: log link status
From: Joe Perches
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 majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH net-next v2] be2net: log link status
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 majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH net-next v2] be2net: log link status
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 majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH net-next v2] be2net: log link status
The driver unlike other drivers does not log link state changes.
v2: added current link speed to log message
Cc: Sathya Perla
Cc: Subbu Seetharaman
Cc: Ajit Khaparde
Signed-off-by: Ivan Vecera
---
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(-)
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);
--
2.0.5
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.
