Re: [PATCH 3/3] brcmfmac: make setting SDIO workqueue WQ_HIGHPRI a module parameter

[Arend Van Spriel <arend.vanspriel@broadcom.com>]

On 3/20/2020 9:08 AM, Wright Feng wrote:
> 
> 
> Arend Van Spriel 於 3/19/2020 4:55 PM 寫道:
>> + Tejun - regarding alloc_workqueue usage
>>
>> On 3/19/2020 8:53 AM, Wright Feng wrote:
>>> With setting sdio_wq_highpri=1 in module parameters, tasks submitted to
>>> SDIO workqueue will put at the head of the queue and run immediately.
>>> This parameter is for getting higher TX/RX throughput with SDIO bus.
>>>
>>> Signed-off-by: Wright Feng <wright.feng@cypress.com>
>>> Signed-off-by: Chi-hsien Lin <chi-hsien.lin@cypress.com>
>>> ---
>>>   .../wireless/broadcom/brcm80211/brcmfmac/common.c  |  5 +++++
>>>   .../wireless/broadcom/brcm80211/brcmfmac/common.h  |  2 ++
>>>   .../wireless/broadcom/brcm80211/brcmfmac/sdio.c    | 22 
>>> ++++++++++++++--------
>>>   3 files changed, 21 insertions(+), 8 deletions(-)
>>>
>>
>> [...]
>>
>>> diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c 
>>> b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c
>>> index 3a08252..885e8bd 100644
>>> --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c
>>> +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c
>>> @@ -4342,9 +4342,21 @@ struct brcmf_sdio *brcmf_sdio_probe(struct 
>>> brcmf_sdio_dev *sdiodev)
>>>       bus->txminmax = BRCMF_TXMINMAX;
>>>       bus->tx_seq = SDPCM_SEQ_WRAP - 1;
>>> +    /* attempt to attach to the dongle */
>>> +    if (!(brcmf_sdio_probe_attach(bus))) {
>>> +        brcmf_err("brcmf_sdio_probe_attach failed\n");
>>> +        goto fail;
>>> +    }
>>> +
>>>       /* single-threaded workqueue */
>>> -    wq = alloc_ordered_workqueue("brcmf_wq/%s", WQ_MEM_RECLAIM,
>>> -                     dev_name(&sdiodev->func1->dev));
>>> +    if (sdiodev->settings->sdio_wq_highpri) {
>>> +        wq = alloc_workqueue("brcmf_wq/%s",
>>> +                     WQ_HIGHPRI | WQ_MEM_RECLAIM | WQ_UNBOUND,
>>> +                     1, dev_name(&sdiodev->func1->dev));
>>
>> So two things changed, 1) WQ_HIGHPRI flag added *and* 2) use 
>> allow_workqueue basically dropping __WQ_ORDERED. Not sure which one 
>> contributes to the behavior described in the commit message.
> 
> The combination of Unbound and max_active==1 implies ordered, so I 
> suppose we don't need to set __WQ_ORDERED bit in flags.

My reason for asking was the idea to only determine flags in the 
if-statement and have following by one alloc_wq call, ie.:

wq_flags = WQ_MEM_RECLAIM;
if (sdio_wq_highpri)
	wq_flags |= WQ_HIGHPRI
wq = alloc_ordered_workqueue(..., wq_flags,...);

Regards,
Arend