Messages in this thread | | | Date | Sun, 3 Apr 2022 22:51:39 +0200 | Subject | Re: staging: r8188eu: how to handle nested mutex under spinlock | From | Michael Straube <> |
| |
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
| |