Re: [PATCH] Revert "wlcore: Adding suppoprt for IGTK key in wlcore driver"

[Steve deRosier]

Hi Mauro,

On Thu, Aug 27, 2020 at 10:42 AM Mauro Carvalho Chehab
 wrote:
>
> Em Thu, 27 Aug 2020 08:48:30 -0700
> Steve deRosier  escreveu:
>
> > On Tue, Aug 25, 2020 at 10:49 PM Mauro Carvalho Chehab
> >  wrote:
> > >
> > > This patch causes a regression betwen Kernel 5.7 and 5.8 at wlcore:
> > > with it applied, WiFi stops working, and the Kernel starts printing
> > > this message every second:
> > >
> > >wlcore: PHY firmware version: Rev 8.2.0.0.242
> > >wlcore: firmware booted (Rev 8.9.0.0.79)
> > >wlcore: ERROR command execute failure 14
> >
> > Only if NO firmware for the device in question supports the `KEY_IGTK`
> > value, then this revert is appropriate. Otherwise, it likely isn't.
>
> Yeah, that's what I suspect too: some specific firmware is required
> for KEY_IGTK to work.
>
> >  My suspicion is that the feature that `KEY_IGTK` is enabling is
> > specific to a newer firmware that Mauro hasn't upgraded to. What the
> > OP should do is find the updated firmware and give it a try.
>
> I didn't try checking if linux-firmware tree has a newer version on
> it. I'm using Debian Bullseye on this device. So, I suspect that
> it may have a relatively new firmware.
>
> Btw, that's also the version that came together with Fedora 32:
>
> $ strings /lib/firmware/ti-connectivity/wl18xx-fw-4.bin |grep FRev
> FRev 8.9.0.0.79
> FRev 8.2.0.0.242
>
> Looking at:
> https://git.ti.com/cgit/wilink8-wlan/wl18xx_fw/
>
> It sounds that there's a newer version released this year:
>
> 2020-05-28  Updated to FW 8.9.0.0.81
> 2018-07-29  Updated to FW 8.9.0.0.79
>
> However, it doesn't reached linux-firmware upstream yet:
>
> $ git log --pretty=oneline ti-connectivity/wl18xx-fw-4.bin
> 3a5103fc3c29 wl18xx: update firmware file 8.9.0.0.79
> 65b1c68c63f9 wl18xx: update firmware file 8.9.0.0.76
> dbb85a5154a5 wl18xx: update firmware file
> 69a250dd556b wl18xx: update firmware file
> dbe3f134bb69 wl18xx: update firmware file, remove conf file
> dab4b79b3fbc wl18xx: add version 4 of the wl18xx firmware
>
> > AND - since there's some firmware the feature doesn't work with, the
> > driver should be fixed to detect the running firmware version and not
> > do things that the firmware doesn't support.  AND the firmware writer
> > should also make it so the firmware doesn't barf on bad input and
> > instead rejects it politely.
>
> Agreed. The main issue here seems to be that the current patch
> assumes that this feature is available. A proper approach would
> be to check if this feature is available before trying to use it.
>
> Now, I dunno if version 8.9.0.0.81 has what's required for it to
> work - or if KEY_IGTK require some custom firmware version.
>
> If it works with such version, one way would be to add a check
> for this specific version, disabling KEY_IGTK otherwise.
>
> Also, someone from TI should be sending the newer version to
> be added at linux-firmware.
>
> I'll try to do a test maybe tomorrow.
>

I think we're totally agreed on all of the above points.
Fundamentally: the orig patch should've been coded defensively and
tested properly since clearly it causes certain firmwares to break.
Be nice if TI would both update the firmware and also update the
driver to detect the relevant version for features.  I don't know
about this one, but I do know the QCA firmwares (and others) have a
set of feature flags that are detected by the drivers to determine
what is supported.

I look forward to hearing the results of your test.  This whole thing
has gotten me interested. I'd be tempted to pull out the relevant dev
boards and play with them myself, but IIRC they got sent back to a
previous employer and I don't have access to them anymore.


> > But I will say I'm making an educated guess; while I have played with
> > the TI devices in the past, it was years ago and I won't claim to be
> > an expert. I also am unable to fix it myself at this time.
> >
> > I'd just rather see it fixed properly instead of a knee-jerk reaction
> > of reverting it simply because the OP doesn't have current firmware.
>
> > And let's revisit the discussion of having a kernel splat because an
> > unrelated piece of code fails yet the driver does exactly what it is
> > supposed to do. We shouldn't be dumping registers and stack-trace when
> > the code that crashed has nothing to do with the registers and
> > stack-trace outputted. It is a false positive.  A simple printk WARN
> > or ERROR should output notifying us that the chip firmware has crashed
> > and why.  IMHO.
>
> Thanks,
> Mauro

Thanks,
- Steve

Re: [PATCH] Revert "wlcore: Adding suppoprt for IGTK key in wlcore driver"

[Steve deRosier]

On Tue, Aug 25, 2020 at 10:49 PM Mauro Carvalho Chehab
 wrote:
>
> This patch causes a regression betwen Kernel 5.7 and 5.8 at wlcore:
> with it applied, WiFi stops working, and the Kernel starts printing
> this message every second:
>
>wlcore: PHY firmware version: Rev 8.2.0.0.242
>wlcore: firmware booted (Rev 8.9.0.0.79)
>wlcore: ERROR command execute failure 14

Only if NO firmware for the device in question supports the `KEY_IGTK`
value, then this revert is appropriate. Otherwise, it likely isn't.
 My suspicion is that the feature that `KEY_IGTK` is enabling is
specific to a newer firmware that Mauro hasn't upgraded to. What the
OP should do is find the updated firmware and give it a try.

AND - since there's some firmware the feature doesn't work with, the
driver should be fixed to detect the running firmware version and not
do things that the firmware doesn't support.  AND the firmware writer
should also make it so the firmware doesn't barf on bad input and
instead rejects it politely.

But I will say I'm making an educated guess; while I have played with
the TI devices in the past, it was years ago and I won't claim to be
an expert. I also am unable to fix it myself at this time.

I'd just rather see it fixed properly instead of a knee-jerk reaction
of reverting it simply because the OP doesn't have current firmware.

And let's revisit the discussion of having a kernel splat because an
unrelated piece of code fails yet the driver does exactly what it is
supposed to do. We shouldn't be dumping registers and stack-trace when
the code that crashed has nothing to do with the registers and
stack-trace outputted. It is a false positive.  A simple printk WARN
or ERROR should output notifying us that the chip firmware has crashed
and why.  IMHO.

- Steve

Re: [PATCH] ath10k: fix the status check and wrong return

[Steve deRosier]

On Mon, Aug 17, 2020 at 6:43 PM Tang Bin  wrote:
>
> Hi Kalle：
>
> 在 2020/8/17 22:26, Kalle Valo 写道:
> >> In the function ath10k_ahb_clock_init(), devm_clk_get() doesn't
> >> return NULL. Thus use IS_ERR() and PTR_ERR() to validate
> >> the returned value instead of IS_ERR_OR_NULL().
> > Why? What's the benefit of this patch? Or what harm does
> > IS_ERR_OR_NULL() create?
>
> Thanks for you reply, the benefit of this patch is simplify the code,
> because in
>
> this function, I don't think the situation of 'devm_clk_get() return
> NULL' exists.
>

I admit I'm not looking at HEAD, but at least in the two versions I've
got checked out, devm_clk_get() can theoretically return NULL. This
feels like a gratuitous change anyway, but in any case it's wrong and
could cause wrong behavior.

- Steve

Re: [RFC 1/2] devlink: add simple fw crash helpers

[Steve deRosier]

On Fri, May 22, 2020 at 2:51 PM Luis Chamberlain  wrote:
>
> On Fri, May 22, 2020 at 10:46:07PM +0200, Johannes Berg wrote:
> > FWIW, I still completely disagree on that taint. You (Luis) obviously
> > have been running into a bug in that driver, I doubt the firmware
> > actually managed to wedge the hardware.
>
> This hasn't happened just once, its happed many times sporadically now,
> once a week or two weeks I'd say. And the system isn't being moved
> around.
>
> > But even if it did, that's still not really a kernel taint. The kernel
> > itself isn't in any way affected by this.
>
> Of course it is, a full reboot is required.
>
> > Yes, the system is in a weird state now. But that's *not* equivalent to
> > "kernel tainted".
>
> Requiring a full reboot is a dire situation to be in, and loosing
> connectivity to the point this is not recoverable likewise.
>
> You guys are making out a taint to be the end of the world. We have a
> taint even for a kernel warning, and as others have mentioned mac80211
> already produces these.
>

I had to go RTFM re: kernel taints because it has been a very long
time since I looked at them. It had always seemed to me that most were
caused by "kernel-unfriendly" user actions.  The most famous of course
is loading proprietary modules, out-of-tree modules, forced module
loads, etc...  Honestly, I had forgotten the large variety of uses of
the taint flags. For anyone who hasn't looked at taints recently, I
recommend: 
https://www.kernel.org/doc/html/latest/admin-guide/tainted-kernels.html

In light of this I don't object to setting a taint on this anymore.
I'm a little uneasy, but I've softened on it now, and now I feel it
depends on implementation.

Specifically, I don't think we should set a taint flag when a driver
easily handles a routine firmware crash and is confident that things
have come up just fine again. In other words, triggering the taint in
every driver module where it spits out a log comment that it had a
firmware crash and had to recover seems too much. Sure, firmware
shouldn't crash, sure it should be open source so we can fix it,
whatever... those sort of wishful comments simply ignore reality and
our ability to affect effective change. A lot of WiFi firmware crashes
and for well-known cases the drivers handle them well. And in some
cases, not so well and that should be a place the driver should detect
and thus raise a red flag.  If a WiFi firmware crash can bring down
the kernel, there's either a major driver bug or some very funky
hardware crap going on. That sort of thing we should be able to
detect, mark with a taint (or something), and fix if within our sphere
of influence. I guess what it comes down to me is how aggressive we
are about setting the flag.

I would like there to be a single solution, or a minimized set
depending on what makes sense for the requirements. I haven't had time
to look into the alternatives mentioned here so I don't have an
informed opinion about the solution. I do think Luis is trying to
solve a real problem though. Can we look at this from the point of
view of what are the requirements?  What is it we're trying to solve?

I _think_ that the goal of Luis's original proposal is to report up to
the user, at some future point when the user is interested (because
something super drastic just occured, but long after the fw crash),
that there was a firmware crash without the user having to grep
through all logs on the machine. And then if the user sees that flag
and suspects it, then they can bother to find it in the logs or do
more drastic debugging steps like finding the fw crash in the log and
pulling firmware crash dumps, etc.

I think the various alternate solutions are great but perhaps solving
a superset of features (like adding in user-space notifications etc)?
Perhaps different people on these related threads are trying to solve
different problems?


- Steve

Re: [PATCH v4 5/5] mtd: rawnand: micron: Micron SLC NAND filling block

[Steve deRosier]

On Mon, May 18, 2020 at 7:00 AM Bean Huo  wrote:
>
> From: Bean Huo 
>
> On some legacy planar 2D Micron NAND devices when a block erase command

I object the use of the qualifications you're putting in this
sentence. By saying "some legacy" you're implying that there's a
set that does and a set that doesn't require this. Which then leads
the reader of this commit message to #1 look for which ones this
applies to vs not, and #2 want to remove/exclude the feature when
they're using a "current" device. The wiggle-word wording is confusing
and dishonest.

I've followed this discussion now intently and it seems like Micron is
either unable or unwilling to determine which specific devices this
does or doesn't apply to. If you are unable to identify and restrict
this functionality to a specific subset of devices, then the fact is
it's "all."  Let's just say that and eliminate the confusion. And
please also update your datasheets to indicate that this is the
correct algorithm for working with these devices. Better would be to
issue an errata on the chips and notify your customers. I feel for
those customers who aren't using Linux and don't know the reliability
problem they've been tracking down for the last couple of years is
already known but they don't have any way of knowing about it.

In your commit message, rewording to "On planar 2D Micron NAND devices
when a block erase command..." is sufficient.

- Steve


> is issued, occasionally even though a block erase operation completes and
> returns a pass status, the flash block may not be completely erased.
> Subsequent operations to this block on very rare cases can result in subtle
> failures or corruption. These extremely rare cases should nevertheless be
> considered. These rare occurrences have been observed on partially written
> blocks.
>
> To avoid this rare occurrence, we should make sure that at least 15 pages
> have been programmed to a block before it is erased. In case we find that
> less than 15 pages have been programmed, we will rewrite first 15 pages of
> block.
>
> Signed-off-by: Bean Huo 
> ---
>  drivers/mtd/nand/raw/nand_micron.c | 102 +
>  1 file changed, 102 insertions(+)
>
> diff --git a/drivers/mtd/nand/raw/nand_micron.c 
> b/drivers/mtd/nand/raw/nand_micron.c
> index b3485b0995ad..c5fd9e60f46d 100644
> --- a/drivers/mtd/nand/raw/nand_micron.c
> +++ b/drivers/mtd/nand/raw/nand_micron.c
> @@ -36,6 +36,9 @@
>  #define NAND_ECC_STATUS_1_3_CORRECTED  BIT(4)
>  #define NAND_ECC_STATUS_7_8_CORRECTED  (BIT(4) | BIT(3))
>
> +#define MICRON_SHALLOW_ERASE_MIN_PAGE 15
> +#define MICRON_PAGE_MASK_TRIGGER GENMASK(MICRON_SHALLOW_ERASE_MIN_PAGE, 0)
> +
>  struct nand_onfi_vendor_micron {
> u8 two_plane_read;
> u8 read_cache;
> @@ -64,6 +67,7 @@ struct micron_on_die_ecc {
>
>  struct micron_nand {
> struct micron_on_die_ecc ecc;
> +   u16 *writtenp;
>  };
>
>  static int micron_nand_setup_read_retry(struct nand_chip *chip, int 
> retry_mode)
> @@ -429,6 +433,93 @@ static int micron_supports_on_die_ecc(struct nand_chip 
> *chip)
> return MICRON_ON_DIE_SUPPORTED;
>  }
>
> +static int micron_nand_pre_erase(struct nand_chip *chip, u32 eraseblock)
> +{
> +   struct micron_nand *micron = nand_get_manufacturer_data(chip);
> +   struct mtd_info *mtd = nand_to_mtd(chip);
> +   u8 last_page = MICRON_SHALLOW_ERASE_MIN_PAGE - 1;
> +   u32 page;
> +   u8 *data_buf;
> +   int ret, i;
> +
> +   data_buf = nand_get_data_buf(chip);
> +   WARN_ON(!data_buf);
> +
> +   if (likely(micron->writtenp[eraseblock] & BIT(last_page)))
> +   return 0;
> +
> +   page = eraseblock << (chip->phys_erase_shift - chip->page_shift);
> +
> +   if (unlikely(micron->writtenp[eraseblock] == 0)) {
> +   ret = nand_read_page_raw(chip, data_buf, 1, page + last_page);
> +   if (ret)
> +   return ret; /* Read error */
> +   ret = nand_check_is_erased_page(chip, data_buf, true);
> +   if (!ret)
> +   return 0;
> +   }
> +
> +   memset(data_buf, 0x00, mtd->writesize);
> +
> +   for (i = 0; i < MICRON_SHALLOW_ERASE_MIN_PAGE; i++) {
> +   ret = nand_write_page_raw(chip, data_buf, false, page + i);
> +   if (ret)
> +   return ret;
> +   }
> +
> +   return 0;
> +}
> +
> +static int micron_nand_post_erase(struct nand_chip *chip, u32 eraseblock)
> +{
> +   struct micron_nand *micron = nand_get_manufacturer_data(chip);
> +
> +   if (!micron)
> +   return -EINVAL;
> +
> +   micron->writtenp[eraseblock] = 0;
> +
> +   return 0;
> +}
> +
> +static int micron_nand_write_oob(struct nand_chip *chip, loff_t to,
> +struct mtd_oob_ops *ops)
> +{
> +   struct micron_nand *micron = nand_get_manufacturer_data(chip);
> +   u32 eb_sz = nanddev_eraseblock_size(>base);
> +   u32 

Re: [PATCH v2 12/15] ath10k: use new module_firmware_crashed()

[Steve deRosier]

On Mon, May 18, 2020 at 10:19 AM Luis Chamberlain  wrote:
>
> On Mon, May 18, 2020 at 10:15:45AM -0700, Ben Greear wrote:
> >
> >
> > On 05/18/2020 10:09 AM, Luis Chamberlain wrote:
> > > On Mon, May 18, 2020 at 09:58:53AM -0700, Ben Greear wrote:
> > > >
> > > >
> > > > On 05/18/2020 09:51 AM, Luis Chamberlain wrote:
> > > > > On Sat, May 16, 2020 at 03:24:01PM +0200, Johannes Berg wrote:
> > > > > > On Fri, 2020-05-15 at 21:28 +, Luis Chamberlain wrote:> 
> > > > > > module_firmware_crashed
> > > > > >
> > > > > > You didn't CC me or the wireless list on the rest of the patches, 
> > > > > > so I'm
> > > > > > replying to a random one, but ...
> > > > > >
> > > > > > What is the point here?
> > > > > >
> > > > > > This should in no way affect the integrity of the system/kernel, for
> > > > > > most devices anyway.
> > > > >
> > > > > Keyword you used here is "most device". And in the worst case, *who*
> > > > > knows what other odd things may happen afterwards.
> > > > >
> > > > > > So what if ath10k's firmware crashes? If there's a driver bug it 
> > > > > > will
> > > > > > not handle it right (and probably crash, WARN_ON, or something 
> > > > > > else),
> > > > > > but if the driver is working right then that will not affect the 
> > > > > > kernel
> > > > > > at all.
> > > > >
> > > > > Sometimes the device can go into a state which requires driver removal
> > > > > and addition to get things back up.
> > > >
> > > > It would be lovely to be able to detect this case in the driver/system
> > > > somehow!  I haven't seen any such cases recently,
> > >
> > > I assure you that I have run into it. Once it does again I'll report
> > > the crash, but the problem with some of this is that unless you scrape
> > > the log you won't know. Eventually, a uevent would indeed tell inform
> > > me.
> > >
> > > > but in case there is
> > > > some common case you see, maybe we can think of a way to detect it?
> > >
> > > ath10k is just one case, this patch series addresses a simple way to
> > > annotate this tree-wide.
> > >
> > > > > > So maybe I can understand that maybe you want an easy way to 
> > > > > > discover -
> > > > > > per device - that the firmware crashed, but that still doesn't 
> > > > > > warrant a
> > > > > > complete kernel taint.
> > > > >
> > > > > That is one reason, another is that a taint helps support cases *fast*
> > > > > easily detect if the issue was a firmware crash, instead of scraping
> > > > > logs for driver specific ways to say the firmware has crashed.
> > > >
> > > > You can listen for udev events (I think that is the right term),
> > > > and find crashes that way.  You get the actual crash info as well.
> > >
> > > My follow up to this was to add uevent to add_taint() as well, this way
> > > these could generically be processed by userspace.
> >
> > I'm not opposed to the taint, though I have not thought much on it.
> >
> > But, if you can already get the crash info from uevent, and it automatically
> > comes without polling or scraping logs, then what benefit beyond that does
> > the taint give you?
>
> From a support perspective it is a *crystal* clear sign that the device
> and / or device driver may be in a very bad state, in a generic way.
>

Unfortunately a "taint" is interpreted by many users as: "your kernel
is really F#*D up, you better do something about it right now."
Assuming they're paying attention at all in the first place of course.

The fact is, WiFi chip firmware crashes, and in most cases the driver
is able to recover seamlessly. At least that is the case with most QCA
chipsets I work with. And the users or our ability to do anything
about it is minimal to none as we don't have access to firmware
source. It's too bad and I wish it weren't the case, but we have
embraced reality and most drivers have a recovery mechanism built in
for this case. In short, it's a non-event. I fear that elevating this
to a kernel taint will significantly increase "support" requests that
really are nothing but noise; similar to how the firmware load failure
messages (fail to load fw-2.bin, fail to load fw-1.bin, yay loaded
fw-0.bin) cause a lot of noise.

Not specifically opposed, but I wonder what it really accomplishes in
a world where the firmware crashing is pretty much a normal
occurrence.

If it goes in, I think that the drivers shouldn't trigger the taint if
they're able to recover normally. Only trigger on failure to come back
up.  In other words, the ideal place in the ath10k driver isn't where
you have proposed as at that point operation is normal and we're doing
a routine recovery.

- Steve





>   Luis

Re: [PATCH 3/3] libertas_tf: get the MAC address before registering the device

[Steve deRosier]

On Sun, Feb 10, 2019 at 11:52 AM Lubomir Rintel  wrote:
>
> The start() callback is too late for this: NetworkManager would already
> have seen the hardware, thinking 00:00:00:00:00:00 is its permanent
> address.
>
> Signed-off-by: Lubomir Rintel 
> ---
>  .../net/wireless/marvell/libertas_tf/main.c   | 57 ---
>  1 file changed, 11 insertions(+), 46 deletions(-)
>
> diff --git a/drivers/net/wireless/marvell/libertas_tf/main.c 
> b/drivers/net/wireless/marvell/libertas_tf/main.c
> index b4bd3047eb4e..da53da71987e 100644
> --- a/drivers/net/wireless/marvell/libertas_tf/main.c
> +++ b/drivers/net/wireless/marvell/libertas_tf/main.c
> @@ -125,37 +125,6 @@ static void lbtf_cmd_work(struct work_struct *work)
> lbtf_deb_leave(LBTF_DEB_CMD);
>  }
>
> -/**
> - *  lbtf_setup_firmware: initialize firmware.
> - *
> - *  @privA pointer to struct lbtf_private structure
> - *
> - *  Returns: 0 on success.
> - */
> -static int lbtf_setup_firmware(struct lbtf_private *priv)
> -{
> -   int ret = -1;
> -
> -   lbtf_deb_enter(LBTF_DEB_FW);
> -   /*
> -* Read priv address from HW
> -*/
> -   eth_broadcast_addr(priv->current_addr);
> -   ret = lbtf_update_hw_spec(priv);
> -   if (ret) {
> -   ret = -1;
> -   goto done;
> -   }
> -
> -   lbtf_set_mac_control(priv);
> -   lbtf_set_radio_control(priv);
> -
> -   ret = 0;
> -done:
> -   lbtf_deb_leave_args(LBTF_DEB_FW, "ret: %d", ret);
> -   return ret;
> -}
> -
>  /**
>   *  This function handles the timeout of command sending.
>   *  It will re-send the same command again.
> @@ -289,30 +258,17 @@ static void lbtf_tx_work(struct work_struct *work)
>  static int lbtf_op_start(struct ieee80211_hw *hw)
>  {
> struct lbtf_private *priv = hw->priv;
> -   int ret = -1;
>
> lbtf_deb_enter(LBTF_DEB_MACOPS);
>
> -   /* poke the firmware */
> priv->capability = WLAN_CAPABILITY_SHORT_PREAMBLE;
> priv->radioon = RADIO_ON;
> priv->mac_control = CMD_ACT_MAC_RX_ON | CMD_ACT_MAC_TX_ON;
> -   ret = lbtf_setup_firmware(priv);
> -   if (ret)
> -   goto err_setup_firmware;
> -
> -   if ((priv->fwrelease < LBTF_FW_VER_MIN) ||
> -   (priv->fwrelease > LBTF_FW_VER_MAX)) {
> -   ret = -1;
> -   goto err_setup_firmware;
> -   }
> +   lbtf_set_mac_control(priv);
> +   lbtf_set_radio_control(priv);
>
> lbtf_deb_leave(LBTF_DEB_MACOPS);
> return 0;
> -
> -err_setup_firmware:
> -   lbtf_deb_leave_args(LBTF_DEB_MACOPS, "fw setup error; ret=%d", ret);
> -   return ret;
>  }
>
>  static void lbtf_op_stop(struct ieee80211_hw *hw)
> @@ -649,6 +605,15 @@ struct lbtf_private *lbtf_add_card(void *card, struct 
> device *dmdev,
> goto err_init_adapter;
> }
>
> +   eth_broadcast_addr(priv->current_addr);
> +   if (lbtf_update_hw_spec(priv))
> +   goto err_init_adapter;
> +
> +   if (priv->fwrelease < LBTF_FW_VER_MIN ||
> +   priv->fwrelease > LBTF_FW_VER_MAX) {
> +   goto err_init_adapter;
> +   }
> +
> /* The firmware seems to start with the radio enabled. Turn it
>  * off before an actual mac80211 start callback is invoked.
>  */
> --
> 2.20.1
>

Reviewed-by: Steve deRosier 

Re: [PATCH 2/3] libertas_tf: don't defer firmware loading until start()

[Steve deRosier]

On Sun, Feb 10, 2019 at 11:52 AM Lubomir Rintel  wrote:
>
> In order to be able to get a MAC address before we register the device
> with ieee80211 we'll need to load the firmware way earlier.
>
> There seems to be one problem with this: the device seems to start
> with radio enabled and starts sending in frames right after the firmware
> load finishes. This might be a firmware bug. Disable the radio as soon
> as possible.
>

I've got a quibble with calling the behavior a bug. As far as I
remember, it's behaving as designed/specified and was quite
appropriate back when this driver originally went upstream. This is a
10 year old fw and driver and needs have changed - which doesn't mean
it was wrong to do originally.

Now, I'm not saying there's no bugs in the fw. Nor that it wouldn't be
nice to change the behavior to accommodate this change. If I still had
access to the firmware source I even might do so.

...
> @@ -648,6 +642,19 @@ struct lbtf_private *lbtf_add_card(void *card, struct 
> device *dmdev,
>
> INIT_WORK(>cmd_work, lbtf_cmd_work);
> INIT_WORK(>tx_work, lbtf_tx_work);
> +
> +   if (priv->ops->hw_prog_firmware(priv)) {
> +   lbtf_deb_usbd(>dev, "Error programming the firmware\n");
> +   priv->ops->hw_reset_device(priv);
> +   goto err_init_adapter;
> +   }
> +
> +   /* The firmware seems to start with the radio enabled. Turn it
> +* off before an actual mac80211 start callback is invoked.
> +*/
> +   priv->radioon = RADIO_OFF;

Maybe move this up a few lines to before you program the fw? Seems
appropriate to initialize it first. While I expect there's no chance
of a race as the
mac80211 start callback hasn't been invoked yet, superficially it
looks like there could be.


> +   lbtf_set_radio_control(priv);
> +
> if (ieee80211_register_hw(hw))
> goto err_init_adapter;
>
> --
> 2.20.1
>

Reviewed-by: Steve deRosier 

Re: [PATCH 1/3] libertas_tf: move hardware callbacks to a separate structure

[Steve deRosier]

f/main.c 
> b/drivers/net/wireless/marvell/libertas_tf/main.c
> index f93b400db949..8ed3cd158cf5 100644
> --- a/drivers/net/wireless/marvell/libertas_tf/main.c
> +++ b/drivers/net/wireless/marvell/libertas_tf/main.c
> @@ -281,7 +281,7 @@ static void lbtf_tx_work(struct work_struct *work)
> BUG_ON(priv->tx_skb);
> spin_lock_irq(>driver_lock);
> priv->tx_skb = skb;
> -   err = priv->hw_host_to_card(priv, MVMS_DAT, skb->data, skb->len);
> +   err = priv->ops->hw_host_to_card(priv, MVMS_DAT, skb->data, skb->len);
> spin_unlock_irq(>driver_lock);
> if (err) {
> dev_kfree_skb_any(skb);
> @@ -301,7 +301,7 @@ static int lbtf_op_start(struct ieee80211_hw *hw)
>
> if (!priv->fw_ready)
> /* Upload firmware */
> -   if (priv->hw_prog_firmware(card))
> +   if (priv->ops->hw_prog_firmware(card))
> goto err_prog_firmware;
>
> /* poke the firmware */
> @@ -322,7 +322,7 @@ static int lbtf_op_start(struct ieee80211_hw *hw)
> return 0;
>
>  err_prog_firmware:
> -   priv->hw_reset_device(card);
> +   priv->ops->hw_reset_device(card);
> lbtf_deb_leave_args(LBTF_DEB_MACOPS, "error programming fw; ret=%d", 
> ret);
> return ret;
>  }
> @@ -605,7 +605,8 @@ EXPORT_SYMBOL_GPL(lbtf_rx);
>   *
>   *  Returns: pointer to struct lbtf_priv.
>   */
> -struct lbtf_private *lbtf_add_card(void *card, struct device *dmdev)
> +struct lbtf_private *lbtf_add_card(void *card, struct device *dmdev,
> +  const struct lbtf_ops *ops)
>  {
> struct ieee80211_hw *hw;
> struct lbtf_private *priv = NULL;
> @@ -622,6 +623,7 @@ struct lbtf_private *lbtf_add_card(void *card, struct 
> device *dmdev)
>
> priv->hw = hw;
> priv->card = card;
> +   priv->ops = ops;
> priv->tx_skb = NULL;
>
> hw->queues = 1;
> --
> 2.20.1
>

Reviewed-by: Steve deRosier 

Re: [PATCH] libertas_tf: fix signal reporting

[Steve deRosier]

On Sun, Feb 10, 2019 at 11:48 AM Lubomir Rintel  wrote:
>
> Instead of exposing the signal-to-noise ration, calculate the actual signal
> level taking the noise floor into account.
>
> Also, flip the SIGNAL_DBM bit on, so that mac80211 exposes the signal
> level along with the station info in scan results. This fills
> NetworkManager's "nmcli d wifi output" output with colors, bars and joy.
>
> Signed-off-by: Lubomir Rintel 
> ---
>  drivers/net/wireless/marvell/libertas_tf/main.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/net/wireless/marvell/libertas_tf/main.c 
> b/drivers/net/wireless/marvell/libertas_tf/main.c
> index c71ad87cdc44..f93b400db949 100644
> --- a/drivers/net/wireless/marvell/libertas_tf/main.c
> +++ b/drivers/net/wireless/marvell/libertas_tf/main.c
> @@ -562,7 +562,7 @@ int lbtf_rx(struct lbtf_private *priv, struct sk_buff 
> *skb)
> stats.flag |= RX_FLAG_FAILED_FCS_CRC;
> stats.freq = priv->cur_freq;
> stats.band = NL80211_BAND_2GHZ;
> -   stats.signal = prxpd->snr;
> +   stats.signal = prxpd->snr - prxpd->nf;
> priv->noise = prxpd->nf;
> /* Marvell rate index has a hole at value 4 */
> if (prxpd->rx_rate > 4)
> @@ -626,6 +626,7 @@ struct lbtf_private *lbtf_add_card(void *card, struct 
> device *dmdev)
>
> hw->queues = 1;
> ieee80211_hw_set(hw, HOST_BROADCAST_PS_BUFFERING);
> +   ieee80211_hw_set(hw, SIGNAL_DBM);
> hw->extra_tx_headroom = sizeof(struct txpd);
> memcpy(priv->channels, lbtf_channels, sizeof(lbtf_channels));
> memcpy(priv->rates, lbtf_rates, sizeof(lbtf_rates));
> --
> 2.20.1
>

Reviewed-by: Steve deRosier 

Re: [PATCH] libertas_tf: move the banner to a more appropriate place

[Steve deRosier]

On Sun, Feb 10, 2019 at 11:48 AM Lubomir Rintel  wrote:
>
> Also, turn it to a dev_info() to make checkpatch.pl happy.
>
> Signed-off-by: Lubomir Rintel 
> ---
>  drivers/net/wireless/marvell/libertas_tf/main.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/net/wireless/marvell/libertas_tf/main.c 
> b/drivers/net/wireless/marvell/libertas_tf/main.c
> index 1d45da187b9b..c71ad87cdc44 100644
> --- a/drivers/net/wireless/marvell/libertas_tf/main.c
> +++ b/drivers/net/wireless/marvell/libertas_tf/main.c
> @@ -318,7 +318,6 @@ static int lbtf_op_start(struct ieee80211_hw *hw)
> goto err_prog_firmware;
> }
>
> -   printk(KERN_INFO "libertastf: Marvell WLAN 802.11 thinfirm 
> adapter\n");
> lbtf_deb_leave(LBTF_DEB_MACOPS);
> return 0;
>
> @@ -649,6 +648,7 @@ struct lbtf_private *lbtf_add_card(void *card, struct 
> device *dmdev)
> if (ieee80211_register_hw(hw))
> goto err_init_adapter;
>
> +   dev_info(dmdev, "libertastf: Marvell WLAN 802.11 thinfirm adapter\n");
> goto done;
>
>  err_init_adapter:
> --
> 2.20.1
>

Reviewed-by: Steve deRosier 

Re: [PATCH] libertas_tf: don't set URB_ZERO_PACKET on IN USB transfer

[Steve deRosier]

On Sun, Feb 10, 2019 at 11:48 AM Lubomir Rintel  wrote:
>
> It doesn't make sense and the USB core warns on each submit of such
> URB, easily flooding the message buffer with tracebacks.
>
> Analogous issue was fixed in regular libertas driver in commit 6528d8804780
> ("libertas: don't set URB_ZERO_PACKET on IN USB transfer").
>
> Cc: sta...@vger.kernel.org
> Signed-off-by: Lubomir Rintel 
> ---
>  drivers/net/wireless/marvell/libertas_tf/if_usb.c | 2 --
>  1 file changed, 2 deletions(-)
>
> diff --git a/drivers/net/wireless/marvell/libertas_tf/if_usb.c 
> b/drivers/net/wireless/marvell/libertas_tf/if_usb.c
> index 789337ea676a..6ede6168bd85 100644
> --- a/drivers/net/wireless/marvell/libertas_tf/if_usb.c
> +++ b/drivers/net/wireless/marvell/libertas_tf/if_usb.c
> @@ -433,8 +433,6 @@ static int __if_usb_submit_rx_urb(struct if_usb_card 
> *cardp,
>   skb_tail_pointer(skb),
>   MRVDRV_ETH_RX_PACKET_BUFFER_SIZE, callbackfn, 
> cardp);
>
> -   cardp->rx_urb->transfer_flags |= URB_ZERO_PACKET;
> -
> lbtf_deb_usb2(>udev->dev, "Pointer for rx_urb %p\n",
>     cardp->rx_urb);
> ret = usb_submit_urb(cardp->rx_urb, GFP_ATOMIC);
> --
> 2.20.1
>

Reviewed-by: Steve deRosier 

Re: [PATCH] libertas_tf: lower the debug level of command trace

[Steve deRosier]

On Sun, Feb 10, 2019 at 11:47 AM Lubomir Rintel  wrote:
>
> Logging each and every command response is way too much for INFO level.
> Silence this, unless CONFIG_LIBERTAS_THINFIRM_DEBUG has been enabled.
>
> Signed-off-by: Lubomir Rintel 
> ---
>  drivers/net/wireless/marvell/libertas_tf/cmd.c | 7 +++
>  1 file changed, 3 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/net/wireless/marvell/libertas_tf/cmd.c 
> b/drivers/net/wireless/marvell/libertas_tf/cmd.c
> index 909ac3685010..64b147dd2432 100644
> --- a/drivers/net/wireless/marvell/libertas_tf/cmd.c
> +++ b/drivers/net/wireless/marvell/libertas_tf/cmd.c
> @@ -737,10 +737,9 @@ int lbtf_process_rx_command(struct lbtf_private *priv)
> respcmd = le16_to_cpu(resp->command);
> result = le16_to_cpu(resp->result);
>
> -   if (net_ratelimit())
> -   pr_info("libertastf: cmd response 0x%04x, seq %d, size %d\n",
> -   respcmd, le16_to_cpu(resp->seqnum),
> -   le16_to_cpu(resp->size));
> +   lbtf_deb_cmd("libertastf: cmd response 0x%04x, seq %d, size %d\n",
> +respcmd, le16_to_cpu(resp->seqnum),
> +le16_to_cpu(resp->size));
>
> if (resp->seqnum != priv->cur_cmd->cmdbuf->seqnum) {
> spin_unlock_irqrestore(>driver_lock, flags);
> --
> 2.20.1
>

Reviewed-by: Steve deRosier 

Re: [PATCH] wireless: remove unneeded semicolon

[Steve deRosier]

On Thu, Jan 17, 2019 at 7:33 PM YueHaibing  wrote:
>
> remove unneeded semicolon
>
> Signed-off-by: YueHaibing 
> ---
>  drivers/net/wireless/ath/ath10k/wmi.h | 2 +-
>  drivers/net/wireless/ath/ath6kl/init.c| 2 +-
>  drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcmsdh.c | 2 +-
>  drivers/net/wireless/ray_cs.c | 2 +-
>  drivers/net/wireless/realtek/rtlwifi/rtl8723be/phy.c  | 6 +++---
>  5 files changed, 7 insertions(+), 7 deletions(-)
>
...
> diff --git a/drivers/net/wireless/ath/ath6kl/init.c 
> b/drivers/net/wireless/ath/ath6kl/init.c
> index 54132af..aa1c71a 100644
> --- a/drivers/net/wireless/ath/ath6kl/init.c
> +++ b/drivers/net/wireless/ath/ath6kl/init.c
> @@ -1140,7 +1140,7 @@ static int ath6kl_fetch_fw_apin(struct ath6kl *ar, 
> const char *name)
>
> len -= ie_len;
> data += ie_len;
> -   };
> +   }
>
> ret = 0;
>  out:

For ath6kl

Acked-by: Steve deRosier 

Re: [PATCH v2] ath6kl: mark expected switch fall-throughs

[Steve deRosier]

On Fri, May 25, 2018 at 11:23 AM Gustavo A. R. Silva
<gust...@embeddedor.com>
wrote:

> In preparation to enabling -Wimplicit-fallthrough, mark switch cases
> where we are expecting to fall through.

> Signed-off-by: Gustavo A. R. Silva <gust...@embeddedor.com>
> ---
> Changes in v2:
>- Place code comments on a line of their own.

>drivers/net/wireless/ath/ath6kl/cfg80211.c | 3 +++
>1 file changed, 3 insertions(+)

> diff --git a/drivers/net/wireless/ath/ath6kl/cfg80211.c
b/drivers/net/wireless/ath/ath6kl/cfg80211.c
> index 2ba8cf3..a16ee5d 100644
> --- a/drivers/net/wireless/ath/ath6kl/cfg80211.c
> +++ b/drivers/net/wireless/ath/ath6kl/cfg80211.c
> @@ -3899,16 +3899,19 @@ int ath6kl_cfg80211_init(struct ath6kl *ar)
>   switch (ar->hw.cap) {
>   case WMI_11AN_CAP:
>   ht = true;
> +   /* fall through */
>   case WMI_11A_CAP:
>   band_5gig = true;
>   break;
>   case WMI_11GN_CAP:
>   ht = true;
> +   /* fall through */
>   case WMI_11G_CAP:
>   band_2gig = true;
>   break;
>   case WMI_11AGN_CAP:
>   ht = true;
> +   /* fall through */
>   case WMI_11AG_CAP:
>   band_2gig = true;
>   band_5gig = true;
> --
> 2.7.4


Gustavo,

Thanks for the adjustment.  It now looks good to me.

Reviewed-by: Steve deRosier <deros...@cal-sierra.com>

Re: [PATCH v2] ath6kl: mark expected switch fall-throughs

[Steve deRosier]

On Fri, May 25, 2018 at 11:23 AM Gustavo A. R. Silva

wrote:

> In preparation to enabling -Wimplicit-fallthrough, mark switch cases
> where we are expecting to fall through.

> Signed-off-by: Gustavo A. R. Silva 
> ---
> Changes in v2:
>- Place code comments on a line of their own.

>drivers/net/wireless/ath/ath6kl/cfg80211.c | 3 +++
>1 file changed, 3 insertions(+)

> diff --git a/drivers/net/wireless/ath/ath6kl/cfg80211.c
b/drivers/net/wireless/ath/ath6kl/cfg80211.c
> index 2ba8cf3..a16ee5d 100644
> --- a/drivers/net/wireless/ath/ath6kl/cfg80211.c
> +++ b/drivers/net/wireless/ath/ath6kl/cfg80211.c
> @@ -3899,16 +3899,19 @@ int ath6kl_cfg80211_init(struct ath6kl *ar)
>   switch (ar->hw.cap) {
>   case WMI_11AN_CAP:
>   ht = true;
> +   /* fall through */
>   case WMI_11A_CAP:
>   band_5gig = true;
>   break;
>   case WMI_11GN_CAP:
>   ht = true;
> +   /* fall through */
>   case WMI_11G_CAP:
>   band_2gig = true;
>   break;
>   case WMI_11AGN_CAP:
>   ht = true;
> +   /* fall through */
>   case WMI_11AG_CAP:
>   band_2gig = true;
>   band_5gig = true;
> --
> 2.7.4


Gustavo,

Thanks for the adjustment.  It now looks good to me.

Reviewed-by: Steve deRosier 

Re: [PATCH] mtd: partitions: Handle add_mtd_device() failures gracefully

[Steve deRosier]

On Tue, Apr 10, 2018 at 7:47 AM, Geert Uytterhoeven
 wrote:
> Hi Marek,
>
> On Tue, Apr 10, 2018 at 4:37 PM, Marek Vasut  wrote:
>> On 04/10/2018 03:26 PM, Geert Uytterhoeven wrote:
>>> On Mon, Apr 9, 2018 at 11:59 PM, Marek Vasut  wrote:
 On 04/09/2018 02:25 PM, Geert Uytterhoeven wrote:
> Currently add_mtd_device() failures are plainly ignored, which may lead
> to kernel crashes later.
>>>
> Fix this by ignoring and freeing partitions that failed to add in
> add_mtd_partitions().  The same issue is present in mtd_add_partition(),
> so fix that as well.
>
> Signed-off-by: Geert Uytterhoeven 
> ---
> I don't know if it is worthwhile factoring out the common handling.
>
> Should allocate_partition() fail instead?  There's a comment saying
> "let's register it anyway to preserve ordering".
>>>
> --- a/drivers/mtd/mtdpart.c
> +++ b/drivers/mtd/mtdpart.c
>>>
> @@ -746,7 +753,15 @@ int add_mtd_partitions(struct mtd_info *master,
>   list_add(>list, _partitions);
>   mutex_unlock(_partitions_mutex);
>
> - add_mtd_device(>mtd);
> + ret = add_mtd_device(>mtd);
> + if (ret) {
> + mutex_lock(_partitions_mutex);
> + list_del(>list);
> + mutex_unlock(_partitions_mutex);
> + free_partition(slave);
> + continue;
> + }

 Why is the partition even in the list in the first place ? Can we avoid
 adding it rather than adding and removing it ?
>>>
>>> Hence my question "Should allocate_partition() fail instead?".
>>> Note that if we go that route, it should be a "soft" failure, as we
>>> probably don't
>>> want to drop all other partitions on the device.
>> Is the number of partitions ie. in /proc/mtdparts an ABI ?
>
> I don't know.
>

I don't know if it's an ABI, but having consistent /dev/mtdX numbering
is important, even in the case of a failed partition.  Many scripts on
embedded systems are hard-coded to /dev/mtdX identifies with the
expectation that they can access a particular address region of flash.
I'm sure that's what the "let's register it anyway to preserve
ordering" comment was trying to get across. I've even seen weird
things in dts files where later entries specify earlier addresses in
order to leave the old /dev/mtdX numbering alone.

Obviously, a better user solution is to construct the mtdX number from
/proc/mtd based on filtering for the name field, but not everyone
does.

I'd be wary about doing any fix that disturbs the numbering as you'll
be disturbing users.  At a minum, a loud warning in the log.

That said - obviously fixing the kernel crash must happen.

- Steve

Re: [PATCH] mtd: partitions: Handle add_mtd_device() failures gracefully

[Steve deRosier]

On Tue, Apr 10, 2018 at 7:47 AM, Geert Uytterhoeven
 wrote:
> Hi Marek,
>
> On Tue, Apr 10, 2018 at 4:37 PM, Marek Vasut  wrote:
>> On 04/10/2018 03:26 PM, Geert Uytterhoeven wrote:
>>> On Mon, Apr 9, 2018 at 11:59 PM, Marek Vasut  wrote:
 On 04/09/2018 02:25 PM, Geert Uytterhoeven wrote:
> Currently add_mtd_device() failures are plainly ignored, which may lead
> to kernel crashes later.
>>>
> Fix this by ignoring and freeing partitions that failed to add in
> add_mtd_partitions().  The same issue is present in mtd_add_partition(),
> so fix that as well.
>
> Signed-off-by: Geert Uytterhoeven 
> ---
> I don't know if it is worthwhile factoring out the common handling.
>
> Should allocate_partition() fail instead?  There's a comment saying
> "let's register it anyway to preserve ordering".
>>>
> --- a/drivers/mtd/mtdpart.c
> +++ b/drivers/mtd/mtdpart.c
>>>
> @@ -746,7 +753,15 @@ int add_mtd_partitions(struct mtd_info *master,
>   list_add(>list, _partitions);
>   mutex_unlock(_partitions_mutex);
>
> - add_mtd_device(>mtd);
> + ret = add_mtd_device(>mtd);
> + if (ret) {
> + mutex_lock(_partitions_mutex);
> + list_del(>list);
> + mutex_unlock(_partitions_mutex);
> + free_partition(slave);
> + continue;
> + }

 Why is the partition even in the list in the first place ? Can we avoid
 adding it rather than adding and removing it ?
>>>
>>> Hence my question "Should allocate_partition() fail instead?".
>>> Note that if we go that route, it should be a "soft" failure, as we
>>> probably don't
>>> want to drop all other partitions on the device.
>> Is the number of partitions ie. in /proc/mtdparts an ABI ?
>
> I don't know.
>

I don't know if it's an ABI, but having consistent /dev/mtdX numbering
is important, even in the case of a failed partition.  Many scripts on
embedded systems are hard-coded to /dev/mtdX identifies with the
expectation that they can access a particular address region of flash.
I'm sure that's what the "let's register it anyway to preserve
ordering" comment was trying to get across. I've even seen weird
things in dts files where later entries specify earlier addresses in
order to leave the old /dev/mtdX numbering alone.

Obviously, a better user solution is to construct the mtdX number from
/proc/mtd based on filtering for the name field, but not everyone
does.

I'd be wary about doing any fix that disturbs the numbering as you'll
be disturbing users.  At a minum, a loud warning in the log.

That said - obviously fixing the kernel crash must happen.

- Steve

Re: RTL8723BE performance regression

[Steve deRosier]

On Tue, Apr 3, 2018 at 6:51 PM, João Paulo Rechi Vita <jprv...@gmail.com> wrote:
>
> This are the results (testing with speedtest.net) I got at some key points:
>
> VersionCommitPingDownUp
>
> v4.11a351e9b1225.445.99
> v4.11a351e9b131  17.025.89
>
> v4.13569dbb8174  14.080.00
> v4.13569dbb8261  8.41  0.00
>
> v4.15+revert d8a5b801923.861.41
> v4.15+revert d8a5b80189  18.691.39
>

I recommend doing throughput testing in a closed system using iperf.
speedtest.net is potentially useful for testing your ISP's bandwidth
at some particular point in time, but little else as it exposes you to
too many variables. I wouldn't take those numbers to mean much and the
inconclusive results you're getting could be explained by external
network loading and having little to do with your bisect effort. I can
get that spread in numbers from speedtest.net without making any
changes other than the time of day I do the test.

Here's how to do it. Install iperf2 (you could use iperf3, personal
choice) on two machines, one being your device under test (DUT). Setup
a network configuration that looks similar to this:

server <==hardwire==> AP <--wireless link--> DUT

Be sure your hardwire is more bandwidth than your wireless link is
capable of, or set it up where the server is the AP. What you're
looking for here is environmental consistency, not maximum throughput
numbers.

On the computer hardwired to the network, start the server, we'll
assume it has an ip of 192.168.33.18:

iperf -s

On your DUT:

iperf -c 192.168.33.18

That's the most basic setup, check the man page for more options.

You will get best results if you can exclude other computers from your
test network and other wireless devices from your airspace.

- Steve

--
Steve deRosier
Cal-Sierra Consulting LLC
https://www.cal-sierra.com/

Re: RTL8723BE performance regression

[Steve deRosier]

On Tue, Apr 3, 2018 at 6:51 PM, João Paulo Rechi Vita  wrote:
>
> This are the results (testing with speedtest.net) I got at some key points:
>
> VersionCommitPingDownUp
>
> v4.11a351e9b1225.445.99
> v4.11a351e9b131  17.025.89
>
> v4.13569dbb8174  14.080.00
> v4.13569dbb8261  8.41  0.00
>
> v4.15+revert d8a5b801923.861.41
> v4.15+revert d8a5b80189  18.691.39
>

I recommend doing throughput testing in a closed system using iperf.
speedtest.net is potentially useful for testing your ISP's bandwidth
at some particular point in time, but little else as it exposes you to
too many variables. I wouldn't take those numbers to mean much and the
inconclusive results you're getting could be explained by external
network loading and having little to do with your bisect effort. I can
get that spread in numbers from speedtest.net without making any
changes other than the time of day I do the test.

Here's how to do it. Install iperf2 (you could use iperf3, personal
choice) on two machines, one being your device under test (DUT). Setup
a network configuration that looks similar to this:

server <==hardwire==> AP <--wireless link--> DUT

Be sure your hardwire is more bandwidth than your wireless link is
capable of, or set it up where the server is the AP. What you're
looking for here is environmental consistency, not maximum throughput
numbers.

On the computer hardwired to the network, start the server, we'll
assume it has an ip of 192.168.33.18:

iperf -s

On your DUT:

iperf -c 192.168.33.18

That's the most basic setup, check the man page for more options.

You will get best results if you can exclude other computers from your
test network and other wireless devices from your airspace.

- Steve

--
Steve deRosier
Cal-Sierra Consulting LLC
https://www.cal-sierra.com/

Re: [PATCH] ubi: Reject MLC NAND

[Steve deRosier]

Hi Pavel,

On Wed, Mar 7, 2018 at 1:43 PM, Pavel Machek  wrote:
> On Wed 2018-03-07 09:01:16, Richard Weinberger wrote:
>> Pavel,
>>
>> Am Mittwoch, 7. März 2018, 00:18:05 CET schrieb Pavel Machek:
>> > On Sat 2018-03-03 11:45:54, Richard Weinberger wrote:
>> > > While UBI and UBIFS seem to work at first sight with MLC NAND, you will
>> > > most likely lose all your data upon a power-cut or due to read/write
>> > > disturb.
>> > > In order to protect users from bad surprises, refuse to attach to MLC
>> > > NAND.
>> > >
>> > > Cc: sta...@vger.kernel.org
>> >
>> > That sounds like _really_ bad idea for stable. All it does is it
>> > removes support for hardware that somehow works.
>>
>> MLC is not supported and does not work. Full stop.
>> If someone manages to get it somehow work, either with hardware or software
>> hacks they are on their own.
>> Having it in stable is the only chance we have to get it into vendor
>> kernels.
>
> Can you show how it meets the stable kernel criteria? They are
> documented in tree. This should not be in stable.
>
> And I'd like to see changelog improved. Real reason MLC is not
> supported is upper/lower page parts on MLC. And real fix to work with
> bigger pages in UBI.
>

To clarify one thing: the reason for this is MLC has actually never
been supported, nor worked properly. The fact that it kinda worked was
incidental and the cause of major problems for people due to that not
being clear. This patch only makes it explicit and avoids people
mistakenly trying to use UBIFS on MLC flash and risking their data and
products. To me, that's what's important.

This is an important patch, even if all it does is keep people from
loosing data. It also changes the conversation from "I have a
corrupted UBIFS device, BTW it's on MLC..." to "What can we do to get
UBIFS to work on MLC".

I don't know what the stable criteria is with re: to this patch. But
what I do know is if it doesn't go back into the various stables,
there will be manufacturers who will continue to try to use UBIFS on
MLC in ignorance for the next several years until the current stable
kernels EOL, despite there being a known patch that would make it
immediately obvious they shouldn't.

Thanks,
- Steve

Re: [PATCH] ubi: Reject MLC NAND

[Steve deRosier]

Hi Pavel,

On Wed, Mar 7, 2018 at 1:43 PM, Pavel Machek  wrote:
> On Wed 2018-03-07 09:01:16, Richard Weinberger wrote:
>> Pavel,
>>
>> Am Mittwoch, 7. März 2018, 00:18:05 CET schrieb Pavel Machek:
>> > On Sat 2018-03-03 11:45:54, Richard Weinberger wrote:
>> > > While UBI and UBIFS seem to work at first sight with MLC NAND, you will
>> > > most likely lose all your data upon a power-cut or due to read/write
>> > > disturb.
>> > > In order to protect users from bad surprises, refuse to attach to MLC
>> > > NAND.
>> > >
>> > > Cc: sta...@vger.kernel.org
>> >
>> > That sounds like _really_ bad idea for stable. All it does is it
>> > removes support for hardware that somehow works.
>>
>> MLC is not supported and does not work. Full stop.
>> If someone manages to get it somehow work, either with hardware or software
>> hacks they are on their own.
>> Having it in stable is the only chance we have to get it into vendor
>> kernels.
>
> Can you show how it meets the stable kernel criteria? They are
> documented in tree. This should not be in stable.
>
> And I'd like to see changelog improved. Real reason MLC is not
> supported is upper/lower page parts on MLC. And real fix to work with
> bigger pages in UBI.
>

To clarify one thing: the reason for this is MLC has actually never
been supported, nor worked properly. The fact that it kinda worked was
incidental and the cause of major problems for people due to that not
being clear. This patch only makes it explicit and avoids people
mistakenly trying to use UBIFS on MLC flash and risking their data and
products. To me, that's what's important.

This is an important patch, even if all it does is keep people from
loosing data. It also changes the conversation from "I have a
corrupted UBIFS device, BTW it's on MLC..." to "What can we do to get
UBIFS to work on MLC".

I don't know what the stable criteria is with re: to this patch. But
what I do know is if it doesn't go back into the various stables,
there will be manufacturers who will continue to try to use UBIFS on
MLC in ignorance for the next several years until the current stable
kernels EOL, despite there being a known patch that would make it
immediately obvious they shouldn't.

Thanks,
- Steve

Re: [PATCH v2 02/20] ath6kl: constify usb_device_id

[Steve deRosier]

On Wed, Aug 9, 2017 at 9:23 AM, Arvind Yadav <arvind.yadav...@gmail.com> wrote:
> usb_device_id are not supposed to change at runtime. All functions
> working with usb_device_id provided by  work with
> const usb_device_id. So mark the non-const structs as const.
>
> Signed-off-by: Arvind Yadav <arvind.yadav...@gmail.com>
> ---
> changes in v2:
>   Re-submitting wireless separately.
>
>  drivers/net/wireless/ath/ath6kl/usb.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/net/wireless/ath/ath6kl/usb.c 
> b/drivers/net/wireless/ath/ath6kl/usb.c
> index 9da3594..4defb7a 100644
> --- a/drivers/net/wireless/ath/ath6kl/usb.c
> +++ b/drivers/net/wireless/ath/ath6kl/usb.c
> @@ -1201,7 +1201,7 @@ static int ath6kl_usb_pm_resume(struct usb_interface 
> *interface)
>  #endif
>
>  /* table of devices that work with this driver */
> -static struct usb_device_id ath6kl_usb_ids[] = {
> +static const struct usb_device_id ath6kl_usb_ids[] = {
> {USB_DEVICE(0x0cf3, 0x9375)},
> {USB_DEVICE(0x0cf3, 0x9374)},
> { /* Terminating entry */ },
> --
> 2.7.4
>

Looks good. Also builds and works properly.

Reviewed-by: Steve deRosier <deros...@gmail.com>
Tested-by: Steve deRosier <deros...@gmail.com>

- Steve

Re: [PATCH v2 02/20] ath6kl: constify usb_device_id

[Steve deRosier]

On Wed, Aug 9, 2017 at 9:23 AM, Arvind Yadav  wrote:
> usb_device_id are not supposed to change at runtime. All functions
> working with usb_device_id provided by  work with
> const usb_device_id. So mark the non-const structs as const.
>
> Signed-off-by: Arvind Yadav 
> ---
> changes in v2:
>   Re-submitting wireless separately.
>
>  drivers/net/wireless/ath/ath6kl/usb.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/net/wireless/ath/ath6kl/usb.c 
> b/drivers/net/wireless/ath/ath6kl/usb.c
> index 9da3594..4defb7a 100644
> --- a/drivers/net/wireless/ath/ath6kl/usb.c
> +++ b/drivers/net/wireless/ath/ath6kl/usb.c
> @@ -1201,7 +1201,7 @@ static int ath6kl_usb_pm_resume(struct usb_interface 
> *interface)
>  #endif
>
>  /* table of devices that work with this driver */
> -static struct usb_device_id ath6kl_usb_ids[] = {
> +static const struct usb_device_id ath6kl_usb_ids[] = {
> {USB_DEVICE(0x0cf3, 0x9375)},
> {USB_DEVICE(0x0cf3, 0x9374)},
> { /* Terminating entry */ },
> --
> 2.7.4
>

Looks good. Also builds and works properly.

Reviewed-by: Steve deRosier 
Tested-by: Steve deRosier 

- Steve

Re: [PATCH] ath6kl: fix spelling mistake: "Indicat" -> "Indicate"

[Steve deRosier]

On Sun, Jun 4, 2017 at 9:36 AM, Colin King <colin.k...@canonical.com> wrote:
> From: Colin Ian King <colin.k...@canonical.com>
>
> Trivial fix to spelling mistake in ath6kl_dbg debug message
>
> Signed-off-by: Colin Ian King <colin.k...@canonical.com>
> ---
>  drivers/net/wireless/ath/ath6kl/htc_pipe.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/net/wireless/ath/ath6kl/htc_pipe.c 
> b/drivers/net/wireless/ath/ath6kl/htc_pipe.c
> index d127a08d60df..d4fd9e40fffb 100644
> --- a/drivers/net/wireless/ath/ath6kl/htc_pipe.c
> +++ b/drivers/net/wireless/ath/ath6kl/htc_pipe.c
> @@ -383,7 +383,7 @@ static enum htc_send_queue_result htc_try_send(struct 
> htc_target *target,
> list_for_each_entry_safe(packet, tmp_pkt,
>  txq, list) {
> ath6kl_dbg(ATH6KL_DBG_HTC,
> -  "%s: Indicat overflowed TX pkts: 
> %p\n",
> +  "%s: Indicate overflowed TX pkts: 
> %p\n",
>__func__, packet);
> action = ep->ep_cb.tx_full(ep->target, 
> packet);
>     if (action == HTC_SEND_FULL_DROP) {
> --
> 2.11.0
>

Looks good to me.

Reviewed-by: Steve deRosier <deros...@gmail.com>

- Steve

Re: [PATCH] ath6kl: fix spelling mistake: "Indicat" -> "Indicate"

[Steve deRosier]

On Sun, Jun 4, 2017 at 9:36 AM, Colin King  wrote:
> From: Colin Ian King 
>
> Trivial fix to spelling mistake in ath6kl_dbg debug message
>
> Signed-off-by: Colin Ian King 
> ---
>  drivers/net/wireless/ath/ath6kl/htc_pipe.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/net/wireless/ath/ath6kl/htc_pipe.c 
> b/drivers/net/wireless/ath/ath6kl/htc_pipe.c
> index d127a08d60df..d4fd9e40fffb 100644
> --- a/drivers/net/wireless/ath/ath6kl/htc_pipe.c
> +++ b/drivers/net/wireless/ath/ath6kl/htc_pipe.c
> @@ -383,7 +383,7 @@ static enum htc_send_queue_result htc_try_send(struct 
> htc_target *target,
> list_for_each_entry_safe(packet, tmp_pkt,
>  txq, list) {
> ath6kl_dbg(ATH6KL_DBG_HTC,
> -  "%s: Indicat overflowed TX pkts: 
> %p\n",
> +  "%s: Indicate overflowed TX pkts: 
> %p\n",
>__func__, packet);
> action = ep->ep_cb.tx_full(ep->target, 
> packet);
>     if (action == HTC_SEND_FULL_DROP) {
> --
> 2.11.0
>

Looks good to me.

Reviewed-by: Steve deRosier 

- Steve

Re: [PATCH] ath6kl: Add __printf verification to ath6kl_dbg

[Steve deRosier]

On Fri, Mar 31, 2017 at 10:45 AM, Joe Perches <j...@perches.com> wrote:
> On Fri, 2017-03-31 at 10:34 -0700, Steve deRosier wrote:
>> On Fri, Mar 31, 2017 at 10:23 AM, Joe Perches <j...@perches.com> wrote:
>> > On Fri, 2017-03-31 at 10:19 -0700, Steve deRosier wrote:
>> > > On Thu, Mar 30, 2017 at 3:57 PM, Joe Perches <j...@perches.com> wrote:
>> > > > Fix fallout too.
>> >
>> > []
>> > > My only question is why bother doing a format check on something
>> > > that's going to be compiled out anyway?
>> >
>> > To avoid introducing defects when writing new code
>> > and not using the debugging code path.
>> >
>>
>> Fair enough. And I totally agree with the defensive programming here
>> in that case and feel it's worth the tradeoff (if indeed there really
>> is any cost, I'm unsure what gcc actually does in this instance).
>>
>> For sake of discussion though - shouldn't anything not using the debug
>> code path in this case always be of the form that compiles out? ie
>> would be empty functions intended here just to make compilation work
>> and the code that depends on it simpler? Thus, there really should
>> never be a risk of introducing said defects. If any "real" code were
>> put in that else clause, that'd be a big red-flag in the review of
>> said hypothetical patch.
>
> Generically, all debugging forms should strive to avoid side-effects.
>

Yes. Of course. Lightbulb.

I wasn't even thinking of the fact someone could load the printf
arguments with code that might have side-effects instead of simple
variables to print. I never do it for obvious reasons, but I could see
it happening.

Thanks for spending the time going back and forth with me about it.

Thanks,
- Steve

Re: [PATCH] ath6kl: Add __printf verification to ath6kl_dbg

[Steve deRosier]

On Fri, Mar 31, 2017 at 10:45 AM, Joe Perches  wrote:
> On Fri, 2017-03-31 at 10:34 -0700, Steve deRosier wrote:
>> On Fri, Mar 31, 2017 at 10:23 AM, Joe Perches  wrote:
>> > On Fri, 2017-03-31 at 10:19 -0700, Steve deRosier wrote:
>> > > On Thu, Mar 30, 2017 at 3:57 PM, Joe Perches  wrote:
>> > > > Fix fallout too.
>> >
>> > []
>> > > My only question is why bother doing a format check on something
>> > > that's going to be compiled out anyway?
>> >
>> > To avoid introducing defects when writing new code
>> > and not using the debugging code path.
>> >
>>
>> Fair enough. And I totally agree with the defensive programming here
>> in that case and feel it's worth the tradeoff (if indeed there really
>> is any cost, I'm unsure what gcc actually does in this instance).
>>
>> For sake of discussion though - shouldn't anything not using the debug
>> code path in this case always be of the form that compiles out? ie
>> would be empty functions intended here just to make compilation work
>> and the code that depends on it simpler? Thus, there really should
>> never be a risk of introducing said defects. If any "real" code were
>> put in that else clause, that'd be a big red-flag in the review of
>> said hypothetical patch.
>
> Generically, all debugging forms should strive to avoid side-effects.
>

Yes. Of course. Lightbulb.

I wasn't even thinking of the fact someone could load the printf
arguments with code that might have side-effects instead of simple
variables to print. I never do it for obvious reasons, but I could see
it happening.

Thanks for spending the time going back and forth with me about it.

Thanks,
- Steve

Re: [PATCH] ath6kl: Add __printf verification to ath6kl_dbg

[Steve deRosier]

On Fri, Mar 31, 2017 at 10:23 AM, Joe Perches <j...@perches.com> wrote:
> On Fri, 2017-03-31 at 10:19 -0700, Steve deRosier wrote:
>> On Thu, Mar 30, 2017 at 3:57 PM, Joe Perches <j...@perches.com> wrote:
>> > Fix fallout too.
> []
>> My only question is why bother doing a format check on something
>> that's going to be compiled out anyway?
>
> To avoid introducing defects when writing new code
> and not using the debugging code path.
>

Fair enough. And I totally agree with the defensive programming here
in that case and feel it's worth the tradeoff (if indeed there really
is any cost, I'm unsure what gcc actually does in this instance).

For sake of discussion though - shouldn't anything not using the debug
code path in this case always be of the form that compiles out? ie
would be empty functions intended here just to make compilation work
and the code that depends on it simpler? Thus, there really should
never be a risk of introducing said defects. If any "real" code were
put in that else clause, that'd be a big red-flag in the review of
said hypothetical patch.

Thanks,
- Steve

Re: [PATCH] ath6kl: Add __printf verification to ath6kl_dbg

[Steve deRosier]

On Fri, Mar 31, 2017 at 10:23 AM, Joe Perches  wrote:
> On Fri, 2017-03-31 at 10:19 -0700, Steve deRosier wrote:
>> On Thu, Mar 30, 2017 at 3:57 PM, Joe Perches  wrote:
>> > Fix fallout too.
> []
>> My only question is why bother doing a format check on something
>> that's going to be compiled out anyway?
>
> To avoid introducing defects when writing new code
> and not using the debugging code path.
>

Fair enough. And I totally agree with the defensive programming here
in that case and feel it's worth the tradeoff (if indeed there really
is any cost, I'm unsure what gcc actually does in this instance).

For sake of discussion though - shouldn't anything not using the debug
code path in this case always be of the form that compiles out? ie
would be empty functions intended here just to make compilation work
and the code that depends on it simpler? Thus, there really should
never be a risk of introducing said defects. If any "real" code were
put in that else clause, that'd be a big red-flag in the review of
said hypothetical patch.

Thanks,
- Steve

Re: [PATCH] ath6kl: Add __printf verification to ath6kl_dbg

[Steve deRosier]

On Thu, Mar 30, 2017 at 3:57 PM, Joe Perches <j...@perches.com> wrote:
> Fix fallout too.
>
> Signed-off-by: Joe Perches <j...@perches.com>
> ---
>  drivers/net/wireless/ath/ath6kl/debug.h| 2 ++
>  drivers/net/wireless/ath/ath6kl/htc_pipe.c | 2 +-
>  drivers/net/wireless/ath/ath6kl/wmi.c  | 2 +-
>  3 files changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/net/wireless/ath/ath6kl/debug.h 
> b/drivers/net/wireless/ath/ath6kl/debug.h
> index 0614393dd7ae..94297572914f 100644
> --- a/drivers/net/wireless/ath/ath6kl/debug.h
> +++ b/drivers/net/wireless/ath/ath6kl/debug.h
> @@ -63,6 +63,7 @@ int ath6kl_read_tgt_stats(struct ath6kl *ar, struct 
> ath6kl_vif *vif);
>
>  #ifdef CONFIG_ATH6KL_DEBUG
>
> +__printf(2, 3)
>  void ath6kl_dbg(enum ATH6K_DEBUG_MASK mask, const char *fmt, ...);
>  void ath6kl_dbg_dump(enum ATH6K_DEBUG_MASK mask,
>  const char *msg, const char *prefix,
> @@ -83,6 +84,7 @@ int ath6kl_debug_init_fs(struct ath6kl *ar);
>  void ath6kl_debug_cleanup(struct ath6kl *ar);
>
>  #else
> +__printf(2, 3)
>  static inline void ath6kl_dbg(enum ATH6K_DEBUG_MASK dbg_mask,
>   const char *fmt, ...)
>  {

My only question is why bother doing a format check on something
that's going to be compiled out anyway? I suppose the only harm is a
tiny extra bit of compile time due to the check and I'm sure that's
measured in micro-seconds on full development systems, but if we do it
everywhere those tiny bits of time would eventually add up.

Admittedly it's a comment that probably isn't worth redoing the commit
over.  I guess I'm bringing up the point more discuss the question:
"Should we add the printf format verification on the clauses that get
compiled out?"

So, it looks good to me as is, or if you feel like making the change
I'm suggesting, that's fine too.  And it builds and runs on my
platforms.

Reviewed-by: Steve deRosier <deros...@gmail.com>

- Steve

Re: [PATCH] ath6kl: Add __printf verification to ath6kl_dbg

[Steve deRosier]

On Thu, Mar 30, 2017 at 3:57 PM, Joe Perches  wrote:
> Fix fallout too.
>
> Signed-off-by: Joe Perches 
> ---
>  drivers/net/wireless/ath/ath6kl/debug.h| 2 ++
>  drivers/net/wireless/ath/ath6kl/htc_pipe.c | 2 +-
>  drivers/net/wireless/ath/ath6kl/wmi.c  | 2 +-
>  3 files changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/net/wireless/ath/ath6kl/debug.h 
> b/drivers/net/wireless/ath/ath6kl/debug.h
> index 0614393dd7ae..94297572914f 100644
> --- a/drivers/net/wireless/ath/ath6kl/debug.h
> +++ b/drivers/net/wireless/ath/ath6kl/debug.h
> @@ -63,6 +63,7 @@ int ath6kl_read_tgt_stats(struct ath6kl *ar, struct 
> ath6kl_vif *vif);
>
>  #ifdef CONFIG_ATH6KL_DEBUG
>
> +__printf(2, 3)
>  void ath6kl_dbg(enum ATH6K_DEBUG_MASK mask, const char *fmt, ...);
>  void ath6kl_dbg_dump(enum ATH6K_DEBUG_MASK mask,
>  const char *msg, const char *prefix,
> @@ -83,6 +84,7 @@ int ath6kl_debug_init_fs(struct ath6kl *ar);
>  void ath6kl_debug_cleanup(struct ath6kl *ar);
>
>  #else
> +__printf(2, 3)
>  static inline void ath6kl_dbg(enum ATH6K_DEBUG_MASK dbg_mask,
>   const char *fmt, ...)
>  {

My only question is why bother doing a format check on something
that's going to be compiled out anyway? I suppose the only harm is a
tiny extra bit of compile time due to the check and I'm sure that's
measured in micro-seconds on full development systems, but if we do it
everywhere those tiny bits of time would eventually add up.

Admittedly it's a comment that probably isn't worth redoing the commit
over.  I guess I'm bringing up the point more discuss the question:
"Should we add the printf format verification on the clauses that get
compiled out?"

So, it looks good to me as is, or if you feel like making the change
I'm suggesting, that's fine too.  And it builds and runs on my
platforms.

Reviewed-by: Steve deRosier 

- Steve

Re: mwifiex: PCIe8997 chip specific handling

[Steve deRosier]

Hi,

On Wed, Aug 10, 2016 at 12:07 AM, Amitkumar Karwar  wrote:
> Hi Brian,
>
>> From: Brian Norris [mailto:briannor...@chromium.org]
>> Sent: Wednesday, August 10, 2016 12:14 AM
>> To: Amitkumar Karwar
>> Cc: linux-wirel...@vger.kernel.org; Cathy Luo; Nishant Sarmukadam;
>> linux-kernel@vger.kernel.org
>> Subject: Re: mwifiex: PCIe8997 chip specific handling
>>
>> Hi,
>>
>> On Fri, Jul 29, 2016 at 04:08:51PM +0530, Amitkumar Karwar wrote:
>> > The patch corrects the revision id register and uses it along with
>> > magic value and chip version registers to download appropriate
>> > firmware image.
>> >
>> > PCIe8997 Z chipset variant code has been removed, as it won't be used
>> > in production.
>> >
>> > Signed-off-by: Amitkumar Karwar 
>> > ---
>> >  drivers/net/wireless/marvell/mwifiex/pcie.c | 35
>> > ++---
>> > drivers/net/wireless/marvell/mwifiex/pcie.h | 14 +---
>> >  2 files changed, 18 insertions(+), 31 deletions(-)
>>
>> [...]
>>
>> > diff --git a/drivers/net/wireless/marvell/mwifiex/pcie.h
>> > b/drivers/net/wireless/marvell/mwifiex/pcie.h
>> > index f6992f0..46f99ca 100644
>> > --- a/drivers/net/wireless/marvell/mwifiex/pcie.h
>> > +++ b/drivers/net/wireless/marvell/mwifiex/pcie.h
>> > @@ -32,12 +32,9 @@
>> >  #define PCIE8897_DEFAULT_FW_NAME "mrvl/pcie8897_uapsta.bin"
>> >  #define PCIE8897_A0_FW_NAME "mrvl/pcie8897_uapsta_a0.bin"
>> >  #define PCIE8897_B0_FW_NAME "mrvl/pcie8897_uapsta.bin"
>> > -#define PCIE8997_DEFAULT_FW_NAME "mrvl/pcieusb8997_combo_v2.bin"
>> > -#define PCIEUART8997_FW_NAME_Z "mrvl/pcieuart8997_combo.bin"
>> > -#define PCIEUART8997_FW_NAME_V2 "mrvl/pcieuart8997_combo_v2.bin"
>> > -#define PCIEUSB8997_FW_NAME_Z "mrvl/pcieusb8997_combo.bin"
>> > -#define PCIEUSB8997_FW_NAME_V2 "mrvl/pcieusb8997_combo_v2.bin"
>> > -#define PCIE8997_DEFAULT_WIFIFW_NAME "mrvl/pcie8997_wlan.bin"
>> > +#define PCIEUART8997_FW_NAME_V4 "mrvl/pcieuart8997_combo_v4.bin"
>> > +#define PCIEUSB8997_FW_NAME_V4 "mrvl/pcieusb8997_combo_v4.bin"
>> > +#define PCIE8997_DEFAULT_WIFIFW_NAME "mrvl/pcie8997_wlan_v4.bin"
>>
>> Why do version bumps require firmware renames? Is this just to make sure
>> you don't load the new firmware on old chip revs that you don't plan to
>> support for production (i.e., only early revs like the _Z you're
>> dropping)? This doesn't seems like a good long-term solution, at least
>> once you start getting this silicon out in the wild. At some point, I'd
>> expect to see a stable file name.
>>
>> Brian
>>
>
> We haven't yet submitted any firmware image upstream for 8997 chipset.
> pcieuart8997_combo_v4.bin/pcieusb8997_combo_v4.bin would be our firmware 
> candidate for upstream submission. The filename would remain same hereafter.
>
> pcie*8997_combo_v2.bin had support only for A0 chipset
> pcie*8997_combo_v3.bin was our internal development version which had support 
> for A1 chipset
> pcie*8997_combo_v4.bin has support for both A0 and A1 chipsets and this is 
> the version that shall be released to customers/upstream from now on.
>

Seems to me then it should just be named pcie*8997_wlan.bin.  A
version number shouldn't be part of the file name in this case. Having
to update the driver for a firmware name change is silly. Most
wireless drivers have different names for different hardware/chip revs
and/or an incompatible API change.  Most distributions would typically
only carry a single instance of the firmware for a particular chip.
Speaking for the ones I work with, I usually keep the original
filename intact (with a version number) and make a symlink to it with
the name the driver expects.  eg:

fw-4.bin -> fw_v3.4.0.94.bin
fw_v3.2.0.144.bin
fw_v3.4.0.94.bin

That way I can keep track of the version in my filesystem, but I'm not
hacking the driver every couple of weeks.  And we do issue new
firmware every few weeks. I can't imagine asking our customers to keep
updating the driver for each firmware enhancement.

IMHO changing the driver to rename the firmwares on new versions seems
both inconvenient to people using it, and extra non-useful commit
noise.

- Steve

Re: mwifiex: PCIe8997 chip specific handling

[Steve deRosier]

Hi,

On Wed, Aug 10, 2016 at 12:07 AM, Amitkumar Karwar  wrote:
> Hi Brian,
>
>> From: Brian Norris [mailto:briannor...@chromium.org]
>> Sent: Wednesday, August 10, 2016 12:14 AM
>> To: Amitkumar Karwar
>> Cc: linux-wirel...@vger.kernel.org; Cathy Luo; Nishant Sarmukadam;
>> linux-kernel@vger.kernel.org
>> Subject: Re: mwifiex: PCIe8997 chip specific handling
>>
>> Hi,
>>
>> On Fri, Jul 29, 2016 at 04:08:51PM +0530, Amitkumar Karwar wrote:
>> > The patch corrects the revision id register and uses it along with
>> > magic value and chip version registers to download appropriate
>> > firmware image.
>> >
>> > PCIe8997 Z chipset variant code has been removed, as it won't be used
>> > in production.
>> >
>> > Signed-off-by: Amitkumar Karwar 
>> > ---
>> >  drivers/net/wireless/marvell/mwifiex/pcie.c | 35
>> > ++---
>> > drivers/net/wireless/marvell/mwifiex/pcie.h | 14 +---
>> >  2 files changed, 18 insertions(+), 31 deletions(-)
>>
>> [...]
>>
>> > diff --git a/drivers/net/wireless/marvell/mwifiex/pcie.h
>> > b/drivers/net/wireless/marvell/mwifiex/pcie.h
>> > index f6992f0..46f99ca 100644
>> > --- a/drivers/net/wireless/marvell/mwifiex/pcie.h
>> > +++ b/drivers/net/wireless/marvell/mwifiex/pcie.h
>> > @@ -32,12 +32,9 @@
>> >  #define PCIE8897_DEFAULT_FW_NAME "mrvl/pcie8897_uapsta.bin"
>> >  #define PCIE8897_A0_FW_NAME "mrvl/pcie8897_uapsta_a0.bin"
>> >  #define PCIE8897_B0_FW_NAME "mrvl/pcie8897_uapsta.bin"
>> > -#define PCIE8997_DEFAULT_FW_NAME "mrvl/pcieusb8997_combo_v2.bin"
>> > -#define PCIEUART8997_FW_NAME_Z "mrvl/pcieuart8997_combo.bin"
>> > -#define PCIEUART8997_FW_NAME_V2 "mrvl/pcieuart8997_combo_v2.bin"
>> > -#define PCIEUSB8997_FW_NAME_Z "mrvl/pcieusb8997_combo.bin"
>> > -#define PCIEUSB8997_FW_NAME_V2 "mrvl/pcieusb8997_combo_v2.bin"
>> > -#define PCIE8997_DEFAULT_WIFIFW_NAME "mrvl/pcie8997_wlan.bin"
>> > +#define PCIEUART8997_FW_NAME_V4 "mrvl/pcieuart8997_combo_v4.bin"
>> > +#define PCIEUSB8997_FW_NAME_V4 "mrvl/pcieusb8997_combo_v4.bin"
>> > +#define PCIE8997_DEFAULT_WIFIFW_NAME "mrvl/pcie8997_wlan_v4.bin"
>>
>> Why do version bumps require firmware renames? Is this just to make sure
>> you don't load the new firmware on old chip revs that you don't plan to
>> support for production (i.e., only early revs like the _Z you're
>> dropping)? This doesn't seems like a good long-term solution, at least
>> once you start getting this silicon out in the wild. At some point, I'd
>> expect to see a stable file name.
>>
>> Brian
>>
>
> We haven't yet submitted any firmware image upstream for 8997 chipset.
> pcieuart8997_combo_v4.bin/pcieusb8997_combo_v4.bin would be our firmware 
> candidate for upstream submission. The filename would remain same hereafter.
>
> pcie*8997_combo_v2.bin had support only for A0 chipset
> pcie*8997_combo_v3.bin was our internal development version which had support 
> for A1 chipset
> pcie*8997_combo_v4.bin has support for both A0 and A1 chipsets and this is 
> the version that shall be released to customers/upstream from now on.
>

Seems to me then it should just be named pcie*8997_wlan.bin.  A
version number shouldn't be part of the file name in this case. Having
to update the driver for a firmware name change is silly. Most
wireless drivers have different names for different hardware/chip revs
and/or an incompatible API change.  Most distributions would typically
only carry a single instance of the firmware for a particular chip.
Speaking for the ones I work with, I usually keep the original
filename intact (with a version number) and make a symlink to it with
the name the driver expects.  eg:

fw-4.bin -> fw_v3.4.0.94.bin
fw_v3.2.0.144.bin
fw_v3.4.0.94.bin

That way I can keep track of the version in my filesystem, but I'm not
hacking the driver every couple of weeks.  And we do issue new
firmware every few weeks. I can't imagine asking our customers to keep
updating the driver for each firmware enhancement.

IMHO changing the driver to rename the firmwares on new versions seems
both inconvenient to people using it, and extra non-useful commit
noise.

- Steve