Hi,
> -----Original Message-----
> From: Brian Norris [mailto:briannor...@chromium.org]
> Sent: 2017年4月14日 4:11
> To: Xinming Hu
> Cc: Linux Wireless; Kalle Valo; Dmitry Torokhov; raja...@google.com; Cathy
> Luo;
> Xinming Hu; Ganapathi Bhat
> Subject: [EXT] [PATCH v5 4/4] mwifiex: pcie: extract wifi part from combo
> firmware during function level reset
>
> External Email
>
> ----------------------------------------------------------------------
> From: Xinming Hu <h...@marvell.com>
>
> A separate wifi-only firmware was download during pcie function level reset.
> It
> is in fact the tail part of wifi/bt combo firmware. Per Brian's and Dmitry's
> suggestion, this patch extract the wifi part from combo firmware.
>
> After that, the mrvl/pcie8997_wlan_v4.bin image in linux-firmware repo is
> redundant (though I guess we keep it around to support older kernels).
>
> Signed-off-by: Xinming Hu <h...@marvell.com>
> Signed-off-by: Ganapathi Bhat <gb...@marvell.com>
> Signed-off-by: Cathy Luo <c...@marvell.com>
> Signed-off-by: Brian Norris <briannor...@chromium.org>
> ---
> On Thu, Apr 13, 2017 at 11:46:30AM -0700, Brian Norris wrote:
> > I might just rewrite this and send it myself, if I get the time.
>
> Done.
>
> v2: extract wifi part from combo firmware(Dmitry and Brian)
> add more description(Kalle)
> v3: same as v2
> v4: add sequence comments, code enhance(Brian)
> v5: (Brian) fix overflow errors
> (Brian) add missing newline chars
> (Brian) consolidate header-skipping logic
>
> Note: I only resubmitted the 4th patch, as the others look fine
> ---
> drivers/net/wireless/marvell/mwifiex/fw.h | 18 +++++
> drivers/net/wireless/marvell/mwifiex/pcie.c | 114
> ++++++++++++++++++++++++++--
> drivers/net/wireless/marvell/mwifiex/pcie.h | 3 +-
> 3 files changed, 127 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/net/wireless/marvell/mwifiex/fw.h
> b/drivers/net/wireless/marvell/mwifiex/fw.h
> index 0b683742e30c..6cf9ab9133ea 100644
> --- a/drivers/net/wireless/marvell/mwifiex/fw.h
> +++ b/drivers/net/wireless/marvell/mwifiex/fw.h
> @@ -43,6 +43,24 @@ struct tx_packet_hdr {
> struct rfc_1042_hdr rfc1042_hdr;
> } __packed;
>
> +struct mwifiex_fw_header {
> + __le32 dnld_cmd;
> + __le32 base_addr;
> + __le32 data_length;
> + __le32 crc;
> +} __packed;
> +
> +struct mwifiex_fw_data {
> + struct mwifiex_fw_header header;
> + __le32 seq_num;
> + u8 data[1];
> +} __packed;
> +
> +#define MWIFIEX_FW_DNLD_CMD_1 0x1
> +#define MWIFIEX_FW_DNLD_CMD_5 0x5
> +#define MWIFIEX_FW_DNLD_CMD_6 0x6
> +#define MWIFIEX_FW_DNLD_CMD_7 0x7
> +
> #define B_SUPPORTED_RATES 5
> #define G_SUPPORTED_RATES 9
> #define BG_SUPPORTED_RATES 13
> diff --git a/drivers/net/wireless/marvell/mwifiex/pcie.c
> b/drivers/net/wireless/marvell/mwifiex/pcie.c
> index 061223149bed..63102efb388e 100644
> --- a/drivers/net/wireless/marvell/mwifiex/pcie.c
> +++ b/drivers/net/wireless/marvell/mwifiex/pcie.c
> @@ -1956,6 +1956,94 @@ static int mwifiex_pcie_event_complete(struct
> mwifiex_adapter *adapter,
> return ret;
> }
>
> +/* Combo firmware image is a combination of
> + * (1) combo crc heaer, start with CMD5
> + * (2) bluetooth image, start with CMD7, end with CMD6, data wrapped in
> CMD1.
> + * (3) wifi image.
> + *
> + * This function bypass the header and bluetooth part, return
> + * the offset of tail wifi-only part.
> + */
> +
> +static int mwifiex_extract_wifi_fw(struct mwifiex_adapter *adapter,
> + const void *firmware, u32 firmware_len) {
> + const struct mwifiex_fw_data *fwdata;
> + u32 offset = 0, data_len, dnld_cmd;
> + int ret = 0;
> + bool cmd7_before = false;
> +
> + while (1) {
> + /* Check for integer and buffer overflow */
> + if (offset + sizeof(fwdata->header) < sizeof(fwdata->header) ||
> + offset + sizeof(fwdata->header) >= firmware_len) {
> + mwifiex_dbg(adapter, ERROR,
> + "extract wifi-only fw failure!\n");
> + ret = -1;
> + goto done;
> + }
> +
> + fwdata = firmware + offset;
> + dnld_cmd = le32_to_cpu(fwdata->header.dnld_cmd);
> + data_len = le32_to_cpu(fwdata->header.data_length);
> +
> + /* Skip past header */
> + offset += sizeof(fwdata->header);
> +
> + switch (dnld_cmd) {
> + case MWIFIEX_FW_DNLD_CMD_1:
> + if (!cmd7_before) {
> + mwifiex_dbg(adapter, ERROR,
> + "no cmd7 before cmd1!\n");
> + ret = -1;
> + goto done;
> + }
> + if (offset + data_len < data_len) {
> + mwifiex_dbg(adapter, ERROR, "bad FW parse\n");
> + ret = -1;
> + goto done;
> + }
> + offset += data_len;
Looks fine to me.
Even though data_len should not exceed MWIFIEX_UPLD_SIZE according to firmware
download protocol, and we can add sanity check like，
if (data_len > MWIFIEX_UPLD_SIZE - sizeof(fwdata->header))
*error*
Considering the future protocol might change MWIFIEX_UPLD_SIZE and the ability
to compatible with old driver at that time.
overflow check here does provide a general way, and let device take care of
invalid data_Len case would be reasonable.
Thanks,
Simon
> + break;
> + case MWIFIEX_FW_DNLD_CMD_5:
> + /* Check for integer overflow */
> + if (offset + data_len < data_len) {
> + mwifiex_dbg(adapter, ERROR, "bad FW parse\n");
> + ret = -1;
> + goto done;
> + }
> + offset += data_len;
> + break;
> + case MWIFIEX_FW_DNLD_CMD_6:
> + /* Check for integer overflow */
> + if (offset + data_len < data_len) {
> + mwifiex_dbg(adapter, ERROR, "bad FW parse\n");
> + ret = -1;
> + goto done;
> + }
> + offset += data_len;
> + if (offset >= firmware_len) {
> + mwifiex_dbg(adapter, ERROR,
> + "extract wifi-only fw failure!\n");
> + ret = -1;
> + } else {
> + ret = offset;
> + }
> + goto done;
> + case MWIFIEX_FW_DNLD_CMD_7:
> + cmd7_before = true;
> + break;
> + default:
> + mwifiex_dbg(adapter, ERROR, "unknown dnld_cmd %d\n",
> + dnld_cmd);
> + ret = -1;
> + goto done;
> + }
> + }
> +
> +done:
> + return ret;
> +}
> +
> /*
> * This function downloads the firmware to the card.
> *
> @@ -1971,7 +2059,7 @@ static int mwifiex_prog_fw_w_helper(struct
> mwifiex_adapter *adapter,
> u32 firmware_len = fw->fw_len;
> u32 offset = 0;
> struct sk_buff *skb;
> - u32 txlen, tx_blocks = 0, tries, len;
> + u32 txlen, tx_blocks = 0, tries, len, val;
> u32 block_retry_cnt = 0;
> struct pcie_service_card *card = adapter->card;
> const struct mwifiex_pcie_card_reg *reg = card->pcie.reg; @@ -1998,6
> +2086,24 @@ static int mwifiex_prog_fw_w_helper(struct mwifiex_adapter
> *adapter,
> goto done;
> }
>
> + ret = mwifiex_read_reg(adapter, PCIE_SCRATCH_13_REG, &val);
> + if (ret) {
> + mwifiex_dbg(adapter, FATAL, "Failed to read scratch register
> 13\n");
> + goto done;
> + }
> +
> + /* PCIE FLR case: extract wifi part from combo firmware*/
> + if (val == MWIFIEX_PCIE_FLR_HAPPENS) {
> + ret = mwifiex_extract_wifi_fw(adapter, firmware, firmware_len);
> + if (ret < 0) {
> + mwifiex_dbg(adapter, ERROR, "Failed to extract wifi
> fw\n");
> + goto done;
> + }
> + offset = ret;
> + mwifiex_dbg(adapter, MSG,
> + "info: dnld wifi firmware from %d bytes\n", offset);
> + }
> +
> /* Perform firmware data transfer */
> do {
> u32 ireg_intr = 0;
> @@ -3070,12 +3176,6 @@ static void mwifiex_pcie_up_dev(struct
> mwifiex_adapter *adapter)
> struct pci_dev *pdev = card->dev;
> const struct mwifiex_pcie_card_reg *reg = card->pcie.reg;
>
> - /* Bluetooth is not on pcie interface. Download Wifi only firmware
> - * during pcie FLR, so that bluetooth part of firmware which is
> - * already running doesn't get affected.
> - */
> - strcpy(adapter->fw_name, PCIE8997_DEFAULT_WIFIFW_NAME);
> -
> /* tx_buf_size might be changed to 3584 by firmware during
> * data transfer, we should reset it to default size.
> */
> diff --git a/drivers/net/wireless/marvell/mwifiex/pcie.h
> b/drivers/net/wireless/marvell/mwifiex/pcie.h
> index 7e2450ce79d3..f7ce9b6db6b4 100644
> --- a/drivers/net/wireless/marvell/mwifiex/pcie.h
> +++ b/drivers/net/wireless/marvell/mwifiex/pcie.h
> @@ -35,7 +35,6 @@
> #define PCIE8897_B0_FW_NAME "mrvl/pcie8897_uapsta.bin"
> #define PCIEUART8997_FW_NAME_V4 "mrvl/pcieuart8997_combo_v4.bin"
> #define PCIEUSB8997_FW_NAME_V4 "mrvl/pcieusb8997_combo_v4.bin"
> -#define PCIE8997_DEFAULT_WIFIFW_NAME "mrvl/pcie8997_wlan_v4.bin"
>
> #define PCIE_VENDOR_ID_MARVELL (0x11ab)
> #define PCIE_VENDOR_ID_V2_MARVELL (0x1b4b)
> @@ -120,6 +119,8 @@
> #define MWIFIEX_SLEEP_COOKIE_SIZE 4
> #define MWIFIEX_MAX_DELAY_COUNT 100
>
> +#define MWIFIEX_PCIE_FLR_HAPPENS 0xFEDCBABA
> +
> struct mwifiex_pcie_card_reg {
> u16 cmd_addr_lo;
> u16 cmd_addr_hi;
> --
> 2.12.2.762
