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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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




[Index of Archives]     [Linux Driver Development]     [Linux Driver Backports]     [DMA Engine]     [Linux GPIO]     [Linux SPI]     [Video for Linux]     [Linux USB Devel]     [Linux Coverity]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [Yosemite Backpacking]
  Powered by Linux