* [RFT 0/3] brcmfmac: firmware loading rework
@ 2017-05-30 12:35 Arend van Spriel
2017-05-30 12:35 ` [RFT 1/3] brcmfmac: add parameter to pass error code in firmware callback Arend van Spriel
` (3 more replies)
0 siblings, 4 replies; 10+ messages in thread
From: Arend van Spriel @ 2017-05-30 12:35 UTC (permalink / raw)
To: Enric Balletbo i Serra; +Cc: linux-wireless, Arend van Spriel
Hi Enric,
Could you please try these patches and let me know if they resolve
the issue for you.
Regards,
Arend
Arend van Spriel (3):
brcmfmac: add parameter to pass error code in firmware callback
brcmfmac: use firmware callback upon failure to load
brcmfmac: unbind all devices upon failure in firmware callback
.../broadcom/brcm80211/brcmfmac/firmware.c | 35 +++++++++++-----------
.../broadcom/brcm80211/brcmfmac/firmware.h | 4 +--
.../wireless/broadcom/brcm80211/brcmfmac/pcie.c | 17 +++++++----
.../wireless/broadcom/brcm80211/brcmfmac/sdio.c | 18 +++++++----
.../net/wireless/broadcom/brcm80211/brcmfmac/usb.c | 6 ++--
5 files changed, 47 insertions(+), 33 deletions(-)
--
1.9.1
^ permalink raw reply [flat|nested] 10+ messages in thread
* [RFT 1/3] brcmfmac: add parameter to pass error code in firmware callback
2017-05-30 12:35 [RFT 0/3] brcmfmac: firmware loading rework Arend van Spriel
@ 2017-05-30 12:35 ` Arend van Spriel
2017-05-31 11:16 ` Enric Balletbo Serra
2017-05-30 12:35 ` [RFT 2/3] brcmfmac: use firmware callback upon failure to load Arend van Spriel
` (2 subsequent siblings)
3 siblings, 1 reply; 10+ messages in thread
From: Arend van Spriel @ 2017-05-30 12:35 UTC (permalink / raw)
To: Enric Balletbo i Serra; +Cc: linux-wireless, Arend van Spriel
Extend the parameters in the firmware callback so it can be called
upon success and failure. This allows the caller to properly clear
all resources in the failure path. Right now the error code is
always zero, ie. success.
Reviewed-by: Hante Meuleman <hante.meuleman@broadcom.com>
Reviewed-by: Pieter-Paul Giesberts <pieter-paul.giesberts@broadcom.com>
Reviewed-by: Franky Lin <franky.lin@broadcom.com>
Signed-off-by: Arend van Spriel <arend.vanspriel@broadcom.com>
---
.../net/wireless/broadcom/brcm80211/brcmfmac/firmware.c | 10 +++++-----
.../net/wireless/broadcom/brcm80211/brcmfmac/firmware.h | 4 ++--
drivers/net/wireless/broadcom/brcm80211/brcmfmac/pcie.c | 17 ++++++++++++-----
drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c | 17 +++++++++++------
drivers/net/wireless/broadcom/brcm80211/brcmfmac/usb.c | 6 ++++--
5 files changed, 34 insertions(+), 20 deletions(-)
diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/firmware.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/firmware.c
index c7c1e99..ae61a24 100644
--- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/firmware.c
+++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/firmware.c
@@ -442,7 +442,7 @@ struct brcmf_fw {
const char *nvram_name;
u16 domain_nr;
u16 bus_nr;
- void (*done)(struct device *dev, const struct firmware *fw,
+ void (*done)(struct device *dev, int err, const struct firmware *fw,
void *nvram_image, u32 nvram_len);
};
@@ -477,7 +477,7 @@ static void brcmf_fw_request_nvram_done(const struct firmware *fw, void *ctx)
if (!nvram && !(fwctx->flags & BRCMF_FW_REQ_NV_OPTIONAL))
goto fail;
- fwctx->done(fwctx->dev, fwctx->code, nvram, nvram_length);
+ fwctx->done(fwctx->dev, 0, fwctx->code, nvram, nvram_length);
kfree(fwctx);
return;
@@ -499,7 +499,7 @@ static void brcmf_fw_request_code_done(const struct firmware *fw, void *ctx)
/* only requested code so done here */
if (!(fwctx->flags & BRCMF_FW_REQUEST_NVRAM)) {
- fwctx->done(fwctx->dev, fw, NULL, 0);
+ fwctx->done(fwctx->dev, 0, fw, NULL, 0);
kfree(fwctx);
return;
}
@@ -522,7 +522,7 @@ static void brcmf_fw_request_code_done(const struct firmware *fw, void *ctx)
int brcmf_fw_get_firmwares_pcie(struct device *dev, u16 flags,
const char *code, const char *nvram,
- void (*fw_cb)(struct device *dev,
+ void (*fw_cb)(struct device *dev, int err,
const struct firmware *fw,
void *nvram_image, u32 nvram_len),
u16 domain_nr, u16 bus_nr)
@@ -555,7 +555,7 @@ int brcmf_fw_get_firmwares_pcie(struct device *dev, u16 flags,
int brcmf_fw_get_firmwares(struct device *dev, u16 flags,
const char *code, const char *nvram,
- void (*fw_cb)(struct device *dev,
+ void (*fw_cb)(struct device *dev, int err,
const struct firmware *fw,
void *nvram_image, u32 nvram_len))
{
diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/firmware.h b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/firmware.h
index d3c9f0d..8fa4b7e 100644
--- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/firmware.h
+++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/firmware.h
@@ -73,13 +73,13 @@ int brcmf_fw_map_chip_to_name(u32 chip, u32 chiprev,
*/
int brcmf_fw_get_firmwares_pcie(struct device *dev, u16 flags,
const char *code, const char *nvram,
- void (*fw_cb)(struct device *dev,
+ void (*fw_cb)(struct device *dev, int err,
const struct firmware *fw,
void *nvram_image, u32 nvram_len),
u16 domain_nr, u16 bus_nr);
int brcmf_fw_get_firmwares(struct device *dev, u16 flags,
const char *code, const char *nvram,
- void (*fw_cb)(struct device *dev,
+ void (*fw_cb)(struct device *dev, int err,
const struct firmware *fw,
void *nvram_image, u32 nvram_len));
diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/pcie.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/pcie.c
index f36b96d..f878706 100644
--- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/pcie.c
+++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/pcie.c
@@ -1650,16 +1650,23 @@ static void brcmf_pcie_buscore_activate(void *ctx, struct brcmf_chip *chip,
.write32 = brcmf_pcie_buscore_write32,
};
-static void brcmf_pcie_setup(struct device *dev, const struct firmware *fw,
+static void brcmf_pcie_setup(struct device *dev, int ret,
+ const struct firmware *fw,
void *nvram, u32 nvram_len)
{
- struct brcmf_bus *bus = dev_get_drvdata(dev);
- struct brcmf_pciedev *pcie_bus_dev = bus->bus_priv.pcie;
- struct brcmf_pciedev_info *devinfo = pcie_bus_dev->devinfo;
+ struct brcmf_bus *bus;
+ struct brcmf_pciedev *pcie_bus_dev;
+ struct brcmf_pciedev_info *devinfo;
struct brcmf_commonring **flowrings;
- int ret;
u32 i;
+ /* check firmware loading result */
+ if (ret)
+ goto fail;
+
+ bus = dev_get_drvdata(dev);
+ pcie_bus_dev = bus->bus_priv.pcie;
+ devinfo = pcie_bus_dev->devinfo;
brcmf_pcie_attach(devinfo);
/* Some of the firmwares have the size of the memory of the device
diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c
index a999f95..270c0ad 100644
--- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c
+++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c
@@ -3978,21 +3978,26 @@ static void brcmf_sdio_buscore_write32(void *ctx, u32 addr, u32 val)
.get_memdump = brcmf_sdio_bus_get_memdump,
};
-static void brcmf_sdio_firmware_callback(struct device *dev,
+static void brcmf_sdio_firmware_callback(struct device *dev, int err,
const struct firmware *code,
void *nvram, u32 nvram_len)
{
- struct brcmf_bus *bus_if = dev_get_drvdata(dev);
- struct brcmf_sdio_dev *sdiodev = bus_if->bus_priv.sdio;
- struct brcmf_sdio *bus = sdiodev->bus;
- int err = 0;
+ struct brcmf_bus *bus_if;
+ struct brcmf_sdio_dev *sdiodev;
+ struct brcmf_sdio *bus;
u8 saveclk;
- brcmf_dbg(TRACE, "Enter: dev=%s\n", dev_name(dev));
+ brcmf_dbg(TRACE, "Enter: dev=%s, err=%d\n", dev_name(dev), err);
+ if (err)
+ goto fail;
+ bus_if = dev_get_drvdata(dev);
if (!bus_if->drvr)
return;
+ sdiodev = bus_if->bus_priv.sdio;
+ bus = sdiodev->bus;
+
/* try to download image and nvram to the dongle */
bus->alp_only = true;
err = brcmf_sdio_download_firmware(bus, code, nvram, nvram_len);
diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/usb.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/usb.c
index e4d545f..9ce3b55 100644
--- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/usb.c
+++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/usb.c
@@ -1159,13 +1159,15 @@ static int brcmf_usb_bus_setup(struct brcmf_usbdev_info *devinfo)
return ret;
}
-static void brcmf_usb_probe_phase2(struct device *dev,
+static void brcmf_usb_probe_phase2(struct device *dev, int ret,
const struct firmware *fw,
void *nvram, u32 nvlen)
{
struct brcmf_bus *bus = dev_get_drvdata(dev);
struct brcmf_usbdev_info *devinfo;
- int ret;
+
+ if (ret)
+ goto error;
brcmf_dbg(USB, "Start fw downloading\n");
--
1.9.1
^ permalink raw reply related [flat|nested] 10+ messages in thread
* [RFT 2/3] brcmfmac: use firmware callback upon failure to load
2017-05-30 12:35 [RFT 0/3] brcmfmac: firmware loading rework Arend van Spriel
2017-05-30 12:35 ` [RFT 1/3] brcmfmac: add parameter to pass error code in firmware callback Arend van Spriel
@ 2017-05-30 12:35 ` Arend van Spriel
2017-05-30 12:35 ` [RFT 3/3] brcmfmac: unbind all devices upon failure in firmware callback Arend van Spriel
2017-05-31 11:10 ` [RFT 0/3] brcmfmac: firmware loading rework Enric Balletbo Serra
3 siblings, 0 replies; 10+ messages in thread
From: Arend van Spriel @ 2017-05-30 12:35 UTC (permalink / raw)
To: Enric Balletbo i Serra; +Cc: linux-wireless, Arend van Spriel
When firmware loading failed the code used to unbind the device provided
by the calling code. However, for the sdio driver two devices are bound
and both need to be released upon failure. The callback has been extended
with parameter to pass error code so add that in this commit upon firmware
loading failure.
Reviewed-by: Hante Meuleman <hante.meuleman@broadcom.com>
Reviewed-by: Pieter-Paul Giesberts <pieter-paul.giesberts@broadcom.com>
Reviewed-by: Franky Lin <franky.lin@broadcom.com>
Signed-off-by: Arend van Spriel <arend.vanspriel@broadcom.com>
---
.../broadcom/brcm80211/brcmfmac/firmware.c | 27 +++++++++++-----------
1 file changed, 13 insertions(+), 14 deletions(-)
diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/firmware.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/firmware.c
index ae61a24..d231042 100644
--- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/firmware.c
+++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/firmware.c
@@ -484,39 +484,38 @@ static void brcmf_fw_request_nvram_done(const struct firmware *fw, void *ctx)
fail:
brcmf_dbg(TRACE, "failed: dev=%s\n", dev_name(fwctx->dev));
release_firmware(fwctx->code);
- device_release_driver(fwctx->dev);
+ fwctx->done(fwctx->dev, -ENOENT, NULL, NULL, 0);
kfree(fwctx);
}
static void brcmf_fw_request_code_done(const struct firmware *fw, void *ctx)
{
struct brcmf_fw *fwctx = ctx;
- int ret;
+ int ret = 0;
brcmf_dbg(TRACE, "enter: dev=%s\n", dev_name(fwctx->dev));
- if (!fw)
+ if (!fw) {
+ ret = -ENOENT;
goto fail;
-
- /* only requested code so done here */
- if (!(fwctx->flags & BRCMF_FW_REQUEST_NVRAM)) {
- fwctx->done(fwctx->dev, 0, fw, NULL, 0);
- kfree(fwctx);
- return;
}
+ /* only requested code so done here */
+ if (!(fwctx->flags & BRCMF_FW_REQUEST_NVRAM))
+ goto done;
+
fwctx->code = fw;
ret = request_firmware_nowait(THIS_MODULE, true, fwctx->nvram_name,
fwctx->dev, GFP_KERNEL, fwctx,
brcmf_fw_request_nvram_done);
- if (!ret)
- return;
-
- brcmf_fw_request_nvram_done(NULL, fwctx);
+ /* pass NULL to nvram callback for bcm47xx fallback */
+ if (ret)
+ brcmf_fw_request_nvram_done(NULL, fwctx);
return;
fail:
brcmf_dbg(TRACE, "failed: dev=%s\n", dev_name(fwctx->dev));
- device_release_driver(fwctx->dev);
+done:
+ fwctx->done(fwctx->dev, ret, fw, NULL, 0);
kfree(fwctx);
}
--
1.9.1
^ permalink raw reply related [flat|nested] 10+ messages in thread
* [RFT 3/3] brcmfmac: unbind all devices upon failure in firmware callback
2017-05-30 12:35 [RFT 0/3] brcmfmac: firmware loading rework Arend van Spriel
2017-05-30 12:35 ` [RFT 1/3] brcmfmac: add parameter to pass error code in firmware callback Arend van Spriel
2017-05-30 12:35 ` [RFT 2/3] brcmfmac: use firmware callback upon failure to load Arend van Spriel
@ 2017-05-30 12:35 ` Arend van Spriel
2017-05-31 11:13 ` Enric Balletbo Serra
2017-05-31 11:10 ` [RFT 0/3] brcmfmac: firmware loading rework Enric Balletbo Serra
3 siblings, 1 reply; 10+ messages in thread
From: Arend van Spriel @ 2017-05-30 12:35 UTC (permalink / raw)
To: Enric Balletbo i Serra; +Cc: linux-wireless, Arend van Spriel
In brcmf_sdio_firmware_callback() we need to unbind the driver from
both sdio_func devices.
Reviewed-by: Hante Meuleman <hante.meuleman@broadcom.com>
Reviewed-by: Pieter-Paul Giesberts <pieter-paul.giesberts@broadcom.com>
Reviewed-by: Franky Lin <franky.lin@broadcom.com>
Signed-off-by: Arend van Spriel <arend.vanspriel@broadcom.com>
---
drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c
index 270c0ad..a5b27a4 100644
--- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c
+++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c
@@ -3988,14 +3988,14 @@ static void brcmf_sdio_firmware_callback(struct device *dev, int err,
u8 saveclk;
brcmf_dbg(TRACE, "Enter: dev=%s, err=%d\n", dev_name(dev), err);
+ bus_if = dev_get_drvdata(dev);
+ sdiodev = bus_if->bus_priv.sdio;
if (err)
goto fail;
- bus_if = dev_get_drvdata(dev);
if (!bus_if->drvr)
return;
- sdiodev = bus_if->bus_priv.sdio;
bus = sdiodev->bus;
/* try to download image and nvram to the dongle */
@@ -4084,6 +4084,7 @@ static void brcmf_sdio_firmware_callback(struct device *dev, int err,
fail:
brcmf_dbg(TRACE, "failed: dev=%s, err=%d\n", dev_name(dev), err);
device_release_driver(dev);
+ device_release_driver(&sdiodev->func[1]->dev);
}
struct brcmf_sdio *brcmf_sdio_probe(struct brcmf_sdio_dev *sdiodev)
--
1.9.1
^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [RFT 0/3] brcmfmac: firmware loading rework
2017-05-30 12:35 [RFT 0/3] brcmfmac: firmware loading rework Arend van Spriel
` (2 preceding siblings ...)
2017-05-30 12:35 ` [RFT 3/3] brcmfmac: unbind all devices upon failure in firmware callback Arend van Spriel
@ 2017-05-31 11:10 ` Enric Balletbo Serra
2017-05-31 12:00 ` Arend van Spriel
3 siblings, 1 reply; 10+ messages in thread
From: Enric Balletbo Serra @ 2017-05-31 11:10 UTC (permalink / raw)
To: Arend van Spriel; +Cc: Enric Balletbo i Serra, linux-wireless
Hi Arend,
2017-05-30 14:35 GMT+02:00 Arend van Spriel <arend.vanspriel@broadcom.com>:
> Hi Enric,
>
> Could you please try these patches and let me know if they resolve
> the issue for you.
>
> Regards,
> Arend
>
> Arend van Spriel (3):
> brcmfmac: add parameter to pass error code in firmware callback
> brcmfmac: use firmware callback upon failure to load
> brcmfmac: unbind all devices upon failure in firmware callback
>
> .../broadcom/brcm80211/brcmfmac/firmware.c | 35 +++++++++++-----------
> .../broadcom/brcm80211/brcmfmac/firmware.h | 4 +--
> .../wireless/broadcom/brcm80211/brcmfmac/pcie.c | 17 +++++++----
> .../wireless/broadcom/brcm80211/brcmfmac/sdio.c | 18 +++++++----
> .../net/wireless/broadcom/brcm80211/brcmfmac/usb.c | 6 ++--
> 5 files changed, 47 insertions(+), 33 deletions(-)
>
> --
> 1.9.1
>
After these patches the error path when firmware is not found seems
the correct way to proceed but I'm still getting the same kernel panic
[1].
I added some printk and I saw that when firmware load fails
[ 7.696463] brcmfmac mmc1:0001:1: Direct firmware load for
brcm/brcmfmac4354-sdio.bin failed with error -2
The brcmf_sdio_firmware_callback fails, and goes to the fail label and
the devices are released.
device_release_driver(dev);
device_release_driver(&sdiodev->func[1]->dev);
But PM ops are not unregistered for mmc1:0001:2 and
brcmf_ops_sdio_resume is still called, so I'm wondering if you missed
here to add
device_release_driver(&sdiodev->func[2]->dev);
Adding that seems to solve the problem, not sure if there is any
collateral effect.
+ info:
Steps to reproduce the Oops:
1. Remove the brcm/brcmfmac4354-sdio.bin firmware
2. modprobe brcmfmac
[ 6.879679] brcmfmac mmc1:0001:1: Direct firmware load for
brcm/brcmfmac4354-sdio.bin failed with error -2
3. Do a suspend/resume cycle
echo mem > /sys/power/state && # hit a key
[1] Kernel Oops:
[ 29.375149] Unable to handle kernel NULL pointer dereference at
virtual address 00000000
[ 29.384270] pgd = c0204000
[ 29.387310] [00000000] *pgd=00000000
[ 29.391336] Internal error: Oops: 17 [#1] SMP ARM
[ 29.396628] Modules linked in: cpufreq_powersave cpufreq_userspace
cpufreq_conservative bq27xxx_battery_i2c bq27xxx_battery cros_ec_keyb
i2c_cros_ec_tunnel cros_ec_devs cros_ec_spi cros_ec_core
snd_soc_rockchip_max98090 brcmfmac snd_soc_ts3a227e snd_soc_max98090
snd_soc_rockchip_i2s cfg80211 snd_soc_core ac97_bus uvcvideo
videobuf2_vmalloc videobuf2_memops videobuf2_v4l2 videobuf2_core
videodev snd_pcm_dmaengine brcmutil snd_pcm gpio_charger snd_timer
media nvmem_rockchip_efuse rk_crypto md5 sha256_generic sha1_generic
des_generic phy_rockchip_dp rtc_rk808 snd soundcore pwm_rockchip
rockchipdrm tpm_i2c_infineon dw_hdmi tpm analogix_dp pwm_bl
spi_rockchip
[ 29.461958] CPU: 0 PID: 3030 Comm: kworker/u8:28 Not tainted
4.12.0-rc2-next-20170529-00003-g38fc31b-dirty #3
[ 29.473118] Hardware name: Rockchip (Device Tree)
[ 29.478415] Workqueue: events_unbound async_run_entry_fn
[ 29.484384] task: ed6dc200 task.stack: ed27c000
[ 29.489494] PC is at brcmf_ops_sdio_resume+0x10/0x5c [brcmfmac]
[ 29.496158] LR is at dpm_run_callback+0x38/0x160
[ 29.501351] pc : [<bf4c0fc0>] lr : [<c087fb70>] psr: 60000113
[ 29.508394] sp : ed27dec8 ip : 60000113 fp : c1572a54
[ 29.514262] r10: 00000000 r9 : c0fe196c r8 : 00000010
[ 29.520131] r7 : c087b900 r6 : 00000000 r5 : ed1b5208 r4 : 00000001
[ 29.527476] r3 : 00000000 r2 : 00000002 r1 : ed1b5208 r0 : ed1b5208
[ 29.537127] Flags: nZCv IRQs on FIQs on Mode SVC_32 ISA ARM Segment none
[ 29.547454] Control: 10c5387d Table: 2d75c06a DAC: 00000051
[ 29.556215] Process kworker/u8:28 (pid: 3030, stack limit = 0xed27c220)
[ 29.565965] Stack: (0xed27dec8 to 0xed27e000)
[ 29.573150] dec0: 00000001 c087fb70 00000001
ed1b5208 00000000 00000010
[ 29.584656] dee0: ed1b523c ed27c000 00000000 c08802d4 c15c54fc
ed1b5208 ed542140 ee006500
[ 29.596172] df00: 00000000 c08804b8 ed542150 c157fda0 ed542140
c036525c ec820280 ed542150
[ 29.607694] df20: ee004800 ee006500 00000000 c035cc28 eef914c0
ee004848 ee004818 ee004800
[ 29.619249] df40: ec820298 00000088 ee004818 c1402d00 ed27c000
ee004800 ec820280 c035cf58
[ 29.630816] df60: ee004960 00000000 ec820280 ec802000 00000000
ed7d7ac0 ec820280 c035cf20
[ 29.642397] df80: ec80201c ec96bee8 00000000 c0362358 ed7d7ac0
c036222c 00000000 00000000
[ 29.654046] dfa0: 00000000 00000000 00000000 c0307e78 00000000
00000000 00000000 00000000
[ 29.665707] dfc0: 00000000 00000000 00000000 00000000 00000000
00000000 00000000 00000000
[ 29.677322] dfe0: 00000000 00000000 00000000 00000000 00000013
00000000 00000000 00000000
[ 29.688968] [<bf4c0fc0>] (brcmf_ops_sdio_resume [brcmfmac]) from
[<c087fb70>] (dpm_run_callback+0x38/0x160)
[ 29.702379] [<c087fb70>] (dpm_run_callback) from [<c08802d4>]
(device_resume+0x94/0x25c)
[ 29.713951] [<c08802d4>] (device_resume) from [<c08804b8>]
(async_resume+0x1c/0x44)
[ 29.725063] [<c08804b8>] (async_resume) from [<c036525c>]
(async_run_entry_fn+0x44/0x108)
[ 29.736788] [<c036525c>] (async_run_entry_fn) from [<c035cc28>]
(process_one_work+0x14c/0x444)
[ 29.749022] [<c035cc28>] (process_one_work) from [<c035cf58>]
(worker_thread+0x38/0x510)
[ 29.760675] [<c035cf58>] (worker_thread) from [<c0362358>]
(kthread+0x12c/0x15c)
[ 29.771521] [<c0362358>] (kthread) from [<c0307e78>]
(ret_from_fork+0x14/0x3c)
[ 29.782226] Code: e92d4010 e59021a4 e5903054 e3520002 (e5934000)
[ 29.791751] ---[ end trace 035307c5087a8fee ]---
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [RFT 3/3] brcmfmac: unbind all devices upon failure in firmware callback
2017-05-30 12:35 ` [RFT 3/3] brcmfmac: unbind all devices upon failure in firmware callback Arend van Spriel
@ 2017-05-31 11:13 ` Enric Balletbo Serra
0 siblings, 0 replies; 10+ messages in thread
From: Enric Balletbo Serra @ 2017-05-31 11:13 UTC (permalink / raw)
To: Arend van Spriel; +Cc: Enric Balletbo i Serra, linux-wireless
2017-05-30 14:35 GMT+02:00 Arend van Spriel <arend.vanspriel@broadcom.com>:
> In brcmf_sdio_firmware_callback() we need to unbind the driver from
> both sdio_func devices.
>
> Reviewed-by: Hante Meuleman <hante.meuleman@broadcom.com>
> Reviewed-by: Pieter-Paul Giesberts <pieter-paul.giesberts@broadcom.com>
> Reviewed-by: Franky Lin <franky.lin@broadcom.com>
> Signed-off-by: Arend van Spriel <arend.vanspriel@broadcom.com>
> ---
> drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c | 5 +++--
> 1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c
> index 270c0ad..a5b27a4 100644
> --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c
> +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c
> @@ -3988,14 +3988,14 @@ static void brcmf_sdio_firmware_callback(struct device *dev, int err,
> u8 saveclk;
>
> brcmf_dbg(TRACE, "Enter: dev=%s, err=%d\n", dev_name(dev), err);
> + bus_if = dev_get_drvdata(dev);
> + sdiodev = bus_if->bus_priv.sdio;
> if (err)
> goto fail;
>
> - bus_if = dev_get_drvdata(dev);
> if (!bus_if->drvr)
> return;
>
> - sdiodev = bus_if->bus_priv.sdio;
> bus = sdiodev->bus;
>
> /* try to download image and nvram to the dongle */
> @@ -4084,6 +4084,7 @@ static void brcmf_sdio_firmware_callback(struct device *dev, int err,
> fail:
> brcmf_dbg(TRACE, "failed: dev=%s, err=%d\n", dev_name(dev), err);
> device_release_driver(dev);
> + device_release_driver(&sdiodev->func[1]->dev);
missing device_release_driver(&sdiodev->func[2]->dev); ?
See my answers to the cover-letter.
> }
>
> struct brcmf_sdio *brcmf_sdio_probe(struct brcmf_sdio_dev *sdiodev)
> --
> 1.9.1
>
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [RFT 1/3] brcmfmac: add parameter to pass error code in firmware callback
2017-05-30 12:35 ` [RFT 1/3] brcmfmac: add parameter to pass error code in firmware callback Arend van Spriel
@ 2017-05-31 11:16 ` Enric Balletbo Serra
0 siblings, 0 replies; 10+ messages in thread
From: Enric Balletbo Serra @ 2017-05-31 11:16 UTC (permalink / raw)
To: Arend van Spriel; +Cc: Enric Balletbo i Serra, linux-wireless
2017-05-30 14:35 GMT+02:00 Arend van Spriel <arend.vanspriel@broadcom.com>:
> Extend the parameters in the firmware callback so it can be called
> upon success and failure. This allows the caller to properly clear
> all resources in the failure path. Right now the error code is
> always zero, ie. success.
>
> Reviewed-by: Hante Meuleman <hante.meuleman@broadcom.com>
> Reviewed-by: Pieter-Paul Giesberts <pieter-paul.giesberts@broadcom.com>
> Reviewed-by: Franky Lin <franky.lin@broadcom.com>
> Signed-off-by: Arend van Spriel <arend.vanspriel@broadcom.com>
> ---
> .../net/wireless/broadcom/brcm80211/brcmfmac/firmware.c | 10 +++++-----
> .../net/wireless/broadcom/brcm80211/brcmfmac/firmware.h | 4 ++--
> drivers/net/wireless/broadcom/brcm80211/brcmfmac/pcie.c | 17 ++++++++++++-----
> drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c | 17 +++++++++++------
> drivers/net/wireless/broadcom/brcm80211/brcmfmac/usb.c | 6 ++++--
> 5 files changed, 34 insertions(+), 20 deletions(-)
>
> diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/firmware.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/firmware.c
> index c7c1e99..ae61a24 100644
> --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/firmware.c
> +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/firmware.c
> @@ -442,7 +442,7 @@ struct brcmf_fw {
> const char *nvram_name;
> u16 domain_nr;
> u16 bus_nr;
> - void (*done)(struct device *dev, const struct firmware *fw,
> + void (*done)(struct device *dev, int err, const struct firmware *fw,
> void *nvram_image, u32 nvram_len);
> };
>
> @@ -477,7 +477,7 @@ static void brcmf_fw_request_nvram_done(const struct firmware *fw, void *ctx)
> if (!nvram && !(fwctx->flags & BRCMF_FW_REQ_NV_OPTIONAL))
> goto fail;
>
> - fwctx->done(fwctx->dev, fwctx->code, nvram, nvram_length);
> + fwctx->done(fwctx->dev, 0, fwctx->code, nvram, nvram_length);
> kfree(fwctx);
> return;
>
> @@ -499,7 +499,7 @@ static void brcmf_fw_request_code_done(const struct firmware *fw, void *ctx)
>
> /* only requested code so done here */
> if (!(fwctx->flags & BRCMF_FW_REQUEST_NVRAM)) {
> - fwctx->done(fwctx->dev, fw, NULL, 0);
> + fwctx->done(fwctx->dev, 0, fw, NULL, 0);
> kfree(fwctx);
> return;
> }
> @@ -522,7 +522,7 @@ static void brcmf_fw_request_code_done(const struct firmware *fw, void *ctx)
>
> int brcmf_fw_get_firmwares_pcie(struct device *dev, u16 flags,
> const char *code, const char *nvram,
> - void (*fw_cb)(struct device *dev,
> + void (*fw_cb)(struct device *dev, int err,
> const struct firmware *fw,
> void *nvram_image, u32 nvram_len),
> u16 domain_nr, u16 bus_nr)
> @@ -555,7 +555,7 @@ int brcmf_fw_get_firmwares_pcie(struct device *dev, u16 flags,
>
> int brcmf_fw_get_firmwares(struct device *dev, u16 flags,
> const char *code, const char *nvram,
> - void (*fw_cb)(struct device *dev,
> + void (*fw_cb)(struct device *dev, int err,
> const struct firmware *fw,
> void *nvram_image, u32 nvram_len))
> {
> diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/firmware.h b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/firmware.h
> index d3c9f0d..8fa4b7e 100644
> --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/firmware.h
> +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/firmware.h
> @@ -73,13 +73,13 @@ int brcmf_fw_map_chip_to_name(u32 chip, u32 chiprev,
> */
> int brcmf_fw_get_firmwares_pcie(struct device *dev, u16 flags,
> const char *code, const char *nvram,
> - void (*fw_cb)(struct device *dev,
> + void (*fw_cb)(struct device *dev, int err,
> const struct firmware *fw,
> void *nvram_image, u32 nvram_len),
> u16 domain_nr, u16 bus_nr);
> int brcmf_fw_get_firmwares(struct device *dev, u16 flags,
> const char *code, const char *nvram,
> - void (*fw_cb)(struct device *dev,
> + void (*fw_cb)(struct device *dev, int err,
> const struct firmware *fw,
> void *nvram_image, u32 nvram_len));
>
> diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/pcie.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/pcie.c
> index f36b96d..f878706 100644
> --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/pcie.c
> +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/pcie.c
> @@ -1650,16 +1650,23 @@ static void brcmf_pcie_buscore_activate(void *ctx, struct brcmf_chip *chip,
> .write32 = brcmf_pcie_buscore_write32,
> };
>
> -static void brcmf_pcie_setup(struct device *dev, const struct firmware *fw,
> +static void brcmf_pcie_setup(struct device *dev, int ret,
> + const struct firmware *fw,
> void *nvram, u32 nvram_len)
> {
> - struct brcmf_bus *bus = dev_get_drvdata(dev);
> - struct brcmf_pciedev *pcie_bus_dev = bus->bus_priv.pcie;
> - struct brcmf_pciedev_info *devinfo = pcie_bus_dev->devinfo;
> + struct brcmf_bus *bus;
> + struct brcmf_pciedev *pcie_bus_dev;
> + struct brcmf_pciedev_info *devinfo;
> struct brcmf_commonring **flowrings;
> - int ret;
> u32 i;
>
> + /* check firmware loading result */
> + if (ret)
> + goto fail;
> +
> + bus = dev_get_drvdata(dev);
> + pcie_bus_dev = bus->bus_priv.pcie;
> + devinfo = pcie_bus_dev->devinfo;
> brcmf_pcie_attach(devinfo);
>
> /* Some of the firmwares have the size of the memory of the device
> diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c
> index a999f95..270c0ad 100644
> --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c
> +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c
> @@ -3978,21 +3978,26 @@ static void brcmf_sdio_buscore_write32(void *ctx, u32 addr, u32 val)
> .get_memdump = brcmf_sdio_bus_get_memdump,
> };
>
> -static void brcmf_sdio_firmware_callback(struct device *dev,
> +static void brcmf_sdio_firmware_callback(struct device *dev, int err,
> const struct firmware *code,
> void *nvram, u32 nvram_len)
> {
> - struct brcmf_bus *bus_if = dev_get_drvdata(dev);
> - struct brcmf_sdio_dev *sdiodev = bus_if->bus_priv.sdio;
> - struct brcmf_sdio *bus = sdiodev->bus;
> - int err = 0;
> + struct brcmf_bus *bus_if;
> + struct brcmf_sdio_dev *sdiodev;
> + struct brcmf_sdio *bus;
> u8 saveclk;
>
> - brcmf_dbg(TRACE, "Enter: dev=%s\n", dev_name(dev));
> + brcmf_dbg(TRACE, "Enter: dev=%s, err=%d\n", dev_name(dev), err);
> + if (err)
> + goto fail;
>
> + bus_if = dev_get_drvdata(dev);
> if (!bus_if->drvr)
> return;
>
> + sdiodev = bus_if->bus_priv.sdio;
> + bus = sdiodev->bus;
> +
> /* try to download image and nvram to the dongle */
> bus->alp_only = true;
> err = brcmf_sdio_download_firmware(bus, code, nvram, nvram_len);
> diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/usb.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/usb.c
> index e4d545f..9ce3b55 100644
> --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/usb.c
> +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/usb.c
> @@ -1159,13 +1159,15 @@ static int brcmf_usb_bus_setup(struct brcmf_usbdev_info *devinfo)
> return ret;
> }
>
> -static void brcmf_usb_probe_phase2(struct device *dev,
> +static void brcmf_usb_probe_phase2(struct device *dev, int ret,
nit: int err ?
IMHO is more clear use err than ret which is normally used as a return
value in a function not as a parameter
> const struct firmware *fw,
> void *nvram, u32 nvlen)
> {
> struct brcmf_bus *bus = dev_get_drvdata(dev);
> struct brcmf_usbdev_info *devinfo;
> - int ret;
> +
> + if (ret)
nit: if (err)
> + goto error;
>
> brcmf_dbg(USB, "Start fw downloading\n");
>
> --
> 1.9.1
>
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [RFT 0/3] brcmfmac: firmware loading rework
2017-05-31 11:10 ` [RFT 0/3] brcmfmac: firmware loading rework Enric Balletbo Serra
@ 2017-05-31 12:00 ` Arend van Spriel
2017-05-31 12:45 ` Enric Balletbo Serra
0 siblings, 1 reply; 10+ messages in thread
From: Arend van Spriel @ 2017-05-31 12:00 UTC (permalink / raw)
To: Enric Balletbo Serra; +Cc: Enric Balletbo i Serra, linux-wireless
On 31-05-17 13:10, Enric Balletbo Serra wrote:
> Hi Arend,
>
> 2017-05-30 14:35 GMT+02:00 Arend van Spriel <arend.vanspriel@broadcom.com>:
>> Hi Enric,
>>
>> Could you please try these patches and let me know if they resolve
>> the issue for you.
>>
>> Regards,
>> Arend
>>
>> Arend van Spriel (3):
>> brcmfmac: add parameter to pass error code in firmware callback
>> brcmfmac: use firmware callback upon failure to load
>> brcmfmac: unbind all devices upon failure in firmware callback
>>
>> .../broadcom/brcm80211/brcmfmac/firmware.c | 35 +++++++++++-----------
>> .../broadcom/brcm80211/brcmfmac/firmware.h | 4 +--
>> .../wireless/broadcom/brcm80211/brcmfmac/pcie.c | 17 +++++++----
>> .../wireless/broadcom/brcm80211/brcmfmac/sdio.c | 18 +++++++----
>> .../net/wireless/broadcom/brcm80211/brcmfmac/usb.c | 6 ++--
>> 5 files changed, 47 insertions(+), 33 deletions(-)
>>
>> --
>> 1.9.1
>>
>
> After these patches the error path when firmware is not found seems
> the correct way to proceed but I'm still getting the same kernel panic
> [1].
>
> I added some printk and I saw that when firmware load fails
>
> [ 7.696463] brcmfmac mmc1:0001:1: Direct firmware load for
> brcm/brcmfmac4354-sdio.bin failed with error -2
>
> The brcmf_sdio_firmware_callback fails, and goes to the fail label and
> the devices are released.
>
> device_release_driver(dev);
> device_release_driver(&sdiodev->func[1]->dev);
>
> But PM ops are not unregistered for mmc1:0001:2 and
> brcmf_ops_sdio_resume is still called, so I'm wondering if you missed
> here to add
>
> device_release_driver(&sdiodev->func[2]->dev);
>
> Adding that seems to solve the problem, not sure if there is any
> collateral effect.
Looking closer at the firmware load failure message it uses mmc1:0001:1,
ie. func[1]->dev. I wrongly assume everything was using func[2]->dev
hence I added the device_release_driver() call for func[1]->dev. So we
are calling device_release_driver() twice for the same device. Just
change func[1] into func[2].
Regards,
Arend
> + info:
>
> Steps to reproduce the Oops:
>
> 1. Remove the brcm/brcmfmac4354-sdio.bin firmware
> 2. modprobe brcmfmac
> [ 6.879679] brcmfmac mmc1:0001:1: Direct firmware load for
> brcm/brcmfmac4354-sdio.bin failed with error -2
> 3. Do a suspend/resume cycle
> echo mem > /sys/power/state && # hit a key
>
> [1] Kernel Oops:
>
> [ 29.375149] Unable to handle kernel NULL pointer dereference at
> virtual address 00000000
> [ 29.384270] pgd = c0204000
> [ 29.387310] [00000000] *pgd=00000000
> [ 29.391336] Internal error: Oops: 17 [#1] SMP ARM
> [ 29.396628] Modules linked in: cpufreq_powersave cpufreq_userspace
> cpufreq_conservative bq27xxx_battery_i2c bq27xxx_battery cros_ec_keyb
> i2c_cros_ec_tunnel cros_ec_devs cros_ec_spi cros_ec_core
> snd_soc_rockchip_max98090 brcmfmac snd_soc_ts3a227e snd_soc_max98090
> snd_soc_rockchip_i2s cfg80211 snd_soc_core ac97_bus uvcvideo
> videobuf2_vmalloc videobuf2_memops videobuf2_v4l2 videobuf2_core
> videodev snd_pcm_dmaengine brcmutil snd_pcm gpio_charger snd_timer
> media nvmem_rockchip_efuse rk_crypto md5 sha256_generic sha1_generic
> des_generic phy_rockchip_dp rtc_rk808 snd soundcore pwm_rockchip
> rockchipdrm tpm_i2c_infineon dw_hdmi tpm analogix_dp pwm_bl
> spi_rockchip
> [ 29.461958] CPU: 0 PID: 3030 Comm: kworker/u8:28 Not tainted
> 4.12.0-rc2-next-20170529-00003-g38fc31b-dirty #3
> [ 29.473118] Hardware name: Rockchip (Device Tree)
> [ 29.478415] Workqueue: events_unbound async_run_entry_fn
> [ 29.484384] task: ed6dc200 task.stack: ed27c000
> [ 29.489494] PC is at brcmf_ops_sdio_resume+0x10/0x5c [brcmfmac]
> [ 29.496158] LR is at dpm_run_callback+0x38/0x160
> [ 29.501351] pc : [<bf4c0fc0>] lr : [<c087fb70>] psr: 60000113
> [ 29.508394] sp : ed27dec8 ip : 60000113 fp : c1572a54
> [ 29.514262] r10: 00000000 r9 : c0fe196c r8 : 00000010
> [ 29.520131] r7 : c087b900 r6 : 00000000 r5 : ed1b5208 r4 : 00000001
> [ 29.527476] r3 : 00000000 r2 : 00000002 r1 : ed1b5208 r0 : ed1b5208
> [ 29.537127] Flags: nZCv IRQs on FIQs on Mode SVC_32 ISA ARM Segment none
> [ 29.547454] Control: 10c5387d Table: 2d75c06a DAC: 00000051
> [ 29.556215] Process kworker/u8:28 (pid: 3030, stack limit = 0xed27c220)
> [ 29.565965] Stack: (0xed27dec8 to 0xed27e000)
> [ 29.573150] dec0: 00000001 c087fb70 00000001
> ed1b5208 00000000 00000010
> [ 29.584656] dee0: ed1b523c ed27c000 00000000 c08802d4 c15c54fc
> ed1b5208 ed542140 ee006500
> [ 29.596172] df00: 00000000 c08804b8 ed542150 c157fda0 ed542140
> c036525c ec820280 ed542150
> [ 29.607694] df20: ee004800 ee006500 00000000 c035cc28 eef914c0
> ee004848 ee004818 ee004800
> [ 29.619249] df40: ec820298 00000088 ee004818 c1402d00 ed27c000
> ee004800 ec820280 c035cf58
> [ 29.630816] df60: ee004960 00000000 ec820280 ec802000 00000000
> ed7d7ac0 ec820280 c035cf20
> [ 29.642397] df80: ec80201c ec96bee8 00000000 c0362358 ed7d7ac0
> c036222c 00000000 00000000
> [ 29.654046] dfa0: 00000000 00000000 00000000 c0307e78 00000000
> 00000000 00000000 00000000
> [ 29.665707] dfc0: 00000000 00000000 00000000 00000000 00000000
> 00000000 00000000 00000000
> [ 29.677322] dfe0: 00000000 00000000 00000000 00000000 00000013
> 00000000 00000000 00000000
> [ 29.688968] [<bf4c0fc0>] (brcmf_ops_sdio_resume [brcmfmac]) from
> [<c087fb70>] (dpm_run_callback+0x38/0x160)
> [ 29.702379] [<c087fb70>] (dpm_run_callback) from [<c08802d4>]
> (device_resume+0x94/0x25c)
> [ 29.713951] [<c08802d4>] (device_resume) from [<c08804b8>]
> (async_resume+0x1c/0x44)
> [ 29.725063] [<c08804b8>] (async_resume) from [<c036525c>]
> (async_run_entry_fn+0x44/0x108)
> [ 29.736788] [<c036525c>] (async_run_entry_fn) from [<c035cc28>]
> (process_one_work+0x14c/0x444)
> [ 29.749022] [<c035cc28>] (process_one_work) from [<c035cf58>]
> (worker_thread+0x38/0x510)
> [ 29.760675] [<c035cf58>] (worker_thread) from [<c0362358>]
> (kthread+0x12c/0x15c)
> [ 29.771521] [<c0362358>] (kthread) from [<c0307e78>]
> (ret_from_fork+0x14/0x3c)
> [ 29.782226] Code: e92d4010 e59021a4 e5903054 e3520002 (e5934000)
> [ 29.791751] ---[ end trace 035307c5087a8fee ]---
>
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [RFT 0/3] brcmfmac: firmware loading rework
2017-05-31 12:00 ` Arend van Spriel
@ 2017-05-31 12:45 ` Enric Balletbo Serra
2017-05-31 19:32 ` Arend van Spriel
0 siblings, 1 reply; 10+ messages in thread
From: Enric Balletbo Serra @ 2017-05-31 12:45 UTC (permalink / raw)
To: Arend van Spriel; +Cc: Enric Balletbo i Serra, linux-wireless
2017-05-31 14:00 GMT+02:00 Arend van Spriel <arend.vanspriel@broadcom.com>:
> On 31-05-17 13:10, Enric Balletbo Serra wrote:
>> Hi Arend,
>>
>> 2017-05-30 14:35 GMT+02:00 Arend van Spriel <arend.vanspriel@broadcom.com>:
>>> Hi Enric,
>>>
>>> Could you please try these patches and let me know if they resolve
>>> the issue for you.
>>>
>>> Regards,
>>> Arend
>>>
>>> Arend van Spriel (3):
>>> brcmfmac: add parameter to pass error code in firmware callback
>>> brcmfmac: use firmware callback upon failure to load
>>> brcmfmac: unbind all devices upon failure in firmware callback
>>>
>>> .../broadcom/brcm80211/brcmfmac/firmware.c | 35 +++++++++++-----------
>>> .../broadcom/brcm80211/brcmfmac/firmware.h | 4 +--
>>> .../wireless/broadcom/brcm80211/brcmfmac/pcie.c | 17 +++++++----
>>> .../wireless/broadcom/brcm80211/brcmfmac/sdio.c | 18 +++++++----
>>> .../net/wireless/broadcom/brcm80211/brcmfmac/usb.c | 6 ++--
>>> 5 files changed, 47 insertions(+), 33 deletions(-)
>>>
>>> --
>>> 1.9.1
>>>
>>
>> After these patches the error path when firmware is not found seems
>> the correct way to proceed but I'm still getting the same kernel panic
>> [1].
>>
>> I added some printk and I saw that when firmware load fails
>>
>> [ 7.696463] brcmfmac mmc1:0001:1: Direct firmware load for
>> brcm/brcmfmac4354-sdio.bin failed with error -2
>>
>> The brcmf_sdio_firmware_callback fails, and goes to the fail label and
>> the devices are released.
>>
>> device_release_driver(dev);
>> device_release_driver(&sdiodev->func[1]->dev);
>>
>> But PM ops are not unregistered for mmc1:0001:2 and
>> brcmf_ops_sdio_resume is still called, so I'm wondering if you missed
>> here to add
>>
>> device_release_driver(&sdiodev->func[2]->dev);
>>
>> Adding that seems to solve the problem, not sure if there is any
>> collateral effect.
>
> Looking closer at the firmware load failure message it uses mmc1:0001:1,
> ie. func[1]->dev. I wrongly assume everything was using func[2]->dev
> hence I added the device_release_driver() call for func[1]->dev. So we
> are calling device_release_driver() twice for the same device. Just
> change func[1] into func[2].
>
- device_release_driver(&sdiodev->func[1]->dev);
+ device_release_driver(&sdiodev->func[2]->dev);
Right this works, thanks Arend. If you send a new patch revision you can add my
Tested-by: Enric Balletbo i Serra <enric.balletbo@collabora.com>
Regards,
Enric
> Regards,
> Arend
>
>> + info:
>>
>> Steps to reproduce the Oops:
>>
>> 1. Remove the brcm/brcmfmac4354-sdio.bin firmware
>> 2. modprobe brcmfmac
>> [ 6.879679] brcmfmac mmc1:0001:1: Direct firmware load for
>> brcm/brcmfmac4354-sdio.bin failed with error -2
>> 3. Do a suspend/resume cycle
>> echo mem > /sys/power/state && # hit a key
>>
>> [1] Kernel Oops:
>>
>> [ 29.375149] Unable to handle kernel NULL pointer dereference at
>> virtual address 00000000
>> [ 29.384270] pgd = c0204000
>> [ 29.387310] [00000000] *pgd=00000000
>> [ 29.391336] Internal error: Oops: 17 [#1] SMP ARM
>> [ 29.396628] Modules linked in: cpufreq_powersave cpufreq_userspace
>> cpufreq_conservative bq27xxx_battery_i2c bq27xxx_battery cros_ec_keyb
>> i2c_cros_ec_tunnel cros_ec_devs cros_ec_spi cros_ec_core
>> snd_soc_rockchip_max98090 brcmfmac snd_soc_ts3a227e snd_soc_max98090
>> snd_soc_rockchip_i2s cfg80211 snd_soc_core ac97_bus uvcvideo
>> videobuf2_vmalloc videobuf2_memops videobuf2_v4l2 videobuf2_core
>> videodev snd_pcm_dmaengine brcmutil snd_pcm gpio_charger snd_timer
>> media nvmem_rockchip_efuse rk_crypto md5 sha256_generic sha1_generic
>> des_generic phy_rockchip_dp rtc_rk808 snd soundcore pwm_rockchip
>> rockchipdrm tpm_i2c_infineon dw_hdmi tpm analogix_dp pwm_bl
>> spi_rockchip
>> [ 29.461958] CPU: 0 PID: 3030 Comm: kworker/u8:28 Not tainted
>> 4.12.0-rc2-next-20170529-00003-g38fc31b-dirty #3
>> [ 29.473118] Hardware name: Rockchip (Device Tree)
>> [ 29.478415] Workqueue: events_unbound async_run_entry_fn
>> [ 29.484384] task: ed6dc200 task.stack: ed27c000
>> [ 29.489494] PC is at brcmf_ops_sdio_resume+0x10/0x5c [brcmfmac]
>> [ 29.496158] LR is at dpm_run_callback+0x38/0x160
>> [ 29.501351] pc : [<bf4c0fc0>] lr : [<c087fb70>] psr: 60000113
>> [ 29.508394] sp : ed27dec8 ip : 60000113 fp : c1572a54
>> [ 29.514262] r10: 00000000 r9 : c0fe196c r8 : 00000010
>> [ 29.520131] r7 : c087b900 r6 : 00000000 r5 : ed1b5208 r4 : 00000001
>> [ 29.527476] r3 : 00000000 r2 : 00000002 r1 : ed1b5208 r0 : ed1b5208
>> [ 29.537127] Flags: nZCv IRQs on FIQs on Mode SVC_32 ISA ARM Segment none
>> [ 29.547454] Control: 10c5387d Table: 2d75c06a DAC: 00000051
>> [ 29.556215] Process kworker/u8:28 (pid: 3030, stack limit = 0xed27c220)
>> [ 29.565965] Stack: (0xed27dec8 to 0xed27e000)
>> [ 29.573150] dec0: 00000001 c087fb70 00000001
>> ed1b5208 00000000 00000010
>> [ 29.584656] dee0: ed1b523c ed27c000 00000000 c08802d4 c15c54fc
>> ed1b5208 ed542140 ee006500
>> [ 29.596172] df00: 00000000 c08804b8 ed542150 c157fda0 ed542140
>> c036525c ec820280 ed542150
>> [ 29.607694] df20: ee004800 ee006500 00000000 c035cc28 eef914c0
>> ee004848 ee004818 ee004800
>> [ 29.619249] df40: ec820298 00000088 ee004818 c1402d00 ed27c000
>> ee004800 ec820280 c035cf58
>> [ 29.630816] df60: ee004960 00000000 ec820280 ec802000 00000000
>> ed7d7ac0 ec820280 c035cf20
>> [ 29.642397] df80: ec80201c ec96bee8 00000000 c0362358 ed7d7ac0
>> c036222c 00000000 00000000
>> [ 29.654046] dfa0: 00000000 00000000 00000000 c0307e78 00000000
>> 00000000 00000000 00000000
>> [ 29.665707] dfc0: 00000000 00000000 00000000 00000000 00000000
>> 00000000 00000000 00000000
>> [ 29.677322] dfe0: 00000000 00000000 00000000 00000000 00000013
>> 00000000 00000000 00000000
>> [ 29.688968] [<bf4c0fc0>] (brcmf_ops_sdio_resume [brcmfmac]) from
>> [<c087fb70>] (dpm_run_callback+0x38/0x160)
>> [ 29.702379] [<c087fb70>] (dpm_run_callback) from [<c08802d4>]
>> (device_resume+0x94/0x25c)
>> [ 29.713951] [<c08802d4>] (device_resume) from [<c08804b8>]
>> (async_resume+0x1c/0x44)
>> [ 29.725063] [<c08804b8>] (async_resume) from [<c036525c>]
>> (async_run_entry_fn+0x44/0x108)
>> [ 29.736788] [<c036525c>] (async_run_entry_fn) from [<c035cc28>]
>> (process_one_work+0x14c/0x444)
>> [ 29.749022] [<c035cc28>] (process_one_work) from [<c035cf58>]
>> (worker_thread+0x38/0x510)
>> [ 29.760675] [<c035cf58>] (worker_thread) from [<c0362358>]
>> (kthread+0x12c/0x15c)
>> [ 29.771521] [<c0362358>] (kthread) from [<c0307e78>]
>> (ret_from_fork+0x14/0x3c)
>> [ 29.782226] Code: e92d4010 e59021a4 e5903054 e3520002 (e5934000)
>> [ 29.791751] ---[ end trace 035307c5087a8fee ]---
>>
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [RFT 0/3] brcmfmac: firmware loading rework
2017-05-31 12:45 ` Enric Balletbo Serra
@ 2017-05-31 19:32 ` Arend van Spriel
0 siblings, 0 replies; 10+ messages in thread
From: Arend van Spriel @ 2017-05-31 19:32 UTC (permalink / raw)
To: Enric Balletbo Serra; +Cc: Enric Balletbo i Serra, linux-wireless
On 31-05-17 14:45, Enric Balletbo Serra wrote:
> 2017-05-31 14:00 GMT+02:00 Arend van Spriel <arend.vanspriel@broadcom.com>:
>> On 31-05-17 13:10, Enric Balletbo Serra wrote:
>>> Hi Arend,
>>>
>>> 2017-05-30 14:35 GMT+02:00 Arend van Spriel <arend.vanspriel@broadcom.com>:
>>>> Hi Enric,
>>>>
>>>> Could you please try these patches and let me know if they resolve
>>>> the issue for you.
>>>>
>>>> Regards,
>>>> Arend
>>>>
>>>> Arend van Spriel (3):
>>>> brcmfmac: add parameter to pass error code in firmware callback
>>>> brcmfmac: use firmware callback upon failure to load
>>>> brcmfmac: unbind all devices upon failure in firmware callback
>>>>
>>>> .../broadcom/brcm80211/brcmfmac/firmware.c | 35 +++++++++++-----------
>>>> .../broadcom/brcm80211/brcmfmac/firmware.h | 4 +--
>>>> .../wireless/broadcom/brcm80211/brcmfmac/pcie.c | 17 +++++++----
>>>> .../wireless/broadcom/brcm80211/brcmfmac/sdio.c | 18 +++++++----
>>>> .../net/wireless/broadcom/brcm80211/brcmfmac/usb.c | 6 ++--
>>>> 5 files changed, 47 insertions(+), 33 deletions(-)
>>>>
>>>> --
>>>> 1.9.1
>>>>
>>>
>>> After these patches the error path when firmware is not found seems
>>> the correct way to proceed but I'm still getting the same kernel panic
>>> [1].
>>>
>>> I added some printk and I saw that when firmware load fails
>>>
>>> [ 7.696463] brcmfmac mmc1:0001:1: Direct firmware load for
>>> brcm/brcmfmac4354-sdio.bin failed with error -2
>>>
>>> The brcmf_sdio_firmware_callback fails, and goes to the fail label and
>>> the devices are released.
>>>
>>> device_release_driver(dev);
>>> device_release_driver(&sdiodev->func[1]->dev);
>>>
>>> But PM ops are not unregistered for mmc1:0001:2 and
>>> brcmf_ops_sdio_resume is still called, so I'm wondering if you missed
>>> here to add
>>>
>>> device_release_driver(&sdiodev->func[2]->dev);
>>>
>>> Adding that seems to solve the problem, not sure if there is any
>>> collateral effect.
>>
>> Looking closer at the firmware load failure message it uses mmc1:0001:1,
>> ie. func[1]->dev. I wrongly assume everything was using func[2]->dev
>> hence I added the device_release_driver() call for func[1]->dev. So we
>> are calling device_release_driver() twice for the same device. Just
>> change func[1] into func[2].
>>
>
> - device_release_driver(&sdiodev->func[1]->dev);
> + device_release_driver(&sdiodev->func[2]->dev);
>
> Right this works, thanks Arend. If you send a new patch revision you can add my
>
> Tested-by: Enric Balletbo i Serra <enric.balletbo@collabora.com>
Thanks for testing. Will update the patches.
Regards,
Arend
> Regards,
> Enric
>
>> Regards,
>> Arend
>>
>>> + info:
>>>
>>> Steps to reproduce the Oops:
>>>
>>> 1. Remove the brcm/brcmfmac4354-sdio.bin firmware
>>> 2. modprobe brcmfmac
>>> [ 6.879679] brcmfmac mmc1:0001:1: Direct firmware load for
>>> brcm/brcmfmac4354-sdio.bin failed with error -2
>>> 3. Do a suspend/resume cycle
>>> echo mem > /sys/power/state && # hit a key
>>>
>>> [1] Kernel Oops:
>>>
>>> [ 29.375149] Unable to handle kernel NULL pointer dereference at
>>> virtual address 00000000
>>> [ 29.384270] pgd = c0204000
>>> [ 29.387310] [00000000] *pgd=00000000
>>> [ 29.391336] Internal error: Oops: 17 [#1] SMP ARM
>>> [ 29.396628] Modules linked in: cpufreq_powersave cpufreq_userspace
>>> cpufreq_conservative bq27xxx_battery_i2c bq27xxx_battery cros_ec_keyb
>>> i2c_cros_ec_tunnel cros_ec_devs cros_ec_spi cros_ec_core
>>> snd_soc_rockchip_max98090 brcmfmac snd_soc_ts3a227e snd_soc_max98090
>>> snd_soc_rockchip_i2s cfg80211 snd_soc_core ac97_bus uvcvideo
>>> videobuf2_vmalloc videobuf2_memops videobuf2_v4l2 videobuf2_core
>>> videodev snd_pcm_dmaengine brcmutil snd_pcm gpio_charger snd_timer
>>> media nvmem_rockchip_efuse rk_crypto md5 sha256_generic sha1_generic
>>> des_generic phy_rockchip_dp rtc_rk808 snd soundcore pwm_rockchip
>>> rockchipdrm tpm_i2c_infineon dw_hdmi tpm analogix_dp pwm_bl
>>> spi_rockchip
>>> [ 29.461958] CPU: 0 PID: 3030 Comm: kworker/u8:28 Not tainted
>>> 4.12.0-rc2-next-20170529-00003-g38fc31b-dirty #3
>>> [ 29.473118] Hardware name: Rockchip (Device Tree)
>>> [ 29.478415] Workqueue: events_unbound async_run_entry_fn
>>> [ 29.484384] task: ed6dc200 task.stack: ed27c000
>>> [ 29.489494] PC is at brcmf_ops_sdio_resume+0x10/0x5c [brcmfmac]
>>> [ 29.496158] LR is at dpm_run_callback+0x38/0x160
>>> [ 29.501351] pc : [<bf4c0fc0>] lr : [<c087fb70>] psr: 60000113
>>> [ 29.508394] sp : ed27dec8 ip : 60000113 fp : c1572a54
>>> [ 29.514262] r10: 00000000 r9 : c0fe196c r8 : 00000010
>>> [ 29.520131] r7 : c087b900 r6 : 00000000 r5 : ed1b5208 r4 : 00000001
>>> [ 29.527476] r3 : 00000000 r2 : 00000002 r1 : ed1b5208 r0 : ed1b5208
>>> [ 29.537127] Flags: nZCv IRQs on FIQs on Mode SVC_32 ISA ARM Segment none
>>> [ 29.547454] Control: 10c5387d Table: 2d75c06a DAC: 00000051
>>> [ 29.556215] Process kworker/u8:28 (pid: 3030, stack limit = 0xed27c220)
>>> [ 29.565965] Stack: (0xed27dec8 to 0xed27e000)
>>> [ 29.573150] dec0: 00000001 c087fb70 00000001
>>> ed1b5208 00000000 00000010
>>> [ 29.584656] dee0: ed1b523c ed27c000 00000000 c08802d4 c15c54fc
>>> ed1b5208 ed542140 ee006500
>>> [ 29.596172] df00: 00000000 c08804b8 ed542150 c157fda0 ed542140
>>> c036525c ec820280 ed542150
>>> [ 29.607694] df20: ee004800 ee006500 00000000 c035cc28 eef914c0
>>> ee004848 ee004818 ee004800
>>> [ 29.619249] df40: ec820298 00000088 ee004818 c1402d00 ed27c000
>>> ee004800 ec820280 c035cf58
>>> [ 29.630816] df60: ee004960 00000000 ec820280 ec802000 00000000
>>> ed7d7ac0 ec820280 c035cf20
>>> [ 29.642397] df80: ec80201c ec96bee8 00000000 c0362358 ed7d7ac0
>>> c036222c 00000000 00000000
>>> [ 29.654046] dfa0: 00000000 00000000 00000000 c0307e78 00000000
>>> 00000000 00000000 00000000
>>> [ 29.665707] dfc0: 00000000 00000000 00000000 00000000 00000000
>>> 00000000 00000000 00000000
>>> [ 29.677322] dfe0: 00000000 00000000 00000000 00000000 00000013
>>> 00000000 00000000 00000000
>>> [ 29.688968] [<bf4c0fc0>] (brcmf_ops_sdio_resume [brcmfmac]) from
>>> [<c087fb70>] (dpm_run_callback+0x38/0x160)
>>> [ 29.702379] [<c087fb70>] (dpm_run_callback) from [<c08802d4>]
>>> (device_resume+0x94/0x25c)
>>> [ 29.713951] [<c08802d4>] (device_resume) from [<c08804b8>]
>>> (async_resume+0x1c/0x44)
>>> [ 29.725063] [<c08804b8>] (async_resume) from [<c036525c>]
>>> (async_run_entry_fn+0x44/0x108)
>>> [ 29.736788] [<c036525c>] (async_run_entry_fn) from [<c035cc28>]
>>> (process_one_work+0x14c/0x444)
>>> [ 29.749022] [<c035cc28>] (process_one_work) from [<c035cf58>]
>>> (worker_thread+0x38/0x510)
>>> [ 29.760675] [<c035cf58>] (worker_thread) from [<c0362358>]
>>> (kthread+0x12c/0x15c)
>>> [ 29.771521] [<c0362358>] (kthread) from [<c0307e78>]
>>> (ret_from_fork+0x14/0x3c)
>>> [ 29.782226] Code: e92d4010 e59021a4 e5903054 e3520002 (e5934000)
>>> [ 29.791751] ---[ end trace 035307c5087a8fee ]---
>>>
^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2017-05-31 19:32 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-05-30 12:35 [RFT 0/3] brcmfmac: firmware loading rework Arend van Spriel
2017-05-30 12:35 ` [RFT 1/3] brcmfmac: add parameter to pass error code in firmware callback Arend van Spriel
2017-05-31 11:16 ` Enric Balletbo Serra
2017-05-30 12:35 ` [RFT 2/3] brcmfmac: use firmware callback upon failure to load Arend van Spriel
2017-05-30 12:35 ` [RFT 3/3] brcmfmac: unbind all devices upon failure in firmware callback Arend van Spriel
2017-05-31 11:13 ` Enric Balletbo Serra
2017-05-31 11:10 ` [RFT 0/3] brcmfmac: firmware loading rework Enric Balletbo Serra
2017-05-31 12:00 ` Arend van Spriel
2017-05-31 12:45 ` Enric Balletbo Serra
2017-05-31 19:32 ` Arend van Spriel
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.