These three patches fix a reported issue that results in a crash
for sdio devices, because when the probe fails due to firmware load
failure or other reasons, the driver does not unbind all devices but
frees all resources. This causes a null-deref upon resuming from
system suspend. The solution has been split up.
This patch series is intended for 4.12 and applies to the master
branch of the wireless-drivers repository.
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
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.
Cc: [email protected] # 4.9.x-
Reviewed-by: Hante Meuleman <[email protected]>
Reviewed-by: Pieter-Paul Giesberts <[email protected]>
Reviewed-by: Franky Lin <[email protected]>
Signed-off-by: Arend van Spriel <[email protected]>
---
.../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 e034500..6e1fcdc 100644
--- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c
+++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c
@@ -3982,21 +3982,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
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.
Cc: [email protected] # 4.9.x-
Reviewed-by: Hante Meuleman <[email protected]>
Reviewed-by: Pieter-Paul Giesberts <[email protected]>
Reviewed-by: Franky Lin <[email protected]>
Signed-off-by: Arend van Spriel <[email protected]>
---
.../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
Arend Van Spriel <[email protected]> wrote:
> 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.
>
> Cc: [email protected] # 4.9.x-
> Reviewed-by: Hante Meuleman <[email protected]>
> Reviewed-by: Pieter-Paul Giesberts <[email protected]>
> Reviewed-by: Franky Lin <[email protected]>
> Signed-off-by: Arend van Spriel <[email protected]>
3 patches applied to wireless-drivers.git, thanks.
6d0507a777fb brcmfmac: add parameter to pass error code in firmware callback
03fb0e8393fa brcmfmac: use firmware callback upon failure to load
7a51461fc2da brcmfmac: unbind all devices upon failure in firmware callback
--
https://patchwork.kernel.org/patch/9780793/
https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches
Arend Van Spriel <[email protected]> wrote:
> In brcmf_sdio_firmware_callback() we need to unbind the driver from
> both sdio_func devices.
>
> Cc: [email protected] # 4.9.x-
> Tested-by: Enric Balletbo i Serra <[email protected]>
> Reviewed-by: Hante Meuleman <[email protected]>
> Reviewed-by: Pieter-Paul Giesberts <[email protected]>
> Reviewed-by: Franky Lin <[email protected]>
> Signed-off-by: Arend van Spriel <[email protected]>
The commit log doesn't really describe the bug you are fixing (ie. doesn't
answer "why?"). Can you give some more info, logs etc (as a reply to this mail)
and I'll add it.
--
https://patchwork.kernel.org/patch/9780797/
https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches
Arend van Spriel <[email protected]> writes:
> 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.
>
> Cc: [email protected] # 4.9.x-
> Reviewed-by: Hante Meuleman <[email protected]>
> Reviewed-by: Pieter-Paul Giesberts <[email protected]>
> Reviewed-by: Franky Lin <[email protected]>
> Signed-off-by: Arend van Spriel <[email protected]>
[...]
> -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 =3D dev_get_drvdata(dev);
> struct brcmf_usbdev_info *devinfo;
> - int ret;
> +
> + if (ret)
> + goto error;
This introduces a new warning:
drivers/net/wireless/broadcom/brcm80211/brcmfmac/usb.c:1198:2: warning:
=E2=80=98devinfo=E2=80=99 may be used uninitialized in this function
[-Wmaybe-uninitialized]
>From a quick look I think it's valid, but due to my mistake I only
noticed it after I had applied the patch. So can you send a small
followup patch to fix this, please?
--=20
Kalle Valo
In brcmf_sdio_firmware_callback() we need to unbind the driver from
both sdio_func devices.
Cc: [email protected] # 4.9.x-
Tested-by: Enric Balletbo i Serra <[email protected]>
Reviewed-by: Hante Meuleman <[email protected]>
Reviewed-by: Pieter-Paul Giesberts <[email protected]>
Reviewed-by: Franky Lin <[email protected]>
Signed-off-by: Arend van Spriel <[email protected]>
---
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 6e1fcdc..5653d6d 100644
--- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c
+++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c
@@ -3992,14 +3992,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 */
@@ -4088,6 +4088,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[2]->dev);
}
struct brcmf_sdio *brcmf_sdio_probe(struct brcmf_sdio_dev *sdiodev)
--
1.9.1