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
