Hi Brain, > -----Original Message----- > From: Brian Norris [mailto:briannorris@xxxxxxxxxxxx] > Sent: 2017年4月11日 9:37 > To: Xinming Hu > Cc: Linux Wireless; Kalle Valo; Dmitry Torokhov; rajatja@xxxxxxxxxx; > Amitkumar Karwar; Cathy Luo; Xinming Hu; Ganapathi Bhat > Subject: [EXT] Re: [PATCH v3 4/4] mwifiex: pcie: extract wifi part from combo > firmware during function level reset > > External Email > > ---------------------------------------------------------------------- > Hi, > > On Mon, Apr 10, 2017 at 09:09:34AM +0000, Xinming Hu wrote: > > From: Xinming Hu <huxm@xxxxxxxxxxx> > > > > 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@xxxxxxxxxxx> > > Signed-off-by: Ganapathi Bhat <gbhat@xxxxxxxxxxx> > > Signed-off-by: Cathy Luo <cluo@xxxxxxxxxxx> > > --- > > 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 *'. > OK, Thanks for the review. > > + 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; > Ok. > > + 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'. > There is the sanity check for the offset after bypass CMD1/5/7 in the start of while-loop, enhanced in v4 as, if (offset >= firmware_len) > > + 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); > The sequence have been added to function comments in v4. > You don't check for overflow here. Check this before returning this (either here, > or in the 'done' label). > Yes, add sanity check for the offset in end of CMD6. > > + ret = offset; > > + goto done; > > + case MWIFIEX_FW_DNLD_CMD_7: > > + if (!cmd7_before) > > ^^ This 'if' isn't really needed. Done. > > > + 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. Removed in V4. > > 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 > >
