[LEDE-DEV,v1,1/2] base-files, mac80211, broadcom-wl: plug-and-play wifi detection

Hello,

On Friday, October 7, 2016 8:29:30 PM CEST Matthias Schiffer wrote:
> On 10/07/2016 08:10 PM, Christian Lamparter wrote:
> > Currently, the wifi detection script is executed as part of
> > the (early) boot process. Pluggable wifi USB devices, which
> > are inserted at a later time are not automatically
> > detected and therefore they don't show up in LuCI.
> > 
> > A user has to deal with wifi detection manually, or restart
> > the router.
> > 
> > [...]
> > ---
> > We would like to hear, if these changes work with broadcom-wl.
> > (Felix removed the hostap, so this isn't included anymore).
> > 
> > trap and lock are part of the default busybox setup.
> 
> Hi,
> it would be great to remove the direct write of /etc/config/wireless
> completely, as it won't lock against other users of UCI that modify the
> wireless config. IMO, it should just use UCI to modify the configuration as
> everything else does.

Well, What's the situation with ECE and configd? I didn't want to touch it
since you plan to move away from the config files and replace them with
json. 

Anyway, I attached two RFCs (one for broadcom, the other mac80211)
that replaces the code with uci calls.

One issue is that there's no longer the "# REMOVE THIS LINE TO ENABLE WIFI:"
line and people might overlook the "disabled 1" setting.

Note: the "> /dev/null" for uci calls were added just in case someone still
has the old /etc/init.d/boot and to not write garbage into /e/c/wireless.

Note2: I've also changed the "plug-and-play wifi" patch and removed the
/tmp/wireless.tmp step. But we still need proper locking. 
(That said, I would like to move the locking to the mac80211.sh / broadcom.sh
detect functions, is everyone fine with that?)

---
mac80211: use uci to generate wireless config file

Previously, wifi detect simply dumped its generated wireless
configuration out to STDOUT. A second step was needed to
append the configuration to /etc/config/wireless (or create
it, if it didn't exist).

With this patch, The wifi detection script will now use uci
to update the wireless configuration directly.

Signed-off-by: Christian Lamparter <chunkeey@gmail.com>
---

Dnia 2016-10-08, o godz. 17:41:35
Christian Lamparter <chunkeey@googlemail.com> napisał(a):

> Hello,
> 
> On Friday, October 7, 2016 8:29:30 PM CEST Matthias Schiffer wrote:
> > On 10/07/2016 08:10 PM, Christian Lamparter wrote:  
> > > Currently, the wifi detection script is executed as part of
> > > the (early) boot process. Pluggable wifi USB devices, which
> > > are inserted at a later time are not automatically
> > > detected and therefore they don't show up in LuCI.
> > > 
> > > A user has to deal with wifi detection manually, or restart
> > > the router.
> > > 
> > > [...]
> > > ---
> > > We would like to hear, if these changes work with broadcom-wl.
> > > (Felix removed the hostap, so this isn't included anymore).
> > > 
> > > trap and lock are part of the default busybox setup.  
> > 
> > Hi,
> > it would be great to remove the direct write of /etc/config/wireless
> > completely, as it won't lock against other users of UCI that modify the
> > wireless config. IMO, it should just use UCI to modify the configuration as
> > everything else does.  
> 
> Well, What's the situation with ECE and configd? I didn't want to touch it
> since you plan to move away from the config files and replace them with
> json. 
> 
> Anyway, I attached two RFCs (one for broadcom, the other mac80211)
> that replaces the code with uci calls.
> 
> One issue is that there's no longer the "# REMOVE THIS LINE TO ENABLE WIFI:"
> line and people might overlook the "disabled 1" setting.
> 
> Note: the "> /dev/null" for uci calls were added just in case someone still
> has the old /etc/init.d/boot and to not write garbage into /e/c/wireless.
> 
> Note2: I've also changed the "plug-and-play wifi" patch and removed the
> /tmp/wireless.tmp step. But we still need proper locking. 
> (That said, I would like to move the locking to the mac80211.sh / broadcom.sh
> detect functions, is everyone fine with that?)
> 
> ---
> mac80211: use uci to generate wireless config file
> 
> Previously, wifi detect simply dumped its generated wireless
> configuration out to STDOUT. A second step was needed to
> append the configuration to /etc/config/wireless (or create
> it, if it didn't exist).
> 
> With this patch, The wifi detection script will now use uci
> to update the wireless configuration directly.
> 
> Signed-off-by: Christian Lamparter <chunkeey@gmail.com>
> ---
> diff --git a/package/kernel/mac80211/files/lib/wifi/mac80211.sh b/package/kernel/mac80211/files/lib/wifi/mac80211.sh
> --- a/package/kernel/mac80211/files/lib/wifi/mac80211.sh
> +++ b/package/kernel/mac80211/files/lib/wifi/mac80211.sh
> @@ -92,7 +92,7 @@ detect_mac80211() {
>  			htmode="VHT80"
>  		}
>  
> -		[ -n $htmode ] && append ht_capab "	option htmode	$htmode" "$N"
> +		[ -n $htmode ] && ht_capab="set wireless.radio${devidx}.htmode=$htmode"
>  
>  		if [ -x /usr/bin/readlink -a -h /sys/class/ieee80211/${dev} ]; then
>  			path="$(readlink -f /sys/class/ieee80211/${dev}/device)"
> @@ -104,30 +104,30 @@ detect_mac80211() {
>  			case "$path" in
>  				platform*/pci*) path="${path##platform/}";;
>  			esac
> -			dev_id="	option path	'$path'"
> +			dev_id="set wireless.radio${devidx}.path='$path'"
>  		else
> -			dev_id="	option macaddr	$(cat /sys/class/ieee80211/${dev}/macaddress)"
> +			dev_id="set wireless.radio${devidx}.macaddr=$(cat /sys/class/ieee80211/${dev}/macaddress)"
>  		fi
>  
> -		cat <<EOF
> -config wifi-device  radio$devidx
> -	option type     mac80211
> -	option channel  ${channel}
> -	option hwmode	11${mode_band}
> -$dev_id
> -$ht_capab
> -	# REMOVE THIS LINE TO ENABLE WIFI:
> -	option disabled 1
> -
> -config wifi-iface
> -	option device   radio$devidx
> -	option network  lan
> -	option mode     ap
> -	option ssid     LEDE
> -	option encryption none
> -
> +		uci -q batch > /dev/null <<-EOF
> +			set wireless.radio${devidx}=wifi-device
> +			set wireless.radio${devidx}.type=mac80211
> +			set wireless.radio${devidx}.channel=${channel}
> +			set wireless.radio${devidx}.hwmode=11${mode_band}
> +			${dev_id}
> +			${ht_capab}
> +			set wireless.radio${devidx}.disabled=1
> +
> +			add wireless wifi-iface
> +			set wireless.@wifi-iface[-1]=wifi-iface
> +			set wireless.@wifi-iface[-1].device=radio${devidx}
> +			set wireless.@wifi-iface[-1].network=lan
> +			set wireless.@wifi-iface[-1].mode=ap


> +			set wireless.@wifi-iface[-1].ssid=LEDE

[...]
> +			set wireless.@wifi-iface[-1].ssid=Lede${i#0}

BTW:

Dnia 2016-10-08, o godz. 17:41:35
Christian Lamparter <chunkeey@googlemail.com> napisał(a):

Hello,

[...]

> Anyway, I attached two RFCs (one for broadcom, the other mac80211)
> that replaces the code with uci calls.

[...]

> +			set wireless.@wifi-iface[-1].ssid=LEDE

[...]

> +			set wireless.@wifi-iface[-1].ssid=Lede${i#0}

BTW: Can we change default SSID as was mentioned here: 
https://patchwork.ozlabs.org/patch/625510/ ? (LEDE + last digits of mac address?)

On 10/08/2016 05:41 PM, Christian Lamparter wrote:
> Hello,
> 
> On Friday, October 7, 2016 8:29:30 PM CEST Matthias Schiffer wrote:
>> On 10/07/2016 08:10 PM, Christian Lamparter wrote:
>>> Currently, the wifi detection script is executed as part of
>>> the (early) boot process. Pluggable wifi USB devices, which
>>> are inserted at a later time are not automatically
>>> detected and therefore they don't show up in LuCI.
>>>
>>> A user has to deal with wifi detection manually, or restart
>>> the router.
>>>
>>> [...]
>>> ---
>>> We would like to hear, if these changes work with broadcom-wl.
>>> (Felix removed the hostap, so this isn't included anymore).
>>>
>>> trap and lock are part of the default busybox setup.
>>
>> Hi,
>> it would be great to remove the direct write of /etc/config/wireless
>> completely, as it won't lock against other users of UCI that modify the
>> wireless config. IMO, it should just use UCI to modify the configuration as
>> everything else does.
> 

> Well, What's the situation with ECE and configd? I didn't want to touch it
> since you plan to move away from the config files and replace them with
> json. 

There isn't a concrete plan to integrate ECE with LEDE yet (there are still
some TODOs, and it will need to be discussed further with the LEDE
community). It will provide a bidirectional UCI mapping; this means as long
as the uci CLI and similar tools are used, most things should just continue
work with ECE.

> 
> Anyway, I attached two RFCs (one for broadcom, the other mac80211)
> that replaces the code with uci calls.

Thanks, I'll try it out later.

> 
> One issue is that there's no longer the "# REMOVE THIS LINE TO ENABLE WIFI:"
> line and people might overlook the "disabled 1" setting.

This single comment seems to be the only reason this was directly written
to the config file in the first place...

> 
> Note: the "> /dev/null" for uci calls were added just in case someone still
> has the old /etc/init.d/boot and to not write garbage into /e/c/wireless.
> 
> Note2: I've also changed the "plug-and-play wifi" patch and removed the
> /tmp/wireless.tmp step. But we still need proper locking. 
> (That said, I would like to move the locking to the mac80211.sh / broadcom.sh
> detect functions, is everyone fine with that?)

Makes sense to me (maybe we can further factor out common parts of
mac80211.sh and broadcom.sh?). Note that you don't need any locking in the
hotplug handlers anyways, they are always run sequentially.

Regards,
Matthias


> 
> ---
> mac80211: use uci to generate wireless config file
> 
> Previously, wifi detect simply dumped its generated wireless
> configuration out to STDOUT. A second step was needed to
> append the configuration to /etc/config/wireless (or create
> it, if it didn't exist).
> 
> With this patch, The wifi detection script will now use uci
> to update the wireless configuration directly.
> 
> Signed-off-by: Christian Lamparter <chunkeey@gmail.com>
> ---
> diff --git a/package/kernel/mac80211/files/lib/wifi/mac80211.sh b/package/kernel/mac80211/files/lib/wifi/mac80211.sh
> --- a/package/kernel/mac80211/files/lib/wifi/mac80211.sh
> +++ b/package/kernel/mac80211/files/lib/wifi/mac80211.sh
> @@ -92,7 +92,7 @@ detect_mac80211() {
>  			htmode="VHT80"
>  		}
>  
> -		[ -n $htmode ] && append ht_capab "	option htmode	$htmode" "$N"
> +		[ -n $htmode ] && ht_capab="set wireless.radio${devidx}.htmode=$htmode"
>  
>  		if [ -x /usr/bin/readlink -a -h /sys/class/ieee80211/${dev} ]; then
>  			path="$(readlink -f /sys/class/ieee80211/${dev}/device)"
> @@ -104,30 +104,30 @@ detect_mac80211() {
>  			case "$path" in
>  				platform*/pci*) path="${path##platform/}";;
>  			esac
> -			dev_id="	option path	'$path'"
> +			dev_id="set wireless.radio${devidx}.path='$path'"
>  		else
> -			dev_id="	option macaddr	$(cat /sys/class/ieee80211/${dev}/macaddress)"
> +			dev_id="set wireless.radio${devidx}.macaddr=$(cat /sys/class/ieee80211/${dev}/macaddress)"
>  		fi
>  
> -		cat <<EOF
> -config wifi-device  radio$devidx
> -	option type     mac80211
> -	option channel  ${channel}
> -	option hwmode	11${mode_band}
> -$dev_id
> -$ht_capab
> -	# REMOVE THIS LINE TO ENABLE WIFI:
> -	option disabled 1
> -
> -config wifi-iface
> -	option device   radio$devidx
> -	option network  lan
> -	option mode     ap
> -	option ssid     LEDE
> -	option encryption none
> -
> +		uci -q batch > /dev/null <<-EOF
> +			set wireless.radio${devidx}=wifi-device
> +			set wireless.radio${devidx}.type=mac80211
> +			set wireless.radio${devidx}.channel=${channel}
> +			set wireless.radio${devidx}.hwmode=11${mode_band}
> +			${dev_id}
> +			${ht_capab}
> +			set wireless.radio${devidx}.disabled=1
> +
> +			add wireless wifi-iface
> +			set wireless.@wifi-iface[-1]=wifi-iface
> +			set wireless.@wifi-iface[-1].device=radio${devidx}
> +			set wireless.@wifi-iface[-1].network=lan
> +			set wireless.@wifi-iface[-1].mode=ap
> +			set wireless.@wifi-iface[-1].ssid=LEDE
> +			set wireless.@wifi-iface[-1].encryption=none
>  EOF
> -	devidx=$(($devidx + 1))
> -	done
> -}
> +		uci commit &> /dev/null
>  
> +		devidx=$(($devidx + 1))
> +		done
> +}
> ---
> broadcom-wl: use uci to generate wireless config file
> 
> Previously, wifi detect simply dumped its generated wireless
> configuration out to STDOUT. A second step was needed to
> append the configuration to /etc/config/wireless (or create
> it, if it didn't exist).
> 
> With this patch, The wifi detection script will now use uci
> to update the wireless configuration directly.
> 
> Signed-off-by: Christian Lamparter <chunkeey@gmail.com>
> ---
> diff --git a/package/kernel/broadcom-wl/files/lib/wifi/broadcom.sh b/package/kernel/broadcom-wl/files/lib/wifi/broadcom.sh
> --- a/package/kernel/broadcom-wl/files/lib/wifi/broadcom.sh
> +++ b/package/kernel/broadcom-wl/files/lib/wifi/broadcom.sh
> @@ -456,22 +456,23 @@ detect_broadcom() {
>  		config_get type wl${i} type
>  		[ "$type" = broadcom ] && continue
>  		channel=`wlc ifname wl${i} channel`
> -		cat <<EOF
> -config wifi-device  wl${i}
> -	option type     broadcom
> -	option channel  ${channel:-11}
> -	option txantenna 3
> -	option rxantenna 3
> -	# REMOVE THIS LINE TO ENABLE WIFI:
> -	option disabled 1
> -
> -config wifi-iface
> -	option device   wl${i}
> -	option network	lan
> -	option mode     ap
> -	option ssid     Lede${i#0}
> -	option encryption none
>  
> +		uci -q batch > /dev/null <<-EOF
> +			set wireless.wl${i}=wifi-device
> +			set wireless.wl${i}.type=broadcom
> +			set wireless.wl${i}.channel=${channel:-11}
> +			set wireless.wl${i}.txantenna=3
> +			set wireless.wl${i}.rxantenna=3
> +			set wireless.wl${i}.disabled=1
> +
> +			add wireless wifi-iface
> +			set wireless.@wifi-iface[-1]=wifi-iface
> +			set wireless.@wifi-iface[-1].device=wl${i}
> +			set wireless.@wifi-iface[-1].network=lan
> +			set wireless.@wifi-iface[-1].mode=ap
> +			set wireless.@wifi-iface[-1].ssid=Lede${i#0}
> +			set wireless.@wifi-iface[-1].encryption=none
>  EOF
> +	uci commit &> /dev/null
>  	done
>  }
>

On Saturday, October 8, 2016 6:04:09 PM CEST Matthias Schiffer wrote:
> On 10/08/2016 05:41 PM, Christian Lamparter wrote:
> > On Friday, October 7, 2016 8:29:30 PM CEST Matthias Schiffer wrote:
> >> On 10/07/2016 08:10 PM, Christian Lamparter wrote:
> >>> Currently, the wifi detection script is executed as part of
> >>> the (early) boot process. Pluggable wifi USB devices, which
> >>> are inserted at a later time are not automatically
> >>> detected and therefore they don't show up in LuCI.
> >>>
> >>> A user has to deal with wifi detection manually, or restart
> >>> the router.
> >>>
> >>> [...]
> >>> ---
> >>> We would like to hear, if these changes work with broadcom-wl.
> >>> (Felix removed the hostap, so this isn't included anymore).
> >>>
> >>> trap and lock are part of the default busybox setup.
> >>
> >> Hi,
> >> it would be great to remove the direct write of /etc/config/wireless
> >> completely, as it won't lock against other users of UCI that modify the
> >> wireless config. IMO, it should just use UCI to modify the configuration as
> >> everything else does.
> > 
> 
> > Well, What's the situation with ECE and configd? I didn't want to touch it
> > since you plan to move away from the config files and replace them with
> > json. 
> 
> There isn't a concrete plan to integrate ECE with LEDE yet (there are still
> some TODOs, and it will need to be discussed further with the LEDE
> community). It will provide a bidirectional UCI mapping; this means as long
> as the uci CLI and similar tools are used, most things should just continue
> work with ECE.
Ok. But how would the /etc/config/wireless have worked? As it just produced a
file and bypassed all the uci CLI and similar tools. Did you setup file
notifications to check if something as modified in /etc/config/ run uci to
import it or sth. like that? (Because that was why I contacted you earlier :) )
 
> > Note: the "> /dev/null" for uci calls were added just in case someone still
> > has the old /etc/init.d/boot and to not write garbage into /e/c/wireless.
> > 
> > Note2: I've also changed the "plug-and-play wifi" patch and removed the
> > /tmp/wireless.tmp step. But we still need proper locking. 
> > (That said, I would like to move the locking to the mac80211.sh / broadcom.sh
> > detect functions, is everyone fine with that?)
> 
> Makes sense to me (maybe we can further factor out common parts of
> mac80211.sh and broadcom.sh?).
I don't think that's within the scope of this series :-D. Also, I don't have
any  broadcom devices... 

> Note that you don't need any locking in the hotplug handlers anyways, they are
> always run sequentially.
I moved the locking to detect_mac80211 and detect_broadcom. 
A user might still want to run "wifi detect" (out of habit?) and that
could race with hotplug event .

As for procd:
Can you tell me where this serialization happens or how it works?

I've looked into this earlier and found that procd uses the hotplug-call
in hotplug.json to run all the scripts under /etc/hotplug.d/%SUBSYSTEMS%.

I've looked up handle_exec (which is used to run hotplug-call)
and found execvp [0]. The thing with execvp is that it fully replaces
the current thread with the new program it is supposed to execute
(it never returns, unless it fails before it jumps to the new program).
So procd needs to fork() before it can run hotplug-call and this is
done in queue_next [1].

However, if procd is supposed to serialize these calls, it will
need to use some sort of wait() or waitpid() to wait for the
forked processes to finish. Since there is no wait()/waitpid() in 
there, the hotplug-call can run concurrently.

(I know that the event messages from the kernel to procd are indeed
serialized by the netlink message interface.)

so, what is wrong here?

Regards,
Christian

[0] <https://git.lede-project.org/?p=project/procd.git;a=blob;f=plug/hotplug.c;h=85959155634f1a6a9ada11c7045601885a0abad8;hb=72f63807b3644ef7b8ab9025a02d7f509f39ad14#l204>

[1] <https://git.lede-project.org/?p=project/procd.git;a=blob;f=plug/hotplug.c;h=85959155634f1a6a9ada11c7045601885a0abad8;hb=72f63807b3644ef7b8ab9025a02d7f509f39ad14#l347>

On 10/08/2016 07:13 PM, Christian Lamparter wrote:
> On Saturday, October 8, 2016 6:04:09 PM CEST Matthias Schiffer wrote:
>> On 10/08/2016 05:41 PM, Christian Lamparter wrote:
>>> On Friday, October 7, 2016 8:29:30 PM CEST Matthias Schiffer wrote:
>>>> On 10/07/2016 08:10 PM, Christian Lamparter wrote:
>>>>> Currently, the wifi detection script is executed as part of
>>>>> the (early) boot process. Pluggable wifi USB devices, which
>>>>> are inserted at a later time are not automatically
>>>>> detected and therefore they don't show up in LuCI.
>>>>>
>>>>> A user has to deal with wifi detection manually, or restart
>>>>> the router.
>>>>>
>>>>> [...]
>>>>> ---
>>>>> We would like to hear, if these changes work with broadcom-wl.
>>>>> (Felix removed the hostap, so this isn't included anymore).
>>>>>
>>>>> trap and lock are part of the default busybox setup.
>>>>
>>>> Hi,
>>>> it would be great to remove the direct write of /etc/config/wireless
>>>> completely, as it won't lock against other users of UCI that modify the
>>>> wireless config. IMO, it should just use UCI to modify the configuration as
>>>> everything else does.
>>>
>>
>>> Well, What's the situation with ECE and configd? I didn't want to touch it
>>> since you plan to move away from the config files and replace them with
>>> json. 
>>
>> There isn't a concrete plan to integrate ECE with LEDE yet (there are still
>> some TODOs, and it will need to be discussed further with the LEDE
>> community). It will provide a bidirectional UCI mapping; this means as long
>> as the uci CLI and similar tools are used, most things should just continue
>> work with ECE.
> Ok. But how would the /etc/config/wireless have worked? As it just produced a
> file and bypassed all the uci CLI and similar tools. Did you setup file
> notifications to check if something as modified in /etc/config/ run uci to
> import it or sth. like that? (Because that was why I contacted you earlier :) )

Converting that script to the UCI CLI was still a TODO for making this
work. So far, I haven't even finished the binding code itself (I've been
working on other projects during the last weeks), and haven't had a closer
look at all UCI users yet to look for potential issues. I plan to return to
working on ECE soon.

>  
>>> Note: the "> /dev/null" for uci calls were added just in case someone still
>>> has the old /etc/init.d/boot and to not write garbage into /e/c/wireless.
>>>
>>> Note2: I've also changed the "plug-and-play wifi" patch and removed the
>>> /tmp/wireless.tmp step. But we still need proper locking. 
>>> (That said, I would like to move the locking to the mac80211.sh / broadcom.sh
>>> detect functions, is everyone fine with that?)
>>
>> Makes sense to me (maybe we can further factor out common parts of
>> mac80211.sh and broadcom.sh?).
> I don't think that's within the scope of this series :-D. Also, I don't have
> any  broadcom devices... 
> 
>> Note that you don't need any locking in the hotplug handlers anyways, they are
>> always run sequentially.
> I moved the locking to detect_mac80211 and detect_broadcom. 
> A user might still want to run "wifi detect" (out of habit?) and that
> could race with hotplug event .
> 
> As for procd:
> Can you tell me where this serialization happens or how it works?
> 
> I've looked into this earlier and found that procd uses the hotplug-call
> in hotplug.json to run all the scripts under /etc/hotplug.d/%SUBSYSTEMS%.
> 
> I've looked up handle_exec (which is used to run hotplug-call)
> and found execvp [0]. The thing with execvp is that it fully replaces
> the current thread with the new program it is supposed to execute
> (it never returns, unless it fails before it jumps to the new program).
> So procd needs to fork() before it can run hotplug-call and this is
> done in queue_next [1].
> 
> However, if procd is supposed to serialize these calls, it will
> need to use some sort of wait() or waitpid() to wait for the
> forked processes to finish. Since there is no wait()/waitpid() in 
> there, the hotplug-call can run concurrently.

queue_proc_cb() is run as soon as the previous hotplug-call is finished
(the wait is hidden behind uloop), which will then call queue_next() to
handle the next hotplug event.

> 
> (I know that the event messages from the kernel to procd are indeed
> serialized by the netlink message interface.)
> 
> so, what is wrong here?
> 
> Regards,
> Christian
> 
> [0] <https://git.lede-project.org/?p=project/procd.git;a=blob;f=plug/hotplug.c;h=85959155634f1a6a9ada11c7045601885a0abad8;hb=72f63807b3644ef7b8ab9025a02d7f509f39ad14#l204>
> 
> [1] <https://git.lede-project.org/?p=project/procd.git;a=blob;f=plug/hotplug.c;h=85959155634f1a6a9ada11c7045601885a0abad8;hb=72f63807b3644ef7b8ab9025a02d7f509f39ad14#l347>
>

On Saturday, October 8, 2016 7:29:13 PM CEST Matthias Schiffer wrote:
> On 10/08/2016 07:13 PM, Christian Lamparter wrote:
> > On Saturday, October 8, 2016 6:04:09 PM CEST Matthias Schiffer wrote:
> >> On 10/08/2016 05:41 PM, Christian Lamparter wrote:
> >>> On Friday, October 7, 2016 8:29:30 PM CEST Matthias Schiffer wrote:
> >>>> On 10/07/2016 08:10 PM, Christian Lamparter wrote:
> >>>>> Currently, the wifi detection script is executed as part of
> >>>>> the (early) boot process. Pluggable wifi USB devices, which
> >>>>> are inserted at a later time are not automatically
> >>>>> detected and therefore they don't show up in LuCI.
> >>>>>
> >>>>> A user has to deal with wifi detection manually, or restart
> >>>>> the router.
> >>>>>
> >>>>> [...]
> >>>>> ---
> >>>>> We would like to hear, if these changes work with broadcom-wl.
> >>>>> (Felix removed the hostap, so this isn't included anymore).
> >>>>>
> >>>>> trap and lock are part of the default busybox setup.
> >>>>
> >>>> Hi,
> >>>> it would be great to remove the direct write of /etc/config/wireless
> >>>> completely, as it won't lock against other users of UCI that modify the
> >>>> wireless config. IMO, it should just use UCI to modify the configuration as
> >>>> everything else does.
> >>>
> >>
> >>> Well, What's the situation with ECE and configd? I didn't want to touch it
> >>> since you plan to move away from the config files and replace them with
> >>> json. 
> >>
> >> There isn't a concrete plan to integrate ECE with LEDE yet (there are still
> >> some TODOs, and it will need to be discussed further with the LEDE
> >> community). It will provide a bidirectional UCI mapping; this means as long
> >> as the uci CLI and similar tools are used, most things should just continue
> >> work with ECE.
> > Ok. But how would the /etc/config/wireless have worked? As it just produced a
> > file and bypassed all the uci CLI and similar tools. Did you setup file
> > notifications to check if something as modified in /etc/config/ run uci to
> > import it or sth. like that? (Because that was why I contacted you earlier :) )
> 
> Converting that script to the UCI CLI was still a TODO for making this
> work. So far, I haven't even finished the binding code itself (I've been
> working on other projects during the last weeks), and haven't had a closer
> look at all UCI users yet to look for potential issues. I plan to return to
> working on ECE soon.

Ok, so what do I get for that. I would be happy with a Reviewed-by / Tested-by
for this series :-).

> >>> Note: the "> /dev/null" for uci calls were added just in case someone still
> >>> has the old /etc/init.d/boot and to not write garbage into /e/c/wireless.
> >>>
> >>> Note2: I've also changed the "plug-and-play wifi" patch and removed the
> >>> /tmp/wireless.tmp step. But we still need proper locking. 
> >>> (That said, I would like to move the locking to the mac80211.sh / broadcom.sh
> >>> detect functions, is everyone fine with that?)
> >>
> >> Makes sense to me (maybe we can further factor out common parts of
> >> mac80211.sh and broadcom.sh?).
> > I don't think that's within the scope of this series :-D. Also, I don't have
> > any  broadcom devices... 
> > 
> >> Note that you don't need any locking in the hotplug handlers anyways, they are
> >> always run sequentially.
> > I moved the locking to detect_mac80211 and detect_broadcom. 
> > A user might still want to run "wifi detect" (out of habit?) and that
> > could race with hotplug event .
> > 
> > As for procd:
> > Can you tell me where this serialization happens or how it works?
> > 
> > I've looked into this earlier and found that procd uses the hotplug-call
> > in hotplug.json to run all the scripts under /etc/hotplug.d/%SUBSYSTEMS%.
> > 
> > I've looked up handle_exec (which is used to run hotplug-call)
> > and found execvp [0]. The thing with execvp is that it fully replaces
> > the current thread with the new program it is supposed to execute
> > (it never returns, unless it fails before it jumps to the new program).
> > So procd needs to fork() before it can run hotplug-call and this is
> > done in queue_next [1].
> > 
> > However, if procd is supposed to serialize these calls, it will
> > need to use some sort of wait() or waitpid() to wait for the
> > forked processes to finish. Since there is no wait()/waitpid() in 
> > there, the hotplug-call can run concurrently.
> 
> queue_proc_cb() is run as soon as the previous hotplug-call is finished
> (the wait is hidden behind uloop), which will then call queue_next() to
> handle the next hotplug event.
Ok, thanks.

I moved the locking to detect_mac80211 and detect_broadcom.
(I thought about moving it to /sbin/wifi.sh detect. But the driver's
detect routines have race-conditions and not the wifi code).
This should protect against the a user that manages to run the old
"wifi detect >> /e/c/wireless" at a "bad time".

For testing purposes, I added a long sleep after the "found" bail-out
in wifi detect to see what happens if the routine is called multiple
times concurrently. From what I can tell, it just adds more default
"wifi-ifaces" (SSID: LEDE, no encryption) for the same radio instance.

if some user manages to pull this off, several things could happen.

1. he/she makes a bugreport.

2. removes the duplicated instances.

3. ignores it, makes the changes he/she wants and enables the radio.
	(This of course will leave him/her with an open network).

i. (make your own scenario / aka. Write Prompt!)

Locking should prevent that nothing bad happens for case 1, 3, etc...
So, what do you say?

Regards,
Christian