[PATCH v2 0/2] mwifiex: Work around firmware bugs on 88W8897 chip

[PATCH v2 0/2] mwifiex: Work around firmware bugs on 88W8897 chip

[Jonas Dreßler]

This is the second revision of the patch, the first one is here:
https://lore.kernel.org/linux-wireless/20210830123704.221494-1-verdre@v0yd.nl/

Changes between v1 and v2:
 - Only read-back the register write to the TX ring write pointer, not all writes
 - Mention firmware version in commit message+code comment for future reference
 - Use -EIO return value in second patch
 - Use definitions for waiting intervals in second patch

Jonas Dreßler (2):
  mwifiex: Use non-posted PCI write when setting TX ring write pointer
  mwifiex: Try waking the firmware until we get an interrupt

 drivers/net/wireless/marvell/mwifiex/pcie.c | 59 +++++++++++++++++----
 1 file changed, 50 insertions(+), 9 deletions(-)

-- 
2.31.1

[PATCH v2 1/2] mwifiex: Use non-posted PCI write when setting TX ring write pointer

[Jonas Dreßler]

On the 88W8897 card it's very important the TX ring write pointer is
updated correctly to its new value before setting the TX ready
interrupt, otherwise the firmware appears to crash (probably because
it's trying to DMA-read from the wrong place). The issue is present in
the latest firmware version 15.68.19.p21 of the pcie+usb card.

Since PCI uses "posted writes" when writing to a register, it's not
guaranteed that a write will happen immediately. That means the pointer
might be outdated when setting the TX ready interrupt, leading to
firmware crashes especially when ASPM L1 and L1 substates are enabled
(because of the higher link latency, the write will probably take
longer).

So fix those firmware crashes by always using a non-posted write for
this specific register write. We do that by simply reading back the
register after writing it, just as a few other PCI drivers do.

This fixes a bug where during rx/tx traffic and with ASPM L1 substates
enabled (the enabled substates are platform dependent), the firmware
crashes and eventually a command timeout appears in the logs.

Cc: stable@vger.kernel.org
Signed-off-by: Jonas Dreßler <verdre@v0yd.nl>
---
 drivers/net/wireless/marvell/mwifiex/pcie.c | 26 ++++++++++++++++++---
 1 file changed, 23 insertions(+), 3 deletions(-)

diff --git a/drivers/net/wireless/marvell/mwifiex/pcie.c b/drivers/net/wireless/marvell/mwifiex/pcie.c
index c6ccce426b49..0eff717ac5fa 100644
--- a/drivers/net/wireless/marvell/mwifiex/pcie.c
+++ b/drivers/net/wireless/marvell/mwifiex/pcie.c
@@ -240,6 +240,20 @@ static int mwifiex_write_reg(struct mwifiex_adapter *adapter, int reg, u32 data)
 	return 0;
 }
 
+/*
+ * This function does a non-posted write into a PCIE card register, ensuring
+ * it's completion before returning.
+ */
+static int mwifiex_write_reg_np(struct mwifiex_adapter *adapter, int reg, u32 data)
+{
+	struct pcie_service_card *card = adapter->card;
+
+	iowrite32(data, card->pci_mmap1 + reg);
+	ioread32(card->pci_mmap1 + reg);
+
+	return 0;
+}
+
 /* This function reads data from PCIE card register.
  */
 static int mwifiex_read_reg(struct mwifiex_adapter *adapter, int reg, u32 *data)
@@ -1482,9 +1496,15 @@ mwifiex_pcie_send_data(struct mwifiex_adapter *adapter, struct sk_buff *skb,
 						reg->tx_rollover_ind);
 
 		rx_val = card->rxbd_rdptr & reg->rx_wrap_mask;
-		/* Write the TX ring write pointer in to reg->tx_wrptr */
-		if (mwifiex_write_reg(adapter, reg->tx_wrptr,
-				      card->txbd_wrptr | rx_val)) {
+		/* Write the TX ring write pointer in to reg->tx_wrptr.
+		 * The firmware (latest version 15.68.19.p21) of the 88W8897
+		 * pcie+usb card seems to crash when getting the TX ready
+		 * interrupt but the TX ring write pointer points to an outdated
+		 * address, so it's important we do a non-posted write here to
+		 * force the completion of the write.
+		 */
+		if (mwifiex_write_reg_np(adapter, reg->tx_wrptr,
+				        card->txbd_wrptr | rx_val)) {
 			mwifiex_dbg(adapter, ERROR,
 				    "SEND DATA: failed to write reg->tx_wrptr\n");
 			ret = -1;
-- 
2.31.1

Re: [PATCH v2 1/2] mwifiex: Use non-posted PCI write when setting TX ring write pointer

[Andy Shevchenko]

On Tue, Sep 14, 2021 at 01:48:12PM +0200, Jonas Dreßler wrote:
> On the 88W8897 card it's very important the TX ring write pointer is
> updated correctly to its new value before setting the TX ready
> interrupt, otherwise the firmware appears to crash (probably because
> it's trying to DMA-read from the wrong place). The issue is present in
> the latest firmware version 15.68.19.p21 of the pcie+usb card.

Please, be consistent in the commit message(s) and the code (esp. if the term
comes from a specification).

Here, PCIe (same in the code, at least that I have noticed, but should be done
everywhere).

> Since PCI uses "posted writes" when writing to a register, it's not
> guaranteed that a write will happen immediately. That means the pointer
> might be outdated when setting the TX ready interrupt, leading to
> firmware crashes especially when ASPM L1 and L1 substates are enabled
> (because of the higher link latency, the write will probably take
> longer).
> 
> So fix those firmware crashes by always using a non-posted write for
> this specific register write. We do that by simply reading back the
> register after writing it, just as a few other PCI drivers do.
> 
> This fixes a bug where during rx/tx traffic and with ASPM L1 substates

Ditto. TX/RX.

> enabled (the enabled substates are platform dependent), the firmware
> crashes and eventually a command timeout appears in the logs.

Should it have a Fixes tag?

> Cc: stable@vger.kernel.org
> Signed-off-by: Jonas Dreßler <verdre@v0yd.nl>

...

> -		/* Write the TX ring write pointer in to reg->tx_wrptr */
> -		if (mwifiex_write_reg(adapter, reg->tx_wrptr,
> -				      card->txbd_wrptr | rx_val)) {
> +		/* Write the TX ring write pointer in to reg->tx_wrptr.
> +		 * The firmware (latest version 15.68.19.p21) of the 88W8897
> +		 * pcie+usb card seems to crash when getting the TX ready
> +		 * interrupt but the TX ring write pointer points to an outdated
> +		 * address, so it's important we do a non-posted write here to
> +		 * force the completion of the write.
> +		 */
> +		if (mwifiex_write_reg_np(adapter, reg->tx_wrptr,
> +				        card->txbd_wrptr | rx_val)) {

>  			mwifiex_dbg(adapter, ERROR,
>  				    "SEND DATA: failed to write reg->tx_wrptr\n");
>  			ret = -1;

I'm not sure how this is not a dead code.

On top of that, I would rather to call old function and explicitly put the
dummy read after it.

		/* Write the TX ring write pointer in to reg->tx_wrptr */
		if (mwifiex_write_reg(adapter, reg->tx_wrptr,
				      card->txbd_wrptr | rx_val)) {
			...eliminate dead code in the following patch(es)...
		}

+		/* The firmware (latest version 15.68.19.p21) of the 88W8897
+		 * pcie+usb card seems to crash when getting the TX ready
+		 * interrupt but the TX ring write pointer points to an outdated
+		 * address, so it's important we do a non-posted write here to
+		 * force the completion of the write.
+		 */
		mwifiex_read_reg(...);

Now, since I found the dummy read function to be present, perhaps you need to
dive more into the code and understand why it exists.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 1/2] mwifiex: Use non-posted PCI write when setting TX ring write pointer

[Jonas Dreßler]

On 9/22/21 1:17 PM, Andy Shevchenko wrote:
> On Tue, Sep 14, 2021 at 01:48:12PM +0200, Jonas Dreßler wrote:
>> On the 88W8897 card it's very important the TX ring write pointer is
>> updated correctly to its new value before setting the TX ready
>> interrupt, otherwise the firmware appears to crash (probably because
>> it's trying to DMA-read from the wrong place). The issue is present in
>> the latest firmware version 15.68.19.p21 of the pcie+usb card.
> 
> Please, be consistent in the commit message(s) and the code (esp. if the term
> comes from a specification).
> 
> Here, PCIe (same in the code, at least that I have noticed, but should be done
> everywhere).
> 
>> Since PCI uses "posted writes" when writing to a register, it's not
>> guaranteed that a write will happen immediately. That means the pointer
>> might be outdated when setting the TX ready interrupt, leading to
>> firmware crashes especially when ASPM L1 and L1 substates are enabled
>> (because of the higher link latency, the write will probably take
>> longer).
>>
>> So fix those firmware crashes by always using a non-posted write for
>> this specific register write. We do that by simply reading back the
>> register after writing it, just as a few other PCI drivers do.
>>
>> This fixes a bug where during rx/tx traffic and with ASPM L1 substates
> 
> Ditto. TX/RX.
> 
>> enabled (the enabled substates are platform dependent), the firmware
>> crashes and eventually a command timeout appears in the logs.
> 
> Should it have a Fixes tag?
> 

Don't think so, there's the infamous 
(https://bugzilla.kernel.org/show_bug.cgi?id=109681) Bugzilla bug it 
fixes though, I'll mention that in v3.

>> Cc: stable@vger.kernel.org
>> Signed-off-by: Jonas Dreßler <verdre@v0yd.nl>
> 
> ...
> 
>> -		/* Write the TX ring write pointer in to reg->tx_wrptr */
>> -		if (mwifiex_write_reg(adapter, reg->tx_wrptr,
>> -				      card->txbd_wrptr | rx_val)) {
>> +		/* Write the TX ring write pointer in to reg->tx_wrptr.
>> +		 * The firmware (latest version 15.68.19.p21) of the 88W8897
>> +		 * pcie+usb card seems to crash when getting the TX ready
>> +		 * interrupt but the TX ring write pointer points to an outdated
>> +		 * address, so it's important we do a non-posted write here to
>> +		 * force the completion of the write.
>> +		 */
>> +		if (mwifiex_write_reg_np(adapter, reg->tx_wrptr,
>> +				        card->txbd_wrptr | rx_val)) {
> 
>>   			mwifiex_dbg(adapter, ERROR,
>>   				    "SEND DATA: failed to write reg->tx_wrptr\n");
>>   			ret = -1;
> 
> I'm not sure how this is not a dead code.
> 
> On top of that, I would rather to call old function and explicitly put the
> dummy read after it
> 
> 		/* Write the TX ring write pointer in to reg->tx_wrptr */
> 		if (mwifiex_write_reg(adapter, reg->tx_wrptr,
> 				      card->txbd_wrptr | rx_val)) {
> 			...eliminate dead code in the following patch(es)...
> 		}
> 
> +		/* The firmware (latest version 15.68.19.p21) of the 88W8897
> +		 * pcie+usb card seems to crash when getting the TX ready
> +		 * interrupt but the TX ring write pointer points to an outdated
> +		 * address, so it's important we do a non-posted write here to
> +		 * force the completion of the write.
> +		 */
> 		mwifiex_read_reg(...);
> 
> Now, since I found the dummy read function to be present, perhaps you need to
> dive more into the code and understand why it exists.
> 

Interesting, I haven't noticed that mwifiex_write_reg() always returns 
0. So are you suggesting to remove that return value and get rid of all 
the "if (mwifiex_write_reg()) {}" checks in a separate commit?

As for why the dummy read/write functions exist, I have no idea. Looking 
at git history it seems they were always there (only change is that 
mwifiex_read_reg() started to handle read errors with commit 
af05148392f50490c662dccee6c502d9fcba33e2). My bet would be that they 
were created to be consistent with sdio.c which is the oldest supported 
bus type in mwifiex.

Re: [PATCH v2 1/2] mwifiex: Use non-posted PCI write when setting TX ring write pointer

[Andy Shevchenko]

On Wed, Sep 22, 2021 at 02:08:39PM +0200, Jonas Dreßler wrote:
> On 9/22/21 1:17 PM, Andy Shevchenko wrote:
> > On Tue, Sep 14, 2021 at 01:48:12PM +0200, Jonas Dreßler wrote:

...

> > Should it have a Fixes tag?
> > 
> 
> Don't think so, there's the infamous
> (https://bugzilla.kernel.org/show_bug.cgi?id=109681) Bugzilla bug it fixes
> though, I'll mention that in v3.

Good idea, use BugLink tag for that!

...

> Interesting, I haven't noticed that mwifiex_write_reg() always returns 0. So
> are you suggesting to remove that return value and get rid of all the "if
> (mwifiex_write_reg()) {}" checks in a separate commit?

Something like this, yes.

> As for why the dummy read/write functions exist, I have no idea. Looking at
> git history it seems they were always there (only change is that
> mwifiex_read_reg() started to handle read errors with commit
> af05148392f50490c662dccee6c502d9fcba33e2). My bet would be that they were
> created to be consistent with sdio.c which is the oldest supported bus type
> in mwifiex.

It has a check against all ones. Also your another patch mentioned wake up.
Perhaps the purpose is to wake up and return if device was/is in power off mode
(D3hot).

-- 
With Best Regards,
Andy Shevchenko

RE: [PATCH v2 1/2] mwifiex: Use non-posted PCI write when setting TX ring write pointer

[David Laight]

From: Jonas Dreßler
> Sent: 14 September 2021 12:48
> 
> On the 88W8897 card it's very important the TX ring write pointer is
> updated correctly to its new value before setting the TX ready
> interrupt, otherwise the firmware appears to crash (probably because
> it's trying to DMA-read from the wrong place). The issue is present in
> the latest firmware version 15.68.19.p21 of the pcie+usb card.
> 
> Since PCI uses "posted writes" when writing to a register, it's not
> guaranteed that a write will happen immediately. That means the pointer
> might be outdated when setting the TX ready interrupt, leading to
> firmware crashes especially when ASPM L1 and L1 substates are enabled
> (because of the higher link latency, the write will probably take
> longer).
> 
> So fix those firmware crashes by always using a non-posted write for
> this specific register write. We do that by simply reading back the
> register after writing it, just as a few other PCI drivers do.
> 
> This fixes a bug where during rx/tx traffic and with ASPM L1 substates
> enabled (the enabled substates are platform dependent), the firmware
> crashes and eventually a command timeout appears in the logs.

I think you need to change your terminology.
PCIe does have some non-posted write transactions - but I can't
remember when they are used.

What you need to say is that you are flushing the PCIe posted
writes in order to avoid a timing 'issue' setting the TX ring
write pointer.

Quite where the bug is, and why the read-back actually fixes
it is another matter.

A typical ethernet transmit needs three things written
in the correct order (as seen by the hardware):

1) The transmit frame data.
2) The descriptor ring entry referring to the frame.
3) The 'prod' of the MAC engine to process the frame.

You seems to also have:
2.5) Write the TX ring write pointer to the MAC engine.

The updates of (1) and (2) are normally handles by DMA coherent
memory or cache flushes done by using the DMA APIs.

If the writes for (2.5) and (3) are both writing to the
PCIe card (which seems likely) then the PCIe spec will
guarantee that they happen in the correct order.

This means that the PCIe readback of the (2.5) write doesn't
have any effect on the order of the bus cycles seen by the card.
So flushing the PCIe write isn't what fixes your problem.

The readback between (2.5) and (3) does have two effects:
a) it adds a short delay between the two writes.
b) it (probably) forces the first write to by flushed through
   any posted-write buffers on the card itself.

It may well be that the card has separate posted write buffers
for different parts of the hardware.
In that case the write (3) might get actioned before the write (2.5).
OTOH you'd expect that to only cause packet transmit to be delayed.

If the write (2.5) ends up being non-atomic (ie a 64bit write
converted to multiple 8 bit writes internally) then you'll hit
problems if the mac engine looks at the register while it is
being changed just after transmitting the previous packet.
(ie when the tx starts before write (3) because the tx logic
is active.)

The other horrid possibility is that you have a truly broken
PCIe slave that corrupts its posted-write buffer when a second
write arrives.
If that is actually true then you may need to also add locks
to ensure that multiple threads cannot do writes at the same time.
Or do all (and I mean all) accesses from a single thread/context.

The latter problem reminds me of a PCI card that got terribly
confused if it saw a read request from a 2nd cpu while generating
'cycle rerun' responses to an earlier read request.

Most code that flushes posted writes only needs to do so for
writes that drop level-sensitive interrupt requests.
Failure to flush those can lead to unexpected interrupts.
That problem goes back to VMEbus sunos (amongst others).

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)

Re: [PATCH v2 1/2] mwifiex: Use non-posted PCI write when setting TX ring write pointer

[Pali Rohár]

On Wednesday 22 September 2021 14:03:25 David Laight wrote:
> From: Jonas Dreßler
> > Sent: 14 September 2021 12:48
> > 
> > On the 88W8897 card it's very important the TX ring write pointer is
> > updated correctly to its new value before setting the TX ready
> > interrupt, otherwise the firmware appears to crash (probably because
> > it's trying to DMA-read from the wrong place). The issue is present in
> > the latest firmware version 15.68.19.p21 of the pcie+usb card.
> > 
> > Since PCI uses "posted writes" when writing to a register, it's not
> > guaranteed that a write will happen immediately. That means the pointer
> > might be outdated when setting the TX ready interrupt, leading to
> > firmware crashes especially when ASPM L1 and L1 substates are enabled
> > (because of the higher link latency, the write will probably take
> > longer).
> > 
> > So fix those firmware crashes by always using a non-posted write for
> > this specific register write. We do that by simply reading back the
> > register after writing it, just as a few other PCI drivers do.
> > 
> > This fixes a bug where during rx/tx traffic and with ASPM L1 substates
> > enabled (the enabled substates are platform dependent), the firmware
> > crashes and eventually a command timeout appears in the logs.
> 
> I think you need to change your terminology.
> PCIe does have some non-posted write transactions - but I can't
> remember when they are used.

In PCIe are all memory write requests as posted.

Non-posted writes in PCIe are used only for IO and config requests. But
this is not case for proposed patch change as it access only card's
memory space.

Technically this patch does not use non-posted memory write (as PCIe
does not support / provide it), just adds something like a barrier and
I'm not sure if it is really correct (you already wrote more details
about it, so I will let it be).

I'm not sure what is the correct terminology, I do not know how this
kind of write-followed-by-read "trick" is correctly called.

RE: [PATCH v2 1/2] mwifiex: Use non-posted PCI write when setting TX ring write pointer

[David Laight]

From: Pali Rohár
> Sent: 22 September 2021 15:27
> 
> On Wednesday 22 September 2021 14:03:25 David Laight wrote:
> > From: Jonas Dreßler
> > > Sent: 14 September 2021 12:48
> > >
> > > On the 88W8897 card it's very important the TX ring write pointer is
> > > updated correctly to its new value before setting the TX ready
> > > interrupt, otherwise the firmware appears to crash (probably because
> > > it's trying to DMA-read from the wrong place). The issue is present in
> > > the latest firmware version 15.68.19.p21 of the pcie+usb card.
> > >
> > > Since PCI uses "posted writes" when writing to a register, it's not
> > > guaranteed that a write will happen immediately. That means the pointer
> > > might be outdated when setting the TX ready interrupt, leading to
> > > firmware crashes especially when ASPM L1 and L1 substates are enabled
> > > (because of the higher link latency, the write will probably take
> > > longer).
> > >
> > > So fix those firmware crashes by always using a non-posted write for
> > > this specific register write. We do that by simply reading back the
> > > register after writing it, just as a few other PCI drivers do.
> > >
> > > This fixes a bug where during rx/tx traffic and with ASPM L1 substates
> > > enabled (the enabled substates are platform dependent), the firmware
> > > crashes and eventually a command timeout appears in the logs.
> >
> > I think you need to change your terminology.
> > PCIe does have some non-posted write transactions - but I can't
> > remember when they are used.
> 
> In PCIe are all memory write requests as posted.
> 
> Non-posted writes in PCIe are used only for IO and config requests. But
> this is not case for proposed patch change as it access only card's
> memory space.
> 
> Technically this patch does not use non-posted memory write (as PCIe
> does not support / provide it), just adds something like a barrier and
> I'm not sure if it is really correct (you already wrote more details
> about it, so I will let it be).
> 
> I'm not sure what is the correct terminology, I do not know how this
> kind of write-followed-by-read "trick" is correctly called.

I think it is probably best to say:
   "flush the posted write when setting the TX ring write pointer".

The write can get posted in any/all of the following places:
1) The cpu store buffer.
2) The PCIe host bridge.
3) Any other PCIe bridges.
4) The PCIe slave logic in the target.
   There could be separate buffers for each BAR,
5) The actual target logic for that address block.
   The target (probably) will look a bit like an old fashioned cpu
   motherboard with the PCIe slave logic as the main bus master.

The readback forces all the posted write buffers be flushed.

In this case I suspect it is either flushing (5) or the extra
delay of the read TLP processing that 'fixes' the problem.

Note that depending on the exact code and host cpu the second
write may not need to wait for the response to the read TLP.
So the write, readback, write TLP may be back to back on the
actual PCIe link.

Although I don't have access to an actual PCIe monitor we
do have the ability to trace 'data' TLP into fpga memory
on one of our systems.
This is near real-time but they are slightly munged.
Watching the TLP can be illuminating!

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)

Re: [PATCH v2 1/2] mwifiex: Use non-posted PCI write when setting TX ring write pointer

[Jonas Dreßler]

On 9/22/21 5:54 PM, David Laight wrote:
> 
> From: Pali Rohár
>> Sent: 22 September 2021 15:27
>>
>> On Wednesday 22 September 2021 14:03:25 David Laight wrote:
>>> From: Jonas Dreßler
>>>> Sent: 14 September 2021 12:48
>>>>
>>>> On the 88W8897 card it's very important the TX ring write pointer is
>>>> updated correctly to its new value before setting the TX ready
>>>> interrupt, otherwise the firmware appears to crash (probably because
>>>> it's trying to DMA-read from the wrong place). The issue is present in
>>>> the latest firmware version 15.68.19.p21 of the pcie+usb card.
>>>>
>>>> Since PCI uses "posted writes" when writing to a register, it's not
>>>> guaranteed that a write will happen immediately. That means the pointer
>>>> might be outdated when setting the TX ready interrupt, leading to
>>>> firmware crashes especially when ASPM L1 and L1 substates are enabled
>>>> (because of the higher link latency, the write will probably take
>>>> longer).
>>>>
>>>> So fix those firmware crashes by always using a non-posted write for
>>>> this specific register write. We do that by simply reading back the
>>>> register after writing it, just as a few other PCI drivers do.
>>>>
>>>> This fixes a bug where during rx/tx traffic and with ASPM L1 substates
>>>> enabled (the enabled substates are platform dependent), the firmware
>>>> crashes and eventually a command timeout appears in the logs.
>>>
>>> I think you need to change your terminology.
>>> PCIe does have some non-posted write transactions - but I can't
>>> remember when they are used.
>>
>> In PCIe are all memory write requests as posted.
>>
>> Non-posted writes in PCIe are used only for IO and config requests. But
>> this is not case for proposed patch change as it access only card's
>> memory space.
>>
>> Technically this patch does not use non-posted memory write (as PCIe
>> does not support / provide it), just adds something like a barrier and
>> I'm not sure if it is really correct (you already wrote more details
>> about it, so I will let it be).
>>
>> I'm not sure what is the correct terminology, I do not know how this
>> kind of write-followed-by-read "trick" is correctly called.
> 
> I think it is probably best to say:
>     "flush the posted write when setting the TX ring write pointer".
> 
> The write can get posted in any/all of the following places:
> 1) The cpu store buffer.
> 2) The PCIe host bridge.
> 3) Any other PCIe bridges.
> 4) The PCIe slave logic in the target.
>     There could be separate buffers for each BAR,
> 5) The actual target logic for that address block.
>     The target (probably) will look a bit like an old fashioned cpu
>     motherboard with the PCIe slave logic as the main bus master.
> 
> The readback forces all the posted write buffers be flushed.
> 
> In this case I suspect it is either flushing (5) or the extra
> delay of the read TLP processing that 'fixes' the problem.
> 
> Note that depending on the exact code and host cpu the second
> write may not need to wait for the response to the read TLP.
> So the write, readback, write TLP may be back to back on the
> actual PCIe link.
> 
> Although I don't have access to an actual PCIe monitor we
> do have the ability to trace 'data' TLP into fpga memory
> on one of our systems.
> This is near real-time but they are slightly munged.
> Watching the TLP can be illuminating!
> 
> 	David
> 
> -
> Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
> Registration No: 1397386 (Wales)
> 

Thanks for the detailed explanations, it looks like indeed the read-back 
is not the real fix here, a simple udelay(50) before sending the "TX 
ready" interrupt also does the trick.

                 } else {
+                       udelay(50);
+
                         /* Send the TX ready interrupt */
                         if (mwifiex_write_reg(adapter, PCIE_CPU_INT_EVENT,
                                               CPU_INTR_DNLD_RDY)) {

I've tested that for a week now and haven't seen any firmware crashes. 
Interestingly enough it looks like the delay can also be added after 
setting the "TX ready" interrupt, just not before updating the TX ring 
write pointer.

I have no idea if 50 usecs is a good duration to wait here, from trying 
different values I found that 10 to 20 usecs is not enough, but who 
knows, maybe that's platform dependent?

Re: [PATCH v2 1/2] mwifiex: Use non-posted PCI write when setting TX ring write pointer

[Jonas Dreßler]

On 9/30/21 16:27, Jonas Dreßler wrote:
> On 9/22/21 5:54 PM, David Laight wrote:
>>
>> From: Pali Rohár
>>> Sent: 22 September 2021 15:27
>>>
>>> On Wednesday 22 September 2021 14:03:25 David Laight wrote:
>>>> From: Jonas Dreßler
>>>>> Sent: 14 September 2021 12:48
>>>>>
>>>>> On the 88W8897 card it's very important the TX ring write pointer is
>>>>> updated correctly to its new value before setting the TX ready
>>>>> interrupt, otherwise the firmware appears to crash (probably because
>>>>> it's trying to DMA-read from the wrong place). The issue is present in
>>>>> the latest firmware version 15.68.19.p21 of the pcie+usb card.
>>>>>
>>>>> Since PCI uses "posted writes" when writing to a register, it's not
>>>>> guaranteed that a write will happen immediately. That means the pointer
>>>>> might be outdated when setting the TX ready interrupt, leading to
>>>>> firmware crashes especially when ASPM L1 and L1 substates are enabled
>>>>> (because of the higher link latency, the write will probably take
>>>>> longer).
>>>>>
>>>>> So fix those firmware crashes by always using a non-posted write for
>>>>> this specific register write. We do that by simply reading back the
>>>>> register after writing it, just as a few other PCI drivers do.
>>>>>
>>>>> This fixes a bug where during rx/tx traffic and with ASPM L1 substates
>>>>> enabled (the enabled substates are platform dependent), the firmware
>>>>> crashes and eventually a command timeout appears in the logs.
>>>>
>>>> I think you need to change your terminology.
>>>> PCIe does have some non-posted write transactions - but I can't
>>>> remember when they are used.
>>>
>>> In PCIe are all memory write requests as posted.
>>>
>>> Non-posted writes in PCIe are used only for IO and config requests. But
>>> this is not case for proposed patch change as it access only card's
>>> memory space.
>>>
>>> Technically this patch does not use non-posted memory write (as PCIe
>>> does not support / provide it), just adds something like a barrier and
>>> I'm not sure if it is really correct (you already wrote more details
>>> about it, so I will let it be).
>>>
>>> I'm not sure what is the correct terminology, I do not know how this
>>> kind of write-followed-by-read "trick" is correctly called.
>>
>> I think it is probably best to say:
>>     "flush the posted write when setting the TX ring write pointer".
>>
>> The write can get posted in any/all of the following places:
>> 1) The cpu store buffer.
>> 2) The PCIe host bridge.
>> 3) Any other PCIe bridges.
>> 4) The PCIe slave logic in the target.
>>     There could be separate buffers for each BAR,
>> 5) The actual target logic for that address block.
>>     The target (probably) will look a bit like an old fashioned cpu
>>     motherboard with the PCIe slave logic as the main bus master.
>>
>> The readback forces all the posted write buffers be flushed.
>>
>> In this case I suspect it is either flushing (5) or the extra
>> delay of the read TLP processing that 'fixes' the problem.
>>
>> Note that depending on the exact code and host cpu the second
>> write may not need to wait for the response to the read TLP.
>> So the write, readback, write TLP may be back to back on the
>> actual PCIe link.
>>
>> Although I don't have access to an actual PCIe monitor we
>> do have the ability to trace 'data' TLP into fpga memory
>> on one of our systems.
>> This is near real-time but they are slightly munged.
>> Watching the TLP can be illuminating!
>>
>>     David
>>
>> -
>> Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
>> Registration No: 1397386 (Wales)
>>
> 
> Thanks for the detailed explanations, it looks like indeed the read-back is not the real fix here, a simple udelay(50) before sending the "TX ready" interrupt also does the trick.
> 
>                  } else {
> +                       udelay(50);
> +
>                          /* Send the TX ready interrupt */
>                          if (mwifiex_write_reg(adapter, PCIE_CPU_INT_EVENT,
>                                                CPU_INTR_DNLD_RDY)) {
> 
> I've tested that for a week now and haven't seen any firmware crashes. Interestingly enough it looks like the delay can also be added after setting the "TX ready" interrupt, just not before updating the TX ring write pointer.
> 
> I have no idea if 50 usecs is a good duration to wait here, from trying different values I found that 10 to 20 usecs is not enough, but who knows, maybe that's platform dependent?

So I spent the last few days going slightly crazy while trying to dig deeper
into this.

My theory was that the udelay() delays some subsequent register write or
other communication with the card that would trigger the crash if executed
too early after writing the TX ring write pointer. So I tried moving the
udelay() around, carefully checking when the crash is gone and when it isn't.

In the end my theory turned out completely wrong, what I found was this:
Pinning down the last place where the udelay() is effective gets us here
(https://elixir.bootlin.com/linux/latest/source/drivers/net/wireless/marvell/mwifiex/main.c#L340),
right before we bail out of the main process and idle.

I tried adding the udelay() as the first thing we do on the next run of the
while-loop after that break, but with that the crash came back.

So what does this mean, we fix the crash by sleeping before idling? Sounds
a bit counterintuitive to me...

The only thing I can take away from this is that maybe the udelay() keeps
the CPU from entering some powersaving state and with that the PCI bus from
entering ASPM states (considering that the crash can also be fixed by
disabling ASPM L1.2).

[PATCH v2 2/2] mwifiex: Try waking the firmware until we get an interrupt

[Jonas Dreßler]

It seems that the firmware of the 88W8897 card sometimes ignores or
misses when we try to wake it up by writing to the firmware status
register. This leads to the firmware wakeup timeout expiring and the
driver resetting the card because we assume the firmware has hung up or
crashed (unfortunately that's not unlikely with this card).

Turns out that most of the time the firmware actually didn't hang up,
but simply "missed" our wakeup request and didn't send us an AWAKE
event.

Trying again to read the firmware status register after a short timeout
usually makes the firmware wake up as expected, so add a small retry
loop to mwifiex_pm_wakeup_card() that looks at the interrupt status to
check whether the card woke up.

The number of tries and timeout lengths for this were determined
experimentally: The firmware usually takes about 500 us to wake up
after we attempt to read the status register. In some cases where the
firmware is very busy (for example while doing a bluetooth scan) it
might even miss our requests for multiple milliseconds, which is why
after 15 tries the waiting time gets increased to 10 ms. The maximum
number of tries it took to wake the firmware when testing this was
around 20, so a maximum number of 50 tries should give us plenty of
safety margin.

A good reproducer for this issue is letting the firmware sleep and wake
up in very short intervals, for example by pinging a device on the
network every 0.1 seconds.

Cc: stable@vger.kernel.org
Signed-off-by: Jonas Dreßler <verdre@v0yd.nl>
---
 drivers/net/wireless/marvell/mwifiex/pcie.c | 33 +++++++++++++++++----
 1 file changed, 27 insertions(+), 6 deletions(-)

diff --git a/drivers/net/wireless/marvell/mwifiex/pcie.c b/drivers/net/wireless/marvell/mwifiex/pcie.c
index 0eff717ac5fa..7fea319e013c 100644
--- a/drivers/net/wireless/marvell/mwifiex/pcie.c
+++ b/drivers/net/wireless/marvell/mwifiex/pcie.c
@@ -661,11 +661,15 @@ static void mwifiex_delay_for_sleep_cookie(struct mwifiex_adapter *adapter,
 			    "max count reached while accessing sleep cookie\n");
 }
 
+#define N_WAKEUP_TRIES_SHORT_INTERVAL 15
+#define N_WAKEUP_TRIES_LONG_INTERVAL 35
+
 /* This function wakes up the card by reading fw_status register. */
 static int mwifiex_pm_wakeup_card(struct mwifiex_adapter *adapter)
 {
 	struct pcie_service_card *card = adapter->card;
 	const struct mwifiex_pcie_card_reg *reg = card->pcie.reg;
+	int n_tries = 0;
 
 	mwifiex_dbg(adapter, EVENT,
 		    "event: Wakeup device...\n");
@@ -673,12 +677,29 @@ static int mwifiex_pm_wakeup_card(struct mwifiex_adapter *adapter)
 	if (reg->sleep_cookie)
 		mwifiex_pcie_dev_wakeup_delay(adapter);
 
-	/* Accessing fw_status register will wakeup device */
-	if (mwifiex_write_reg(adapter, reg->fw_status, FIRMWARE_READY_PCIE)) {
-		mwifiex_dbg(adapter, ERROR,
-			    "Writing fw_status register failed\n");
-		return -1;
-	}
+	/* Access the fw_status register to wake up the device.
+	 * Since the 88W8897 firmware sometimes appears to ignore or miss
+	 * that wakeup request, we continue trying until we receive an
+	 * interrupt from the card.
+	 */
+	do {
+		if (mwifiex_write_reg(adapter, reg->fw_status, FIRMWARE_READY_PCIE)) {
+			mwifiex_dbg(adapter, ERROR,
+				    "Writing fw_status register failed\n");
+			return -EIO;
+		}
+
+		n_tries++;
+
+		if (n_tries <= N_WAKEUP_TRIES_SHORT_INTERVAL)
+			usleep_range(400, 700);
+		else
+			msleep(10);
+	} while (n_tries <= N_WAKEUP_TRIES_SHORT_INTERVAL + N_WAKEUP_TRIES_LONG_INTERVAL &&
+		 READ_ONCE(adapter->int_status) == 0);
+
+	mwifiex_dbg(adapter, EVENT,
+		    "event: Tried %d times until firmware woke up\n", n_tries);
 
 	if (reg->sleep_cookie) {
 		mwifiex_pcie_dev_wakeup_delay(adapter);
-- 
2.31.1

Re: [PATCH v2 2/2] mwifiex: Try waking the firmware until we get an interrupt

[Andy Shevchenko]

On Tue, Sep 14, 2021 at 01:48:13PM +0200, Jonas Dreßler wrote:
> It seems that the firmware of the 88W8897 card sometimes ignores or
> misses when we try to wake it up by writing to the firmware status
> register. This leads to the firmware wakeup timeout expiring and the
> driver resetting the card because we assume the firmware has hung up or
> crashed (unfortunately that's not unlikely with this card).
> 
> Turns out that most of the time the firmware actually didn't hang up,
> but simply "missed" our wakeup request and didn't send us an AWAKE
> event.
> 
> Trying again to read the firmware status register after a short timeout
> usually makes the firmware wake up as expected, so add a small retry
> loop to mwifiex_pm_wakeup_card() that looks at the interrupt status to
> check whether the card woke up.
> 
> The number of tries and timeout lengths for this were determined
> experimentally: The firmware usually takes about 500 us to wake up
> after we attempt to read the status register. In some cases where the
> firmware is very busy (for example while doing a bluetooth scan) it
> might even miss our requests for multiple milliseconds, which is why
> after 15 tries the waiting time gets increased to 10 ms. The maximum
> number of tries it took to wake the firmware when testing this was
> around 20, so a maximum number of 50 tries should give us plenty of
> safety margin.
> 
> A good reproducer for this issue is letting the firmware sleep and wake
> up in very short intervals, for example by pinging a device on the
> network every 0.1 seconds.

...

> +	do {
> +		if (mwifiex_write_reg(adapter, reg->fw_status, FIRMWARE_READY_PCIE)) {
> +			mwifiex_dbg(adapter, ERROR,
> +				    "Writing fw_status register failed\n");
> +			return -EIO;
> +		}
> +
> +		n_tries++;
> +
> +		if (n_tries <= N_WAKEUP_TRIES_SHORT_INTERVAL)
> +			usleep_range(400, 700);
> +		else
> +			msleep(10);
> +	} while (n_tries <= N_WAKEUP_TRIES_SHORT_INTERVAL + N_WAKEUP_TRIES_LONG_INTERVAL &&
> +		 READ_ONCE(adapter->int_status) == 0);

Can't you use read_poll_timeout() twice instead of this custom approach?

> +	mwifiex_dbg(adapter, EVENT,
> +		    "event: Tried %d times until firmware woke up\n", n_tries);

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 2/2] mwifiex: Try waking the firmware until we get an interrupt

[Jonas Dreßler]

On 9/22/21 1:19 PM, Andy Shevchenko wrote:
> On Tue, Sep 14, 2021 at 01:48:13PM +0200, Jonas Dreßler wrote:
>> It seems that the firmware of the 88W8897 card sometimes ignores or
>> misses when we try to wake it up by writing to the firmware status
>> register. This leads to the firmware wakeup timeout expiring and the
>> driver resetting the card because we assume the firmware has hung up or
>> crashed (unfortunately that's not unlikely with this card).
>>
>> Turns out that most of the time the firmware actually didn't hang up,
>> but simply "missed" our wakeup request and didn't send us an AWAKE
>> event.
>>
>> Trying again to read the firmware status register after a short timeout
>> usually makes the firmware wake up as expected, so add a small retry
>> loop to mwifiex_pm_wakeup_card() that looks at the interrupt status to
>> check whether the card woke up.
>>
>> The number of tries and timeout lengths for this were determined
>> experimentally: The firmware usually takes about 500 us to wake up
>> after we attempt to read the status register. In some cases where the
>> firmware is very busy (for example while doing a bluetooth scan) it
>> might even miss our requests for multiple milliseconds, which is why
>> after 15 tries the waiting time gets increased to 10 ms. The maximum
>> number of tries it took to wake the firmware when testing this was
>> around 20, so a maximum number of 50 tries should give us plenty of
>> safety margin.
>>
>> A good reproducer for this issue is letting the firmware sleep and wake
>> up in very short intervals, for example by pinging a device on the
>> network every 0.1 seconds.
> 
> ...
> 
>> +	do {
>> +		if (mwifiex_write_reg(adapter, reg->fw_status, FIRMWARE_READY_PCIE)) {
>> +			mwifiex_dbg(adapter, ERROR,
>> +				    "Writing fw_status register failed\n");
>> +			return -EIO;
>> +		}
>> +
>> +		n_tries++;
>> +
>> +		if (n_tries <= N_WAKEUP_TRIES_SHORT_INTERVAL)
>> +			usleep_range(400, 700);
>> +		else
>> +			msleep(10);
>> +	} while (n_tries <= N_WAKEUP_TRIES_SHORT_INTERVAL + N_WAKEUP_TRIES_LONG_INTERVAL &&
>> +		 READ_ONCE(adapter->int_status) == 0);
> 
> Can't you use read_poll_timeout() twice instead of this custom approach?
> 

I've tried this now, but read_poll_timeout() is not ideal for our 
use-case. What we'd need would be read->sleep->poll->repeat instead of 
read->poll->sleep->repeat. With read_poll_timeout() we always end up 
doing one more (unnecessary) write.

>> +	mwifiex_dbg(adapter, EVENT,
>> +		    "event: Tried %d times until firmware woke up\n", n_tries);
> 

Re: [PATCH v2 2/2] mwifiex: Try waking the firmware until we get an interrupt

[Andy Shevchenko]

On Thu, Sep 30, 2021 at 08:04:00PM +0200, Jonas Dreßler wrote:
> On 9/22/21 1:19 PM, Andy Shevchenko wrote:
> > On Tue, Sep 14, 2021 at 01:48:13PM +0200, Jonas Dreßler wrote:

...

> > > +	do {
> > > +		if (mwifiex_write_reg(adapter, reg->fw_status, FIRMWARE_READY_PCIE)) {
> > > +			mwifiex_dbg(adapter, ERROR,
> > > +				    "Writing fw_status register failed\n");
> > > +			return -EIO;
> > > +		}
> > > +
> > > +		n_tries++;
> > > +
> > > +		if (n_tries <= N_WAKEUP_TRIES_SHORT_INTERVAL)
> > > +			usleep_range(400, 700);
> > > +		else
> > > +			msleep(10);
> > > +	} while (n_tries <= N_WAKEUP_TRIES_SHORT_INTERVAL + N_WAKEUP_TRIES_LONG_INTERVAL &&
> > > +		 READ_ONCE(adapter->int_status) == 0);
> > 
> > Can't you use read_poll_timeout() twice instead of this custom approach?
> 
> I've tried this now, but read_poll_timeout() is not ideal for our use-case.
> What we'd need would be read->sleep->poll->repeat instead of
> read->poll->sleep->repeat. With read_poll_timeout() we always end up doing
> one more (unnecessary) write.

First of all, there is a parameter to get sleep beforehand.
Second, what is the problem with having one write more or less?
Your current code doesn't guarantee this either. It only decreases
probability of such scenario. Am I wrong?


-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 2/2] mwifiex: Try waking the firmware until we get an interrupt

[Jonas Dreßler]

On 9/30/21 10:58 PM, Andy Shevchenko wrote:
> On Thu, Sep 30, 2021 at 08:04:00PM +0200, Jonas Dreßler wrote:
>> On 9/22/21 1:19 PM, Andy Shevchenko wrote:
>>> On Tue, Sep 14, 2021 at 01:48:13PM +0200, Jonas Dreßler wrote:
> 
> ...
> 
>>>> +	do {
>>>> +		if (mwifiex_write_reg(adapter, reg->fw_status, FIRMWARE_READY_PCIE)) {
>>>> +			mwifiex_dbg(adapter, ERROR,
>>>> +				    "Writing fw_status register failed\n");
>>>> +			return -EIO;
>>>> +		}
>>>> +
>>>> +		n_tries++;
>>>> +
>>>> +		if (n_tries <= N_WAKEUP_TRIES_SHORT_INTERVAL)
>>>> +			usleep_range(400, 700);
>>>> +		else
>>>> +			msleep(10);
>>>> +	} while (n_tries <= N_WAKEUP_TRIES_SHORT_INTERVAL + N_WAKEUP_TRIES_LONG_INTERVAL &&
>>>> +		 READ_ONCE(adapter->int_status) == 0);
>>>
>>> Can't you use read_poll_timeout() twice instead of this custom approach?
>>
>> I've tried this now, but read_poll_timeout() is not ideal for our use-case.
>> What we'd need would be read->sleep->poll->repeat instead of
>> read->poll->sleep->repeat. With read_poll_timeout() we always end up doing
>> one more (unnecessary) write.
> 
> First of all, there is a parameter to get sleep beforehand.

Sleeping beforehand will sleep before doing the first write, so that's 
just wasted time.

> Second, what is the problem with having one write more or less?
> Your current code doesn't guarantee this either. It only decreases
> probability of such scenario. Am I wrong?
> 
> 

Indeed my approach just decreases the probability and we sometimes end 
up writing twice to wakeup the card, but it would kinda bug me if we'd 
always do one write too much.

Anyway, if you still prefer the read_poll_timeout() solution I'd be 
alright with that of course.

Re: [PATCH v2 2/2] mwifiex: Try waking the firmware until we get an interrupt

[Andy Shevchenko]

On Thu, Sep 30, 2021 at 11:07:09PM +0200, Jonas Dreßler wrote:
> On 9/30/21 10:58 PM, Andy Shevchenko wrote:
> > On Thu, Sep 30, 2021 at 08:04:00PM +0200, Jonas Dreßler wrote:

...

> > Second, what is the problem with having one write more or less?
> > Your current code doesn't guarantee this either. It only decreases
> > probability of such scenario. Am I wrong?
> 
> Indeed my approach just decreases the probability and we sometimes end up
> writing twice to wakeup the card, but it would kinda bug me if we'd always
> do one write too much.
> 
> Anyway, if you still prefer the read_poll_timeout() solution I'd be alright
> with that of course.

Yes, it will make code cleaner.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 2/2] mwifiex: Try waking the firmware until we get an interrupt

[Jonas Dreßler]

On 9/14/21 13:48, Jonas Dreßler wrote:
> It seems that the firmware of the 88W8897 card sometimes ignores or
> misses when we try to wake it up by writing to the firmware status
> register. This leads to the firmware wakeup timeout expiring and the
> driver resetting the card because we assume the firmware has hung up or
> crashed (unfortunately that's not unlikely with this card).
> 
> Turns out that most of the time the firmware actually didn't hang up,
> but simply "missed" our wakeup request and didn't send us an AWAKE
> event.
> 
> Trying again to read the firmware status register after a short timeout
> usually makes the firmware wake up as expected, so add a small retry
> loop to mwifiex_pm_wakeup_card() that looks at the interrupt status to
> check whether the card woke up.
> 
> The number of tries and timeout lengths for this were determined
> experimentally: The firmware usually takes about 500 us to wake up
> after we attempt to read the status register. In some cases where the
> firmware is very busy (for example while doing a bluetooth scan) it
> might even miss our requests for multiple milliseconds, which is why
> after 15 tries the waiting time gets increased to 10 ms. The maximum
> number of tries it took to wake the firmware when testing this was
> around 20, so a maximum number of 50 tries should give us plenty of
> safety margin.
> 
> A good reproducer for this issue is letting the firmware sleep and wake
> up in very short intervals, for example by pinging a device on the
> network every 0.1 seconds.
> 
> Cc: stable@vger.kernel.org
> Signed-off-by: Jonas Dreßler <verdre@v0yd.nl>
> ---
>   drivers/net/wireless/marvell/mwifiex/pcie.c | 33 +++++++++++++++++----
>   1 file changed, 27 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/net/wireless/marvell/mwifiex/pcie.c b/drivers/net/wireless/marvell/mwifiex/pcie.c
> index 0eff717ac5fa..7fea319e013c 100644
> --- a/drivers/net/wireless/marvell/mwifiex/pcie.c
> +++ b/drivers/net/wireless/marvell/mwifiex/pcie.c
> @@ -661,11 +661,15 @@ static void mwifiex_delay_for_sleep_cookie(struct mwifiex_adapter *adapter,
>   			    "max count reached while accessing sleep cookie\n");
>   }
>   
> +#define N_WAKEUP_TRIES_SHORT_INTERVAL 15
> +#define N_WAKEUP_TRIES_LONG_INTERVAL 35
> +
>   /* This function wakes up the card by reading fw_status register. */
>   static int mwifiex_pm_wakeup_card(struct mwifiex_adapter *adapter)
>   {
>   	struct pcie_service_card *card = adapter->card;
>   	const struct mwifiex_pcie_card_reg *reg = card->pcie.reg;
> +	int n_tries = 0;
>   
>   	mwifiex_dbg(adapter, EVENT,
>   		    "event: Wakeup device...\n");
> @@ -673,12 +677,29 @@ static int mwifiex_pm_wakeup_card(struct mwifiex_adapter *adapter)
>   	if (reg->sleep_cookie)
>   		mwifiex_pcie_dev_wakeup_delay(adapter);
>   
> -	/* Accessing fw_status register will wakeup device */
> -	if (mwifiex_write_reg(adapter, reg->fw_status, FIRMWARE_READY_PCIE)) {
> -		mwifiex_dbg(adapter, ERROR,
> -			    "Writing fw_status register failed\n");
> -		return -1;
> -	}
> +	/* Access the fw_status register to wake up the device.
> +	 * Since the 88W8897 firmware sometimes appears to ignore or miss
> +	 * that wakeup request, we continue trying until we receive an
> +	 * interrupt from the card.
> +	 */
> +	do {
> +		if (mwifiex_write_reg(adapter, reg->fw_status, FIRMWARE_READY_PCIE)) {
> +			mwifiex_dbg(adapter, ERROR,
> +				    "Writing fw_status register failed\n");
> +			return -EIO;
> +		}
> +
> +		n_tries++;
> +
> +		if (n_tries <= N_WAKEUP_TRIES_SHORT_INTERVAL)
> +			usleep_range(400, 700);
> +		else
> +			msleep(10);
> +	} while (n_tries <= N_WAKEUP_TRIES_SHORT_INTERVAL + N_WAKEUP_TRIES_LONG_INTERVAL &&
> +		 READ_ONCE(adapter->int_status) == 0);
> +
> +	mwifiex_dbg(adapter, EVENT,
> +		    "event: Tried %d times until firmware woke up\n", n_tries);
>   
>   	if (reg->sleep_cookie) {
>   		mwifiex_pcie_dev_wakeup_delay(adapter);
> 

So I think I have another solution that might be a lot more elegant, how
about this:

try_again:
	n_tries++;

	mwifiex_write_reg(adapter, reg->fw_status, FIRMWARE_READY_PCIE);

	if (wait_event_interruptible_timeout(adapter->card_wakeup_wait_q,
					     READ_ONCE(adapter->int_status) != 0,
					     WAKEUP_TRY_AGAIN_TIMEOUT) == 0 &&
	    n_tries < MAX_N_WAKEUP_TRIES) {
		goto try_again;
	}


and then call wake_up_interruptible() in the mwifiex_interrupt_status()
interrupt handler.

This solution should make sure we always keep wakeup latency to a minimum
and can still retry the register write if things didn't work.

Re: [PATCH v2 2/2] mwifiex: Try waking the firmware until we get an interrupt

[Brian Norris]

Hi,

On Sun, Oct 3, 2021 at 2:18 AM Jonas Dreßler <verdre@v0yd.nl> wrote:
> So I think I have another solution that might be a lot more elegant, how
> about this:
>
> try_again:
>         n_tries++;
>
>         mwifiex_write_reg(adapter, reg->fw_status, FIRMWARE_READY_PCIE);
>
>         if (wait_event_interruptible_timeout(adapter->card_wakeup_wait_q,
>                                              READ_ONCE(adapter->int_status) != 0,
>                                              WAKEUP_TRY_AGAIN_TIMEOUT) == 0 &&
>             n_tries < MAX_N_WAKEUP_TRIES) {
>                 goto try_again;
>         }

Isn't wait_event_interruptible_timeout()'s timeout in jiffies, which
is not necessarily that predictable, and also a lot more
coarse-grained than we want? (As in, if HZ=100, we're looking at
precision on the order of 10ms, whereas the expected wakeup latency is
~6ms.) That would be OK for well-behaved PCI cases, where we never
miss a write, but it could ~double your latency for your bad systems
that will need more than one run of the loop.

Also, feels like a do/while could be cleaner, but that's a lesser detail.

> and then call wake_up_interruptible() in the mwifiex_interrupt_status()
> interrupt handler.
>
> This solution should make sure we always keep wakeup latency to a minimum
> and can still retry the register write if things didn't work.

Brian

Re: [PATCH v2 0/2] mwifiex: Work around firmware bugs on 88W8897 chip

[Brian Norris]

On Tue, Sep 14, 2021 at 4:48 AM Jonas Dreßler <verdre@v0yd.nl> wrote:
>
> This is the second revision of the patch, the first one is here:
> https://lore.kernel.org/linux-wireless/20210830123704.221494-1-verdre@v0yd.nl/
>
> Changes between v1 and v2:
>  - Only read-back the register write to the TX ring write pointer, not all writes
>  - Mention firmware version in commit message+code comment for future reference
>  - Use -EIO return value in second patch
>  - Use definitions for waiting intervals in second patch

I tested this version, and it doesn't have the same issues v1 had
(regarding long-blocking reads, causing audio dropouts, etc.), so:

Tested-by: Brian Norris <briannorris@chromium.org>

As suggested elsewhere, this polling loop approach is a little slower
than just waiting for an interrupt instead (and that proves out; the
wakeup latency seems to increase by ~1 "short" polling interval; so
about half a millisecond). It seems like that could be optimized if
needed, because you *are* still waiting for an interrupt anyway. But I
haven't tried benchmarking anything that would really matter for this;
when we're already waiting 6+ ms, another 0.5ms isn't the end of the
world.

This doesn't really count as Reviewed-by. There are probably better
improvements to the poling loop (e.g., Andy's existing suggestions);
and frankly, I'd rather see if the dropped writes can be fixed
somehow. But I'm not holding my breath for the latter, and don't have
any good suggestions. So if this is the best we can do, so be it.

Regards,
Brian