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

[Christian Lamparter]

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