Message-ID: <9d20b408-72a4-49f0-aca6-108dfdd65f99@denx.de>
Date: Wed, 23 Oct 2024 20:47:46 +0200
From: Marek Vasut <marex@...x.de>
To: Ajay.Kathat@...rochip.com, 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

On 10/23/24 7:54 PM, Ajay.Kathat@...rochip.com wrote:
> Hello Marek,

Hi,

>>>> 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?

No

> 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).

I can trigger them during this test:

$ while true ; ifconfig wlan0 up ; ifconfig wlan0 down ; done

But they happen sporadically and seemingly at random.

I already stuffed the driver with trace_printk()s through and through, 
but the trace log does not indicate anything that would be off in any way.

> Is power-save enabled during the test. With PS enabled, The SDIO commands may
> fail momentarily but it should recover.

It seems it gets enabled after first ifconfig up, that's a good hint, 
I'll try to disable it and see if that makes them errors go away. Thanks!

Do you have any details on WHY would such sporadic errors occur and how 
to make those go away even with PS enabled ?

>>> 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.

And is that blocking of SD bus actually a problem ?

>> 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.

Can multiple register IO operations like that be interleaved with 
ksdioirqd access to SDIO_CCCR_INTx too ?

Take e.g. wilc_wlan_handle_txq(), is the following order legal?

thread1                                | thread2
ret = func->hif_write_reg(wilc,        |
            WILC_CORTUS_INTERRUPT_BASE, |
            1);                         |
                                        | ksdioirqd {
                                        | ret = mmc_io_rw_direct(card, 0,
                                        | 0, SDIO_CCCR_INTx, 0, pending);
                                        | }
ret = func->hif_read_reg(wilc,         |
            WILC_CORTUS_INTERRUPT_BASE, |
            &reg);                      |

> 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.

Unrelated to this, but ... is it possible to send the entire 3-Byte 
register CSA address using a single command instead of 3 separate 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.

I can do the serialization per-command, but please see above.

Powered by blists - more mailing lists