Re: [PATCH 9/9] brcmfmac: use request_firmware_nowait2 to load firmware without warnings

[Andres Rodriguez]

On 2018-04-20 06:26 AM, Kalle Valo wrote:
> Andres Rodriguez <andresx7@xxxxxxxxx> writes:
>
> > This reduces the unnecessary spew when trying to load optional firmware:
> > "Direct firmware load for ... failed with error -2"
> >
> > Signed-off-by: Andres Rodriguez <andresx7@xxxxxxxxx>
> > ---
> >   drivers/net/wireless/broadcom/brcm80211/brcmfmac/firmware.c | 7 ++++---
> >   1 file changed, 4 insertions(+), 3 deletions(-)
>
> With wireless patches always CC linux-wireless list, please. Adding it
> now.
>
> > diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/firmware.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/firmware.c
> > index 091b52979e03..26db3ebd52dc 100644
> > --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/firmware.c
> > +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/firmware.c
> > @@ -503,8 +503,9 @@ static void brcmf_fw_request_code_done(const struct firmware *fw, void *ctx)
> >   		goto done;
> >   
> >   	fwctx->code = fw;
> > -	ret = request_firmware_nowait(THIS_MODULE, true, fwctx->nvram_name,
> > -				      fwctx->dev, GFP_KERNEL, fwctx,
> > +	ret = request_firmware_nowait(THIS_MODULE, true, false,
>
> A perfect example why enums should be in function calls instead of
> booleans, that "true, false" tells nothing to me and it would be time
> consuming to check from headers files what it means. If you had proper
> enums, for example "FIRMWARE_MODE_FOO, FIRMWARE_STATE_BAR", it would be
> immediately obvious for the reader what the parameters are. Of course
> the first boolean was already there before, but maybe change the new
> boolean to an enum >

Anyone else got any feedback before I re-spin the _nowait() API. I'm on 
board for the boolean->enum change.


> > +				      fwctx->nvram_name, fwctx->dev,
> > +				      GFP_KERNEL, fwctx,
> >   				      brcmf_fw_request_nvram_done);
> >   
> >   	/* pass NULL to nvram callback for bcm47xx fallback */
> > @@ -547,7 +548,7 @@ int brcmf_fw_get_firmwares_pcie(struct device *dev, u16 flags,
> >   	fwctx->domain_nr = domain_nr;
> >   	fwctx->bus_nr = bus_nr;
> >   
> > -	return request_firmware_nowait(THIS_MODULE, true, code, dev,
> > +	return request_firmware_nowait2(THIS_MODULE, true, false, code, dev,
> >   				       GFP_KERNEL, fwctx,
> >   				       brcmf_fw_request_code_done);
> >   }
>
> Also the number two in the function name is not really telling anything.
> I think that something like request_firmware_nowait_nowarn() would be
> better, even if it's so ugly.

The 2 is meant to signify that this is an new version of the api with 
different parameters. I don't think we need to codify into the name what 
the new parameters mean (mostly because when a 2 version of an api is 
implemented, usage of the original version tends to be discouraged).

If we go for something like _nowait_nowarn(), then we would need to drop 
the warn parameter altogether.

Another alternative, drop both bool warn and bool uevent and expose take 
in enum fw_opt directly.

Any thought on exposing the enum directly Luis for _nowait(). I know you 
mentioned this was for some reason decided against for the rest of the API.

Regards,
Andres