Hi,
On 10/22/24 06:19, Marek Vasut wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the
> content is safe
>
> On 10/22/24 12:43 PM, Alexis Lothoré wrote:
>> Hi Marek,
>
> Hi,
>
>> On 10/22/24 03:38, Marek Vasut wrote:
>>> The bus locking in this driver is broken and produces subtle race
>>> condition with ksdioirqd and its mmc_claim_host()/mmc_release_host()
>>> usage in case of SDIO bus. Rework the locking to avoid this race
>>> condition.
>>>
>>> The problem is the hif_cs mutex used in acquire_bus()/release_bus(),
>>> which makes it look like calling acquire_bus() results in exclusive
>>> access to the bus, but that is not true for SDIO bus. For SDIO bus,
>>> to obtain exclusive access (any access, really), it is necessary to
>>> call sdio_claim_host(), which is a wrapper around mmc_claim_host(),
>>> which does its own locking. The acquire_bus() does not do that, but
>>> the SDIO interface implementation does call sdio_claim_host() and
>>> sdio_release_host() every single command, which is problematic. To
>>> make things worse, wilc_sdio_interrupt() implementation called from
>>> ksdioirqd first calls sdio_release_host(), then interrupt handling
>>> and finally sdio_claim_host().
>>>
>>> The core problem is that sdio_claim_host() cannot be done per command,
>>> but has to be done per register/data IO which consists of multiple
>>> commands.
>>
>> Is it really true ? What makes you say that we can not perform multiple
>> operations under the same exclusive sdio section ?
>
> 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.
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.
>
> thread 1 thread2
> do_some_higher_level_op() {
> ...
> read_register_0x3b0000() {
> claim_bus
> CMD52 0x00
> release bus ksdioirqd() {
> claim_bus
> CMD52 0x0f, lets read SDIO_CCCR_INTx
> release_bus
> claim bus }
> CMD52 0x00
> release_bus
> claim_bus
> CMD52 0x3b
> release_bus
> claim_bus
> CMD53 lets read data
> release_bus
> }
> ...
> }
>
> What should happen is either:
>
> thread 1 thread2
> ksdioirqd() { // option 1
> claim_bus
> CMD52 0x0f, lets read SDIO_CCCR_INTx
> release_bus
> }
> do_some_higher_level_op() {
> claim_bus
> ...
> read_register_0x3b0000 {
> CMD52 0x00
> CMD52 0x00
> CMD52 0x3b
> CMD53 lets read data
> }
> ...
> read_another_register()
> ...
> release_bus
> }
> ksdioirqd() { // option 2
> claim_bus
> CMD52 0x0f, lets read SDIO_CCCR_INTx
> release_bus
> }
>
> That's what this patch implements, to avoid the interference.
>
> Maybe I should include the infographics? Or reword this somehow?
>
>> Usually the WILC register read/write consists of 3x CMD52
>>> to push in CSA pointer address and 1x CMD53 to read/write data to that
>>> address. Most other accesses are also composed of multiple commands.
>>>
>>> Currently, if ksdioirqd wakes up and attempts to read SDIO_CCCR_INTx
>>> to get pending SDIO IRQs in sdio_get_pending_irqs(), it can easily
>>> perform that transfer between two consecutive CMD52 which are pushing
>>> in the CSA pointer address and possibly disrupt the WILC operation.
>>> This is undesired behavior.
>>
>> I agree about the observation, and then I disagree about the statement above on
>> sdio_claim_host/sdio_release_host not meant to be used for multiple commands.
>
> I think we have a misunderstanding here, see above.
>
>> I see plenty of sdio wireless drivers performing multiple sdio operations under
>> the same sdio exclusive bus access section, either explicitely in their
>> code, or
>> through a sdio dedicated helper (eg: sdio_enable_func, sdio_disable_func).
>>
>> But more concerns below
>>>
>>> Rework the locking.
>>>
>>> Introduce new .hif_claim/.hif_release callbacks which implement bus
>>> specific locking. Lock/unlock SDIO bus access using sdio_claim_host()
>>> and sdio_release_host(), lock/unlock SPI bus access using the current
>>> hif_cs mutex moved purely into the spi.c interface. Make acquire_bus()
>>> and release_bus() call the .hif_claim/.hif_release() callbacks and do
>>> not access the hif_cs mutex from there at all.
>>>
>>> Remove any SDIO bus locking used directly in commands and the broken
>>> SDIO bus unlocking in wilc_sdio_interrupt(), this is no longer needed.
>>> Fix up SDIO initialization code which newly needs sdio_claim_host()
>>> and sdio_release_host(), since it cannot depend on the locking being
>>> done per-command anymore.
>>>
>>> Signed-off-by: Marek Vasut <marex@xxxxxxx>
>>
>> [...]
>>
>>>
>>> -static void wilc_sdio_interrupt(struct sdio_func *func)
>>> +static void wilc_sdio_claim(struct wilc *wilc)
>>> +{
>>> + struct sdio_func *func = container_of(wilc->dev, struct sdio_func, dev);
>>> +
>>> + sdio_claim_host(func);
>>> +}
>>> +
>>> +static void wilc_sdio_release(struct wilc *wilc)
>>> {
>>> + struct sdio_func *func = container_of(wilc->dev, struct sdio_func, dev);
>>> +
>>> sdio_release_host(func);
>>> +}
>>
>> So with this series, we end up with some bus-specific operations needing some
>> locking, but is now up to the upper layer to control this locking. This feels
>> wrong.
>
> It always was the upper layer (wlan.c) that attempted to do bus locking,
> except it was incomplete. The acquire_bus()/release_bus() primitives
> seems to be an attempt at serializing bus access across multiple
> operations (which really boils down to multiple SPI or SDIO commands).
>
> The problem is, acquire_bus()/release_bus() does not really work,
> because it does not prevent e.g. ksdioirqd from inserting a command on
> the SDIO bus. SDIO (or really, mmc framework) has its own way of doing
> bus locking, that's the sdio_claim_host()/sdio_release_host(), SPI does
> have spi_bus_lock()/spi_bus_unlock() which I should use in V2.
>
> Those sdio_claim_host()/sdio_release_host() and
> spi_bus_lock()/spi_bus_unlock() should be called in
> acquire_bus()/release_bus() to correctly serialize bus access across
> multiple operations. That will also eliminate the custom and not really
> functional hif_cs mutex.
>
>> The driver has a dedicated sdio layer, so if we need some locking for
>> sdio-specific operations, it should be handled autonomously by the sdio layer,
>> right ?
>
> Not quite, I don't think the intention was to let anything communicate
> with the WILC device within block "protected" by
> acquire_bus()/release_bus() pair. That's why I believe this is where bus
> lock and unlock should happen too.
>
>> [...]
>>
>>> static int wilc_wlan_cfg_commit(struct wilc_vif *vif, int type,
>>> diff --git a/drivers/net/wireless/microchip/wilc1000/wlan.h
>>> b/drivers/net/wireless/microchip/wilc1000/wlan.h
>>> index b9e7f9222eadd..ade2db95e8a0f 100644
>>> --- a/drivers/net/wireless/microchip/wilc1000/wlan.h
>>> +++ b/drivers/net/wireless/microchip/wilc1000/wlan.h
>>> @@ -403,6 +403,8 @@ struct wilc_hif_func {
>>> void (*disable_interrupt)(struct wilc *nic);
>>> int (*hif_reset)(struct wilc *wilc);
>>> bool (*hif_is_init)(struct wilc *wilc);
>>> + void (*hif_claim)(struct wilc *wilc);
>>> + void (*hif_release)(struct wilc *wilc);
>>
>> So IIUC, your series push the hif_cs lock into each bus layer of the driver,
>> remove any explicit call to bus-specific locking mechanism from those layers,
>> and makes the upper layer control the locking. As mentioned above, I don't
>> understand why those layers can not manage the bus-specific locking by
>> themselves (which would be a big win for the upper layer).
>
> Because of acquire_bus()/release_bus() which I think is an attempt to
> serialize bus access across multiple complex operations (=commands sent
> to the card), see above.
>
>> For SDIO specifically, I feel like letting the layer handle those claim/release
>> would even allow to remove this hif_cs mutex (but we may still need a lock for
>> SPI side)
>>
>> But I may be missing something, so feel free to prove me wrong.
> With spi_bus_lock()/unlock() we can actually dispose of the hif_cs mutex
> altogether.
