diff mbox series

brcmfmac: firmware: Allow per-board firmware binaries

Message ID 20210711231659.255479-1-linus.walleij@linaro.org
State New
Headers show
Series brcmfmac: firmware: Allow per-board firmware binaries | expand

Commit Message

Linus Walleij July 11, 2021, 11:16 p.m. UTC 
After some crashes in the 3D engine (!) on the Samsung GT-I8530
it turns out that the main firmware file can be device dependent,
something that was previously only handled for the NVRAM
parameter file.

Rewrite the code a bit so we can a per-board suffixed firmware
binary as well, if this does not exist we fall back to the
canonical firmware name.

Example: a 4330 device with the OF board compatible is
"samsung,gavini". We will first try
"brcmfmac4330-sdio.samsung,gavini.bin" then "brcmfmac4330-sdio.bin"
if that does not work.

Cc: phone-devel@vger.kernel.org
Cc: newbyte@disroot.org
Cc: Stephan Gerhold <stephan@gerhold.net>
Signed-off-by: Linus Walleij <linus.walleij@linaro.org>

---
 .../broadcom/brcm80211/brcmfmac/firmware.c    | 53 +++++++++++++++----
 1 file changed, 42 insertions(+), 11 deletions(-)

-- 
2.31.1

Comments

Linus Walleij July 30, 2021, 9:21 a.m. UTC | #1 
On Mon, Jul 12, 2021 at 1:19 AM Linus Walleij <linus.walleij@linaro.org> wrote:

> After some crashes in the 3D engine (!) on the Samsung GT-I8530

> it turns out that the main firmware file can be device dependent,

> something that was previously only handled for the NVRAM

> parameter file.

>

> Rewrite the code a bit so we can a per-board suffixed firmware

> binary as well, if this does not exist we fall back to the

> canonical firmware name.

>

> Example: a 4330 device with the OF board compatible is

> "samsung,gavini". We will first try

> "brcmfmac4330-sdio.samsung,gavini.bin" then "brcmfmac4330-sdio.bin"

> if that does not work.

>

> Cc: phone-devel@vger.kernel.org

> Cc: newbyte@disroot.org

> Cc: Stephan Gerhold <stephan@gerhold.net>

> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>


Who can pick this up by the way?

The patch is in active use in PostmarketOS and work fine there.

Kalle?

Yours,
Linus Walleij
Kalle Valo July 30, 2021, 5:58 p.m. UTC | #2 
Linus Walleij <linus.walleij@linaro.org> writes:

> On Mon, Jul 12, 2021 at 1:19 AM Linus Walleij <linus.walleij@linaro.org> wrote:

>

>> After some crashes in the 3D engine (!) on the Samsung GT-I8530

>> it turns out that the main firmware file can be device dependent,

>> something that was previously only handled for the NVRAM

>> parameter file.

>>

>> Rewrite the code a bit so we can a per-board suffixed firmware

>> binary as well, if this does not exist we fall back to the

>> canonical firmware name.

>>

>> Example: a 4330 device with the OF board compatible is

>> "samsung,gavini". We will first try

>> "brcmfmac4330-sdio.samsung,gavini.bin" then "brcmfmac4330-sdio.bin"

>> if that does not work.

>>

>> Cc: phone-devel@vger.kernel.org

>> Cc: newbyte@disroot.org

>> Cc: Stephan Gerhold <stephan@gerhold.net>

>> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>

>

> Who can pick this up by the way?

>

> The patch is in active use in PostmarketOS and work fine there.

>

> Kalle?


Yes, if the patch is ok I can take it to wireless-drivers-next.

-- 
https://patchwork.kernel.org/project/linux-wireless/list/

https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches
Kalle Valo Aug. 1, 2021, 10:27 a.m. UTC | #3 
Linus Walleij <linus.walleij@linaro.org> wrote:

> After some crashes in the 3D engine (!) on the Samsung GT-I8530

> it turns out that the main firmware file can be device dependent,

> something that was previously only handled for the NVRAM

> parameter file.

> 

> Rewrite the code a bit so we can a per-board suffixed firmware

> binary as well, if this does not exist we fall back to the

> canonical firmware name.

> 

> Example: a 4330 device with the OF board compatible is

> "samsung,gavini". We will first try

> "brcmfmac4330-sdio.samsung,gavini.bin" then "brcmfmac4330-sdio.bin"

> if that does not work.

> 

> Cc: phone-devel@vger.kernel.org

> Cc: newbyte@disroot.org

> Cc: Stephan Gerhold <stephan@gerhold.net>

> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>


Patch applied to wireless-drivers-next.git, thanks.

5ff013914c62 brcmfmac: firmware: Allow per-board firmware binaries

-- 
https://patchwork.kernel.org/project/linux-wireless/patch/20210711231659.255479-1-linus.walleij@linaro.org/

https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches
Dmitry Osipenko Aug. 3, 2021, 3:28 p.m. UTC | #4 
12.07.2021 02:16, Linus Walleij пишет:
> After some crashes in the 3D engine (!) on the Samsung GT-I8530

> it turns out that the main firmware file can be device dependent,

> something that was previously only handled for the NVRAM

> parameter file.

> 

> Rewrite the code a bit so we can a per-board suffixed firmware

> binary as well, if this does not exist we fall back to the

> canonical firmware name.

> 

> Example: a 4330 device with the OF board compatible is

> "samsung,gavini". We will first try

> "brcmfmac4330-sdio.samsung,gavini.bin" then "brcmfmac4330-sdio.bin"

> if that does not work.

> 

> Cc: phone-devel@vger.kernel.org

> Cc: newbyte@disroot.org

> Cc: Stephan Gerhold <stephan@gerhold.net>

> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>

> ---

>  .../broadcom/brcm80211/brcmfmac/firmware.c    | 53 +++++++++++++++----

>  1 file changed, 42 insertions(+), 11 deletions(-)

> 

> diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/firmware.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/firmware.c

> index d40104b8df55..adfdfc654b10 100644

> --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/firmware.c

> +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/firmware.c

> @@ -594,28 +594,47 @@ static int brcmf_fw_complete_request(const struct firmware *fw,

>  	return (cur->flags & BRCMF_FW_REQF_OPTIONAL) ? 0 : ret;

>  }

>  

> +static char *brcm_alt_fw_path(const char *path, const char *board_type)

> +{

> +	char alt_path[BRCMF_FW_NAME_LEN];

> +	char suffix[5];

> +

> +	strscpy(alt_path, path, BRCMF_FW_NAME_LEN);

> +	/* At least one character + suffix */

> +	if (strlen(alt_path) < 5)

> +		return NULL;

> +

> +	/* strip .txt or .bin at the end */

> +	strscpy(suffix, alt_path + strlen(alt_path) - 4, 5);

> +	alt_path[strlen(alt_path) - 4] = 0;

> +	strlcat(alt_path, ".", BRCMF_FW_NAME_LEN);

> +	strlcat(alt_path, board_type, BRCMF_FW_NAME_LEN);

> +	strlcat(alt_path, suffix, BRCMF_FW_NAME_LEN);

> +

> +	return kstrdup(alt_path, GFP_KERNEL);

> +}

> +

>  static int brcmf_fw_request_firmware(const struct firmware **fw,

>  				     struct brcmf_fw *fwctx)

>  {

>  	struct brcmf_fw_item *cur = &fwctx->req->items[fwctx->curpos];

>  	int ret;

>  

> -	/* nvram files are board-specific, first try a board-specific path */

> +	/* Files can be board-specific, first try a board-specific path */

>  	if (cur->type == BRCMF_FW_TYPE_NVRAM && fwctx->req->board_type) {

> -		char alt_path[BRCMF_FW_NAME_LEN];

> +		char *alt_path;

>  

> -		strlcpy(alt_path, cur->path, BRCMF_FW_NAME_LEN);

> -		/* strip .txt at the end */

> -		alt_path[strlen(alt_path) - 4] = 0;

> -		strlcat(alt_path, ".", BRCMF_FW_NAME_LEN);

> -		strlcat(alt_path, fwctx->req->board_type, BRCMF_FW_NAME_LEN);

> -		strlcat(alt_path, ".txt", BRCMF_FW_NAME_LEN);

> +		alt_path = brcm_alt_fw_path(cur->path, fwctx->req->board_type);

> +		if (!alt_path)

> +			goto fallback;

>  

>  		ret = request_firmware(fw, alt_path, fwctx->dev);

> +		kfree(alt_path);

>  		if (ret == 0)

>  			return ret;

>  	}

>  

> +fallback:

>  	return request_firmware(fw, cur->path, fwctx->dev);

>  }

>  

> @@ -660,6 +679,7 @@ int brcmf_fw_get_firmwares(struct device *dev, struct brcmf_fw_request *req,

>  {

>  	struct brcmf_fw_item *first = &req->items[0];

>  	struct brcmf_fw *fwctx;

> +	char *alt_path;

>  	int ret;

>  

>  	brcmf_dbg(TRACE, "enter: dev=%s\n", dev_name(dev));

> @@ -677,9 +697,20 @@ int brcmf_fw_get_firmwares(struct device *dev, struct brcmf_fw_request *req,

>  	fwctx->req = req;

>  	fwctx->done = fw_cb;

>  

> -	ret = request_firmware_nowait(THIS_MODULE, true, first->path,

> -				      fwctx->dev, GFP_KERNEL, fwctx,

> -				      brcmf_fw_request_done);

> +	/* First try alternative board-specific path if any */

> +	alt_path = brcm_alt_fw_path(first->path, fwctx->req->board_type);

> +	if (alt_path) {

> +		ret = request_firmware_nowait(THIS_MODULE, true, alt_path,

> +					      fwctx->dev, GFP_KERNEL, fwctx,

> +					      brcmf_fw_request_done);

> +		kfree(alt_path);

> +	}

> +	/* Else try canonical path */

> +	if (ret) {

> +		ret = request_firmware_nowait(THIS_MODULE, true, first->path,

> +					      fwctx->dev, GFP_KERNEL, fwctx,

> +					      brcmf_fw_request_done);

> +	}

>  	if (ret < 0)

>  		brcmf_fw_request_done(NULL, fwctx);

>  

> 


Hi,

I'm getting this error on Nexus 7 (bcm4330) and Acer A500 (bcm4329)
using today's -next, WiFi doesn't work at all:

brcmfmac: brcmf_sdio_htclk: HT Avail timeout (1000000): clkctl 0x50

Reverting this patch helps, any ideas? Maybe the missing firmware
creates the offending delay somewhere. Adding the per-board FW file
makes WiFi to work, there is no error message.

Linus, have you tested this patch without the per-board file?

BTW, we also need the per-board FW for Nexus 7 because stock bcm4330
firmware doesn't work properly on Nexus. It sees networks, but can't
connect anywhere.

I'm also aware about Lenovo tablet with bcm4330 that doesn't work at all
until Bluetooth FW is loaded. So the FW support status is not ideal yet,
but the per-board FW is a step forward!
Dmitry Osipenko Aug. 3, 2021, 3:53 p.m. UTC | #5 
03.08.2021 18:28, Dmitry Osipenko пишет:
> 12.07.2021 02:16, Linus Walleij пишет:

>> After some crashes in the 3D engine (!) on the Samsung GT-I8530

>> it turns out that the main firmware file can be device dependent,

>> something that was previously only handled for the NVRAM

>> parameter file.

>>

>> Rewrite the code a bit so we can a per-board suffixed firmware

>> binary as well, if this does not exist we fall back to the

>> canonical firmware name.

>>

>> Example: a 4330 device with the OF board compatible is

>> "samsung,gavini". We will first try

>> "brcmfmac4330-sdio.samsung,gavini.bin" then "brcmfmac4330-sdio.bin"

>> if that does not work.

>>

>> Cc: phone-devel@vger.kernel.org

>> Cc: newbyte@disroot.org

>> Cc: Stephan Gerhold <stephan@gerhold.net>

>> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>

>> ---

>>  .../broadcom/brcm80211/brcmfmac/firmware.c    | 53 +++++++++++++++----

>>  1 file changed, 42 insertions(+), 11 deletions(-)

>>

>> diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/firmware.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/firmware.c

>> index d40104b8df55..adfdfc654b10 100644

>> --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/firmware.c

>> +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/firmware.c

>> @@ -594,28 +594,47 @@ static int brcmf_fw_complete_request(const struct firmware *fw,

>>  	return (cur->flags & BRCMF_FW_REQF_OPTIONAL) ? 0 : ret;

>>  }

>>  

>> +static char *brcm_alt_fw_path(const char *path, const char *board_type)

>> +{

>> +	char alt_path[BRCMF_FW_NAME_LEN];

>> +	char suffix[5];

>> +

>> +	strscpy(alt_path, path, BRCMF_FW_NAME_LEN);

>> +	/* At least one character + suffix */

>> +	if (strlen(alt_path) < 5)

>> +		return NULL;

>> +

>> +	/* strip .txt or .bin at the end */

>> +	strscpy(suffix, alt_path + strlen(alt_path) - 4, 5);

>> +	alt_path[strlen(alt_path) - 4] = 0;

>> +	strlcat(alt_path, ".", BRCMF_FW_NAME_LEN);

>> +	strlcat(alt_path, board_type, BRCMF_FW_NAME_LEN);

>> +	strlcat(alt_path, suffix, BRCMF_FW_NAME_LEN);

>> +

>> +	return kstrdup(alt_path, GFP_KERNEL);

>> +}

>> +

>>  static int brcmf_fw_request_firmware(const struct firmware **fw,

>>  				     struct brcmf_fw *fwctx)

>>  {

>>  	struct brcmf_fw_item *cur = &fwctx->req->items[fwctx->curpos];

>>  	int ret;

>>  

>> -	/* nvram files are board-specific, first try a board-specific path */

>> +	/* Files can be board-specific, first try a board-specific path */

>>  	if (cur->type == BRCMF_FW_TYPE_NVRAM && fwctx->req->board_type) {

>> -		char alt_path[BRCMF_FW_NAME_LEN];

>> +		char *alt_path;

>>  

>> -		strlcpy(alt_path, cur->path, BRCMF_FW_NAME_LEN);

>> -		/* strip .txt at the end */

>> -		alt_path[strlen(alt_path) - 4] = 0;

>> -		strlcat(alt_path, ".", BRCMF_FW_NAME_LEN);

>> -		strlcat(alt_path, fwctx->req->board_type, BRCMF_FW_NAME_LEN);

>> -		strlcat(alt_path, ".txt", BRCMF_FW_NAME_LEN);

>> +		alt_path = brcm_alt_fw_path(cur->path, fwctx->req->board_type);

>> +		if (!alt_path)

>> +			goto fallback;

>>  

>>  		ret = request_firmware(fw, alt_path, fwctx->dev);

>> +		kfree(alt_path);

>>  		if (ret == 0)

>>  			return ret;

>>  	}

>>  

>> +fallback:

>>  	return request_firmware(fw, cur->path, fwctx->dev);

>>  }

>>  

>> @@ -660,6 +679,7 @@ int brcmf_fw_get_firmwares(struct device *dev, struct brcmf_fw_request *req,

>>  {

>>  	struct brcmf_fw_item *first = &req->items[0];

>>  	struct brcmf_fw *fwctx;

>> +	char *alt_path;

>>  	int ret;

>>  

>>  	brcmf_dbg(TRACE, "enter: dev=%s\n", dev_name(dev));

>> @@ -677,9 +697,20 @@ int brcmf_fw_get_firmwares(struct device *dev, struct brcmf_fw_request *req,

>>  	fwctx->req = req;

>>  	fwctx->done = fw_cb;

>>  

>> -	ret = request_firmware_nowait(THIS_MODULE, true, first->path,

>> -				      fwctx->dev, GFP_KERNEL, fwctx,

>> -				      brcmf_fw_request_done);

>> +	/* First try alternative board-specific path if any */

>> +	alt_path = brcm_alt_fw_path(first->path, fwctx->req->board_type);

>> +	if (alt_path) {

>> +		ret = request_firmware_nowait(THIS_MODULE, true, alt_path,

>> +					      fwctx->dev, GFP_KERNEL, fwctx,

>> +					      brcmf_fw_request_done);


This return 0 immediately, despite of the missing firmware file. It's
not a blocking FW request.
Linus Walleij Aug. 3, 2021, 11:28 p.m. UTC | #6 
On Tue, Aug 3, 2021 at 5:53 PM Dmitry Osipenko <digetx@gmail.com> wrote:

> >> +            ret = request_firmware_nowait(THIS_MODULE, true, alt_path,

> >> +                                          fwctx->dev, GFP_KERNEL, fwctx,

> >> +                                          brcmf_fw_request_done);

>

> This return 0 immediately, despite of the missing firmware file. It's

> not a blocking FW request.


Ooops I see the bug. I've sent a patch, please test!

Thanks for finding this!
Linus Walleij
Arend Van Spriel Aug. 4, 2021, 8:48 a.m. UTC | #7 
On August 3, 2021 5:53:14 PM Dmitry Osipenko <digetx@gmail.com> wrote:

> 03.08.2021 18:28, Dmitry Osipenko пишет:

>> 12.07.2021 02:16, Linus Walleij пишет:

>>> After some crashes in the 3D engine (!) on the Samsung GT-I8530

>>> it turns out that the main firmware file can be device dependent,

>>> something that was previously only handled for the NVRAM

>>> parameter file.

>>>

>>> Rewrite the code a bit so we can a per-board suffixed firmware

>>> binary as well, if this does not exist we fall back to the

>>> canonical firmware name.

>>>

>>> Example: a 4330 device with the OF board compatible is

>>> "samsung,gavini". We will first try

>>> "brcmfmac4330-sdio.samsung,gavini.bin" then "brcmfmac4330-sdio.bin"

>>> if that does not work.

>>>

>>> Cc: phone-devel@vger.kernel.org

>>> Cc: newbyte@disroot.org

>>> Cc: Stephan Gerhold <stephan@gerhold.net>

>>> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>

>>> ---

>>> .../broadcom/brcm80211/brcmfmac/firmware.c    | 53 +++++++++++++++----

>>> 1 file changed, 42 insertions(+), 11 deletions(-)

>>>

>>> diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/firmware.c 

>>> b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/firmware.c

>>> index d40104b8df55..adfdfc654b10 100644

>>> --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/firmware.c

>>> +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/firmware.c

>>> @@ -594,28 +594,47 @@ static int brcmf_fw_complete_request(const struct 

>>> firmware *fw,

>>> return (cur->flags & BRCMF_FW_REQF_OPTIONAL) ? 0 : ret;

>>> }

>>>

>>> +static char *brcm_alt_fw_path(const char *path, const char *board_type)

>>> +{

>>> + char alt_path[BRCMF_FW_NAME_LEN];

>>> + char suffix[5];

>>> +

>>> + strscpy(alt_path, path, BRCMF_FW_NAME_LEN);

>>> + /* At least one character + suffix */

>>> + if (strlen(alt_path) < 5)

>>> + return NULL;

>>> +

>>> + /* strip .txt or .bin at the end */

>>> + strscpy(suffix, alt_path + strlen(alt_path) - 4, 5);

>>> + alt_path[strlen(alt_path) - 4] = 0;

>>> + strlcat(alt_path, ".", BRCMF_FW_NAME_LEN);

>>> + strlcat(alt_path, board_type, BRCMF_FW_NAME_LEN);

>>> + strlcat(alt_path, suffix, BRCMF_FW_NAME_LEN);

>>> +

>>> + return kstrdup(alt_path, GFP_KERNEL);

>>> +}

>>> +

>>> static int brcmf_fw_request_firmware(const struct firmware **fw,

>>>   struct brcmf_fw *fwctx)

>>> {

>>> struct brcmf_fw_item *cur = &fwctx->req->items[fwctx->curpos];

>>> int ret;

>>>

>>> - /* nvram files are board-specific, first try a board-specific path */

>>> + /* Files can be board-specific, first try a board-specific path */

>>> if (cur->type == BRCMF_FW_TYPE_NVRAM && fwctx->req->board_type) {

>>> - char alt_path[BRCMF_FW_NAME_LEN];

>>> + char *alt_path;

>>>

>>> - strlcpy(alt_path, cur->path, BRCMF_FW_NAME_LEN);

>>> - /* strip .txt at the end */

>>> - alt_path[strlen(alt_path) - 4] = 0;

>>> - strlcat(alt_path, ".", BRCMF_FW_NAME_LEN);

>>> - strlcat(alt_path, fwctx->req->board_type, BRCMF_FW_NAME_LEN);

>>> - strlcat(alt_path, ".txt", BRCMF_FW_NAME_LEN);

>>> + alt_path = brcm_alt_fw_path(cur->path, fwctx->req->board_type);

>>> + if (!alt_path)

>>> + goto fallback;

>>>

>>> ret = request_firmware(fw, alt_path, fwctx->dev);

>>> + kfree(alt_path);

>>> if (ret == 0)

>>> return ret;

>>> }

>>>

>>> +fallback:

>>> return request_firmware(fw, cur->path, fwctx->dev);

>>> }

>>>

>>> @@ -660,6 +679,7 @@ int brcmf_fw_get_firmwares(struct device *dev, struct 

>>> brcmf_fw_request *req,

>>> {

>>> struct brcmf_fw_item *first = &req->items[0];

>>> struct brcmf_fw *fwctx;

>>> + char *alt_path;

>>> int ret;

>>>

>>> brcmf_dbg(TRACE, "enter: dev=%s\n", dev_name(dev));

>>> @@ -677,9 +697,20 @@ int brcmf_fw_get_firmwares(struct device *dev, struct 

>>> brcmf_fw_request *req,

>>> fwctx->req = req;

>>> fwctx->done = fw_cb;

>>>

>>> - ret = request_firmware_nowait(THIS_MODULE, true, first->path,

>>> -      fwctx->dev, GFP_KERNEL, fwctx,

>>> -      brcmf_fw_request_done);

>>> + /* First try alternative board-specific path if any */

>>> + alt_path = brcm_alt_fw_path(first->path, fwctx->req->board_type);

>>> + if (alt_path) {

>>> + ret = request_firmware_nowait(THIS_MODULE, true, alt_path,

>>> +      fwctx->dev, GFP_KERNEL, fwctx,

>>> +      brcmf_fw_request_done);

>

> This return 0 immediately, despite of the missing firmware file. It's

> not a blocking FW request.


Right. I didn't get to looking at this earlier, but indeed the check 
whether the requested firmware exists is done in another thread context 
so the return value here only indicates whether the firmware request 
could be scheduled or not.

My first reaction to the patch was to reject it, but leaning towards 
supporting this now. OEMs tend to get tailor-made firmware in terms of 
features. Depending on their requirements they get their mix of firmware 
features. That such difference lead to a crash in 3d engine is somewhat 
surprising. I am curious if we can debug this more and learn how a 
firmware variant could cause such a crash. Maybe some DMA issue?

Regards,
Arend
Linus Walleij Aug. 4, 2021, 9:35 a.m. UTC | #8 
On Wed, Aug 4, 2021 at 10:48 AM Arend van Spriel
<arend.vanspriel@broadcom.com> wrote:

> Right. I didn't get to looking at this earlier, but indeed the check

> whether the requested firmware exists is done in another thread context

> so the return value here only indicates whether the firmware request

> could be scheduled or not.


I think my recent patch fixes is, please have a look.

> My first reaction to the patch was to reject it, but leaning towards

> supporting this now. OEMs tend to get tailor-made firmware in terms of

> features. Depending on their requirements they get their mix of firmware

> features. That such difference lead to a crash in 3d engine is somewhat

> surprising. I am curious if we can debug this more and learn how a

> firmware variant could cause such a crash. Maybe some DMA issue?


I am not certain what happens, but I think the 3D engine misses its
interrupts. This may in turn be because GPIO IRQs are held
low or fireing repeatedly for an extensive period of time, stressing
the system to the point that other important IRQs are missed.

This in turn can be caused by the wrong (non-custom) firmware
managing these GPIO IRQs fireing left and right.

I have noticed that the config files for brcmfmac contain words
about GPIOs and so on and that is what makes me think this way.

I can tell for sure that brcmfmac has definately had special
firmware tailored by/for Samsung for these phones. We can just
look at the files extracted from the platforms (the original
files are named bcmdhd_sta.bin_b2 or similar):

BRCMFMAC 4330
-rw-r--r--. 1 linus linus 213390 Mar 22 23:32
brcmfmac4330-sdio.samsung,janice.bin
-rw-r--r--. 1 linus linus 203593 Jul 11 01:53
brcmfmac4330-sdio.samsung,codina.bin
-rw-r--r--. 1 linus linus 212956 Mar 22 23:31
brcmfmac4330-sdio.samsung,gavini.bin

BRCMFMAC 4334
-rw-r--r--. 1 linus linus 346151 Mar 16 22:53
brcmfmac4334-sdio.samsung,golden.bin
-rw-r--r--. 1 linus linus 434236 Jul  7 00:43 brcmfmac4334-sdio.samsung,kyle.bin
-rw-r--r--. 1 linus linus 434236 Mar 16 22:54
brcmfmac4334-sdio.samsung,skomer.bin

All different file sizes, except Kyle and Skomer, who actually share
the same firmware. (Those were the two last phones produced
in this series BTW.) Doing strings * on each file reveals that they
were compiled at different dates around the time these phones
were produced.

These are all for standard WiFi functionality. There is two more
firmwares for each phone, one for the access point usecase and
one more which I don't know what it is for, the actual set of firmware
for each phone is for example (Skomer):

bcmdhd_apsta.bin_b2
bcmdhd_mfg.bin_b2
bcmdhd_p2p.bin_b2
bcmdhd_sta.bin_b2

So I am half-guessing that bcmdhd_sta.bin_b2 is obviously for the
ordinary use case, *mfg* is probably for manufacturing, *apsta*
for mobile hotspot (access point) and *p2p* for some other cool
thing where phones do peer-to-peer.

If you could shed some light on the above it'd be great :)

Yours,
Linus Walleij
Arend Van Spriel Aug. 4, 2021, 11:32 a.m. UTC | #9 
On 04-08-2021 11:35, Linus Walleij wrote:
> On Wed, Aug 4, 2021 at 10:48 AM Arend van Spriel

> <arend.vanspriel@broadcom.com> wrote:

>

>> Right. I didn't get to looking at this earlier, but indeed the check

>> whether the requested firmware exists is done in another thread context

>> so the return value here only indicates whether the firmware request

>> could be scheduled or not.

>

> I think my recent patch fixes is, please have a look.


Right. I want to explore another option for myself, but I will first 
look at your patch so we can fix the current issue in timely manner.

>> My first reaction to the patch was to reject it, but leaning towards

>> supporting this now. OEMs tend to get tailor-made firmware in terms of

>> features. Depending on their requirements they get their mix of firmware

>> features. That such difference lead to a crash in 3d engine is somewhat

>> surprising. I am curious if we can debug this more and learn how a

>> firmware variant could cause such a crash. Maybe some DMA issue?

>

> I am not certain what happens, but I think the 3D engine misses its

> interrupts. This may in turn be because GPIO IRQs are held

> low or fireing repeatedly for an extensive period of time, stressing

> the system to the point that other important IRQs are missed.

>

> This in turn can be caused by the wrong (non-custom) firmware

> managing these GPIO IRQs fireing left and right.

>

> I have noticed that the config files for brcmfmac contain words

> about GPIOs and so on and that is what makes me think this way.


Not sure what config files you refer to. I am only aware of the device 
tree bindings mentioning GPIO for out-of-band SDIO interrupt.

> I can tell for sure that brcmfmac has definately had special

> firmware tailored by/for Samsung for these phones. We can just

> look at the files extracted from the platforms (the original

> files are named bcmdhd_sta.bin_b2 or similar):

>

> BRCMFMAC 4330

> -rw-r--r--. 1 linus linus 213390 Mar 22 23:32

> brcmfmac4330-sdio.samsung,janice.bin

> -rw-r--r--. 1 linus linus 203593 Jul 11 01:53

> brcmfmac4330-sdio.samsung,codina.bin

> -rw-r--r--. 1 linus linus 212956 Mar 22 23:31

> brcmfmac4330-sdio.samsung,gavini.bin

>

> BRCMFMAC 4334

> -rw-r--r--. 1 linus linus 346151 Mar 16 22:53

> brcmfmac4334-sdio.samsung,golden.bin

> -rw-r--r--. 1 linus linus 434236 Jul  7 00:43 brcmfmac4334-sdio.samsung,kyle.bin

> -rw-r--r--. 1 linus linus 434236 Mar 16 22:54

> brcmfmac4334-sdio.samsung,skomer.bin

>

> All different file sizes, except Kyle and Skomer, who actually share

> the same firmware. (Those were the two last phones produced

> in this series BTW.) Doing strings * on each file reveals that they

> were compiled at different dates around the time these phones

> were produced.


As said earlier customers get their mix of firmware features. Apart from 
the compile date using strings will also give you the firmware compile 
target, ie. 4330*-roml/... Could you share that?

> These are all for standard WiFi functionality. There is two more

> firmwares for each phone, one for the access point usecase and

> one more which I don't know what it is for, the actual set of firmware

> for each phone is for example (Skomer):

>

> bcmdhd_apsta.bin_b2

> bcmdhd_mfg.bin_b2

> bcmdhd_p2p.bin_b2

> bcmdhd_sta.bin_b2

>

> So I am half-guessing that bcmdhd_sta.bin_b2 is obviously for the

> ordinary use case, *mfg* is probably for manufacturing, *apsta*

> for mobile hotspot (access point) and *p2p* for some other cool


Half-guessing seems sufficient ;-) If I recall correctly on Android the 
driver loads different firmware based on what a user selects in the gui. 
Not something we do in upstream linux. p2p is a WFA specification and 
supported in upstream linux cfg80211. Many TV sets support it to show 
content from your portable device.

Regards,
Arend
Linus Walleij Aug. 4, 2021, 7:18 p.m. UTC | #10 
On Wed, Aug 4, 2021 at 1:32 PM Arend van Spriel
<arend.vanspriel@broadcom.com> wrote:

> Apart from

> the compile date using strings will also give you the firmware compile

> target, ie. 4330*-roml/... Could you share that?


I have this:

BCM4330 devices:
Janice:
*unknown*/sdio-ag-p2p-aoe-pktfilter-keepalive-pno-*unknown* Version:
5.99.7.0 CRC: 76bfb595 Date: Thu 2012-11-15 16:51:35 KST
Codina:
4330b2-roml/sdio-g-p2p-aoe-pktfilter-keepalive-pno-ccx-wepso Version:
5.99.10.0 CRC: 4f7fccf Date: Wed 2012-12-05 01:02:50 PST FWID
01-52653ba9
Gavini:
*unknown*/sdio-g-apsta-idsup-idauth-aoe-pktfilter-keepalive-btamp-p2p-pno-ccx
Version: 5.90.125.169 CRC: 323b9dfb Date: Tue 2012-03-27 16:27:59 KST

BCM4334 devices:
Golden:
4334b1-roml/sdio-ag-p2p-extsup-aoe-pktfilter-keepalive-pno-dmatxrc-rxov-proptxstatus-vsdb-mchan-okc-rcc-fmc-wepso-txpwr-autoabn-sr
Version: 6.10.58.99 CRC: 828f9174 Date: Mon 2013-08-26 02:13:44 PDT
FWID 01-e39d4d77
Skomer:
*unknown*/sdio-g-p2p-extsup-aoe-pktfilter-keepalive-pno-*unknown*-*unknown*-proptxstatus-*unknown*-*unknown*-*unknown*-*unknown*-*unknown*-*unknown*
Version: 6.10.58.45 CRC: 7d0fc51b Date: Wed 2012-11-21 00:22:18 KST
Kyle:
*unknown*/sdio-g-p2p-extsup-aoe-pktfilter-keepalive-pno-*unknown*-*unknown*-proptxstatus-*unknown*-*unknown*-*unknown*-*unknown*-*unknown*-*unknown*
Version: 6.10.58.45 CRC: 7d0fc51b Date: Wed 2012-11-21 00:22:18 KST

> > So I am half-guessing that bcmdhd_sta.bin_b2 is obviously for the

> > ordinary use case, *mfg* is probably for manufacturing, *apsta*

> > for mobile hotspot (access point) and *p2p* for some other cool

>

> Half-guessing seems sufficient ;-) If I recall correctly on Android the

> driver loads different firmware based on what a user selects in the gui.

> Not something we do in upstream linux. p2p is a WFA specification and

> supported in upstream linux cfg80211. Many TV sets support it to show

> content from your portable device.


Aha makes perfect sense!

Yours,
Linus Walleij
diff mbox series

Patch

diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/firmware.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/firmware.c
index d40104b8df55..adfdfc654b10 100644
--- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/firmware.c
+++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/firmware.c
@@ -594,28 +594,47 @@  static int brcmf_fw_complete_request(const struct firmware *fw,
 	return (cur->flags & BRCMF_FW_REQF_OPTIONAL) ? 0 : ret;
 }
 
+static char *brcm_alt_fw_path(const char *path, const char *board_type)
+{
+	char alt_path[BRCMF_FW_NAME_LEN];
+	char suffix[5];
+
+	strscpy(alt_path, path, BRCMF_FW_NAME_LEN);
+	/* At least one character + suffix */
+	if (strlen(alt_path) < 5)
+		return NULL;
+
+	/* strip .txt or .bin at the end */
+	strscpy(suffix, alt_path + strlen(alt_path) - 4, 5);
+	alt_path[strlen(alt_path) - 4] = 0;
+	strlcat(alt_path, ".", BRCMF_FW_NAME_LEN);
+	strlcat(alt_path, board_type, BRCMF_FW_NAME_LEN);
+	strlcat(alt_path, suffix, BRCMF_FW_NAME_LEN);
+
+	return kstrdup(alt_path, GFP_KERNEL);
+}
+
 static int brcmf_fw_request_firmware(const struct firmware **fw,
 				     struct brcmf_fw *fwctx)
 {
 	struct brcmf_fw_item *cur = &fwctx->req->items[fwctx->curpos];
 	int ret;
 
-	/* nvram files are board-specific, first try a board-specific path */
+	/* Files can be board-specific, first try a board-specific path */
 	if (cur->type == BRCMF_FW_TYPE_NVRAM && fwctx->req->board_type) {
-		char alt_path[BRCMF_FW_NAME_LEN];
+		char *alt_path;
 
-		strlcpy(alt_path, cur->path, BRCMF_FW_NAME_LEN);
-		/* strip .txt at the end */
-		alt_path[strlen(alt_path) - 4] = 0;
-		strlcat(alt_path, ".", BRCMF_FW_NAME_LEN);
-		strlcat(alt_path, fwctx->req->board_type, BRCMF_FW_NAME_LEN);
-		strlcat(alt_path, ".txt", BRCMF_FW_NAME_LEN);
+		alt_path = brcm_alt_fw_path(cur->path, fwctx->req->board_type);
+		if (!alt_path)
+			goto fallback;
 
 		ret = request_firmware(fw, alt_path, fwctx->dev);
+		kfree(alt_path);
 		if (ret == 0)
 			return ret;
 	}
 
+fallback:
 	return request_firmware(fw, cur->path, fwctx->dev);
 }
 
@@ -660,6 +679,7 @@  int brcmf_fw_get_firmwares(struct device *dev, struct brcmf_fw_request *req,
 {
 	struct brcmf_fw_item *first = &req->items[0];
 	struct brcmf_fw *fwctx;
+	char *alt_path;
 	int ret;
 
 	brcmf_dbg(TRACE, "enter: dev=%s\n", dev_name(dev));
@@ -677,9 +697,20 @@  int brcmf_fw_get_firmwares(struct device *dev, struct brcmf_fw_request *req,
 	fwctx->req = req;
 	fwctx->done = fw_cb;
 
-	ret = request_firmware_nowait(THIS_MODULE, true, first->path,
-				      fwctx->dev, GFP_KERNEL, fwctx,
-				      brcmf_fw_request_done);
+	/* First try alternative board-specific path if any */
+	alt_path = brcm_alt_fw_path(first->path, fwctx->req->board_type);
+	if (alt_path) {
+		ret = request_firmware_nowait(THIS_MODULE, true, alt_path,
+					      fwctx->dev, GFP_KERNEL, fwctx,
+					      brcmf_fw_request_done);
+		kfree(alt_path);
+	}
+	/* Else try canonical path */
+	if (ret) {
+		ret = request_firmware_nowait(THIS_MODULE, true, first->path,
+					      fwctx->dev, GFP_KERNEL, fwctx,
+					      brcmf_fw_request_done);
+	}
 	if (ret < 0)
 		brcmf_fw_request_done(NULL, fwctx);