Re: staging: r8188eu: how to handle nested mutex under spinlock

[Michael Straube <straube.linux@gmail.com>]

On 4/3/22 15:02, Pavel Skripkin wrote:
> Hi Fabio,
> 
> On 4/3/22 15:55, Fabio M. De Francesco wrote:
>> On domenica 3 aprile 2022 14:45:49 CEST Pavel Skripkin wrote:
>>> Hi Fabio,
>>>
>>> On 4/3/22 15:37, Fabio M. De Francesco wrote:
>>> >> > >> > drivers/staging/r8188eu/core/rtw_pwrctrl.c:379
>>> >> > >> >        if (pwrpriv->ps_processing) {
>>> >> >            while (pwrpriv->ps_processing && 
>>> rtw_get_passing_time_ms(start) <= 3000)
>>> >> >                msleep(10);
>>> >> >        }
>>> >> > >> >> Hm, just wondering, shouldn't we annotate load from >> 
>>> pwrpriv->ps_processing with READ_ONCE() inside while loop?
>>> >> IIUC compiler might want to cache first load into register and we 
>>> will >> stuck here forever.
>>> > > You're right. This can be cached. In situations like these one 
>>> should use
>>> > barriers or other API that use barriers implicitly (completions, 
>>> for example).
>>> >
>>> Not sure about completions, since they may sleep.
>>
>> No completions in this special context. They for _sure_ might sleep. I 
>> was
>> talking about general cases when you are in a loop and wait for status 
>> change.
>>
>>>
>>> Also, don't think that barriers are needed here, since this code just 
>>> waiting for observing value 1. Might be barrier will slightly speed 
>>> up waiting thread, but will also slow down other thread
>>
>> Here, I cannot help with a 100% good answer. Maybe Greg wants to say 
>> something
>> about it?
>>
> 
> IMO, the best answer is just remove this loop, since it does nothing. Or 
> redesign it to be more sane
> 
> It waits for ps_processing to become 0 for 3000 ms, but if 3000 ms 
> expires... execution goes forward like as ps_processing was 0 from the 
> beginning
> 
> Maybe it's something hw related, like wait for 3000 ms and all will be 
> ok. Can't say...
> 

Hi Pavel,

same with the loop that follows:

	/* System suspend is not allowed to wakeup */
	if (pwrpriv->bInSuspend) {
		while (pwrpriv->bInSuspend &&
		       (rtw_get_passing_time_ms(start) <= 3000 ||
		       (rtw_get_passing_time_ms(start) <= 500)))
				msleep(10);
	}

I just waits 500ms if pwrpriv->bInSuspend is true. Additionaly the
<= 3000 has no effect here because of the ored <= 500.

Even worse the comment seems misleading because pwrpriv->bInSuspend 
indicates usb autosuspend but not system suspend.

regards,
Michael