| Message ID | F51492713EF10846800D8C0ED37A7DCE0194449E@SJEXCHMB15.corp.ad.broadcom.com |
|---|---|
| State | Rejected |
| Delegated to: | Felix Fietkau |
| Headers | show |
On 12 May 2015 at 10:35, Hante Meuleman <meuleman@broadcom.com> wrote: > The boot script gives 1 second for wifi modules to settle. For newer routers > > like R8000 which has three wifi devices which all need to be firmware > > downloaded, nvram downloaded and initialized this delay is getting very > > tight. On some bootups it was seen that not all wifi modules were > > initialized by the time the "wifi detect" command was executed. > > > > This patch increases the timeout from 1 to 2, leaving plenty of time for the > > wifi modules to initialize: Few comments: 1) Use prefix for your patch (see e.g. "git log package/base-files" for a reference) 2) Sign your patch(es) 3) *Don't* send HTML e-mails PLEASE
On 2015-05-12 10:35, Hante Meuleman wrote: > The boot script gives 1 second for wifi modules to settle. For newer > routers > > like R8000 which has three wifi devices which all need to be firmware > > downloaded, nvram downloaded and initialized this delay is getting very > > tight. On some bootups it was seen that not all wifi modules were > > initialized by the time the "wifi detect" command was executed. > > > > This patch increases the timeout from 1 to 2, leaving plenty of time for > the > > wifi modules to initialize: NACK from me. I think we should instead add an OpenWrt specifc patch to brcmfmac, similar to the one we have for ath10k, which makes device probing (and firmware loading) synchronous. The ath10k patch is 921-ath10k_init_devices_synchronously.patch - Felix
Understood, what is wifi detect using as input? Do the netdevs have to be up? Where is the information that wifi app is reading coming from? brcmfmac uses different method for firmware loading. It is not as easily patched as the ath10k driver. But I would like to know exactly what wifi detect uses as input. As in case of brcmfmac I expect that firmware loading will not be the only asynchronous "problem". What would be easy is adding a delay of 2 seconds to the function brcmfmac_module_init in core.c, but that won't guarantee it will work. -----Original Message----- From: Felix Fietkau [mailto:nbd@openwrt.org] On 2015-05-12 10:35, Hante Meuleman wrote: > The boot script gives 1 second for wifi modules to settle. For newer > routers > > like R8000 which has three wifi devices which all need to be firmware > > downloaded, nvram downloaded and initialized this delay is getting very > > tight. On some bootups it was seen that not all wifi modules were > > initialized by the time the "wifi detect" command was executed. > > > > This patch increases the timeout from 1 to 2, leaving plenty of time for > the > > wifi modules to initialize: NACK from me. I think we should instead add an OpenWrt specifc patch to brcmfmac, similar to the one we have for ath10k, which makes device probing (and firmware loading) synchronous. The ath10k patch is 921-ath10k_init_devices_synchronously.patch - Felix
On 12 May 2015 at 11:33, Hante Meuleman <meuleman@broadcom.com> wrote: > Understood, what is wifi detect using as input? Do the netdevs have to > be up? Where is the information that wifi app is reading coming from? What about just checking it by yourself? vim package/base-files/files/sbin/wifi vim package/kernel/mac80211/files/lib/wifi/mac80211.sh
On 2015-05-12 11:33, Hante Meuleman wrote: > Understood, what is wifi detect using as input? Do the netdevs have to > be up? Where is the information that wifi app is reading coming from? It looks for registered cfg80211 wiphys. It does not care about netdevs. > brcmfmac uses different method for firmware loading. It is not as > easily patched as the ath10k driver. But I would like to know exactly > what wifi detect uses as input. As in case of brcmfmac I expect that > firmware loading will not be the only asynchronous "problem". It should be enough to rework the request_firmware_nowait calls into request_firmware calls. > What would be easy is adding a delay of 2 seconds to the function > brcmfmac_module_init in core.c, but that won't guarantee it will work. Still seems more fragile and hackish. - Felix
Ok, thanks, that requires the wifi device to be completely up and initialized. There is way more involved in getting that synchronous. So what about the idea of adding a delay of two seconds to brcmfmac_module_init? Would that patch be accepted? -----Original Message----- From: Rafał Miłecki [mailto:zajec5@gmail.com] On 12 May 2015 at 11:33, Hante Meuleman <meuleman@broadcom.com> wrote: > Understood, what is wifi detect using as input? Do the netdevs have to > be up? Where is the information that wifi app is reading coming from? What about just checking it by yourself? vim package/base-files/files/sbin/wifi vim package/kernel/mac80211/files/lib/wifi/mac80211.sh
It is a bit more than just changing request_firmware_nowait into request_firmware. The worker in core.c needs to be removed. The function brcmf_pcie_setup needs to be updated as it cannot call device_release_driver during probe. As a result brcmf_fw_get_firmwares_pcie has to return the error, which means the api for brcmf_fw_get_firmwares_pcie will change, that will mean usb and sdio needs to patched as well. So it isn't going to be a small patch, but it can be done. I just wonder if it is worth the effort. The patch needs to be maintained as well. -----Original Message----- From: Felix Fietkau [mailto:nbd@openwrt.org] On 2015-05-12 11:33, Hante Meuleman wrote: > Understood, what is wifi detect using as input? Do the netdevs have to > be up? Where is the information that wifi app is reading coming from? It looks for registered cfg80211 wiphys. It does not care about netdevs. > brcmfmac uses different method for firmware loading. It is not as > easily patched as the ath10k driver. But I would like to know exactly > what wifi detect uses as input. As in case of brcmfmac I expect that > firmware loading will not be the only asynchronous "problem". It should be enough to rework the request_firmware_nowait calls into request_firmware calls. > What would be easy is adding a delay of 2 seconds to the function > brcmfmac_module_init in core.c, but that won't guarantee it will work. Still seems more fragile and hackish. - Felix
On 05/12/15 12:25, Hante Meuleman wrote: > It is a bit more than just changing request_firmware_nowait into > request_firmware. The worker in core.c needs to be removed. The > function brcmf_pcie_setup needs to be updated as it cannot call > device_release_driver during probe. As a result > brcmf_fw_get_firmwares_pcie has to return the error, which means > the api for brcmf_fw_get_firmwares_pcie will change, that will > mean usb and sdio needs to patched as well. So it isn't going to be > a small patch, but it can be done. I just wonder if it is worth the effort. > The patch needs to be maintained as well. I think it kinda sucks that a driver is restricted to use a kernel API because of some user-space app behaviour. So can we dive a bit deeper into that. Is the app detecting the wiphy instances dynamically (through /sys/class/ieee80211?) or does it have some expectation of the number of available wiphy instances. On the first run with OpenWRT it probably can not have such expectation so is that what we are trying to deal with here? Regards, Arend > -----Original Message----- > From: Felix Fietkau [mailto:nbd@openwrt.org] > > On 2015-05-12 11:33, Hante Meuleman wrote: >> Understood, what is wifi detect using as input? Do the netdevs have to >> be up? Where is the information that wifi app is reading coming from? > It looks for registered cfg80211 wiphys. It does not care about netdevs. > >> brcmfmac uses different method for firmware loading. It is not as >> easily patched as the ath10k driver. But I would like to know exactly >> what wifi detect uses as input. As in case of brcmfmac I expect that >> firmware loading will not be the only asynchronous "problem". > It should be enough to rework the request_firmware_nowait calls into > request_firmware calls. > >> What would be easy is adding a delay of 2 seconds to the function >> brcmfmac_module_init in core.c, but that won't guarantee it will work. > Still seems more fragile and hackish. > > - Felix > _______________________________________________ > openwrt-devel mailing list > openwrt-devel@lists.openwrt.org > https://lists.openwrt.org/cgi-bin/mailman/listinfo/openwrt-devel
On 15 May 2015 at 09:02, Arend van Spriel <arend@broadcom.com> wrote: > On 05/12/15 12:25, Hante Meuleman wrote: >> >> It is a bit more than just changing request_firmware_nowait into >> request_firmware. The worker in core.c needs to be removed. The >> function brcmf_pcie_setup needs to be updated as it cannot call >> device_release_driver during probe. As a result >> brcmf_fw_get_firmwares_pcie has to return the error, which means >> the api for brcmf_fw_get_firmwares_pcie will change, that will >> mean usb and sdio needs to patched as well. So it isn't going to be >> a small patch, but it can be done. I just wonder if it is worth the >> effort. >> The patch needs to be maintained as well. > > > I think it kinda sucks that a driver is restricted to use a kernel API > because of some user-space app behaviour. I agree. > Is the app detecting the wiphy instances dynamically (through > /sys/class/ieee80211?) or does it have some expectation of the number of > available wiphy instances. On the first run with OpenWRT it probably can not > have such expectation so is that what we are trying to deal with here? I pointed you to the responsible files. 1) /etc/init.d/boot http://git.openwrt.org/?p=openwrt.git;a=blob;f=package/base-files/files/etc/init.d/boot;hb=HEAD As you already noticed, it calls "wifi detect" 2) /sbin/wifi http://git.openwrt.org/?p=openwrt.git;a=blob;f=package/base-files/files/sbin/wifi;hb=HEAD It calls driver specific (e.g. mac80211 which is really a cfg80211) function to detect wifi. 3) /lib/wifi/mac80211.sh http://git.openwrt.org/?p=openwrt.git;a=blob;f=package/kernel/mac80211/files/lib/wifi/mac80211.sh;hb=HEAD It implements detect_mac80211 which uses /sys/class/ieee80211/ and generates wifi config
On 2015-05-15 09:02, Arend van Spriel wrote: > On 05/12/15 12:25, Hante Meuleman wrote: >> It is a bit more than just changing request_firmware_nowait into >> request_firmware. The worker in core.c needs to be removed. The >> function brcmf_pcie_setup needs to be updated as it cannot call >> device_release_driver during probe. As a result >> brcmf_fw_get_firmwares_pcie has to return the error, which means >> the api for brcmf_fw_get_firmwares_pcie will change, that will >> mean usb and sdio needs to patched as well. So it isn't going to be >> a small patch, but it can be done. I just wonder if it is worth the effort. >> The patch needs to be maintained as well. > > I think it kinda sucks that a driver is restricted to use a kernel API > because of some user-space app behaviour. So can we dive a bit deeper > into that. Is the app detecting the wiphy instances dynamically (through > /sys/class/ieee80211?) or does it have some expectation of the number of > available wiphy instances. On the first run with OpenWRT it probably can > not have such expectation so is that what we are trying to deal with here? Here's some context on this issue: At boot time, a script looks for cfg80211 interfaces and creates a default config. This needs to happen before the uci-defaults scripts run, as they are allowed to make changes to the default config of wifi interfaces. As far as I know, there is no way for a script or utility to block until all device probing actions have been completed. We don't have a fancy per-wifi-device defaults override, and we don't have any hotplug based config modifications yet (this might also lead to some interesting race conditions), so we currently rely on core devices having been probed after kernel modules are loaded. - Felix
On 05/15/15 21:28, Felix Fietkau wrote: > On 2015-05-15 09:02, Arend van Spriel wrote: >> On 05/12/15 12:25, Hante Meuleman wrote: >>> It is a bit more than just changing request_firmware_nowait into >>> request_firmware. The worker in core.c needs to be removed. The >>> function brcmf_pcie_setup needs to be updated as it cannot call >>> device_release_driver during probe. As a result >>> brcmf_fw_get_firmwares_pcie has to return the error, which means >>> the api for brcmf_fw_get_firmwares_pcie will change, that will >>> mean usb and sdio needs to patched as well. So it isn't going to be >>> a small patch, but it can be done. I just wonder if it is worth the effort. >>> The patch needs to be maintained as well. >> >> I think it kinda sucks that a driver is restricted to use a kernel API >> because of some user-space app behaviour. So can we dive a bit deeper >> into that. Is the app detecting the wiphy instances dynamically (through >> /sys/class/ieee80211?) or does it have some expectation of the number of >> available wiphy instances. On the first run with OpenWRT it probably can >> not have such expectation so is that what we are trying to deal with here? > Here's some context on this issue: > At boot time, a script looks for cfg80211 interfaces and creates a > default config. This needs to happen before the uci-defaults scripts > run, as they are allowed to make changes to the default config of wifi > interfaces. > As far as I know, there is no way for a script or utility to block until > all device probing actions have been completed. > We don't have a fancy per-wifi-device defaults override, and we don't > have any hotplug based config modifications yet (this might also lead to > some interesting race conditions), so we currently rely on core devices > having been probed after kernel modules are loaded. Hi Felix, Thanks for the explanation. Although it goes a bit further as you rely on the wiphy being registered during the probe and I suspect the probe should be done in PID=0 context, right? This is also not true for brcmfmac which schedules a work during module init and the work does the driver registration. Regards, Arend
diff --git a/package/base-files/files/etc/init.d/boot b/package/base-files/files/etc/init.d/boot index 6950130..534de53 100755 --- a/package/base-files/files/etc/init.d/boot +++ b/package/base-files/files/etc/init.d/boot @@ -40,7 +40,7 @@ boot() { /sbin/kmodloader # allow wifi modules time to settle - sleep 1 + sleep 2 /sbin/wifi detect > /tmp/wireless.tmp [ -s /tmp/wireless.tmp ] && {