Re: [PATCH] brcmfmac: Fix glob_skb leak in brcmf_sdiod_recv_chain

2017-06-02 Thread Franky Lin 
On Fri, Jun 2, 2017 at 10:22 AM, Peter S. Housel  wrote:
> An earlier change to this function (3bdae810721b) fixed a leak in the
> case of an unsuccessful call to brcmf_sdiod_buffrw(). However, the
> glob_skb buffer, used for emulating a scattering read, is never used
> or referenced after its contents are copied into the destination
> buffers, and therefore always needs to be freed by the end of the
> function.
>
> Signed-off-by: Peter S. Housel 
> ---
>  drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcmsdh.c | 9 +
>  1 file changed, 5 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcmsdh.c 
> b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcmsdh.c
> index 9b970dc..4c5064f 100644
> --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcmsdh.c
> +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcmsdh.c
> @@ -727,15 +727,16 @@ int brcmf_sdiod_recv_chain(struct brcmf_sdio_dev 
> *sdiodev,
> return -ENOMEM;
> err = brcmf_sdiod_buffrw(sdiodev, SDIO_FUNC_2, false, addr,
>  glom_skb);
> -   if (err) {
> -   brcmu_pkt_buf_free_skb(glom_skb);
> -   goto done;
> -   }
> +   if (err)
> +   goto free_glom_skb;
>
> skb_queue_walk(pktq, skb) {
> memcpy(skb->data, glom_skb->data, skb->len);
> skb_pull(glom_skb, skb->len);
> }
> +
> +free_glom_skb:
> +   brcmu_pkt_buf_free_skb(glom_skb);

What about
if (!err) {
skb_queue_walk(pktq, skb) {
memcpy(skb->data, glom_skb->data, skb->len);
skb_pull(glom_skb, skb->len);
}
}
brcmu_pkt_buf_free_skb(glom_skb);

Then no goto is needed.

Thanks,
Franky

> } else
> err = brcmf_sdiod_sglist_rw(sdiodev, SDIO_FUNC_2, false, addr,
> pktq);
> --
> 2.7.4
>

Re: [PATCH] brcmfmac: Fix kernel oops on resume when request firmware fails.

2017-05-23 Thread Franky Lin 
Hi Enric,

On Tue, May 23, 2017 at 11:07 AM, Enric Balletbo i Serra
 wrote:
> When request firmware fails, brcmf_ops_sdio_remove is being called and
> brcmf_bus freed. In such circumstancies if you do a suspend/resume cycle
> the kernel hangs on resume due a NULL pointer dereference in resume
> function.
>
> Steps to reproduce the problem:
>  - modprobe brcmfmac without the firmware
>  brcmfmac mmc1:0001:1: Direct firmware load for brcm/brcmfmac4354-sdio.bin
>  failed with error -2
>  - do a suspend/resume cycle (echo mem > /sys/power/state)
>
> Protect against the NULL pointer derefence by checking if dev_get_drvdata
> returned a valid pointer.
>
> Signed-off-by: Enric Balletbo i Serra 
> ---
> I'm not sure about if this is the correct way to fix this but at least it
> prevents the kernel to hang. From one side I'm not sure why suspend/resume
> functions are called in such case and why the device is not removed from
> the bus, from the other side I saw, that others drivers only unregisters
> from sdio when the driver is removed so I supose this is the normal behavior.
>

Thank you for reporting this. I also think these questions you listed
should be answered before putting the null check in resume routine. I
will dig deeper and share my finding on the thread.

Regards,
Franky

> Cheers,
>  Enric
>
>  drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcmsdh.c | 7 ---
>  1 file changed, 4 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcmsdh.c 
> b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcmsdh.c
> index 9b970dc..aa0e7c2 100644
> --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcmsdh.c
> +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcmsdh.c
> @@ -1274,14 +1274,16 @@ static int brcmf_ops_sdio_suspend(struct device *dev)
>  static int brcmf_ops_sdio_resume(struct device *dev)
>  {
> struct brcmf_bus *bus_if = dev_get_drvdata(dev);
> -   struct brcmf_sdio_dev *sdiodev = bus_if->bus_priv.sdio;
> struct sdio_func *func = container_of(dev, struct sdio_func, dev);
>
> brcmf_dbg(SDIO, "Enter: F%d\n", func->num);
> if (func->num != SDIO_FUNC_2)
> return 0;
>
> -   brcmf_sdiod_freezer_off(sdiodev);
> +   if (!bus_if)
> +   return 0;
> +
> +   brcmf_sdiod_freezer_off(bus_if->bus_priv.sdio);
> return 0;
>  }
>
> @@ -1319,4 +1321,3 @@ void brcmf_sdio_exit(void)
>
> sdio_unregister_driver(_sdmmc_driver);
>  }
> -
> --
> 2.9.3
>

Re: brcmfmac firmware issue on NanoPi K2

2017-05-22 Thread Franky Lin 
Hi Andreas,

On Sun, May 21, 2017 at 9:20 AM, Andreas Färber  wrote:
> Hello,
>
> The NanoPi K2 has an Ampak AP6212 SDIO module. brcmfmac driver loads
> brcmfmac43430-sdio.bin.
>
> When using the firmware file from linux-firmware.git that openSUSE ships
> I get the following errors on 4.11.0 and next-20170519:
>
> [ 2103.618716] brcmfmac: brcmf_sdio_htclk: HT Avail timeout (100):
> clkctl 0x50
> [ 2104.668746] brcmfmac: brcmf_sdio_htclk: HT Avail timeout (100):
> clkctl 0x50
> [ 2105.678677] brcmfmac: brcmf_sdio_htclk: HT Avail timeout (100):
> clkctl 0x50
>
> If I overwrite /lib/firmware/brcm/bcm43430-sdio.bin with
> fw_bcm43438a0.bin from FriendlyARM's Android repository it suddenly works:
>
> [  +0.157738] brcmfmac: brcmf_c_preinit_dcmds: Firmware version = wl0:
> Jun  6 2014 14:50:39 version 7.10.226.49 (r) FWID 01-8962686a
> [  +0.160108] brcmfmac: brcmf_cfg80211_reg_notifier: not a ISO3166 code
> (0x30 0x30)
>
> I recall using the linux-firmware.git brcmfmac43430-sdio.bin file
> successfully on the Raspberry Pi 3 with a downstream (Leap 42.2) kernel.
>
> I've tested both nvram_ap6212.txt and nvram_ap6212a.txt, the latter has
> the following diff to nvram.txt:
>
> --- nvram_ap6212.txt2017-05-21 04:24:40.372113426 +0200
> +++ nvram_ap6212a.txt   2017-05-21 04:24:49.852116599 +0200
> @@ -1,4 +1,4 @@
> -#AP6212_NVRAM_V1.0_20140603
> +#AP6212_NVRAM_V1.0.1_20160606
>  # 2.4 GHz, 20 MHz BW mode
>
>  # The following parameter values are just placeholders, need to be updated.
> @@ -51,4 +51,4 @@
>  muxenab=0x10
>  # CLDO PWM voltage settings - 0x4 - 1.1 volt
>  #cldo_pwm=0x4
> -
> +glitch_based_crsmin=1
>
> https://github.com/friendlyarm/android_hardware_amlogic_wifi/tree/l-amlogic-gx-sync/bcm_ampak/config/6212
>
> * Does the linux-firmware.git brcmfmac43430-sdio.bin need a fix for AP6212?
> https://git.kernel.org/pub/scm/linux/kernel/git/firmware/linux-firmware.git/tree/brcm
>
> * Does the brcmfmac driver need to distinguish revisions in sdio.c as
> done for 43241, plus a separate firmware file?
> BRCMF_FW_NVRAM_ENTRY(BRCM_CC_43430_CHIP_ID, 0x, 43430),

It seems we are dealing with different revisions of 43430. The
firmware file you pointed to is for 43430a0 and raspberry pi 3 is
using 43430a1. So yes brcmfmac needs to load different firmware and
nvram for them just like 43241.

Could you please help try the attached patch? I don't have the
hardware to test it. Please rename the 43438 firmware and nvram to
brcmfmac43430a0-sdio.bin/txt.

Thanks,
Franky

---

diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c
b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c
index a999f95..1b12ccb 100644
--- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c
+++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c
@@ -608,6 +608,7 @@ BRCMF_FW_NVRAM_DEF(43340,
"brcmfmac43340-sdio.bin", "brcmfmac43340-sdio.txt");
 BRCMF_FW_NVRAM_DEF(4335, "brcmfmac4335-sdio.bin", "brcmfmac4335-sdio.txt");
 BRCMF_FW_NVRAM_DEF(43362, "brcmfmac43362-sdio.bin", "brcmfmac43362-sdio.txt");
 BRCMF_FW_NVRAM_DEF(4339, "brcmfmac4339-sdio.bin", "brcmfmac4339-sdio.txt");
+BRCMF_FW_NVRAM_DEF(43430A0, "brcmfmac43430a0-sdio.bin",
"brcmfmac43430a0-sdio.txt");
 BRCMF_FW_NVRAM_DEF(43430, "brcmfmac43430-sdio.bin", "brcmfmac43430-sdio.txt");
 BRCMF_FW_NVRAM_DEF(43455, "brcmfmac43455-sdio.bin", "brcmfmac43455-sdio.txt");
 BRCMF_FW_NVRAM_DEF(4354, "brcmfmac4354-sdio.bin", "brcmfmac4354-sdio.txt");
@@ -626,7 +627,8 @@ static struct brcmf_firmware_mapping
brcmf_sdio_fwnames[] = {
  BRCMF_FW_NVRAM_ENTRY(BRCM_CC_4335_CHIP_ID, 0x, 4335),
  BRCMF_FW_NVRAM_ENTRY(BRCM_CC_43362_CHIP_ID, 0xFFFE, 43362),
  BRCMF_FW_NVRAM_ENTRY(BRCM_CC_4339_CHIP_ID, 0x, 4339),
- BRCMF_FW_NVRAM_ENTRY(BRCM_CC_43430_CHIP_ID, 0x, 43430),
+ BRCMF_FW_NVRAM_ENTRY(BRCM_CC_43430_CHIP_ID, 0x0001, 43430A0),
+ BRCMF_FW_NVRAM_ENTRY(BRCM_CC_43430_CHIP_ID, 0xFFFE, 43430),
  BRCMF_FW_NVRAM_ENTRY(BRCM_CC_4345_CHIP_ID, 0xFFC0, 43455),
  BRCMF_FW_NVRAM_ENTRY(BRCM_CC_4354_CHIP_ID, 0x, 4354),
  BRCMF_FW_NVRAM_ENTRY(BRCM_CC_4356_CHIP_ID, 0x, 4356)