* [PATCH v2] staging: r8188eu: Provide a TODO file for this driver @ 2021-08-26 13:03 Fabio M. De Francesco 2021-08-26 13:54 ` Dan Carpenter 0 siblings, 1 reply; 8+ messages in thread From: Fabio M. De Francesco @ 2021-08-26 13:03 UTC (permalink / raw) To: gregkh, Larry.Finger, phil, Fabio Aiuto, linux-staging, linux-kernel Cc: Fabio M. De Francesco Provide a TODO file that lists the tasks that should be carried out in order to move this driver off drivers/staging. Signed-off-by: Fabio M. De Francesco <fmdefrancesco@gmail.com> --- v1->v2: According to reviews by Fabio Aiuto <fabioaiuto83@gmail.com> and Greg K-H <gregkh@linuxfoundation.org>, remove "[] is currently in development...", indent and properly wrap the lines, remove the unnecessary last paragraph. Thanks to Phillip Potter <phil@philpotter.co.uk> for for providing the first draft. drivers/staging/r8188eu/TODO | 12 ++++++++++++ 1 file changed, 12 insertions(+) create mode 100644 drivers/staging/r8188eu/TODO diff --git a/drivers/staging/r8188eu/TODO b/drivers/staging/r8188eu/TODO new file mode 100644 index 000000000000..d2f21b35fbcb --- /dev/null +++ b/drivers/staging/r8188eu/TODO @@ -0,0 +1,12 @@ +To-do list: + +* Correct the coding style according to Linux guidelines; please read the document + at https://www.kernel.org/doc/html/latest/process/coding-style.html. +* Remove unnecessary debugging/printing macros; for those that are still needed + use the proper kernel API (pr_debug(), dev_dbg(), netdev_dbg()). +* Remove dead code such as unusued functions, variables, fields, etc.. +* Use in-kernel API and remove unnecessary wrappers where possible. +* Remove the HAL layer and migrate its functionality into the relevant parts of + the driver. +* Switch to use LIB80211. +* Switch to use MAC80211. -- 2.32.0 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH v2] staging: r8188eu: Provide a TODO file for this driver 2021-08-26 13:03 [PATCH v2] staging: r8188eu: Provide a TODO file for this driver Fabio M. De Francesco @ 2021-08-26 13:54 ` Dan Carpenter 2021-08-26 16:04 ` Fabio M. De Francesco 2021-10-09 16:31 ` Fabio M. De Francesco 0 siblings, 2 replies; 8+ messages in thread From: Dan Carpenter @ 2021-08-26 13:54 UTC (permalink / raw) To: Fabio M. De Francesco Cc: gregkh, Larry.Finger, phil, Fabio Aiuto, linux-staging, linux-kernel Another thing to fix are some of the sleeping in atomic bugs. drivers/staging/r8188eu/core/rtw_ap.c:139 update_BCNTIM() warn: sleeping in atomic context drivers/staging/r8188eu/core/rtw_ap.c:1296 update_bcn_wps_ie() warn: sleeping in atomic context drivers/staging/r8188eu/core/rtw_ap.c:1361 update_beacon() warn: sleeping in atomic context drivers/staging/r8188eu/core/rtw_ap.c:1725 ap_free_sta() warn: sleeping in atomic context drivers/staging/r8188eu/core/rtw_pwrctrl.c:79 ips_leave() warn: sleeping in atomic context drivers/staging/r8188eu/core/rtw_pwrctrl.c:81 ips_leave() warn: sleeping in atomic context drivers/staging/r8188eu/core/rtw_mlme_ext.c:6764 receive_disconnect() warn: sleeping in atomic context drivers/staging/r8188eu/core/rtw_mlme_ext.c:7083 report_del_sta_event() warn: sleeping in atomic context drivers/staging/r8188eu/core/rtw_mlme_ext.c:8133 set_tx_beacon_cmd() warn: sleeping in atomic context drivers/staging/r8188eu/os_dep/mlme_linux.c:117 rtw_report_sec_ie() warn: sleeping in atomic context There are a few in rtl8723bs as well since the code came from the same place. drivers/staging/rtl8723bs/core/rtw_ap.c:1601 update_beacon() warn: sleeping in atomic context drivers/staging/rtl8723bs/core/rtw_ap.c:1919 ap_free_sta() warn: sleeping in atomic context drivers/staging/rtl8723bs/core/rtw_mlme_ext.c:4270 receive_disconnect() warn: sleeping in atomic context drivers/staging/rtl8723bs/hal/hal_intf.c:100 rtw_hal_init() warn: sleeping in atomic context regards, dan carpenter ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v2] staging: r8188eu: Provide a TODO file for this driver 2021-08-26 13:54 ` Dan Carpenter @ 2021-08-26 16:04 ` Fabio M. De Francesco 2021-10-09 16:31 ` Fabio M. De Francesco 1 sibling, 0 replies; 8+ messages in thread From: Fabio M. De Francesco @ 2021-08-26 16:04 UTC (permalink / raw) To: Dan Carpenter Cc: gregkh, Larry.Finger, phil, Fabio Aiuto, linux-staging, linux-kernel On Thursday, August 26, 2021 3:54:13 PM CEST Dan Carpenter wrote: > Another thing to fix are some of the sleeping in atomic bugs. > > drivers/staging/r8188eu/core/rtw_ap.c:139 update_BCNTIM() warn: sleeping in atomic context > drivers/staging/r8188eu/core/rtw_ap.c:1296 update_bcn_wps_ie() warn: sleeping in atomic context > drivers/staging/r8188eu/core/rtw_ap.c:1361 update_beacon() warn: sleeping in atomic context > drivers/staging/r8188eu/core/rtw_ap.c:1725 ap_free_sta() warn: sleeping in atomic context > drivers/staging/r8188eu/core/rtw_pwrctrl.c:79 ips_leave() warn: sleeping in atomic context > drivers/staging/r8188eu/core/rtw_pwrctrl.c:81 ips_leave() warn: sleeping in atomic context > drivers/staging/r8188eu/core/rtw_mlme_ext.c:6764 receive_disconnect() warn: sleeping in atomic context > drivers/staging/r8188eu/core/rtw_mlme_ext.c:7083 report_del_sta_event() warn: sleeping in atomic context > drivers/staging/r8188eu/core/rtw_mlme_ext.c:8133 set_tx_beacon_cmd() warn: sleeping in atomic context > drivers/staging/r8188eu/os_dep/mlme_linux.c:117 rtw_report_sec_ie() warn: sleeping in atomic context > > There are a few in rtl8723bs as well since the code came from the same > place. > > drivers/staging/rtl8723bs/core/rtw_ap.c:1601 update_beacon() warn: sleeping in atomic context > drivers/staging/rtl8723bs/core/rtw_ap.c:1919 ap_free_sta() warn: sleeping in atomic context > drivers/staging/rtl8723bs/core/rtw_mlme_ext.c:4270 receive_disconnect() warn: sleeping in atomic context > drivers/staging/rtl8723bs/hal/hal_intf.c:100 rtw_hal_init() warn: sleeping in atomic context > > regards, > dan carpenter Thanks for your suggestion, I'll add this too to next version. However I prefer to wait until tomorrow because it is probable that someone else have think of more tasks to add to the list. Regards, Fabio ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v2] staging: r8188eu: Provide a TODO file for this driver 2021-08-26 13:54 ` Dan Carpenter 2021-08-26 16:04 ` Fabio M. De Francesco @ 2021-10-09 16:31 ` Fabio M. De Francesco 2021-10-10 9:21 ` Fabio M. De Francesco 1 sibling, 1 reply; 8+ messages in thread From: Fabio M. De Francesco @ 2021-10-09 16:31 UTC (permalink / raw) To: Dan Carpenter, linux-staging; +Cc: gregkh, linux-kernel On Thursday, August 26, 2021 3:54:13 PM CEST Dan Carpenter wrote: > Another thing to fix are some of the sleeping in atomic bugs. > > drivers/staging/r8188eu/core/rtw_ap.c:139 update_BCNTIM() warn: sleeping in atomic context > drivers/staging/r8188eu/core/rtw_ap.c:1296 update_bcn_wps_ie() warn: sleeping in atomic context > drivers/staging/r8188eu/core/rtw_ap.c:1361 update_beacon() warn: sleeping in atomic context > drivers/staging/r8188eu/core/rtw_ap.c:1725 ap_free_sta() warn: sleeping in atomic context > drivers/staging/r8188eu/core/rtw_pwrctrl.c:79 ips_leave() warn: sleeping in atomic context > drivers/staging/r8188eu/core/rtw_pwrctrl.c:81 ips_leave() warn: sleeping in atomic context > drivers/staging/r8188eu/core/rtw_mlme_ext.c:6764 receive_disconnect() warn: sleeping in atomic context > drivers/staging/r8188eu/core/rtw_mlme_ext.c:7083 report_del_sta_event() warn: sleeping in atomic context > drivers/staging/r8188eu/core/rtw_mlme_ext.c:8133 set_tx_beacon_cmd() warn: sleeping in atomic context > drivers/staging/r8188eu/os_dep/mlme_linux.c:117 rtw_report_sec_ie() warn: sleeping in atomic context > > There are a few in rtl8723bs as well since the code came from the same > place. > > drivers/staging/rtl8723bs/core/rtw_ap.c:1601 update_beacon() warn: sleeping in atomic context > drivers/staging/rtl8723bs/core/rtw_ap.c:1919 ap_free_sta() warn: sleeping in atomic context > drivers/staging/rtl8723bs/core/rtw_mlme_ext.c:4270 receive_disconnect() warn: sleeping in atomic context > drivers/staging/rtl8723bs/hal/hal_intf.c:100 rtw_hal_init() warn: sleeping in atomic context > > regards, > dan carpenter > Hello Dan, I'd like to address these kind of bugs, but I have a couple of questions about them. 1) You've listed what looks like the output of a compiler or static analyzer. How did you get the warnings you copy-pasted above? 2) I know that both the execution of interrupt handlers (ISRs) as well as any code blocks that are executed holding spinlocks are "atomic contexts". In these cases, "sleeping" is not allowed (for obvious reasons). Besides the two mentioned above, are there any further cases of "atomic contexts" in the kernel? Thank you in advance, Fabio ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v2] staging: r8188eu: Provide a TODO file for this driver 2021-10-09 16:31 ` Fabio M. De Francesco @ 2021-10-10 9:21 ` Fabio M. De Francesco 2021-10-11 8:21 ` Dan Carpenter 0 siblings, 1 reply; 8+ messages in thread From: Fabio M. De Francesco @ 2021-10-10 9:21 UTC (permalink / raw) To: Dan Carpenter, linux-staging; +Cc: gregkh, linux-kernel On Saturday, October 9, 2021 6:31:12 PM CEST Fabio M. De Francesco wrote: > On Thursday, August 26, 2021 3:54:13 PM CEST Dan Carpenter wrote: > > Another thing to fix are some of the sleeping in atomic bugs. > > > > drivers/staging/r8188eu/core/rtw_ap.c:139 update_BCNTIM() warn: sleeping > > in atomic context > > drivers/staging/r8188eu/core/rtw_ap.c:1296 update_bcn_wps_ie() warn: > > sleeping in atomic context > > > > [...] > > > Hello Dan, > > I'd like to address these kind of bugs, but I have a couple of questions > about them. > > 1) You've listed what looks like the output of a compiler or static > analyzer. > How did you get the warnings you copy-pasted above? > > 2) I know that both the execution of interrupt handlers (ISRs) as well as > any > code blocks that are executed holding spinlocks are "atomic contexts". In > these cases, "sleeping" is not allowed (for obvious reasons). Besides the > two > mentioned above, are there any further cases of "atomic contexts" in the > kernel? After some research, I've found that Softirqs and Tasklets are also executed in "atomic context", as hardware interrupt service routines are. Furthermore, I've also found a .config option named DEBUG_ATOMIC_SLEEP that should warn if some code is sleeping in "atomic context". However, the documentation of that option does not explain where the output of these checks can be read. I would appreciate any help on this matter. Thanks, Fabio > > Thank you in advance, > > Fabio > > > > ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v2] staging: r8188eu: Provide a TODO file for this driver 2021-10-10 9:21 ` Fabio M. De Francesco @ 2021-10-11 8:21 ` Dan Carpenter 2021-10-11 8:28 ` Dan Carpenter 2021-10-11 9:53 ` Fabio M. De Francesco 0 siblings, 2 replies; 8+ messages in thread From: Dan Carpenter @ 2021-10-11 8:21 UTC (permalink / raw) To: Fabio M. De Francesco; +Cc: linux-staging, gregkh, linux-kernel On Sun, Oct 10, 2021 at 11:21:49AM +0200, Fabio M. De Francesco wrote: > On Saturday, October 9, 2021 6:31:12 PM CEST Fabio M. De Francesco wrote: > > On Thursday, August 26, 2021 3:54:13 PM CEST Dan Carpenter wrote: > > > Another thing to fix are some of the sleeping in atomic bugs. > > > > > > drivers/staging/r8188eu/core/rtw_ap.c:139 update_BCNTIM() warn: sleeping > > > in atomic context > > > drivers/staging/r8188eu/core/rtw_ap.c:1296 update_bcn_wps_ie() warn: > > > sleeping in atomic context > > > > > > [...] > > > > > Hello Dan, > > > > I'd like to address these kind of bugs, but I have a couple of questions > > about them. > > > > 1) You've listed what looks like the output of a compiler or static > > analyzer. > > How did you get the warnings you copy-pasted above? > > > > 2) I know that both the execution of interrupt handlers (ISRs) as well as > > any > > code blocks that are executed holding spinlocks are "atomic contexts". In > > these cases, "sleeping" is not allowed (for obvious reasons). Besides the > > two > > mentioned above, are there any further cases of "atomic contexts" in the > > kernel? > > After some research, I've found that Softirqs and Tasklets are also executed > in "atomic context", as hardware interrupt service routines are. > > Furthermore, I've also found a .config option named DEBUG_ATOMIC_SLEEP > that should warn if some code is sleeping in "atomic context". However, the > documentation of that option does not explain where the output of these > checks can be read. > > I would appreciate any help on this matter. These are a new Smatch warning. https://lkml.org/lkml/2021/9/1/950 You would need to rebuild the Smatch database probably around five times to see the warnings. It takes a long time to build... It's probably not that hard to figure out just from looking at the code without the generating the warning. So spin locks can't sleep. Mutexes can. There are read/write locks, built on both mutexes and spin locks. Rcu_read/write cannot sleep. regards, dan carpenter ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v2] staging: r8188eu: Provide a TODO file for this driver 2021-10-11 8:21 ` Dan Carpenter @ 2021-10-11 8:28 ` Dan Carpenter 2021-10-11 9:53 ` Fabio M. De Francesco 1 sibling, 0 replies; 8+ messages in thread From: Dan Carpenter @ 2021-10-11 8:28 UTC (permalink / raw) To: Fabio M. De Francesco; +Cc: linux-staging, gregkh, linux-kernel Here it the `smdb.py preempt <func>` output for all the r8188eu warnings. Unfiltered and unchecked. regards, dan carpenter ====== drivers/staging/r8188eu/core/rtw_ap.c:1080 ap_free_sta() warn: sleeping in atomic context OnDeAuth() <- disables preempt OnDisassoc() <- disables preempt expire_timeout_chk() <- disables preempt rtw_wx_set_wap() <- disables preempt rtw_wx_set_essid() <- disables preempt -> rtw_set_802_11_infrastructure_mode() <- disables preempt -> stop_ap_mode() -> rtw_sta_flush() <- disables preempt rtw_del_sta() <- disables preempt cfg80211_rtw_del_station() <- disables preempt ap_control_kickall() <- disables preempt -> ap_free_sta() ====== drivers/staging/r8188eu/core/rtw_pwrctrl.c:76 ips_leave() warn: sleeping in atomic context drivers/staging/r8188eu/core/rtw_pwrctrl.c:78 ips_leave() warn: sleeping in atomic context rtw_set_802_11_disassociate() <- disables preempt -> _rtw_pwr_wakeup() -> LeaveAllPowerSaveMode() _rtw_pwr_wakeup() <duplicate> -> ips_leave() ====== drivers/staging/r8188eu/core/rtw_mlme.c:1602 rtw_set_key() warn: sleeping in atomic context rtw_set_802_11_disassociate() <- disables preempt -> _rtw_pwr_wakeup() -> LeaveAllPowerSaveMode() _rtw_pwr_wakeup() <duplicate> -> ips_leave() -> _ips_leave() ips_leave() <duplicate> -> rtw_ips_pwr_up() -> ips_netdrv_open() -> rtw_hal_init() -> rtw_sec_restore_wep_key() ips_leave() <duplicate> -> rtw_set_key() ====== drivers/staging/r8188eu/core/rtw_mlme_ext.c:6540 receive_disconnect() warn: sleeping in atomic context rtw_surveydone_event_callback() <- disables preempt -> receive_disconnect() ====== drivers/staging/r8188eu/core/rtw_mlme_ext.c:6859 report_del_sta_event() warn: sleeping in atomic context rtw_surveydone_event_callback() <- disables preempt -> receive_disconnect() OnDeAuth() <- disables preempt OnDisassoc() <- disables preempt expire_timeout_chk() <- disables preempt rtw_wx_set_wap() <- disables preempt rtw_wx_set_essid() <- disables preempt -> rtw_set_802_11_infrastructure_mode() <- disables preempt -> stop_ap_mode() -> rtw_sta_flush() <- disables preempt rtw_del_sta() <- disables preempt cfg80211_rtw_del_station() <- disables preempt ap_control_kickall() <- disables preempt -> ap_free_sta() -> report_del_sta_event() ====== drivers/staging/r8188eu/os_dep/mlme_linux.c:115 rtw_report_sec_ie() warn: sleeping in atomic context rtw_set_802_11_ssid() <- disables preempt rtw_set_802_11_connect() <- disables preempt _rtw_join_timeout_handler() <- disables preempt rtw_stadel_event_callback() <- disables preempt rtw_roaming() <- disables preempt -> _rtw_roaming() rtw_set_802_11_bssid() <- disables preempt -> rtw_do_join() rtw_surveydone_event_callback() <- disables preempt -> rtw_select_and_join_from_scanned_queue() <- disables preempt -> rtw_joinbss_cmd() -> rtw_restruct_sec_ie() -> rtw_report_sec_ie() ====== ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v2] staging: r8188eu: Provide a TODO file for this driver 2021-10-11 8:21 ` Dan Carpenter 2021-10-11 8:28 ` Dan Carpenter @ 2021-10-11 9:53 ` Fabio M. De Francesco 1 sibling, 0 replies; 8+ messages in thread From: Fabio M. De Francesco @ 2021-10-11 9:53 UTC (permalink / raw) To: Dan Carpenter; +Cc: linux-staging, gregkh, linux-kernel, julia.lawall On Monday, October 11, 2021 10:21:34 AM CEST Dan Carpenter wrote: > On Sun, Oct 10, 2021 at 11:21:49AM +0200, Fabio M. De Francesco wrote: > > On Saturday, October 9, 2021 6:31:12 PM CEST Fabio M. De Francesco wrote: > > > On Thursday, August 26, 2021 3:54:13 PM CEST Dan Carpenter wrote: > > > > Another thing to fix are some of the sleeping in atomic bugs. > > > > > > > > drivers/staging/r8188eu/core/rtw_ap.c:139 update_BCNTIM() warn: sleeping > > > > in atomic context > > > > drivers/staging/r8188eu/core/rtw_ap.c:1296 update_bcn_wps_ie() warn: > > > > sleeping in atomic context > > > > > > > > [...] > > > > > > > Hello Dan, > > > > > > I'd like to address these kind of bugs, but I have a couple of questions > > > about them. > > > > > > 1) You've listed what looks like the output of a compiler or static > > > analyzer. > > > How did you get the warnings you copy-pasted above? > > > > > > 2) I know that both the execution of interrupt handlers (ISRs) as well as > > > any > > > code blocks that are executed holding spinlocks are "atomic contexts". In > > > these cases, "sleeping" is not allowed (for obvious reasons). Besides the > > > two > > > mentioned above, are there any further cases of "atomic contexts" in the > > > kernel? > > > > After some research, I've found that Softirqs and Tasklets are also executed > > in "atomic context", as hardware interrupt service routines are. > > > > Furthermore, I've also found a .config option named DEBUG_ATOMIC_SLEEP > > that should warn if some code is sleeping in "atomic context". However, the > > documentation of that option does not explain where the output of these > > checks can be read. > > > > I would appreciate any help on this matter. > > These are a new Smatch warning. > > https://lkml.org/lkml/2021/9/1/950 > > You would need to rebuild the Smatch database probably around five times > to see the warnings. It takes a long time to build... It's probably > not that hard to figure out just from looking at the code without the > generating the warning. I must confess that, since I started to submit patches to the Kernel this year in April and until now, I have not ever used Smatch. I thought that building with GCC and setting C=2 and W=1 were enough. Sometimes I've also used Coccinelle. That's all. Now I admit that I was plainly wrong. My _big_ fault, sorry :( This morning I have taken a quick look at your code at https://github.com/ error27/smatch. Obviously, what I could see is only the overall design and it looks quite impressive. I'll start using Smatch soon. > So spin locks can't sleep. Mutexes can. There are read/write locks, > built on both mutexes and spin locks. Rcu_read/write cannot sleep. Oh, right. RCU were missing from my lists. Thanks very much for Smatch and for your kind replies. Regards, Fabio M. De Francesco P.S.: Yesterday I read the first one third of a paper co-authored by Julia Lawall (Effective Detection of Sleep-in-atomic-context Bugs in the Linux Kernel - https://doi.org/10.1145/3381990). It talks about a practical static approach named DSAC. It looks really interesting, but it seems that their tool is not yet available to the public. Do you know something about it? I've found an old message by Greg K-H that asked for where to find that tool but, as far as I know, he have not yet had that information. With this message I've Cc'd Julia in case she has time to reply. > regards, > dan carpenter > > ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2021-10-11 9:53 UTC | newest] Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-08-26 13:03 [PATCH v2] staging: r8188eu: Provide a TODO file for this driver Fabio M. De Francesco 2021-08-26 13:54 ` Dan Carpenter 2021-08-26 16:04 ` Fabio M. De Francesco 2021-10-09 16:31 ` Fabio M. De Francesco 2021-10-10 9:21 ` Fabio M. De Francesco 2021-10-11 8:21 ` Dan Carpenter 2021-10-11 8:28 ` Dan Carpenter 2021-10-11 9:53 ` Fabio M. De Francesco
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.