RE: [EXT] Re: [PATCH 3/3] mwifiex: pcie: avoid hardcode wifi-only firmware name

[Xinming Hu]

Hi Brain,

> -Original Message-
> From: Brian Norris [mailto:briannor...@chromium.org]
> Sent: 2017年4月4日 2:49
> To: Dmitry Torokhov
> Cc: Xinming Hu; Linux Wireless; Kalle Valo; Rajat Jain; Amitkumar Karwar; 
> Cathy
> Luo; Xinming Hu
> Subject: [EXT] Re: [PATCH 3/3] mwifiex: pcie: avoid hardcode wifi-only
> firmware name
> 
> External Email
> 
> --
> On Fri, Mar 31, 2017 at 03:50:27PM -0700, Dmitry Torokhov wrote:
> > That said, I consider the whole wifi-only firmware business is quite
> > fragile. Can we have unified firmware and have driver figure out what
> > part shoudl be [re]loaded?
> 
> +1. I think we should really give a stab at this first, and *then* see
> how we want to patch up the flagging of support on a per-chipset basis.
> As-is, you're wasting filesystem space on a duplicate firmware blob, that we
> have to make sure gets updated in sync with the combined firmware every
> time there's an update.
> 
> Proof of the duplicate blob -- the latter portion of the combined FW is 
> identical
> to the WLAN-only:
> 
>   $ cmp mrvl/pcie{usb8997_combo,8997_wlan}_v4.bin $((0x2919c)); echo $?
>   0

Thanks for the review, we have a new solution for extracting wifi-only part 
from combo firmware, will send it in v2.

Thanks
Simon
> 
> Brian

RE: Re: [PATCH 3/3] mwifiex: pcie: avoid hardcode wifi-only firmware name

[Xinming Hu]

Hi Dmitry,

> -Original Message-
> From: Dmitry Torokhov [mailto:d...@google.com]
> Sent: 2017年4月1日 6:50
> To: Xinming Hu
> Cc: Linux Wireless; Kalle Valo; Brian Norris; Rajat Jain; Amitkumar Karwar;
> Cathy Luo; Xinming Hu
> Subject: [EXT] Re: [PATCH 3/3] mwifiex: pcie: avoid hardcode wifi-only
> firmware name
> 
> External Email
> 
> --
> On Thu, Mar 30, 2017 at 2:19 AM, Xinming Hu <huxinming...@gmail.com>
> wrote:
> > From: Xinming Hu <h...@marvell.com>
> >
> > Wifi-only firmware name should be chipset specific.
> >
> > Signed-off-by: Xinming Hu <h...@marvell.com>
> > Signed-off-by: Amitkumar Karwar <akar...@marvell.com>
> > ---
> >  drivers/net/wireless/marvell/mwifiex/pcie.c | 8 +++-
> > drivers/net/wireless/marvell/mwifiex/pcie.h | 6 ++
> >  2 files changed, 13 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/net/wireless/marvell/mwifiex/pcie.c
> > b/drivers/net/wireless/marvell/mwifiex/pcie.c
> > index 59cb01a..282aa9a 100644
> > --- a/drivers/net/wireless/marvell/mwifiex/pcie.c
> > +++ b/drivers/net/wireless/marvell/mwifiex/pcie.c
> > @@ -351,6 +351,12 @@ static void mwifiex_pcie_reset_notify(struct pci_dev
> *pdev, bool prepare)
> > struct pcie_service_card *card = pci_get_drvdata(pdev);
> > struct mwifiex_adapter *adapter = card->adapter;
> >
> > +   if (!card->pcie.flr_support) {
> > +   pr_err("%s: FLR disabled in current chipset\n", __func__);
> > +   return;
> > +   }
> > +
> > +   adapter = card->adapter;
> > if (!adapter) {
> > dev_err(>dev, "%s: adapter structure is not
> valid\n",
> > __func__);
> > @@ -3047,7 +3053,7 @@ static void mwifiex_pcie_up_dev(struct
> mwifiex_adapter *adapter)
> >  * 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);
> > +   strcpy(adapter->fw_name, card->pcie.wifi_fw_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 00e8ee5..d6c8526 100644
> > --- a/drivers/net/wireless/marvell/mwifiex/pcie.h
> > +++ b/drivers/net/wireless/marvell/mwifiex/pcie.h
> > @@ -285,6 +285,8 @@ struct mwifiex_pcie_device {
> > struct memory_type_mapping *mem_type_mapping_tbl;
> > u8 num_mem_types;
> > bool can_ext_scan;
> > +   u8 flr_support;
> 
> This sounds like a boolean, but given that you can key off wifi_fw_name being
> NULL I am not sure it is needed.
> 
> > +   char *wifi_fw_name;
> 
> const char *.
> 
> That said, I consider the whole wifi-only firmware business is quite fragile. 
> Can
> we have unified firmware and have driver figure out what part shoudl be
> [re]loaded?
> 

Thanks for the review, we have tried to extract wifi-only part from combo 
firmware.
The new solution will be send in V2, in this way, we do not need separate 
wifi_fw_name any more.

Thanks
Simon

> >  };
> >
> >  static const struct mwifiex_pcie_device mwifiex_pcie8766 = { @@
> > -293,6 +295,7 @@ struct mwifiex_pcie_device {
> > .tx_buf_size = MWIFIEX_TX_DATA_BUF_SIZE_2K,
> > .can_dump_fw = false,
> > .can_ext_scan = true,
> > +   .flr_support = 0,
> >  };
> >
> >  static const struct mwifiex_pcie_device mwifiex_pcie8897 = { @@
> > -303,6 +306,7 @@ struct mwifiex_pcie_device {
> > .mem_type_mapping_tbl = mem_type_mapping_tbl_w8897,
> > .num_mem_types = ARRAY_SIZE(mem_type_mapping_tbl_w8897),
> > .can_ext_scan = true,
> > +   .flr_support = 0,
> >  };
> >
> >  static const struct mwifiex_pcie_device mwifiex_pcie8997 = { @@
> > -313,6 +317,8 @@ struct mwifiex_pcie_device {
> > .mem_type_mapping_tbl = mem_type_mapping_tbl_w8997,
> > .num_mem_types = ARRAY_SIZE(mem_type_mapping_tbl_w8997),
> > .can_ext_scan = true,
> > +   .flr_support = 1,
> > +   .wifi_fw_name = "mrvl/pcie8997_wlan_v4.bin",
> >  };
> >
> >  struct mwifiex_evt_buf_desc {
> > --
> > 1.8.1.4
> >
> 
> Thanks.
> 
> --
> Dmitry

Re: [PATCH 3/3] mwifiex: pcie: avoid hardcode wifi-only firmware name

[Brian Norris]

On Fri, Mar 31, 2017 at 03:50:27PM -0700, Dmitry Torokhov wrote:
> That said, I consider the whole wifi-only firmware business is quite
> fragile. Can we have unified firmware and have driver figure out what
> part shoudl be [re]loaded?

+1. I think we should really give a stab at this first, and *then* see
how we want to patch up the flagging of support on a per-chipset basis.
As-is, you're wasting filesystem space on a duplicate firmware blob,
that we have to make sure gets updated in sync with the combined
firmware every time there's an update.

Proof of the duplicate blob -- the latter portion of the combined FW is
identical to the WLAN-only:

  $ cmp mrvl/pcie{usb8997_combo,8997_wlan}_v4.bin $((0x2919c)); echo $?
  0

Brian

Re: [PATCH 3/3] mwifiex: pcie: avoid hardcode wifi-only firmware name

[Dmitry Torokhov]

On Thu, Mar 30, 2017 at 2:19 AM, Xinming Hu  wrote:
> From: Xinming Hu 
>
> Wifi-only firmware name should be chipset specific.
>
> Signed-off-by: Xinming Hu 
> Signed-off-by: Amitkumar Karwar 
> ---
>  drivers/net/wireless/marvell/mwifiex/pcie.c | 8 +++-
>  drivers/net/wireless/marvell/mwifiex/pcie.h | 6 ++
>  2 files changed, 13 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/net/wireless/marvell/mwifiex/pcie.c 
> b/drivers/net/wireless/marvell/mwifiex/pcie.c
> index 59cb01a..282aa9a 100644
> --- a/drivers/net/wireless/marvell/mwifiex/pcie.c
> +++ b/drivers/net/wireless/marvell/mwifiex/pcie.c
> @@ -351,6 +351,12 @@ static void mwifiex_pcie_reset_notify(struct pci_dev 
> *pdev, bool prepare)
> struct pcie_service_card *card = pci_get_drvdata(pdev);
> struct mwifiex_adapter *adapter = card->adapter;
>
> +   if (!card->pcie.flr_support) {
> +   pr_err("%s: FLR disabled in current chipset\n", __func__);
> +   return;
> +   }
> +
> +   adapter = card->adapter;
> if (!adapter) {
> dev_err(>dev, "%s: adapter structure is not valid\n",
> __func__);
> @@ -3047,7 +3053,7 @@ static void mwifiex_pcie_up_dev(struct mwifiex_adapter 
> *adapter)
>  * 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);
> +   strcpy(adapter->fw_name, card->pcie.wifi_fw_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 00e8ee5..d6c8526 100644
> --- a/drivers/net/wireless/marvell/mwifiex/pcie.h
> +++ b/drivers/net/wireless/marvell/mwifiex/pcie.h
> @@ -285,6 +285,8 @@ struct mwifiex_pcie_device {
> struct memory_type_mapping *mem_type_mapping_tbl;
> u8 num_mem_types;
> bool can_ext_scan;
> +   u8 flr_support;

This sounds like a boolean, but given that you can key off
wifi_fw_name being NULL I am not sure it is needed.

> +   char *wifi_fw_name;

const char *.

That said, I consider the whole wifi-only firmware business is quite
fragile. Can we have unified firmware and have driver figure out what
part shoudl be [re]loaded?

>  };
>
>  static const struct mwifiex_pcie_device mwifiex_pcie8766 = {
> @@ -293,6 +295,7 @@ struct mwifiex_pcie_device {
> .tx_buf_size = MWIFIEX_TX_DATA_BUF_SIZE_2K,
> .can_dump_fw = false,
> .can_ext_scan = true,
> +   .flr_support = 0,
>  };
>
>  static const struct mwifiex_pcie_device mwifiex_pcie8897 = {
> @@ -303,6 +306,7 @@ struct mwifiex_pcie_device {
> .mem_type_mapping_tbl = mem_type_mapping_tbl_w8897,
> .num_mem_types = ARRAY_SIZE(mem_type_mapping_tbl_w8897),
> .can_ext_scan = true,
> +   .flr_support = 0,
>  };
>
>  static const struct mwifiex_pcie_device mwifiex_pcie8997 = {
> @@ -313,6 +317,8 @@ struct mwifiex_pcie_device {
> .mem_type_mapping_tbl = mem_type_mapping_tbl_w8997,
> .num_mem_types = ARRAY_SIZE(mem_type_mapping_tbl_w8997),
> .can_ext_scan = true,
> +   .flr_support = 1,
> +   .wifi_fw_name = "mrvl/pcie8997_wlan_v4.bin",
>  };
>
>  struct mwifiex_evt_buf_desc {
> --
> 1.8.1.4
>

Thanks.

-- 
Dmitry