Re: [PATCH 01/10] rsi: add support for hardware scan offload

[Amitkumar Karwar]

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

[Kalle Valo]

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

[Johannes Berg]

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

[Amitkumar Karwar]

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

[Johannes Berg]

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

[Amitkumar Karwar]

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

[Kalle Valo]

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

[Amitkumar Karwar]

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

[Amitkumar Karwar]

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(>rx_lock);
>>   mutex_init(>tx_bus_mutex);
>>
>> + rsi_init_event(>chan_set_event);
>> + rsi_init_event(>probe_cfm_event);
>> + rsi_init_event(>chan_change_event);
>> + rsi_init_event(>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]

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

[Kalle Valo]

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(>rx_lock);
>   mutex_init(>tx_bus_mutex);
>  
> + rsi_init_event(>chan_set_event);
> + rsi_init_event(>probe_cfm_event);
> + rsi_init_event(>chan_change_event);
> + rsi_init_event(>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