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
Hi Michael,
On 4/3/22 23:51, Michael Straube wrote:
>>
>> 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) {
^^^^
btw, this part is useless to
> while (pwrpriv->bInSuspend &&
I've looked into what gcc11 produced from this function and looks like
my compiler is smart enough to not cache that value, but I am afraid not
all compilers are that smart.
And looks like it will be better to wait on mutex_lock(&pwrpriv->lock);
rather than odd loops. Ah, we can't wait here...
In first place, why this function cares about usb suspend callback?
I've got too many questions to that code... I'd better stop
> (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.
>
Yeah, and unfortunately it won't be optimized out :(
> Even worse the comment seems misleading because pwrpriv->bInSuspend
> indicates usb autosuspend but not system suspend.
>
With regards,
Pavel Skripkin
From: Pavel Skripkin
> Sent: 03 April 2022 22:15
>
> Hi Michael,
>
> On 4/3/22 23:51, Michael Straube wrote:
> >>
> >> 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) {
>
> ^^^^
>
> btw, this part is useless to
>
>
> > while (pwrpriv->bInSuspend &&
>
> I've looked into what gcc11 produced from this function and looks like
> my compiler is smart enough to not cache that value, but I am afraid not
> all compilers are that smart.
The compiler can't cache the value because of the function call.
Quite whether the code is in any way sane in another matter.
You definitely cannot sleep with a spinlock held.
Imagine what happens if another process tries to acquire the
spinlock while you are sleeping.
It will spin forever.
David
-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)