Re: [PATCH v4] brcmfmac: add CLM download support

2017-08-28 Thread Arend van Spriel 



On 28-08-17 16:52, Steve deRosier wrote:

On Mon, Aug 28, 2017 at 2:25 AM, Wright Feng  wrote:

From: Chung-Hsien Hsu 

The firmware for brcmfmac devices includes information regarding
regulatory constraints. For certain devices this information is kept
separately in a binary form that needs to be downloaded to the device.
This patch adds support to download this so-called CLM blob file. It
uses the same naming scheme as the other firmware files with extension
of .clm_blob.



Not to bikeshed this, but why not just ".clm"?  ".clm_blob" seems
unnecessary and could feel "unprofessional" to users.

To me simple seems better.


Hi Steve,

Glad you ask ;-) It was one of my comments pre-reviewing this 
"internally"*. The problem with calling it .clm is that it means 
something else within Broadcom and Cypress for that matter. The 
.clm_blob is the output of tool compiling the regulatory data according 
an input specification which uses the .clm extension. It is similar to 
regdb.txt and regulatory.bin.


Regards,
Arend

* internally does not quite cover Cypress+Broadcom collaboration.

Re: [PATCH v4] brcmfmac: add CLM download support

2017-08-28 Thread Steve deRosier 
On Mon, Aug 28, 2017 at 2:25 AM, Wright Feng  wrote:
> From: Chung-Hsien Hsu 
>
> The firmware for brcmfmac devices includes information regarding
> regulatory constraints. For certain devices this information is kept
> separately in a binary form that needs to be downloaded to the device.
> This patch adds support to download this so-called CLM blob file. It
> uses the same naming scheme as the other firmware files with extension
> of .clm_blob.
>

Not to bikeshed this, but why not just ".clm"?  ".clm_blob" seems
unnecessary and could feel "unprofessional" to users.

To me simple seems better.

- Steve

Re: [PATCH v4] brcmfmac: add CLM download support

2017-08-28 Thread Chung-Hsien Hsu 
On Mon, Aug 28, 2017 at 12:27:38PM +0200, Arend van Spriel wrote:
> On 8/28/2017 11:25 AM, Wright Feng wrote:
> >From: Chung-Hsien Hsu 
> >
> >The firmware for brcmfmac devices includes information regarding
> >regulatory constraints. For certain devices this information is kept
> >separately in a binary form that needs to be downloaded to the device.
> >This patch adds support to download this so-called CLM blob file. It
> >uses the same naming scheme as the other firmware files with extension
> >of .clm_blob.
> >
> >The CLM blob file is optional. If the file does not exist, the download
> >process will be bypassed. It will not affect the driver loading.
> >
> >Signed-off-by: Chung-Hsien Hsu 
> >---
> >v2: Revise commit message to describe in more detail
> >v3: Add error handling in brcmf_c_get_clm_name function
> >v4: Correct the length of dload_buf in brcmf_c_download function
> >---
> >  .../net/wireless/broadcom/brcm80211/brcmfmac/bus.h |  10 ++
> >  .../wireless/broadcom/brcm80211/brcmfmac/common.c  | 161 
> > +
> >  .../wireless/broadcom/brcm80211/brcmfmac/core.c|   2 +
> >  .../wireless/broadcom/brcm80211/brcmfmac/core.h|   2 +
> >  .../broadcom/brcm80211/brcmfmac/fwil_types.h   |  31 
> >  .../wireless/broadcom/brcm80211/brcmfmac/pcie.c|  19 +++
> >  .../wireless/broadcom/brcm80211/brcmfmac/sdio.c|  19 +++
> >  .../net/wireless/broadcom/brcm80211/brcmfmac/usb.c |  18 +++
> >  8 files changed, 262 insertions(+)
> >
> 
> [...]
> 
> >diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/common.c 
> >b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/common.c
> >index 7a2b495..d09922b 100644
> >--- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/common.c
> >+++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/common.c
> 
> [...]
> 
> >@@ -104,12 +106,139 @@ void brcmf_c_set_joinpref_default(struct brcmf_if 
> >*ifp)
> > brcmf_err("Set join_pref error (%d)\n", err);
> >  }
> >
> >+static int brcmf_c_download(struct brcmf_if *ifp, u16 flag,
> >+struct brcmf_dload_data_le *dload_buf,
> >+u32 len)
> >+{
> >+s32 err;
> >+
> >+flag |= (DLOAD_HANDLER_VER << DLOAD_FLAG_VER_SHIFT);
> >+dload_buf->flag = cpu_to_le16(flag);
> >+dload_buf->dload_type = cpu_to_le16(DL_TYPE_CLM);
> >+dload_buf->len = cpu_to_le32(len);
> >+dload_buf->crc = cpu_to_le32(0);
> >+len = sizeof(*dload_buf) + len - 1;
> >+len = len + 8 - (len % 8);
> 
> In case len is 8 this will result in 16. So better use roundup()
> macro from include/linux/kernel.h [1]. However, this means
> brcmf_fil_iovar_data_set() will read after the allocated buffer
> space.
> 
> By the way, where does the alignment requirement come from?
> 

Thanks for pointing this out. I've tried the download process without
this alignment. There's no error/failure showed up. Looks like the
alignment is not necessary here so I will remove it.

> >+
> >+err = brcmf_fil_iovar_data_set(ifp, "clmload", (void *)dload_buf, len);
> 
> The cast is not necessary so please remove.

Will do it.

Regards,
Chung-Hsien

> 
> Regards,
> Arend
> 
> [1] 
> http://elixir.free-electrons.com/linux/latest/source/include/linux/kernel.h#L90

Re: [PATCH v4] brcmfmac: add CLM download support

2017-08-28 Thread Arend van Spriel 

On 8/28/2017 11:25 AM, Wright Feng wrote:

From: Chung-Hsien Hsu 

The firmware for brcmfmac devices includes information regarding
regulatory constraints. For certain devices this information is kept
separately in a binary form that needs to be downloaded to the device.
This patch adds support to download this so-called CLM blob file. It
uses the same naming scheme as the other firmware files with extension
of .clm_blob.

The CLM blob file is optional. If the file does not exist, the download
process will be bypassed. It will not affect the driver loading.

Signed-off-by: Chung-Hsien Hsu 
---
v2: Revise commit message to describe in more detail
v3: Add error handling in brcmf_c_get_clm_name function
v4: Correct the length of dload_buf in brcmf_c_download function
---
  .../net/wireless/broadcom/brcm80211/brcmfmac/bus.h |  10 ++
  .../wireless/broadcom/brcm80211/brcmfmac/common.c  | 161 +
  .../wireless/broadcom/brcm80211/brcmfmac/core.c|   2 +
  .../wireless/broadcom/brcm80211/brcmfmac/core.h|   2 +
  .../broadcom/brcm80211/brcmfmac/fwil_types.h   |  31 
  .../wireless/broadcom/brcm80211/brcmfmac/pcie.c|  19 +++
  .../wireless/broadcom/brcm80211/brcmfmac/sdio.c|  19 +++
  .../net/wireless/broadcom/brcm80211/brcmfmac/usb.c |  18 +++
  8 files changed, 262 insertions(+)



[...]


diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/common.c 
b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/common.c
index 7a2b495..d09922b 100644
--- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/common.c
+++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/common.c


[...]


@@ -104,12 +106,139 @@ void brcmf_c_set_joinpref_default(struct brcmf_if *ifp)
brcmf_err("Set join_pref error (%d)\n", err);
  }

+static int brcmf_c_download(struct brcmf_if *ifp, u16 flag,
+   struct brcmf_dload_data_le *dload_buf,
+   u32 len)
+{
+   s32 err;
+
+   flag |= (DLOAD_HANDLER_VER << DLOAD_FLAG_VER_SHIFT);
+   dload_buf->flag = cpu_to_le16(flag);
+   dload_buf->dload_type = cpu_to_le16(DL_TYPE_CLM);
+   dload_buf->len = cpu_to_le32(len);
+   dload_buf->crc = cpu_to_le32(0);
+   len = sizeof(*dload_buf) + len - 1;
+   len = len + 8 - (len % 8);


In case len is 8 this will result in 16. So better use roundup() macro 
from include/linux/kernel.h [1]. However, this means 
brcmf_fil_iovar_data_set() will read after the allocated buffer space.


By the way, where does the alignment requirement come from?


+
+   err = brcmf_fil_iovar_data_set(ifp, "clmload", (void *)dload_buf, len);


The cast is not necessary so please remove.

Regards,
Arend

[1] 
http://elixir.free-electrons.com/linux/latest/source/include/linux/kernel.h#L90

[PATCH v4] brcmfmac: add CLM download support

2017-08-28 Thread Wright Feng 
From: Chung-Hsien Hsu 

The firmware for brcmfmac devices includes information regarding
regulatory constraints. For certain devices this information is kept
separately in a binary form that needs to be downloaded to the device.
This patch adds support to download this so-called CLM blob file. It
uses the same naming scheme as the other firmware files with extension
of .clm_blob.

The CLM blob file is optional. If the file does not exist, the download
process will be bypassed. It will not affect the driver loading.

Signed-off-by: Chung-Hsien Hsu 
---
v2: Revise commit message to describe in more detail
v3: Add error handling in brcmf_c_get_clm_name function
v4: Correct the length of dload_buf in brcmf_c_download function
---
 .../net/wireless/broadcom/brcm80211/brcmfmac/bus.h |  10 ++
 .../wireless/broadcom/brcm80211/brcmfmac/common.c  | 161 +
 .../wireless/broadcom/brcm80211/brcmfmac/core.c|   2 +
 .../wireless/broadcom/brcm80211/brcmfmac/core.h|   2 +
 .../broadcom/brcm80211/brcmfmac/fwil_types.h   |  31 
 .../wireless/broadcom/brcm80211/brcmfmac/pcie.c|  19 +++
 .../wireless/broadcom/brcm80211/brcmfmac/sdio.c|  19 +++
 .../net/wireless/broadcom/brcm80211/brcmfmac/usb.c |  18 +++
 8 files changed, 262 insertions(+)

diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bus.h 
b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bus.h
index b55c329..df42e09 100644
--- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bus.h
+++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bus.h
@@ -71,6 +71,7 @@ struct brcmf_bus_dcmd {
  * @wowl_config: specify if dongle is configured for wowl when going to suspend
  * @get_ramsize: obtain size of device memory.
  * @get_memdump: obtain device memory dump in provided buffer.
+ * @get_fwname: obtain firmware name.
  *
  * This structure provides an abstract interface towards the
  * bus specific driver. For control messages to common driver
@@ -87,6 +88,8 @@ struct brcmf_bus_ops {
void (*wowl_config)(struct device *dev, bool enabled);
size_t (*get_ramsize)(struct device *dev);
int (*get_memdump)(struct device *dev, void *data, size_t len);
+   int (*get_fwname)(struct device *dev, uint chip, uint chiprev,
+ unsigned char *fw_name);
 };
 
 
@@ -214,6 +217,13 @@ int brcmf_bus_get_memdump(struct brcmf_bus *bus, void 
*data, size_t len)
return bus->ops->get_memdump(bus->dev, data, len);
 }
 
+static inline
+int brcmf_bus_get_fwname(struct brcmf_bus *bus, uint chip, uint chiprev,
+unsigned char *fw_name)
+{
+   return bus->ops->get_fwname(bus->dev, chip, chiprev, fw_name);
+}
+
 /*
  * interface functions from common layer
  */
diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/common.c 
b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/common.c
index 7a2b495..d09922b 100644
--- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/common.c
+++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/common.c
@@ -18,6 +18,7 @@
 #include 
 #include 
 #include 
+#include 
 #include 
 #include 
 #include "core.h"
@@ -28,6 +29,7 @@
 #include "tracepoint.h"
 #include "common.h"
 #include "of.h"
+#include "firmware.h"
 
 MODULE_AUTHOR("Broadcom Corporation");
 MODULE_DESCRIPTION("Broadcom 802.11 wireless LAN fullmac driver.");
@@ -104,12 +106,139 @@ void brcmf_c_set_joinpref_default(struct brcmf_if *ifp)
brcmf_err("Set join_pref error (%d)\n", err);
 }
 
+static int brcmf_c_download(struct brcmf_if *ifp, u16 flag,
+   struct brcmf_dload_data_le *dload_buf,
+   u32 len)
+{
+   s32 err;
+
+   flag |= (DLOAD_HANDLER_VER << DLOAD_FLAG_VER_SHIFT);
+   dload_buf->flag = cpu_to_le16(flag);
+   dload_buf->dload_type = cpu_to_le16(DL_TYPE_CLM);
+   dload_buf->len = cpu_to_le32(len);
+   dload_buf->crc = cpu_to_le32(0);
+   len = sizeof(*dload_buf) + len - 1;
+   len = len + 8 - (len % 8);
+
+   err = brcmf_fil_iovar_data_set(ifp, "clmload", (void *)dload_buf, len);
+
+   return err;
+}
+
+static int brcmf_c_get_clm_name(struct brcmf_if *ifp, u8 *clm_name)
+{
+   struct brcmf_bus *bus = ifp->drvr->bus_if;
+   struct brcmf_rev_info *ri = >drvr->revinfo;
+   u8 fw_name[BRCMF_FW_NAME_LEN];
+   u8 *ptr;
+   size_t len;
+   s32 err;
+
+   memset(fw_name, 0, BRCMF_FW_NAME_LEN);
+   err = brcmf_bus_get_fwname(bus, ri->chipnum, ri->chiprev, fw_name);
+   if (err) {
+   brcmf_err("get firmware name failed (%d)\n", err);
+   goto done;
+   }
+
+   /* generate CLM blob file name */
+   ptr = strrchr(fw_name, '.');
+   if (!ptr) {
+   err = -ENOENT;
+   goto done;
+   }
+
+   len = ptr - fw_name + 1;
+   if (len + strlen(".clm_blob") > BRCMF_FW_NAME_LEN) {
+   err = -E2BIG;
+   }