diff mbox

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

Message ID 1403649583-12707-4-git-send-email-mcgrof@do-not-panic.com
State Not Applicable, archived
Delegated to: David Miller
Headers show

Commit Message

Luis R. Rodriguez June 24, 2014, 10:39 p.m. UTC
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(-)

Comments

Christian Lamparter June 25, 2014, 1:10 a.m. UTC | #1
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>

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Arend van Spriel June 25, 2014, 7:26 a.m. UTC | #2
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");
>

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Luis R. Rodriguez June 25, 2014, 8:06 a.m. UTC | #3
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
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

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");