Return-path: Received: from mail-pf0-f181.google.com ([209.85.192.181]:33698 "EHLO mail-pf0-f181.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751792AbdDKBh3 (ORCPT ); Mon, 10 Apr 2017 21:37:29 -0400 Received: by mail-pf0-f181.google.com with SMTP id s16so41207517pfs.0 for ; Mon, 10 Apr 2017 18:37:29 -0700 (PDT) Date: Mon, 10 Apr 2017 18:37:25 -0700 From: Brian Norris To: Xinming Hu Cc: Linux Wireless , Kalle Valo , Dmitry Torokhov , rajatja@google.com, Amitkumar Karwar , Cathy Luo , Xinming Hu , Ganapathi Bhat Subject: Re: [PATCH v3 4/4] mwifiex: pcie: extract wifi part from combo firmware during function level reset Message-ID: <20170411013724.GA135531@google.com> (sfid-20170411_033914_059118_92155253) References: <1491815374-6555-1-git-send-email-huxinming820@gmail.com> <1491815374-6555-4-git-send-email-huxinming820@gmail.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii In-Reply-To: <1491815374-6555-4-git-send-email-huxinming820@gmail.com> Sender: linux-wireless-owner@vger.kernel.org List-ID: Hi, On Mon, Apr 10, 2017 at 09:09:34AM +0000, Xinming Hu wrote: > From: Xinming Hu > > 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 > Signed-off-by: Ganapathi Bhat > Signed-off-by: Cathy Luo > --- > 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 >