[PATCH 0/3] drivers: expand usage of request_firmware_direct()

[PATCH 0/3] drivers: expand usage of request_firmware_direct()

[Luis R. Rodriguez]

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

Takashi added request_firmware_direct() via bba3a87e9 through v3.14-rc1
which avoids the unnecessary delay introduced by using the udev firmware
loader in case the first try failed when loading we know loading "firmware"
is optional. The first use case was for microcode update but if drivers are
using it for optional configuration updates, custom EEPROMs, and other
junk other than firmware that should apply as well as good use cases,
specially if the driver already had a first phase in which it loaded
the first required firmware. While reviewing one driver I figured it'd
be best to try to give formalizing a check with SmPL. This isn't perfect
it had 1 false possitive drivers/fmc/fmc-fakedev.c on the entire kernel
run but my hope is this can be extended a bit more to build more
confidence, and then perhaps stuff it as a coccicheck.

I suppose this will not be required once and if we remove
CONFIG_FW_LOADER_USER_HELPER. Is that ever going away for good? I know
there was a recent attempt to remove the udev loader support but
it was unclear if the special alternative helper support would be
removed upstream from the kernel.

@ firmware_not_critical @
expression cf;
expression config_file;
expression dev;
int ret;
identifier l;
statement S;
@@

-	ret = request_firmware(&cf, config_file, dev);
+	ret = request_firmware_direct(&cf, config_file, dev);
	if (ret < 0) {
		... when != goto l;
		    when != return ret;
		    when any
	} else {
		...
		release_firmware(cf);
		...
	}


Luis R. Rodriguez (3):
  mmc: vub300: use request_firmware_direct() for pseudo code
  cxgb4: make configuration load use request_firmware_direct()
  p54: use request_firmware_direct() for optional EEPROM override

 drivers/mmc/host/vub300.c                       | 5 +++--
 drivers/net/ethernet/chelsio/cxgb4/cxgb4_main.c | 2 +-
 drivers/net/wireless/p54/p54spi.c               | 2 +-
 3 files changed, 5 insertions(+), 4 deletions(-)

-- 
2.0.0

[PATCH 1/3] mmc: vub300: use request_firmware_direct() for pseudo code

[Luis R. Rodriguez]

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

vub300 uses the request_firmware() API for downloading
pseudo code twice and if this fails its not considered fatal.
Avoid letting the request linger 60 seconds because of udev if
one of the requests fails by explicitly skipping the udev helper.

This was found with the following SmPL patch.

@ firmware_not_critical @
expression cf;
expression config_file;
expression dev;
int ret;
identifier l;
statement S;
@@

-	ret = request_firmware(&cf, config_file, dev);
+	ret = request_firmware_direct(&cf, config_file, dev);
	if (ret < 0) {
		... when != goto l;
		    when != return ret;
		    when any
	} else {
		...
		release_firmware(cf);
		...
	}

Cc: Tony Olech <tony.olech@elandigitalsystems.com>
Cc: linux-mmc@vger.kernel.org
Cc: linux-usb@vger.kernel.org
Cc: Takashi Iwai <tiwai@suse.de>
Cc: cocci@systeme.lip6.fr
Signed-off-by: Luis R. Rodriguez <mcgrof@suse.com>
---
 drivers/mmc/host/vub300.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/mmc/host/vub300.c b/drivers/mmc/host/vub300.c
index 4262296..15eeaff 100644
--- a/drivers/mmc/host/vub300.c
+++ b/drivers/mmc/host/vub300.c
@@ -1378,11 +1378,12 @@ static void download_offload_pseudocode(struct vub300_mmc_host *vub300)
 	snprintf(vub300->vub_name + l, sizeof(vub300->vub_name) - l, ".bin");
 	dev_info(&vub300->udev->dev, "requesting offload firmware %s\n",
 		 vub300->vub_name);
-	retval = request_firmware(&fw, vub300->vub_name, &card->dev);
+	retval = request_firmware_direct(&fw, vub300->vub_name, &card->dev);
 	if (retval < 0) {
 		strncpy(vub300->vub_name, "vub_default.bin",
 			sizeof(vub300->vub_name));
-		retval = request_firmware(&fw, vub300->vub_name, &card->dev);
+		retval = request_firmware_direct(&fw, vub300->vub_name,
+						 &card->dev);
 		if (retval < 0) {
 			strncpy(vub300->vub_name,
 				"no SDIO offload firmware found",
-- 
2.0.0

[PATCH 2/3] cxgb4: make configuration load use request_firmware_direct()

[Luis R. Rodriguez]

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

cxgb4 uses request_firmware() 3 times, one for firmware, one for
optional configuration files and another for ethtools flash. Since the
configuration update is optional on devices that don't have a
configuration file update it means we'd wait unnecessarily for the
extra udev timeout, which by default is 60 seconds. Avoid this
extra delay.

This was found with the following SmPL patch.

@ firmware_not_critical @
expression cf;
expression config_file;
expression dev;
int ret;
identifier l;
statement S;
@@

-	ret = request_firmware(&cf, config_file, dev);
+	ret = request_firmware_direct(&cf, config_file, dev);
	if (ret < 0) {
		... when != goto l;
		    when != return ret;
		    when any
	} else {
		...
		release_firmware(cf);
		...
	}

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>
Cc: Takashi Iwai <tiwai@suse.de>
Cc: cocci@systeme.lip6.fr
Signed-off-by: Luis R. Rodriguez <mcgrof@suse.com>
---
 drivers/net/ethernet/chelsio/cxgb4/cxgb4_main.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/chelsio/cxgb4/cxgb4_main.c b/drivers/net/ethernet/chelsio/cxgb4/cxgb4_main.c
index 02a0ebf..bd57177 100644
--- a/drivers/net/ethernet/chelsio/cxgb4/cxgb4_main.c
+++ b/drivers/net/ethernet/chelsio/cxgb4/cxgb4_main.c
@@ -4999,7 +4999,7 @@ static int adap_init0_config(struct adapter *adapter, int reset)
 		goto bye;
 	}
 
-	ret = request_firmware(&cf, fw_config_file, adapter->pdev_dev);
+	ret = request_firmware_direct(&cf, fw_config_file, adapter->pdev_dev);
 	if (ret < 0) {
 		config_name = "On FLASH";
 		mtype = FW_MEMTYPE_CF_FLASH;
-- 
2.0.0

Re: [PATCH 2/3] cxgb4: make configuration load use request_firmware_direct()

[Casey Leedom]

[[ Hopefully this makes it through to the kernel.org lists -- I’m using the Mac OS/X Mailer and it’s not clear how to force it not to use HTML format. -- Casey ]]

  So does request_firmware_direct() only fail if the requested file is not present on the file system or does it fail in other cases as well?

  If it’s the former, then the change to cxgb4 is fine.

  But if it’s the latter, then it’s definitely not okay.  While the driver _can_ continue running without the local on-disk Firmware Configuration File, that file can be used to significantly change the behavior and capabilities of the adapter and is user-customizable.  If a user makes changes to the local on-disk Firmware Configuration File and these are randomly silently ignored this will lead to highly annoying support issues.

Casey


On Jun 24, 2014, at 3:39 PM, Luis R. Rodriguez <mcgrof@do-not-panic.com> wrote:

> From: "Luis R. Rodriguez" <mcgrof@suse.com>
> 
> cxgb4 uses request_firmware() 3 times, one for firmware, one for
> optional configuration files and another for ethtools flash. Since the
> configuration update is optional on devices that don't have a
> configuration file update it means we'd wait unnecessarily for the
> extra udev timeout, which by default is 60 seconds. Avoid this
> extra delay.
> 
> This was found with the following SmPL patch.
> 
> @ firmware_not_critical @
> expression cf;
> expression config_file;
> expression dev;
> int ret;
> identifier l;
> statement S;
> @@
> 
> -	ret = request_firmware(&cf, config_file, dev);
> +	ret = request_firmware_direct(&cf, config_file, dev);
> 	if (ret < 0) {
> 		... when != goto l;
> 		    when != return ret;
> 		    when any
> 	} else {
> 		...
> 		release_firmware(cf);
> 		...
> 	}
> 
> 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>
> Cc: Takashi Iwai <tiwai@suse.de>
> Cc: cocci@systeme.lip6.fr
> Signed-off-by: Luis R. Rodriguez <mcgrof@suse.com>
> ---
> drivers/net/ethernet/chelsio/cxgb4/cxgb4_main.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/net/ethernet/chelsio/cxgb4/cxgb4_main.c b/drivers/net/ethernet/chelsio/cxgb4/cxgb4_main.c
> index 02a0ebf..bd57177 100644
> --- a/drivers/net/ethernet/chelsio/cxgb4/cxgb4_main.c
> +++ b/drivers/net/ethernet/chelsio/cxgb4/cxgb4_main.c
> @@ -4999,7 +4999,7 @@ static int adap_init0_config(struct adapter *adapter, int reset)
>  		goto bye;
>  	}
> 
> -	ret = request_firmware(&cf, fw_config_file, adapter->pdev_dev);
> +	ret = request_firmware_direct(&cf, fw_config_file, adapter->pdev_dev);
>  	if (ret < 0) {
>  		config_name = "On FLASH";
>  		mtype = FW_MEMTYPE_CF_FLASH;
> -- 
> 2.0.0
> 

Re: [PATCH 2/3] cxgb4: make configuration load use request_firmware_direct()

[Luis R. Rodriguez]

On Tue, Jun 24, 2014 at 03:54:44PM -0700, Casey Leedom wrote:
> [[ Hopefully this makes it through to the kernel.org lists -- I’m using the
>   Mac OS/X Mailer and it’s not clear how to force it not to use HTML format.
>   -- Casey ]]
> 
>   So does request_firmware_direct() only fail if the requested file is not
>   present on the file system or does it fail in other cases as well?

Same as before they are the same exact call with the only difference
being udev is not used as an extra helper, so it saves the extra
delay caused by udev. That's all.

>   If it’s the former, then the change to cxgb4 is fine.
> 
>   But if it’s the latter, then it’s definitely not okay.  While the driver
>   _can_ continue running without the local on-disk Firmware Configuration
>   File, that file can be used to significantly change the behavior and
>   capabilities of the adapter and is user-customizable.  If a user makes
>   changes to the local on-disk Firmware Configuration File and these are
>   randomly silently ignored this will lead to highly annoying support issues.

This just avoids udev, the request goes directly to the filesystem. The
failure will happen when the file is not present just as before, the
only difference here is skipping udev, it doesn't suffer from the extra 
60 second timeout. There's another possible failure, when
usermodehelper_read_trylock() fails but that is just as the code was before
so this change doesn't introduce that as a new false check. When that
triggers yout get a nasty WARN_ON() just as before.

One thing to note though is that the kernel firmware loader does not
log any failure to the kernel ring buffer if the firmware is not on the
filesystem, while the udev loader is a bit more chatty:

[ 2463.666120] platform fake-dev.0: Direct firmware load failed with error -2
[ 2463.666129] platform fake-dev.0: Falling back to user helper

Stuffing a print into the non-udev approach upstream seems a bit excessive
though (unless folks disagree), so if you want the driver to be a bit more
informative I think its best to place this feedback on the driver for now.

I see the driver does provide this information already though so is any
additional prints really desirable ?

        dev_info(adapter->pdev_dev, "Successfully configured using Firmware "\  
                 "Configuration File \"%s\", version %#x, computed checksum %#x\n",
                 config_name, finiver, cfcsum);   

  Luis

Re: [PATCH 2/3] cxgb4: make configuration load use request_firmware_direct()

[Casey Leedom]

On 06/24/14 18:50, Luis R. Rodriguez wrote:
> On Tue, Jun 24, 2014 at 03:54:44PM -0700, Casey Leedom wrote:
>> [[ Hopefully this makes it through to the kernel.org lists -- I’m using the
>>    Mac OS/X Mailer and it’s not clear how to force it not to use HTML format.
>>    -- Casey ]]
>>
>>    So does request_firmware_direct() only fail if the requested file is not
>>    present on the file system or does it fail in other cases as well?
> Same as before they are the same exact call with the only difference
> being udev is not used as an extra helper, so it saves the extra
> delay caused by udev. That's all.
>
>>    If it’s the former, then the change to cxgb4 is fine.
>>
>>    But if it’s the latter, then it’s definitely not okay.  While the driver
>>    _can_ continue running without the local on-disk Firmware Configuration
>>    File, that file can be used to significantly change the behavior and
>>    capabilities of the adapter and is user-customizable.  If a user makes
>>    changes to the local on-disk Firmware Configuration File and these are
>>    randomly silently ignored this will lead to highly annoying support issues.
> This just avoids udev, the request goes directly to the filesystem. The
> failure will happen when the file is not present just as before, the
> only difference here is skipping udev, it doesn't suffer from the extra
> 60 second timeout. There's another possible failure, when
> usermodehelper_read_trylock() fails but that is just as the code was before
> so this change doesn't introduce that as a new false check. When that
> triggers yout get a nasty WARN_ON() just as before.

   Huh, okay.  I guess I'm confused about the value of 
request_firmware() and the User Device helper.  If 
request_firmware_direct() just goes to the file system to grab the file 
and returns with ENOENT if it's not there, then you could replace every 
usage of request_firmware() in the cxgb4 driver as far as I can see ...  
Either the files are there and we'll use them or they won't be and we'll 
have to cope with that.  Am I missing something?

   And again, this definitely isn't going to solve the problem that 
started this whole line of research: we're still going to time out the 
load of cxgb4 if there are multiple 10Gb/s BT adapters in a system and 
we need to load each one with both base firmware and PHY firmware.

Casey

> One thing to note though is that the kernel firmware loader does not
> log any failure to the kernel ring buffer if the firmware is not on the
> filesystem, while the udev loader is a bit more chatty:
>
> [ 2463.666120] platform fake-dev.0: Direct firmware load failed with error -2
> [ 2463.666129] platform fake-dev.0: Falling back to user helper
>
> Stuffing a print into the non-udev approach upstream seems a bit excessive
> though (unless folks disagree), so if you want the driver to be a bit more
> informative I think its best to place this feedback on the driver for now.
>
> I see the driver does provide this information already though so is any
> additional prints really desirable ?
>
>          dev_info(adapter->pdev_dev, "Successfully configured using Firmware "\
>                   "Configuration File \"%s\", version %#x, computed checksum %#x\n",
>                   config_name, finiver, cfcsum);
>
>    Luis

Re: [PATCH 2/3] cxgb4: make configuration load use request_firmware_direct()

[Luis R. Rodriguez]

On Wed, Jun 25, 2014 at 10:12:20AM -0700, Casey Leedom wrote:
>
> On 06/24/14 18:50, Luis R. Rodriguez wrote:
>> On Tue, Jun 24, 2014 at 03:54:44PM -0700, Casey Leedom wrote:
>>> [[ Hopefully this makes it through to the kernel.org lists -- I’m using the
>>>    Mac OS/X Mailer and it’s not clear how to force it not to use HTML format.
>>>    -- Casey ]]
>>>
>>>    So does request_firmware_direct() only fail if the requested file is not
>>>    present on the file system or does it fail in other cases as well?
>> Same as before they are the same exact call with the only difference
>> being udev is not used as an extra helper, so it saves the extra
>> delay caused by udev. That's all.
>>
>>>    If it’s the former, then the change to cxgb4 is fine.
>>>
>>>    But if it’s the latter, then it’s definitely not okay.  While the driver
>>>    _can_ continue running without the local on-disk Firmware Configuration
>>>    File, that file can be used to significantly change the behavior and
>>>    capabilities of the adapter and is user-customizable.  If a user makes
>>>    changes to the local on-disk Firmware Configuration File and these are
>>>    randomly silently ignored this will lead to highly annoying support issues.
>> This just avoids udev, the request goes directly to the filesystem. The
>> failure will happen when the file is not present just as before, the
>> only difference here is skipping udev, it doesn't suffer from the extra
>> 60 second timeout. There's another possible failure, when
>> usermodehelper_read_trylock() fails but that is just as the code was before
>> so this change doesn't introduce that as a new false check. When that
>> triggers yout get a nasty WARN_ON() just as before.
>
>   Huh, okay.  I guess I'm confused about the value of request_firmware() 
> and the User Device helper.  If request_firmware_direct() just goes to the 
> file system to grab the file and returns with ENOENT if it's not there, 
> then you could replace every usage of request_firmware() in the cxgb4 
> driver as far as I can see ...  Either the files are there and we'll use 
> them or they won't be and we'll have to cope with that.  Am I missing 
> something?

You're actually right specially given that udev firmware uploading will
hopefully eventually be removed eventually (it seems it was just one driver
that caused to consider waiting on the removal, some driver that required
looking for firmware on some custom path I think or used a custom loader), for
now however its best to keep things consistent otherwise we'd replace
everything already. The _direct() call then is best used for optional firmware
for now. Perhaps in the end will be that the non _direct() call will have an
explicit print to the ring buffer if the file was not found.

>   And again, this definitely isn't going to solve the problem that started 
> this whole line of research:

I consider this research part of understanding and optimizing firmware
loading on cxgb4, in this case this would save 60 seconds for each
optional configuration file not present when loading, its not clear to
me how often devices don't have optional configs so its unclear to me the
exact savings in general, but if there's at least one user that should
speed things up.

> we're still going to time out the load of 
> cxgb4 if there are multiple 10Gb/s BT adapters in a system and we need to 
> load each one with both base firmware and PHY firmware.

Again, the timeout is *within* firmware_request(), firmware_release() does
not tell the firmware loader the timeout is over. The timeout is for the
kernel doing the hunt for the file.

As I see it the next steps on the evaluation on firmware loading on cxgb4
would be to evaluate a clean strategy to split things up, and also would
appreciate feedback on the bus master thing. Perhaps best we continue that
discussoin on that thread?

  Luis

Re: [PATCH 2/3] cxgb4: make configuration load use request_firmware_direct()

[Casey Leedom]

   Okay, I'll leave the whole request_firmware{,_direct,_nowait}() thing 
alone.  The request_firmware_direct() will "solve" a non-problem (since 
all of our "firmware" files are _supposed to be_ always present.  (And 
the 60 second timeout for udev to confirm that a file doesn't exist 
seems like udev is just basically broken.)

   That aside, we still need to solve the real problem that we're 
experiencing in that the boot-time load of cxgb4 is timing out on SLE12 
because a maximum load timeout has been instituted in that distribution 
for driver modules and if there are two 10Gb/s-BT adapters present in a 
system which need both base firmware and BT PHY firmware, we exceed that 
timeout.  The timeout really should be per device (since there ~could~ 
be an arbitrary number of devices in a system) and there probably should 
be a way for the driver to notify the kernel timeout mechanism that 
forward progress is being made ...

Casey

On 06/25/14 10:31, Luis R. Rodriguez wrote:
> On Wed, Jun 25, 2014 at 10:12:20AM -0700, Casey Leedom wrote:
>> On 06/24/14 18:50, Luis R. Rodriguez wrote:
>>> On Tue, Jun 24, 2014 at 03:54:44PM -0700, Casey Leedom wrote:
>>>> [[ Hopefully this makes it through to the kernel.org lists -- I’m using the
>>>>     Mac OS/X Mailer and it’s not clear how to force it not to use HTML format.
>>>>     -- Casey ]]
>>>>
>>>>     So does request_firmware_direct() only fail if the requested file is not
>>>>     present on the file system or does it fail in other cases as well?
>>> Same as before they are the same exact call with the only difference
>>> being udev is not used as an extra helper, so it saves the extra
>>> delay caused by udev. That's all.
>>>
>>>>     If it’s the former, then the change to cxgb4 is fine.
>>>>
>>>>     But if it’s the latter, then it’s definitely not okay.  While the driver
>>>>     _can_ continue running without the local on-disk Firmware Configuration
>>>>     File, that file can be used to significantly change the behavior and
>>>>     capabilities of the adapter and is user-customizable.  If a user makes
>>>>     changes to the local on-disk Firmware Configuration File and these are
>>>>     randomly silently ignored this will lead to highly annoying support issues.
>>> This just avoids udev, the request goes directly to the filesystem. The
>>> failure will happen when the file is not present just as before, the
>>> only difference here is skipping udev, it doesn't suffer from the extra
>>> 60 second timeout. There's another possible failure, when
>>> usermodehelper_read_trylock() fails but that is just as the code was before
>>> so this change doesn't introduce that as a new false check. When that
>>> triggers yout get a nasty WARN_ON() just as before.
>>    Huh, okay.  I guess I'm confused about the value of request_firmware()
>> and the User Device helper.  If request_firmware_direct() just goes to the
>> file system to grab the file and returns with ENOENT if it's not there,
>> then you could replace every usage of request_firmware() in the cxgb4
>> driver as far as I can see ...  Either the files are there and we'll use
>> them or they won't be and we'll have to cope with that.  Am I missing
>> something?
> You're actually right specially given that udev firmware uploading will
> hopefully eventually be removed eventually (it seems it was just one driver
> that caused to consider waiting on the removal, some driver that required
> looking for firmware on some custom path I think or used a custom loader), for
> now however its best to keep things consistent otherwise we'd replace
> everything already. The _direct() call then is best used for optional firmware
> for now. Perhaps in the end will be that the non _direct() call will have an
> explicit print to the ring buffer if the file was not found.
>
>>    And again, this definitely isn't going to solve the problem that started
>> this whole line of research:
> I consider this research part of understanding and optimizing firmware
> loading on cxgb4, in this case this would save 60 seconds for each
> optional configuration file not present when loading, its not clear to
> me how often devices don't have optional configs so its unclear to me the
> exact savings in general, but if there's at least one user that should
> speed things up.
>
>> we're still going to time out the load of
>> cxgb4 if there are multiple 10Gb/s BT adapters in a system and we need to
>> load each one with both base firmware and PHY firmware.
> Again, the timeout is *within* firmware_request(), firmware_release() does
> not tell the firmware loader the timeout is over. The timeout is for the
> kernel doing the hunt for the file.
>
> As I see it the next steps on the evaluation on firmware loading on cxgb4
> would be to evaluate a clean strategy to split things up, and also would
> appreciate feedback on the bus master thing. Perhaps best we continue that
> discussoin on that thread?
>
>    Luis
>

Re: [PATCH 2/3] cxgb4: make configuration load use request_firmware_direct()

[Luis R. Rodriguez]

On Wed, Jun 25, 2014 at 11:58:52AM -0700, Casey Leedom wrote:
>   Okay, I'll leave the whole request_firmware{,_direct,_nowait}() thing 
> alone.  The request_firmware_direct() will "solve" a non-problem (since all 
> of our "firmware" files are _supposed to be_ always present.

The code does not reflect that, it allows for files to not be present for
the config stuff. If in practice files are always present that's a bit
different.

> (And the 60 
> second timeout for udev to confirm that a file doesn't exist seems like 
> udev is just basically broken.)

Its one reason it being tossed.

>   That aside, we still need to solve the real problem that we're 
> experiencing in that the boot-time load of cxgb4 is timing out on SLE12 
> because a maximum load timeout has been instituted in that distribution for 
> driver modules and if there are two 10Gb/s-BT adapters present in a system 
> which need both base firmware and BT PHY firmware, we exceed that timeout.  

As for that it'd be great if you can answer some questions I had about
the rest of firmware load processing on the other thread, I had a bit
of questions for you there.

> The timeout really should be per device (since there ~could~ be an 
> arbitrary number of devices in a system) and there probably should be a way 
> for the driver to notify the kernel timeout mechanism that forward progress 
> is being made ...

I'd prefer we dive into this on the other thread.

  Luis

[PATCH 3/3] p54: use request_firmware_direct() for optional EEPROM override

[Luis R. Rodriguez]

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

The p54 driver uses request_firmware() twice, once for actual
firmware and then another time for an optional user overide on
EEPROM, 3826.eeprom. The custom EEPROM  is optional but if not
present we'll introduce an extra lag of 60 seconds with udev
present. Annotate we don't want udev nonsense here to avoid
the lag in case its not present.

This was found with the following SmPL patch.

@ firmware_not_critical @
expression cf;
expression config_file;
expression dev;
int ret;
identifier l;
statement S;
@@

-	ret = request_firmware(&cf, config_file, dev);
+	ret = request_firmware_direct(&cf, config_file, dev);
	if (ret < 0) {
		... when != goto l;
		    when != return ret;
		    when any
	} else {
		...
		release_firmware(cf);
		...
	}

Cc: Takashi Iwai <tiwai@suse.de>
Cc: Christian Lamparter <chunkeey@googlemail.com>
Cc: linux-wireless@vger.kernel.org
Cc: cocci@systeme.lip6.fr
Signed-off-by: Luis R. Rodriguez <mcgrof@suse.com>
---
 drivers/net/wireless/p54/p54spi.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/wireless/p54/p54spi.c b/drivers/net/wireless/p54/p54spi.c
index de15171..63de5ee 100644
--- a/drivers/net/wireless/p54/p54spi.c
+++ b/drivers/net/wireless/p54/p54spi.c
@@ -193,7 +193,7 @@ static int p54spi_request_eeprom(struct ieee80211_hw *dev)
 	/* allow users to customize their eeprom.
 	 */
 
-	ret = request_firmware(&eeprom, "3826.eeprom", &priv->spi->dev);
+	ret = request_firmware_direct(&eeprom, "3826.eeprom", &priv->spi->dev);
 	if (ret < 0) {
 #ifdef CONFIG_P54_SPI_DEFAULT_EEPROM
 		dev_info(&priv->spi->dev, "loading default eeprom...\n");
-- 
2.0.0

Re: [RESEND][PATCH 3/3] p54: use request_firmware_direct() for optional EEPROM override

[Christian Lamparter]

On Tuesday, June 24, 2014 03:39:43 PM Luis R. Rodriguez wrote:
> From: "Luis R. Rodriguez" <mcgrof@suse.com>
> The p54 driver uses request_firmware() twice, once for actual
> firmware and then another time for an optional user overide on
> EEPROM, 3826.eeprom. The custom EEPROM  is optional but if not
> present we'll introduce an extra lag of 60 seconds with udev
> present. Annotate we don't want udev nonsense here to avoid
> the lag in case its not present.
> [...]
> Cc: Takashi Iwai <tiwai@suse.de>
> Cc: Christian Lamparter <chunkeey@googlemail.com>
> Cc: linux-wireless@vger.kernel.org
> Cc: cocci@systeme.lip6.fr
> Signed-off-by: Luis R. Rodriguez <mcgrof@suse.com>
Acked-By: Christian Lamparter <chunkeey@googlemail.com>

Re: [PATCH 3/3] p54: use request_firmware_direct() for optional EEPROM override

[Arend van Spriel]

On 25-06-14 00:39, Luis R. Rodriguez wrote:
> From: "Luis R. Rodriguez" <mcgrof@suse.com>
>
> The p54 driver uses request_firmware() twice, once for actual
> firmware and then another time for an optional user overide on
> EEPROM, 3826.eeprom. The custom EEPROM  is optional but if not
> present we'll introduce an extra lag of 60 seconds with udev
> present. Annotate we don't want udev nonsense here to avoid
> the lag in case its not present.

I guess the fact that EEPROM is optional does not matter much. If doing 
a second request you could always use request_firmware_direct(), right?

Regards,
Arend

> This was found with the following SmPL patch.
>
> @ firmware_not_critical @
> expression cf;
> expression config_file;
> expression dev;
> int ret;
> identifier l;
> statement S;
> @@
>
> -	ret = request_firmware(&cf, config_file, dev);
> +	ret = request_firmware_direct(&cf, config_file, dev);
> 	if (ret < 0) {
> 		... when != goto l;
> 		    when != return ret;
> 		    when any
> 	} else {
> 		...
> 		release_firmware(cf);
> 		...
> 	}
>
> Cc: Takashi Iwai <tiwai@suse.de>
> Cc: Christian Lamparter <chunkeey@googlemail.com>
> Cc: linux-wireless@vger.kernel.org
> Cc: cocci@systeme.lip6.fr
> Signed-off-by: Luis R. Rodriguez <mcgrof@suse.com>
> ---
>   drivers/net/wireless/p54/p54spi.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/net/wireless/p54/p54spi.c b/drivers/net/wireless/p54/p54spi.c
> index de15171..63de5ee 100644
> --- a/drivers/net/wireless/p54/p54spi.c
> +++ b/drivers/net/wireless/p54/p54spi.c
> @@ -193,7 +193,7 @@ static int p54spi_request_eeprom(struct ieee80211_hw *dev)
>   	/* allow users to customize their eeprom.
>   	 */
>
> -	ret = request_firmware(&eeprom, "3826.eeprom", &priv->spi->dev);
> +	ret = request_firmware_direct(&eeprom, "3826.eeprom", &priv->spi->dev);
>   	if (ret < 0) {
>   #ifdef CONFIG_P54_SPI_DEFAULT_EEPROM
>   		dev_info(&priv->spi->dev, "loading default eeprom...\n");
>

Re: [PATCH 3/3] p54: use request_firmware_direct() for optional EEPROM override

[Luis R. Rodriguez]

On Wed, Jun 25, 2014 at 09:26:23AM +0200, Arend van Spriel wrote:
> On 25-06-14 00:39, Luis R. Rodriguez wrote:
>> From: "Luis R. Rodriguez" <mcgrof@suse.com>
>>
>> The p54 driver uses request_firmware() twice, once for actual
>> firmware and then another time for an optional user overide on
>> EEPROM, 3826.eeprom. The custom EEPROM  is optional but if not
>> present we'll introduce an extra lag of 60 seconds with udev
>> present. Annotate we don't want udev nonsense here to avoid
>> the lag in case its not present.
>
> I guess the fact that EEPROM is optional does not matter much. If doing a 
> second request you could always use request_firmware_direct(), right?

The better way to rephrase this from a technical perspective is:

I don't care about udev firmware upload as it'll be removed, and its only
adding 60 second delays. I *know* this driver doesn't require custom paths and
wierd user upload tools so lets evolve a few light years ahead and embrace
non-udev Direct upload for at least optional juju config data.

  Luis

Re: [PATCH 0/3] drivers: expand usage of request_firmware_direct()

[Takashi Iwai]

At Tue, 24 Jun 2014 15:39:40 -0700,
Luis R. Rodriguez wrote:
> 
> From: "Luis R. Rodriguez" <mcgrof@suse.com>
> 
> Takashi added request_firmware_direct() via bba3a87e9 through v3.14-rc1
> which avoids the unnecessary delay introduced by using the udev firmware
> loader in case the first try failed when loading we know loading "firmware"
> is optional. The first use case was for microcode update but if drivers are
> using it for optional configuration updates, custom EEPROMs, and other
> junk other than firmware that should apply as well as good use cases,
> specially if the driver already had a first phase in which it loaded
> the first required firmware. While reviewing one driver I figured it'd
> be best to try to give formalizing a check with SmPL. This isn't perfect
> it had 1 false possitive drivers/fmc/fmc-fakedev.c on the entire kernel
> run but my hope is this can be extended a bit more to build more
> confidence, and then perhaps stuff it as a coccicheck.
> 
> I suppose this will not be required once and if we remove
> CONFIG_FW_LOADER_USER_HELPER. Is that ever going away for good? I know
> there was a recent attempt to remove the udev loader support but
> it was unclear if the special alternative helper support would be
> removed upstream from the kernel.

Actually a few weeks ago I sent a patch to make request_firmware()
with usermode helper explicitly to be used by some drivers (like
dell-rbu).  I hope Greg took it for 3.17.  Once when this patch is in,
distros can turn off the usermode helper fallback gracefully, so no
ugly timeout issue shouldn't happen.


Takashi

Re: [PATCH 0/3] drivers: expand usage of request_firmware_direct()

[Greg KH]

On Thu, Jun 26, 2014 at 06:18:05PM +0200, Takashi Iwai wrote:
> At Tue, 24 Jun 2014 15:39:40 -0700,
> Luis R. Rodriguez wrote:
> > 
> > From: "Luis R. Rodriguez" <mcgrof@suse.com>
> > 
> > Takashi added request_firmware_direct() via bba3a87e9 through v3.14-rc1
> > which avoids the unnecessary delay introduced by using the udev firmware
> > loader in case the first try failed when loading we know loading "firmware"
> > is optional. The first use case was for microcode update but if drivers are
> > using it for optional configuration updates, custom EEPROMs, and other
> > junk other than firmware that should apply as well as good use cases,
> > specially if the driver already had a first phase in which it loaded
> > the first required firmware. While reviewing one driver I figured it'd
> > be best to try to give formalizing a check with SmPL. This isn't perfect
> > it had 1 false possitive drivers/fmc/fmc-fakedev.c on the entire kernel
> > run but my hope is this can be extended a bit more to build more
> > confidence, and then perhaps stuff it as a coccicheck.
> > 
> > I suppose this will not be required once and if we remove
> > CONFIG_FW_LOADER_USER_HELPER. Is that ever going away for good? I know
> > there was a recent attempt to remove the udev loader support but
> > it was unclear if the special alternative helper support would be
> > removed upstream from the kernel.
> 
> Actually a few weeks ago I sent a patch to make request_firmware()
> with usermode helper explicitly to be used by some drivers (like
> dell-rbu).  I hope Greg took it for 3.17.  Once when this patch is in,
> distros can turn off the usermode helper fallback gracefully, so no
> ugly timeout issue shouldn't happen.

I'll take that in a few days, been traveling at the moment...

thanks,

greg k-h

Re: [PATCH 0/3] drivers: expand usage of request_firmware_direct()

[Greg KH]

On Thu, Jun 26, 2014 at 06:18:05PM +0200, Takashi Iwai wrote:
> At Tue, 24 Jun 2014 15:39:40 -0700,
> Luis R. Rodriguez wrote:
> > 
> > From: "Luis R. Rodriguez" <mcgrof@suse.com>
> > 
> > Takashi added request_firmware_direct() via bba3a87e9 through v3.14-rc1
> > which avoids the unnecessary delay introduced by using the udev firmware
> > loader in case the first try failed when loading we know loading "firmware"
> > is optional. The first use case was for microcode update but if drivers are
> > using it for optional configuration updates, custom EEPROMs, and other
> > junk other than firmware that should apply as well as good use cases,
> > specially if the driver already had a first phase in which it loaded
> > the first required firmware. While reviewing one driver I figured it'd
> > be best to try to give formalizing a check with SmPL. This isn't perfect
> > it had 1 false possitive drivers/fmc/fmc-fakedev.c on the entire kernel
> > run but my hope is this can be extended a bit more to build more
> > confidence, and then perhaps stuff it as a coccicheck.
> > 
> > I suppose this will not be required once and if we remove
> > CONFIG_FW_LOADER_USER_HELPER. Is that ever going away for good? I know
> > there was a recent attempt to remove the udev loader support but
> > it was unclear if the special alternative helper support would be
> > removed upstream from the kernel.
> 
> Actually a few weeks ago I sent a patch to make request_firmware()
> with usermode helper explicitly to be used by some drivers (like
> dell-rbu).  I hope Greg took it for 3.17.  Once when this patch is in,
> distros can turn off the usermode helper fallback gracefully, so no
> ugly timeout issue shouldn't happen.

That patch is now merged, so this series should not be needed anymore,
right?

thanks,

greg k-h

Re: [PATCH 0/3] drivers: expand usage of request_firmware_direct()

[Luis R. Rodriguez]

On Tue, Jul 08, 2014 at 03:25:36PM -0700, Greg KH wrote:
> On Thu, Jun 26, 2014 at 06:18:05PM +0200, Takashi Iwai wrote:
> > At Tue, 24 Jun 2014 15:39:40 -0700,
> > Luis R. Rodriguez wrote:
> > > 
> > > From: "Luis R. Rodriguez" <mcgrof@suse.com>
> > > 
> > > Takashi added request_firmware_direct() via bba3a87e9 through v3.14-rc1
> > > which avoids the unnecessary delay introduced by using the udev firmware
> > > loader in case the first try failed when loading we know loading "firmware"
> > > is optional. The first use case was for microcode update but if drivers are
> > > using it for optional configuration updates, custom EEPROMs, and other
> > > junk other than firmware that should apply as well as good use cases,
> > > specially if the driver already had a first phase in which it loaded
> > > the first required firmware. While reviewing one driver I figured it'd
> > > be best to try to give formalizing a check with SmPL. This isn't perfect
> > > it had 1 false possitive drivers/fmc/fmc-fakedev.c on the entire kernel
> > > run but my hope is this can be extended a bit more to build more
> > > confidence, and then perhaps stuff it as a coccicheck.
> > > 
> > > I suppose this will not be required once and if we remove
> > > CONFIG_FW_LOADER_USER_HELPER. Is that ever going away for good? I know
> > > there was a recent attempt to remove the udev loader support but
> > > it was unclear if the special alternative helper support would be
> > > removed upstream from the kernel.
> > 
> > Actually a few weeks ago I sent a patch to make request_firmware()
> > with usermode helper explicitly to be used by some drivers (like
> > dell-rbu).  I hope Greg took it for 3.17.  Once when this patch is in,
> > distros can turn off the usermode helper fallback gracefully, so no
> > ugly timeout issue shouldn't happen.
> 
> That patch is now merged, so this series should not be needed anymore,
> right?

Now that it is merged, and another patch I posted which you also merged about
printing differences, the main difference between request_firmware() and
request_firmware_direct() for distributions that did not enable the fw
loader helper is just a printk. That's all. While the difference is minor
this series addresses a few drivers that we know have firmware that is
optional, so a printk is indeed not really needed as otherwise it can confuse
users in terms of expectations. The SmPL grammar for this series could
likely be expanded to cover other uses cases but obviously this is not
critical and at best best effort. For distributions that stay in the stone age
and do not disable the fw loader helper this will speed up boot for a few use
cases. This series still applies then.

Whether or not its required or optional for firmware to be loaded for a driver
is an example small difference in specifications that I expect drivers /
subsystems to be able to make, I suspect the differences might grow in the
future so I rather keep these requirements well annonated for now. Another
example difference I am looking into is whether or not firmware should be
digitally signed. While it may be questionable whether or not this is needed
for actual firmware that runs on microprocessors some subsystems might want to
use this to abandon other udev helpers which simply throw data over, one of
which I am looking into replacing is CRDA for the regulatory database. We
recently ran into some snags when the internal regdb is used and we use a
parser, having the ability to load it directly using request_firmware_direct()
with digital signature support as an option would enable us to simplify how the
redb is used/parsed on both embedded and non-embedded systems.

  Luis

Re: [PATCH 0/3] drivers: expand usage of request_firmware_direct()

[Greg KH]

On Wed, Jul 09, 2014 at 01:52:44AM +0200, Luis R. Rodriguez wrote:
> On Tue, Jul 08, 2014 at 03:25:36PM -0700, Greg KH wrote:
> > On Thu, Jun 26, 2014 at 06:18:05PM +0200, Takashi Iwai wrote:
> > > At Tue, 24 Jun 2014 15:39:40 -0700,
> > > Luis R. Rodriguez wrote:
> > > > 
> > > > From: "Luis R. Rodriguez" <mcgrof@suse.com>
> > > > 
> > > > Takashi added request_firmware_direct() via bba3a87e9 through v3.14-rc1
> > > > which avoids the unnecessary delay introduced by using the udev firmware
> > > > loader in case the first try failed when loading we know loading "firmware"
> > > > is optional. The first use case was for microcode update but if drivers are
> > > > using it for optional configuration updates, custom EEPROMs, and other
> > > > junk other than firmware that should apply as well as good use cases,
> > > > specially if the driver already had a first phase in which it loaded
> > > > the first required firmware. While reviewing one driver I figured it'd
> > > > be best to try to give formalizing a check with SmPL. This isn't perfect
> > > > it had 1 false possitive drivers/fmc/fmc-fakedev.c on the entire kernel
> > > > run but my hope is this can be extended a bit more to build more
> > > > confidence, and then perhaps stuff it as a coccicheck.
> > > > 
> > > > I suppose this will not be required once and if we remove
> > > > CONFIG_FW_LOADER_USER_HELPER. Is that ever going away for good? I know
> > > > there was a recent attempt to remove the udev loader support but
> > > > it was unclear if the special alternative helper support would be
> > > > removed upstream from the kernel.
> > > 
> > > Actually a few weeks ago I sent a patch to make request_firmware()
> > > with usermode helper explicitly to be used by some drivers (like
> > > dell-rbu).  I hope Greg took it for 3.17.  Once when this patch is in,
> > > distros can turn off the usermode helper fallback gracefully, so no
> > > ugly timeout issue shouldn't happen.
> > 
> > That patch is now merged, so this series should not be needed anymore,
> > right?
> 
> Now that it is merged, and another patch I posted which you also merged about
> printing differences, the main difference between request_firmware() and
> request_firmware_direct() for distributions that did not enable the fw
> loader helper is just a printk. That's all. While the difference is minor
> this series addresses a few drivers that we know have firmware that is
> optional, so a printk is indeed not really needed as otherwise it can confuse
> users in terms of expectations. The SmPL grammar for this series could
> likely be expanded to cover other uses cases but obviously this is not
> critical and at best best effort. For distributions that stay in the stone age
> and do not disable the fw loader helper this will speed up boot for a few use
> cases. This series still applies then.
> 
> Whether or not its required or optional for firmware to be loaded for a driver
> is an example small difference in specifications that I expect drivers /
> subsystems to be able to make, I suspect the differences might grow in the
> future so I rather keep these requirements well annonated for now. Another
> example difference I am looking into is whether or not firmware should be
> digitally signed. While it may be questionable whether or not this is needed
> for actual firmware that runs on microprocessors some subsystems might want to
> use this to abandon other udev helpers which simply throw data over, one of
> which I am looking into replacing is CRDA for the regulatory database. We
> recently ran into some snags when the internal regdb is used and we use a
> parser, having the ability to load it directly using request_firmware_direct()
> with digital signature support as an option would enable us to simplify how the
> redb is used/parsed on both embedded and non-embedded systems.

I'm confused, do you want me to review your patches or not?

If so, care to resend them, they are now purged from my patch queue...

thanks,

greg k-h

Re: [PATCH 0/3] drivers: expand usage of request_firmware_direct()

[Luis R. Rodriguez]

On Tue, Jul 08, 2014 at 05:24:05PM -0700, Greg KH wrote:
> On Wed, Jul 09, 2014 at 01:52:44AM +0200, Luis R. Rodriguez wrote:
> > On Tue, Jul 08, 2014 at 03:25:36PM -0700, Greg KH wrote:
> > > On Thu, Jun 26, 2014 at 06:18:05PM +0200, Takashi Iwai wrote:
> > > > At Tue, 24 Jun 2014 15:39:40 -0700,
> > > > Luis R. Rodriguez wrote:
> > > > > 
> > > > > From: "Luis R. Rodriguez" <mcgrof@suse.com>
> > > > > 
> > > > > Takashi added request_firmware_direct() via bba3a87e9 through v3.14-rc1
> > > > > which avoids the unnecessary delay introduced by using the udev firmware
> > > > > loader in case the first try failed when loading we know loading "firmware"
> > > > > is optional. The first use case was for microcode update but if drivers are
> > > > > using it for optional configuration updates, custom EEPROMs, and other
> > > > > junk other than firmware that should apply as well as good use cases,
> > > > > specially if the driver already had a first phase in which it loaded
> > > > > the first required firmware. While reviewing one driver I figured it'd
> > > > > be best to try to give formalizing a check with SmPL. This isn't perfect
> > > > > it had 1 false possitive drivers/fmc/fmc-fakedev.c on the entire kernel
> > > > > run but my hope is this can be extended a bit more to build more
> > > > > confidence, and then perhaps stuff it as a coccicheck.
> > > > > 
> > > > > I suppose this will not be required once and if we remove
> > > > > CONFIG_FW_LOADER_USER_HELPER. Is that ever going away for good? I know
> > > > > there was a recent attempt to remove the udev loader support but
> > > > > it was unclear if the special alternative helper support would be
> > > > > removed upstream from the kernel.
> > > > 
> > > > Actually a few weeks ago I sent a patch to make request_firmware()
> > > > with usermode helper explicitly to be used by some drivers (like
> > > > dell-rbu).  I hope Greg took it for 3.17.  Once when this patch is in,
> > > > distros can turn off the usermode helper fallback gracefully, so no
> > > > ugly timeout issue shouldn't happen.
> > > 
> > > That patch is now merged, so this series should not be needed anymore,
> > > right?
> > 
> > Now that it is merged, and another patch I posted which you also merged about
> > printing differences, the main difference between request_firmware() and
> > request_firmware_direct() for distributions that did not enable the fw
> > loader helper is just a printk. That's all. While the difference is minor
> > this series addresses a few drivers that we know have firmware that is
> > optional, so a printk is indeed not really needed as otherwise it can confuse
> > users in terms of expectations. The SmPL grammar for this series could
> > likely be expanded to cover other uses cases but obviously this is not
> > critical and at best best effort. For distributions that stay in the stone age
> > and do not disable the fw loader helper this will speed up boot for a few use
> > cases. This series still applies then.
> > 
> > Whether or not its required or optional for firmware to be loaded for a driver
> > is an example small difference in specifications that I expect drivers /
> > subsystems to be able to make, I suspect the differences might grow in the
> > future so I rather keep these requirements well annonated for now. Another
> > example difference I am looking into is whether or not firmware should be
> > digitally signed. While it may be questionable whether or not this is needed
> > for actual firmware that runs on microprocessors some subsystems might want to
> > use this to abandon other udev helpers which simply throw data over, one of
> > which I am looking into replacing is CRDA for the regulatory database. We
> > recently ran into some snags when the internal regdb is used and we use a
> > parser, having the ability to load it directly using request_firmware_direct()
> > with digital signature support as an option would enable us to simplify how the
> > redb is used/parsed on both embedded and non-embedded systems.
> 
> I'm confused, do you want me to review your patches or not?
> 
> If so, care to resend them, they are now purged from my patch queue...

You can ignore these patches and each driver / subsystem maintainer can
merge as they see fit. The p54 patch already went in, the cxgb4 mainainers
and vub300 maintainers should review the patches for those drivers and
can decide.

  Luis