LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
From: "Jonas Dreßler" <verdre@v0yd.nl>
To: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Cc: "Amitkumar Karwar" <amitkarwar@gmail.com>,
	"Ganapathi Bhat" <ganapathi017@gmail.com>,
	"Xinming Hu" <huxinming820@gmail.com>,
	"Kalle Valo" <kvalo@codeaurora.org>,
	"David S. Miller" <davem@davemloft.net>,
	"Jakub Kicinski" <kuba@kernel.org>,
	"Tsuchiya Yuto" <kitakar@gmail.com>,
	linux-wireless@vger.kernel.org, netdev@vger.kernel.org,
	linux-kernel@vger.kernel.org, linux-pci@vger.kernel.org,
	"Maximilian Luz" <luzmaximilian@gmail.com>,
	"Bjorn Helgaas" <bhelgaas@google.com>,
	"Pali Rohár" <pali@kernel.org>,
	"Heiner Kallweit" <hkallweit1@gmail.com>,
	"Johannes Berg" <johannes@sipsolutions.net>,
	"Brian Norris" <briannorris@chromium.org>,
	stable@vger.kernel.org
Subject: Re: [PATCH v2 1/2] mwifiex: Use non-posted PCI write when setting TX ring write pointer
Date: Wed, 22 Sep 2021 14:08:39 +0200	[thread overview]
Message-ID: <9293504f-f70d-61ac-b221-dd466f01b5df@v0yd.nl> (raw)
In-Reply-To: <YUsQ3jU1RuThUYn8@smile.fi.intel.com>

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.

  reply	other threads:[~2021-09-22 12:08 UTC|newest]

Thread overview: 19+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-09-14 11:48 [PATCH v2 0/2] mwifiex: Work around firmware bugs on 88W8897 chip Jonas Dreßler
2021-09-14 11:48 ` [PATCH v2 1/2] mwifiex: Use non-posted PCI write when setting TX ring write pointer Jonas Dreßler
2021-09-22 11:17   ` Andy Shevchenko
2021-09-22 12:08     ` Jonas Dreßler [this message]
2021-09-22 13:22       ` Andy Shevchenko
2021-09-22 14:03   ` David Laight
2021-09-22 14:27     ` Pali Rohár
2021-09-22 15:54       ` David Laight
2021-09-30 14:27         ` Jonas Dreßler
2021-10-06 16:01           ` Jonas Dreßler
2021-09-14 11:48 ` [PATCH v2 2/2] mwifiex: Try waking the firmware until we get an interrupt Jonas Dreßler
2021-09-22 11:19   ` Andy Shevchenko
2021-09-30 18:04     ` Jonas Dreßler
2021-09-30 20:58       ` Andy Shevchenko
2021-09-30 21:07         ` Jonas Dreßler
2021-09-30 21:16           ` Andy Shevchenko
2021-10-03  9:18   ` Jonas Dreßler
2021-10-04 17:52     ` Brian Norris
2021-09-27 20:30 ` [PATCH v2 0/2] mwifiex: Work around firmware bugs on 88W8897 chip Brian Norris

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=9293504f-f70d-61ac-b221-dd466f01b5df@v0yd.nl \
    --to=verdre@v0yd.nl \
    --cc=amitkarwar@gmail.com \
    --cc=andriy.shevchenko@linux.intel.com \
    --cc=bhelgaas@google.com \
    --cc=briannorris@chromium.org \
    --cc=davem@davemloft.net \
    --cc=ganapathi017@gmail.com \
    --cc=hkallweit1@gmail.com \
    --cc=huxinming820@gmail.com \
    --cc=johannes@sipsolutions.net \
    --cc=kitakar@gmail.com \
    --cc=kuba@kernel.org \
    --cc=kvalo@codeaurora.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-pci@vger.kernel.org \
    --cc=linux-wireless@vger.kernel.org \
    --cc=luzmaximilian@gmail.com \
    --cc=netdev@vger.kernel.org \
    --cc=pali@kernel.org \
    --cc=stable@vger.kernel.org \
    --subject='Re: [PATCH v2 1/2] mwifiex: Use non-posted PCI write when setting TX ring write pointer' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).