Hi everyone,
This series contains just the fixes / misc improvements from the
previously submitted series:
brcmfmac: Support Apple T2 and M1 platforms
Patches 8-9 aren't strictly bugfixes but rather just general
improvements; they can be safely skipped, although patch 8 will be a
dependency of the subsequent series to avoid a compile warning.
Hector Martin (9):
brcmfmac: pcie: Release firmwares in the brcmf_pcie_setup error path
brcmfmac: firmware: Allocate space for default boardrev in nvram
brcmfmac: firmware: Do not crash on a NULL board_type
brcmfmac: pcie: Declare missing firmware files in pcie.c
brcmfmac: pcie: Replace brcmf_pcie_copy_mem_todev with memcpy_toio
brcmfmac: pcie: Fix crashes due to early IRQs
brcmfmac: of: Use devm_kstrdup for board_type & check for errors
brcmfmac: fwil: Constify iovar name arguments
brcmfmac: pcie: Read the console on init and shutdown
.../broadcom/brcm80211/brcmfmac/firmware.c | 5 ++
.../broadcom/brcm80211/brcmfmac/fwil.c | 34 ++++----
.../broadcom/brcm80211/brcmfmac/fwil.h | 28 +++----
.../wireless/broadcom/brcm80211/brcmfmac/of.c | 8 +-
.../broadcom/brcm80211/brcmfmac/pcie.c | 77 ++++++++-----------
.../broadcom/brcm80211/brcmfmac/sdio.c | 1 -
6 files changed, 72 insertions(+), 81 deletions(-)
--
2.33.0
This avoids leaking memory if brcmf_chip_get_raminfo fails. Note that
the CLM blob is released in the device remove path.
Fixes: 82f93cf46d60 ("brcmfmac: get chip's default RAM info during PCIe setup")
Reviewed-by: Linus Walleij <[email protected]>
Signed-off-by: Hector Martin <[email protected]>
---
drivers/net/wireless/broadcom/brcm80211/brcmfmac/pcie.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/pcie.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/pcie.c
index 8b149996fc00..f876b1d8d00d 100644
--- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/pcie.c
+++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/pcie.c
@@ -1777,6 +1777,8 @@ static void brcmf_pcie_setup(struct device *dev, int ret,
ret = brcmf_chip_get_raminfo(devinfo->ci);
if (ret) {
brcmf_err(bus, "Failed to get RAM info\n");
+ release_firmware(fw);
+ brcmf_fw_nvram_free(nvram);
goto fail;
}
--
2.33.0
If boardrev is missing from the NVRAM we add a default one, but this
might need more space in the output buffer than was allocated. Ensure
we have enough padding for this in the buffer.
Fixes: 46f2b38a91b0 ("brcmfmac: insert default boardrev in nvram data if missing")
Signed-off-by: Hector Martin <[email protected]>
---
drivers/net/wireless/broadcom/brcm80211/brcmfmac/firmware.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/firmware.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/firmware.c
index 0eb13e5df517..1001c8888bfe 100644
--- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/firmware.c
+++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/firmware.c
@@ -207,6 +207,8 @@ static int brcmf_init_nvram_parser(struct nvram_parser *nvp,
size = BRCMF_FW_MAX_NVRAM_SIZE;
else
size = data_len;
+ /* Add space for properties we may add */
+ size += strlen(BRCMF_FW_DEFAULT_BOARDREV) + 1;
/* Alloc for extra 0 byte + roundup by 4 + length field */
size += 1 + 3 + sizeof(u32);
nvp->nvram = kzalloc(size, GFP_KERNEL);
--
2.33.0
This unbreaks support for USB devices, which do not have a board_type
to create an alt_path out of and thus were running into a NULL
dereference.
Fixes: 5ff013914c62 ("brcmfmac: firmware: Allow per-board firmware binaries")
Signed-off-by: Hector Martin <[email protected]>
---
drivers/net/wireless/broadcom/brcm80211/brcmfmac/firmware.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/firmware.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/firmware.c
index 1001c8888bfe..63821856bbe1 100644
--- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/firmware.c
+++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/firmware.c
@@ -599,6 +599,9 @@ static char *brcm_alt_fw_path(const char *path, const char *board_type)
char alt_path[BRCMF_FW_NAME_LEN];
char suffix[5];
+ if (!board_type)
+ return NULL;
+
strscpy(alt_path, path, BRCMF_FW_NAME_LEN);
/* At least one character + suffix */
if (strlen(alt_path) < 5)
--
2.33.0
The alignment check was wrong (e.g. & 4 instead of & 3), and the logic
was also inefficient if the length was not a multiple of 4, since it
would needlessly fall back to copying the entire buffer bytewise.
We already have a perfectly good memcpy_toio function, so just call that
instead of rolling our own copy logic here. brcmf_pcie_init_ringbuffers
was already using it anyway.
Fixes: 9e37f045d5e7 ("brcmfmac: Adding PCIe bus layer support.")
Reviewed-by: Linus Walleij <[email protected]>
Signed-off-by: Hector Martin <[email protected]>
---
.../broadcom/brcm80211/brcmfmac/pcie.c | 48 ++-----------------
1 file changed, 4 insertions(+), 44 deletions(-)
diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/pcie.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/pcie.c
index b1ae6c41013f..c25f48db1f60 100644
--- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/pcie.c
+++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/pcie.c
@@ -12,6 +12,7 @@
#include <linux/interrupt.h>
#include <linux/bcma/bcma.h>
#include <linux/sched.h>
+#include <linux/io.h>
#include <asm/unaligned.h>
#include <soc.h>
@@ -454,47 +455,6 @@ brcmf_pcie_write_ram32(struct brcmf_pciedev_info *devinfo, u32 mem_offset,
}
-static void
-brcmf_pcie_copy_mem_todev(struct brcmf_pciedev_info *devinfo, u32 mem_offset,
- void *srcaddr, u32 len)
-{
- void __iomem *address = devinfo->tcm + mem_offset;
- __le32 *src32;
- __le16 *src16;
- u8 *src8;
-
- if (((ulong)address & 4) || ((ulong)srcaddr & 4) || (len & 4)) {
- if (((ulong)address & 2) || ((ulong)srcaddr & 2) || (len & 2)) {
- src8 = (u8 *)srcaddr;
- while (len) {
- iowrite8(*src8, address);
- address++;
- src8++;
- len--;
- }
- } else {
- len = len / 2;
- src16 = (__le16 *)srcaddr;
- while (len) {
- iowrite16(le16_to_cpu(*src16), address);
- address += 2;
- src16++;
- len--;
- }
- }
- } else {
- len = len / 4;
- src32 = (__le32 *)srcaddr;
- while (len) {
- iowrite32(le32_to_cpu(*src32), address);
- address += 4;
- src32++;
- len--;
- }
- }
-}
-
-
static void
brcmf_pcie_copy_dev_tomem(struct brcmf_pciedev_info *devinfo, u32 mem_offset,
void *dstaddr, u32 len)
@@ -1570,8 +1530,8 @@ static int brcmf_pcie_download_fw_nvram(struct brcmf_pciedev_info *devinfo,
return err;
brcmf_dbg(PCIE, "Download FW %s\n", devinfo->fw_name);
- brcmf_pcie_copy_mem_todev(devinfo, devinfo->ci->rambase,
- (void *)fw->data, fw->size);
+ memcpy_toio(devinfo->tcm + devinfo->ci->rambase,
+ (void *)fw->data, fw->size);
resetintr = get_unaligned_le32(fw->data);
release_firmware(fw);
@@ -1585,7 +1545,7 @@ static int brcmf_pcie_download_fw_nvram(struct brcmf_pciedev_info *devinfo,
brcmf_dbg(PCIE, "Download NVRAM %s\n", devinfo->nvram_name);
address = devinfo->ci->rambase + devinfo->ci->ramsize -
nvram_len;
- brcmf_pcie_copy_mem_todev(devinfo, address, nvram, nvram_len);
+ memcpy_toio(devinfo->tcm + address, nvram, nvram_len);
brcmf_fw_nvram_free(nvram);
} else {
brcmf_dbg(PCIE, "No matching NVRAM file found %s\n",
--
2.33.0
This was missing a NULL check, and we can collapse the strlen/alloc/copy
into a devm_kstrdup().
Signed-off-by: Hector Martin <[email protected]>
---
drivers/net/wireless/broadcom/brcm80211/brcmfmac/of.c | 8 ++++++--
1 file changed, 6 insertions(+), 2 deletions(-)
diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/of.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/of.c
index 513c7e6421b2..5dc1e942e9e7 100644
--- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/of.c
+++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/of.c
@@ -79,8 +79,12 @@ void brcmf_of_probe(struct device *dev, enum brcmf_bus_type bus_type,
/* get rid of '/' in the compatible string to be able to find the FW */
len = strlen(tmp) + 1;
- board_type = devm_kzalloc(dev, len, GFP_KERNEL);
- strscpy(board_type, tmp, len);
+ board_type = devm_kstrdup(dev, tmp, GFP_KERNEL);
+ if (!board_type) {
+ brcmf_err("out of memory allocating board_type\n");
+ of_node_put(root);
+ return;
+ }
for (i = 0; i < board_type[i]; i++) {
if (board_type[i] == '/')
board_type[i] = '-';
--
2.33.0
Move one of the declarations from sdio.c to pcie.c, since it makes no
sense in the former (SDIO support is optional), and add missing ones.
Fixes: 75729e110e68 ("brcmfmac: expose firmware config files through modinfo")
Reviewed-by: Linus Walleij <[email protected]>
Reviewed-by: Arend van Spriel <[email protected]>
Signed-off-by: Hector Martin <[email protected]>
---
drivers/net/wireless/broadcom/brcm80211/brcmfmac/pcie.c | 7 +++++++
drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c | 1 -
2 files changed, 7 insertions(+), 1 deletion(-)
diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/pcie.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/pcie.c
index f876b1d8d00d..b1ae6c41013f 100644
--- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/pcie.c
+++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/pcie.c
@@ -59,6 +59,13 @@ BRCMF_FW_DEF(4366B, "brcmfmac4366b-pcie");
BRCMF_FW_DEF(4366C, "brcmfmac4366c-pcie");
BRCMF_FW_DEF(4371, "brcmfmac4371-pcie");
+/* firmware config files */
+MODULE_FIRMWARE(BRCMF_FW_DEFAULT_PATH "brcmfmac*-pcie.txt");
+MODULE_FIRMWARE(BRCMF_FW_DEFAULT_PATH "brcmfmac*-pcie.*.txt");
+
+/* per-board firmware binaries */
+MODULE_FIRMWARE(BRCMF_FW_DEFAULT_PATH "brcmfmac*-pcie.*.bin");
+
static const struct brcmf_firmware_mapping brcmf_pcie_fwnames[] = {
BRCMF_FW_ENTRY(BRCM_CC_43602_CHIP_ID, 0xFFFFFFFF, 43602),
BRCMF_FW_ENTRY(BRCM_CC_43465_CHIP_ID, 0xFFFFFFF0, 4366C),
diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c
index 8effeb7a7269..5d156e591b35 100644
--- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c
+++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c
@@ -629,7 +629,6 @@ BRCMF_FW_CLM_DEF(43752, "brcmfmac43752-sdio");
/* firmware config files */
MODULE_FIRMWARE(BRCMF_FW_DEFAULT_PATH "brcmfmac*-sdio.*.txt");
-MODULE_FIRMWARE(BRCMF_FW_DEFAULT_PATH "brcmfmac*-pcie.*.txt");
/* per-board firmware binaries */
MODULE_FIRMWARE(BRCMF_FW_DEFAULT_PATH "brcmfmac*-sdio.*.bin");
--
2.33.0
The driver was enabling IRQs before the message processing was
initialized. This could cause IRQs to come in too early and crash the
driver. Instead, move the IRQ enable and hostready to a bus preinit
function, at which point everything is properly initialized.
Fixes: 9e37f045d5e7 ("brcmfmac: Adding PCIe bus layer support.")
Reviewed-by: Linus Walleij <[email protected]>
Reviewed-by: Arend van Spriel <[email protected]>
Signed-off-by: Hector Martin <[email protected]>
---
.../wireless/broadcom/brcm80211/brcmfmac/pcie.c | 16 +++++++++++++---
1 file changed, 13 insertions(+), 3 deletions(-)
diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/pcie.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/pcie.c
index c25f48db1f60..3ff4997e1c97 100644
--- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/pcie.c
+++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/pcie.c
@@ -1315,6 +1315,18 @@ static void brcmf_pcie_down(struct device *dev)
{
}
+static int brcmf_pcie_preinit(struct device *dev)
+{
+ struct brcmf_bus *bus_if = dev_get_drvdata(dev);
+ struct brcmf_pciedev *buspub = bus_if->bus_priv.pcie;
+
+ brcmf_dbg(PCIE, "Enter\n");
+
+ brcmf_pcie_intr_enable(buspub->devinfo);
+ brcmf_pcie_hostready(buspub->devinfo);
+
+ return 0;
+}
static int brcmf_pcie_tx(struct device *dev, struct sk_buff *skb)
{
@@ -1423,6 +1435,7 @@ static int brcmf_pcie_reset(struct device *dev)
}
static const struct brcmf_bus_ops brcmf_pcie_bus_ops = {
+ .preinit = brcmf_pcie_preinit,
.txdata = brcmf_pcie_tx,
.stop = brcmf_pcie_down,
.txctl = brcmf_pcie_tx_ctlpkt,
@@ -1795,9 +1808,6 @@ static void brcmf_pcie_setup(struct device *dev, int ret,
init_waitqueue_head(&devinfo->mbdata_resp_wait);
- brcmf_pcie_intr_enable(devinfo);
- brcmf_pcie_hostready(devinfo);
-
ret = brcmf_attach(&devinfo->pdev->dev);
if (ret)
goto fail;
--
2.33.0
This allows us to get console messages if the firmware crashed during
early init, or if an operation failed and we're about to shut down.
Reviewed-by: Linus Walleij <[email protected]>
Signed-off-by: Hector Martin <[email protected]>
---
drivers/net/wireless/broadcom/brcm80211/brcmfmac/pcie.c | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/pcie.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/pcie.c
index 3ff4997e1c97..4fe341376a16 100644
--- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/pcie.c
+++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/pcie.c
@@ -744,6 +744,8 @@ static void brcmf_pcie_bus_console_read(struct brcmf_pciedev_info *devinfo,
return;
console = &devinfo->shared.console;
+ if (!console->base_addr)
+ return;
addr = console->base_addr + BRCMF_CONSOLE_WRITEIDX_OFFSET;
newidx = brcmf_pcie_read_tcm32(devinfo, addr);
while (newidx != console->read_idx) {
@@ -1520,6 +1522,7 @@ brcmf_pcie_init_share_ram_info(struct brcmf_pciedev_info *devinfo,
shared->max_rxbufpost, shared->rx_dataoffset);
brcmf_pcie_bus_console_init(devinfo);
+ brcmf_pcie_bus_console_read(devinfo, false);
return 0;
}
@@ -1959,6 +1962,7 @@ brcmf_pcie_remove(struct pci_dev *pdev)
return;
devinfo = bus->bus_priv.pcie->devinfo;
+ brcmf_pcie_bus_console_read(devinfo, false);
devinfo->state = BRCMFMAC_PCIE_STATE_DOWN;
if (devinfo->ci)
--
2.33.0
On Mon, Jan 17, 2022 at 4:30 PM Hector Martin <[email protected]> wrote:
>
> Hi everyone,
>
> This series contains just the fixes / misc improvements from the
> previously submitted series:
>
> brcmfmac: Support Apple T2 and M1 platforms
>
> Patches 8-9 aren't strictly bugfixes but rather just general
> improvements; they can be safely skipped, although patch 8 will be a
> dependency of the subsequent series to avoid a compile warning.
Have I given you a tag? If so, I do not see it applied in the patches...
> Hector Martin (9):
> brcmfmac: pcie: Release firmwares in the brcmf_pcie_setup error path
> brcmfmac: firmware: Allocate space for default boardrev in nvram
> brcmfmac: firmware: Do not crash on a NULL board_type
> brcmfmac: pcie: Declare missing firmware files in pcie.c
> brcmfmac: pcie: Replace brcmf_pcie_copy_mem_todev with memcpy_toio
> brcmfmac: pcie: Fix crashes due to early IRQs
> brcmfmac: of: Use devm_kstrdup for board_type & check for errors
> brcmfmac: fwil: Constify iovar name arguments
> brcmfmac: pcie: Read the console on init and shutdown
>
> .../broadcom/brcm80211/brcmfmac/firmware.c | 5 ++
> .../broadcom/brcm80211/brcmfmac/fwil.c | 34 ++++----
> .../broadcom/brcm80211/brcmfmac/fwil.h | 28 +++----
> .../wireless/broadcom/brcm80211/brcmfmac/of.c | 8 +-
> .../broadcom/brcm80211/brcmfmac/pcie.c | 77 ++++++++-----------
> .../broadcom/brcm80211/brcmfmac/sdio.c | 1 -
> 6 files changed, 72 insertions(+), 81 deletions(-)
>
> --
> 2.33.0
>
--
With Best Regards,
Andy Shevchenko
On 18/01/2022 19.43, Andy Shevchenko wrote:
> On Mon, Jan 17, 2022 at 4:30 PM Hector Martin <[email protected]> wrote:
>>
>> Hi everyone,
>>
>> This series contains just the fixes / misc improvements from the
>> previously submitted series:
>>
>> brcmfmac: Support Apple T2 and M1 platforms
>>
>> Patches 8-9 aren't strictly bugfixes but rather just general
>> improvements; they can be safely skipped, although patch 8 will be a
>> dependency of the subsequent series to avoid a compile warning.
>
> Have I given you a tag? If so, I do not see it applied in the patches...
I didn't see any review tags from you in the previous thread. Did I miss
any?
--
Hector Martin ([email protected])
Public Key: https://mrcn.st/pub
On 1/17/2022 3:29 PM, Hector Martin wrote:
> This unbreaks support for USB devices, which do not have a board_type
> to create an alt_path out of and thus were running into a NULL
> dereference.
>
> Fixes: 5ff013914c62 ("brcmfmac: firmware: Allow per-board firmware binaries")
Reviewed-by: Arend van Spriel <[email protected]>
> Signed-off-by: Hector Martin <[email protected]>
> ---
> drivers/net/wireless/broadcom/brcm80211/brcmfmac/firmware.c | 3 +++
> 1 file changed, 3 insertions(+)
On 1/17/2022 3:29 PM, Hector Martin wrote:
> This avoids leaking memory if brcmf_chip_get_raminfo fails. Note that
> the CLM blob is released in the device remove path.
>
> Fixes: 82f93cf46d60 ("brcmfmac: get chip's default RAM info during PCIe setup")
Reviewed-by: Arend van Spriel <[email protected]>
> Reviewed-by: Linus Walleij <[email protected]>
> Signed-off-by: Hector Martin <[email protected]>
> ---
> drivers/net/wireless/broadcom/brcm80211/brcmfmac/pcie.c | 2 ++
> 1 file changed, 2 insertions(+)
On 1/17/2022 3:29 PM, Hector Martin wrote:
> If boardrev is missing from the NVRAM we add a default one, but this
> might need more space in the output buffer than was allocated. Ensure
> we have enough padding for this in the buffer.
>
> Fixes: 46f2b38a91b0 ("brcmfmac: insert default boardrev in nvram data if missing")
Reviewed-by: Arend van Spriel <[email protected]>
> Signed-off-by: Hector Martin <[email protected]>
> ---
> drivers/net/wireless/broadcom/brcm80211/brcmfmac/firmware.c | 2 ++
> 1 file changed, 2 insertions(+)
On 1/17/2022 3:29 PM, Hector Martin wrote:
> The alignment check was wrong (e.g. & 4 instead of & 3), and the logic
> was also inefficient if the length was not a multiple of 4, since it
> would needlessly fall back to copying the entire buffer bytewise.
>
> We already have a perfectly good memcpy_toio function, so just call that
> instead of rolling our own copy logic here. brcmf_pcie_init_ringbuffers
> was already using it anyway.
>
> Fixes: 9e37f045d5e7 ("brcmfmac: Adding PCIe bus layer support.")
> Reviewed-by: Linus Walleij <[email protected]>
Reviewed-by: Arend van Spriel <[email protected]>
> Signed-off-by: Hector Martin <[email protected]>
> ---
> .../broadcom/brcm80211/brcmfmac/pcie.c | 48 ++-----------------
> 1 file changed, 4 insertions(+), 44 deletions(-)
>
> diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/pcie.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/pcie.c
> index b1ae6c41013f..c25f48db1f60 100644
> --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/pcie.c
> +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/pcie.c
> @@ -12,6 +12,7 @@
> #include <linux/interrupt.h>
> #include <linux/bcma/bcma.h>
> #include <linux/sched.h>
> +#include <linux/io.h>
As brcmf_pcie_init_ringbuffers was already using it I suppose this was
already implicitly included. Still good to make it explicit.
> #include <asm/unaligned.h>
>
> #include <soc.h>
On Mon, Jan 17, 2022 at 4:30 PM Hector Martin <[email protected]> wrote:
>
> This avoids leaking memory if brcmf_chip_get_raminfo fails. Note that
> the CLM blob is released in the device remove path.
...
> if (ret) {
> brcmf_err(bus, "Failed to get RAM info\n");
> + release_firmware(fw);
> + brcmf_fw_nvram_free(nvram);
Can we first undo the things and only after print a message?
> goto fail;
> }
--
With Best Regards,
Andy Shevchenko
On Wed, Jan 19, 2022 at 11:22 PM Dmitry Osipenko <[email protected]> wrote:
>
> 19.01.2022 20:49, Andy Shevchenko пишет:
> > On Mon, Jan 17, 2022 at 4:30 PM Hector Martin <[email protected]> wrote:
> >>
> >> This avoids leaking memory if brcmf_chip_get_raminfo fails. Note that
> >> the CLM blob is released in the device remove path.
> >
> > ...
> >
> >> if (ret) {
> >
> >> brcmf_err(bus, "Failed to get RAM info\n");
> >> + release_firmware(fw);
> >> + brcmf_fw_nvram_free(nvram);
> >
> > Can we first undo the things and only after print a message?
>
> Having message first usually is more preferred because at minimum you'll
> get the message if "undoing the things" crashes, i.e. will be more
> obvious what happened.
If "undo the things" crashes, I would rather like to see that crash
report, while serial UART at 9600 will continue flushing the message
and then hang without any pointers to what the heck happened. Not
here, but in general, messages are also good to be out of the locks.
--
With Best Regards,
Andy Shevchenko
On Mon, Jan 17, 2022 at 4:30 PM Hector Martin <[email protected]> wrote:
>
> If boardrev is missing from the NVRAM we add a default one, but this
> might need more space in the output buffer than was allocated. Ensure
> we have enough padding for this in the buffer.
Do you know this ahead (before allocation happens)?
If yes, the size change should happen conditionally.
Alternatively, krealloc() may be used.
--
With Best Regards,
Andy Shevchenko
On Mon, Jan 17, 2022 at 4:30 PM Hector Martin <[email protected]> wrote:
>
> This unbreaks support for USB devices, which do not have a board_type
> to create an alt_path out of and thus were running into a NULL
> dereference.
In v5.16 we have two call sites:
1.
if (cur->type == BRCMF_FW_TYPE_NVRAM && fwctx->req->board_type) {
...
alt_path = brcm_alt_fw_path(cur->path, fwctx->req->board_type);
2.
alt_path = brcm_alt_fw_path(first->path, fwctx->req->board_type);
if (alt_path) {
...
Looking at them I would rather expect to see (as a quick fix, the
better solution is to unify those call sites by splitting out a common
helper):
if (fwctx->req->board_type) {
alt_path = brcm_alt_fw_path(first->path, fwctx->req->board_type);
else
alt_path = NULL;
...
> @@ -599,6 +599,9 @@ static char *brcm_alt_fw_path(const char *path, const char *board_type)
> char alt_path[BRCMF_FW_NAME_LEN];
> char suffix[5];
>
> + if (!board_type)
> + return NULL;
--
With Best Regards,
Andy Shevchenko
19.01.2022 20:49, Andy Shevchenko пишет:
> On Mon, Jan 17, 2022 at 4:30 PM Hector Martin <[email protected]> wrote:
>>
>> This avoids leaking memory if brcmf_chip_get_raminfo fails. Note that
>> the CLM blob is released in the device remove path.
>
> ...
>
>> if (ret) {
>
>> brcmf_err(bus, "Failed to get RAM info\n");
>> + release_firmware(fw);
>> + brcmf_fw_nvram_free(nvram);
>
> Can we first undo the things and only after print a message?
Having message first usually is more preferred because at minimum you'll
get the message if "undoing the things" crashes, i.e. will be more
obvious what happened.
On 20/01/2022 06.35, Andy Shevchenko wrote:
> On Mon, Jan 17, 2022 at 4:30 PM Hector Martin <[email protected]> wrote:
>>
>> If boardrev is missing from the NVRAM we add a default one, but this
>> might need more space in the output buffer than was allocated. Ensure
>> we have enough padding for this in the buffer.
>
> Do you know this ahead (before allocation happens)?
> If yes, the size change should happen conditionally.
>
> Alternatively, krealloc() may be used.
We don't know if we will have to add a boardrev until we find it's
missing. The buffer is already oversized anyway, as its size is computed
based on the input NVRAM size, while comments and such will be removed.
This is just increasing the existing worst-case allocation to properly
account for the true worst-case case.
I don't think trying to keep the buffer perfectly sized buys us anything
here, since it will be freed after download anyway. There's no point in
saving a few bytes of memory in the interim.
--
Hector Martin ([email protected])
Public Key: https://mrcn.st/pub
On 1/19/2022 6:49 PM, Andy Shevchenko wrote:
> On Mon, Jan 17, 2022 at 4:30 PM Hector Martin <[email protected]> wrote:
>>
>> This avoids leaking memory if brcmf_chip_get_raminfo fails. Note that
>> the CLM blob is released in the device remove path.
>
> ...
>
>> if (ret) {
>
>> brcmf_err(bus, "Failed to get RAM info\n");
>> + release_firmware(fw);
>> + brcmf_fw_nvram_free(nvram);
>
> Can we first undo the things and only after print a message?
What would be your motivation? When reading logs I am used to seeing an
error message followed by cleanup related messages. Following your
suggestion you could see cleanup related messages, the error print as
above, followed by more cleanup related messages. The cleanup routine
would preferably be silent, but I tend to flip on extra debug message
levels.
Regards,
Arend
>> goto fail;
>> }
>
>
On 1/19/2022 11:02 PM, Dmitry Osipenko wrote:
> 17.01.2022 17:29, Hector Martin пишет:
>> This unbreaks support for USB devices, which do not have a board_type
>> to create an alt_path out of and thus were running into a NULL
>> dereference.
>>
>> Fixes: 5ff013914c62 ("brcmfmac: firmware: Allow per-board firmware binaries")
>> Signed-off-by: Hector Martin <[email protected]>
>
> Technically, all patches that are intended to be included into next
> stable kernel update require the "Cc: [email protected]" tag.
Being the nit picker that I am I would say it is recommended to safe
yourself extra work, not required, for the reason you give below.
> In practice such patches usually auto-picked by the patch bot, so no
> need to resend.
Regards,
Arend
On Mon, Jan 17, 2022 at 4:31 PM Hector Martin <[email protected]> wrote:
>
> The alignment check was wrong (e.g. & 4 instead of & 3), and the logic
> was also inefficient if the length was not a multiple of 4, since it
> would needlessly fall back to copying the entire buffer bytewise.
>
> We already have a perfectly good memcpy_toio function, so just call that
> instead of rolling our own copy logic here. brcmf_pcie_init_ringbuffers
> was already using it anyway.
My gosh, what a nice fix!
Reviewed-by: Andy Shevchenko <[email protected]>
> Fixes: 9e37f045d5e7 ("brcmfmac: Adding PCIe bus layer support.")
> Reviewed-by: Linus Walleij <[email protected]>
> Signed-off-by: Hector Martin <[email protected]>
> ---
> .../broadcom/brcm80211/brcmfmac/pcie.c | 48 ++-----------------
> 1 file changed, 4 insertions(+), 44 deletions(-)
>
> diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/pcie.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/pcie.c
> index b1ae6c41013f..c25f48db1f60 100644
> --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/pcie.c
> +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/pcie.c
> @@ -12,6 +12,7 @@
> #include <linux/interrupt.h>
> #include <linux/bcma/bcma.h>
> #include <linux/sched.h>
> +#include <linux/io.h>
> #include <asm/unaligned.h>
>
> #include <soc.h>
> @@ -454,47 +455,6 @@ brcmf_pcie_write_ram32(struct brcmf_pciedev_info *devinfo, u32 mem_offset,
> }
>
>
> -static void
> -brcmf_pcie_copy_mem_todev(struct brcmf_pciedev_info *devinfo, u32 mem_offset,
> - void *srcaddr, u32 len)
> -{
> - void __iomem *address = devinfo->tcm + mem_offset;
> - __le32 *src32;
> - __le16 *src16;
> - u8 *src8;
> -
> - if (((ulong)address & 4) || ((ulong)srcaddr & 4) || (len & 4)) {
> - if (((ulong)address & 2) || ((ulong)srcaddr & 2) || (len & 2)) {
> - src8 = (u8 *)srcaddr;
> - while (len) {
> - iowrite8(*src8, address);
> - address++;
> - src8++;
> - len--;
> - }
> - } else {
> - len = len / 2;
> - src16 = (__le16 *)srcaddr;
> - while (len) {
> - iowrite16(le16_to_cpu(*src16), address);
> - address += 2;
> - src16++;
> - len--;
> - }
> - }
> - } else {
> - len = len / 4;
> - src32 = (__le32 *)srcaddr;
> - while (len) {
> - iowrite32(le32_to_cpu(*src32), address);
> - address += 4;
> - src32++;
> - len--;
> - }
> - }
> -}
> -
> -
> static void
> brcmf_pcie_copy_dev_tomem(struct brcmf_pciedev_info *devinfo, u32 mem_offset,
> void *dstaddr, u32 len)
> @@ -1570,8 +1530,8 @@ static int brcmf_pcie_download_fw_nvram(struct brcmf_pciedev_info *devinfo,
> return err;
>
> brcmf_dbg(PCIE, "Download FW %s\n", devinfo->fw_name);
> - brcmf_pcie_copy_mem_todev(devinfo, devinfo->ci->rambase,
> - (void *)fw->data, fw->size);
> + memcpy_toio(devinfo->tcm + devinfo->ci->rambase,
> + (void *)fw->data, fw->size);
>
> resetintr = get_unaligned_le32(fw->data);
> release_firmware(fw);
> @@ -1585,7 +1545,7 @@ static int brcmf_pcie_download_fw_nvram(struct brcmf_pciedev_info *devinfo,
> brcmf_dbg(PCIE, "Download NVRAM %s\n", devinfo->nvram_name);
> address = devinfo->ci->rambase + devinfo->ci->ramsize -
> nvram_len;
> - brcmf_pcie_copy_mem_todev(devinfo, address, nvram, nvram_len);
> + memcpy_toio(devinfo->tcm + address, nvram, nvram_len);
> brcmf_fw_nvram_free(nvram);
> } else {
> brcmf_dbg(PCIE, "No matching NVRAM file found %s\n",
> --
> 2.33.0
>
--
With Best Regards,
Andy Shevchenko
20.01.2022 00:31, Andy Shevchenko пишет:
> On Wed, Jan 19, 2022 at 11:22 PM Dmitry Osipenko <[email protected]> wrote:
>>
>> 19.01.2022 20:49, Andy Shevchenko пишет:
>>> On Mon, Jan 17, 2022 at 4:30 PM Hector Martin <[email protected]> wrote:
>>>>
>>>> This avoids leaking memory if brcmf_chip_get_raminfo fails. Note that
>>>> the CLM blob is released in the device remove path.
>>>
>>> ...
>>>
>>>> if (ret) {
>>>
>>>> brcmf_err(bus, "Failed to get RAM info\n");
>>>> + release_firmware(fw);
>>>> + brcmf_fw_nvram_free(nvram);
>>>
>>> Can we first undo the things and only after print a message?
>>
>> Having message first usually is more preferred because at minimum you'll
>> get the message if "undoing the things" crashes, i.e. will be more
>> obvious what happened.
>
> If "undo the things" crashes, I would rather like to see that crash
> report, while serial UART at 9600 will continue flushing the message
> and then hang without any pointers to what the heck happened. Not
> here, but in general, messages are also good to be out of the locks.
The hang is actually a better example. It's the most annoying when there
is a silent hang and no error messages.
20.01.2022 11:29, Arend van Spriel пишет:
> On 1/19/2022 11:02 PM, Dmitry Osipenko wrote:
>> 17.01.2022 17:29, Hector Martin пишет:
>>> This unbreaks support for USB devices, which do not have a board_type
>>> to create an alt_path out of and thus were running into a NULL
>>> dereference.
>>>
>>> Fixes: 5ff013914c62 ("brcmfmac: firmware: Allow per-board firmware
>>> binaries")
>>> Signed-off-by: Hector Martin <[email protected]>
>>
>> Technically, all patches that are intended to be included into next
>> stable kernel update require the "Cc: [email protected]" tag.
>
> Being the nit picker that I am I would say it is recommended to safe
> yourself extra work, not required, for the reason you give below.
Will be nice if stable tag could officially become a recommendation,
implying the stable tag. It's a requirement today, at least Greg KH
always demands to add it :)
On 1/20/2022 2:24 PM, Dmitry Osipenko wrote:
> 20.01.2022 16:23, Dmitry Osipenko пишет:
>> 20.01.2022 11:29, Arend van Spriel пишет:
>>> On 1/19/2022 11:02 PM, Dmitry Osipenko wrote:
>>>> 17.01.2022 17:29, Hector Martin пишет:
>>>>> This unbreaks support for USB devices, which do not have a board_type
>>>>> to create an alt_path out of and thus were running into a NULL
>>>>> dereference.
>>>>>
>>>>> Fixes: 5ff013914c62 ("brcmfmac: firmware: Allow per-board firmware
>>>>> binaries")
>>>>> Signed-off-by: Hector Martin <[email protected]>
>>>>
>>>> Technically, all patches that are intended to be included into next
>>>> stable kernel update require the "Cc: [email protected]" tag.
>>>
>>> Being the nit picker that I am I would say it is recommended to safe
>>> yourself extra work, not required, for the reason you give below.
>>
>> Will be nice if stable tag could officially become a recommendation,
>> implying the stable tag. It's a requirement today, at least Greg KH
>> always demands to add it :)
>
> *implying the stable tag if "fixes" tag presents.
I was a little confused reading your previous email in this thread. This
makes a lot more sense :-p