linux-kernel - Re: staging: r8188eu: how to handle nested mutex under spinlock

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <942c0fbd-f8b2-4cae-dd21-79bc55c54902@gmail.com>
Date:   Sun, 3 Apr 2022 22:51:39 +0200
From:   Michael Straube <straube.linux@...il.com>
To:     Pavel Skripkin <paskripkin@...il.com>,
        "Fabio M. De Francesco" <fmdefrancesco@...il.com>
Cc:     Greg KH <gregkh@...uxfoundation.org>,
        Larry Finger <Larry.Finger@...inger.net>,
        Phillip Potter <phil@...lpotter.co.uk>,
        "open list:STAGING SUBSYSTEM" <linux-staging@...ts.linux.dev>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>
Subject: Re: staging: r8188eu: how to handle nested mutex under spinlock

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