* Re: [RFC 2/3] mac80211: Implement data xmit for 802.11 encap offload
From: Johannes Berg @ 2016-12-16 9:25 UTC (permalink / raw)
To: Thiagarajan, Vasanthakumar, nbd@nbd.name; +Cc: linux-wireless@vger.kernel.org
In-Reply-To: <58537DA3.1090901@qti.qualcomm.com>
On Fri, 2016-12-16 at 05:37 +0000, Thiagarajan, Vasanthakumar wrote:
> QCA988X does not have capability to configure vif specific decap
> mode. Encap mode is configurable per packet for all the ath10k based
> chips so this part should be fine to support per vif configuration.
Ok, that's good.
> Newer QCA chips like QCA9984, QCA4019, QCA9888 and QCA99X0 supports
> decap mode configuration per vif.
Also good.
> To reduce the complexity, we can probably make per vif encap/decap
> configuration mandatory to enable ethernet frame format, not sure how
> this will work with non-QCA capable hardware.
I don't know either, nobody else has talked about it yet :-)
Anyway, if we (for now) we can assume that TX can be constant 802.3
encap mode once enabled, we don't have to deal with the messy dynamic
switching you had there.
RX can switch more dynamically independent of all the mac80211 code
since it basically just means the driver calls one function or the
other - if everything is offloaded correctly there shouldn't really be
a difference.
Then also things like IEEE80211_CONF_CHANGE_80211_HDR_OFFL etc. can go
away entirely because you don't have to switch anything dynamically at
a global level. That will simplify everything a lot.
johannes
^ permalink raw reply
* Re: [RFC 1/3] mac80211: Add provision for 802.11 encap/decap offload
From: Johannes Berg @ 2016-12-16 9:30 UTC (permalink / raw)
To: Thiagarajan, Vasanthakumar; +Cc: linux-wireless@vger.kernel.org
In-Reply-To: <585273D4.7030100@qti.qualcomm.com>
On Thu, 2016-12-15 at 10:43 +0000, Thiagarajan, Vasanthakumar wrote:
> On Thursday 15 December 2016 02:46 PM, Johannes Berg wrote:
> >
> > > Drivers advertising this capability should also implement other
> > > functionalities which deal with 802.11 frame format like below
> > > - ADDBA/DELBA offload
> >
> > This shouldn't be necessary.
>
> Ok. Since driver/hw needs to implement Tx/Rx aggregation related
> functionalities I thought ADDBA/DELBA processing will be little
> important to mac80211. May be I'm missing something.
It needs to do the aggregation of the data flows, but I don't think the
control flows will need to be offloaded entirely? We'd need to have
some feedback mechanism for the SN used etc., so that might be up to
the driver to implement and might not be easy.
Anyway, I'm only suggesting to drop this from the documentation since
it doesn't seem strictly necessary, assuming the driver can report the
correct SSN in the callback all the A-MPDU setup should work more or
less as-is (except I think mac80211 won't buffer frames while doing it,
so also that would have to be done by the driver).
> > > - Powersave offload
> >
> > That's ambiguous - you do need to handle sleeping stations (and PS-
> > Poll/U-APSD) in AP mode in the device with this,
>
> I did not dig deep into PS requirement with ethernet frame format
> because frame control is not available to mac80211, so I thought to
> mention PS offload is a requirement. May be there is already an
> infrastructure in mac80211 to learn PS state of client which was
> notified in the current data frame (other than 802.11 frame control).
>
> but I don't see a deep
> > technical reason to require it for client mode. OTOH, client mode
> > is
> > almost always offloaded anyway.
>
> Ok.
Actually, come to think of it, the whole client-side powersave stuff is
so broken that I can't recommend using it - perhaps just clarify this
and say:
* AP: offloaded support for powersaving clients
* non-AP: offloaded support for powersave (if desired)
> > I think you may have forgotten one important item,
> >
> > - control port handling
>
> Hmmm, I'm getting WPA-PSK working with this. May be there are other
> control port handling which will not work with ethernet frame format?
I think I later saw control port handling in your 802.3 RX code - my
review at this point was tainted by some thoughts I had how I thought
it should work, but it's different :)
> > > + int (*get_vif_80211_hdr_offload)(struct ieee80211_hw
> > > *hw,
> > > + struct ieee80211_vif
> > > *vif,
> > > + bool is_4addr, bool
> > > *supported);
> >
> > Why are you not simply returning "supported"?
> >
> > I don't like passing the vif pointer here. At this point, the vif
> > pointer isn't known to the driver yet (through drv_add_interface)
> > so it's a dead pointer as far as the sequencing is concerned.
> >
> > Is there a possibility that drivers need to switch off ethernet
> > format handling entirely when an incompatible interface is added?
> > For example, if you add a mesh interface, is there a chance that
> > the AP interface might no longer be able to handle this?
>
> >
> > I'd hope this doesn't happen because I think that would
> be extremely
> > complicated to handle safely.
>
> Unfortunately "[RFC 2/3] mac80211: Implement data xmit for 802.11
> encap offload" tries to implement this but more likely buggy :(. You
> are right, it is very complex to get that right. May be we should not
> allow interface needing dynamic switch?
We discussed this over in the other part of the thread :)
johannes
^ permalink raw reply
* Re: [PATCH 2/4] cfg80211: Add new NL80211_CMD_SET_BTCOEX_PRIORITY to support BTCOEX
From: Johannes Berg @ 2016-12-16 9:37 UTC (permalink / raw)
To: Tamizh chelvam; +Cc: c_traja, linux-wireless, ath10k
In-Reply-To: <134cc8e58ecb804b6dda0137c4c37be8@codeaurora.org>
> > > is it fine to have as WIPHY_BTCOEX_BE_PREFERRED ?
> >
> > It's not really clear to me what you intend to do this - if it's
> > really support flags then you really should name those better.
> >
>
> This is support flags and it used by the driver to intimate driver
> supported frame type for the BTCOEX to cfg like
> "wiphy_wowlan_support_flags" implementation. Please suggest if this
> is ok ? I will be thankful if you can suggest a better one if this
> is not ok "WIPHY_BTCOEX_SUPPORTS_BE"
Well, I see a few things here:
1) does it even make sense to split it out per AC? wouldn't it be weird
if you supported this only for VO and BK, and not the others, or
something like that?
2) Wouldn't it make more sense to define this in nl80211 and just pass
the bitmap through to userspace? That would save quite a bit of netlink
mangling complexity.
3) I think the naming is confusing - "WIPHY_BTCOEX_SUPPORTS_BE_PREF" or
so might be more appropriate?
> Do you mean to say, sending a value from user space and parse that
> in the driver?
I was more thinking of the capability advertisement, but yeah, both
ways seems reasonable.
johannes
^ permalink raw reply
* Re: [RFC 3/3] mac80211: Add receive path for ethernet frame format
From: Johannes Berg @ 2016-12-16 9:14 UTC (permalink / raw)
To: Thiagarajan, Vasanthakumar; +Cc: linux-wireless@vger.kernel.org
In-Reply-To: <1481879623.27953.4.camel@sipsolutions.net>
> > > > + return;
> > > > +
> > > > +mic_fail:
> > > > + cfg80211_michael_mic_failure(sdata->dev, sta->addr,
> > > > + (status->flag &
> > > > RX_FLAG_MCAST)
> > > > ?
> > > > + NL80211_KEYTYPE_GROUP :
> > > > + NL80211_KEYTYPE_PAIRWISE,
> > > > + key ? key->conf.keyidx :
> > > > -1,
> > > > + NULL, GFP_ATOMIC);
> > >
> > > Do we really want to handle that inline here? The driver probably
> > > has a different check to even set RX_FLAG_MMIC_ERROR, so we could
> > > just ask it to call cfg80211_michael_mic_failure() [or a wrapper
> > > to
> > > get sdata->dev] instead? I guess this works too though, and might
> > > be easier to understand.
> >
> > Yeah, driver directly reporting MIC failure will be fine. I think a
> > wrapper may be required rather than mac80211 based driver directly
> > calling cfg80211 function?
>
> It would be, because the driver can't get sdata->dev (although I
> think there's now a hidden path to do this?)
However, we can do both ways, I don't really care that much. It seems
possible though that a driver would not even report the frame, but only
the necessary info, in this case - so that we might need an out-of-band
path for it anyway?
johannes
^ permalink raw reply
* Re: [PATCH v2 2/2] cfg80211: Add support to sched scan to report better BSSs
From: Johannes Berg @ 2016-12-16 9:56 UTC (permalink / raw)
To: Malinen, Jouni
Cc: Arend Van Spriel, Vamsi, Krishna, linux-wireless@vger.kernel.org
In-Reply-To: <20161215110635.GA11840@jouni.qca.qualcomm.com>
On Thu, 2016-12-15 at 11:06 +0000, Malinen, Jouni wrote:
> On Tue, Dec 13, 2016 at 04:56:55PM +0100, Johannes Berg wrote:
> > Regarding reusing attributes, we have (for the BSS selection thing)
> > the attribute NL80211_BSS_SELECT_ATTR_RSSI_ADJUST, which is really
> > quite similar to your
> > new NL80211_ATTR_SCHED_SCAN_RELATIVE_RSSI_5G_PREF since while
> > connected (which BSS_SELECT_ATTR_* assumes) the current BSS is
> > always part of the considered BSSes, I'd think.
>
> It seems to have somewhat similar meaning, but it looks more generic
> by not being tied to just two bands (2.4 and 5). And now that I
> actually looked again at this, it does not look like
> NL80211_BSS_SELECT_ATTR_RSSI_ADJUST actually allows more than a
> single band,delta pair to be provided and as such, it actually would
> not work very well with more than two bands even if it might be a bit
> more generic by allowing band to be set to something else than 2.4 or
> 5.
Agree, it wouldn't work well with more than 2 bands. Not that we have
them now (60GHz doesn't really count here).
> By the way, nl80211.h does not seem to document what values struct
> nl82011_bss_select_rssi_adjust band uses.. Is this enum
> nl80211_band?
I believe so, we should fix that.
> Neither does it say that delta is in dB (is it?).
We should also fix that, but let's assume so for the sake of this
discussion.
> > OTOH, the new NL80211_ATTR_SCHED_SCAN_RELATIVE_RSSI_5G_PREF doesn't
> > make this quite clear - is the current BSS to be adjusted before
> > comparing, if it's 5 GHz? If so, the semantics are equivalent. If
> > not, it doesn't actually make much sense ;-)
>
> Maybe the nl80211.h description was not clear enough, but the
> comments in cfg80211.h should be quite clear on how this was designed
> to work at the implementation level:
>
> "If the current connected BSS is in the 2.4 GHz band, other BSSs in
> the 2.4 GHz band to be reported should have better RSSI by
> @relative_rssi and other BSSs in the 5 GHz band to be reported should
> have better RSSI by (@relative_rssi - @relative_rssi_5g_pref).
> If the current connected BSS is in the 5 GHz band, other BSSs in the
> 2.4 GHz band to be reported should have better RSSI by
> (@relative_rssi + @relative_rssi_5g_pref) and other BSSs in the 5 GHz
> band to be reported should have better RSSI by by @relative_rssi."
Oh, right. Should probably be in nl80211.h too, to set expectations
from userspace.
> I'm not sure I'd describe this as adjusting the current BSS RSSI, but
> more like adjusting the RSSI threshold value if roaming would be from
> one band to another and doing that adjustment by adding or
> decrementing based on whether the roam would be from 2.4 to 5 or from
> 5 to 2.4.
It's functionally equivalent to doing the following adjustment
compare_rssi = current_bss_rssi
if current_bss_is_5ghz:
compare_rssi += relative_rssi_5g_pref
and then comparing all values (again adjusted for 5 GHz BSSes) to this.
(which is what I meant by "adjusting the current BSS RSSI").
> > So assuming that it is in fact taken into account after the same
> > adjustment, the two attributes are equivalent, and then perhaps it
> > would make sense to use struct nl80211_bss_select_rssi_adjust for
> > the new attribute. If a driver doesn't support arbitrary bands, but
> > just 5 GHz as in your example, it can just flip it around to 2.4
> > GHz by switching the sign.
> >
> > Perhaps we should even consider doing that in cfg80211 and
> > adjusting the internal API for both that way?
>
> I'm not completely sure I understood. One thing to note about
> differences here is that NL80211_BSS_SELECT_ATTR_* seems to be
> defining some preferences for BSS selection based on RSSI with an
> additional band preference, but it does not seem to define the
> threshold by how many dB the new candidate BSS should be better.
It's defining more than what we're talking about here, absolutely.
Clearly somebody thought a pure band preference might be useful (to
never connect to a 2.4GHz AP if a 5 GHz AP is available, regardless of
their relative RSSI), but that's not what we're looking at here.
(That could probably also be achieved by setting the adjustment to
90dB, but whatever)
> At minimum, we would need to clearly document struct
> nl80211_bss_select_rssi_adjust, but even if we do, I'm not sure it
> really is ideal mechanism to move to now that I realized it is not an
> array, but a single band,delta pair.
We can move to an array easily in the future by extending the attribute
length and advertising the number of array entries that are supported,
if that's your biggest concern? I don't see it as being very useful
right now since I don't think we'll see offloaded roaming between 2.4/5
and 60 GHz anytime soon. This may change when we add more bands later,
I suppose.
> Arend:
> > > I am not saying it should be avoided. Just looking at it
> > > conceptually
> > > the scheduled scan request holds so-called matchsets that specify
> > > the
> > > constraints to determine whether a BSS was found that is worth
> > > notifying the host/user-space about. As such I would expect the
> > > relative RSSI attribute(s) to be part of the matchset. That way
> > > you
> > > can specify it together with the currently connected SSID in a
> > > single
> > > matchset.
> >
> > I think this makes a lot of sense.
>
> If we are talking only about roaming within an ESS (a single SSID),
> that would sound clear, but which relative RSSI rules would apply if
> there are match sets for both the currently connected SSID and
> another SSID that the candidate BSS is for?
Right, I see how this might become a problem. I generally see no issue
with supporting multiple matchsets with different SSIDs but all having
the "relative to connected BSS RSSI filter" (which would disregard the
SSID specified in the matchset), but this then would become a problem
when multiple matchsets are specified with *different* such RSSI
filters, e.g. one matchset would specify that you want a 5G preference
of 10dB, but the other would specify a preference of only 5dB.
Conceptually in this approach, that would be supported, but firmware
likely would not be able to express this, I suppose?
> > We already have NL80211_SCHED_SCAN_MATCH_ATTR_RSSI, which asks to
> > be reporting only networks that have an *absolute* RSSI value above
> > the value of the attribute - a new attribute to make it relative to
> > the current network instead would make sense.
>
> When you say "current network", do you mean the current BSS? This
> gets complex when thinking about multiple SSIDs (which some call
> "networks") and there being NL80211_SCHED_SCAN_MATCH_ATTR_SSID and
> multiple match sets..
Yes, I did mean "current BSS". And yes - it does seem complex, see
above.
> > That would indeed be equivalent to NL80211_BSS_SELECT_ATTR_RSSI
> > then.
>
> NL80211_BSS_SELECT_ATTR_RSSI is a flag attribute.. BSS select
> mechanism does not provide an absolute RSSI value or threshold; it
> just seems to indicate use of RSSI-based selection mechanism without
> defining what exactly is better. There is
> NL80211_BSS_SELECT_ATTR_BAND_PREF that gives a preference to a
> specific band (without defining what that preference is) and then the
> NL80211_BSS_SELECT_ATTR_RSSI_ADJUST that can actually give a specific
> RSSI adjustment value (in dB?).
>
> > Now, if we consider this, NL80211_ATTR_SCHED_SCAN_RELATIVE_RSSI
> > actually is equivalent to NL80211_BSS_SELECT_ATTR_RSSI (a flag
> > attribute indicating whether or not RSSI-based selection/matching
> > is done) and NL80211_ATTR_SCHED_SCAN_RELATIVE_RSSI_5G_PREF is
> > equivalent to NL80211_BSS_SELECT_ATTR_RSSI_ADJUST, both need to be
> > given with the flag and affect operation.
>
> Hmm.. So you did notice it is a flag attribute.. So how would this
> match NL80211_ATTR_SCHED_SCAN_RELATIVE_RSSI which provides the
> threshold value for how many dB better the new BSS needs to be for it
> to be reported?
Yeah, umm, I think I got confused. There's no threshold in the
BSS_SELECT case, and maybe there doesn't need to be (at least not to
avoid host wakeups, though you'd want some hysteresis to avoid swapping
around too much).
> > So, how about we move these into NL80211_SCHED_SCAN_MATCH_ATTR_* as
> > suggested by Arend, and define them with the same content as the
> > corresponding NL80211_BSS_SELECT_ATTR_*?
>
> I think I'm missing something here.. Where would the threshold value
> (how much better new BSS needs to be) be stored? And do we really
> want something like the combination of
> NL80211_BSS_SELECT_ATTR_BAND_PREF and
> NL80211_BSS_SELECT_ATTR_RSSI_ADJUST which seems to be two different
> ways of doing band preference (the former without specifying delta
> and the latter with specific delta)? Or am I still not understanding
> how NL80211_BSS_SELECT_ATTR_* really works?
No, you're right, I missed the "better by" threshold.
I think between that (unless we add that, we could technically extend
flag attributes to allow them being an int as well, or add a new one)
and the fact that the device may not support different relative RSSI
matches in different matchsets, I'm almost convinced that adding new
attributes is better.
> The main concern I have for optional features with sched_scan is in
> whether the device ends up being woken up constantly if the driver
> does not understand a constraint that user space is trying to use to
> avoid being notified all the time.
Agree.
johannes
^ permalink raw reply
* Re: [RFC V3 03/11] nl80211: add support for gscan
From: Johannes Berg @ 2016-12-16 10:13 UTC (permalink / raw)
To: Arend Van Spriel; +Cc: linux-wireless
In-Reply-To: <f4f83c57-3e46-b017-fa36-b3776d71670a@broadcom.com>
On Wed, 2016-12-14 at 10:01 +0100, Arend Van Spriel wrote:
> Had to look for "> 16" ;-)
Sorry.
> Here an instance of the tab vs. space issue you mentioned. Will go
> over the patch and fix that.
There were a few, not really interesting though - git would probably
flag it anyway, or checkpatch :)
> > + if (num_chans > 16)
> > + return -EINVAL;
>
> I suspect this is the restriction you were referring to.
Yes.
> There is no
> reason for this although the android wifi hal has max 16 channels in
> a bucket so I might have picked that up.
I thought I saw something with a u16 bitmap that seemed related, but I
don't see that now so I'm probably just confused.
> So could a driver have a similar limit and should we add such to the
> gscan capabilities? For instance our firmware api has a nasty
> restriction of 64 channels for all buckets together, eg. can do 4
> buckets of 16 channels each.
We do have a limit of the maximum scan buckets, which seems to be 16
right now. We also have a limit on the number of channels per bucket,
which is also 16, but no combined limit afaict (so 16x16 seems fine).
Maybe we do need some advertisement in that area then? Right now,
wifihal seems to be able to read as capabilities the number of buckets
(wifi_gscan_capabilities), but assumes the number of channels:
const unsigned MAX_CHANNELS = 16;
const unsigned MAX_BUCKETS = 16;
I guess we took that and combined it, and you had more negotiation with
Google ;-)
We may then have to actually advertise the limit you have ("64 channels
combined over all buckets"), unless you can get away with just
advertising 4 buckets (and us saying 16 channels per bucket is enough?)
I'm a bit tempted to make this more forward compatible though and not
hard-limit the number of channels per bucket in the code.
johannes
^ permalink raw reply
* Re: [RFC v2 06/11] ath10k: htc: Added ATH10K_HTC_FLAG_BUNDLE_LSB
From: Valo, Kalle @ 2016-12-16 10:23 UTC (permalink / raw)
To: Erik Stromdahl; +Cc: linux-wireless@vger.kernel.org, ath10k@lists.infradead.org
In-Reply-To: <1479496971-19174-7-git-send-email-erik.stromdahl@gmail.com>
Erik Stromdahl <erik.stromdahl@gmail.com> writes:
> Signed-off-by: Erik Stromdahl <erik.stromdahl@gmail.com>
> ---
> drivers/net/wireless/ath/ath10k/htc.h | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/drivers/net/wireless/ath/ath10k/htc.h b/drivers/net/wireless=
/ath/ath10k/htc.h
> index f94b25a..2963694 100644
> --- a/drivers/net/wireless/ath/ath10k/htc.h
> +++ b/drivers/net/wireless/ath/ath10k/htc.h
> @@ -62,6 +62,8 @@ enum ath10k_htc_rx_flags {
> ATH10K_HTC_FLAG_BUNDLE_MASK =3D 0xF0
> };
> =20
> +#define ATH10K_HTC_FLAG_BUNDLE_LSB 4
> +
I think you could fold patches from 6 to 11 into one patch. They are all
about adding code, and most of the commit logs are empty anyway.
--=20
Kalle Valo=
^ permalink raw reply
* Re: wl1251 & mac address & calibration data
From: Pali Rohár @ 2016-12-16 10:26 UTC (permalink / raw)
To: Arend Van Spriel
Cc: Kalle Valo, Sebastian Reichel, Pavel Machek, Michal Kazior,
Ivaylo Dimitrov, Aaro Koskinen, Tony Lindgren, linux-wireless,
Network Development, linux-kernel, Luis R. Rodriguez
In-Reply-To: <76365770-f5ba-9565-3fca-710518f64d81@broadcom.com>
[-- Attachment #1: Type: Text/Plain, Size: 3509 bytes --]
On Thursday 15 December 2016 21:12:47 Arend Van Spriel wrote:
> On 15-12-2016 16:33, Pali Rohár wrote:
> > On Thu Dec 15 09:18:44 2016 Kalle Valo <kvalo@codeaurora.org> wrote:
> >> (Adding Luis because he has been working on request_firmware()
> >> lately)
> >>
> >> Pali Rohár <pali.rohar@gmail.com> writes:
> >>>>> So no, there is no argument against... request_firmware() in
> >>>>> fallback mode with userspace helper is by design blocking and
> >>>>> waiting for userspace. But waiting for some change in DTS in
> >>>>> kernel is just nonsense.
> >>>>
> >>>> I would just mark the wlan device with status = "disabled" and
> >>>> enable it in the overlay together with adding the NVS & MAC
> >>>> info.
> >>>
> >>> So if you think that this solution make sense, we can wait what
> >>> net wireless maintainers say about it...
> >>>
> >>> For me it looks like that solution can be:
> >>>
> >>> extending request_firmware() to use only userspace helper
> >>
> >> I haven't followed the discussion very closely but this is my
> >> preference what drivers should do:
> >>
> >> 1) First the driver should do try to get the calibration data and
> >> mac
> >>
> >> address from the device tree.
> >
> > Ok, but there is no (dynamic, device specific) data in DTS for
> > N900. So 1) is noop.
>
> Uhm. What do you mean? You can propose a patch to the DT bindings [1]
> to get it in there and create your N900 DTB or am I missing
> something here. Are there hardware restrictions that do not allow
> you to boot with your own DTB.
What is [1]?
N900's bootloader does not support DTB and it does not pass any DTB. It
boots kernel in ATAGs mode. What we are doing is appending DTB compiled
from kernel sources to end of zImage.
But that appended DTB cannot contains device specific nodes (e.g.
calibration data for device) as zImage is there for any N900 device, not
just only one.
> >> 2) If they are not in DT the driver should retrieve the
> >> calibration data
> >>
> >> with request_firmware(). BUT with an option for user space
> >> to implement that with a helper script so that the data
> >> can be created dynamically, which I believe openwrt does
> >> with ath10k calibration data right now.
> >
> > Currently there is flag for request_firmware() that it should
> > fallback to user helper if direct VFS access not find needed
> > firmware.
> >
> > But this flag is not suitable as /lib/firmware already provides
> > default (not device specific) calibration data.
> >
> > So I would suggest to add another flag/function which will primary
> > use user helper.
>
> I recall Luis saying that user-mode helper (fallback) should be
> discouraged, because there is no assurance that there is a user-mode
> helper so you might just be pissing in the wind. The idea was to have
> a dedicated API call that explicitly does the request towards
> user-space.
>
> By the way, are we talking here about wl1251 device or driver as you
> also mentioned wl12xx? I did not read the entire thread.
Yes, we are talking about wl1251 device, which is in Nokia N900 and
wl1251.ko and wl1251_spi.ko drivers.
I mentioned wl12xx as it already uses similar approach with mac address
via request_firmware(). And as those drivers are very similar plus from
one manufactor and in same kernel folder I mentioned similar API for
consistency...
--
Pali Rohár
pali.rohar@gmail.com
[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 198 bytes --]
^ permalink raw reply
* Re: wl1251 & mac address & calibration data
From: Pali Rohár @ 2016-12-16 10:35 UTC (permalink / raw)
To: Luis R. Rodriguez
Cc: Arend Van Spriel, Tom Gundersen, Daniel Wagner, Johannes Berg,
Ming Lei, Mimi Zohar, Bjorn Andersson, Rafał Miłecki,
Kalle Valo, Sebastian Reichel, Pavel Machek, Michal Kazior,
Ivaylo Dimitrov, Aaro Koskinen, Tony Lindgren, linux-wireless,
Network Development, linux-kernel@vger.kernel.org,
David Woodhouse, Takashi Iwai, Josh Boyer, Dmitry Torokhov
In-Reply-To: <CAB=NE6VHF+U7DzapENvLMKtN4Dgi9J0qMoeUJ8o4vSwc2-rRFQ@mail.gmail.com>
[-- Attachment #1: Type: Text/Plain, Size: 8476 bytes --]
On Friday 16 December 2016 03:03:19 Luis R. Rodriguez wrote:
> On Thu, Dec 15, 2016 at 2:12 PM, Arend Van Spriel
>
> <arend.vanspriel@broadcom.com> wrote:
> > On 15-12-2016 16:33, Pali Rohár wrote:
> >> On Thu Dec 15 09:18:44 2016 Kalle Valo <kvalo@codeaurora.org>
> >> wrote:
> >>> (Adding Luis because he has been working on request_firmware()
> >>> lately)
> >>>
> >>> Pali Rohár <pali.rohar@gmail.com> writes:
> >>>>>> So no, there is no argument against... request_firmware() in
> >>>>>> fallback mode with userspace helper is by design blocking and
> >>>>>> waiting for userspace. But waiting for some change in DTS in
> >>>>>> kernel is just nonsense.
> >>>>>
> >>>>> I would just mark the wlan device with status = "disabled" and
> >>>>> enable it in the overlay together with adding the NVS & MAC
> >>>>> info.
> >>>>
> >>>> So if you think that this solution make sense, we can wait what
> >>>> net wireless maintainers say about it...
> >>>>
> >>>> For me it looks like that solution can be:
> >>>>
> >>>> extending request_firmware() to use only userspace helper
> >>>
> >>> I haven't followed the discussion very closely but this is my
> >>> preference what drivers should do:
> >>>
> >>> 1) First the driver should do try to get the calibration data and
> >>> mac
> >>>
> >>> address from the device tree.
> >>
> >> Ok, but there is no (dynamic, device specific) data in DTS for
> >> N900. So 1) is noop.
> >
> > Uhm. What do you mean? You can propose a patch to the DT bindings
> > [1] to get it in there and create your N900 DTB or am I missing
> > something here. Are there hardware restrictions that do not allow
> > you to boot with your own DTB.
> >
> >>> 2) If they are not in DT the driver should retrieve the
> >>> calibration data
> >>>
> >>> with request_firmware(). BUT with an option for user space
> >>> to implement that with a helper script so that the data
> >>> can be created dynamically, which I believe openwrt does
> >>> with ath10k calibration data right now.
> >>
> >> Currently there is flag for request_firmware() that it should
> >> fallback to user helper if direct VFS access not find needed
> >> firmware.
> >>
> >> But this flag is not suitable as /lib/firmware already provides
> >> default (not device specific) calibration data.
> >>
> >> So I would suggest to add another flag/function which will primary
> >> use user helper.
> >
> > I recall Luis saying that user-mode helper (fallback) should be
> > discouraged, because there is no assurance that there is a
> > user-mode helper so you might just be pissing in the wind.
>
> There's tons of issues with the current status quo of the so called
> "firmware usermode helper". To start off with its a complete
> misnomer, the kernel's usermode helper infrastructure is implemented
> in lib/kmod.c and it provides an API to help call out to userspace
> some helper for you. That's the real kernel usermode helper. In so
> far as the firmware_class.c driver is concerned -- it only makes use
> of the kernel user mode helper as an option if you have
> CONFIG_UEVENT_HELPER_PATH set to help address kobject uevents. Most
> distributions do not use this, and back in the day systemd udev, and
> prior to that hotplug used to process firmware kobject uevents.
> systemd udev is long gone. Gone. This kobject uevents really are a
> "fallback mechanism" for firmware only -- if cached firmware,
> built-in firmware, or direct filesystem lookup fails. These each are
> their own beast. Finally kobject uevents are only one of the
> fallback
> mechanisms, "custom fallback mechanisms" are the other option and its
> what you and others seem to describe to want for these sorts of
> custom things.
>
> There are issues with the existing request_firmware() API in that
> everyone and their mother keeps extending it with stupid small API
> extensions to do yet-another-tweak, and then having to go and change
> tons of drivers. Or a new API call added for just one custom knob.
> Naturally this is all plain dumb, so yet-another-API call or new
> argument is not going to help us. We don't have "flags" persi in this
> API, that was one issue with the design of the API, but there are
> much more. The entire change in mechanism of "firmware fallback
> mechanism" depending on which kernel config options you have is
> another. Its a big web of gum put together. All this needs to end.
>
> I've recently proposed a new patch to first help with understanding
> each of the individual items I've listed above with as much new
> information as is possible. I myself need to re-read it to just keep
> tabs on what the hell is going on at times. After this I have a new
> API proposal which I've been working on for a long time now which
> adds an extensible API with only 2 calls: sync, async, and lets us
> modify attributes through a parameters struct. This is what we
> should use in the future for further extensions.
>
> For the new API a solution for "fallback mechanisms" should be clean
> though and I am looking to stay as far as possible from the existing
> mess. A solution to help both the old API and new API is possible for
> the "fallback mechanism" though -- but for that I can only refer you
> at this point to some of Daniel Wagner and Tom Gunderson's firmwared
> deamon prospect. It should help pave the way for a clean solution and
> help address other stupid issues.
>
> I'll note -- the "custom fallback mechanism", part of the old API is
> just a terrible idea for many reasons. I've documented issues in my
> documentation revamp, I suggest to read that, refer to this thread:
>
> https://lkml.kernel.org/r/20161213030828.17820-4-mcgrof@kernel.org
>
> > The idea was to have a
> > dedicated API call that explicitly does the request towards
> > user-space.
>
> In so far as this driver example that was mentioned in this thread my
> recommendation is to use the default existing MAC address /
> calibration data on /lib/firmware/ and then use another request to
> help override for the custom thing. The only issue of course is that
> the timeout for the custom thing is 60 seconds, but if your platforms
> are controlled -- just reduce this to 1 second or so given that udev
> is long gone and it would seem you'd only use a custom fw request to
> depend on. You could also wrap this thing into a kconfig option for
> now, and have the filename specified, if empty then no custom request
> is sent. If set then you have it request it.
>
> Please note the other patches in my series for the custom fallback
> though. We have only 2 users upstream -- and from recent discussions
> it would seem it might be possible to address what you need with
> firmwared in a clean way without this custom old fallback crap thing.
> Johannes at least has a similar requirement and is convinced a
> "custom" thing can be done without even adding an extra custom
> kobject uevent attribute as from what I recall he hinted, drivers
> have no business to be involved in this. If you do end up using the
> custom fallback mechanism be sure to add super crystal clear
> documentation for it (see my other patches for the declarer for this
> documentation). Since we have only 2 drivers using this custom thing
> its hard to get folks to commit to writing the docs, write it now
> and be sure you think of the future as well.
>
> Oh also the "custom firmware fallback" was also broken on recent
> kernels for many kernel revisions, it just did not work until
> recently a fix which went in, so your users wills need this fix
> cherry picked. Hey I tell you, the custom fw thing was a terrible
> incarnation. Only 2 drivers use it. There are good reasons to not
> like it (be sure to read the suspend issue I documented).
>
> > By the way, are we talking here about wl1251 device or driver as
> > you also mentioned wl12xx? I did not read the entire thread.
>
> Luis
Hi Luis! wl1251 already uses request_firmware for loading calibration
data from VFS. And because /lib/firmware/ contains preinstalled default
calibration data it uses it -- which is wrong as wireless has bad
quality.
In my setup I'm going to use udev with firmware loading support
(probably fork eudev), so it should be systemd-independent.
--
Pali Rohár
pali.rohar@gmail.com
[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 198 bytes --]
^ permalink raw reply
* Re: [RFC V3 04/11] nl80211: add driver api for gscan notifications
From: Johannes Berg @ 2016-12-16 10:02 UTC (permalink / raw)
To: Arend Van Spriel; +Cc: linux-wireless
In-Reply-To: <69ec7e9f-cd46-c46d-140c-0b30343cc0f7@broadcom.com>
> Not sure what is meant by "through the buckets".
TBH, I was handwaving because I don't understand this part of gscan
well :-)
> Referring to your
> remark/question in the "Unversal scan proposal" thread:
>
> """
> I'm much more worried about the "bucket reporting" since that doesn't
> fit into the current full BSS reporting model at all. What's your
> suggestion for this?
> """
>
> So this is exactly the dilemma I considered so I decided to stick
> with the full BSS reporting model for gscan as well merely to get it
> discussed so glad you brought it up ;-).
Heh.
Ok, so I missed that. Somehow I thought hidden in the buckets was a
partial reporting :-)
> The problem here is that gscan is a vehicle that serves a number of
> use-cases. So ignoring hotlists, ePNO, etc. the gscan configuration
> still hold several notification types:
>
> - report after completing M scans capturing N best APs or a
> percentage of (M * N).
> - report after completing a scan include a specific bucket.
> - report full scan results.
>
> The first two notification trigger retrieval of gscan results which
> are historic, ie. partial scan results for max M scans.
>
> As said earlier the universal scan proposal has some similarities to
> gscan. Guess you share that as you are using the term "bucket
> reporting" in that discussion ;-). The historic results are needed
> for location (if I am not mistaken) so the full BSS reporting model
> does not fit that. Question is what particular attribute in the
> historic results is needed for location (suspecting only rssi and
> possibly the timestamp, but would like to see that confirmed). I was
> thinking about have historic storage in cfg80211 so we do not need a
> per-driver solution.
Ok, now I'm starting to understand this better, I guess.
As far as I can tell from our code, for cached results we're reporting
the following data:
* some flags
* a scan ID
* and for each AP:
* RSSI
* timestamp
* channel
* BSSID
* SSID (which internally we even have a separate table for and each
AP just has an index to it, to save memory I guess)
* beacon period
* capability field
No IEs and similar things like differentiating probe response/beacon,
so we can't use the full reporting for this.
I have no problem introducing a common storage for this, if necessary
with some fields/nl attributes being optional, but I suspect this is
actually a necessary part of gscan, otherwise you're not able to report
all the necessary data?
johannes
^ permalink raw reply
* Re: wl1251 & mac address & calibration data
From: Pali Rohár @ 2016-12-16 10:40 UTC (permalink / raw)
To: Daniel Wagner
Cc: Luis R. Rodriguez, Arend Van Spriel, Tom Gundersen, Johannes Berg,
Ming Lei, Mimi Zohar, Bjorn Andersson, Rafał Miłecki,
Kalle Valo, Sebastian Reichel, Pavel Machek, Michal Kazior,
Ivaylo Dimitrov, Aaro Koskinen, Tony Lindgren, linux-wireless,
Network Development, linux-kernel@vger.kernel.org,
David Woodhouse, Takashi Iwai, Josh Boyer, Dmitry Torokhov
In-Reply-To: <07be7fc8-6c7f-6e03-379d-fa781b248259@monom.org>
[-- Attachment #1: Type: Text/Plain, Size: 2061 bytes --]
On Friday 16 December 2016 08:25:44 Daniel Wagner wrote:
> On 12/16/2016 03:03 AM, Luis R. Rodriguez wrote:
> > For the new API a solution for "fallback mechanisms" should be
> > clean though and I am looking to stay as far as possible from the
> > existing mess. A solution to help both the old API and new API is
> > possible for the "fallback mechanism" though -- but for that I can
> > only refer you at this point to some of Daniel Wagner and Tom
> > Gunderson's firmwared deamon prospect. It should help pave the way
> > for a clean solution and help address other stupid issues.
>
> The firmwared project is hosted here
>
> https://github.com/teg/firmwared
>
> As Luis pointed out, firmwared relies on
> FW_LOADER_USER_HELPER_FALLBACK, which is not enabled by default.
I know. But it does not mean that I cannot enable this option at kernel
compile time.
Bigger problem is that currently request_firmware() first try to load
firmware directly from VFS and after that (if fails) fallback to user
helper.
So I would need to extend kernel firmware code with new function (or
flag) to not use VFS and try only user mode helper.
> I
> don't see any reason why firmwared should not also support loading
> calibration data. If we find a sound way to do this.
It can, but why should I use another daemon for firmware loading as
non-systemd version of udev (and eudev fork) support firmware loading?
I think I stay with udev/eudev.
> As you can see from the commit history it is a pretty young project
> and more ore less reanimation of the old udev firmware loader
> feature. We are getting int into shape, adding integration tests
> etc.
>
> The main motivation for this project is the get movement back in
> stuck discussion on the firmware loader API. Luis was very busy
> writing up all the details on the current situation and purely from
> the amount of documentation need to describe the API you can tell
> something is awry.
>
> Thanks,
> Daniel
--
Pali Rohár
pali.rohar@gmail.com
[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 198 bytes --]
^ permalink raw reply
* wl1251 NVS calibration data format
From: Pali Rohár @ 2016-12-16 11:01 UTC (permalink / raw)
To: Gery Kahn, Shahar Lev, Kalle Valo, linux-wireless, linux-kernel,
Sebastian Reichel, Pavel Machek, Ivaylo Dimitrov
[-- Attachment #1: Type: Text/Plain, Size: 831 bytes --]
Hi! Do you know format of wl1251 NVS calibration data file?
I found that there is tool for changing NVS file for wl1271 and newer
chips (so not for wl1251!) at: https://github.com/gxk/ti-utils
And wl1271 has in NVS data already place for MAC address. And in wlcore
(for wl1271 and newer) there is really kernel code which is doing
something with MAC address in NVS, see:
https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/drivers/net/wireless/ti/wlcore/boot.c#n352
So... I would like to know if in wl1251 NVS calibration file is also
some place for MAC address or not.
Default wl1251 NVS calibration file is available in linux-firmware:
https://git.kernel.org/cgit/linux/kernel/git/firmware/linux-firmware.git/tree/ti-connectivity/wl1251-nvs.bin
--
Pali Rohár
pali.rohar@gmail.com
[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 198 bytes --]
^ permalink raw reply
* Re: wl1251 NVS calibration data format
From: Pali Rohár @ 2016-12-16 11:12 UTC (permalink / raw)
To: Gery Kahn, Shahar Lev, Kalle Valo, linux-wireless, linux-kernel,
Sebastian Reichel, Pavel Machek, Ivaylo Dimitrov
In-Reply-To: <201612161201.48356@pali>
[-- Attachment #1: Type: Text/Plain, Size: 1133 bytes --]
Resending email to new Gery's address...
On Friday 16 December 2016 12:01:48 Pali Rohár wrote:
> Hi! Do you know format of wl1251 NVS calibration data file?
>
> I found that there is tool for changing NVS file for wl1271 and newer
> chips (so not for wl1251!) at: https://github.com/gxk/ti-utils
>
> And wl1271 has in NVS data already place for MAC address. And in wlcore
> (for wl1271 and newer) there is really kernel code which is doing
> something with MAC address in NVS, see:
> https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/drivers/net/wireless/ti/wlcore/boot.c#n352
Also, there is parsing MAC address from NVS wl1271 data:
https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/drivers/net/wireless/ti/wlcore/main.c#n6009
> So... I would like to know if in wl1251 NVS calibration file is also
> some place for MAC address or not.
>
> Default wl1251 NVS calibration file is available in linux-firmware:
> https://git.kernel.org/cgit/linux/kernel/git/firmware/linux-firmware.git/tree/ti-connectivity/wl1251-nvs.bin
--
Pali Rohár
pali.rohar@gmail.com
[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 198 bytes --]
^ permalink raw reply
* Re: [RFC v2 11/11] ath10k: Added sdio support
From: Valo, Kalle @ 2016-12-16 11:21 UTC (permalink / raw)
To: Erik Stromdahl; +Cc: linux-wireless@vger.kernel.org, ath10k@lists.infradead.org
In-Reply-To: <1479496971-19174-12-git-send-email-erik.stromdahl@gmail.com>
Erik Stromdahl <erik.stromdahl@gmail.com> writes:
> Initial HIF sdio/mailbox implementation.
>
> Signed-off-by: Erik Stromdahl <erik.stromdahl@gmail.com>
I know most of this coming from ath6kl but I think we should still
improve the code. Lots of comments will follow, don't get scared :)
> +#define CALC_TXRX_PADDED_LEN(ar_sdio, len) \
> + (__ALIGN_MASK((len), (ar_sdio)->mbox_info.block_mask))
I think this could be a proper static inline function. Andrew Morton
once said: "Write in C, not CPP" (or something like that), I think
that's a really good point.
> +static int ath10k_sdio_read_write_sync(struct ath10k *ar, u32 addr, u8 *=
buf,
> + u32 len, u32 request);
> +static int ath10k_sdio_hif_diag_read(struct ath10k *ar, u32 address, voi=
d *buf,
> + size_t buf_len);
> +static int ath10k_sdio_hif_diag_read32(struct ath10k *ar, u32 address,
> + u32 *value);
We prefer to avoid forward declarations if at all possible. I didn't
check but if there's a clean way to avoid these please remove them.
> +/* HIF mbox interrupt handling */
> +
> +static int ath10k_sdio_mbox_rx_process_packet(struct ath10k_sdio *ar_sdi=
o,
> + struct ath10k_sdio_rx_data *pkt,
> + u32 *lookaheads,
> + int *n_lookaheads)
> +{
So the style in ath10k is that all functions (of course this doesn't
apply to callbacks etc) have "struct ath10k *ar" as the first parameter.
This way there's no need to check if a function takes ar or ar_sdio.
> + int status =3D 0;
In ath10k we prefer to use ret. And avoid initialising it, please.
> + struct ath10k_htc *htc =3D &ar_sdio->ar->htc;
> + struct sk_buff *skb =3D pkt->skb;
> + struct ath10k_htc_hdr *htc_hdr =3D (struct ath10k_htc_hdr *)skb->data;
> + bool trailer_present =3D htc_hdr->flags & ATH10K_HTC_FLAG_TRAILER_PRESE=
NT;
> + u16 payload_len;
> +
> + payload_len =3D le16_to_cpu(htc_hdr->len);
> +
> + if (trailer_present) {
> + u8 *trailer;
> + enum ath10k_htc_ep_id eid;
> +
> + trailer =3D skb->data + sizeof(struct ath10k_htc_hdr) +
> + payload_len - htc_hdr->trailer_len;
> +
> + eid =3D (enum ath10k_htc_ep_id)htc_hdr->eid;
A some kind of mapping function for ep id would be nice, it makes it
more visible how it works.
> +static int ath10k_sdio_mbox_rx_process_packets(struct ath10k_sdio *ar_sd=
io,
> + u32 lookaheads[],
> + int *n_lookahead)
> +{
> + struct ath10k *ar =3D ar_sdio->ar;
> + struct ath10k_htc *htc =3D &ar->htc;
> + struct ath10k_sdio_rx_data *pkt;
> + int status =3D 0, i;
> +
> + for (i =3D 0; i < ar_sdio->n_rx_pkts; i++) {
> + struct ath10k_htc_ep *ep;
> + enum ath10k_htc_ep_id id;
> + u32 *lookaheads_local =3D lookaheads;
> + int *n_lookahead_local =3D n_lookahead;
> +
> + id =3D ((struct ath10k_htc_hdr *)&lookaheads[i])->eid;
> +
> + if (id >=3D ATH10K_HTC_EP_COUNT) {
> + ath10k_err(ar, "Invalid endpoint in look-ahead: %d\n",
> + id);
In ath10k we use ath10k_err() for errors from which can't survive and
ath10k_warn() for errors where we try to continue. So ath10k_warn()
would be more approriate here and most of other cases in sdio.c
> + status =3D -ENOMEM;
> + goto out;
> + }
> +
> + ep =3D &htc->endpoint[id];
> +
> + if (ep->service_id =3D=3D 0) {
> + ath10k_err(ar, "ep %d is not connected !\n", id);
> + status =3D -ENOMEM;
> + goto out;
> + }
> +
> + pkt =3D &ar_sdio->rx_pkts[i];
> +
> + if (pkt->part_of_bundle && !pkt->last_in_bundle) {
> + /* Only read lookahead's from RX trailers
> + * for the last packet in a bundle.
> + */
> + lookaheads_local =3D NULL;
> + n_lookahead_local =3D NULL;
> + }
> +
> + status =3D ath10k_sdio_mbox_rx_process_packet(ar_sdio,
> + pkt,
> + lookaheads_local,
> + n_lookahead_local);
> + if (status)
> + goto out;
> +
> + ep->ep_ops.ep_rx_complete(ar_sdio->ar, pkt->skb);
> + /* The RX complete handler now owns the skb...*/
> + pkt->skb =3D NULL;
> + pkt->alloc_len =3D 0;
> + }
> +
> +out:
> + /* Free all packets that was not passed on to the RX completion
> + * handler...
> + */
> + for (; i < ar_sdio->n_rx_pkts; i++)
> + ath10k_sdio_mbox_free_rx_pkt(&ar_sdio->rx_pkts[i]);
I got a bit fooled by not initialising i here and only then realised
why. I guess it's ok but a bit of so and so
> +
> + return status;
> +}
> +
> +static int alloc_pkt_bundle(struct ath10k *ar,
> + struct ath10k_sdio_rx_data *rx_pkts,
> + struct ath10k_htc_hdr *htc_hdr,
> + size_t full_len, size_t act_len, size_t *bndl_cnt)
> +{
> + int i, status =3D 0;
> +
> + *bndl_cnt =3D (htc_hdr->flags & ATH10K_HTC_FLAG_BUNDLE_MASK) >>
> + ATH10K_HTC_FLAG_BUNDLE_LSB;
We recently got FIELD_GET() and FIELD_PREP() to kernel for handling
bitmasks. ath10k is not yet converted (patches welcome!) but it would be
good to use those already in sdio.c. Also SM() could be replaced with
those.
> +int ath10k_sdio_mbox_rxmsg_pending_handler(struct ath10k_sdio *ar_sdio,
> + u32 msg_lookahead, bool *done)
> +{
> + struct ath10k *ar =3D ar_sdio->ar;
> + int status =3D 0;
> + u32 lookaheads[ATH10K_SDIO_MAX_RX_MSGS];
> + int n_lookaheads =3D 1;
> +
> + *done =3D true;
> +
> + /* Copy the lookahead obtained from the HTC register table into our
> + * temp array as a start value.
> + */
> + lookaheads[0] =3D msg_lookahead;
> +
> + for (;;) {
Iternal loops in kernel can be dangerous. Better to add some sort of
timeout check with a warning message, something like:
while ((time_before(jiffies, timeout)) {
}
if (timed out)
ath10k_warn("timeout in foo");
> + /* Try to allocate as many HTC RX packets indicated by
> + * n_lookaheads.
> + */
> + status =3D ath10k_sdio_mbox_rx_alloc(ar_sdio, lookaheads,
> + n_lookaheads);
> + if (status)
> + break;
> +
> + if (ar_sdio->n_rx_pkts >=3D 2)
> + /* A recv bundle was detected, force IRQ status
> + * re-check again.
> + */
> + *done =3D false;
> +
> + status =3D ath10k_sdio_mbox_rx_fetch(ar_sdio);
> +
> + /* Process fetched packets. This will potentially update
> + * n_lookaheads depending on if the packets contain lookahead
> + * reports.
> + */
> + n_lookaheads =3D 0;
> + status =3D ath10k_sdio_mbox_rx_process_packets(ar_sdio,
> + lookaheads,
> + &n_lookaheads);
> +
> + if (!n_lookaheads || status)
> + break;
> +
> + /* For SYNCH processing, if we get here, we are running
> + * through the loop again due to updated lookaheads. Set
> + * flag that we should re-check IRQ status registers again
> + * before leaving IRQ processing, this can net better
> + * performance in high throughput situations.
> + */
> + *done =3D false;
> + }
> +
> + if (status && (status !=3D -ECANCELED))
> + ath10k_err(ar, "failed to get pending recv messages: %d\n",
> + status);
> +
> + if (atomic_read(&ar_sdio->stopping)) {
> + ath10k_warn(ar, "host is going to stop. Turning of RX\n");
> + ath10k_sdio_hif_rx_control(ar_sdio, false);
> + }
I'm always skeptic when I use atomic variables used like this, I doubt
it's really correct.
> +
> + return status;
> +}
> +
> +static int ath10k_sdio_mbox_proc_dbg_intr(struct ath10k_sdio *ar_sdio)
> +{
> + int ret;
> + u32 dummy;
> + struct ath10k *ar =3D ar_sdio->ar;
> +
> + ath10k_warn(ar, "firmware crashed\n");
We have firmware crash dump support in ath10k. You could add a "TODO:"
comment about implementing that later.
> +static int ath10k_sdio_mbox_proc_err_intr(struct ath10k_sdio *ar_sdio)
> +{
> + int status;
> + u8 error_int_status;
> + u8 reg_buf[4];
> + struct ath10k_sdio_irq_data *irq_data =3D &ar_sdio->irq_data;
> + struct ath10k *ar =3D ar_sdio->ar;
> +
> + ath10k_dbg(ar, ATH10K_DBG_SDIO, "error interrupt\n");
> +
> + error_int_status =3D irq_data->irq_proc_reg.error_int_status & 0x0F;
> + if (!error_int_status) {
> + WARN_ON(1);
> + return -EIO;
> + }
> +
> + ath10k_dbg(ar, ATH10K_DBG_SDIO,
> + "valid interrupt source(s) in ERROR_INT_STATUS: 0x%x\n",
> + error_int_status);
> +
> + if (MS(error_int_status, MBOX_ERROR_INT_STATUS_WAKEUP))
> + ath10k_dbg(ar, ATH10K_DBG_SDIO, "error : wakeup\n");
> +
> + if (MS(error_int_status, MBOX_ERROR_INT_STATUS_RX_UNDERFLOW))
> + ath10k_err(ar, "rx underflow\n");
> +
> + if (MS(error_int_status, MBOX_ERROR_INT_STATUS_TX_OVERFLOW))
> + ath10k_err(ar, "tx overflow\n");
> +
> + /* Clear the interrupt */
> + irq_data->irq_proc_reg.error_int_status &=3D ~error_int_status;
> +
> + /* set W1C value to clear the interrupt, this hits the register first *=
/
> + reg_buf[0] =3D error_int_status;
> + reg_buf[1] =3D 0;
> + reg_buf[2] =3D 0;
> + reg_buf[3] =3D 0;
> +
> + status =3D ath10k_sdio_read_write_sync(ar,
> + MBOX_ERROR_INT_STATUS_ADDRESS,
> + reg_buf, 4, HIF_WR_SYNC_BYTE_FIX);
> +
> + WARN_ON(status);
This is a bit dangerous, in worst case it can spam the kernel log and
force a host reboot due watchdog timing out etc.
Better to replace with standard warning message:
ret =3D ath10k_sdio_read_write_sync(ar,
MBOX_ERROR_INT_STATUS_ADDRESS,
reg_buf, 4, HIF_WR_SYNC_BYTE_FIX);
if (ret) {
ath10k_warn("failed to read interrupr status: %d", ret);
return ret;
}
> +static int ath10k_sdio_mbox_proc_cpu_intr(struct ath10k_sdio *ar_sdio)
> +{
> + int status;
> + struct ath10k_sdio_irq_data *irq_data =3D &ar_sdio->irq_data;
> + struct ath10k *ar =3D ar_sdio->ar;
> + u8 cpu_int_status, reg_buf[4];
> +
> + cpu_int_status =3D irq_data->irq_proc_reg.cpu_int_status &
> + irq_data->irq_en_reg.cpu_int_status_en;
> + if (!cpu_int_status) {
> + WARN_ON(1);
> + return -EIO;
> + }
Ditto about WARN_ON(), ath10k_warn() is much better.
> +/* process pending interrupts synchronously */
> +static int ath10k_sdio_mbox_proc_pending_irqs(struct ath10k_sdio *ar_sdi=
o,
> + bool *done)
> +{
> + struct ath10k_sdio_irq_data *irq_data =3D &ar_sdio->irq_data;
> + struct ath10k *ar =3D ar_sdio->ar;
> + struct ath10k_sdio_irq_proc_registers *rg;
> + int status =3D 0;
> + u8 host_int_status =3D 0;
> + u32 lookahead =3D 0;
> + u8 htc_mbox =3D 1 << ATH10K_HTC_MAILBOX;
> +
> + /* NOTE: HIF implementation guarantees that the context of this
> + * call allows us to perform SYNCHRONOUS I/O, that is we can block,
> + * sleep or call any API that can block or switch thread/task
> + * contexts. This is a fully schedulable context.
> + */
> +
> + /* Process pending intr only when int_status_en is clear, it may
> + * result in unnecessary bus transaction otherwise. Target may be
> + * unresponsive at the time.
> + */
> + if (irq_data->irq_en_reg.int_status_en) {
> + /* Read the first sizeof(struct ath10k_irq_proc_registers)
> + * bytes of the HTC register table. This
> + * will yield us the value of different int status
> + * registers and the lookahead registers.
> + */
> + status =3D ath10k_sdio_read_write_sync(
> + ar,
> + MBOX_HOST_INT_STATUS_ADDRESS,
> + (u8 *)&irq_data->irq_proc_reg,
> + sizeof(irq_data->irq_proc_reg),
> + HIF_RD_SYNC_BYTE_INC);
> + if (status)
> + goto out;
> +
> + /* Update only those registers that are enabled */
> + host_int_status =3D irq_data->irq_proc_reg.host_int_status &
> + irq_data->irq_en_reg.int_status_en;
> +
> + /* Look at mbox status */
> + if (host_int_status & htc_mbox) {
> + /* Mask out pending mbox value, we use look ahead as
> + * the real flag for mbox processing.
> + */
> + host_int_status &=3D ~htc_mbox;
> + if (irq_data->irq_proc_reg.rx_lookahead_valid &
> + htc_mbox) {
> + rg =3D &irq_data->irq_proc_reg;
> + lookahead =3D le32_to_cpu(
> + rg->rx_lookahead[ATH10K_HTC_MAILBOX]);
> + if (!lookahead)
> + ath10k_err(ar, "lookahead is zero!\n");
> + }
> + }
> + }
> +
> + if (!host_int_status && !lookahead) {
> + *done =3D true;
> + goto out;
> + }
> +
> + if (lookahead) {
> + ath10k_dbg(ar, ATH10K_DBG_SDIO,
> + "pending mailbox msg, lookahead: 0x%08X\n",
> + lookahead);
> +
> + status =3D ath10k_sdio_mbox_rxmsg_pending_handler(ar_sdio,
> + lookahead,
> + done);
> + if (status)
> + goto out;
> + }
> +
> + /* now, handle the rest of the interrupts */
> + ath10k_dbg(ar, ATH10K_DBG_SDIO,
> + "valid interrupt source(s) for other interrupts: 0x%x\n",
> + host_int_status);
> +
> + if (MS(host_int_status, MBOX_HOST_INT_STATUS_CPU)) {
> + /* CPU Interrupt */
> + status =3D ath10k_sdio_mbox_proc_cpu_intr(ar_sdio);
> + if (status)
> + goto out;
> + }
> +
> + if (MS(host_int_status, MBOX_HOST_INT_STATUS_ERROR)) {
> + /* Error Interrupt */
> + status =3D ath10k_sdio_mbox_proc_err_intr(ar_sdio);
> + if (status)
> + goto out;
> + }
> +
> + if (MS(host_int_status, MBOX_HOST_INT_STATUS_COUNTER))
> + /* Counter Interrupt */
> + status =3D ath10k_sdio_mbox_proc_counter_intr(ar_sdio);
> +
> +out:
> + /* An optimization to bypass reading the IRQ status registers
> + * unecessarily which can re-wake the target, if upper layers
> + * determine that we are in a low-throughput mode, we can rely on
> + * taking another interrupt rather than re-checking the status
> + * registers which can re-wake the target.
> + *
> + * NOTE : for host interfaces that makes use of detecting pending
> + * mbox messages at hif can not use this optimization due to
> + * possible side effects, SPI requires the host to drain all
> + * messages from the mailbox before exiting the ISR routine.
> + */
> +
> + ath10k_dbg(ar, ATH10K_DBG_SDIO,
> + "%s: (done:%d, status=3D%d)\n", __func__, *done, status);
We try to follow this kind of format for debug messages:
"sdio pending irqs done %d status %d"
So start with the debug level name followed by the debug separated with spa=
ces.
And IIRC no need for "\n", the macro adds that automatically.
> +
> + return status;
> +}
> +
> +/* Macro to check if DMA buffer is WORD-aligned and DMA-able.
> + * Most host controllers assume the buffer is DMA'able and will
> + * bug-check otherwise (i.e. buffers on the stack). virt_addr_valid
> + * check fails on stack memory.
> + */
> +static inline bool buf_needs_bounce(u8 *buf)
> +{
> + return ((unsigned long)buf & 0x3) || !virt_addr_valid(buf);
> +}
IS_ALIGNED()? And this is super ugly, do we really need this? I would
much prefer that we would directly use struct sk_buff, more of that
later.
> +static inline enum ath10k_htc_ep_id pipe_id_to_eid(u8 pipe_id)
> +{
> + return (enum ath10k_htc_ep_id)pipe_id;
> +}
Oh, we already have a this kind of mapping function? Can't this be used
earlier?
> +static void ath10k_sdio_set_mbox_info(struct ath10k_sdio *ar_sdio)
> +{
> + struct ath10k_mbox_info *mbox_info =3D &ar_sdio->mbox_info;
> + u16 device =3D ar_sdio->func->device;
> +
> + mbox_info->htc_addr =3D ATH10K_HIF_MBOX_BASE_ADDR;
> + mbox_info->block_size =3D ATH10K_HIF_MBOX_BLOCK_SIZE;
> + mbox_info->block_mask =3D ATH10K_HIF_MBOX_BLOCK_SIZE - 1;
> + mbox_info->gmbox_addr =3D ATH10K_HIF_GMBOX_BASE_ADDR;
> + mbox_info->gmbox_sz =3D ATH10K_HIF_GMBOX_WIDTH;
> +
> + mbox_info->ext_info[0].htc_ext_addr =3D ATH10K_HIF_MBOX0_EXT_BASE_ADDR;
> +
> + if ((device & ATH10K_MANUFACTURER_ID_REV_MASK) < 4)
> + mbox_info->ext_info[0].htc_ext_sz =3D ATH10K_HIF_MBOX0_EXT_WIDTH;
> + else
> + /* from rome 2.0(0x504), the width has been extended
> + * to 56K
> + */
> + mbox_info->ext_info[0].htc_ext_sz =3D
> + ATH10K_HIF_MBOX0_EXT_WIDTH_ROME_2_0;
> +
> + mbox_info->ext_info[1].htc_ext_addr =3D
> + mbox_info->ext_info[0].htc_ext_addr +
> + mbox_info->ext_info[0].htc_ext_sz +
> + ATH10K_HIF_MBOX_DUMMY_SPACE_SIZE;
> + mbox_info->ext_info[1].htc_ext_sz =3D ATH10K_HIF_MBOX1_EXT_WIDTH;
> +}
> +
> +static inline void ath10k_sdio_set_cmd52_arg(u32 *arg, u8 write, u8 raw,
> + unsigned int address,
> + unsigned char val)
> +{
> + const u8 func =3D 0;
> +
> + *arg =3D ((write & 1) << 31) |
> + ((func & 0x7) << 28) |
> + ((raw & 1) << 27) |
> + (1 << 26) |
> + ((address & 0x1FFFF) << 9) |
> + (1 << 8) |
> + (val & 0xFF);
> +}
Quite ugly. FIELD_PREP() & co?
> +
> +static int ath10k_sdio_func0_cmd52_wr_byte(struct mmc_card *card,
> + unsigned int address,
> + unsigned char byte)
> +{
> + struct mmc_command io_cmd;
> +
> + memset(&io_cmd, 0, sizeof(io_cmd));
> + ath10k_sdio_set_cmd52_arg(&io_cmd.arg, 1, 0, address, byte);
> + io_cmd.opcode =3D SD_IO_RW_DIRECT;
> + io_cmd.flags =3D MMC_RSP_R5 | MMC_CMD_AC;
> +
> + return mmc_wait_for_cmd(card->host, &io_cmd, 0);
> +}
> +
> +static int ath10k_sdio_func0_cmd52_rd_byte(struct mmc_card *card,
> + unsigned int address,
> + unsigned char *byte)
> +{
> + int ret;
> + struct mmc_command io_cmd;
> +
> + memset(&io_cmd, 0, sizeof(io_cmd));
> + ath10k_sdio_set_cmd52_arg(&io_cmd.arg, 0, 0, address, 0);
> + io_cmd.opcode =3D SD_IO_RW_DIRECT;
> + io_cmd.flags =3D MMC_RSP_R5 | MMC_CMD_AC;
> +
> + ret =3D mmc_wait_for_cmd(card->host, &io_cmd, 0);
> + if (!ret)
> + *byte =3D io_cmd.resp[0];
> +
> + return ret;
> +}
> +
> +static int ath10k_sdio_io(struct ath10k_sdio *ar_sdio, u32 request, u32 =
addr,
> + u8 *buf, u32 len)
> +{
> + int ret =3D 0;
Avoid these kind of unnecessary initialisations.
> + struct sdio_func *func =3D ar_sdio->func;
> + struct ath10k *ar =3D ar_sdio->ar;
> +
> + sdio_claim_host(func);
> +
> + if (request & HIF_WRITE) {
> + if (request & HIF_FIXED_ADDRESS)
> + ret =3D sdio_writesb(func, addr, buf, len);
> + else
> + ret =3D sdio_memcpy_toio(func, addr, buf, len);
> + } else {
> + if (request & HIF_FIXED_ADDRESS)
> + ret =3D sdio_readsb(func, buf, addr, len);
> + else
> + ret =3D sdio_memcpy_fromio(func, buf, addr, len);
> + }
> +
> + sdio_release_host(func);
> +
> + ath10k_dbg(ar, ATH10K_DBG_SDIO, "%s addr 0x%x%s buf 0x%p len %d\n",
> + request & HIF_WRITE ? "wr" : "rd", addr,
> + request & HIF_FIXED_ADDRESS ? " (fixed)" : "", buf, len);
> + ath10k_dbg_dump(ar, ATH10K_DBG_SDIO_DUMP, NULL,
> + request & HIF_WRITE ? "sdio wr " : "sdio rd ",
> + buf, len);
> +
> + return ret;
> +}
> +
> +static struct ath10k_sdio_bus_request
> +*ath10k_sdio_alloc_busreq(struct ath10k_sdio *ar_sdio)
> +{
> + struct ath10k_sdio_bus_request *bus_req;
> +
> + spin_lock_bh(&ar_sdio->lock);
> +
> + if (list_empty(&ar_sdio->bus_req_freeq)) {
> + spin_unlock_bh(&ar_sdio->lock);
> + return NULL;
> + }
> +
> + bus_req =3D list_first_entry(&ar_sdio->bus_req_freeq,
> + struct ath10k_sdio_bus_request, list);
> + list_del(&bus_req->list);
> +
> + spin_unlock_bh(&ar_sdio->lock);
> +
> + return bus_req;
> +}
> +
> +static void ath10k_sdio_free_bus_req(struct ath10k_sdio *ar_sdio,
> + struct ath10k_sdio_bus_request *bus_req)
> +{
> + spin_lock_bh(&ar_sdio->lock);
> + list_add_tail(&bus_req->list, &ar_sdio->bus_req_freeq);
> + spin_unlock_bh(&ar_sdio->lock);
> +}
> +
> +static int ath10k_sdio_read_write_sync(struct ath10k *ar, u32 addr, u8 *=
buf,
> + u32 len, u32 request)
> +{
> + struct ath10k_sdio *ar_sdio =3D ath10k_sdio_priv(ar);
> + u8 *tbuf =3D NULL;
Extra space after u8?
> + int ret;
> + bool bounced =3D false;
> +
> + if (request & HIF_BLOCK_BASIS)
> + len =3D round_down(len, ar_sdio->mbox_info.block_size);
> +
> + if (buf_needs_bounce(buf)) {
> + if (!ar_sdio->dma_buffer)
> + return -ENOMEM;
> + /* FIXME: I am not sure if it is always correct to assume
> + * that the SDIO irq is a "fake" irq and sleep is possible.
> + * (this function will get called from
> + * ath10k_sdio_irq_handler
> + */
> + mutex_lock(&ar_sdio->dma_buffer_mutex);
> + tbuf =3D ar_sdio->dma_buffer;
> +
> + if (request & HIF_WRITE)
> + memcpy(tbuf, buf, len);
> +
> + bounced =3D true;
> + } else {
> + tbuf =3D buf;
> + }
So I really hate that ar_sdio->dma_buffer, do we really need it? What if
we just get rid of it and allocate struct sk_buff and pass that around?
No need to do extra copying then, I hope :)
> +
> + ret =3D ath10k_sdio_io(ar_sdio, request, addr, tbuf, len);
> + if ((request & HIF_READ) && bounced)
> + memcpy(buf, tbuf, len);
> +
> + if (bounced)
> + mutex_unlock(&ar_sdio->dma_buffer_mutex);
> +
> + return ret;
> +}
> +
> +static void __ath10k_sdio_write_async(struct ath10k_sdio *ar_sdio,
> + struct ath10k_sdio_bus_request *req)
> +{
> + int status;
> + struct ath10k_htc_ep *ep;
> + struct sk_buff *skb;
> +
> + skb =3D req->skb;
> + status =3D ath10k_sdio_read_write_sync(ar_sdio->ar, req->address,
> + skb->data, req->len,
> + req->request);
> + ep =3D &ar_sdio->ar->htc.endpoint[req->eid];
> + ath10k_htc_notify_tx_completion(ep, skb);
> + ath10k_sdio_free_bus_req(ar_sdio, req);
> +}
> +
> +static void ath10k_sdio_write_async_work(struct work_struct *work)
> +{
> + struct ath10k_sdio *ar_sdio;
> + struct ath10k_sdio_bus_request *req, *tmp_req;
> +
> + ar_sdio =3D container_of(work, struct ath10k_sdio, wr_async_work);
> +
> + spin_lock_bh(&ar_sdio->wr_async_lock);
> + list_for_each_entry_safe(req, tmp_req, &ar_sdio->wr_asyncq, list) {
> + list_del(&req->list);
> + spin_unlock_bh(&ar_sdio->wr_async_lock);
> + __ath10k_sdio_write_async(ar_sdio, req);
> + spin_lock_bh(&ar_sdio->wr_async_lock);
> + }
> + spin_unlock_bh(&ar_sdio->wr_async_lock);
> +}
> +
> +static void ath10k_sdio_irq_handler(struct sdio_func *func)
> +{
> + int status =3D 0;
> + unsigned long timeout;
> + struct ath10k_sdio *ar_sdio;
> + bool done =3D false;
> +
> + ar_sdio =3D sdio_get_drvdata(func);
> + atomic_set(&ar_sdio->irq_handling, 1);
> +
> + /* Release the host during interrupts so we can pick it back up when
> + * we process commands.
> + */
> + sdio_release_host(ar_sdio->func);
> +
> + timeout =3D jiffies + ATH10K_SDIO_HIF_COMMUNICATION_TIMEOUT_HZ;
> + while (time_before(jiffies, timeout) && !done) {
> + status =3D ath10k_sdio_mbox_proc_pending_irqs(ar_sdio, &done);
> + if (status)
> + break;
> + }
> +
> + sdio_claim_host(ar_sdio->func);
> +
> + atomic_set(&ar_sdio->irq_handling, 0);
> + wake_up(&ar_sdio->irq_wq);
> +
> + WARN_ON(status && status !=3D -ECANCELED);
> +}
Questionable use of an atomic variable again, looks like badly implement
poor man's locking to me. And instead of wake_up() we should workqueues
or threaded irqs (if sdio supports that).
> +
> +static int ath10k_sdio_hif_disable_intrs(struct ath10k_sdio *ar_sdio)
> +{
> + int ret;
> + struct ath10k_sdio_irq_enable_reg regs;
> + struct ath10k_sdio_irq_data *irq_data =3D &ar_sdio->irq_data;
> +
> + memset(®s, 0, sizeof(regs));
> +
> + ret =3D ath10k_sdio_read_write_sync(ar_sdio->ar,
> + MBOX_INT_STATUS_ENABLE_ADDRESS,
> + ®s.int_status_en, sizeof(regs),
> + HIF_WR_SYNC_BYTE_INC);
> + if (ret) {
> + ath10k_err(ar_sdio->ar, "Unable to disable sdio interrupts\n");
> + return ret;
> + }
> +
> + spin_lock_bh(&irq_data->lock);
> + irq_data->irq_en_reg =3D regs;
> + spin_unlock_bh(&irq_data->lock);
> +
> + return 0;
> +}
> +
> +static int ath10k_sdio_hif_power_up(struct ath10k *ar)
> +{
> + struct ath10k_sdio *ar_sdio =3D ath10k_sdio_priv(ar);
> + struct sdio_func *func =3D ar_sdio->func;
> + int ret =3D 0;
> +
> + if (!ar_sdio->is_disabled)
> + return 0;
> +
> + ath10k_dbg(ar, ATH10K_DBG_BOOT, "sdio power on\n");
> +
> + sdio_claim_host(func);
> +
> + ret =3D sdio_enable_func(func);
> + if (ret) {
> + ath10k_err(ar, "Unable to enable sdio func: %d)\n", ret);
> + sdio_release_host(func);
> + return ret;
> + }
> +
> + sdio_release_host(func);
> +
> + /* Wait for hardware to initialise. It should take a lot less than
> + * 20 ms but let's be conservative here.
> + */
> + msleep(20);
> +
> + ar_sdio->is_disabled =3D false;
> +
> + ret =3D ath10k_sdio_hif_disable_intrs(ar_sdio);
> +
> + return ret;
> +}
> +
> +static void ath10k_sdio_hif_power_down(struct ath10k *ar)
> +{
> + struct ath10k_sdio *ar_sdio =3D ath10k_sdio_priv(ar);
> + int ret;
> +
> + if (ar_sdio->is_disabled)
> + return;
> +
> + ath10k_dbg(ar, ATH10K_DBG_BOOT, "sdio power off\n");
> +
> + /* Disable the card */
> + sdio_claim_host(ar_sdio->func);
> + ret =3D sdio_disable_func(ar_sdio->func);
> + sdio_release_host(ar_sdio->func);
> +
> + if (ret)
> + ath10k_dbg(ar, ATH10K_DBG_BOOT,
> + "Unable to disable sdio: %d\n", ret);
Shouldn't this be ath10k_warn()?
> +
> + ar_sdio->is_disabled =3D true;
> +}
> +
> +int ath10k_sdio_hif_tx_sg(struct ath10k *ar, u8 pipe_id,
> + struct ath10k_hif_sg_item *items, int n_items)
> +{
> + int i;
> + u32 address;
> + struct ath10k_sdio *ar_sdio =3D ath10k_sdio_priv(ar);
> + struct ath10k_sdio_bus_request *bus_req;
> +
> + bus_req =3D ath10k_sdio_alloc_busreq(ar_sdio);
> +
> + if (WARN_ON_ONCE(!bus_req))
> + return -ENOMEM;
Is ath10k_warn() more approriate?
> + for (i =3D 0; i < n_items; i++) {
> + bus_req->skb =3D items[i].transfer_context;
> + bus_req->request =3D HIF_WRITE;
> + bus_req->eid =3D pipe_id_to_eid(pipe_id);
> + /* Write TX data to the end of the mbox address space */
> + address =3D ar_sdio->mbox_addr[bus_req->eid] +
> + ar_sdio->mbox_size[bus_req->eid] - bus_req->skb->len;
> + bus_req->address =3D address;
> + bus_req->len =3D CALC_TXRX_PADDED_LEN(ar_sdio, bus_req->skb->len);
> +
> + spin_lock_bh(&ar_sdio->wr_async_lock);
> + list_add_tail(&bus_req->list, &ar_sdio->wr_asyncq);
> + spin_unlock_bh(&ar_sdio->wr_async_lock);
> + }
> +
> + queue_work(ar_sdio->workqueue, &ar_sdio->wr_async_work);
> +
> + return 0;
> +}
> +
> +static int ath10k_sdio_hif_enable_intrs(struct ath10k_sdio *ar_sdio)
> +{
> + struct ath10k_sdio_irq_enable_reg regs;
> + int status;
> + struct ath10k_sdio_irq_data *irq_data =3D &ar_sdio->irq_data;
> +
> + memset(®s, 0, sizeof(regs));
> +
> + /* Enable all but CPU interrupts */
> + regs.int_status_en =3D SM(0x01, MBOX_INT_STATUS_ENABLE_ERROR) |
> + SM(0x01, MBOX_INT_STATUS_ENABLE_CPU) |
> + SM(0x01, MBOX_INT_STATUS_ENABLE_COUNTER);
> +
> + /* NOTE: There are some cases where HIF can do detection of
> + * pending mbox messages which is disabled now.
> + */
> + regs.int_status_en |=3D SM(0x01, MBOX_INT_STATUS_ENABLE_MBOX_DATA);
> +
> + /* Set up the CPU Interrupt status Register */
> + regs.cpu_int_status_en =3D 0;
> +
> + /* Set up the Error Interrupt status Register */
> + regs.err_int_status_en =3D
> + SM(0x01, MBOX_ERROR_STATUS_ENABLE_RX_UNDERFLOW) |
> + SM(0x01, MBOX_ERROR_STATUS_ENABLE_TX_OVERFLOW);
> +
> + /* Enable Counter interrupt status register to get fatal errors for
> + * debugging.
> + */
> + regs.cntr_int_status_en =3D SM(ATH10K_SDIO_TARGET_DEBUG_INTR_MASK,
> + MBOX_COUNTER_INT_STATUS_ENABLE_BIT);
> +
> + status =3D ath10k_sdio_read_write_sync(ar_sdio->ar,
> + MBOX_INT_STATUS_ENABLE_ADDRESS,
> + ®s.int_status_en, sizeof(regs),
> + HIF_WR_SYNC_BYTE_INC);
> + if (status) {
> + ath10k_err(ar_sdio->ar,
> + "failed to update interrupt ctl reg err: %d\n",
> + status);
> + return status;
> + }
> +
> + spin_lock_bh(&irq_data->lock);
> + irq_data->irq_en_reg =3D regs;
> + spin_unlock_bh(&irq_data->lock);
> +
> + return 0;
> +}
> +
> +static int ath10k_sdio_hif_start(struct ath10k *ar)
> +{
> + int ret;
> + struct ath10k_sdio *ar_sdio =3D ath10k_sdio_priv(ar);
> + u32 addr, val;
> +
> + addr =3D host_interest_item_address(HI_ITEM(hi_acs_flags));
> +
> + ret =3D ath10k_sdio_hif_diag_read32(ar, addr, &val);
> + if (ret) {
> + ath10k_err(ar, "Unable to read diag mem: %d\n", ret);
> + goto out;
> + }
> +
> + if (val & HI_ACS_FLAGS_SDIO_SWAP_MAILBOX_FW_ACK) {
> + ath10k_dbg(ar, ATH10K_DBG_SDIO,
> + "Mailbox SWAP Service is enabled\n");
> + ar_sdio->swap_mbox =3D true;
> + }
> +
> + /* eid 0 always uses the lower part of the extended mailbox address
> + * space (ext_info[0].htc_ext_addr).
> + */
> + ar_sdio->mbox_addr[0] =3D ar_sdio->mbox_info.ext_info[0].htc_ext_addr;
> + ar_sdio->mbox_size[0] =3D ar_sdio->mbox_info.ext_info[0].htc_ext_sz;
> +
> + sdio_claim_host(ar_sdio->func);
> +
> + /* Register the isr */
> + ret =3D sdio_claim_irq(ar_sdio->func, ath10k_sdio_irq_handler);
> + if (ret) {
> + ath10k_err(ar, "Failed to claim sdio irq: %d\n", ret);
> + sdio_release_host(ar_sdio->func);
> + goto out;
> + }
> +
> + sdio_release_host(ar_sdio->func);
> +
> + ret =3D ath10k_sdio_hif_enable_intrs(ar_sdio);
> + if (ret)
> + ath10k_err(ar, "Failed to enable sdio interrupts: %d\n", ret);
> +
> +out:
> + return ret;
> +}
> +
> +static bool ath10k_sdio_is_on_irq(struct ath10k *ar)
> +{
> + struct ath10k_sdio *ar_sdio =3D ath10k_sdio_priv(ar);
> +
> + return !atomic_read(&ar_sdio->irq_handling);
> +}
Yikes.
> +
> +static void ath10k_sdio_irq_disable(struct ath10k *ar)
> +{
> + struct ath10k_sdio *ar_sdio =3D ath10k_sdio_priv(ar);
> + int ret;
> +
> + sdio_claim_host(ar_sdio->func);
> +
> + atomic_set(&ar_sdio->stopping, 1);
> +
> + if (atomic_read(&ar_sdio->irq_handling)) {
> + sdio_release_host(ar_sdio->func);
> +
> + ret =3D wait_event_interruptible(ar_sdio->irq_wq,
> + ath10k_sdio_is_on_irq(ar));
> + if (ret)
> + return;
> +
> + sdio_claim_host(ar_sdio->func);
> + }
There has to be a better way to implement this, now it feels that it's
just reimplementing the wheel. We should have proper way to wait for
interrupt handlers and workqueues to end etc.
> + ret =3D sdio_release_irq(ar_sdio->func);
> + if (ret)
> + ath10k_err(ar, "Failed to release sdio irq: %d\n", ret);
> +
> + sdio_release_host(ar_sdio->func);
> +}
> +
> +static int ath10k_sdio_config(struct ath10k_sdio *ar_sdio)
> +{
> + struct ath10k *ar =3D ar_sdio->ar;
> + struct sdio_func *func =3D ar_sdio->func;
> + unsigned char byte, asyncintdelay =3D 2;
> + int ret;
> +
> + ath10k_dbg(ar, ATH10K_DBG_BOOT, "SDIO configuration\n");
> +
> + sdio_claim_host(func);
> +
> + byte =3D 0;
> + ret =3D ath10k_sdio_func0_cmd52_rd_byte(func->card,
> + SDIO_CCCR_DRIVE_STRENGTH,
> + &byte);
> +
> + byte =3D (byte & (~(SDIO_DRIVE_DTSx_MASK << SDIO_DRIVE_DTSx_SHIFT))) |
> + SDIO_DTSx_SET_TYPE_D;
FIELD_PREP(). There are lots of places where that an be used.
> +static void ath10k_sdio_write32(struct ath10k *ar, u32 offset, u32 value=
)
> +{
> +}
> +
> +static u32 ath10k_sdio_read32(struct ath10k *ar, u32 offset)
> +{
> + return 0;
> +}
Somekind of FIXME/TODO comments for write32() and read32()? What
functionality are we going to miss when we don't implement these?
> +static void ath10k_sdio_hif_send_complete_check(struct ath10k *ar,
> + u8 pipe, int force)
> +{
> +}
> +
> +static u16 ath10k_sdio_hif_get_free_queue_number(struct ath10k *ar, u8 p=
ipe)
> +{
> + return 0;
> +}
Similar comments here also.
> +
> +static const struct ath10k_hif_ops ath10k_sdio_hif_ops =3D {
> + .tx_sg =3D ath10k_sdio_hif_tx_sg,
> + .diag_read =3D ath10k_sdio_hif_diag_read,
> + .diag_write =3D ath10k_sdio_diag_write_mem,
> + .exchange_bmi_msg =3D ath10k_sdio_hif_exchange_bmi_msg,
> + .start =3D ath10k_sdio_hif_start,
> + .stop =3D ath10k_sdio_hif_stop,
> + .map_service_to_pipe =3D ath10k_sdio_hif_map_service_to_pipe,
> + .get_default_pipe =3D ath10k_sdio_hif_get_default_pipe,
> + .send_complete_check =3D ath10k_sdio_hif_send_complete_check,
> + .get_free_queue_number =3D ath10k_sdio_hif_get_free_queue_number,
> + .power_up =3D ath10k_sdio_hif_power_up,
> + .power_down =3D ath10k_sdio_hif_power_down,
> + .read32 =3D ath10k_sdio_read32,
> + .write32 =3D ath10k_sdio_write32,
> +#ifdef CONFIG_PM
> + .suspend =3D ath10k_sdio_hif_suspend,
> + .resume =3D ath10k_sdio_hif_resume,
> +#endif
> + .fetch_cal_eeprom =3D ath10k_sdio_hif_fetch_cal_eeprom,
> +};
> +
> +#ifdef CONFIG_PM_SLEEP
> +
> +/* Empty handlers so that mmc subsystem doesn't remove us entirely durin=
g
> + * suspend. We instead follow cfg80211 suspend/resume handlers.
> + */
> +static int ath10k_sdio_pm_suspend(struct device *device)
> +{
> + return 0;
> +}
> +
> +static int ath10k_sdio_pm_resume(struct device *device)
> +{
> + return 0;
> +}
> +
> +static SIMPLE_DEV_PM_OPS(ath10k_sdio_pm_ops, ath10k_sdio_pm_suspend,
> + ath10k_sdio_pm_resume);
> +
> +#define ATH10K_SDIO_PM_OPS (&ath10k_sdio_pm_ops)
> +
> +#else
> +
> +#define ATH10K_SDIO_PM_OPS NULL
> +
> +#endif /* CONFIG_PM_SLEEP */
> +
> +static int ath10k_sdio_probe(struct sdio_func *func,
> + const struct sdio_device_id *id)
> +{
> + int ret;
> + struct ath10k_sdio *ar_sdio;
> + struct ath10k *ar;
> + int i;
> + enum ath10k_hw_rev hw_rev;
> +
> + hw_rev =3D ATH10K_HW_QCA6174;
This needs a comment, at least to remind if ever add something else than
QCA6174 based SDIO design.
> +
> + ar =3D ath10k_core_create(sizeof(*ar_sdio), &func->dev, ATH10K_BUS_SDIO=
,
> + hw_rev, &ath10k_sdio_hif_ops);
> + if (!ar) {
> + dev_err(&func->dev, "failed to allocate core\n");
> + return -ENOMEM;
> + }
> +
> + ath10k_dbg(ar, ATH10K_DBG_BOOT,
> + "sdio new func %d vendor 0x%x device 0x%x block 0x%x/0x%x\n",
> + func->num, func->vendor, func->device,
> + func->max_blksize, func->cur_blksize);
> +
> + ar_sdio =3D ath10k_sdio_priv(ar);
> +
> + ar_sdio->dma_buffer =3D kzalloc(ATH10K_HIF_DMA_BUFFER_SIZE, GFP_KERNEL)=
;
> + if (!ar_sdio->dma_buffer) {
> + ret =3D -ENOMEM;
> + goto err_core_destroy;
> + }
> +
> + ar_sdio->func =3D func;
> + sdio_set_drvdata(func, ar_sdio);
> +
> + ar_sdio->is_disabled =3D true;
> + ar_sdio->ar =3D ar;
> +
> + spin_lock_init(&ar_sdio->lock);
> + spin_lock_init(&ar_sdio->wr_async_lock);
> + spin_lock_init(&ar_sdio->irq_data.lock);
> + mutex_init(&ar_sdio->dma_buffer_mutex);
> +
> + INIT_LIST_HEAD(&ar_sdio->bus_req_freeq);
> + INIT_LIST_HEAD(&ar_sdio->wr_asyncq);
> +
> + INIT_WORK(&ar_sdio->wr_async_work, ath10k_sdio_write_async_work);
> + ar_sdio->workqueue =3D create_singlethread_workqueue("ath10k_sdio_wq");
> + if (!ar_sdio->workqueue)
> + goto err;
> +
> + init_waitqueue_head(&ar_sdio->irq_wq);
> +
> + for (i =3D 0; i < ATH10K_SDIO_BUS_REQUEST_MAX_NUM; i++)
> + ath10k_sdio_free_bus_req(ar_sdio, &ar_sdio->bus_req[i]);
> +
> + ar->dev_id =3D id->device;
> + ar->id.vendor =3D id->vendor;
> + ar->id.device =3D id->device;
> +
> + ath10k_sdio_set_mbox_info(ar_sdio);
> +
> + ret =3D ath10k_sdio_config(ar_sdio);
> + if (ret) {
> + ath10k_err(ar, "Failed to config sdio: %d\n", ret);
> + goto err;
> + }
> +
> + ret =3D ath10k_core_register(ar, 0/*chip_id is not applicaple for SDIO*=
/);
> + if (ret) {
> + ath10k_err(ar, "failed to register driver core: %d\n", ret);
> + goto err;
> + }
I would assume that chip_id is applicaple also with SDIO, there has to
be a register where to get it. Also this kind of comment style is
preferred:
/* TODO: don't know yet how to get chip_id with SDIO */
chip_id =3D 0;
ret =3D ath10k_core_register(ar, chip_id);
> +
> + return ret;
> +
> +err:
> + kfree(ar_sdio->dma_buffer);
> +err_core_destroy:
> + ath10k_core_destroy(ar);
> +
> + return ret;
> +}
> +
> +static void ath10k_sdio_remove(struct sdio_func *func)
> +{
> + struct ath10k_sdio *ar_sdio;
> + struct ath10k *ar;
> +
> + ar_sdio =3D sdio_get_drvdata(func);
> + ar =3D ar_sdio->ar;
> +
> + ath10k_dbg(ar, ATH10K_DBG_BOOT,
> + "sdio removed func %d vendor 0x%x device 0x%x\n",
> + func->num, func->vendor, func->device);
> +
> + (void)ath10k_sdio_hif_disable_intrs(ar_sdio);
> + cancel_work_sync(&ar_sdio->wr_async_work);
> + ath10k_core_unregister(ar);
> + ath10k_core_destroy(ar);
> +
> + kfree(ar_sdio->dma_buffer);
> +}
> +
> +static const struct sdio_device_id ath10k_sdio_devices[] =3D {
> + {SDIO_DEVICE(ATH10K_MANUFACTURER_CODE,
> + (ATH10K_MANUFACTURER_ID_AR6005_BASE | 0xA))},
> + {},
> +};
I suspect there's a more sensible way to create the device table than
this, just no time to check it now. Anyone know?
The naming "ath10k manufacturer" is also wrong, it should be QCA or
Qualcomm.
--=20
Kalle Valo=
^ permalink raw reply
* [PATCH] nl80211: rework {sched_,}scan event related functions
From: Arend van Spriel @ 2016-12-16 11:21 UTC (permalink / raw)
To: Johannes Berg; +Cc: linux-wireless, Arend van Spriel
A couple of functions used with scan events were named with
term "send" although they were only preparing the the event
message so renamed those.
Signed-off-by: Arend van Spriel <arend.vanspriel@broadcom.com>
---
net/wireless/nl80211.c | 34 ++++++++--------------------------
net/wireless/nl80211.h | 6 ++----
net/wireless/scan.c | 9 +++++----
3 files changed, 15 insertions(+), 34 deletions(-)
diff --git a/net/wireless/nl80211.c b/net/wireless/nl80211.c
index 3df85a7..727ca50 100644
--- a/net/wireless/nl80211.c
+++ b/net/wireless/nl80211.c
@@ -12810,7 +12810,7 @@ static int nl80211_add_scan_req(struct sk_buff *msg,
return -ENOBUFS;
}
-static int nl80211_send_scan_msg(struct sk_buff *msg,
+static int nl80211_prep_scan_msg(struct sk_buff *msg,
struct cfg80211_registered_device *rdev,
struct wireless_dev *wdev,
u32 portid, u32 seq, int flags,
@@ -12841,7 +12841,7 @@ static int nl80211_send_scan_msg(struct sk_buff *msg,
}
static int
-nl80211_send_sched_scan_msg(struct sk_buff *msg,
+nl80211_prep_sched_scan_msg(struct sk_buff *msg,
struct cfg80211_registered_device *rdev,
struct net_device *netdev,
u32 portid, u32 seq, int flags, u32 cmd)
@@ -12873,7 +12873,7 @@ void nl80211_send_scan_start(struct cfg80211_registered_device *rdev,
if (!msg)
return;
- if (nl80211_send_scan_msg(msg, rdev, wdev, 0, 0, 0,
+ if (nl80211_prep_scan_msg(msg, rdev, wdev, 0, 0, 0,
NL80211_CMD_TRIGGER_SCAN) < 0) {
nlmsg_free(msg);
return;
@@ -12892,7 +12892,7 @@ struct sk_buff *nl80211_build_scan_msg(struct cfg80211_registered_device *rdev,
if (!msg)
return NULL;
- if (nl80211_send_scan_msg(msg, rdev, wdev, 0, 0, 0,
+ if (nl80211_prep_scan_msg(msg, rdev, wdev, 0, 0, 0,
aborted ? NL80211_CMD_SCAN_ABORTED :
NL80211_CMD_NEW_SCAN_RESULTS) < 0) {
nlmsg_free(msg);
@@ -12902,31 +12902,13 @@ struct sk_buff *nl80211_build_scan_msg(struct cfg80211_registered_device *rdev,
return msg;
}
-void nl80211_send_scan_result(struct cfg80211_registered_device *rdev,
- struct sk_buff *msg)
-{
- if (!msg)
- return;
-
- genlmsg_multicast_netns(&nl80211_fam, wiphy_net(&rdev->wiphy), msg, 0,
- NL80211_MCGRP_SCAN, GFP_KERNEL);
-}
-
-void nl80211_send_sched_scan_results(struct cfg80211_registered_device *rdev,
- struct net_device *netdev)
+/* send message created by nl80211_build_scan_msg() */
+void nl80211_send_scan_msg(struct cfg80211_registered_device *rdev,
+ struct sk_buff *msg)
{
- struct sk_buff *msg;
-
- msg = nlmsg_new(NLMSG_DEFAULT_SIZE, GFP_KERNEL);
if (!msg)
return;
- if (nl80211_send_sched_scan_msg(msg, rdev, netdev, 0, 0, 0,
- NL80211_CMD_SCHED_SCAN_RESULTS) < 0) {
- nlmsg_free(msg);
- return;
- }
-
genlmsg_multicast_netns(&nl80211_fam, wiphy_net(&rdev->wiphy), msg, 0,
NL80211_MCGRP_SCAN, GFP_KERNEL);
}
@@ -12940,7 +12922,7 @@ void nl80211_send_sched_scan(struct cfg80211_registered_device *rdev,
if (!msg)
return;
- if (nl80211_send_sched_scan_msg(msg, rdev, netdev, 0, 0, 0, cmd) < 0) {
+ if (nl80211_prep_sched_scan_msg(msg, rdev, netdev, 0, 0, 0, cmd) < 0) {
nlmsg_free(msg);
return;
}
diff --git a/net/wireless/nl80211.h b/net/wireless/nl80211.h
index 7e3821d..75f8252 100644
--- a/net/wireless/nl80211.h
+++ b/net/wireless/nl80211.h
@@ -14,12 +14,10 @@ void nl80211_send_scan_start(struct cfg80211_registered_device *rdev,
struct wireless_dev *wdev);
struct sk_buff *nl80211_build_scan_msg(struct cfg80211_registered_device *rdev,
struct wireless_dev *wdev, bool aborted);
-void nl80211_send_scan_result(struct cfg80211_registered_device *rdev,
- struct sk_buff *msg);
+void nl80211_send_scan_msg(struct cfg80211_registered_device *rdev,
+ struct sk_buff *msg);
void nl80211_send_sched_scan(struct cfg80211_registered_device *rdev,
struct net_device *netdev, u32 cmd);
-void nl80211_send_sched_scan_results(struct cfg80211_registered_device *rdev,
- struct net_device *netdev);
void nl80211_common_reg_change_event(enum nl80211_commands cmd_id,
struct regulatory_request *request);
diff --git a/net/wireless/scan.c b/net/wireless/scan.c
index b5bd58d..76cdd1d 100644
--- a/net/wireless/scan.c
+++ b/net/wireless/scan.c
@@ -176,7 +176,7 @@ void ___cfg80211_scan_done(struct cfg80211_registered_device *rdev,
ASSERT_RTNL();
if (rdev->scan_msg) {
- nl80211_send_scan_result(rdev, rdev->scan_msg);
+ nl80211_send_scan_msg(rdev, rdev->scan_msg);
rdev->scan_msg = NULL;
return;
}
@@ -222,7 +222,7 @@ void ___cfg80211_scan_done(struct cfg80211_registered_device *rdev,
if (!send_message)
rdev->scan_msg = msg;
else
- nl80211_send_scan_result(rdev, msg);
+ nl80211_send_scan_msg(rdev, msg);
}
void __cfg80211_scan_done(struct work_struct *wk)
@@ -270,7 +270,8 @@ void __cfg80211_sched_scan_results(struct work_struct *wk)
spin_unlock_bh(&rdev->bss_lock);
request->scan_start = jiffies;
}
- nl80211_send_sched_scan_results(rdev, request->dev);
+ nl80211_send_sched_scan(rdev, request->dev,
+ NL80211_CMD_SCHED_SCAN_RESULTS);
}
rtnl_unlock();
@@ -1078,7 +1079,7 @@ struct cfg80211_bss *
else
rcu_assign_pointer(tmp.pub.beacon_ies, ies);
rcu_assign_pointer(tmp.pub.ies, ies);
-
+
memcpy(tmp.pub.bssid, mgmt->bssid, ETH_ALEN);
tmp.pub.channel = channel;
tmp.pub.scan_width = data->scan_width;
--
1.9.1
^ permalink raw reply related
* [PATCH] nl80211: better describe field in struct nl80211_bss_select_rssi_adjust
From: Arend van Spriel @ 2016-12-16 12:15 UTC (permalink / raw)
To: Johannes Berg; +Cc: linux-wireless, Arend van Spriel
The two fields in struct nl80211_bss_select_rssi_adjust did not state
their type or unit. Adding documentation.
Reported-by: Jouni Malinen <j@w1.fi>
Signed-off-by: Arend van Spriel <arend.vanspriel@broadcom.com>
---
include/uapi/linux/nl80211.h | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/include/uapi/linux/nl80211.h b/include/uapi/linux/nl80211.h
index 6b76e3b..d74e10b 100644
--- a/include/uapi/linux/nl80211.h
+++ b/include/uapi/linux/nl80211.h
@@ -4964,8 +4964,9 @@ enum nl80211_sched_scan_plan {
/**
* struct nl80211_bss_select_rssi_adjust - RSSI adjustment parameters.
*
- * @band: band of BSS that must match for RSSI value adjustment.
- * @delta: value used to adjust the RSSI value of matching BSS.
+ * @band: band of BSS that must match for RSSI value adjustment. The value
+ * of this field is according to &enum nl80211_band.
+ * @delta: value used to adjust the RSSI value of matching BSS in dB.
*/
struct nl80211_bss_select_rssi_adjust {
__u8 band;
--
1.9.1
^ permalink raw reply related
* Re: [RFC V3 04/11] nl80211: add driver api for gscan notifications
From: Arend Van Spriel @ 2016-12-16 12:17 UTC (permalink / raw)
To: Johannes Berg; +Cc: linux-wireless
In-Reply-To: <1481882578.27953.20.camel@sipsolutions.net>
On 16-12-2016 11:02, Johannes Berg wrote:
>
>> Not sure what is meant by "through the buckets".
>
> TBH, I was handwaving because I don't understand this part of gscan
> well :-)
>
>> Referring to your
>> remark/question in the "Unversal scan proposal" thread:
>>
>> """
>> I'm much more worried about the "bucket reporting" since that doesn't
>> fit into the current full BSS reporting model at all. What's your
>> suggestion for this?
>> """
>>
>> So this is exactly the dilemma I considered so I decided to stick
>> with the full BSS reporting model for gscan as well merely to get it
>> discussed so glad you brought it up ;-).
>
> Heh.
>
> Ok, so I missed that. Somehow I thought hidden in the buckets was a
> partial reporting :-)
>
>> The problem here is that gscan is a vehicle that serves a number of
>> use-cases. So ignoring hotlists, ePNO, etc. the gscan configuration
>> still hold several notification types:
>>
>> - report after completing M scans capturing N best APs or a
>> percentage of (M * N).
>> - report after completing a scan include a specific bucket.
>> - report full scan results.
>>
>> The first two notification trigger retrieval of gscan results which
>> are historic, ie. partial scan results for max M scans.
>>
>> As said earlier the universal scan proposal has some similarities to
>> gscan. Guess you share that as you are using the term "bucket
>> reporting" in that discussion ;-). The historic results are needed
>> for location (if I am not mistaken) so the full BSS reporting model
>> does not fit that. Question is what particular attribute in the
>> historic results is needed for location (suspecting only rssi and
>> possibly the timestamp, but would like to see that confirmed). I was
>> thinking about have historic storage in cfg80211 so we do not need a
>> per-driver solution.
>
> Ok, now I'm starting to understand this better, I guess.
>
> As far as I can tell from our code, for cached results we're reporting
> the following data:
>
> * some flags
> * a scan ID
> * and for each AP:
> * RSSI
> * timestamp
> * channel
> * BSSID
> * SSID (which internally we even have a separate table for and each
> AP just has an index to it, to save memory I guess)
> * beacon period
> * capability field
>
> No IEs and similar things like differentiating probe response/beacon,
> so we can't use the full reporting for this.
>
> I have no problem introducing a common storage for this, if necessary
> with some fields/nl attributes being optional, but I suspect this is
> actually a necessary part of gscan, otherwise you're not able to report
> all the necessary data?
If you just look at the gscan api in wifihal then yes. I was just
wondering whether "all the necessary data" really comprises all these
from a use-case perspective. As an example the api also has rtt fields,
but both brcm and intel solutions do not report that.
Regards,
Arend
^ permalink raw reply
* Re: [RFC V3 03/11] nl80211: add support for gscan
From: Arend Van Spriel @ 2016-12-16 12:21 UTC (permalink / raw)
To: Johannes Berg; +Cc: linux-wireless
In-Reply-To: <1481883237.27953.22.camel@sipsolutions.net>
On 16-12-2016 11:13, Johannes Berg wrote:
> On Wed, 2016-12-14 at 10:01 +0100, Arend Van Spriel wrote:
>
>> Had to look for "> 16" ;-)
>
> Sorry.
>
>> Here an instance of the tab vs. space issue you mentioned. Will go
>> over the patch and fix that.
>
> There were a few, not really interesting though - git would probably
> flag it anyway, or checkpatch :)
>
>>> + if (num_chans > 16)
>>> + return -EINVAL;
>>
>> I suspect this is the restriction you were referring to.
>
> Yes.
>
>> There is no
>> reason for this although the android wifi hal has max 16 channels in
>> a bucket so I might have picked that up.
>
> I thought I saw something with a u16 bitmap that seemed related, but I
> don't see that now so I'm probably just confused.
>
>> So could a driver have a similar limit and should we add such to the
>> gscan capabilities? For instance our firmware api has a nasty
>> restriction of 64 channels for all buckets together, eg. can do 4
>> buckets of 16 channels each.
>
> We do have a limit of the maximum scan buckets, which seems to be 16
> right now. We also have a limit on the number of channels per bucket,
> which is also 16, but no combined limit afaict (so 16x16 seems fine).
>
> Maybe we do need some advertisement in that area then? Right now,
> wifihal seems to be able to read as capabilities the number of buckets
> (wifi_gscan_capabilities), but assumes the number of channels:
>
> const unsigned MAX_CHANNELS = 16;
> const unsigned MAX_BUCKETS = 16;
>
> I guess we took that and combined it, and you had more negotiation with
> Google ;-)
I was not so much involved with the initial gscan effort, but I guess
for brcm it might be true.
> We may then have to actually advertise the limit you have ("64 channels
> combined over all buckets"), unless you can get away with just
> advertising 4 buckets (and us saying 16 channels per bucket is enough?)
>
> I'm a bit tempted to make this more forward compatible though and not
> hard-limit the number of channels per bucket in the code.
Indeed so I will remove it.
Regards,
Arend
^ permalink raw reply
* Re: [PATCH] nl80211: rework {sched_,}scan event related functions
From: Johannes Berg @ 2016-12-16 12:34 UTC (permalink / raw)
To: Arend van Spriel; +Cc: linux-wireless
In-Reply-To: <1481887314-9471-1-git-send-email-arend.vanspriel@broadcom.com>
On Fri, 2016-12-16 at 11:21 +0000, Arend van Spriel wrote:
> A couple of functions used with scan events were named with
> term "send" although they were only preparing the the event
> message so renamed those.
Applied - I added a mention of nl80211_send_sched_scan_results() to the
commit log since I was confused about that at first :)
johannes
^ permalink raw reply
* Re: [PATCH] nl80211: better describe field in struct nl80211_bss_select_rssi_adjust
From: Johannes Berg @ 2016-12-16 12:33 UTC (permalink / raw)
To: Arend van Spriel; +Cc: linux-wireless
In-Reply-To: <1481890554-18019-1-git-send-email-arend.vanspriel@broadcom.com>
On Fri, 2016-12-16 at 12:15 +0000, Arend van Spriel wrote:
> The two fields in struct nl80211_bss_select_rssi_adjust did not state
> their type or unit. Adding documentation.
>
Applied, thanks :)
johannes
^ permalink raw reply
* pull-request: mac80211 2016-12-16
From: Johannes Berg @ 2016-12-16 12:39 UTC (permalink / raw)
To: David Miller; +Cc: netdev, linux-wireless
Hi Dave,
Since you seem to be updating net, I thought I'd send you a few fixes.
These aren't really all that important though, so if you want to let
them wait for a bit I can live with that.
Please pull and let me know if there's any problem.
Thanks,
johannes
The following changes since commit 8fa3b6f9392bf6d90cb7b908e07bd90166639f0a:
Merge tag 'cris-for-4.10' of git://git.kernel.org/pub/scm/linux/kernel/git/jesper/cris (2016-12-12 09:06:38 -0800)
are available in the git repository at:
git://git.kernel.org/pub/scm/linux/kernel/git/jberg/mac80211.git tags/mac80211-for-davem-2016-12-16
for you to fetch changes up to a17d93ff3a950fefaea40e4a4bf3669b9137c533:
mac80211: fix legacy and invalid rx-rate report (2016-12-15 10:54:48 +0100)
----------------------------------------------------------------
Three fixes:
* avoid a WARN_ON() when trying to use WEP with AP_VLANs
* ensure enough headroom on mesh forwarding packets
* don't report unknown/invalid rates to userspace
----------------------------------------------------------------
Ben Greear (1):
mac80211: fix legacy and invalid rx-rate report
Cedric Izoard (1):
mac80211: Ensure enough headroom when forwarding mesh pkt
Johannes Berg (1):
mac80211: don't call drv_set_default_unicast_key() for VLANs
net/mac80211/key.c | 3 ++-
net/mac80211/rx.c | 2 +-
net/mac80211/sta_info.c | 14 ++++++++------
3 files changed, 11 insertions(+), 8 deletions(-)
^ permalink raw reply
* Re: [RFC V3 04/11] nl80211: add driver api for gscan notifications
From: Johannes Berg @ 2016-12-16 12:36 UTC (permalink / raw)
To: Arend Van Spriel; +Cc: linux-wireless
In-Reply-To: <9715e731-efcc-0252-ed7b-5ca67e2c0b5b@broadcom.com>
On Fri, 2016-12-16 at 13:17 +0100, Arend Van Spriel wrote:
>
> > I have no problem introducing a common storage for this, if
> > necessary with some fields/nl attributes being optional, but I
> > suspect this is actually a necessary part of gscan, otherwise
> > you're not able to report all the necessary data?
>
> If you just look at the gscan api in wifihal then yes. I was just
> wondering whether "all the necessary data" really comprises all these
> from a use-case perspective. As an example the api also has rtt
> fields, but both brcm and intel solutions do not report that.
Yeah, no idea. Sorry - my wording wasn't quite precise. By "this is
actually a necessary part of gscan" I meant the partial history
reporting itself, not any particular field thereof.
johannes
^ permalink raw reply
* "rfkill: Add rfkill-any LED trigger" causes deadlock
From: Михаил Кринкин @ 2016-12-16 16:46 UTC (permalink / raw)
To: kernel, johannes.berg, linux-wireless, davem, netdev
In-Reply-To: <20161216163707.GA2629@gmail.com>
Hi,
with recent i can't load my thinkpad with recent linux-next, bisect points
at the commit 73f4f76a196d7adb ("rfkill: Add rfkill-any LED trigger").
Problem occurs because thinkapd_acpi rfkill set_block handler
tpacpi_rfk_hook_set_block calls rfkill_set_sw_state, which in turn calls
new rfkill_any_led_trigger_event. So when rfkill_set_block called from
rfkill_register we have deadlock.
I added WARN to __rfkill_any_led_trigger_event to see how deadlock occurs,
here is backtrace:
[ 6.090079] WARNING: CPU: 2 PID: 307 at net/rfkill/core.c:184
rfkill_any_led_trigger_event+0x2d/0x40
[ 6.090080] reached __rfkill_any_led_trigger_event
[ 6.090081] Modules linked in:
[ 6.090082] snd_pcm sg thinkpad_acpi(+) mei_me nvram snd_seq mei
idma64 virt_dma intel_pch_thermal ucsi intel_lpss_pci snd_seq_device
hci_uart snd_timer snd btbcm soundcore btqca led_class btintel
bluetooth i8042 rtc_cmos serio evdev intel_lpss_acpi intel_lpss
acpi_pad tpm_infineon mfd_core kvm_intel kvm irqbypass ipv6 autofs4
ext4 jbd2 mbcache sd_mod i915 i2c_algo_bit drm_kms_helper syscopyarea
sysfillrect sysimgblt fb_sys_fops drm ahci libahci video
pinctrl_sunrisepoint pinctrl_intel
[ 6.090112] CPU: 2 PID: 307 Comm: systemd-udevd Tainted: G W
4.9.0-01741-g73f4f76-dirty #106
[ 6.090113] Hardware name: LENOVO 20GJ004ERT/20GJ004ERT, BIOS
R0CET21W (1.09 ) 03/07/2016
[ 6.090114] ffffb199c023f998 ffffffffb4334b6b ffffb199c023f9e8
0000000000000000
[ 6.090117] ffffb199c023f9d8 ffffffffb40658ab 000000b84dd75d28
0000000080000006
[ 6.090119] ffff99ea4dd75d28 0000000000000001 0000000000000001
0000000000000000
[ 6.090122] Call Trace:
[ 6.090126] [<ffffffffb4334b6b>] dump_stack+0x4d/0x72
[ 6.090128] [<ffffffffb40658ab>] __warn+0xcb/0xf0
[ 6.090130] [<ffffffffb406592f>] warn_slowpath_fmt+0x5f/0x80
[ 6.090132] [<ffffffffb4631bfd>] rfkill_any_led_trigger_event+0x2d/0x40
[ 6.090135] [<ffffffffb46322d9>] rfkill_set_sw_state+0x89/0xd0
[ 6.090142] [<ffffffffc06c9921>]
tpacpi_rfk_update_swstate+0x31/0x50 [thinkpad_acpi]
[ 6.090148] [<ffffffffc06c9972>]
tpacpi_rfk_hook_set_block+0x32/0x70 [thinkpad_acpi]
[ 6.090150] [<ffffffffb46327f0>] rfkill_set_block+0x90/0x160
[ 6.090152] [<ffffffffb4632930>] __rfkill_switch_all+0x70/0xa0
[ 6.090154] [<ffffffffb4633028>] rfkill_register+0x278/0x2b0
[ 6.090159] [<ffffffffc06e1f0e>] tpacpi_new_rfkill+0xef/0x15d
[thinkpad_acpi]
[ 6.090164] [<ffffffffc06e2407>] bluetooth_init+0x15e/0x1a9 [thinkpad_acpi]
[ 6.090169] [<ffffffffc06e2fb5>]
thinkpad_acpi_module_init.part.47+0x5e7/0x996 [thinkpad_acpi]
[ 6.090174] [<ffffffffc06e3364>] ?
thinkpad_acpi_module_init.part.47+0x996/0x996 [thinkpad_acpi]
[ 6.090179] [<ffffffffc06e36af>]
thinkpad_acpi_module_init+0x34b/0xc9c [thinkpad_acpi]
[ 6.090181] [<ffffffffb4000450>] do_one_initcall+0x50/0x180
[ 6.090184] [<ffffffffb4177e12>] ? do_init_module+0x27/0x1ee
[ 6.090186] [<ffffffffb41cc4b6>] ? kmem_cache_alloc_trace+0x156/0x1a0
[ 6.090188] [<ffffffffb4177e4a>] do_init_module+0x5f/0x1ee
[ 6.090191] [<ffffffffb40ed562>] load_module+0x22c2/0x28d0
[ 6.090192] [<ffffffffb40ea0f0>] ? __symbol_put+0x60/0x60
[ 6.090195] [<ffffffffb42ee15d>] ? ima_post_read_file+0x7d/0xa0
[ 6.090198] [<ffffffffb42a809b>] ? security_kernel_post_read_file+0x6b/0x80
[ 6.090200] [<ffffffffb40eddcf>] SYSC_finit_module+0xdf/0x110
[ 6.090202] [<ffffffffb40ede1e>] SyS_finit_module+0xe/0x10
[ 6.090204] [<ffffffffb463d524>] entry_SYSCALL_64_fastpath+0x17/0x98
[ 6.090206] ---[ end trace ea6da61c1ec208d1 ]---
[ 6.090207] rfkill_any_led_trigger unlocked
[ 6.091664] ------------[ cut here ]------------
^ permalink raw reply
* "rfkill: Add rfkill-any LED trigger" causes deadlock
From: Mike Krinkin @ 2016-12-16 17:09 UTC (permalink / raw)
To: kernel, johannes.berg, linux-wireless, davem, netdev
In-Reply-To: <CACpa5=_chGuU3zVcVR3nkNySwz1cGAQTs39v+BjwcDNSvOqhTw@mail.gmail.com>
On Fri, Dec 16, 2016 at 07:46:06PM +0300, Михаил Кринкин wrote:
> Hi,
>
> with recent i can't load my thinkpad with recent linux-next, bisect points
> at the commit 73f4f76a196d7adb ("rfkill: Add rfkill-any LED trigger").
>
> Problem occurs because thinkapd_acpi rfkill set_block handler
> tpacpi_rfk_hook_set_block calls rfkill_set_sw_state, which in turn calls
> new rfkill_any_led_trigger_event. So when rfkill_set_block called from
> rfkill_register we have deadlock.
>
> I added WARN to __rfkill_any_led_trigger_event to see how deadlock occurs,
> here is backtrace:
>
> [ 6.090079] WARNING: CPU: 2 PID: 307 at net/rfkill/core.c:184
> rfkill_any_led_trigger_event+0x2d/0x40
> [ 6.090080] reached __rfkill_any_led_trigger_event
> [ 6.090081] Modules linked in:
> [ 6.090082] snd_pcm sg thinkpad_acpi(+) mei_me nvram snd_seq mei
> idma64 virt_dma intel_pch_thermal ucsi intel_lpss_pci snd_seq_device
> hci_uart snd_timer snd btbcm soundcore btqca led_class btintel
> bluetooth i8042 rtc_cmos serio evdev intel_lpss_acpi intel_lpss
> acpi_pad tpm_infineon mfd_core kvm_intel kvm irqbypass ipv6 autofs4
> ext4 jbd2 mbcache sd_mod i915 i2c_algo_bit drm_kms_helper syscopyarea
> sysfillrect sysimgblt fb_sys_fops drm ahci libahci video
> pinctrl_sunrisepoint pinctrl_intel
> [ 6.090112] CPU: 2 PID: 307 Comm: systemd-udevd Tainted: G W
> 4.9.0-01741-g73f4f76-dirty #106
> [ 6.090113] Hardware name: LENOVO 20GJ004ERT/20GJ004ERT, BIOS
> R0CET21W (1.09 ) 03/07/2016
> [ 6.090114] ffffb199c023f998 ffffffffb4334b6b ffffb199c023f9e8
> 0000000000000000
> [ 6.090117] ffffb199c023f9d8 ffffffffb40658ab 000000b84dd75d28
> 0000000080000006
> [ 6.090119] ffff99ea4dd75d28 0000000000000001 0000000000000001
> 0000000000000000
> [ 6.090122] Call Trace:
> [ 6.090126] [<ffffffffb4334b6b>] dump_stack+0x4d/0x72
> [ 6.090128] [<ffffffffb40658ab>] __warn+0xcb/0xf0
> [ 6.090130] [<ffffffffb406592f>] warn_slowpath_fmt+0x5f/0x80
> [ 6.090132] [<ffffffffb4631bfd>] rfkill_any_led_trigger_event+0x2d/0x40
> [ 6.090135] [<ffffffffb46322d9>] rfkill_set_sw_state+0x89/0xd0
> [ 6.090142] [<ffffffffc06c9921>]
> tpacpi_rfk_update_swstate+0x31/0x50 [thinkpad_acpi]
> [ 6.090148] [<ffffffffc06c9972>]
> tpacpi_rfk_hook_set_block+0x32/0x70 [thinkpad_acpi]
> [ 6.090150] [<ffffffffb46327f0>] rfkill_set_block+0x90/0x160
> [ 6.090152] [<ffffffffb4632930>] __rfkill_switch_all+0x70/0xa0
> [ 6.090154] [<ffffffffb4633028>] rfkill_register+0x278/0x2b0
> [ 6.090159] [<ffffffffc06e1f0e>] tpacpi_new_rfkill+0xef/0x15d
> [thinkpad_acpi]
> [ 6.090164] [<ffffffffc06e2407>] bluetooth_init+0x15e/0x1a9 [thinkpad_acpi]
> [ 6.090169] [<ffffffffc06e2fb5>]
> thinkpad_acpi_module_init.part.47+0x5e7/0x996 [thinkpad_acpi]
> [ 6.090174] [<ffffffffc06e3364>] ?
> thinkpad_acpi_module_init.part.47+0x996/0x996 [thinkpad_acpi]
> [ 6.090179] [<ffffffffc06e36af>]
> thinkpad_acpi_module_init+0x34b/0xc9c [thinkpad_acpi]
> [ 6.090181] [<ffffffffb4000450>] do_one_initcall+0x50/0x180
> [ 6.090184] [<ffffffffb4177e12>] ? do_init_module+0x27/0x1ee
> [ 6.090186] [<ffffffffb41cc4b6>] ? kmem_cache_alloc_trace+0x156/0x1a0
> [ 6.090188] [<ffffffffb4177e4a>] do_init_module+0x5f/0x1ee
> [ 6.090191] [<ffffffffb40ed562>] load_module+0x22c2/0x28d0
> [ 6.090192] [<ffffffffb40ea0f0>] ? __symbol_put+0x60/0x60
> [ 6.090195] [<ffffffffb42ee15d>] ? ima_post_read_file+0x7d/0xa0
> [ 6.090198] [<ffffffffb42a809b>] ? security_kernel_post_read_file+0x6b/0x80
> [ 6.090200] [<ffffffffb40eddcf>] SYSC_finit_module+0xdf/0x110
> [ 6.090202] [<ffffffffb40ede1e>] SyS_finit_module+0xe/0x10
> [ 6.090204] [<ffffffffb463d524>] entry_SYSCALL_64_fastpath+0x17/0x98
> [ 6.090206] ---[ end trace ea6da61c1ec208d1 ]---
> [ 6.090207] rfkill_any_led_trigger unlocked
> [ 6.091664] ------------[ cut here ]------------
^ permalink raw reply
* Re: [PATCH 02/14 V2] rtlwifi: Remove RT_TRACE messages that use DBG_EMERG
From: Joe Perches @ 2016-12-16 19:31 UTC (permalink / raw)
To: Larry Finger, kvalo; +Cc: devel, linux-wireless, Ping-Ke Shih
In-Reply-To: <20161215182310.13713-3-Larry.Finger@lwfinger.net>
On Thu, 2016-12-15 at 12:22 -0600, Larry Finger wrote:
> These messages are always logged and represent error conditions, thus
> we can use pr_err().
OK and some trivialities:
> diff --git a/drivers/net/wireless/realtek/rtlwifi/base.c b/drivers/net/wireless/realtek/rtlwifi/base.c
[]
> @@ -389,8 +388,8 @@ static void _rtl_init_mac80211(struct ieee80211_hw *hw)
> /* <4> set mac->sband to wiphy->sband */
> hw->wiphy->bands[NL80211_BAND_5GHZ] = sband;
> } else {
> - RT_TRACE(rtlpriv, COMP_INIT, DBG_EMERG, "Err BAND %d\n",
> - rtlhal->current_bandtype);
> + pr_err("Err BAND %d\n",
> + rtlhal->current_bandtype);
It's nice to rewrap lines to 80 columns where possible.
> @@ -1886,8 +1883,7 @@ void rtl_phy_scan_operation_backup(struct ieee80211_hw *hw, u8 operation)
> (u8 *)&iotype);
> break;
> default:
> - RT_TRACE(rtlpriv, COMP_ERR, DBG_EMERG,
> - "Unknown Scan Backup operation.\n");
> + pr_err("Unknown Scan Backup operation.\n");
And it's also nice to remove unnecessary periods for
output message consistency. Most don't use it.
> diff --git a/drivers/net/wireless/realtek/rtlwifi/cam.c b/drivers/net/wireless/realtek/rtlwifi/cam.c
[]
> @@ -285,8 +285,7 @@ u8 rtl_cam_get_free_entry(struct ieee80211_hw *hw, u8 *sta_addr)
> u8 i, *addr;
>
> if (NULL == sta_addr) {
> - RT_TRACE(rtlpriv, COMP_SEC, DBG_EMERG,
> - "sta_addr is NULL.\n");
> + pr_err("sta_addr is NULL.\n");
etc...
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox