Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <942c0fbd-f8b2-4cae-dd21-79bc55c54902@gmail.com>
Date:   Sun, 3 Apr 2022 22:51:39 +0200
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101
 Thunderbird/91.7.0
Subject: Re: staging: r8188eu: how to handle nested mutex under spinlock
Content-Language: en-US
To:     Pavel Skripkin <paskripkin@gmail.com>,
        "Fabio M. De Francesco" <fmdefrancesco@gmail.com>
Cc:     Greg KH <gregkh@linuxfoundation.org>,
        Larry Finger <Larry.Finger@lwfinger.net>,
        Phillip Potter <phil@philpotter.co.uk>,
        "open list:STAGING SUBSYSTEM" <linux-staging@lists.linux.dev>,
        Linux Kernel Mailing List <linux-kernel@vger.kernel.org>
References: <356c24cf-625b-eea2-2c04-ce132d881cac@gmail.com>
 <4412825.cEBGB3zze1@leap> <26ac4c2d-91cf-656d-2b7e-21a95e500e70@gmail.com>
 <2029549.KlZ2vcFHjT@leap> <7d3d23c3-1839-3e6a-27bf-85bad384e5e4@gmail.com>
From:   Michael Straube <straube.linux@gmail.com>
In-Reply-To: <7d3d23c3-1839-3e6a-27bf-85bad384e5e4@gmail.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit
Precedence: bulk

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