Re: [PATCH v3 4/4] mwifiex: pcie: extract wifi part from combo firmware during function level reset

[Brian Norris <briannorris@chromium.org>]

Hi,

On Mon, Apr 10, 2017 at 09:09:34AM +0000, Xinming Hu wrote:
> From: Xinming Hu <huxm@marvell.com>
> 
> A seperate 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, we can discard the redudant image in linux-firmware repo.
> 
> Signed-off-by: Xinming Hu <huxm@marvell.com>
> Signed-off-by: Ganapathi Bhat <gbhat@marvell.com>
> Signed-off-by: Cathy Luo <cluo@marvell.com>
> ---
> v2: extract wifi part from combo firmware(Dimtry and Brain)
>     add more description(Kalle)
> v3: same as v2
> ---
>  drivers/net/wireless/marvell/mwifiex/fw.h   | 18 +++++++
>  drivers/net/wireless/marvell/mwifiex/pcie.c | 83 ++++++++++++++++++++++++++---
>  drivers/net/wireless/marvell/mwifiex/pcie.h |  2 +
>  3 files changed, 96 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/net/wireless/marvell/mwifiex/fw.h b/drivers/net/wireless/marvell/mwifiex/fw.h
> index 0b68374..6cf9ab9 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 a07cb0a..ebf00d9 100644
> --- a/drivers/net/wireless/marvell/mwifiex/pcie.c
> +++ b/drivers/net/wireless/marvell/mwifiex/pcie.c
> @@ -1956,6 +1956,63 @@ static int mwifiex_pcie_event_complete(struct mwifiex_adapter *adapter,
>  	return ret;
>  }
>  
> +/* Extract wifi part from wifi-bt combo firmware image.
> + */
> +
> +static int mwifiex_extract_wifi_fw(struct mwifiex_adapter *adapter,
> +				   u8 *firmware, u32 firmware_len) {

Can you make 'firmware' const? Also, to help below, it might work better
as 'const void *'.

> +	struct mwifiex_fw_data fwdata;
> +	u32 offset = 0, data_len, dnld_cmd;
> +	int ret = 0;
> +	bool cmd7_before = false;
> +
> +	while (1) {
> +		if (offset + sizeof(fwdata.header) >= firmware_len) {
> +			mwifiex_dbg(adapter, ERROR,
> +				    "extract wifi-only firmware failure!");
> +			ret = -1;
> +			goto done;
> +		}
> +
> +		memcpy(&fwdata.header, firmware + offset,
> +		       sizeof(fwdata.header));

Do you actually need to copy this? Can't you just keep a pointer to the
location? e.g.

	const struct mwifiex_fw_data *fwdata;
...
	fwdata = firmware + offset;

> +		dnld_cmd = le32_to_cpu(fwdata.header.dnld_cmd);
> +		data_len = le32_to_cpu(fwdata.header.data_length);
> +
> +		switch (dnld_cmd) {
> +		case MWIFIEX_FW_DNLD_CMD_1:
> +			if (!cmd7_before) {
> +				mwifiex_dbg(adapter, ERROR,
> +					    "no cmd7 before cmd1!");
> +				ret = -1;
> +				goto done;
> +			}
> +			offset += data_len + sizeof(fwdata.header);

Technically, we need an overflow check to make sure that 'data_len'
isn't some bogus value that overflows 'offset'.

> +			break;
> +		case MWIFIEX_FW_DNLD_CMD_5:
> +			offset += data_len + sizeof(fwdata.header);

Same here.

> +			break;
> +		case MWIFIEX_FW_DNLD_CMD_6:

Can we have a comment, either here or above this function, to describe
what this sequence is? Or particularly, what is this terminating
condition? "CMD_6" doesn't really tell me that this is the start of the
Wifi blob.

> +			offset += data_len + sizeof(fwdata.header);

You don't check for overflow here. Check this before returning this
(either here, or in the 'done' label).

> +			ret = offset;
> +			goto done;
> +		case MWIFIEX_FW_DNLD_CMD_7:
> +			if (!cmd7_before)

^^ This 'if' isn't really needed.

> +				cmd7_before = true;
> +			offset += sizeof(fwdata.header);
> +			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 +2028,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 +2055,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;
> @@ -3060,12 +3135,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);

Now that there's no users, delete PCIE8997_DEFAULT_WIFIFW_NAME from
pcie.h.

Brian

> -
>  	/* 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 7e2450c..54aecda 100644
> --- a/drivers/net/wireless/marvell/mwifiex/pcie.h
> +++ b/drivers/net/wireless/marvell/mwifiex/pcie.h
> @@ -120,6 +120,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;
> -- 
> 1.8.1.4
>