Re: [PATCH 01/10] rsi: add support for hardware scan offload
Hi Kalle/Johannes, On Tue, Mar 27, 2018 at 7:48 PM, Kalle Valo wrote: > Johannes Berg writes: > >> On Fri, 2018-03-23 at 20:20 +0530, Amitkumar Karwar wrote: >> >>> > But maybe that's not really true at all? At least in one case it seems >>> > you just kick off something called "bgscan". >>> >>> Yes. We have different scan implementations for device is connected >>> and non-connected cases. In connected case, firmware will take care of >>> timings when driver configures bgscan parameters due to power save and >>> coex restrictions. In non-connected state, driver is taking care of >>> it. >>> I found hardware scan in mac80211 more suitable for our device. >> >> Yeah it's a bit odd though that you're still implementing software scan >> :-) >> >> Perhaps we could make a special return code from the hwscan callback >> that would tell mac80211 to fall back to software scanning, so you'd >> only implement the connected case, and leave the rest up to mac80211? > > Hehe, this is exactly what I proposed during my review :) > Sounds good. I will prepare a patch with this approach. Regards, Amitkumar
Re: [PATCH 01/10] rsi: add support for hardware scan offload
Johannes Berg writes: > On Fri, 2018-03-23 at 20:20 +0530, Amitkumar Karwar wrote: > >> > But maybe that's not really true at all? At least in one case it seems >> > you just kick off something called "bgscan". >> >> Yes. We have different scan implementations for device is connected >> and non-connected cases. In connected case, firmware will take care of >> timings when driver configures bgscan parameters due to power save and >> coex restrictions. In non-connected state, driver is taking care of >> it. >> I found hardware scan in mac80211 more suitable for our device. > > Yeah it's a bit odd though that you're still implementing software scan > :-) > > Perhaps we could make a special return code from the hwscan callback > that would tell mac80211 to fall back to software scanning, so you'd > only implement the connected case, and leave the rest up to mac80211? Hehe, this is exactly what I proposed during my review :) -- Kalle Valo
Re: [PATCH 01/10] rsi: add support for hardware scan offload
On Fri, 2018-03-23 at 20:20 +0530, Amitkumar Karwar wrote: > > But maybe that's not really true at all? At least in one case it seems > > you just kick off something called "bgscan". > > Yes. We have different scan implementations for device is connected > and non-connected cases. In connected case, firmware will take care of > timings when driver configures bgscan parameters due to power save and > coex restrictions. In non-connected state, driver is taking care of > it. > I found hardware scan in mac80211 more suitable for our device. Yeah it's a bit odd though that you're still implementing software scan :-) Perhaps we could make a special return code from the hwscan callback that would tell mac80211 to fall back to software scanning, so you'd only implement the connected case, and leave the rest up to mac80211? johannes
Re: [PATCH 01/10] rsi: add support for hardware scan offload
On Wed, Mar 21, 2018 at 4:02 AM, Johannes Berg wrote: > On Thu, 2018-03-15 at 14:57 +0530, Amitkumar Karwar wrote: >> >> > What I don't like here is that you are duplicating functionality >> > already >> > existing in mac80211 and I hope there is a better way to solve the >> > problem. Just as a a crazy idea what if the driver returns >> > -EOPNOTSUPP >> > when hardware scan is not possible and mac80211 falls back to >> > software >> > scan? But of course this depends on what Johannes thinks. >> >> Currently mac80211 offloads scan to driver if "ops->hw_scan" is >> implemented. Otherwise falls back to software scan. >> I can see below vendors have already implemented hw_scan with their >> own scan state machine. This patch does the same thing. >> Let me know if I missed anything here. > > I think the argument is that at least it looks like you're implementing > the timing etc. in software in the driver again, which others don't do, > they do it in firmware. Which is just software again, but we don't see > it ;-) Understood. Timing logic is either is hardware or mac80211 for others. > But maybe that's not really true at all? At least in one case it seems > you just kick off something called "bgscan". Yes. We have different scan implementations for device is connected and non-connected cases. In connected case, firmware will take care of timings when driver configures bgscan parameters due to power save and coex restrictions. In non-connected state, driver is taking care of it. I found hardware scan in mac80211 more suitable for our device. Regards, Amitkumar Karwar
Re: [PATCH 01/10] rsi: add support for hardware scan offload
On Thu, 2018-03-15 at 14:57 +0530, Amitkumar Karwar wrote: > > > What I don't like here is that you are duplicating functionality > > already > > existing in mac80211 and I hope there is a better way to solve the > > problem. Just as a a crazy idea what if the driver returns > > -EOPNOTSUPP > > when hardware scan is not possible and mac80211 falls back to > > software > > scan? But of course this depends on what Johannes thinks. > > Currently mac80211 offloads scan to driver if "ops->hw_scan" is > implemented. Otherwise falls back to software scan. > I can see below vendors have already implemented hw_scan with their > own scan state machine. This patch does the same thing. > Let me know if I missed anything here. I think the argument is that at least it looks like you're implementing the timing etc. in software in the driver again, which others don't do, they do it in firmware. Which is just software again, but we don't see it ;-) But maybe that's not really true at all? At least in one case it seems you just kick off something called "bgscan". johannes
Re: [PATCH 01/10] rsi: add support for hardware scan offload
On Thu, Mar 15, 2018 at 2:30 PM, Kalle Valo wrote: > Amitkumar Karwar writes: > >> On Tue, Mar 13, 2018 at 8:46 PM, Kalle Valo wrote: >>> Amitkumar Karwar writes: >>> From: Prameela Rani Garnepudi With the current approach of scanning, roaming delays are observed. Firmware has support for back ground scanning. To get this advantage, mac80211 hardware scan is implemented. In this method, foreground scan is performed in driver and back ground scan is configured to firmware. >>> >>> To me doesn't like a good idea to duplicate scan functionality in the >>> driver. >> >> There is a limitation with our device. We need to configure background >> scan parameters to firmware when device is connected. > > Yeah, I guessed that. > >> In non-connected state, we can directly dump probe requests received >> from mac80211 as a part of software scan. Some synchronization issues >> are with existing software scan when device is connected. This patch >> implements hw_scan where these issues are no seen, as driver has more >> control on scan state machine > > What I don't like here is that you are duplicating functionality already > existing in mac80211 and I hope there is a better way to solve the > problem. Just as a a crazy idea what if the driver returns -EOPNOTSUPP > when hardware scan is not possible and mac80211 falls back to software > scan? But of course this depends on what Johannes thinks. Currently mac80211 offloads scan to driver if "ops->hw_scan" is implemented. Otherwise falls back to software scan. I can see below vendors have already implemented hw_scan with their own scan state machine. This patch does the same thing. Let me know if I missed anything here. /ath/ath10k/mac.c:7684: .hw_scan = ath10k_hw_scan, ./ath/wcn36xx/main.c:1115: .hw_scan = wcn36xx_hw_scan, ./ath/ath9k/main.c:2626: ath9k_ops.hw_scan = ath9k_hw_scan; ./st/cw1200/main.c:215: .hw_scan = cw1200_hw_scan, ./atmel/at76c50x-usb.c:2195: .hw_scan = at76_hw_scan, ./ti/wl1251/main.c:1376: .hw_scan = wl1251_op_hw_scan, ./ti/wlcore/main.c:5923: .hw_scan = wl1271_op_hw_scan, ./intel/iwlegacy/3945-mac.c:3485: .hw_scan = il_mac_hw_scan, ./intel/iwlegacy/3945-mac.c:3915: il3945_mac_ops.hw_scan = NULL; ./intel/iwlegacy/4965-mac.c:6352: .hw_scan = il_mac_hw_scan, ./intel/iwlwifi/mvm/mac80211.c:4343: .hw_scan = iwl_mvm_mac_hw_scan, ./intel/iwlwifi/dvm/mac80211.c:1627: .hw_scan = iwlagn_mac_hw_scan, ./mac80211_hwsim.c:2390: .hw_scan = mac80211_hwsim_hw_scan, Regards, Amitkumar
Re: [PATCH 01/10] rsi: add support for hardware scan offload
Amitkumar Karwar writes: > On Tue, Mar 13, 2018 at 8:46 PM, Kalle Valo wrote: >> Amitkumar Karwar writes: >> >>> From: Prameela Rani Garnepudi >>> >>> With the current approach of scanning, roaming delays >>> are observed. Firmware has support for back ground scanning. >>> To get this advantage, mac80211 hardware scan is implemented. >>> In this method, foreground scan is performed in driver and >>> back ground scan is configured to firmware. >> >> To me doesn't like a good idea to duplicate scan functionality in the >> driver. > > There is a limitation with our device. We need to configure background > scan parameters to firmware when device is connected. Yeah, I guessed that. > In non-connected state, we can directly dump probe requests received > from mac80211 as a part of software scan. Some synchronization issues > are with existing software scan when device is connected. This patch > implements hw_scan where these issues are no seen, as driver has more > control on scan state machine What I don't like here is that you are duplicating functionality already existing in mac80211 and I hope there is a better way to solve the problem. Just as a a crazy idea what if the driver returns -EOPNOTSUPP when hardware scan is not possible and mac80211 falls back to software scan? But of course this depends on what Johannes thinks. -- Kalle Valo
Re: [PATCH 01/10] rsi: add support for hardware scan offload
On Tue, Mar 13, 2018 at 8:48 PM, Kalle Valo wrote: > Kalle Valo writes: > >> Amitkumar Karwar writes: >> >>> From: Prameela Rani Garnepudi >>> >>> With the current approach of scanning, roaming delays >>> are observed. Firmware has support for back ground scanning. >>> To get this advantage, mac80211 hardware scan is implemented. >>> In this method, foreground scan is performed in driver and >>> back ground scan is configured to firmware. >> >> To me doesn't like a good idea to duplicate scan functionality in the >> driver. > > Also a pro tip: Don't place controversial patches as the first patch in > a big patchset, instead put them last so that I can apply rest of > patches anyway. Even better to submit them separately as RFC. Got it. I will follow this. Regards, Amitkumar Karwar
Re: [PATCH 01/10] rsi: add support for hardware scan offload
On Tue, Mar 13, 2018 at 8:46 PM, Kalle Valo wrote: > Amitkumar Karwar writes: > >> From: Prameela Rani Garnepudi >> >> With the current approach of scanning, roaming delays >> are observed. Firmware has support for back ground scanning. >> To get this advantage, mac80211 hardware scan is implemented. >> In this method, foreground scan is performed in driver and >> back ground scan is configured to firmware. > > To me doesn't like a good idea to duplicate scan functionality in the > driver. There is a limitation with our device. We need to configure background scan parameters to firmware when device is connected. In non-connected state, we can directly dump probe requests received from mac80211 as a part of software scan. Some synchronization issues are with existing software scan when device is connected. This patch implements hw_scan where these issues are no seen, as driver has more control on scan state machine > >> --- a/drivers/net/wireless/rsi/rsi_91x_main.c >> +++ b/drivers/net/wireless/rsi/rsi_91x_main.c >> @@ -324,6 +324,14 @@ struct rsi_hw *rsi_91x_init(u16 oper_mode) >> mutex_init(&common->rx_lock); >> mutex_init(&common->tx_bus_mutex); >> >> + rsi_init_event(&common->chan_set_event); >> + rsi_init_event(&common->probe_cfm_event); >> + rsi_init_event(&common->chan_change_event); >> + rsi_init_event(&common->cancel_hw_scan_event); > > And I'm starting to dislike this rsi_init_event() even more (see my > other mail). In upstream driver's custom abstractions are very much > frowned upon, especially that it makes review harder. Agreed. I will get rid of this in a separate cleanup patch series. Regards, Amitkumar Karwar
Re: [PATCH 01/10] rsi: add support for hardware scan offload
Kalle Valo writes: > Amitkumar Karwar writes: > >> From: Prameela Rani Garnepudi >> >> With the current approach of scanning, roaming delays >> are observed. Firmware has support for back ground scanning. >> To get this advantage, mac80211 hardware scan is implemented. >> In this method, foreground scan is performed in driver and >> back ground scan is configured to firmware. > > To me doesn't like a good idea to duplicate scan functionality in the > driver. Also a pro tip: Don't place controversial patches as the first patch in a big patchset, instead put them last so that I can apply rest of patches anyway. Even better to submit them separately as RFC. -- Kalle Valo
Re: [PATCH 01/10] rsi: add support for hardware scan offload
Amitkumar Karwar writes: > From: Prameela Rani Garnepudi > > With the current approach of scanning, roaming delays > are observed. Firmware has support for back ground scanning. > To get this advantage, mac80211 hardware scan is implemented. > In this method, foreground scan is performed in driver and > back ground scan is configured to firmware. To me doesn't like a good idea to duplicate scan functionality in the driver. > --- a/drivers/net/wireless/rsi/rsi_91x_main.c > +++ b/drivers/net/wireless/rsi/rsi_91x_main.c > @@ -324,6 +324,14 @@ struct rsi_hw *rsi_91x_init(u16 oper_mode) > mutex_init(&common->rx_lock); > mutex_init(&common->tx_bus_mutex); > > + rsi_init_event(&common->chan_set_event); > + rsi_init_event(&common->probe_cfm_event); > + rsi_init_event(&common->chan_change_event); > + rsi_init_event(&common->cancel_hw_scan_event); And I'm starting to dislike this rsi_init_event() even more (see my other mail). In upstream driver's custom abstractions are very much frowned upon, especially that it makes review harder. -- Kalle Valo