Hi Brain,
> -----Original Message-----
> From: Brian Norris [mailto:[email protected]]
> Sent: 2017年4月11日 9:37
> To: Xinming Hu
> Cc: Linux Wireless; Kalle Valo; Dmitry Torokhov; [email protected];
> 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 <[email protected]>
> >
> > 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 <[email protected]>
> > Signed-off-by: Ganapathi Bhat <[email protected]>
> > Signed-off-by: Cathy Luo <[email protected]>
> > ---
> > 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
> >
