* Re: [PATCH 0/4] mwifiex PCI/wake-up interrupt fixes
From: Ard Biesheuvel @ 2019-02-27 10:16 UTC (permalink / raw)
To: Marc Zyngier
Cc: Brian Norris, Ganapathi Bhat, Jeffy Chen, Heiko Stuebner,
Devicetree List, Xinming Hu, <netdev@vger.kernel.org>,
linux-pm, <linux-wireless@vger.kernel.org>,
Linux Kernel Mailing List, Amitkumar Karwar, linux-rockchip,
Nishant Sarmukadam, Rob Herring, Rafael J. Wysocki,
linux-arm-kernel, Enric Balletbo i Serra, Lorenzo Pieralisi,
David S. Miller, Kalle Valo, Tony Lindgren
In-Reply-To: <d67512fe-42b4-513f-d27a-fed85c19e9c2@arm.com>
On Wed, 27 Feb 2019 at 11:02, Marc Zyngier <marc.zyngier@arm.com> wrote:
>
> + Lorenzo
>
> Hi Brian,
>
> On 26/02/2019 23:28, Brian Norris wrote:
> > + others
> >
> > Hi Marc,
> >
> > Thanks for the series. I have a few bits of history to add to this, and
> > some comments.
> >
> > On Sun, Feb 24, 2019 at 02:04:22PM +0000, Marc Zyngier wrote:
> >> For quite some time, I wondered why the PCI mwifiex device built in my
> >> Chromebook was unable to use the good old legacy interrupts. But as MSIs
> >> were working fine, I never really bothered investigating. I finally had a
> >> look, and the result isn't very pretty.
> >>
> >> On this machine (rk3399-based kevin), the wake-up interrupt is described as
> >> such:
> >>
> >> &pci_rootport {
> >> mvl_wifi: wifi@0,0 {
> >> compatible = "pci1b4b,2b42";
> >> reg = <0x83010000 0x0 0x00000000 0x0 0x00100000
> >> 0x83010000 0x0 0x00100000 0x0 0x00100000>;
> >> interrupt-parent = <&gpio0>;
> >> interrupts = <8 IRQ_TYPE_LEVEL_LOW>;
> >> pinctrl-names = "default";
> >> pinctrl-0 = <&wlan_host_wake_l>;
> >> wakeup-source;
> >> };
> >> };
> >>
> >> Note how the interrupt is part of the properties directly attached to the
> >> PCI node. And yet, this interrupt has nothing to do with a PCI legacy
> >> interrupt, as it is attached to the wake-up widget that bypasses the PCIe RC
> >> altogether (Yay for the broken design!). This is in total violation of the
> >> IEEE Std 1275-1994 spec[1], which clearly documents that such interrupt
> >> specifiers describe the PCI device interrupts, and must obey the
> >> INT-{A,B,C,D} mapping. Oops!
> >
> > You're not the first person to notice this. All the motivations are not
> > necessarily painted clearly in their cover letter, but here are some
> > previous attempts at solving this problem:
> >
> > [RFC PATCH v11 0/5] PCI: rockchip: Move PCIe WAKE# handling into pci core
> > https://lkml.kernel.org/lkml/20171225114742.18920-1-jeffy.chen@rock-chips.com/
> > http://lkml.kernel.org/lkml/20171226023646.17722-1-jeffy.chen@rock-chips.com/
> >
> > As you can see by the 12th iteration, it wasn't left unsolved for lack
> > of trying...
>
> I wasn't aware of this. That's definitely a better approach than my
> hack, and I would really like this to be revived.
>
I don't think this approach is entirely sound either.
From the side of the PCI device, WAKE# is just a GPIO line, and how it
is wired into the system is an entirely separate matter. So I don't
think it is justified to overload the notion of legacy interrupts with
some other pin that may behave in a way that is vaguely similar to how
a true wake-up capable interrupt works.
So I'd argue that we should add an optional 'wake-gpio' DT property
instead to the generic PCI device binding, and leave the interrupt
binding and discovery alone.
^ permalink raw reply
* Re: [PATCHv2 1/9] nl80211: New netlink command for TID specific configuration
From: Sergey Matyukevich @ 2019-02-27 10:01 UTC (permalink / raw)
To: Tamizh chelvam
Cc: johannes@sipsolutions.net, ath10k@lists.infradead.org,
linux-wireless@vger.kernel.org
In-Reply-To: <1be400e2f95b87ca0f9e7144177a9b3e@codeaurora.org>
Hi Tamizh,
> Hi Sergey,
> >
> > > Signed-off-by: Tamizh chelvam <tamizhr@codeaurora.org>
> > > ---
> > > include/net/cfg80211.h | 35 +++++++++++++++
> > > include/uapi/linux/nl80211.h | 51 ++++++++++++++++++++++
> > > net/wireless/nl80211.c | 102
> > > +++++++++++++++++++++++++++++++++++++++++++
> > > net/wireless/rdev-ops.h | 11 +++++
> > > net/wireless/trace.h | 18 ++++++++
> > > 5 files changed, 217 insertions(+)
> >
> > ...
> >
> > > diff --git a/net/wireless/nl80211.c b/net/wireless/nl80211.c
> > > index 82d5e1e..352eb4a2 100644
> > > --- a/net/wireless/nl80211.c
> > > +++ b/net/wireless/nl80211.c
> > > @@ -280,6 +280,13 @@ static int validate_ie_attr(const struct nlattr
> > > *attr,
> > >
> > > NLA_POLICY_NESTED_ARRAY(nl80211_psmr_peer_attr_policy),
> > > };
> > >
> > > +static const struct nla_policy
> > > +nl80211_attr_tid_config_policy[NL80211_ATTR_TID_CONFIG_MAX + 1] = {
> > > + [NL80211_ATTR_TID_CONFIG_TID] = NLA_POLICY_MAX(NLA_U8, 7),
> >
> > Such a policy permits configuration of per-TID settings, either
> > for per-STA or for all the STAs. However it is not possible to
> > perform configuration for all TIDs at once, e.g. passing -1 value
> > to driver.
> >
> > Maybe simplify policy and use .type = NLA_U8 ?
> >
> > Sanity check, if needed, can be performed by driver or even by
> > firmware.
> >
> Sure, we can have like that. And do you feel driver should advertise
> support to perform configuration for all TIDs(like accepting tid -1) ?
Do you think this additional level of granularity is needed ?
IIUC if driver/firmware supports per TID feature configuration,
then can handle all-TIDs configuration as well. Anyway, attempt
for global feature configuration can be rejected on driver
or firmware level if needed.
> > > + [NL80211_ATTR_TID_CONFIG_NOACK] =
> > > + NLA_POLICY_MAX(NLA_U8,
> > > NL80211_TID_CONFIG_DISABLE),
> > > +};
> > > +
> > > const struct nla_policy nl80211_policy[NUM_NL80211_ATTR] = {
> > > [NL80211_ATTR_WIPHY] = { .type = NLA_U32 },
> > > [NL80211_ATTR_WIPHY_NAME] = { .type = NLA_NUL_STRING,
> > > @@ -541,6 +548,8 @@ static int validate_ie_attr(const struct nlattr
> > > *attr,
> > > [NL80211_ATTR_PEER_MEASUREMENTS] =
> > > NLA_POLICY_NESTED(nl80211_pmsr_attr_policy),
> > > [NL80211_ATTR_AIRTIME_WEIGHT] = NLA_POLICY_MIN(NLA_U16, 1),
> > > + [NL80211_ATTR_TID_CONFIG] =
> > > +
> > > NLA_POLICY_NESTED(nl80211_attr_tid_config_policy),
> > > };
> >
> > ...
> >
> > > +static int nl80211_set_tid_config(struct sk_buff *skb,
> > > + struct genl_info *info)
> > > +{
> > > + struct cfg80211_registered_device *rdev = info->user_ptr[0];
> > > + struct nlattr *attrs[NL80211_ATTR_TID_CONFIG_MAX + 1];
> > > + struct net_device *dev = info->user_ptr[1];
> > > + struct ieee80211_tid_config *tid_conf;
> > > + struct nlattr *tid;
> > > + int conf_idx = 0, rem_conf;
> > > + u32 num_conf = 0, size_of_conf;
> > > + int ret = -EINVAL;
> > > +
> > > + if (!info->attrs[NL80211_ATTR_TID_CONFIG])
> > > + return -EINVAL;
> > > +
> > > + if (!rdev->ops->set_tid_config)
> > > + return -EOPNOTSUPP;
> > > +
> > > + nla_for_each_nested(tid, info->attrs[NL80211_ATTR_TID_CONFIG],
> > > + rem_conf)
> > > + num_conf++;
> > > +
> > > + size_of_conf = sizeof(struct ieee80211_tid_config) +
> > > + num_conf * sizeof(struct ieee80211_tid_cfg);
> > > +
> > > + tid_conf = kzalloc(size_of_conf, GFP_KERNEL);
> > > + if (!tid_conf)
> > > + return -ENOMEM;
> > > +
> > > + tid_conf->n_tid_conf = num_conf;
> > > +
> > > + if (info->attrs[NL80211_ATTR_MAC])
> > > + tid_conf->peer =
> > > nla_data(info->attrs[NL80211_ATTR_MAC]);
> > > + else
> > > + tid_conf->peer = NULL;
> > > +
> > > + nla_for_each_nested(tid, info->attrs[NL80211_ATTR_TID_CONFIG],
> > > + rem_conf) {
> > > + ret = nla_parse_nested(attrs,
> > > NL80211_ATTR_TID_CONFIG_MAX, tid,
> > > + NULL, NULL);
> > > +
> > > + if (ret)
> > > + return ret;
> > > +
> > > + if (!attrs[NL80211_ATTR_TID_CONFIG_TID])
> > > + return -EINVAL;
> > > +
> > > + ret = parse_tid_conf(rdev, attrs,
> > > &tid_conf->tid_conf[conf_idx],
> > > + tid_conf->peer);
> > > + if (ret)
> > > + goto bad_tid_conf;
> > > +
> > > + conf_idx++;
> > > + }
> > > +
> > > + return rdev_set_tid_config(rdev, dev, tid_conf);
> >
> > What is the ownership rule for tid_conf ? In other words, who is
> > responsible for freeing this structure ?
> >
> > Unless I am missing something, in the suggested patches there is no
> > kfree for this structure and all the nested structures (see Tx bitrate
> > patch), neither in cfg80211/mac80211 nor in ath10k.
> >
> > As far as I understand, we have two options here. One option is to
> > explicitly specify that ownership is passed to the caller, similar
> > to nl80211_set_reg. Another option is to release memory in this
> > function after driver makes copies of all necessary fields,
> > e.g. see nl80211_set_mac_acl.
> >
> Thanks for pointing out, will free it in this function(cfg80211) itself.
> I will make the change and send next patchset.
Great!
Thanks,
Sergey
^ permalink raw reply
* Re: [PATCH 0/4] mwifiex PCI/wake-up interrupt fixes
From: Marc Zyngier @ 2019-02-27 10:02 UTC (permalink / raw)
To: Brian Norris
Cc: Amitkumar Karwar, Enric Balletbo i Serra, Ganapathi Bhat,
Heiko Stuebner, Kalle Valo, Nishant Sarmukadam, Rob Herring,
Xinming Hu, David S. Miller, devicetree, linux-arm-kernel,
linux-kernel, linux-rockchip, linux-wireless, netdev, linux-pm,
Jeffy Chen, Rafael J. Wysocki, Tony Lindgren, Lorenzo Pieralisi
In-Reply-To: <20190226232822.GA174696@google.com>
+ Lorenzo
Hi Brian,
On 26/02/2019 23:28, Brian Norris wrote:
> + others
>
> Hi Marc,
>
> Thanks for the series. I have a few bits of history to add to this, and
> some comments.
>
> On Sun, Feb 24, 2019 at 02:04:22PM +0000, Marc Zyngier wrote:
>> For quite some time, I wondered why the PCI mwifiex device built in my
>> Chromebook was unable to use the good old legacy interrupts. But as MSIs
>> were working fine, I never really bothered investigating. I finally had a
>> look, and the result isn't very pretty.
>>
>> On this machine (rk3399-based kevin), the wake-up interrupt is described as
>> such:
>>
>> &pci_rootport {
>> mvl_wifi: wifi@0,0 {
>> compatible = "pci1b4b,2b42";
>> reg = <0x83010000 0x0 0x00000000 0x0 0x00100000
>> 0x83010000 0x0 0x00100000 0x0 0x00100000>;
>> interrupt-parent = <&gpio0>;
>> interrupts = <8 IRQ_TYPE_LEVEL_LOW>;
>> pinctrl-names = "default";
>> pinctrl-0 = <&wlan_host_wake_l>;
>> wakeup-source;
>> };
>> };
>>
>> Note how the interrupt is part of the properties directly attached to the
>> PCI node. And yet, this interrupt has nothing to do with a PCI legacy
>> interrupt, as it is attached to the wake-up widget that bypasses the PCIe RC
>> altogether (Yay for the broken design!). This is in total violation of the
>> IEEE Std 1275-1994 spec[1], which clearly documents that such interrupt
>> specifiers describe the PCI device interrupts, and must obey the
>> INT-{A,B,C,D} mapping. Oops!
>
> You're not the first person to notice this. All the motivations are not
> necessarily painted clearly in their cover letter, but here are some
> previous attempts at solving this problem:
>
> [RFC PATCH v11 0/5] PCI: rockchip: Move PCIe WAKE# handling into pci core
> https://lkml.kernel.org/lkml/20171225114742.18920-1-jeffy.chen@rock-chips.com/
> http://lkml.kernel.org/lkml/20171226023646.17722-1-jeffy.chen@rock-chips.com/
>
> As you can see by the 12th iteration, it wasn't left unsolved for lack
> of trying...
I wasn't aware of this. That's definitely a better approach than my
hack, and I would really like this to be revived.
>
> Frankly, if a proper DT replacement to the admittedly bad binding isn't
> agreed upon quickly, I'd be very happy to just have WAKE# support
> removed from the DTS for now, and the existing mwifiex binding should
> just be removed. (Wake-on-WiFi was never properly vetted on these
> platforms anyway.) It mostly serves to just cause problems like you've
> noticed.
Agreed. If there is no actual use for this, and that we can build a case
for a better solution, let's remove the wakeup support from the Gru DT
(it is invalid anyway), and bring it back if and when we get the right
level of support.
[...]
> One problem Rockchip authors were also trying to resolve here is that
> PCIe WAKE# handling should not really be something the PCI device driver
> has to handle directly. Despite your complaints about not using in-band
> TLP wakeup, a separate WAKE# pin is in fact a documented part of the
> PCIe standard, and it so happens that the Rockchip RC does not support
> handling TLPs in S3, if you want to have decent power consumption. (Your
> "bad hardware" complaints could justifiably fall here, I suppose.)
>
> Additionally, I've had pushback from PCI driver authors/maintainers on
> adding more special handling for this sort of interrupt property (not
> the binding specifically, but just the concept of handling WAKE# in the
> driver), as they claim this should be handled by the system firmware,
> when they set the appropriate wakeup flags, which filter down to
> __pci_enable_wake() -> platform_pci_set_wakeup(). That's how x86 systems
> do it (note: I know for a fact that many have a very similar
> architecture -- WAKE# is not routed to the RC, because, why does it need
> to? and they *don't* use TLP wakeup either -- they just hide it in
> firmware better), and it Just Works.
Even on an arm64 platform, there is no reason why a wakeup interrupt
couldn't be handled by FW rather than the OS. It just need to be wired
to the right spot (so that it generates a secure interrupt that can be
handled by FW).
> So, we basically concluded that we should standardize on a way to
> describe WAKE# interrupts such that PCI drivers don't have to deal with
> it at all, and the PCI core can do it for us. 12 revisions later
> and...we still never agreed on a good device tree binding for this.
Is the DT binding the only problem? Do we have an agreement for the core
code?
> IOW, maybe your wake-up sub-node is the best way to side-step the
> problems of conflicting with the OF PCI spec. But I'd still really like
> to avoid parsing it in mwifiex, if at all possible.
Honestly, my solution is just a terrible hack. I wasn't aware that this
was a more general problem, and I'd love it to be addressed in the core
PCI code.
> (We'd still be left with the marvell,wakeup-pin propery to parse
> specifically in mwifiex, which sadly has to exist because....well,
> Samsung decided to do chip-on-board, and then they failed to use the
> correct pin on Marvell's side when wiring up WAKE#. Sigh.)
Oh well...
Thanks,
M.
--
Jazz is not dead. It just smells funny...
^ permalink raw reply
* Re: [PATCH 0/4] mwifiex PCI/wake-up interrupt fixes
From: Marc Zyngier @ 2019-02-27 9:27 UTC (permalink / raw)
To: Brian Norris
Cc: Ard Biesheuvel, Amitkumar Karwar, Enric Balletbo i Serra,
Ganapathi Bhat, Heiko Stuebner, Kalle Valo, Nishant Sarmukadam,
Rob Herring, Xinming Hu, Devicetree List,
<netdev@vger.kernel.org>,
<linux-wireless@vger.kernel.org>, Linux Kernel Mailing List,
linux-rockchip, David S. Miller, linux-arm-kernel, linux-pci,
linux-pm, Jeffy Chen
In-Reply-To: <20190226234422.GD174696@google.com>
On 26/02/2019 23:44, Brian Norris wrote:
> Hi,
>
> On Tue, Feb 26, 2019 at 05:14:00PM +0000, Marc Zyngier wrote:
>> On 26/02/2019 16:21, Ard Biesheuvel wrote:
>>> On Mon, 25 Feb 2019 at 15:53, Marc Zyngier <marc.zyngier@arm.com> wrote:
>>>> It outlines one thing: If you have to interpret per-device PCI
>>>> properties from DT, you're in for serious trouble. I should get some
>>>> better HW.
>>>>
>>>
>>> Yeah, it obviously makes no sense at all for the interrupt parent of a
>>> PCI device to deviate from the host bridge's interrupt parent, and
>>> it's quite unfortunate that we can't simply ban it now that the cat is
>>> out of the bag already.
>>>
>>> Arguably, the wake up widget is not part of the PCI device, but I have
>>> no opinion as to whether it is better modeling it as a sub device as
>>> you are proposing or as an entirely separate device referenced via a
>>> phandle.
>>
>> It is not that clear. The widget seems to be an integral part of the
>> device, as it is the same basic IP that is used for SDIO and USB.
>
> It's not really a widget specific to this IP. It's just a GPIO. It so
> happens that both SDIO and PCIe designs have wanted to use a GPIO for
> wakeup, as many other devices do. (Note: it's not just cheap ARM
> devices; pulling up some Intel Chromebook designs, I see the exact same
> WAKE# GPIO on their PCIe WiFi as well.)
Arghh! If I can't even point people to an Intel design as an example of
something done halfway right, we're screwed! ;-)
>
>> It looks like the good old pre-PCI-2.2 days, where you had to have a
>> separate cable between your network card and the base-board for the
>> wake-up interrupt to be delivered. Starting with PCI-2.2, the bus can
>> carry the signal just fine. With PCIe, it should just be an interrupt
>> TLP sent to the RC, but that's obviously not within the capabilities of
>> the HW.
>
> You should search the PCI Express specification for WAKE#. There is a
> clearly-documented "side-band wake" feature that is part of the
> standard, as an alternative to in-band TLP wakeup. While you claim this
> is an ancient thing, it in fact still in use on many systems -- it's
> just usually abstracted better by ACPI firmware, whereas the dirty
> laundry is aired a bit more on a Device Tree system. And we got it
> wrong.
I stand corrected. I was really hoping for these side-band wires to be a
thing of the past, but clearly the world hasn't moved as quickly as I hoped.
>> Anyway, it'd be good if the Marvell people could chime in and let us
>> know how they'd prefer to handle this.
>
> I'm not sure this is really a Marvell-specific problem. (Well, except
> for the marvell,wakeup-pin silliness, which is somewhat orthogonal.) In
> fact, if we cared a little more about Wake-on-WiFi, we'd be trying to
> support the same (out-of-band WAKE#) with other WiFi drivers.
I'd definitely like to see this standardized. It would give us more
coverage, and prevent everyone from doing their own thing. But as you
mention, WoW doesn't seem to get much traction.
Thanks,
M.
--
Jazz is not dead. It just smells funny...
^ permalink raw reply
* [PATCH 2/2] ath10k: Add support for per peer HTT tx stats for WCN3990
From: Surabhi Vishnoi @ 2019-02-27 7:13 UTC (permalink / raw)
To: ath10k; +Cc: linux-wireless, Surabhi Vishnoi
In-Reply-To: <1551251628-22518-1-git-send-email-svishnoi@codeaurora.org>
The firmware sends peer stats to the host driver if the firmware
advertises WMI_SERVICE_PEER_STATS service and the host driver
indicates the WMI_RSRC_CFG_FLAG_TX_PEER_STATS capability in the
host capab flag in wmi init cmd.
When peer stats are enabled, firmware sends one HTT event
HTT_TLV_T2H_MSG_TYPE_PEER_STATS for every four PPDUs transmitted.
HTT msg payload has tag followed by length followed by
success pkts/bytes, failed pkts/bytes, retry pkts/bytes and rate
info per ppdu.
Parse peer stats sent by the firmware in tlv format and update the
tx rate information and tx_stats debugfs entry per STA.
To get the tx_stats:
echo 1 > /sys/kernel/debug/ieee80211/phyX/ath10k/enable_extd_tx_stats
cat /sys/kernel/debug/ieee80211/phyX/net:wlanX/stations/xx:xx:xx:xx:xx:xx/tx_stats
Tested HW: WCN3990
Tested FW: WLAN.HL.3.1-00784-QCAHLSWMTPLZ-1
Signed-off-by: Surabhi Vishnoi <svishnoi@codeaurora.org>
---
drivers/net/wireless/ath/ath10k/htt.c | 2 +
drivers/net/wireless/ath/ath10k/htt.h | 64 ++++++++++++++
drivers/net/wireless/ath/ath10k/htt_rx.c | 136 +++++++++++++++++++++++++++++-
drivers/net/wireless/ath/ath10k/hw.c | 45 +++++++++-
drivers/net/wireless/ath/ath10k/hw.h | 34 ++++++++
drivers/net/wireless/ath/ath10k/wmi-tlv.c | 3 +
drivers/net/wireless/ath/ath10k/wmi-tlv.h | 33 ++++++++
drivers/net/wireless/ath/ath10k/wmi.h | 3 -
8 files changed, 312 insertions(+), 8 deletions(-)
diff --git a/drivers/net/wireless/ath/ath10k/htt.c b/drivers/net/wireless/ath/ath10k/htt.c
index d235ff3..828e76d 100644
--- a/drivers/net/wireless/ath/ath10k/htt.c
+++ b/drivers/net/wireless/ath/ath10k/htt.c
@@ -89,6 +89,8 @@
[HTT_TLV_T2H_MSG_TYPE_RX_OFLD_PKT_ERR] =
HTT_T2H_MSG_TYPE_RX_OFLD_PKT_ERR,
[HTT_TLV_T2H_MSG_TYPE_TEST] = HTT_T2H_MSG_TYPE_TEST,
+ [HTT_TLV_T2H_MSG_TYPE_PEER_STATS] =
+ HTT_T2H_MSG_TYPE_PEER_STATS,
};
static const enum htt_t2h_msg_type htt_10_4_t2h_msg_types[] = {
diff --git a/drivers/net/wireless/ath/ath10k/htt.h b/drivers/net/wireless/ath/ath10k/htt.h
index 0ab29bd..2c2b1c7 100644
--- a/drivers/net/wireless/ath/ath10k/htt.h
+++ b/drivers/net/wireless/ath/ath10k/htt.h
@@ -449,6 +449,12 @@ enum htt_tlv_t2h_msg_type {
HTT_TLV_T2H_MSG_TYPE_WDI_IPA_OP_RESPONSE = 0x14,
HTT_TLV_T2H_MSG_TYPE_CHAN_CHANGE = 0x15,
HTT_TLV_T2H_MSG_TYPE_RX_OFLD_PKT_ERR = 0x16,
+ HTT_TLV_T2H_MSG_TYPE_PEER_MAP_V2 = 0x1e,
+ HTT_TLV_T2H_MSG_TYPE_PEER_UNMAP_V2 = 0x1f,
+ HTT_TLV_T2H_MSG_TYPE_MONITOR_MAC_HEADER_IND = 0x20,
+ HTT_TLV_T2H_MSG_TYPE_FLOW_POOL_RESIZE = 0x21,
+ HTT_TLV_T2H_MSG_TYPE_CFR_DUMP_COMPL_IND = 0x22,
+ HTT_TLV_T2H_MSG_TYPE_PEER_STATS = 0x23,
HTT_TLV_T2H_MSG_TYPE_TEST,
/* keep this last */
HTT_TLV_T2H_NUM_MSGS
@@ -1604,6 +1610,63 @@ struct htt_channel_change {
__le32 phymode;
} __packed;
+#define HTT_TAG_ADDR_MASK GENMASK(11, 0)
+#define HTT_LEN_ADDR_MASK GENMASK(23, 12)
+#define HTT_LEN_ADDR_LSB 12
+
+struct htt_tlv_msg {
+ /* BIT [11 : 0] :- tag
+ * BIT [23 : 12] :- length
+ * BIT [31 : 24] :- reserved
+ */
+ __le32 tag_length;
+ u8 payload[0];
+};
+
+struct htt_tlv {
+ __le16 reserved1;
+ u8 reserved2;
+ u8 payload[0];
+};
+
+#define HTT_TX_STATS_IS_AMPDU_MASK GENMASK(0, 0)
+#define HTT_TX_STATS_BA_ACK_FAILED_MASK GENMASK(2, 1)
+#define HTT_TX_STATS_BW_MASK GENMASK(5, 3)
+#define HTT_TX_STATS_GI_MASK GENMASK(6, 6)
+#define HTT_TX_STATS_SKIPPED_RATE_CTRL_MASK GENMASK(7, 7)
+
+enum htt_tx_stats_valid_bitmap {
+ HTT_TX_STATS_VALID,
+ HTT_TX_STATS_SUCCESS_BYTES,
+ HTT_TX_STATS_RETRY_BYTES,
+ HTT_TX_STATS_FAILED_BYTES,
+ HTT_TX_STATS_RATECODE,
+ HTT_TX_STATS_IS_AMPDU,
+ HTT_TX_STATS_BA_ACK_FAILED,
+ HTT_TX_STATS_BW,
+ HTT_TX_STATS_GI,
+ HTT_TX_STATS_SKIPPED_RATE_CTRL,
+ HTT_TX_STATS_PEER_ID,
+ HTT_TX_STATS_SUCCESS_PKTS,
+ HTT_TX_STATS_RETRY_PKTS,
+ HTT_TX_STATS_FAILED_PKTS,
+ HTT_TX_STATS_TX_DURATION,
+};
+
+struct htt_tlv_per_peer_tx_stats_ind {
+ __le32 succ_bytes;
+ __le32 retry_bytes;
+ __le32 failed_bytes;
+ u8 ratecode;
+ u8 flags;
+ __le16 peer_id;
+ __le16 succ_pkts;
+ __le16 retry_pkts;
+ __le16 failed_pkts;
+ __le16 tx_duration;
+ __le32 valid_bitmap;
+} __packed;
+
struct htt_per_peer_tx_stats_ind {
__le32 succ_bytes;
__le32 retry_bytes;
@@ -1699,6 +1762,7 @@ struct htt_resp {
struct htt_tx_mode_switch_ind tx_mode_switch_ind;
struct htt_channel_change chan_change;
struct htt_peer_tx_stats peer_tx_stats;
+ struct htt_tlv tlv;
};
} __packed;
diff --git a/drivers/net/wireless/ath/ath10k/htt_rx.c b/drivers/net/wireless/ath/ath10k/htt_rx.c
index c7a2411..b3bbc8f 100644
--- a/drivers/net/wireless/ath/ath10k/htt_rx.c
+++ b/drivers/net/wireless/ath/ath10k/htt_rx.c
@@ -3039,11 +3039,12 @@ static inline s8 ath10k_get_legacy_rate_idx(struct ath10k *ar, u8 rate)
lockdep_assert_held(&ar->data_lock);
txrate.flags = ATH10K_HW_PREAMBLE(peer_stats->ratecode);
- txrate.bw = ATH10K_HW_BW(peer_stats->flags);
+ txrate.bw = ath10k_get_bw(&ar->hw_params, peer_stats->flags);
txrate.nss = ATH10K_HW_NSS(peer_stats->ratecode);
txrate.mcs = ATH10K_HW_MCS_RATE(peer_stats->ratecode);
- sgi = ATH10K_HW_GI(peer_stats->flags);
- skip_auto_rate = ATH10K_FW_SKIPPED_RATE_CTRL(peer_stats->flags);
+ sgi = ath10k_get_gi(&ar->hw_params, peer_stats->flags);
+ skip_auto_rate = ath10k_get_skipped_rate_ctrl(&ar->hw_params,
+ peer_stats->flags);
/* Firmware's rate control skips broadcast/management frames,
* if host has configure fixed rates and in some other special cases.
@@ -3146,6 +3147,133 @@ static inline s8 ath10k_get_legacy_rate_idx(struct ath10k *ar, u8 rate)
static void ath10k_htt_fetch_peer_stats_tlv(struct ath10k *ar,
struct sk_buff *skb)
{
+ struct ath10k_per_peer_tx_stats *p_tx_stats = &ar->peer_tx_stats;
+ struct htt_resp *resp = (struct htt_resp *)skb->data;
+ struct htt_tlv_per_peer_tx_stats_ind *tx_stats;
+ const struct htt_tlv_msg *tlv;
+ unsigned long valid_bitmap;
+ struct ieee80211_sta *sta;
+ struct ath10k_sta *arsta;
+ struct ath10k_peer *peer;
+ u16 tlv_tag, tlv_len;
+ void *begin, *ptr;
+ int peer_id, len;
+
+ begin = resp->tlv.payload;
+ ptr = begin;
+ len = skb->len - sizeof(struct htt_tlv);
+
+ while (len > 0) {
+ if (len < sizeof(*tlv)) {
+ ath10k_dbg(ar, ATH10K_DBG_HTT,
+ "HTT tlv parse failure at byte %zd (%d bytes left, %zu expected)\n",
+ ptr - begin, len, sizeof(*tlv));
+ return;
+ }
+
+ tlv = (struct htt_tlv_msg *)ptr;
+ tlv_tag = HTT_TAG_ADDR_MASK & __le32_to_cpu(tlv->tag_length);
+ tlv_len = MS(__le32_to_cpu(tlv->tag_length), HTT_LEN_ADDR);
+ ptr += sizeof(*tlv);
+ len -= sizeof(*tlv);
+
+ if (tlv_len > len) {
+ ath10k_dbg(ar, ATH10K_DBG_HTT,
+ "HTT tlv parse failure of tag %hu at byte %zd (%d bytes left, %hu expected)\n",
+ tlv_tag, ptr - begin, len, tlv_len);
+ return;
+ }
+
+ tx_stats = (struct htt_tlv_per_peer_tx_stats_ind *)ptr;
+ valid_bitmap = __le32_to_cpu(tx_stats->valid_bitmap);
+
+ if (test_bit(HTT_TX_STATS_VALID, &valid_bitmap)) {
+ if (test_bit(HTT_TX_STATS_PEER_ID, &valid_bitmap)) {
+ peer_id = __le16_to_cpu(tx_stats->peer_id);
+ } else {
+ ath10k_dbg(ar, ATH10K_DBG_HTT, "Peer id is not received, ignoring\n");
+ goto next;
+ }
+
+ rcu_read_lock();
+ spin_lock_bh(&ar->data_lock);
+ peer = ath10k_peer_find_by_id(ar, peer_id);
+ if (!peer) {
+ ath10k_warn(ar, "Invalid peer id %d peer stats buffer\n",
+ peer_id);
+ spin_unlock_bh(&ar->data_lock);
+ rcu_read_unlock();
+ return;
+ }
+
+ sta = peer->sta;
+ arsta = (struct ath10k_sta *)sta->drv_priv;
+ p_tx_stats->flags = 0;
+
+ if (test_bit(HTT_TX_STATS_SUCCESS_BYTES, &valid_bitmap))
+ p_tx_stats->succ_bytes =
+ __le32_to_cpu(tx_stats->succ_bytes);
+
+ if (test_bit(HTT_TX_STATS_RETRY_BYTES, &valid_bitmap))
+ p_tx_stats->retry_bytes =
+ __le32_to_cpu(tx_stats->retry_bytes);
+
+ if (test_bit(HTT_TX_STATS_FAILED_BYTES, &valid_bitmap))
+ p_tx_stats->failed_bytes =
+ __le32_to_cpu(tx_stats->failed_bytes);
+
+ if (test_bit(HTT_TX_STATS_RATECODE, &valid_bitmap))
+ p_tx_stats->ratecode = tx_stats->ratecode;
+
+ if (test_bit(HTT_TX_STATS_IS_AMPDU, &valid_bitmap))
+ p_tx_stats->flags |= tx_stats->flags &
+ HTT_TX_STATS_IS_AMPDU_MASK;
+
+ if (test_bit(HTT_TX_STATS_BA_ACK_FAILED, &valid_bitmap))
+ p_tx_stats->flags |= tx_stats->flags &
+ HTT_TX_STATS_BA_ACK_FAILED_MASK;
+
+ if (test_bit(HTT_TX_STATS_BW, &valid_bitmap))
+ p_tx_stats->flags |= tx_stats->flags &
+ HTT_TX_STATS_BW_MASK;
+
+ if (test_bit(HTT_TX_STATS_GI, &valid_bitmap))
+ p_tx_stats->flags |= tx_stats->flags &
+ HTT_TX_STATS_GI_MASK;
+
+ if (test_bit(HTT_TX_STATS_SKIPPED_RATE_CTRL,
+ &valid_bitmap))
+ p_tx_stats->flags |= tx_stats->flags &
+ HTT_TX_STATS_SKIPPED_RATE_CTRL_MASK;
+
+ if (test_bit(HTT_TX_STATS_SUCCESS_PKTS, &valid_bitmap))
+ p_tx_stats->succ_pkts =
+ __le16_to_cpu(tx_stats->succ_pkts);
+
+ if (test_bit(HTT_TX_STATS_RETRY_PKTS, &valid_bitmap))
+ p_tx_stats->retry_pkts =
+ __le16_to_cpu(tx_stats->retry_pkts);
+
+ if (test_bit(HTT_TX_STATS_FAILED_PKTS, &valid_bitmap))
+ p_tx_stats->failed_pkts =
+ __le16_to_cpu(tx_stats->failed_pkts);
+
+ if (test_bit(HTT_TX_STATS_TX_DURATION, &valid_bitmap))
+ p_tx_stats->duration =
+ __le16_to_cpu(tx_stats->tx_duration);
+
+ ath10k_update_per_peer_tx_stats(ar, sta, p_tx_stats);
+
+ spin_unlock_bh(&ar->data_lock);
+ rcu_read_unlock();
+ } else {
+ ath10k_dbg(ar, ATH10K_DBG_HTT, "received invalid bitmap 0x%lx for tag %hu at byte %zd\n",
+ valid_bitmap, tlv_tag, ptr - begin);
+ }
+next:
+ ptr += tlv_len;
+ len -= tlv_len;
+ }
}
static void ath10k_htt_fetch_peer_stats(struct ath10k *ar,
@@ -3438,7 +3566,7 @@ bool ath10k_htt_t2h_msg_handler(struct ath10k *ar, struct sk_buff *skb)
ath10k_htt_rx_tx_mode_switch_ind(ar, skb);
break;
case HTT_T2H_MSG_TYPE_PEER_STATS:
- ath10k_htt_fetch_peer_stats(ar, skb);
+ htt->rx_ops->htt_fetch_peer_stats(ar, skb);
break;
case HTT_T2H_MSG_TYPE_EN_STATS:
default:
diff --git a/drivers/net/wireless/ath/ath10k/hw.c b/drivers/net/wireless/ath/ath10k/hw.c
index eeaee8e..54b2735 100644
--- a/drivers/net/wireless/ath/ath10k/hw.c
+++ b/drivers/net/wireless/ath/ath10k/hw.c
@@ -1100,8 +1100,26 @@ int ath10k_hw_diag_fast_download(struct ath10k *ar,
return ret;
}
+static int ath10k_hw_bw(u8 flags)
+{
+ return FIELD_GET(ATH10K_HW_BW_MASK, flags);
+}
+
+static int ath10k_hw_gi(u8 flags)
+{
+ return FIELD_GET(ATH10K_HW_GI_MASK, flags);
+}
+
+static int ath10k_hw_skipped_rate_ctrl(u8 flags)
+{
+ return FIELD_GET(ATH10K_HW_SKIPPED_RATE_CTRL_MASK, flags);
+}
+
const struct ath10k_hw_ops qca988x_ops = {
.set_coverage_class = ath10k_hw_qca988x_set_coverage_class,
+ .get_bw = ath10k_hw_bw,
+ .get_gi = ath10k_hw_gi,
+ .get_skipped_rate_ctrl = ath10k_hw_skipped_rate_ctrl,
};
static int ath10k_qca99x0_rx_desc_get_l3_pad_bytes(struct htt_rx_desc *rxd)
@@ -1119,11 +1137,36 @@ static bool ath10k_qca99x0_rx_desc_msdu_limit_error(struct htt_rx_desc *rxd)
const struct ath10k_hw_ops qca99x0_ops = {
.rx_desc_get_l3_pad_bytes = ath10k_qca99x0_rx_desc_get_l3_pad_bytes,
.rx_desc_get_msdu_limit_error = ath10k_qca99x0_rx_desc_msdu_limit_error,
+ .get_bw = ath10k_hw_bw,
+ .get_gi = ath10k_hw_gi,
+ .get_skipped_rate_ctrl = ath10k_hw_skipped_rate_ctrl,
};
const struct ath10k_hw_ops qca6174_ops = {
.set_coverage_class = ath10k_hw_qca988x_set_coverage_class,
.enable_pll_clk = ath10k_hw_qca6174_enable_pll_clock,
+ .get_bw = ath10k_hw_bw,
+ .get_gi = ath10k_hw_gi,
+ .get_skipped_rate_ctrl = ath10k_hw_skipped_rate_ctrl,
};
-const struct ath10k_hw_ops wcn3990_ops = {};
+static int ath10k_hw_wcn3990_bw(u8 flags)
+{
+ return FIELD_GET(ATH10K_HW_WCN3990_BW_BIT_MASK, flags);
+}
+
+static int ath10k_hw_wcn3990_gi(u8 flags)
+{
+ return FIELD_GET(ATH10K_HW_WCN3990_GI_MASK, flags);
+}
+
+static int ath10k_hw_wcn3990_skipped_rate_ctrl(u8 flags)
+{
+ return FIELD_GET(ATH10K_HW_WCN3990_SKIPPED_RATE_CTRL_MASK, flags);
+}
+
+const struct ath10k_hw_ops wcn3990_ops = {
+ .get_bw = ath10k_hw_wcn3990_bw,
+ .get_gi = ath10k_hw_wcn3990_gi,
+ .get_skipped_rate_ctrl = ath10k_hw_wcn3990_skipped_rate_ctrl,
+};
diff --git a/drivers/net/wireless/ath/ath10k/hw.h b/drivers/net/wireless/ath/ath10k/hw.h
index de7dc01..737e4f7 100644
--- a/drivers/net/wireless/ath/ath10k/hw.h
+++ b/drivers/net/wireless/ath/ath10k/hw.h
@@ -159,6 +159,13 @@ enum qca9377_chip_id_rev {
#define REG_DUMP_COUNT_QCA988X 60
+#define ATH10K_HW_BW_MASK GENMASK(4, 3)
+#define ATH10K_HW_GI_MASK GENMASK(5, 5)
+#define ATH10K_HW_SKIPPED_RATE_CTRL_MASK GENMASK(6, 6)
+#define ATH10K_HW_WCN3990_BW_BIT_MASK GENMASK(5, 3)
+#define ATH10K_HW_WCN3990_GI_MASK GENMASK(6, 6)
+#define ATH10K_HW_WCN3990_SKIPPED_RATE_CTRL_MASK GENMASK(7, 7)
+
struct ath10k_fw_ie {
__le32 id;
__le32 len;
@@ -616,6 +623,9 @@ struct ath10k_hw_ops {
void (*set_coverage_class)(struct ath10k *ar, s16 value);
int (*enable_pll_clk)(struct ath10k *ar);
bool (*rx_desc_get_msdu_limit_error)(struct htt_rx_desc *rxd);
+ int (*get_bw)(u8 flags);
+ int (*get_gi)(u8 flags);
+ int (*get_skipped_rate_ctrl)(u8 flags);
};
extern const struct ath10k_hw_ops qca988x_ops;
@@ -643,6 +653,30 @@ struct ath10k_hw_ops {
return false;
}
+static inline int
+ath10k_get_bw(struct ath10k_hw_params *hw, u8 flags)
+{
+ if (hw->hw_ops->get_bw)
+ return hw->hw_ops->get_bw(flags);
+ return 0;
+}
+
+static inline int
+ath10k_get_gi(struct ath10k_hw_params *hw, u8 flags)
+{
+ if (hw->hw_ops->get_gi)
+ return hw->hw_ops->get_gi(flags);
+ return 0;
+}
+
+static inline int
+ath10k_get_skipped_rate_ctrl(struct ath10k_hw_params *hw, u8 flags)
+{
+ if (hw->hw_ops->get_skipped_rate_ctrl)
+ return hw->hw_ops->get_skipped_rate_ctrl(flags);
+ return 0;
+}
+
/* Target specific defines for MAIN firmware */
#define TARGET_NUM_VDEVS 8
#define TARGET_NUM_PEER_AST 2
diff --git a/drivers/net/wireless/ath/ath10k/wmi-tlv.c b/drivers/net/wireless/ath/ath10k/wmi-tlv.c
index 9b2ed0e..b700fe1 100644
--- a/drivers/net/wireless/ath/ath10k/wmi-tlv.c
+++ b/drivers/net/wireless/ath/ath10k/wmi-tlv.c
@@ -1706,6 +1706,9 @@ static struct sk_buff *ath10k_wmi_tlv_op_gen_init(struct ath10k *ar)
cfg->num_ocb_schedules = __cpu_to_le32(0);
cfg->host_capab = __cpu_to_le32(WMI_TLV_FLAG_MGMT_BUNDLE_TX_COMPL);
+ if (ath10k_peer_stats_enabled(ar))
+ cfg->host_capab |= __cpu_to_le32(WMI_RSRC_CFG_FLAG_TX_PEER_STATS);
+
ath10k_wmi_put_host_mem_chunks(ar, chunks);
ath10k_dbg(ar, ATH10K_DBG_WMI, "wmi tlv init\n");
diff --git a/drivers/net/wireless/ath/ath10k/wmi-tlv.h b/drivers/net/wireless/ath/ath10k/wmi-tlv.h
index 03d02ed..d5a090f 100644
--- a/drivers/net/wireless/ath/ath10k/wmi-tlv.h
+++ b/drivers/net/wireless/ath/ath10k/wmi-tlv.h
@@ -13,6 +13,7 @@
#define WMI_TLV_PDEV_PARAM_UNSUPPORTED 0
#define WMI_TLV_VDEV_PARAM_UNSUPPORTED 0
#define WMI_TLV_MGMT_TX_FRAME_MAX_LEN 64
+#define WMI_RSRC_CFG_FLAG_TX_PEER_STATS BIT(21)
enum wmi_tlv_grp_id {
WMI_TLV_GRP_START = 0x3,
@@ -1384,6 +1385,36 @@ enum wmi_tlv_service {
WMI_TLV_SERVICE_AP_TWT = 153,
WMI_TLV_SERVICE_GMAC_OFFLOAD_SUPPORT = 154,
WMI_TLV_SERVICE_SPOOF_MAC_SUPPORT = 155,
+ WMI_TLV_SERVICE_PEER_TID_CONFIGS_SUPPORT = 156,
+ WMI_TLV_SERVICE_VDEV_SWRETRY_PER_AC_CONFIG_SUPPORT = 157,
+ WMI_TLV_SERVICE_DUAL_BEACON_ON_SINGLE_MAC_SCC_SUPPORT = 158,
+ WMI_TLV_SERVICE_DUAL_BEACON_ON_SINGLE_MAC_MCC_SUPPORT = 159,
+ WMI_TLV_SERVICE_MOTION_DET = 160,
+ WMI_TLV_SERVICE_INFRA_MBSSID = 161,
+ WMI_TLV_SERVICE_OBSS_SPATIAL_REUSE = 162,
+ WMI_TLV_SERVICE_VDEV_DIFFERENT_BEACON_INTERVAL_SUPPORT = 163,
+ WMI_TLV_SERVICE_NAN_DBS_SUPPORT = 164,
+ WMI_TLV_SERVICE_NDI_DBS_SUPPORT = 165,
+ WMI_TLV_SERVICE_NAN_SAP_SUPPORT = 166,
+ WMI_TLV_SERVICE_NDI_SAP_SUPPORT = 167,
+ WMI_TLV_SERVICE_CFR_CAPTURE_SUPPORT = 168,
+ WMI_TLV_SERVICE_CFR_CAPTURE_IND_MSG_TYPE_1 = 169,
+ WMI_TLV_SERVICE_ESP_SUPPORT = 170,
+ WMI_TLV_SERVICE_PEER_CHWIDTH_CHANGE = 171,
+ WMI_TLV_SERVICE_WLAN_HPCS_PULSE = 172,
+ WMI_TLV_SERVICE_PER_VDEV_CHAINMASK_CONFIG_SUPPORT = 173,
+ WMI_TLV_SERVICE_TX_DATA_MGMT_ACK_RSSI = 174,
+ WMI_TLV_SERVICE_NAN_DISABLE_SUPPORT = 175,
+ WMI_TLV_SERVICE_HTT_H2T_NO_HTC_HDR_LEN_IN_MSG_LEN = 176,
+ WMI_TLV_SERVICE_COEX_SUPPORT_UNEQUAL_ISOLATION = 177,
+ WMI_TLV_SERVICE_HW_DB2DBM_CONVERSION_SUPPORT = 178,
+ WMI_TLV_SERVICE_SUPPORT_EXTEND_ADDRESS = 179,
+ WMI_TLV_SERVICE_BEACON_RECEPTION_STATS = 180,
+ WMI_TLV_SERVICE_FETCH_TX_PN = 181,
+ WMI_TLV_SERVICE_PEER_UNMAP_RESPONSE_SUPPORT = 182,
+ WMI_TLV_SERVICE_TX_PER_PEER_AMPDU_SIZE = 183,
+ WMI_TLV_SERVICE_BSS_COLOR_SWITCH_COUNT = 184,
+ WMI_TLV_SERVICE_PEER_STATS = 185,
WMI_TLV_MAX_EXT_SERVICE = 256,
};
@@ -1557,6 +1588,8 @@ enum wmi_tlv_service {
SVCMAP(WMI_TLV_SERVICE_THERM_THROT,
WMI_SERVICE_THERM_THROT,
WMI_TLV_MAX_SERVICE);
+ SVCMAP(WMI_TLV_SERVICE_PEER_STATS,
+ WMI_SERVICE_PEER_STATS, WMI_TLV_MAX_SERVICE);
}
#undef SVCMAP
diff --git a/drivers/net/wireless/ath/ath10k/wmi.h b/drivers/net/wireless/ath/ath10k/wmi.h
index 39a6568..e89066b 100644
--- a/drivers/net/wireless/ath/ath10k/wmi.h
+++ b/drivers/net/wireless/ath/ath10k/wmi.h
@@ -5043,13 +5043,10 @@ enum wmi_rate_preamble {
#define ATH10K_HW_PREAMBLE(rate) (((rate) >> 6) & 0x3)
#define ATH10K_HW_MCS_RATE(rate) ((rate) & 0xf)
#define ATH10K_HW_LEGACY_RATE(rate) ((rate) & 0x3f)
-#define ATH10K_HW_BW(flags) (((flags) >> 3) & 0x3)
-#define ATH10K_HW_GI(flags) (((flags) >> 5) & 0x1)
#define ATH10K_HW_RATECODE(rate, nss, preamble) \
(((preamble) << 6) | ((nss) << 4) | (rate))
#define ATH10K_HW_AMPDU(flags) ((flags) & 0x1)
#define ATH10K_HW_BA_FAIL(flags) (((flags) >> 1) & 0x3)
-#define ATH10K_FW_SKIPPED_RATE_CTRL(flags) (((flags) >> 6) & 0x1)
#define ATH10K_VHT_MCS_NUM 10
#define ATH10K_BW_NUM 4
--
1.9.1
^ permalink raw reply related
* [PATCH 1/2] ath10k: Add support for adding htt_rx_ops based on htt_version
From: Surabhi Vishnoi @ 2019-02-27 7:13 UTC (permalink / raw)
To: ath10k; +Cc: linux-wireless, Surabhi Vishnoi
In-Reply-To: <1551251628-22518-1-git-send-email-svishnoi@codeaurora.org>
Refactor the code to add the support to attach htt_rx_ops
based on HTT version.
Tested HW: WCN3990
Tested FW: WLAN.HL.3.1-00784-QCAHLSWMTPLZ-1
Signed-off-by: Surabhi Vishnoi <svishnoi@codeaurora.org>
---
drivers/net/wireless/ath/ath10k/htt.h | 3 ++-
drivers/net/wireless/ath/ath10k/htt_rx.c | 29 ++++++++++++++++++++++++++---
2 files changed, 28 insertions(+), 4 deletions(-)
diff --git a/drivers/net/wireless/ath/ath10k/htt.h b/drivers/net/wireless/ath/ath10k/htt.h
index fef716a..0ab29bd 100644
--- a/drivers/net/wireless/ath/ath10k/htt.h
+++ b/drivers/net/wireless/ath/ath10k/htt.h
@@ -1894,7 +1894,7 @@ struct ath10k_htt {
bool tx_mem_allocated;
const struct ath10k_htt_tx_ops *tx_ops;
- const struct ath10k_htt_rx_ops *rx_ops;
+ struct ath10k_htt_rx_ops *rx_ops;
};
struct ath10k_htt_tx_ops {
@@ -1969,6 +1969,7 @@ struct ath10k_htt_rx_ops {
int idx);
void* (*htt_get_vaddr_ring)(struct ath10k_htt *htt);
void (*htt_reset_paddrs_ring)(struct ath10k_htt *htt, int idx);
+ void (*htt_fetch_peer_stats)(struct ath10k *ar, struct sk_buff *skb);
};
static inline size_t ath10k_htt_get_rx_ring_size(struct ath10k_htt *htt)
diff --git a/drivers/net/wireless/ath/ath10k/htt_rx.c b/drivers/net/wireless/ath/ath10k/htt_rx.c
index 4fc8856..c7a2411 100644
--- a/drivers/net/wireless/ath/ath10k/htt_rx.c
+++ b/drivers/net/wireless/ath/ath10k/htt_rx.c
@@ -3143,6 +3143,11 @@ static inline s8 ath10k_get_legacy_rate_idx(struct ath10k *ar, u8 rate)
rate_idx);
}
+static void ath10k_htt_fetch_peer_stats_tlv(struct ath10k *ar,
+ struct sk_buff *skb)
+{
+}
+
static void ath10k_htt_fetch_peer_stats(struct ath10k *ar,
struct sk_buff *skb)
{
@@ -3556,7 +3561,7 @@ int ath10k_htt_txrx_compl_task(struct ath10k *ar, int budget)
}
EXPORT_SYMBOL(ath10k_htt_txrx_compl_task);
-static const struct ath10k_htt_rx_ops htt_rx_ops_32 = {
+static struct ath10k_htt_rx_ops htt_rx_ops_32 = {
.htt_get_rx_ring_size = ath10k_htt_get_rx_ring_size_32,
.htt_config_paddrs_ring = ath10k_htt_config_paddrs_ring_32,
.htt_set_paddrs_ring = ath10k_htt_set_paddrs_ring_32,
@@ -3564,7 +3569,10 @@ int ath10k_htt_txrx_compl_task(struct ath10k *ar, int budget)
.htt_reset_paddrs_ring = ath10k_htt_reset_paddrs_ring_32,
};
-static const struct ath10k_htt_rx_ops htt_rx_ops_64 = {
+/* FIXME: Some other way to attach ops to static const htt rx_ops
+ * without removing const??
+ */
+static struct ath10k_htt_rx_ops htt_rx_ops_64 = {
.htt_get_rx_ring_size = ath10k_htt_get_rx_ring_size_64,
.htt_config_paddrs_ring = ath10k_htt_config_paddrs_ring_64,
.htt_set_paddrs_ring = ath10k_htt_set_paddrs_ring_64,
@@ -3572,7 +3580,7 @@ int ath10k_htt_txrx_compl_task(struct ath10k *ar, int budget)
.htt_reset_paddrs_ring = ath10k_htt_reset_paddrs_ring_64,
};
-static const struct ath10k_htt_rx_ops htt_rx_ops_hl = {
+static struct ath10k_htt_rx_ops htt_rx_ops_hl = {
};
void ath10k_htt_set_rx_ops(struct ath10k_htt *htt)
@@ -3585,4 +3593,19 @@ void ath10k_htt_set_rx_ops(struct ath10k_htt *htt)
htt->rx_ops = &htt_rx_ops_64;
else
htt->rx_ops = &htt_rx_ops_32;
+
+ switch (ar->running_fw->fw_file.htt_op_version) {
+ case ATH10K_FW_HTT_OP_VERSION_MAIN:
+ case ATH10K_FW_HTT_OP_VERSION_10_1:
+ case ATH10K_FW_HTT_OP_VERSION_10_4:
+ htt->rx_ops->htt_fetch_peer_stats = ath10k_htt_fetch_peer_stats;
+ break;
+ case ATH10K_FW_HTT_OP_VERSION_TLV:
+ htt->rx_ops->htt_fetch_peer_stats = ath10k_htt_fetch_peer_stats_tlv;
+ break;
+ case ATH10K_FW_HTT_OP_VERSION_MAX:
+ case ATH10K_FW_HTT_OP_VERSION_UNSET:
+ WARN_ON(1);
+ return;
+ }
}
--
1.9.1
^ permalink raw reply related
* [PATCH 0/2] Add support for per peer HTT tx stats for WCN3990
From: Surabhi Vishnoi @ 2019-02-27 7:13 UTC (permalink / raw)
To: ath10k; +Cc: linux-wireless, Surabhi Vishnoi
WCN3990 firmware sends the HTT_TLV_T2H_MSG_TYPE_PEER_STATS in tlv
format so the handling of this message has to be done in different
manner.
This patchset adds support to attach rx_ops based on htt version
and adds the handling of HTT_TLV_T2H_MSG_TYPE_PEER_STATS message
based on HTT version.
Surabhi Vishnoi (2):
ath10k: Add support for adding htt_rx_ops based on htt_version
ath10k: Add support for per peer HTT tx stats for WCN3990
drivers/net/wireless/ath/ath10k/htt.c | 2 +
drivers/net/wireless/ath/ath10k/htt.h | 67 +++++++++++-
drivers/net/wireless/ath/ath10k/htt_rx.c | 165 ++++++++++++++++++++++++++++--
drivers/net/wireless/ath/ath10k/hw.c | 45 +++++++-
drivers/net/wireless/ath/ath10k/hw.h | 34 ++++++
drivers/net/wireless/ath/ath10k/wmi-tlv.c | 3 +
drivers/net/wireless/ath/ath10k/wmi-tlv.h | 33 ++++++
drivers/net/wireless/ath/ath10k/wmi.h | 3 -
8 files changed, 340 insertions(+), 12 deletions(-)
--
1.9.1
^ permalink raw reply
* [PATCH 0/2] Add support for per peer HTT tx stats for WCN3990
From: Surabhi Vishnoi @ 2019-02-27 7:12 UTC (permalink / raw)
To: ath10k; +Cc: linux-wireless, Surabhi Vishnoi
WCN3990 firmware sends the HTT_TLV_T2H_MSG_TYPE_PEER_STATS in tlv
format so the handling of this message has to be done in different
manner.
This patchset adds support to attach rx_ops based on htt version
and adds the handling of HTT_TLV_T2H_MSG_TYPE_PEER_STATS message
based on HTT version.
Surabhi Vishnoi (2):
ath10k: Add support for adding htt_rx_ops based on htt_version
ath10k: Add support for per peer HTT tx stats for WCN3990
drivers/net/wireless/ath/ath10k/htt.c | 2 +
drivers/net/wireless/ath/ath10k/htt.h | 67 +++++++++++-
drivers/net/wireless/ath/ath10k/htt_rx.c | 165 ++++++++++++++++++++++++++++--
drivers/net/wireless/ath/ath10k/hw.c | 45 +++++++-
drivers/net/wireless/ath/ath10k/hw.h | 34 ++++++
drivers/net/wireless/ath/ath10k/wmi-tlv.c | 3 +
drivers/net/wireless/ath/ath10k/wmi-tlv.h | 33 ++++++
drivers/net/wireless/ath/ath10k/wmi.h | 3 -
8 files changed, 340 insertions(+), 12 deletions(-)
--
1.9.1
^ permalink raw reply
* Re: [PATCHv2 1/9] nl80211: New netlink command for TID specific configuration
From: Tamizh chelvam @ 2019-02-27 6:03 UTC (permalink / raw)
To: Sergey Matyukevich; +Cc: johannes, ath10k, linux-wireless
In-Reply-To: <20190226122901.focow4odsbkmv7jn@bars>
Hi Sergey,
>
>> Signed-off-by: Tamizh chelvam <tamizhr@codeaurora.org>
>> ---
>> include/net/cfg80211.h | 35 +++++++++++++++
>> include/uapi/linux/nl80211.h | 51 ++++++++++++++++++++++
>> net/wireless/nl80211.c | 102
>> +++++++++++++++++++++++++++++++++++++++++++
>> net/wireless/rdev-ops.h | 11 +++++
>> net/wireless/trace.h | 18 ++++++++
>> 5 files changed, 217 insertions(+)
>
> ...
>
>> diff --git a/net/wireless/nl80211.c b/net/wireless/nl80211.c
>> index 82d5e1e..352eb4a2 100644
>> --- a/net/wireless/nl80211.c
>> +++ b/net/wireless/nl80211.c
>> @@ -280,6 +280,13 @@ static int validate_ie_attr(const struct nlattr
>> *attr,
>>
>> NLA_POLICY_NESTED_ARRAY(nl80211_psmr_peer_attr_policy),
>> };
>>
>> +static const struct nla_policy
>> +nl80211_attr_tid_config_policy[NL80211_ATTR_TID_CONFIG_MAX + 1] = {
>> + [NL80211_ATTR_TID_CONFIG_TID] = NLA_POLICY_MAX(NLA_U8, 7),
>
> Such a policy permits configuration of per-TID settings, either
> for per-STA or for all the STAs. However it is not possible to
> perform configuration for all TIDs at once, e.g. passing -1 value
> to driver.
>
> Maybe simplify policy and use .type = NLA_U8 ?
>
> Sanity check, if needed, can be performed by driver or even by
> firmware.
>
Sure, we can have like that. And do you feel driver should advertise
support to perform configuration for all TIDs(like accepting tid -1) ?
>> + [NL80211_ATTR_TID_CONFIG_NOACK] =
>> + NLA_POLICY_MAX(NLA_U8,
>> NL80211_TID_CONFIG_DISABLE),
>> +};
>> +
>> const struct nla_policy nl80211_policy[NUM_NL80211_ATTR] = {
>> [NL80211_ATTR_WIPHY] = { .type = NLA_U32 },
>> [NL80211_ATTR_WIPHY_NAME] = { .type = NLA_NUL_STRING,
>> @@ -541,6 +548,8 @@ static int validate_ie_attr(const struct nlattr
>> *attr,
>> [NL80211_ATTR_PEER_MEASUREMENTS] =
>> NLA_POLICY_NESTED(nl80211_pmsr_attr_policy),
>> [NL80211_ATTR_AIRTIME_WEIGHT] = NLA_POLICY_MIN(NLA_U16, 1),
>> + [NL80211_ATTR_TID_CONFIG] =
>> +
>> NLA_POLICY_NESTED(nl80211_attr_tid_config_policy),
>> };
>
> ...
>
>> +static int nl80211_set_tid_config(struct sk_buff *skb,
>> + struct genl_info *info)
>> +{
>> + struct cfg80211_registered_device *rdev = info->user_ptr[0];
>> + struct nlattr *attrs[NL80211_ATTR_TID_CONFIG_MAX + 1];
>> + struct net_device *dev = info->user_ptr[1];
>> + struct ieee80211_tid_config *tid_conf;
>> + struct nlattr *tid;
>> + int conf_idx = 0, rem_conf;
>> + u32 num_conf = 0, size_of_conf;
>> + int ret = -EINVAL;
>> +
>> + if (!info->attrs[NL80211_ATTR_TID_CONFIG])
>> + return -EINVAL;
>> +
>> + if (!rdev->ops->set_tid_config)
>> + return -EOPNOTSUPP;
>> +
>> + nla_for_each_nested(tid, info->attrs[NL80211_ATTR_TID_CONFIG],
>> + rem_conf)
>> + num_conf++;
>> +
>> + size_of_conf = sizeof(struct ieee80211_tid_config) +
>> + num_conf * sizeof(struct ieee80211_tid_cfg);
>> +
>> + tid_conf = kzalloc(size_of_conf, GFP_KERNEL);
>> + if (!tid_conf)
>> + return -ENOMEM;
>> +
>> + tid_conf->n_tid_conf = num_conf;
>> +
>> + if (info->attrs[NL80211_ATTR_MAC])
>> + tid_conf->peer =
>> nla_data(info->attrs[NL80211_ATTR_MAC]);
>> + else
>> + tid_conf->peer = NULL;
>> +
>> + nla_for_each_nested(tid, info->attrs[NL80211_ATTR_TID_CONFIG],
>> + rem_conf) {
>> + ret = nla_parse_nested(attrs,
>> NL80211_ATTR_TID_CONFIG_MAX, tid,
>> + NULL, NULL);
>> +
>> + if (ret)
>> + return ret;
>> +
>> + if (!attrs[NL80211_ATTR_TID_CONFIG_TID])
>> + return -EINVAL;
>> +
>> + ret = parse_tid_conf(rdev, attrs,
>> &tid_conf->tid_conf[conf_idx],
>> + tid_conf->peer);
>> + if (ret)
>> + goto bad_tid_conf;
>> +
>> + conf_idx++;
>> + }
>> +
>> + return rdev_set_tid_config(rdev, dev, tid_conf);
>
> What is the ownership rule for tid_conf ? In other words, who is
> responsible for freeing this structure ?
>
> Unless I am missing something, in the suggested patches there is no
> kfree for this structure and all the nested structures (see Tx bitrate
> patch), neither in cfg80211/mac80211 nor in ath10k.
>
> As far as I understand, we have two options here. One option is to
> explicitly specify that ownership is passed to the caller, similar
> to nl80211_set_reg. Another option is to release memory in this
> function after driver makes copies of all necessary fields,
> e.g. see nl80211_set_mac_acl.
>
Thanks for pointing out, will free it in this function(cfg80211) itself.
I will make the change and send next patchset.
>> +
>> +bad_tid_conf:
>> + kfree(tid_conf);
>> + return ret;
>> +}
Thanks,
Tamizh.
^ permalink raw reply
* Realtek r8822be kernel module does not negotiate 802.11ac connection
From: David R. Bergstein @ 2019-02-27 4:09 UTC (permalink / raw)
To: pkshih, linux-wireless, netdev, linux-kernel
[-- Attachment #1.1: Type: text/plain, Size: 521 bytes --]
This message is in regard to a bug I have open on bugs.launchpad.net,
1813372, linked below. This issue, originally identified in an Ubuntu
kernel, has been duplicated in the most current mainline kernel,
5.0-rc8, and is in regard to problems attaining a wireless connection at
802.11ac speeds.
https://bugs.launchpad.net/bugs/1813372
At your earliest convenience, please see the bug report above, and
advise if a fix will be available for the r8822be kernel module.
Sincerely,
David R. Bergstein
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 659 bytes --]
^ permalink raw reply
* [PATCH] rtlwifi: rtl8192se: Remove set but not used variable 'seg_ptr'
From: YueHaibing @ 2019-02-27 2:23 UTC (permalink / raw)
To: Ping-Ke Shih, Kalle Valo, Larry Finger
Cc: YueHaibing, linux-wireless, netdev, kernel-janitors
Fixes gcc '-Wunused-but-set-variable' warning:
drivers/net/wireless/realtek/rtlwifi/rtl8192se/fw.c: In function '_rtl92s_firmware_downloadcode':
drivers/net/wireless/realtek/rtlwifi/rtl8192se/fw.c:139:17: warning:
variable 'seg_ptr' set but not used [-Wunused-but-set-variable]
It's not used after commit 59ae1d127ac0 ("networking: introduce and use
skb_put_data()")
Signed-off-by: YueHaibing <yuehaibing@huawei.com>
---
drivers/net/wireless/realtek/rtlwifi/rtl8192se/fw.c | 6 ++----
1 file changed, 2 insertions(+), 4 deletions(-)
diff --git a/drivers/net/wireless/realtek/rtlwifi/rtl8192se/fw.c b/drivers/net/wireless/realtek/rtlwifi/rtl8192se/fw.c
index faa307a0b148..541b7881735e 100644
--- a/drivers/net/wireless/realtek/rtlwifi/rtl8192se/fw.c
+++ b/drivers/net/wireless/realtek/rtlwifi/rtl8192se/fw.c
@@ -136,7 +136,6 @@ static bool _rtl92s_firmware_downloadcode(struct ieee80211_hw *hw,
struct rtl_priv *rtlpriv = rtl_priv(hw);
struct sk_buff *skb;
struct rtl_tcb_desc *tcb_desc;
- unsigned char *seg_ptr;
u16 frag_threshold = MAX_FIRMWARE_CODE_SIZE;
u16 frag_length, frag_offset = 0;
u16 extra_descoffset = 0;
@@ -166,9 +165,8 @@ static bool _rtl92s_firmware_downloadcode(struct ieee80211_hw *hw,
if (!skb)
return false;
skb_reserve(skb, extra_descoffset);
- seg_ptr = skb_put_data(skb,
- code_virtual_address + frag_offset,
- (u32)(frag_length - extra_descoffset));
+ skb_put_data(skb, code_virtual_address + frag_offset,
+ (u32)(frag_length - extra_descoffset));
tcb_desc = (struct rtl_tcb_desc *)(skb->cb);
tcb_desc->queue_index = TXCMD_QUEUE;
^ permalink raw reply related
* [PATCH] rtlwifi: rtl8723ae: Remove set but not used variable 'bt_retry_cnt'
From: YueHaibing @ 2019-02-27 2:23 UTC (permalink / raw)
To: Ping-Ke Shih, Kalle Valo, Larry Finger
Cc: YueHaibing, linux-wireless, netdev, kernel-janitors
Fixes gcc '-Wunused-but-set-variable' warning:
drivers/net/wireless/realtek/rtlwifi/rtl8723ae/hal_btc.c: In function '_rtl8723e_dm_bt_coexist_2_ant':
drivers/net/wireless/realtek/rtlwifi/rtl8723ae/hal_btc.c:1408:5: warning:
variable 'bt_retry_cnt' set but not used [-Wunused-but-set-variable]
It's never used and can be removed.
Signed-off-by: YueHaibing <yuehaibing@huawei.com>
---
drivers/net/wireless/realtek/rtlwifi/rtl8723ae/hal_btc.c | 2 --
1 file changed, 2 deletions(-)
diff --git a/drivers/net/wireless/realtek/rtlwifi/rtl8723ae/hal_btc.c b/drivers/net/wireless/realtek/rtlwifi/rtl8723ae/hal_btc.c
index a6b31dae5691..680198280f8f 100644
--- a/drivers/net/wireless/realtek/rtlwifi/rtl8723ae/hal_btc.c
+++ b/drivers/net/wireless/realtek/rtlwifi/rtl8723ae/hal_btc.c
@@ -1405,7 +1405,6 @@ static void rtl8723e_dm_bt_reset_action_profile_state(struct ieee80211_hw *hw)
static void _rtl8723e_dm_bt_coexist_2_ant(struct ieee80211_hw *hw)
{
struct rtl_priv *rtlpriv = rtl_priv(hw);
- u8 bt_retry_cnt;
u8 bt_info_original;
RT_TRACE(rtlpriv, COMP_BT_COEXIST, DBG_DMESG,
"[BTCoex] Get bt info by fw!!\n");
@@ -1417,7 +1416,6 @@ static void _rtl8723e_dm_bt_coexist_2_ant(struct ieee80211_hw *hw)
"[BTCoex] c2h for bt_info not rcvd yet!!\n");
}
- bt_retry_cnt = hal_coex_8723.bt_retry_cnt;
bt_info_original = hal_coex_8723.c2h_bt_info_original;
/* when bt inquiry or page scan, we have to set h2c 0x25 */
^ permalink raw reply related
* [PATCH] rtlwifi: rtl8723be: Remove set but not used variable 'b_last_is_cur_rdlstate'
From: YueHaibing @ 2019-02-27 2:23 UTC (permalink / raw)
To: Ping-Ke Shih, Kalle Valo, Larry Finger
Cc: YueHaibing, linux-wireless, netdev, kernel-janitors
Fixes gcc '-Wunused-but-set-variable' warning:
drivers/net/wireless/realtek/rtlwifi/rtl8723be/dm.c: In function 'rtl8723be_dm_check_edca_turbo':
drivers/net/wireless/realtek/rtlwifi/rtl8723be/dm.c:998:7: warning:
variable 'b_last_is_cur_rdlstate' set but not used [-Wunused-but-set-variable]
It's never used and can be removed.
Signed-off-by: YueHaibing <yuehaibing@huawei.com>
---
drivers/net/wireless/realtek/rtlwifi/rtl8723be/dm.c | 3 ---
1 file changed, 3 deletions(-)
diff --git a/drivers/net/wireless/realtek/rtlwifi/rtl8723be/dm.c b/drivers/net/wireless/realtek/rtlwifi/rtl8723be/dm.c
index ef355aa88117..b13fd3c0c832 100644
--- a/drivers/net/wireless/realtek/rtlwifi/rtl8723be/dm.c
+++ b/drivers/net/wireless/realtek/rtlwifi/rtl8723be/dm.c
@@ -995,12 +995,9 @@ static void rtl8723be_dm_check_edca_turbo(struct ieee80211_hw *hw)
u32 edca_be = 0x5ea42b;
u32 iot_peer = 0;
bool b_is_cur_rdlstate;
- bool b_last_is_cur_rdlstate = false;
bool b_bias_on_rx = false;
bool b_edca_turbo_on = false;
- b_last_is_cur_rdlstate = rtlpriv->dm.is_cur_rdlstate;
-
cur_txok_cnt = rtlpriv->stats.txbytesunicast - last_txok_cnt;
cur_rxok_cnt = rtlpriv->stats.rxbytesunicast - last_rxok_cnt;
^ permalink raw reply related
* Re: [PATCH 0/4] mwifiex PCI/wake-up interrupt fixes
From: Brian Norris @ 2019-02-26 23:44 UTC (permalink / raw)
To: Marc Zyngier
Cc: Ard Biesheuvel, Amitkumar Karwar, Enric Balletbo i Serra,
Ganapathi Bhat, Heiko Stuebner, Kalle Valo, Nishant Sarmukadam,
Rob Herring, Xinming Hu, Devicetree List,
<netdev@vger.kernel.org>,
<linux-wireless@vger.kernel.org>, Linux Kernel Mailing List,
linux-rockchip, David S. Miller, linux-arm-kernel, linux-pci,
linux-pm, Jeffy Chen
In-Reply-To: <0c433a70-27f6-76ad-c46c-6015de1ffaa4@arm.com>
Hi,
On Tue, Feb 26, 2019 at 05:14:00PM +0000, Marc Zyngier wrote:
> On 26/02/2019 16:21, Ard Biesheuvel wrote:
> > On Mon, 25 Feb 2019 at 15:53, Marc Zyngier <marc.zyngier@arm.com> wrote:
> >> It outlines one thing: If you have to interpret per-device PCI
> >> properties from DT, you're in for serious trouble. I should get some
> >> better HW.
> >>
> >
> > Yeah, it obviously makes no sense at all for the interrupt parent of a
> > PCI device to deviate from the host bridge's interrupt parent, and
> > it's quite unfortunate that we can't simply ban it now that the cat is
> > out of the bag already.
> >
> > Arguably, the wake up widget is not part of the PCI device, but I have
> > no opinion as to whether it is better modeling it as a sub device as
> > you are proposing or as an entirely separate device referenced via a
> > phandle.
>
> It is not that clear. The widget seems to be an integral part of the
> device, as it is the same basic IP that is used for SDIO and USB.
It's not really a widget specific to this IP. It's just a GPIO. It so
happens that both SDIO and PCIe designs have wanted to use a GPIO for
wakeup, as many other devices do. (Note: it's not just cheap ARM
devices; pulling up some Intel Chromebook designs, I see the exact same
WAKE# GPIO on their PCIe WiFi as well.)
> It looks like the good old pre-PCI-2.2 days, where you had to have a
> separate cable between your network card and the base-board for the
> wake-up interrupt to be delivered. Starting with PCI-2.2, the bus can
> carry the signal just fine. With PCIe, it should just be an interrupt
> TLP sent to the RC, but that's obviously not within the capabilities of
> the HW.
You should search the PCI Express specification for WAKE#. There is a
clearly-documented "side-band wake" feature that is part of the
standard, as an alternative to in-band TLP wakeup. While you claim this
is an ancient thing, it in fact still in use on many systems -- it's
just usually abstracted better by ACPI firmware, whereas the dirty
laundry is aired a bit more on a Device Tree system. And we got it
wrong.
> Anyway, it'd be good if the Marvell people could chime in and let us
> know how they'd prefer to handle this.
I'm not sure this is really a Marvell-specific problem. (Well, except
for the marvell,wakeup-pin silliness, which is somewhat orthogonal.) In
fact, if we cared a little more about Wake-on-WiFi, we'd be trying to
support the same (out-of-band WAKE#) with other WiFi drivers.
Brian
^ permalink raw reply
* Re: [PATCH 3/4] mwifiex: Flag wake-up interrupt as IRQ_NOAUTOEN rather than disabling it too late
From: Brian Norris @ 2019-02-26 23:34 UTC (permalink / raw)
To: Marc Zyngier
Cc: Amitkumar Karwar, Enric Balletbo i Serra, Ganapathi Bhat,
Heiko Stuebner, Kalle Valo, Nishant Sarmukadam, Rob Herring,
Xinming Hu, David S. Miller, devicetree, linux-arm-kernel,
linux-kernel, linux-rockchip, linux-wireless, netdev
In-Reply-To: <20190226233130.GB174696@google.com>
On Tue, Feb 26, 2019 at 03:31:31PM -0800, Brian Norris wrote:
> Hi Marc,
>
> On Sun, Feb 24, 2019 at 02:04:25PM +0000, Marc Zyngier wrote:
> > The mwifiex driver makes unsafe assumptions about the state of the
> > wake-up interrupt. It requests it and only then disable it. Of
> > course, the interrupt may be screaming for whatever reason at that
> > time, and the handler will then be called without the interrupt
> > having been registered with the PM/wakeup subsystem. Oops.
> >
> > The right way to handle this kind of situation is to flag the
> > interrupt with IRQ_NOAUTOEN before requesting it. It will then
> > stay disabled until someone (the wake-up subsystem) enables it.
> >
> > Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
>
> This could be:
>
> Fixes: 853402a00823 ("mwifiex: Enable WoWLAN for both sdio and pcie")
Also, this comes after a different change (that's not quite as clearly a
backport-able bugfix). Perhaps it should go first in the series?
Brian
> although, that was just borrowing the existing antipattern from SDIO
> code..
>
> Reviewed-by: Brian Norris <briannorris@chromium.org>
>
> Thanks.
>
> > ---
> > drivers/net/wireless/marvell/mwifiex/main.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/net/wireless/marvell/mwifiex/main.c b/drivers/net/wireless/marvell/mwifiex/main.c
> > index 2105c2b7c627..82cf35e50579 100644
> > --- a/drivers/net/wireless/marvell/mwifiex/main.c
> > +++ b/drivers/net/wireless/marvell/mwifiex/main.c
> > @@ -1610,6 +1610,7 @@ static void mwifiex_probe_of(struct mwifiex_adapter *adapter)
> > "wake-up interrupt outside 'wake-up' subnode of %pOF\n",
> > adapter->dt_node);
> >
> > + irq_set_status_flags(adapter->irq_wakeup, IRQ_NOAUTOEN);
> > ret = devm_request_irq(dev, adapter->irq_wakeup,
> > mwifiex_irq_wakeup_handler, IRQF_TRIGGER_LOW,
> > "wifi_wake", adapter);
> > @@ -1619,7 +1620,6 @@ static void mwifiex_probe_of(struct mwifiex_adapter *adapter)
> > goto err_exit;
> > }
> >
> > - disable_irq(adapter->irq_wakeup);
> > if (device_init_wakeup(dev, true)) {
> > dev_err(dev, "fail to init wakeup for mwifiex\n");
> > goto err_exit;
> > --
> > 2.20.1
> >
^ permalink raw reply
* Re: [PATCH 3/4] mwifiex: Flag wake-up interrupt as IRQ_NOAUTOEN rather than disabling it too late
From: Brian Norris @ 2019-02-26 23:31 UTC (permalink / raw)
To: Marc Zyngier
Cc: Amitkumar Karwar, Enric Balletbo i Serra, Ganapathi Bhat,
Heiko Stuebner, Kalle Valo, Nishant Sarmukadam, Rob Herring,
Xinming Hu, David S. Miller, devicetree, linux-arm-kernel,
linux-kernel, linux-rockchip, linux-wireless, netdev
In-Reply-To: <20190224140426.3267-4-marc.zyngier@arm.com>
Hi Marc,
On Sun, Feb 24, 2019 at 02:04:25PM +0000, Marc Zyngier wrote:
> The mwifiex driver makes unsafe assumptions about the state of the
> wake-up interrupt. It requests it and only then disable it. Of
> course, the interrupt may be screaming for whatever reason at that
> time, and the handler will then be called without the interrupt
> having been registered with the PM/wakeup subsystem. Oops.
>
> The right way to handle this kind of situation is to flag the
> interrupt with IRQ_NOAUTOEN before requesting it. It will then
> stay disabled until someone (the wake-up subsystem) enables it.
>
> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
This could be:
Fixes: 853402a00823 ("mwifiex: Enable WoWLAN for both sdio and pcie")
although, that was just borrowing the existing antipattern from SDIO
code..
Reviewed-by: Brian Norris <briannorris@chromium.org>
Thanks.
> ---
> drivers/net/wireless/marvell/mwifiex/main.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/net/wireless/marvell/mwifiex/main.c b/drivers/net/wireless/marvell/mwifiex/main.c
> index 2105c2b7c627..82cf35e50579 100644
> --- a/drivers/net/wireless/marvell/mwifiex/main.c
> +++ b/drivers/net/wireless/marvell/mwifiex/main.c
> @@ -1610,6 +1610,7 @@ static void mwifiex_probe_of(struct mwifiex_adapter *adapter)
> "wake-up interrupt outside 'wake-up' subnode of %pOF\n",
> adapter->dt_node);
>
> + irq_set_status_flags(adapter->irq_wakeup, IRQ_NOAUTOEN);
> ret = devm_request_irq(dev, adapter->irq_wakeup,
> mwifiex_irq_wakeup_handler, IRQF_TRIGGER_LOW,
> "wifi_wake", adapter);
> @@ -1619,7 +1620,6 @@ static void mwifiex_probe_of(struct mwifiex_adapter *adapter)
> goto err_exit;
> }
>
> - disable_irq(adapter->irq_wakeup);
> if (device_init_wakeup(dev, true)) {
> dev_err(dev, "fail to init wakeup for mwifiex\n");
> goto err_exit;
> --
> 2.20.1
>
^ permalink raw reply
* Re: [PATCH 0/4] mwifiex PCI/wake-up interrupt fixes
From: Brian Norris @ 2019-02-26 23:28 UTC (permalink / raw)
To: Marc Zyngier
Cc: Amitkumar Karwar, Enric Balletbo i Serra, Ganapathi Bhat,
Heiko Stuebner, Kalle Valo, Nishant Sarmukadam, Rob Herring,
Xinming Hu, David S. Miller, devicetree, linux-arm-kernel,
linux-kernel, linux-rockchip, linux-wireless, netdev, linux-pm,
Jeffy Chen, Rafael J. Wysocki, Tony Lindgren
In-Reply-To: <20190224140426.3267-1-marc.zyngier@arm.com>
+ others
Hi Marc,
Thanks for the series. I have a few bits of history to add to this, and
some comments.
On Sun, Feb 24, 2019 at 02:04:22PM +0000, Marc Zyngier wrote:
> For quite some time, I wondered why the PCI mwifiex device built in my
> Chromebook was unable to use the good old legacy interrupts. But as MSIs
> were working fine, I never really bothered investigating. I finally had a
> look, and the result isn't very pretty.
>
> On this machine (rk3399-based kevin), the wake-up interrupt is described as
> such:
>
> &pci_rootport {
> mvl_wifi: wifi@0,0 {
> compatible = "pci1b4b,2b42";
> reg = <0x83010000 0x0 0x00000000 0x0 0x00100000
> 0x83010000 0x0 0x00100000 0x0 0x00100000>;
> interrupt-parent = <&gpio0>;
> interrupts = <8 IRQ_TYPE_LEVEL_LOW>;
> pinctrl-names = "default";
> pinctrl-0 = <&wlan_host_wake_l>;
> wakeup-source;
> };
> };
>
> Note how the interrupt is part of the properties directly attached to the
> PCI node. And yet, this interrupt has nothing to do with a PCI legacy
> interrupt, as it is attached to the wake-up widget that bypasses the PCIe RC
> altogether (Yay for the broken design!). This is in total violation of the
> IEEE Std 1275-1994 spec[1], which clearly documents that such interrupt
> specifiers describe the PCI device interrupts, and must obey the
> INT-{A,B,C,D} mapping. Oops!
You're not the first person to notice this. All the motivations are not
necessarily painted clearly in their cover letter, but here are some
previous attempts at solving this problem:
[RFC PATCH v11 0/5] PCI: rockchip: Move PCIe WAKE# handling into pci core
https://lkml.kernel.org/lkml/20171225114742.18920-1-jeffy.chen@rock-chips.com/
http://lkml.kernel.org/lkml/20171226023646.17722-1-jeffy.chen@rock-chips.com/
As you can see by the 12th iteration, it wasn't left unsolved for lack
of trying...
Frankly, if a proper DT replacement to the admittedly bad binding isn't
agreed upon quickly, I'd be very happy to just have WAKE# support
removed from the DTS for now, and the existing mwifiex binding should
just be removed. (Wake-on-WiFi was never properly vetted on these
platforms anyway.) It mostly serves to just cause problems like you've
noticed.
> The net effect of the above is that Linux tries to do something vaguely
> sensible, and uses the same interrupt for both the wake-up widget and the
> PCI device. This doesn't work for two reasons: (1) the wake-up widget grabs
> the interrupt in exclusive mode, and (2) the PCI interrupt is still routed
> to the RC, leading to a screaming interrupt. This simply cannot work.
>
> To sort out this mess, we need to lift the confusion between the two
> interrupts. This is done by extending the DT binding to allow the wake-up
> interrupt to be described in a 'wake-up' subnode, sidestepping the issue
> completely. On my Chromebook, it now looks like this:
>
> &pci_rootport {
> mvl_wifi: wifi@0,0 {
> compatible = "pci1b4b,2b42";
> reg = <0x83010000 0x0 0x00000000 0x0 0x00100000
> 0x83010000 0x0 0x00100000 0x0 0x00100000>;
> pinctrl-names = "default";
> pinctrl-0 = <&wlan_host_wake_l>;
> wake-up {
> interrupt-parent = <&gpio0>;
> interrupts = <8 IRQ_TYPE_LEVEL_LOW>;
> wakeup-source;
> };
> };
> };
One problem Rockchip authors were also trying to resolve here is that
PCIe WAKE# handling should not really be something the PCI device driver
has to handle directly. Despite your complaints about not using in-band
TLP wakeup, a separate WAKE# pin is in fact a documented part of the
PCIe standard, and it so happens that the Rockchip RC does not support
handling TLPs in S3, if you want to have decent power consumption. (Your
"bad hardware" complaints could justifiably fall here, I suppose.)
Additionally, I've had pushback from PCI driver authors/maintainers on
adding more special handling for this sort of interrupt property (not
the binding specifically, but just the concept of handling WAKE# in the
driver), as they claim this should be handled by the system firmware,
when they set the appropriate wakeup flags, which filter down to
__pci_enable_wake() -> platform_pci_set_wakeup(). That's how x86 systems
do it (note: I know for a fact that many have a very similar
architecture -- WAKE# is not routed to the RC, because, why does it need
to? and they *don't* use TLP wakeup either -- they just hide it in
firmware better), and it Just Works.
So, we basically concluded that we should standardize on a way to
describe WAKE# interrupts such that PCI drivers don't have to deal with
it at all, and the PCI core can do it for us. 12 revisions later
and...we still never agreed on a good device tree binding for this.
IOW, maybe your wake-up sub-node is the best way to side-step the
problems of conflicting with the OF PCI spec. But I'd still really like
to avoid parsing it in mwifiex, if at all possible.
(We'd still be left with the marvell,wakeup-pin propery to parse
specifically in mwifiex, which sadly has to exist because....well,
Samsung decided to do chip-on-board, and then they failed to use the
correct pin on Marvell's side when wiring up WAKE#. Sigh.)
> The driver is then updated to look for this subnode first, and fallback to
> the original, broken behaviour (spitting out a warning in the offending
> configuration).
>
> For good measure, there are two additional patches:
>
> - The wake-up interrupt requesting is horribly racy, and could lead to
> unpredictable behaviours. Let's fix that properly.
Ack. That mistake was repeated in other drivers and has since been fixed
in those. We need it here too.
Brian
> - A final patch implementing the above transformation for the whole
> RK3399-based Chromebook range, which all use the same broken
> configuration.
>
> With all that, I finally have PCI legacy interrupts working with the mwifiex
> driver on my Chromebook.
>
> [1] http://www.devicetree.org/open-firmware/bindings/pci/pci2_1.pdf
>
> Marc Zyngier (4):
> dt-bindings/marvell-8xxx: Allow wake-up interrupt to be placed in a
> separate node
> mwifiex: Fetch wake-up interrupt from 'wake-up' subnode when it exists
> mwifiex: Flag wake-up interrupt as IRQ_NOAUTOEN rather than disabling
> it too late
> arm64: dts: rockchip: gru: Move wifi wake-up interrupt into its own
> subnode
>
> .../bindings/net/wireless/marvell-8xxx.txt | 23 ++++++++++++++++++-
> .../dts/rockchip/rk3399-gru-chromebook.dtsi | 8 ++++---
> drivers/net/wireless/marvell/mwifiex/main.c | 13 +++++++++--
> 3 files changed, 38 insertions(+), 6 deletions(-)
>
> --
> 2.20.1
>
^ permalink raw reply
* Re: iw 5.0.1 broken when compiled with lto
From: Hauke Mehrtens @ 2019-02-26 22:34 UTC (permalink / raw)
To: johannes; +Cc: linux-wireless, stijn, fercerpav, Mantas Pucka
In-Reply-To: <c450c890-a2f6-d598-c81c-8428144cf41a@hauke-m.de>
On 2/26/19 9:59 PM, Hauke Mehrtens wrote:
> Hi Johannes,
>
> When I compile iw 5.0.1 with link time optimization for MIPS32r2 big
> endian all commands are removed, it looks like this:
> ------------------------------------------------------------------------
> root@OpenWrt:/# iw
> Usage: iw [options] command
> Options:
> --debug enable netlink debugging
> --version show version (5.0.1)
> Commands:
>
> Commands that use the netdev ('dev') can also be given the
> 'wdev' instead to identify the device.
>
> You can omit the 'phy' or 'dev' if the identification is unique,
> e.g. "iw wlan0 info" or "iw phy0 info". (Don't when scripting.)
>
> Do NOT screenscrape this tool, we don't consider its output stable.
>
> root@OpenWrt:/#
> ------------------------------------------------------------------------
>
> When I compile it without LTO it works like expected.
> This was seen in OpenWrt in the iw-full version and when I remove all
> the OpenWrt specific patches to iw. The iw-tiny version from OpenWrt
> works like expected. iw 4.14 also works in both versions.
>
> This was report to OpenWrt here:
> https://github.com/openwrt/openwrt/pull/1865
> http://lists.infradead.org/pipermail/openwrt-devel/2018-October/014220.html
>
> With GCC 8.2 this also happens with iw 4.14.
>
>
> The binary with lto is even bigger and they have the same number of
> functions starting with __cmd:
>
> $ ls -al iw-full-*
> -rwxr-xr-x 1 hauke hauke 180164 Feb 26 21:55 iw-full-lto
> -rwxr-xr-x 1 hauke hauke 179016 Feb 26 21:57 iw-full-wo-lto
> $ nm iw-full-lto |fgrep __cmd |wc
> 96 288 7259
> $ nm iw-full-wo-lto |fgrep __cmd |wc
> 96 288 7259
>
> Hauke
Paul Fertser did some further analysis of this problem and we can now
reproduce it also on normal x86 systems when the binary is compiled like
this:
CFLAGS=" -Os -flto" LDFLAGS=" -flto" make
-O3 or -Os and link time optimization is needed to see this problem.
With LTO the compiler randomly reorders the __section_* variables
including __section_set and __section_get and that leads to wrong offset
used to traverse the __cmd section.
Hauke
^ permalink raw reply
* Re: [PATCH] {nl,mac}80211: allow 4addr AP operation on crypto controlled devices
From: Hauke Mehrtens @ 2019-02-26 22:01 UTC (permalink / raw)
To: Manikanta Pubbisetty, johannes; +Cc: linux-wireless, Stefan Lippers-Hollmann
In-Reply-To: <1542798228-3293-1-git-send-email-mpubbise@codeaurora.org>
On 11/21/18 12:03 PM, Manikanta Pubbisetty wrote:
> As per the current design, for sw crypto controlled devices, it is
> the device which has to advertise the support for AP/VLAN iftype
> based on it's capability to tranmsit packets encrypted in software
> (In VLAN functionality, group traffic generated for a specific
> VLAN group is always encrypted in software). Commit db3bdcb9c3ff
> ("mac80211: allow AP_VLAN operation on crypto controlled devices")
> has introduced this change.
>
> Since 4addr AP operation also uses AP/VLAN iftype, this conditional
> way of advertising AP/VLAN support has broken 4addr AP mode operation on
> crypto controlled devices which do not support VLAN functionality.
>
> For example:
> In the case of ath10k driver, not all firmwares have support for VLAN
> functionality but all can support 4addr AP operation. Because AP/VLAN
> support is not advertised for these devices, 4addr AP operations are
> also blocked.
>
> Fix this by allowing 4addr opertion on devices which do not advertise
> AP/VLAN iftype but which can support 4addr operation (the desicion is
> taken based on the wiphy flag WIPHY_FLAG_4ADDR_AP).
>
> Fixes: Commit db3bdcb9c3ff ("mac80211: allow AP_VLAN operation on
> crypto controlled devices")
> Signed-off-by: Manikanta Pubbisetty <mpubbise@codeaurora.org>
> ---
> include/net/cfg80211.h | 3 ++-
> net/mac80211/util.c | 4 +++-
> net/wireless/core.c | 9 +++++++--
> net/wireless/nl80211.c | 10 ++++++++--
> 4 files changed, 20 insertions(+), 6 deletions(-)
>
Hi Manikanta,
Will you send an updated version of this patch to mainline?
Multiple people ran into this problem in OpenWrt and it would be nice if
such a fix would be included in upstream kernel.
See here a pull request:
https://github.com/openwrt/openwrt/pull/1640
Hauke
^ permalink raw reply
* Re: [PATCH] mac80211: allocate tailroom for forwarded mesh packets
From: Sasha Levin @ 2019-02-26 21:23 UTC (permalink / raw)
To: Sasha Levin, Felix Fietkau, linux-wireless
Cc: johannes, stable, stable, stable
In-Reply-To: <20190222122115.68485-1-nbd@nbd.name>
Hi,
[This is an automated email]
This commit has been processed because it contains a -stable tag.
The stable tag indicates that it's relevant for the following trees: all
The bot has tested the following trees: v4.20.12, v4.19.25, v4.14.103, v4.9.160, v4.4.176, v3.18.136.
v4.20.12: Build OK!
v4.19.25: Build OK!
v4.14.103: Failed to apply! Possible dependencies:
0c3d5a96d5e5 ("net: drivers/net: Remove unnecessary skb_copy_expand OOM messages")
v4.9.160: Failed to apply! Possible dependencies:
0c3d5a96d5e5 ("net: drivers/net: Remove unnecessary skb_copy_expand OOM messages")
c38c39bf7cc0 ("mac80211: Fix headroom allocation when forwarding mesh pkt")
d8da0b5d64d5 ("mac80211: Ensure enough headroom when forwarding mesh pkt")
v4.4.176: Failed to apply! Possible dependencies:
0c3d5a96d5e5 ("net: drivers/net: Remove unnecessary skb_copy_expand OOM messages")
c38c39bf7cc0 ("mac80211: Fix headroom allocation when forwarding mesh pkt")
d8da0b5d64d5 ("mac80211: Ensure enough headroom when forwarding mesh pkt")
v3.18.136: Failed to apply! Possible dependencies:
0c3d5a96d5e5 ("net: drivers/net: Remove unnecessary skb_copy_expand OOM messages")
c38c39bf7cc0 ("mac80211: Fix headroom allocation when forwarding mesh pkt")
d8da0b5d64d5 ("mac80211: Ensure enough headroom when forwarding mesh pkt")
How should we proceed with this patch?
--
Thanks,
Sasha
^ permalink raw reply
* iw 5.0.1 broken when compiled with lto
From: Hauke Mehrtens @ 2019-02-26 20:59 UTC (permalink / raw)
To: johannes; +Cc: linux-wireless, stijn
Hi Johannes,
When I compile iw 5.0.1 with link time optimization for MIPS32r2 big
endian all commands are removed, it looks like this:
------------------------------------------------------------------------
root@OpenWrt:/# iw
Usage: iw [options] command
Options:
--debug enable netlink debugging
--version show version (5.0.1)
Commands:
Commands that use the netdev ('dev') can also be given the
'wdev' instead to identify the device.
You can omit the 'phy' or 'dev' if the identification is unique,
e.g. "iw wlan0 info" or "iw phy0 info". (Don't when scripting.)
Do NOT screenscrape this tool, we don't consider its output stable.
root@OpenWrt:/#
------------------------------------------------------------------------
When I compile it without LTO it works like expected.
This was seen in OpenWrt in the iw-full version and when I remove all
the OpenWrt specific patches to iw. The iw-tiny version from OpenWrt
works like expected. iw 4.14 also works in both versions.
This was report to OpenWrt here:
https://github.com/openwrt/openwrt/pull/1865
http://lists.infradead.org/pipermail/openwrt-devel/2018-October/014220.html
With GCC 8.2 this also happens with iw 4.14.
The binary with lto is even bigger and they have the same number of
functions starting with __cmd:
$ ls -al iw-full-*
-rwxr-xr-x 1 hauke hauke 180164 Feb 26 21:55 iw-full-lto
-rwxr-xr-x 1 hauke hauke 179016 Feb 26 21:57 iw-full-wo-lto
$ nm iw-full-lto |fgrep __cmd |wc
96 288 7259
$ nm iw-full-wo-lto |fgrep __cmd |wc
96 288 7259
Hauke
^ permalink raw reply
* Re: [PATCH 0/4] mwifiex PCI/wake-up interrupt fixes
From: Marc Zyngier @ 2019-02-26 17:14 UTC (permalink / raw)
To: Ard Biesheuvel
Cc: Amitkumar Karwar, Enric Balletbo i Serra, Ganapathi Bhat,
Heiko Stuebner, Kalle Valo, Nishant Sarmukadam, Rob Herring,
Xinming Hu, Devicetree List, <netdev@vger.kernel.org>,
<linux-wireless@vger.kernel.org>, Linux Kernel Mailing List,
linux-rockchip, David S. Miller, linux-arm-kernel
In-Reply-To: <CAKv+Gu_=bQyVZqgUnJm9moCcaCcpDrag6NWGEpaR65=pMiTFJQ@mail.gmail.com>
On 26/02/2019 16:21, Ard Biesheuvel wrote:
> On Mon, 25 Feb 2019 at 15:53, Marc Zyngier <marc.zyngier@arm.com> wrote:
>>
>> Hi Ard,
>>
>> On 25/02/2019 12:45, Ard Biesheuvel wrote:
>>> On Sun, 24 Feb 2019 at 15:08, Marc Zyngier <marc.zyngier@arm.com> wrote:
>>>>
>>>> For quite some time, I wondered why the PCI mwifiex device built in my
>>>> Chromebook was unable to use the good old legacy interrupts. But as MSIs
>>>> were working fine, I never really bothered investigating. I finally had a
>>>> look, and the result isn't very pretty.
>>>>
>>>> On this machine (rk3399-based kevin), the wake-up interrupt is described as
>>>> such:
>>>>
>>>> &pci_rootport {
>>>> mvl_wifi: wifi@0,0 {
>>>> compatible = "pci1b4b,2b42";
>>>> reg = <0x83010000 0x0 0x00000000 0x0 0x00100000
>>>> 0x83010000 0x0 0x00100000 0x0 0x00100000>;
>>>> interrupt-parent = <&gpio0>;
>>>> interrupts = <8 IRQ_TYPE_LEVEL_LOW>;
>>>> pinctrl-names = "default";
>>>> pinctrl-0 = <&wlan_host_wake_l>;
>>>> wakeup-source;
>>>> };
>>>> };
>>>>
>>>> Note how the interrupt is part of the properties directly attached to the
>>>> PCI node. And yet, this interrupt has nothing to do with a PCI legacy
>>>> interrupt, as it is attached to the wake-up widget that bypasses the PCIe RC
>>>> altogether (Yay for the broken design!). This is in total violation of the
>>>> IEEE Std 1275-1994 spec[1], which clearly documents that such interrupt
>>>> specifiers describe the PCI device interrupts, and must obey the
>>>> INT-{A,B,C,D} mapping. Oops!
>>>>
>>>
>>> The mapping of legacy PCIe INTx interrupts onto wired system
>>> interrupts is a property of the PCIe host controller, not of a
>>> particular PCIe device. So I would argue that the code is broken here
>>> as well: it should never attempt to interpret 'interrupt' properties
>>> at the PCI device level as having any bearing on how legacy interrupts
>>> are routed.
>>
>> OpenFirmware says that this node contains the interrupt number of the
>> device (4.1.1. Open Firmware-defined Properties for Child Nodes). The
>> trick is that this property is generated *from* the device, and not set
>> in stone.
>>
>> DT, on the other hand, takes whatever is described there and uses it as
>> the gospel to configure the OS, no matter how the PCI device is actually
>> configured. If the two don't match (like in this case), things break.
>> This is the "DT describes the HW" mantra, for (sometimes) better or
>> (more generally) worse.
>>
>> What the DT code does is to interpret the whole interrupt specifier,
>> *including the interrupt-parent*. And that feels wrong. It should always
>> be in the context of the host controller. But on the other side, the DT
>> code is not in the business of validating the DT either...
>>
>> It outlines one thing: If you have to interpret per-device PCI
>> properties from DT, you're in for serious trouble. I should get some
>> better HW.
>>
>
> Yeah, it obviously makes no sense at all for the interrupt parent of a
> PCI device to deviate from the host bridge's interrupt parent, and
> it's quite unfortunate that we can't simply ban it now that the cat is
> out of the bag already.
>
> Arguably, the wake up widget is not part of the PCI device, but I have
> no opinion as to whether it is better modeling it as a sub device as
> you are proposing or as an entirely separate device referenced via a
> phandle.
It is not that clear. The widget seems to be an integral part of the
device, as it is the same basic IP that is used for SDIO and USB.
It looks like the good old pre-PCI-2.2 days, where you had to have a
separate cable between your network card and the base-board for the
wake-up interrupt to be delivered. Starting with PCI-2.2, the bus can
carry the signal just fine. With PCIe, it should just be an interrupt
TLP sent to the RC, but that's obviously not within the capabilities of
the HW.
Anyway, it'd be good if the Marvell people could chime in and let us
know how they'd prefer to handle this.
Thanks,
M.
--
Jazz is not dead. It just smells funny...
^ permalink raw reply
* Re: [PATCH 0/4] mwifiex PCI/wake-up interrupt fixes
From: Ard Biesheuvel @ 2019-02-26 16:21 UTC (permalink / raw)
To: Marc Zyngier
Cc: Amitkumar Karwar, Enric Balletbo i Serra, Ganapathi Bhat,
Heiko Stuebner, Kalle Valo, Nishant Sarmukadam, Rob Herring,
Xinming Hu, Devicetree List, <netdev@vger.kernel.org>,
<linux-wireless@vger.kernel.org>, Linux Kernel Mailing List,
linux-rockchip, David S. Miller, linux-arm-kernel
In-Reply-To: <5310b73b-4821-6dff-b9c0-34c59fb7fd72@arm.com>
On Mon, 25 Feb 2019 at 15:53, Marc Zyngier <marc.zyngier@arm.com> wrote:
>
> Hi Ard,
>
> On 25/02/2019 12:45, Ard Biesheuvel wrote:
> > On Sun, 24 Feb 2019 at 15:08, Marc Zyngier <marc.zyngier@arm.com> wrote:
> >>
> >> For quite some time, I wondered why the PCI mwifiex device built in my
> >> Chromebook was unable to use the good old legacy interrupts. But as MSIs
> >> were working fine, I never really bothered investigating. I finally had a
> >> look, and the result isn't very pretty.
> >>
> >> On this machine (rk3399-based kevin), the wake-up interrupt is described as
> >> such:
> >>
> >> &pci_rootport {
> >> mvl_wifi: wifi@0,0 {
> >> compatible = "pci1b4b,2b42";
> >> reg = <0x83010000 0x0 0x00000000 0x0 0x00100000
> >> 0x83010000 0x0 0x00100000 0x0 0x00100000>;
> >> interrupt-parent = <&gpio0>;
> >> interrupts = <8 IRQ_TYPE_LEVEL_LOW>;
> >> pinctrl-names = "default";
> >> pinctrl-0 = <&wlan_host_wake_l>;
> >> wakeup-source;
> >> };
> >> };
> >>
> >> Note how the interrupt is part of the properties directly attached to the
> >> PCI node. And yet, this interrupt has nothing to do with a PCI legacy
> >> interrupt, as it is attached to the wake-up widget that bypasses the PCIe RC
> >> altogether (Yay for the broken design!). This is in total violation of the
> >> IEEE Std 1275-1994 spec[1], which clearly documents that such interrupt
> >> specifiers describe the PCI device interrupts, and must obey the
> >> INT-{A,B,C,D} mapping. Oops!
> >>
> >
> > The mapping of legacy PCIe INTx interrupts onto wired system
> > interrupts is a property of the PCIe host controller, not of a
> > particular PCIe device. So I would argue that the code is broken here
> > as well: it should never attempt to interpret 'interrupt' properties
> > at the PCI device level as having any bearing on how legacy interrupts
> > are routed.
>
> OpenFirmware says that this node contains the interrupt number of the
> device (4.1.1. Open Firmware-defined Properties for Child Nodes). The
> trick is that this property is generated *from* the device, and not set
> in stone.
>
> DT, on the other hand, takes whatever is described there and uses it as
> the gospel to configure the OS, no matter how the PCI device is actually
> configured. If the two don't match (like in this case), things break.
> This is the "DT describes the HW" mantra, for (sometimes) better or
> (more generally) worse.
>
> What the DT code does is to interpret the whole interrupt specifier,
> *including the interrupt-parent*. And that feels wrong. It should always
> be in the context of the host controller. But on the other side, the DT
> code is not in the business of validating the DT either...
>
> It outlines one thing: If you have to interpret per-device PCI
> properties from DT, you're in for serious trouble. I should get some
> better HW.
>
Yeah, it obviously makes no sense at all for the interrupt parent of a
PCI device to deviate from the host bridge's interrupt parent, and
it's quite unfortunate that we can't simply ban it now that the cat is
out of the bag already.
Arguably, the wake up widget is not part of the PCI device, but I have
no opinion as to whether it is better modeling it as a sub device as
you are proposing or as an entirely separate device referenced via a
phandle.
^ permalink raw reply
* Re: [PATCH RFC] mac80211: Use IFF_ECHO to force delivery of tx_status frames
From: Johannes Berg @ 2019-02-26 13:33 UTC (permalink / raw)
To: Julius Niedworok, Oliver Hartkopp
Cc: linux-wireless, ga58taw, David Hildenbrand, nc, David S. Miller,
Edward Cree, Jiri Pirko, Ido Schimmel, Petr Machata, Kirill Tkhai,
Alexander Duyck, Amritha Nambiar, Li RongQing, netdev,
linux-kernel
In-Reply-To: <6A213CBE-0B6A-466A-B721-E6A728D4888D@gmx.net>
On Tue, 2019-02-26 at 14:13 +0100, Julius Niedworok wrote:
>
> Thank you for the explanation - I can adjust the comment, if you like to.
>
> > So what are you getting back after you enabled IFF_ECHO on your mac80211 device?
> >
> > Is it just a 'status' about a sent packet, or is it the packet ('full content') itself?
>
> We are actually getting back the full content of the packet. So it
> matches the behaviour of the 'echo' in CAN.
I don't think it does, really.
In CAN, if I understand correctly, this is used for regular operation
interfaces, where you might want to run 'tcpdump', on wifi the
equivalent would be 'tcpdump -i wlan0'. This *already* implements full
visibility of outgoing and incoming frames.
Not sure how CAN even manages *not to*, but I don't really need to care
:-)
You're proposing to add this to the *monitor* interfaces and you really
should have made the flag conditional on that to make that clear.
However, even on monitor interfaces, you typically *already* see the
frames you transmitted there (as raw frames, which is the only thing you
can do).
What you're proposing is to use IFF_ECHO to show frames transmitted
through *other* interfaces on the monitor interface.
I don't think the IFF_ECHO semantics really match this.
Additionally, drivers are sort of free to ignore the REQ_TX_STATUS, or
we could in the future add ways of using the _noskb to feed back TX
status to the state machines where needed, so I'm not really sure I even
_want_ this to be set in stone in such an API.
Now, I can also see how this can be useful for debugging, but it feels
to me like this should be a driver (debug) option?
johannes
^ permalink raw reply
* Re: [PATCH RFC] mac80211: Use IFF_ECHO to force delivery of tx_status frames
From: Julius Niedworok @ 2019-02-26 13:13 UTC (permalink / raw)
To: Oliver Hartkopp
Cc: linux-wireless, ga58taw, David Hildenbrand, nc, David S. Miller,
Johannes Berg, Edward Cree, Jiri Pirko, Ido Schimmel,
Petr Machata, Kirill Tkhai, Alexander Duyck, Amritha Nambiar,
Li RongQing, netdev, linux-kernel
In-Reply-To: <c522f2e0-1bef-8679-45f7-707854918820@hartkopp.net>
Hi Oliver,
> On 26.02.2019 12:04, Oliver Hartkopp wrote:
>
> Hi Julius,
>
(..)
>
> The reason for IFF_ECHO was, that the data frame which is sent onto the wire (by one application) is not visible to all the other applications on the same (local) host. Therefore a successful transmission on the wire triggers the 'echo' of the sent content into the local host.
>
Thank you for the explanation - I can adjust the comment, if you like to.
> So what are you getting back after you enabled IFF_ECHO on your mac80211 device?
>
> Is it just a 'status' about a sent packet, or is it the packet ('full content') itself?
We are actually getting back the full content of the packet. So it matches the behaviour of the 'echo' in CAN.
>
> Regards,
> Oliver
>
Many thanks,
Julius
^ 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