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

[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()

[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