netdev - Re: [PATCH net-next] cxgb4: don't hold RTNL during ethtool phys

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-Id: <201104061720.30219.leedom@chelsio.com>
Date:	Wed, 6 Apr 2011 17:20:29 -0700
From:	Casey Leedom <leedom@...lsio.com>
To:	Stephen Hemminger <shemminger@...ux-foundation.org>
Cc:	Dimitris Michailidis <dm@...lsio.com>,
	Ben Hutchings <bhutchings@...arflare.com>,
	David Miller <davem@...emloft.net>, netdev@...r.kernel.org
Subject: Re: [PATCH net-next] cxgb4: don't hold RTNL during ethtool phys_id

| From: Stephen Hemminger <shemminger@...ux-foundation.org>
| Date: Wednesday, April 06, 2011 05:09 pm
| 
| The Chelsio cxgb4 drivers implement blinking in a unique way by
| waiting on the mailbox. This patch cleans it up slightly by no longer
| holding the system wide network configuration lock during the process.
| 
| The patch also uses correct semantics for the time argument
| which is supposed to be in seconds; and zero is supposed
| to signify infinite blinking.
| 
| This is still a bad firmware interface design for this
| since it means the board is basically hung while doing the blink.
| But fixing it correctly would require hardware and firmware
| documentation. With that information the device could be converted
| to the new set_phys_id.
| 
| Compile tested only.
| 
| Signed-off-by: Stephen Hemminger <shemminger@...tta.com>

  Are you assuming that the firmware won't respond with a command completion 
until the LED blinking is complete?  If so, that's a bad assumption.  The 
firmware runs as an asynchronous real-time OS.  The LED blinking simply becomes 
a thread of activity within the OS and the command completes immediately.

Casey

| ---
|  drivers/net/cxgb4/cxgb4_main.c     |   17 ++++++++++++++---
|  drivers/net/cxgb4vf/cxgb4vf_main.c |   21 +++++++++++++++++++--
|  2 files changed, 33 insertions(+), 5 deletions(-)
| 
| --- a/drivers/net/cxgb4/cxgb4_main.c	2011-04-06 16:49:02.045648800 -0700
| +++ b/drivers/net/cxgb4/cxgb4_main.c	2011-04-06 17:00:59.508851692 -0700
| @@ -1339,12 +1339,23 @@ static int restart_autoneg(struct net_de
|  static int identify_port(struct net_device *dev, u32 data)
|  {
|  	struct adapter *adap = netdev2adap(dev);
| +	int rc;
| +	unsigned long blinks;
| 
|  	if (data == 0)
| -		data = 2;     /* default to 2 seconds */
| +		blinks = UINT_MAX;
| +	else
| +		blinks = 2*data + data/2;
| 
| -	return t4_identify_port(adap, adap->fn, netdev2pinfo(dev)->viid,
| -				data * 5);
| +	/* Don't block networking updates while blink is in progress */
| +	dev_hold(dev);
| +	rtnl_unlock();
| +
| +	rc = t4_identify_port(adap, adap->fn, netdev2pinfo(dev)->viid,
| +			      blinks);
| +	rtnl_lock();
| +	dev_put(dev);
| +	return rc;
|  }
| 
|  static unsigned int from_fw_linkcaps(unsigned int type, unsigned int caps)
| --- a/drivers/net/cxgb4vf/cxgb4vf_main.c	2011-04-06 16:49:09.989728600
| -0700 +++ b/drivers/net/cxgb4vf/cxgb4vf_main.c	2011-04-06
| 17:02:38.609846223 -0700 @@ -43,6 +43,7 @@
|  #include <linux/etherdevice.h>
|  #include <linux/debugfs.h>
|  #include <linux/ethtool.h>
| +#include <linux/rtnetlink.h>
| 
|  #include "t4vf_common.h"
|  #include "t4vf_defs.h"
| @@ -1352,11 +1353,27 @@ static int cxgb4vf_set_rx_csum(struct ne
|  /*
|   * Identify the port by blinking the port's LED.
|   */
| -static int cxgb4vf_phys_id(struct net_device *dev, u32 id)
| +static int cxgb4vf_phys_id(struct net_device *dev, u32 data)
|  {
|  	struct port_info *pi = netdev_priv(dev);
| +	int rc;
| +	unsigned int blinks;
| 
| -	return t4vf_identify_port(pi->adapter, pi->viid, 5);
| +	if (data == 0)
| +		blinks = UINT_MAX;
| +	else
| +		blinks = 2*data + data/2;
| +
| +	/* Don't block networking updates while blink is in progress */
| +	dev_hold(dev);
| +	rtnl_unlock();
| +
| +	rc =  t4vf_identify_port(pi->adapter, pi->viid, blinks);
| +
| +	rtnl_lock();
| +	dev_put(dev);
| +
| +	return rc;
|  }
| 
|  /*
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html