[RFT 0/3] cxgb4: use request_firmware_nowait()

[RFT 0/3] cxgb4: use request_firmware_nowait()

[Luis R. Rodriguez]

From: "Luis R. Rodriguez" <mcgrof@suse.com>

Its reported that loading the cxgb4 can take over 1 minute,
use the more sane request_firmware_nowait() API call just
in case this amount of time is causing issues. The driver
uses the firmware API 3 times, one for the firmware, one
for configuration and another one for flash, this provides
the port for all cases.

I don't have the hardware so please test. I did verify we
can use this during pci probe and also during the ethtool
flash callback.

Luis R. Rodriguez (3):
  cxgb4: make ethtool set_flash use request_firmware_nowait()
  cxgb4: make configuration load use request_firmware_nowait()
  cxgb4: make device firmware load use request_firmware_nowait()

 drivers/net/ethernet/chelsio/cxgb4/cxgb4.h      |  13 ++
 drivers/net/ethernet/chelsio/cxgb4/cxgb4_main.c | 258 +++++++++++++++---------
 2 files changed, 176 insertions(+), 95 deletions(-)

-- 
2.0.0

[RFT 1/3] cxgb4: make ethtool set_flash use request_firmware_nowait()

[Luis R. Rodriguez]

From: "Luis R. Rodriguez" <mcgrof@suse.com>

cxgb4 loading can take a while, this is part of the crusade to
change it to be asynchronous.

Cc: Casey Leedom <leedom@chelsio.com>
Cc: Hariprasad Shenai <hariprasad@chelsio.com>
Cc: Philip Oswald <poswald@suse.com>
Cc: Santosh Rastapur <santosh@chelsio.com>
Cc: Jeffrey Cheung <jcheung@suse.com>
Cc: David Chang <dchang@suse.com>
Signed-off-by: Luis R. Rodriguez <mcgrof@suse.com>
---
 drivers/net/ethernet/chelsio/cxgb4/cxgb4.h      |  3 ++
 drivers/net/ethernet/chelsio/cxgb4/cxgb4_main.c | 40 ++++++++++++++++++++-----
 2 files changed, 36 insertions(+), 7 deletions(-)

diff --git a/drivers/net/ethernet/chelsio/cxgb4/cxgb4.h b/drivers/net/ethernet/chelsio/cxgb4/cxgb4.h
index f503dce..bcf9acf 100644
--- a/drivers/net/ethernet/chelsio/cxgb4/cxgb4.h
+++ b/drivers/net/ethernet/chelsio/cxgb4/cxgb4.h
@@ -647,6 +647,9 @@ struct adapter {
 	struct dentry *debugfs_root;
 
 	spinlock_t stats_lock;
+
+	struct completion flash_comp;
+	int flash_comp_status;
 };
 
 /* Defined bit width of user definable filter tuples
diff --git a/drivers/net/ethernet/chelsio/cxgb4/cxgb4_main.c b/drivers/net/ethernet/chelsio/cxgb4/cxgb4_main.c
index 2f8d6b9..9cf6f3e 100644
--- a/drivers/net/ethernet/chelsio/cxgb4/cxgb4_main.c
+++ b/drivers/net/ethernet/chelsio/cxgb4/cxgb4_main.c
@@ -2713,22 +2713,48 @@ out:
 	return err;
 }
 
+static void cxgb4_flash_complete(const struct firmware *fw, void *context)
+{
+	struct adapter *adap = context;
+	int ret;
+
+	if (!fw) {
+		adap->flash_comp_status = -EINVAL;
+		goto out;
+	}
+
+	ret = t4_load_fw(adap, fw->data, fw->size);
+	if (!ret)
+		adap->flash_comp_status = ret;
+
+out:
+	release_firmware(fw);
+	complete(&adap->flash_comp);
+}
+
 static int set_flash(struct net_device *netdev, struct ethtool_flash *ef)
 {
 	int ret;
-	const struct firmware *fw;
 	struct adapter *adap = netdev2adap(netdev);
 
+	init_completion(&adap->flash_comp);
+	adap->flash_comp_status = 0;
+
 	ef->data[sizeof(ef->data) - 1] = '\0';
-	ret = request_firmware(&fw, ef->data, adap->pdev_dev);
+	ret = request_firmware_nowait(THIS_MODULE, 1, ef->data,
+				      adap->pdev_dev, GFP_KERNEL,
+				      adap, cxgb4_flash_complete);
 	if (ret < 0)
 		return ret;
 
-	ret = t4_load_fw(adap, fw->data, fw->size);
-	release_firmware(fw);
-	if (!ret)
-		dev_info(adap->pdev_dev, "loaded firmware %s\n", ef->data);
-	return ret;
+	wait_for_completion(&adap->flash_comp);
+
+	if (adap->flash_comp_status != 0)
+		return adap->flash_comp_status;
+
+	dev_info(adap->pdev_dev, "loaded firmware %s\n", ef->data);
+
+	return 0;
 }
 
 #define WOL_SUPPORTED (WAKE_BCAST | WAKE_MAGIC)
-- 
2.0.0

[RFT 2/3] cxgb4: make configuration load use request_firmware_nowait()

[Luis R. Rodriguez]

From: "Luis R. Rodriguez" <mcgrof@suse.com>

cxgb4 loading can take a while, this is part of the crusade to
change it to be asynchronous. One more to go.

Cc: Philip Oswald <poswald@suse.com>
Cc: Santosh Rastapur <santosh@chelsio.com>
Cc: Jeffrey Cheung <jcheung@suse.com>
Cc: David Chang <dchang@suse.com>
Cc: Casey Leedom <leedom@chelsio.com>
Cc: Hariprasad Shenai <hariprasad@chelsio.com>
Signed-off-by: Luis R. Rodriguez <mcgrof@suse.com>
---
 drivers/net/ethernet/chelsio/cxgb4/cxgb4.h      |   4 +
 drivers/net/ethernet/chelsio/cxgb4/cxgb4_main.c | 113 +++++++++++++++---------
 2 files changed, 73 insertions(+), 44 deletions(-)

diff --git a/drivers/net/ethernet/chelsio/cxgb4/cxgb4.h b/drivers/net/ethernet/chelsio/cxgb4/cxgb4.h
index bcf9acf..1507dc2 100644
--- a/drivers/net/ethernet/chelsio/cxgb4/cxgb4.h
+++ b/drivers/net/ethernet/chelsio/cxgb4/cxgb4.h
@@ -650,6 +650,10 @@ struct adapter {
 
 	struct completion flash_comp;
 	int flash_comp_status;
+
+	char fw_config_file[32];
+	struct completion config_comp;
+	int config_comp_status;
 };
 
 /* Defined bit width of user definable filter tuples
diff --git a/drivers/net/ethernet/chelsio/cxgb4/cxgb4_main.c b/drivers/net/ethernet/chelsio/cxgb4/cxgb4_main.c
index 9cf6f3e..65e4124 100644
--- a/drivers/net/ethernet/chelsio/cxgb4/cxgb4_main.c
+++ b/drivers/net/ethernet/chelsio/cxgb4/cxgb4_main.c
@@ -4827,51 +4827,18 @@ static int adap_init0_tweaks(struct adapter *adapter)
 	return 0;
 }
 
-/*
- * Attempt to initialize the adapter via a Firmware Configuration File.
- */
-static int adap_init0_config(struct adapter *adapter, int reset)
+static void cxgb4_config_complete(const struct firmware *cf, void *context)
 {
-	struct fw_caps_config_cmd caps_cmd;
-	const struct firmware *cf;
+	struct adapter *adapter = context;
 	unsigned long mtype = 0, maddr = 0;
 	u32 finiver, finicsum, cfcsum;
-	int ret;
-	int config_issued = 0;
-	char *fw_config_file, fw_config_file_path[256];
 	char *config_name = NULL;
+	struct fw_caps_config_cmd caps_cmd;
+	int config_issued = 0;
+	int ret = 0;
+	char fw_config_file_path[256];
 
-	/*
-	 * Reset device if necessary.
-	 */
-	if (reset) {
-		ret = t4_fw_reset(adapter, adapter->mbox,
-				  PIORSTMODE | PIORST);
-		if (ret < 0)
-			goto bye;
-	}
-
-	/*
-	 * If we have a T4 configuration file under /lib/firmware/cxgb4/,
-	 * then use that.  Otherwise, use the configuration file stored
-	 * in the adapter flash ...
-	 */
-	switch (CHELSIO_CHIP_VERSION(adapter->params.chip)) {
-	case CHELSIO_T4:
-		fw_config_file = FW4_CFNAME;
-		break;
-	case CHELSIO_T5:
-		fw_config_file = FW5_CFNAME;
-		break;
-	default:
-		dev_err(adapter->pdev_dev, "Device %d is not supported\n",
-		       adapter->pdev->device);
-		ret = -EINVAL;
-		goto bye;
-	}
-
-	ret = request_firmware(&cf, fw_config_file, adapter->pdev_dev);
-	if (ret < 0) {
+	if (!cf) {
 		config_name = "On FLASH";
 		mtype = FW_MEMTYPE_CF_FLASH;
 		maddr = t4_flash_cfg_addr(adapter);
@@ -4879,7 +4846,7 @@ static int adap_init0_config(struct adapter *adapter, int reset)
 		u32 params[7], val[7];
 
 		sprintf(fw_config_file_path,
-			"/lib/firmware/%s", fw_config_file);
+			"/lib/firmware/%s", adapter->fw_config_file);
 		config_name = fw_config_file_path;
 
 		if (cf->size >= FLASH_CFG_MAX_SIZE)
@@ -4898,7 +4865,7 @@ static int adap_init0_config(struct adapter *adapter, int reset)
 				 * to write that out separately since we can't
 				 * guarantee that the bytes following the
 				 * residual byte in the buffer returned by
-				 * request_firmware() are zeroed out ...
+				 * request_firmware_nowait() are zeroed out ...
 				 */
 				size_t resid = cf->size & 0x3;
 				size_t size = cf->size & ~0x3;
@@ -5018,7 +4985,8 @@ static int adap_init0_config(struct adapter *adapter, int reset)
 	dev_info(adapter->pdev_dev, "Successfully configured using Firmware "\
 		 "Configuration File \"%s\", version %#x, computed checksum %#x\n",
 		 config_name, finiver, cfcsum);
-	return 0;
+	complete(&adapter->config_comp);
+	return;
 
 	/*
 	 * Something bad happened.  Return the error ...  (If the "error"
@@ -5026,10 +4994,67 @@ static int adap_init0_config(struct adapter *adapter, int reset)
 	 * want to issue a warning since this is fairly common.)
 	 */
 bye:
+	adapter->flash_comp_status = ret;
 	if (config_issued && ret != -ENOENT)
 		dev_warn(adapter->pdev_dev, "\"%s\" configuration file error %d\n",
 			 config_name, -ret);
-	return ret;
+	complete(&adapter->config_comp);
+}
+
+/*
+ * Attempt to initialize the adapter via a Firmware Configuration File.
+ */
+static int adap_init0_config(struct adapter *adapter, int reset)
+{
+	int ret;
+
+	/*
+	 * Reset device if necessary.
+	 */
+	if (reset) {
+		ret = t4_fw_reset(adapter, adapter->mbox,
+				  PIORSTMODE | PIORST);
+		if (ret < 0)
+			return ret;
+	}
+
+	/*
+	 * If we have a T4 configuration file under /lib/firmware/cxgb4/,
+	 * then use that.  Otherwise, use the configuration file stored
+	 * in the adapter flash ...
+	 */
+	switch (CHELSIO_CHIP_VERSION(adapter->params.chip)) {
+	case CHELSIO_T4:
+		snprintf(adapter->fw_config_file,
+			 sizeof(adapter->fw_config_file),
+			 "%s", FW4_CFNAME);
+		break;
+	case CHELSIO_T5:
+		snprintf(adapter->fw_config_file,
+			 sizeof(adapter->fw_config_file),
+			 "%s", FW5_CFNAME);
+		break;
+	default:
+		dev_err(adapter->pdev_dev, "Device %d is not supported\n",
+		       adapter->pdev->device);
+		ret = -EINVAL;
+		return ret;
+	}
+
+	init_completion(&adapter->config_comp);
+	adapter->config_comp_status = 0;
+
+	ret = request_firmware_nowait(THIS_MODULE, 1, adapter->fw_config_file,
+				      adapter->pdev_dev, GFP_KERNEL,
+				      adapter, cxgb4_config_complete);
+	if (ret < 0)
+		return ret;
+
+	wait_for_completion(&adapter->flash_comp);
+	if (adapter->config_comp_status != 0)
+		return adapter->config_comp_status;
+
+	return 0;
 }
 
 /*
-- 
2.0.0

[RFT 3/3] cxgb4: make device firmware load use request_firmware_nowait()

[Luis R. Rodriguez]

From: "Luis R. Rodriguez" <mcgrof@suse.com>

cxgb4 loading can take a while, this ends the crusade to
change it to be asynchronous.

Cc: Casey Leedom <leedom@chelsio.com>
Cc: Hariprasad Shenai <hariprasad@chelsio.com>
Cc: Philip Oswald <poswald@suse.com>
Cc: Santosh Rastapur <santosh@chelsio.com>
Cc: Jeffrey Cheung <jcheung@suse.com>
Cc: David Chang <dchang@suse.com>
Signed-off-by: Luis R. Rodriguez <mcgrof@suse.com>
---
 drivers/net/ethernet/chelsio/cxgb4/cxgb4.h      |   6 ++
 drivers/net/ethernet/chelsio/cxgb4/cxgb4_main.c | 105 ++++++++++++++----------
 2 files changed, 67 insertions(+), 44 deletions(-)

diff --git a/drivers/net/ethernet/chelsio/cxgb4/cxgb4.h b/drivers/net/ethernet/chelsio/cxgb4/cxgb4.h
index 1507dc2..89296f1 100644
--- a/drivers/net/ethernet/chelsio/cxgb4/cxgb4.h
+++ b/drivers/net/ethernet/chelsio/cxgb4/cxgb4.h
@@ -654,6 +654,12 @@ struct adapter {
 	char fw_config_file[32];
 	struct completion config_comp;
 	int config_comp_status;
+
+	struct fw_info *fw_info;
+	struct completion fw_comp;
+	int fw_comp_status;
+	enum dev_state state;
+	int reset;
 };
 
 /* Defined bit width of user definable filter tuples
diff --git a/drivers/net/ethernet/chelsio/cxgb4/cxgb4_main.c b/drivers/net/ethernet/chelsio/cxgb4/cxgb4_main.c
index 65e4124..105b83a 100644
--- a/drivers/net/ethernet/chelsio/cxgb4/cxgb4_main.c
+++ b/drivers/net/ethernet/chelsio/cxgb4/cxgb4_main.c
@@ -5341,6 +5341,39 @@ static struct fw_info *find_fw_info(int chip)
 	return NULL;
 }
 
+static void cxgb4_fw_complete(const struct firmware *fw, void *context)
+{
+	struct adapter *adap = context;
+	struct fw_hdr *card_fw;
+	const u8 *fw_data = NULL;
+	unsigned int fw_size = 0;
+
+	/* allocate memory to read the header of the firmware on the
+	 * card
+	 */
+	card_fw = t4_alloc_mem(sizeof(*card_fw));
+
+	if (!fw) {
+		dev_err(adap->pdev_dev,
+			"unable to load firmware image %s\n",
+			adap->fw_info->fw_mod_name);
+	} else {
+		fw_data = fw->data;
+		fw_size = fw->size;
+	}
+
+	/* upgrade FW logic */
+	adap->fw_comp_status = t4_prep_fw(adap, adap->fw_info, fw_data,
+					  fw_size, card_fw, adap->state,
+					  &adap->reset);
+
+	/* Cleaning up */
+	if (fw != NULL)
+		release_firmware(fw);
+	t4_free_mem(card_fw);
+	complete(&adap->fw_comp);
+}
+
 /*
  * Phase 0 of initialization: contact FW, obtain config, perform basic init.
  */
@@ -5348,10 +5381,10 @@ static int adap_init0(struct adapter *adap)
 {
 	int ret;
 	u32 v, port_vec;
-	enum dev_state state;
 	u32 params[7], val[7];
 	struct fw_caps_config_cmd caps_cmd;
-	int reset = 1;
+
+	adap->reset = 1;
 
 	/*
 	 * Contact FW, advertising Master capability (and potentially forcing
@@ -5360,7 +5393,7 @@ static int adap_init0(struct adapter *adap)
 	 */
 	ret = t4_fw_hello(adap, adap->mbox, adap->fn,
 			  force_init ? MASTER_MUST : MASTER_MAY,
-			  &state);
+			  &adap->state);
 	if (ret < 0) {
 		dev_err(adap->pdev_dev, "could not connect to FW, error %d\n",
 			ret);
@@ -5368,8 +5401,8 @@ static int adap_init0(struct adapter *adap)
 	}
 	if (ret == adap->mbox)
 		adap->flags |= MASTER_PF;
-	if (force_init && state == DEV_STATE_INIT)
-		state = DEV_STATE_UNINIT;
+	if (force_init && adap->state == DEV_STATE_INIT)
+		adap->state = DEV_STATE_UNINIT;
 
 	/*
 	 * If we're the Master PF Driver and the device is uninitialized,
@@ -5380,51 +5413,34 @@ static int adap_init0(struct adapter *adap)
 	 */
 	t4_get_fw_version(adap, &adap->params.fw_vers);
 	t4_get_tp_version(adap, &adap->params.tp_vers);
-	if ((adap->flags & MASTER_PF) && state != DEV_STATE_INIT) {
-		struct fw_info *fw_info;
-		struct fw_hdr *card_fw;
-		const struct firmware *fw;
-		const u8 *fw_data = NULL;
-		unsigned int fw_size = 0;
+	if ((adap->flags & MASTER_PF) && adap->state != DEV_STATE_INIT) {
+		init_completion(&adap->fw_comp);
+		adap->fw_comp_status = 0;
 
 		/* This is the firmware whose headers the driver was compiled
 		 * against
 		 */
-		fw_info = find_fw_info(CHELSIO_CHIP_VERSION(adap->params.chip));
-		if (fw_info == NULL) {
+		adap->fw_info =
+			find_fw_info(CHELSIO_CHIP_VERSION(adap->params.chip));
+		if (adap->fw_info == NULL) {
 			dev_err(adap->pdev_dev,
 				"unable to get firmware info for chip %d.\n",
 				CHELSIO_CHIP_VERSION(adap->params.chip));
 			return -EINVAL;
 		}
 
-		/* allocate memory to read the header of the firmware on the
-		 * card
-		 */
-		card_fw = t4_alloc_mem(sizeof(*card_fw));
-
 		/* Get FW from from /lib/firmware/ */
-		ret = request_firmware(&fw, fw_info->fw_mod_name,
-				       adap->pdev_dev);
-		if (ret < 0) {
-			dev_err(adap->pdev_dev,
-				"unable to load firmware image %s, error %d\n",
-				fw_info->fw_mod_name, ret);
-		} else {
-			fw_data = fw->data;
-			fw_size = fw->size;
-		}
-
-		/* upgrade FW logic */
-		ret = t4_prep_fw(adap, fw_info, fw_data, fw_size, card_fw,
-				 state, &reset);
-
-		/* Cleaning up */
-		if (fw != NULL)
-			release_firmware(fw);
-		t4_free_mem(card_fw);
-
+		ret = request_firmware_nowait(THIS_MODULE, 1,
+					      adap->fw_info->fw_mod_name,
+					      adap->pdev_dev,
+					      GFP_KERNEL,
+					      adap,
+					      cxgb4_fw_complete);
 		if (ret < 0)
+			return -EINVAL;
+
+		wait_for_completion(&adap->fw_comp);
+		if (adap->fw_comp_status < 0)
 			goto bye;
 	}
 
@@ -5460,7 +5476,7 @@ static int adap_init0(struct adapter *adap)
 	 * adapter parameters.  Otherwise, it's time to try initializing the
 	 * adapter ...
 	 */
-	if (state == DEV_STATE_INIT) {
+	if (adap->state == DEV_STATE_INIT) {
 		dev_info(adap->pdev_dev, "Coming up as %s: "\
 			 "Adapter already initialized\n",
 			 adap->flags & MASTER_PF ? "MASTER" : "SLAVE");
@@ -5477,7 +5493,7 @@ static int adap_init0(struct adapter *adap)
 			dev_warn(adap->pdev_dev, "Firmware doesn't support "
 				 "configuration file.\n");
 		if (force_old_init)
-			ret = adap_init0_no_config(adap, reset);
+			ret = adap_init0_no_config(adap, adap->reset);
 		else {
 			/*
 			 * Find out whether we're dealing with a version of
@@ -5497,7 +5513,7 @@ static int adap_init0(struct adapter *adap)
 			 * Configuration File found.
 			 */
 			if (ret < 0)
-				ret = adap_init0_no_config(adap, reset);
+				ret = adap_init0_no_config(adap, adap->reset);
 			else {
 				/*
 				 * The firmware provides us with a memory
@@ -5506,13 +5522,14 @@ static int adap_init0(struct adapter *adap)
 				 * the Configuration File in flash.
 				 */
 
-				ret = adap_init0_config(adap, reset);
+				ret = adap_init0_config(adap, adap->reset);
 				if (ret == -ENOENT) {
 					dev_info(adap->pdev_dev,
 					    "No Configuration File present "
 					    "on adapter. Using hard-wired "
 					    "configuration parameters.\n");
-					ret = adap_init0_no_config(adap, reset);
+					ret = adap_init0_no_config(adap,
+								   adap->reset);
 				}
 			}
 		}
@@ -5711,7 +5728,7 @@ static int adap_init0(struct adapter *adap)
 	 * parameters.
 	 */
 	t4_read_mtu_tbl(adap, adap->params.mtus, NULL);
-	if (state != DEV_STATE_INIT) {
+	if (adap->state != DEV_STATE_INIT) {
 		int i;
 
 		/* The default MTU Table contains values 1492 and 1500.
-- 
2.0.0

Re: [RFT 0/3] cxgb4: use request_firmware_nowait()

[Casey Leedom]

   I've looked through the patch and I might be wrong, but it appears 
that all the uses of the asynchronous request_firmware_nowait() are 
followed immediately by wait_for_completion() calls which essentially 
would be the same as the previous code with an added layer of 
mechanism.  Am I missing something?

   We do have a problem with initialization of multiple adapters with 
external PHYs since, for each adapter we can check to see if the main 
adapter firmware needs updating, and then load the PHY firmware.  If the 
main firmware needs updating on more than one adapter, the combined time 
to update each adapter's main firmware plus load the PHY firmware can 
exceed some Distribution's default limits for a driver module's load 
time (since the kernel seems to be processing the PCI Probe of each 
device sequentially).

   It seems to me that it's unfortunate that the limit isn't on a per 
device basis since a system could have an arbitrary number of devices 
managed by a driver module.  Also, it might be useful if there was a way 
for the driver module to "tell" the timeout mechanism that forward 
progress _is_ being made so it doesn't blow away the driver module 
load.  And maybe, if I'm right regarding the sequential nature of the 
introduction of devices to driver modules, it might make sense for a 
driver module to be able to "tell" the kernel that it has no per-device 
dependencies and multiple devices may be probed simultaneously ...

Casey

On 06/20/14 17:39, Luis R. Rodriguez wrote:
> From: "Luis R. Rodriguez" <mcgrof@suse.com>
>
> Its reported that loading the cxgb4 can take over 1 minute,
> use the more sane request_firmware_nowait() API call just
> in case this amount of time is causing issues. The driver
> uses the firmware API 3 times, one for the firmware, one
> for configuration and another one for flash, this provides
> the port for all cases.
>
> I don't have the hardware so please test. I did verify we
> can use this during pci probe and also during the ethtool
> flash callback.
>
> Luis R. Rodriguez (3):
>    cxgb4: make ethtool set_flash use request_firmware_nowait()
>    cxgb4: make configuration load use request_firmware_nowait()
>    cxgb4: make device firmware load use request_firmware_nowait()
>
>   drivers/net/ethernet/chelsio/cxgb4/cxgb4.h      |  13 ++
>   drivers/net/ethernet/chelsio/cxgb4/cxgb4_main.c | 258 +++++++++++++++---------
>   2 files changed, 176 insertions(+), 95 deletions(-)
>

Re: [RFT 0/3] cxgb4: use request_firmware_nowait()

[Luis R. Rodriguez]

On Mon, Jun 23, 2014 at 12:06:48PM -0700, Casey Leedom wrote:
>   I've looked through the patch and I might be wrong, but it appears that 
> all the uses of the asynchronous request_firmware_nowait() are followed 
> immediately by wait_for_completion() calls which essentially would be the 
> same as the previous code with an added layer of mechanism.  Am I missing 
> something?

No you're right and I frailed to mention my original goal really is to
see if we can instead move that to ndo_init().

>   We do have a problem with initialization of multiple adapters with 
> external PHYs since, for each adapter we can check to see if the main 
> adapter firmware needs updating, and then load the PHY firmware.  If the 
> main firmware needs updating on more than one adapter, the combined time to 
> update each adapter's main firmware plus load the PHY firmware can exceed 
> some Distribution's default limits for a driver module's load time (since 
> the kernel seems to be processing the PCI Probe of each device 
> sequentially).

I noticed that for configuration updates it is optional for these configuration
updates to exist, in which case then if udev is enabled the driver will wait
quite a long time unncessarily. To fix that it seems request_firmware_direct()
would be better.

>   It seems to me that it's unfortunate that the limit isn't on a per device 
> basis since a system could have an arbitrary number of devices managed by a 
> driver module. 

The timeout is for the amount of time it takes the kernel to get the firemware,
not for device initialization, so its unclear to me that the 60 timeout thing
is actually causing an issue here.

> Also, it might be useful if there was a way for the driver 
> module to "tell" the timeout mechanism that forward progress _is_ being 
> made so it doesn't blow away the driver module load.

Indeed if this is actually needed, but believe the issue here for the
huge delays might be instead the lack of not using request_firmware_direct()
and actual device initialization time, which I do not believe we penalize,
we should be penalizing only the amount of time it takes either the
kernel or udev to read the firmware from the filesystem.

>  And maybe, if I'm 
> right regarding the sequential nature of the introduction of devices to 
> driver modules, it might make sense for a driver module to be able to 
> "tell" the kernel that it has no per-device dependencies and multiple 
> devices may be probed simultaneously ...

What if just configuration updates use request_firmware_direct() and
for the actual firmware which is required a request_firmware_nowait()
with a proper wait_for_completion() on the ndo_init() ?

  Luis

Re: [RFT 0/3] cxgb4: use request_firmware_nowait()

[Casey Leedom]

On 06/23/14 17:29, Luis R. Rodriguez wrote:
> On Mon, Jun 23, 2014 at 12:06:48PM -0700, Casey Leedom wrote:
>>    I've looked through the patch and I might be wrong, but it appears that
>> all the uses of the asynchronous request_firmware_nowait() are followed
>> immediately by wait_for_completion() calls which essentially would be the
>> same as the previous code with an added layer of mechanism.  Am I missing
>> something?
> No you're right and I frailed to mention my original goal really is to
> see if we can instead move that to ndo_init().

   Okay, thanks for confirming that.  I thought I was being very stupid.

   The problem I think with ndo_init() is that comes off a Network 
Device (struct net_device) and you can't have Network Devices till you 
talk to the adpater, discover how many ports you have, what their MAC 
Addresses are, etc.  And of course you'd then have to be prepared for an 
ndo_init() call on every instantiated Network Device on an adapter and 
only do the device initialization for the first (while holding off any 
and all activity on the others), etc.  All doable but a bit more 
complicated than doing this at Device Probe time.

>>    We do have a problem with initialization of multiple adapters with
>> external PHYs since, for each adapter we can check to see if the main
>> adapter firmware needs updating, and then load the PHY firmware.  If the
>> main firmware needs updating on more than one adapter, the combined time to
>> update each adapter's main firmware plus load the PHY firmware can exceed
>> some Distribution's default limits for a driver module's load time (since
>> the kernel seems to be processing the PCI Probe of each device
>> sequentially).
> I noticed that for configuration updates it is optional for these configuration
> updates to exist, in which case then if udev is enabled the driver will wait
> quite a long time unncessarily. To fix that it seems request_firmware_direct()
> would be better.

   Hhmmm, I'm unfamiliar with all of this.  I hadn't looked that far 
under the covers of request_firmware() to know that any of these 
variants existed, what their semantics were and when one over the other 
was the preferred API.

>>    It seems to me that it's unfortunate that the limit isn't on a per device
>> basis since a system could have an arbitrary number of devices managed by a
>> driver module.
> The timeout is for the amount of time it takes the kernel to get the firemware,
> not for device initialization, so its unclear to me that the 60 timeout thing
> is actually causing an issue here.

   Are you sure about that?  I just loaded firmware on two of my 
T4-based adapters and each took about 15 seconds to complete.  The 
firmware is ~0.5MB and needs to be written to the on-adapter FLASH. The 
PHY firmware takes a bit less but not a lot.  Add that to the time to do 
general adapter initialization and you're cruising close to 1 minute for 
two adapters ...  Thus, my comment that whatever timeout is present 
should really be per-adapter based ... and it would be nice if the 
driver could inform the kernel as to the expected maximum device probe 
initialization time so we didn't have to have a huge inappropriate 
timeout for all devices ...

>> Also, it might be useful if there was a way for the driver
>> module to "tell" the timeout mechanism that forward progress _is_ being
>> made so it doesn't blow away the driver module load.
> Indeed if this is actually needed, but believe the issue here for the
> huge delays might be instead the lack of not using request_firmware_direct()
> and actual device initialization time, which I do not believe we penalize,
> we should be penalizing only the amount of time it takes either the
> kernel or udev to read the firmware from the filesystem.

   If you want I can time the actual phases of loading new firmware: 
request_firmware(), writing it to FLASH, release_firmware() ...

>>   And maybe, if I'm
>> right regarding the sequential nature of the introduction of devices to
>> driver modules, it might make sense for a driver module to be able to
>> "tell" the kernel that it has no per-device dependencies and multiple
>> devices may be probed simultaneously ...
> What if just configuration updates use request_firmware_direct() and
> for the actual firmware which is required a request_firmware_nowait()
> with a proper wait_for_completion() on the ndo_init() ?

   I'm not quite sure I understand what you're asking here.  The cxgb4 
driver attaches to the firmware in the probe() stage and, if the 
firmware is older then the supported version, upgrades the firmware and 
RESETs the device.  Then, for adapters with external PHYs (10Gb/s BT 
predominantly), the same process can happen for the PHY firmware (or on 
some adapters which have no PHY-attached FLASH, this happens for every 
driver load).  It's conceivable that we could defer the PHY firmware 
load till the first ndo_open() but we'd still be stuck with the 
seemingly broken idea that a simple timeout for a driver module load is 
good enough when what we seem to need is a timeout per device ...

Casey

Re: [RFT 0/3] cxgb4: use request_firmware_nowait()

[Casey Leedom]

On 06/24/14 08:55, Casey Leedom wrote:
>
> On 06/23/14 17:29, Luis R. Rodriguez wrote:
>> On Mon, Jun 23, 2014 at 12:06:48PM -0700, Casey Leedom wrote:
>>
>>> Also, it might be useful if there was a way for the driver
>>> module to "tell" the timeout mechanism that forward progress _is_ being
>>> made so it doesn't blow away the driver module load.
>> Indeed if this is actually needed, but believe the issue here for the
>> huge delays might be instead the lack of not using 
>> request_firmware_direct()
>> and actual device initialization time, which I do not believe we 
>> penalize,
>> we should be penalizing only the amount of time it takes either the
>> kernel or udev to read the firmware from the filesystem.
>
>   If you want I can time the actual phases of loading new firmware: 
> request_firmware(), writing it to FLASH, release_firmware() ...

   So I just did this for a normal modprobe (after the system is up):

Jiffies    Process
-----------------------------------------------------------------------
       0    begin firmware load process
       3    request_firmware() returns
       7    start looking at the adapter
      10    finish reading the first sector of existing adapter firmware
      26    we've decided that we're going to upgrade the firmware
      28    actual firmware upgrade process starts
     448    we've finished halting the adapter processor
     451    we enter the firmware write routine
   8,470    we've finished erasing the firmware FLASH sectors
  14,336    write of new firmware is complete
  14,340    the new firmware load is complete
  14,949    the adapter processor has been restarted; new firmware running
  14,952    firmware upgrade process complete

Maybe request_firmware() takes more time during the boot phase but as we 
can see from the above timings, it's the actual firmware upgrade process 
which takes the most time ...

Casey

Re: [RFT 0/3] cxgb4: use request_firmware_nowait()

[Luis R. Rodriguez]

On Tue, Jun 24, 2014 at 09:34:19AM -0700, Casey Leedom wrote:
> On 06/24/14 08:55, Casey Leedom wrote:
>> On 06/23/14 17:29, Luis R. Rodriguez wrote:
>   So I just did this for a normal modprobe (after the system is up):
>
> Jiffies    Process
> -----------------------------------------------------------------------
>       0    begin firmware load process
>       3    request_firmware() returns
>       7    start looking at the adapter
>      10    finish reading the first sector of existing adapter firmware
>      26    we've decided that we're going to upgrade the firmware
>      28    actual firmware upgrade process starts
>     448    we've finished halting the adapter processor
>     451    we enter the firmware write routine
>   8,470    we've finished erasing the firmware FLASH sectors
>  14,336    write of new firmware is complete
>  14,340    the new firmware load is complete
>  14,949    the adapter processor has been restarted; new firmware running
>  14,952    firmware upgrade process complete
>
> Maybe request_firmware() takes more time during the boot phase but as we 
> can see from the above timings, it's the actual firmware upgrade process 
> which takes the most time ...

OK so yeah the kernel work on request_firmware() isn't what takes over a
minute, its the actual hardware poking with the firmware it gets, and then
doing all the things you mentioned (a port for each netdevice, etc).  This is a
particularly interesting driver, apart from this I see some code about bus
master and loading firmware only once. Can you elaborate a bit on how that is
designed to work? Is it that only one PCI bus master device is allowed, and
that will do the request_firmware() for all PCI devices? I'm a bit confused
about this part, are we sure the bus master device will probe first? We can
surely keep all this code on the driver but it seems that if all these
complexitities might become the norm we should consider an API for sharing a
clean framework for it.

As you noted the complexities on firmware loading, the number of different
netdevices one device might actually have would make it impractical to try
to do any completion on firmware on the ndo_init() with request_firmware_nowait().
Apart from a netdev specific API to handle all this cleanly, I wonder if
drivers like these merit getting placed by default onto the deferred_probe_active_list.
Typically this is triggered when drivers don't have a resource a subsystem
hasn't yet brought up, the driver returns -EPROBE_DEFER and the core driver
infrastructure later probes these devices on a secondary list. Greg?

  Luis

Re: [RFT 0/3] cxgb4: use request_firmware_nowait()

[Luis R. Rodriguez]

On Wed, Jun 25, 2014 at 01:39:51AM +0200, Luis R. Rodriguez wrote:
> On Tue, Jun 24, 2014 at 09:34:19AM -0700, Casey Leedom wrote:
> > On 06/24/14 08:55, Casey Leedom wrote:
> >> On 06/23/14 17:29, Luis R. Rodriguez wrote:
> >   So I just did this for a normal modprobe (after the system is up):
> >
> > Jiffies    Process
> > -----------------------------------------------------------------------
> >       0    begin firmware load process
> >       3    request_firmware() returns
> >       7    start looking at the adapter
> >      10    finish reading the first sector of existing adapter firmware
> >      26    we've decided that we're going to upgrade the firmware
> >      28    actual firmware upgrade process starts
> >     448    we've finished halting the adapter processor
> >     451    we enter the firmware write routine
> >   8,470    we've finished erasing the firmware FLASH sectors
> >  14,336    write of new firmware is complete
> >  14,340    the new firmware load is complete
> >  14,949    the adapter processor has been restarted; new firmware running
> >  14,952    firmware upgrade process complete
> >
> > Maybe request_firmware() takes more time during the boot phase but as we 
> > can see from the above timings, it's the actual firmware upgrade process 
> > which takes the most time ...
> 
> OK so yeah the kernel work on request_firmware() isn't what takes over a
> minute, its the actual hardware poking with the firmware it gets, and then
> doing all the things you mentioned (a port for each netdevice, etc).  This is a
> particularly interesting driver, apart from this I see some code about bus
> master and loading firmware only once. Can you elaborate a bit on how that is
> designed to work? Is it that only one PCI bus master device is allowed, and
> that will do the request_firmware() for all PCI devices? I'm a bit confused
> about this part, are we sure the bus master device will probe first? We can
> surely keep all this code on the driver but it seems that if all these
> complexitities might become the norm we should consider an API for sharing a
> clean framework for it.
> 
> As you noted the complexities on firmware loading, the number of different
> netdevices one device might actually have would make it impractical to try
> to do any completion on firmware on the ndo_init() with request_firmware_nowait().
> Apart from a netdev specific API to handle all this cleanly, I wonder if
> drivers like these merit getting placed by default onto the deferred_probe_active_list.
> Typically this is triggered when drivers don't have a resource a subsystem
> hasn't yet brought up, the driver returns -EPROBE_DEFER and the core driver
> infrastructure later probes these devices on a secondary list. Greg?

Actually another option to clean this up is to use platform_device_register_simple()
after the initial firmware load and start poking at stuff there. Check out
drivers/net/ethernet/8390/ne.c for an example with probe and all. I think
that can help split up the code paths quite nicely and let you do your
pre port thing there. Thoughts?

I still do have that question about bus master requirement though and ensuring
that there are no races.

  Luis

Re: [RFT 0/3] cxgb4: use request_firmware_nowait()

[Luis R. Rodriguez]

On Wed, Jun 25, 2014 at 04:00:19AM +0200, Luis R. Rodriguez wrote:
> On Wed, Jun 25, 2014 at 01:39:51AM +0200, Luis R. Rodriguez wrote:
> > On Tue, Jun 24, 2014 at 09:34:19AM -0700, Casey Leedom wrote:
> > > On 06/24/14 08:55, Casey Leedom wrote:
> > >> On 06/23/14 17:29, Luis R. Rodriguez wrote:
> > >   So I just did this for a normal modprobe (after the system is up):
> > >
> > > Jiffies    Process
> > > -----------------------------------------------------------------------
> > >       0    begin firmware load process
> > >       3    request_firmware() returns
> > >       7    start looking at the adapter
> > >      10    finish reading the first sector of existing adapter firmware
> > >      26    we've decided that we're going to upgrade the firmware
> > >      28    actual firmware upgrade process starts
> > >     448    we've finished halting the adapter processor
> > >     451    we enter the firmware write routine
> > >   8,470    we've finished erasing the firmware FLASH sectors
> > >  14,336    write of new firmware is complete
> > >  14,340    the new firmware load is complete
> > >  14,949    the adapter processor has been restarted; new firmware running
> > >  14,952    firmware upgrade process complete
> > >
> > > Maybe request_firmware() takes more time during the boot phase but as we 
> > > can see from the above timings, it's the actual firmware upgrade process 
> > > which takes the most time ...
> > 
> > OK so yeah the kernel work on request_firmware() isn't what takes over a
> > minute, its the actual hardware poking with the firmware it gets, and then
> > doing all the things you mentioned (a port for each netdevice, etc).  This is a
> > particularly interesting driver, apart from this I see some code about bus
> > master and loading firmware only once. Can you elaborate a bit on how that is
> > designed to work? Is it that only one PCI bus master device is allowed, and
> > that will do the request_firmware() for all PCI devices? I'm a bit confused
> > about this part, are we sure the bus master device will probe first? We can
> > surely keep all this code on the driver but it seems that if all these
> > complexitities might become the norm we should consider an API for sharing a
> > clean framework for it.

Casey here was the first series of questions.

> > As you noted the complexities on firmware loading, the number of different
> > netdevices one device might actually have would make it impractical to try
> > to do any completion on firmware on the ndo_init() with request_firmware_nowait().
> > Apart from a netdev specific API to handle all this cleanly, I wonder if
> > drivers like these merit getting placed by default onto the deferred_probe_active_list.
> > Typically this is triggered when drivers don't have a resource a subsystem
> > hasn't yet brought up, the driver returns -EPROBE_DEFER and the core driver
> > infrastructure later probes these devices on a secondary list. Greg?
> 
> Actually another option to clean this up is to use platform_device_register_simple()
> after the initial firmware load and start poking at stuff there. Check out
> drivers/net/ethernet/8390/ne.c for an example with probe and all. I think
> that can help split up the code paths quite nicely and let you do your
> pre port thing there. Thoughts?

And here was the other one, what your thoughts are on splitting things up
a bit more for probe as ports part of a platform driver?

In the meantime I'll go and hunt down to see if there are any timeouts other
than the one embedded within request_firmware() (or udev if used). As we have
clarified now the 60s timeout is a timeout embedded as part of the filesystem
lookup of the firmware, not the actual loading of the firmware onto the device.

I for instance can introduce a huge delay on purpose right after
request_firmware() say 3 minutes on a test driver and it loads just fine, but
on my OpenSUSE 13.1 system. I'll go ahead and test this on the other
distribution you mentioned you had issues, curious what could trigger a timeout
failure there that would be distribution specific.

ergon:~ # ls > /lib/firmware/fake.bin

mcgrof@ergon ~/devel/fake-firmware-delay $ cat Makefile
all: modules

.PHONY: modules
modules:
	make -C /lib/modules/$(shell uname -r)/build M=$(PWD) modules

.PHONY: clean
clean:
	make -C /lib/modules/$(shell uname -r)/build clean M=$(PWD)

obj-m += test.o

mcgrof@ergon ~/devel/fake-firmware-delay $ cat test.c
#include <linux/kernel.h>
#include <linux/module.h>
#include <linux/firmware.h>
#include <linux/platform_device.h>
#include <linux/delay.h>

static struct platform_device *pdev;

static int __init test_init(void)
{
	int ret;
	const struct firmware *config;

	pdev = platform_device_register_simple("fake-dev", 0, NULL, 0);
	if (IS_ERR(pdev))
		return PTR_ERR(pdev);

	ret = request_firmware(&config, "fake.bin", &pdev->dev);
	if (ret < 0) {
		dev_set_uevent_suppress(&pdev->dev, true);
		platform_device_unregister(pdev);
		return ret;
	}

	ssleep(180);

	release_firmware(config);

	return 0;
}

static void __exit test_exit(void)
{
	dev_set_uevent_suppress(&pdev->dev, true);
	platform_device_unregister(pdev);
}

module_init(test_init)
module_exit(test_exit)
MODULE_LICENSE("GPL");

  Luis

Re: [RFT 0/3] cxgb4: use request_firmware_nowait()

[Luis R. Rodriguez]

On Wed, Jun 25, 2014 at 2:51 PM, Luis R. Rodriguez <mcgrof@suse.com> wrote:
> I'll go ahead and test this on the other
> distribution you mentioned you had issues, curious what could trigger a timeout
> failure there that would be distribution specific.

I've tested this on SLE12 and see no issues as well.

  Luis