staging: r8188eu: how to handle nested mutex under spinlock

staging: r8188eu: how to handle nested mutex under spinlock

[Michael Straube]

Hi all,

smatch reported a sleeping in atomic context.

rtw_set_802_11_disassociate() <- disables preempt
-> _rtw_pwr_wakeup()
    -> ips_leave()

rtw_set_802_11_disassociate() takes a spinlock and ips_leave() uses a
mutex.

I'm fairly new to the locking stuff, but as far as I know this is not a
false positive since mutex can sleep, but that's not allowed under a
spinlock.

What is the best way to handle this?
I'm not sure if converting the mutex to a spinlock (including all the
other places where the mutex is used) is the right thing to do?

thanks,
Michael

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

[Pavel Skripkin]

Hi Michael,

On 4/2/22 23:47, Michael Straube wrote:
> Hi all,
> 
> smatch reported a sleeping in atomic context.
> 
> rtw_set_802_11_disassociate() <- disables preempt
> -> _rtw_pwr_wakeup()
>      -> ips_leave()
> 
> rtw_set_802_11_disassociate() takes a spinlock and ips_leave() uses a
> mutex.
> 
> I'm fairly new to the locking stuff, but as far as I know this is not a
> false positive since mutex can sleep, but that's not allowed under a
> spinlock.
> 
> What is the best way to handle this?
> I'm not sure if converting the mutex to a spinlock (including all the
> other places where the mutex is used) is the right thing to do?
> 

I've looked into this like a month ago.

IMO, this code just need to be redesigned, since locking scheme is very 
complicated there and, as smatch says, not correct.

Simple s/mutex_lock/spin_lock/ may work in that case, but one day 
locking scheme should be reworked... Or just some code parts should be 
dropped :))




With regards,
Pavel Skripkin

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

[Larry Finger]

On 4/2/22 15:47, Michael Straube wrote:
> Hi all,
> 
> smatch reported a sleeping in atomic context.
> 
> rtw_set_802_11_disassociate() <- disables preempt
> -> _rtw_pwr_wakeup()
>     -> ips_leave()
> 
> rtw_set_802_11_disassociate() takes a spinlock and ips_leave() uses a
> mutex.
> 
> I'm fairly new to the locking stuff, but as far as I know this is not a
> false positive since mutex can sleep, but that's not allowed under a
> spinlock.
> 
> What is the best way to handle this?
> I'm not sure if converting the mutex to a spinlock (including all the
> other places where the mutex is used) is the right thing to do?

In drivers/net/wireless/realtek/rtlwifi, we had a similar problem. There it was 
handled by putting the lps_enter() and lps_leave() operations in a separate 
workqueue. In this case, the routines were rtl_lps_enter() and rtl_lps_leave(). 
Each of them sets a variable to indicate whether enter_ps is true or false, and 
schedules the workqueue. In the workqueue's callback routine, the routines to 
start/stop ps mode are called. The code is in 
drivers/net/wireless/realtek/rtlwifi/ps.c.

This solution is only one of many, and there may be a better one.

Larry

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

[Michael Straube]

On 4/2/22 23:32, Larry Finger wrote:
> In drivers/net/wireless/realtek/rtlwifi, we had a similar problem. There 
> it was handled by putting the lps_enter() and lps_leave() operations in 
> a separate workqueue. In this case, the routines were rtl_lps_enter() 
> and rtl_lps_leave(). Each of them sets a variable to indicate whether 
> enter_ps is true or false, and schedules the workqueue. In the 
> workqueue's callback routine, the routines to start/stop ps mode are 
> called. The code is in drivers/net/wireless/realtek/rtlwifi/ps.c.
> 
> This solution is only one of many, and there may be a better one.
> 
> Larry
> 

Thank you for the explanation Larry.

Michael

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

[Michael Straube]

On 4/3/22 12:49, Fabio M. De Francesco wrote:
> On domenica 3 aprile 2022 12:43:04 CEST Fabio M. De Francesco wrote:
>> On sabato 2 aprile 2022 22:47:27 CEST Michael Straube wrote:
>>> Hi all,
>>>
>>> smatch reported a sleeping in atomic context.
>>>
>>> rtw_set_802_11_disassociate() <- disables preempt
>>> -> _rtw_pwr_wakeup()
>>>      -> ips_leave()
>>>
>>> rtw_set_802_11_disassociate() takes a spinlock and ips_leave() uses a
>>> mutex.
>>>
>>> I'm fairly new to the locking stuff, but as far as I know this is not a
>>> false positive since mutex can sleep, but that's not allowed under a
>>> spinlock.
>>>
>>> What is the best way to handle this?
>>> I'm not sure if converting the mutex to a spinlock (including all the
>>> other places where the mutex is used) is the right thing to do?
>>>
>>> thanks,
>>> Michael
>>>
>> Hi Michael,
>>
>> No, this is a false positive: ips_leave is never called under spinlocks.
>> Some time ago I blindly trusted Smatch and submitted a patch for what you
>> are reporting just now again. Soon after submission I realized it and
>> then I had to ask Greg to discard my patch.
>>
>> Please read the related thread:
>>
>> [PATCH] staging: r8188eu: Use kzalloc() with GFP_ATOMIC in atomic context
>> https://lore.kernel.org/lkml/20220206225943.7848-1-fmdefrancesco@gmail.com/
>>
>> Thanks,
>>
>> Fabio
> 
> I'm sorry, the correct link is the following:
> [PATCH v2 2/2] staging: r8188eu: Use kzalloc() with GFP_ATOMIC in atomic context
> https://lore.kernel.org/lkml/20220208180426.27455-3-fmdefrancesco@gmail.com/
> 
> Fabio
> 

Hi Fabio,

Ah I see now, thanks. Well, I think the code is not very clear and easy 
to follow here. Perhaps we should refactor this area someday to avoid 
future confusions.

regards,
Michael

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

[Michael Straube]

On 4/3/22 13:17, Fabio M. De Francesco wrote:
> On domenica 3 aprile 2022 13:08:35 CEST Michael Straube wrote:
>> On 4/3/22 12:49, Fabio M. De Francesco wrote:
>>> On domenica 3 aprile 2022 12:43:04 CEST Fabio M. De Francesco wrote:
>>>> On sabato 2 aprile 2022 22:47:27 CEST Michael Straube wrote:
>>>>> Hi all,
>>>>>
>>>>> smatch reported a sleeping in atomic context.
>>>>>
>>>>> rtw_set_802_11_disassociate() <- disables preempt
>>>>> -> _rtw_pwr_wakeup()
>>>>>       -> ips_leave()
>>>>>
>>>>> rtw_set_802_11_disassociate() takes a spinlock and ips_leave() uses a
>>>>> mutex.
>>>>>
>>>>> I'm fairly new to the locking stuff, but as far as I know this is not a
>>>>> false positive since mutex can sleep, but that's not allowed under a
>>>>> spinlock.
>>>>>
>>>>> What is the best way to handle this?
>>>>> I'm not sure if converting the mutex to a spinlock (including all the
>>>>> other places where the mutex is used) is the right thing to do?
>>>>>
>>>>> thanks,
>>>>> Michael
>>>>>
>>>> Hi Michael,
>>>>
>>>> No, this is a false positive: ips_leave is never called under spinlocks.
>>>> Some time ago I blindly trusted Smatch and submitted a patch for what you
>>>> are reporting just now again. Soon after submission I realized it and
>>>> then I had to ask Greg to discard my patch.
>>>>
>>>> Please read the related thread:
>>>>
>>>> [PATCH] staging: r8188eu: Use kzalloc() with GFP_ATOMIC in atomic context
>>>> https://lore.kernel.org/lkml/20220206225943.7848-1-fmdefrancesco@gmail.com/
>>>>
>>>> Thanks,
>>>>
>>>> Fabio
>>>
>>> I'm sorry, the correct link is the following:
>>> [PATCH v2 2/2] staging: r8188eu: Use kzalloc() with GFP_ATOMIC in atomic context
>>> https://lore.kernel.org/lkml/20220208180426.27455-3-fmdefrancesco@gmail.com/
>>>
>>> Fabio
>>>
>>
>> Hi Fabio,
>>
>> Ah I see now, thanks. Well, I think the code is not very clear and easy
>> to follow here. Perhaps we should refactor this area someday to avoid
>> future confusions.
>>
>> regards,
>> Michael
>>
> Soon after I sent the email above, I read yours anew. The issue I were trying
> to address were different than what you noticed today. I didn't even see that
> we were in nested mutexes under spinlocks and bottom halves disabled. I just
> saw those kmalloc() with GFP_KERNEL.
> 
> You are noticing something one layer up. And you are right, this is a real
> issue. Larry's suggestion is the only correct one for fixing this.
> 
> I've analyzed and reviewed some code in the tty layer that implements the
> same solution that Larry is talking about. Let me find the link and I'll
> soon send it to you, so that you might be inspired from that implementation.
> 
> Sorry for the confusion.
> 
> Thanks,
> 
> Fabio
> 
> 
> 

Hi Fabio,

wait..

rtw_set_802_11_disassociate() calls rtw_pwr_wakeup() only if
check_fwstate(pmlmepriv, _FW_LINKED) is true.


	if (check_fwstate(pmlmepriv, _FW_LINKED)) {
		rtw_disassoc_cmd(padapter, 0, true);
		rtw_indicate_disconnect(padapter);
		rtw_free_assoc_resources(padapter, 1);
		rtw_pwr_wakeup(padapter);
	}

in rtw_pwr_wakeup() there is the same check and if it is true the
function returns before calling ips_leave().

	if (check_fwstate(pmlmepriv, _FW_LINKED)) {
		ret = _SUCCESS;
		goto exit;
	}
	if (rf_off == pwrpriv->rf_pwrstate) {
		if (_FAIL ==  ips_leave(padapter)) {
			ret = _FAIL;
			goto exit;
		}
	}

So ips_leave() is not called in atomic context in this case and smatch
reports indeed a false positive, or am I wrong?

I have not checked the other calls to rtw_pwr_wakeup().

regards,
Michael

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

[Pavel Skripkin]

Hi Michael,

On 4/3/22 14:41, Michael Straube wrote:
> 
> Hi Fabio,
> 
> wait..
> 
> rtw_set_802_11_disassociate() calls rtw_pwr_wakeup() only if
> check_fwstate(pmlmepriv, _FW_LINKED) is true.
> 
> 
> 	if (check_fwstate(pmlmepriv, _FW_LINKED)) {
> 		rtw_disassoc_cmd(padapter, 0, true);
> 		rtw_indicate_disconnect(padapter);
> 		rtw_free_assoc_resources(padapter, 1);
> 		rtw_pwr_wakeup(padapter);
> 	}
> 

msleep() cannot be called in atomic context:

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);
  	}




With regards,
Pavel Skripkin

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

[Michael Straube]

On 4/3/22 13:56, Fabio M. De Francesco wrote:
> On domenica 3 aprile 2022 13:48:31 CEST Pavel Skripkin wrote:
>> Hi Michael,
>>
>> msleep() cannot be called in atomic context:
>>
>> 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);
>>    	}

Ah, just another issue...

> 
> I wanted to use mdelay() (allowed under spinlocks because it does not
> sleep) but Dan said to leave it as it is. You might easily find this
> discussion on this list. I cannot remember why Dan was against replacing
> msleep() with mdelay(). Please try to find that thread.
> 

I'll take a breath and look further into it later. Seems a solution like
the one Larry mentioned is reasonable here.

Thank you both,
Michael

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

[Pavel Skripkin]

On 4/3/22 14:48, Pavel Skripkin wrote:
> On 4/3/22 14:41, Michael Straube wrote:
>> 
>> Hi Fabio,
>> 
>> wait..
>> 
>> rtw_set_802_11_disassociate() calls rtw_pwr_wakeup() only if
>> check_fwstate(pmlmepriv, _FW_LINKED) is true.
>> 
>> 
>> 	if (check_fwstate(pmlmepriv, _FW_LINKED)) {
>> 		rtw_disassoc_cmd(padapter, 0, true);
>> 		rtw_indicate_disconnect(padapter);
>> 		rtw_free_assoc_resources(padapter, 1);
>> 		rtw_pwr_wakeup(padapter);
>> 	}
>> 
> 
> msleep() cannot be called in atomic context:
> 
> 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.


Am I missing something?



With regards,
Pavel Skripkin

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

[Pavel Skripkin]

[-- Attachment #1.1: Type: text/plain, Size: 1552 bytes --]

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.

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


> In this list, a couple of weeks ago, I suggested someone to use them. He/she
> re-submitted his/her patches with wait_for_completion() / complete(), I placed
> my Reviewed-by tag and Greg was happy to accept it and merge.
> 
> Not sure about READ_ONCE(), because, as far as I know, it is used only to
> prevent load tearing. But I might be wrong here.
> 
> Fabio
> 
> P.S.: Is Paul's book still un-read? Unfortunately, I am not yet done :(
> 

If I could read it 24/7 I will be the happiest person ever :) As you 
might expect I am not yet done as well




With regards,
Pavel Skripkin

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]

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

[Pavel Skripkin]

[-- Attachment #1.1: Type: text/plain, Size: 1807 bytes --]

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...




With regards,
Pavel Skripkin

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]

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

[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

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

[Pavel Skripkin]

[-- Attachment #1.1: Type: text/plain, Size: 1612 bytes --]

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

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]

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

[David Laight]

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)

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

[Pavel Skripkin]

[-- Attachment #1.1: Type: text/plain, Size: 812 bytes --]

Hi David,

On 4/4/22 11:50, David Laight wrote:
>> 
>> > 		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.
> 

Hm, I am a newbie in compilers, so can you, please, explain (or give a 
link to any resource where I can read about it) how function call here 
prevent caching.

IIUC compiler generates code that works well in scope of single-threaded 
application, so why can't compiler cache that value instead of accessing 
memory on each iteration... Isn't register access a way faster than even 
cache hit?


Thanks!

With regards,
Pavel Skripkin

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]

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

[David Laight]

From: Pavel Skripkin
> Sent: 04 April 2022 17:39
> 
> Hi David,
> 
> On 4/4/22 11:50, David Laight wrote:
> >>
> >> > 		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.
> >
> 
> Hm, I am a newbie in compilers, so can you, please, explain (or give a
> link to any resource where I can read about it) how function call here
> prevent caching.
> 
> IIUC compiler generates code that works well in scope of single-threaded
> application, so why can't compiler cache that value instead of accessing
> memory on each iteration... Isn't register access a way faster than even
> cache hit?

Because calls to external functions are allowed to change
any data via 'other' references.
For instance the structure pointer the function has could
also be in global data somewhere.

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)

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

[Pavel Skripkin]

[-- Attachment #1.1: Type: text/plain, Size: 1214 bytes --]

Hi David,

On 4/4/22 19:59, David Laight wrote:
> From: Pavel Skripkin
>> Sent: 04 April 2022 17:39
>> 
>> Hi David,
>> 
>> On 4/4/22 11:50, David Laight wrote:
>> >>
>> >> > 		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.
>> >
>> 
>> Hm, I am a newbie in compilers, so can you, please, explain (or give a
>> link to any resource where I can read about it) how function call here
>> prevent caching.
>> 
>> IIUC compiler generates code that works well in scope of single-threaded
>> application, so why can't compiler cache that value instead of accessing
>> memory on each iteration... Isn't register access a way faster than even
>> cache hit?
> 
> Because calls to external functions are allowed to change
> any data via 'other' references.
> For instance the structure pointer the function has could
> also be in global data somewhere.
> 

Make sense, thank you for explanation!




With regards,
Pavel Skripkin

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]

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

[Michael Straube]

On 4/3/22 14:10, Fabio M. De Francesco wrote:
> For a list of all the paths to a given function you may use Smatch:
> 
> ./smatch/smatch_data/db/smdb.py ips_leave
> 
> or
> 
> ./smatch/smatch_data/db/smdb.py call_tree ips_leave
> 
> But perhaps you already know how to do it.

Yes, but thank you anyway. :)

Michael

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

[Dan Carpenter]

On Sun, Apr 03, 2022 at 02:18:04PM +0200, Michael Straube wrote:
> On 4/3/22 14:10, Fabio M. De Francesco wrote:
> > For a list of all the paths to a given function you may use Smatch:
> > 
> > ./smatch/smatch_data/db/smdb.py ips_leave
> > 
> > or
> > 
> > ./smatch/smatch_data/db/smdb.py call_tree ips_leave
> > 
> > But perhaps you already know how to do it.
> 
> Yes, but thank you anyway. :)
> 

My email (gmail account) has been so weird recently.  I don't know why
I'm not getting Fabio's emails...  Presumably they will show up in a few
days.

The other command to use is:

$ ./smatch/smatch_data/db/smdb.py preempt ips_leave
rtw_set_802_11_disassociate() <- disables preempt
-> _rtw_pwr_wakeup()
   -> LeaveAllPowerSaveMode()
      -> ips_leave()

I save that to a file, open it with with vim and run a vim function
`hall` (for highlight all) from my .vimrc file.

" Use :hall to highlight all the words in a file (for debugging sleeping bugs)
function HLall()
  let a=[]
  %s/\w\+/\=add(a, submatch(0))/gn
  let @/ = join(a, "\\|")
endfunction
cnoreabbrev hall call HLall() <CR>:set hls<CR>

That highlights all the functions is the call tree, then I use cscope to
jump to rtw_set_802_11_disassociate and follow the highlighted functions
to ips_leave().

regards,
dan carpenter

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

[Michael Straube]

On 4/4/22 15:33, Dan Carpenter wrote:
> On Sun, Apr 03, 2022 at 02:18:04PM +0200, Michael Straube wrote:
>> On 4/3/22 14:10, Fabio M. De Francesco wrote:
>>> For a list of all the paths to a given function you may use Smatch:
>>>
>>> ./smatch/smatch_data/db/smdb.py ips_leave
>>>
>>> or
>>>
>>> ./smatch/smatch_data/db/smdb.py call_tree ips_leave
>>>
>>> But perhaps you already know how to do it.
>>
>> Yes, but thank you anyway. :)
>>
> 
> My email (gmail account) has been so weird recently.  I don't know why
> I'm not getting Fabio's emails...  Presumably they will show up in a few
> days.
> 
> The other command to use is:
> 
> $ ./smatch/smatch_data/db/smdb.py preempt ips_leave
> rtw_set_802_11_disassociate() <- disables preempt
> -> _rtw_pwr_wakeup()
>     -> LeaveAllPowerSaveMode()
>        -> ips_leave()
> 
> I save that to a file, open it with with vim and run a vim function
> `hall` (for highlight all) from my .vimrc file.
> 
> " Use :hall to highlight all the words in a file (for debugging sleeping bugs)
> function HLall()
>    let a=[]
>    %s/\w\+/\=add(a, submatch(0))/gn
>    let @/ = join(a, "\\|")
> endfunction
> cnoreabbrev hall call HLall() <CR>:set hls<CR>
> 
> That highlights all the functions is the call tree, then I use cscope to
> jump to rtw_set_802_11_disassociate and follow the highlighted functions
> to ips_leave().

Thank you for this hint Dan. :)

thanks,
Michael

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

[Michael Straube]

On 4/3/22 13:24, Fabio M. De Francesco wrote:
> [PATCH 5.10 67/99] tty: n_hdlc: make n_hdlc_tty_wakeup() asynchronous
> https://lore.kernel.org/lkml/20211220143031.649396763@linuxfoundation.org/
> 
> I hope it helps,
> 

Thank you for the link. :)

thanks,
Michael