netdev - Re: [PATCH] wifi: wilc1000: Rework bus locking

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <880baad9-be3d-41b2-bea3-620f915ca397@microchip.com>
Date: Wed, 23 Oct 2024 17:54:56 +0000
From: <Ajay.Kathat@...rochip.com>
To: <marex@...x.de>, <alexis.lothore@...tlin.com>,
	<linux-wireless@...r.kernel.org>
CC: <davem@...emloft.net>, <adham.abozaeid@...rochip.com>,
	<claudiu.beznea@...on.dev>, <conor+dt@...nel.org>, <edumazet@...gle.com>,
	<kuba@...nel.org>, <kvalo@...nel.org>, <krzk+dt@...nel.org>,
	<pabeni@...hat.com>, <robh@...nel.org>, <devicetree@...r.kernel.org>,
	<netdev@...r.kernel.org>
Subject: Re: [PATCH] wifi: wilc1000: Rework bus locking

Hello Marek,

On 10/23/24 07:44, Marek Vasut wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the
> content is safe
> 
> On 10/23/24 9:17 AM, Ajay.Kathat@...rochip.com wrote:
> 
> Hello Ajay,
> 
>>> What I am trying to say is this:
>>>
>>> With current code, this can happen, which is not good, because transfers
>>> from multiple threads can be interleaved and interfere with each other:
>>
>> Did you observe any bus failure in your test setup with SDIO. Is there any
>> configure to recreate it.
> 
> I am observing sporadic command and data CRC errors on STM32MP157F
> system with SDIO WILC3000.

Does this patch help to resolve the CRC errors?

Do you observe the CRC errors during the bulk data transfer(iPerf) or does it
even happen during the basic WiFi operation like(scan/connect or basic ping
operation).
Is power-save enabled during the test. With PS enabled, The SDIO commands may
fail momentarily but it should recover.

> 
>> The SDIO transfer may appear to be split into into multiple transaction but
>> these calls should not impact each other since most of them are updating the
>> different registers except WILC_SDIO_FBR_CSA_REG register, which is used for
>> CSA read/write. If needed, wilc_sdio_set_func0_csa_address() can be modified
>> to club the 3x CMD52 and 1x CMD53 into a single transfer API.
>>
>> In my opinion, If sdio_claim_host() is moved to acquire_bus() API then the
>> SDIO bus will be claimed longer than required especially in
>> wilc_wlan_handle_txq() API. Ideally, calling sdio_claim_host() call just
>> before the transfer is enough but now the SDIO I/O bus would be continuously
>> blocked for multiple independent SDIO transactions that is not necessary.
> 
> Why would that pose a problem ?

wilc_wlan_handle_txq() performs many operations on different registers which
are not related. It will block the SDIO bux for longer. Otherwise those
registers are allowed to update separately from the WILC SD side.

> 
> I am more concerned that ksdioirqd can insert a command transfer right
> in the middle of CMD52/CMD53 register read composite transfer, because
> while ksdioirqd does use proper sdio_claim/release_host, this driver
> does it per-SDIO-command instead of per the whole e.g. register read
> "transaction".
> 

I think, using sdio_claim/release for each-SDIO command should suffice because
the CMD52/CMD53 modify the specific registers that are unrelated. Each
CMD52/53 should work properly and independently provided they are protected
with sdio_claim/release.
Additionally, there is no WILC SD module limitation requiring a strict order
for CMD52/CMD53, except for Card Storage Area (CSA) read/write operations.

For CSA read/write operations, which are necessary to read/write any specific
address from the card, multiple CMD52 commands are used to pass the desired
address to be read/written using registers (0x10c, 0x10d, 0x10e). Then, CMD53
is used to read/write to address 0x10f (CSA data window register). This
complete command sequence is required for a single CSA address read/write.
Based on this requirement, CSA read/write operations can cause issues if they
run in parallel with another CSA read/write operation, but not with other
CMD52 and CMD53 commands.
Therefore, one approach to resolve this issue is to add sdio_claim/release
around wilc_sdio_set_func0_csa_address(). This way, this function will be
treated as a single operation and it will only modify the required command flow.

Regards,
Ajay