* Re: [RFC PATCH v3] rtl8xxxu: Improve TX performance of RTL8723BU on rtl8xxxu driver
From: Chris Chiu @ 2019-05-30 4:58 UTC (permalink / raw)
To: Larry Finger
Cc: Jes Sorensen, Kalle Valo, David Miller, linux-wireless, netdev,
Linux Kernel, Linux Upstreaming Team
In-Reply-To: <5f5e262d-aadb-cca0-8576-879735366a73@lwfinger.net>
On Thu, May 30, 2019 at 2:22 AM Larry Finger <Larry.Finger@lwfinger.net> wrote:
>
> On 5/29/19 12:03 AM, Chris Chiu wrote:
> > We have 3 laptops which connect the wifi by the same RTL8723BU.
> > The PCI VID/PID of the wifi chip is 10EC:B720 which is supported.
> > They have the same problem with the in-kernel rtl8xxxu driver, the
> > iperf (as a client to an ethernet-connected server) gets ~1Mbps.
> > Nevertheless, the signal strength is reported as around -40dBm,
> > which is quite good. From the wireshark capture, the tx rate for each
> > data and qos data packet is only 1Mbps. Compare to the driver from
> > https://github.com/lwfinger/rtl8723bu, the same iperf test gets ~12
> > Mbps or more. The signal strength is reported similarly around
> > -40dBm. That's why we want to improve.
>
> The driver at GitHub was written by Realtek. I only published it in a prominent
> location, and fix it for kernel API changes. I would say "the Realtek driver at
> https://...", and every mention of "Larry's driver" should say "Realtek's
> driver". That attribution is more correct.
Thanks. I'll modify this in next revision.
> >
> > After reading the source code of the rtl8xxxu driver and Larry's, the
> > major difference is that Larry's driver has a watchdog which will keep
> > monitoring the signal quality and updating the rate mask just like the
> > rtl8xxxu_gen2_update_rate_mask() does if signal quality changes.
> > And this kind of watchdog also exists in rtlwifi driver of some specific
> > chips, ex rtl8192ee, rtl8188ee, rtl8723ae, rtl8821ae...etc. They have
> > the same member function named dm_watchdog and will invoke the
> > corresponding dm_refresh_rate_adaptive_mask to adjust the tx rate
> > mask.
> >
> > With this commit, the tx rate of each data and qos data packet will
> > be 39Mbps (MCS4) with the 0xF00000 as the tx rate mask. The 20th bit
> > to 23th bit means MCS4 to MCS7. It means that the firmware still picks
> > the lowest rate from the rate mask and explains why the tx rate of
> > data and qos data is always lowest 1Mbps because the default rate mask
> > passed is always 0xFFFFFFF ranges from the basic CCK rate, OFDM rate,
> > and MCS rate. However, with Larry's driver, the tx rate observed from
> > wireshark under the same condition is almost 65Mbps or 72Mbps.
> >
> > I believe the firmware of RTL8723BU may need fix. And I think we
> > can still bring in the dm_watchdog as rtlwifi to improve from the
> > driver side. Please leave precious comments for my commits and
> > suggest what I can do better. Or suggest if there's any better idea
> > to fix this. Thanks.
> >
> > Signed-off-by: Chris Chiu <chiu@endlessm.com>
>
> I have not tested this patch, but I plan to soon.
>
> Larry
>
>
^ permalink raw reply
* Re: [PATCH wireless-drivers] mt76: usb: fix buffer allocation for scatter-gather capable devices
From: Kalle Valo @ 2019-05-30 4:56 UTC (permalink / raw)
To: Lorenzo Bianconi; +Cc: linux-wireless, nbd, lorenzo.bianconi, sgruszka
In-Reply-To: <f1f5b9f564e374174a9a2bbae29f4b72fd4c6ddd.1559163190.git.lorenzo@kernel.org>
Lorenzo Bianconi <lorenzo@kernel.org> writes:
> Partially revert commit f8f527b16db5 ("mt76: usb: use EP max packet
> aligned buffer sizes for rx") since it breaks A-MSDU support.
> When A-MSDU is enable the device can receive frames up to
> q->buf_size but they will be discarded in mt76u_process_rx_entry
> since there is no enough room for skb_shared_info.
> Fix it by introducing q->data_size and take info account
> skb_shared_info size in q->buf_size
> Moreover increase buffer size even for legacy mode (scatter-gather not
> available)
>
> Fixes: f8f527b16db5 ("mt76: usb: use EP max packet aligned buffer sizes for rx")
> Signed-off-by: Lorenzo Bianconi <lorenzo@kernel.org>
Felix, can I take this directly to wireless-drivers?
--
Kalle Valo
^ permalink raw reply
* Re: [RFC PATCH v3] rtl8xxxu: Improve TX performance of RTL8723BU on rtl8xxxu driver
From: Chris Chiu @ 2019-05-30 4:51 UTC (permalink / raw)
To: Daniel Drake
Cc: Jes Sorensen, Kalle Valo, David Miller, linux-wireless, netdev,
Linux Kernel, Linux Upstreaming Team
In-Reply-To: <CAD8Lp46on32VgWtCe7WsGHXp3Jk16qTh6saf0Vj0Y4Ry5z1n7g@mail.gmail.com>
On Thu, May 30, 2019 at 2:12 AM Daniel Drake <drake@endlessm.com> wrote:
>
> Hi Chris,
>
> On Tue, May 28, 2019 at 11:03 PM Chris Chiu <chiu@endlessm.com> wrote:
> > + /*
> > + * Single virtual interface permitted since the driver supports STATION
> > + * mode only.
>
> I think you can be a bit more explicit by saying e.g.:
>
> Only one virtual interface permitted because only STA mode is
> supported and no iface_combinations are provided.
>
> > diff --git a/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_core.c b/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_core.c
> > index 039e5ca9d2e4..2d612c2df5b2 100644
> > --- a/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_core.c
> > +++ b/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_core.c
> > @@ -4345,7 +4345,7 @@ void rtl8xxxu_gen2_update_rate_mask(struct rtl8xxxu_priv *priv,
> > h2c.b_macid_cfg.ramask3 = (ramask >> 24) & 0xff;
> >
> > h2c.ramask.arg = 0x80;
> > - h2c.b_macid_cfg.data1 = 0;
> > + h2c.b_macid_cfg.data1 = priv->ratr_index;
>
> I think ratr_index can be moved to be a function parameter of the
> update_rate_mask function. It looks like all callsites already know
> which value they want to set. Then you don't have to store it in the
> priv structure.
>
You mean moving the ratr_index to be the 4th function parameter of
update_rate_mask which has 2 candidates rtl8xxxu_update_rate_mask
and rtl8xxxu_gen2_update_rate_mask? I was planning to keep the
rtl8xxxu_update_rate_mask the same because old chips seems don't
need the rate index when invoking H2C command to change rate mask.
And rate index is not a common phrase/term for rate adaptive. Theoretically
we just need packet error rate, sgi and other factors to determine the rate
mask. This rate index seems to be only specific to newer Realtek drivers
or firmware for rate adaptive algorithm. I'd like to keep this for gen2 but
I admit it's ugly to put it in the priv structure. Any suggestion is
appreciated.
Thanks
> > @@ -5471,6 +5509,10 @@ static int rtl8xxxu_add_interface(struct ieee80211_hw *hw,
> >
> > switch (vif->type) {
> > case NL80211_IFTYPE_STATION:
> > + if (!priv->vif)
> > + priv->vif = vif;
> > + else
> > + return -EOPNOTSUPP;
> > rtl8xxxu_stop_tx_beacon(priv);
>
> rtl8xxxu_remove_interface should also set priv->vif back to NULL.
>
> > @@ -6183,6 +6259,8 @@ static void rtl8xxxu_disconnect(struct usb_interface *interface)
> > mutex_destroy(&priv->usb_buf_mutex);
> > mutex_destroy(&priv->h2c_mutex);
> >
> > + cancel_delayed_work_sync(&priv->ra_watchdog);
>
> Given that the work was started in rtl8xxxu_start, I think it should
> be cancelled in rtl8xxxu_stop() instead.
>
> Daniel
^ permalink raw reply
* Re: [PATCH 01/11] rtw88: resolve order of tx power setting routines
From: Larry Finger @ 2019-05-30 2:57 UTC (permalink / raw)
To: Tony Chuang, kvalo@codeaurora.org; +Cc: linux-wireless@vger.kernel.org
In-Reply-To: <F7CD281DE3E379468C6D07993EA72F84D17FABBF@RTITMBSVM04.realtek.com.tw>
On 5/29/19 9:29 PM, Tony Chuang wrote:
>
>
>> -----Original Message-----
>> From: Larry Finger [mailto:larry.finger@gmail.com] On Behalf Of Larry Finger
>> Sent: Wednesday, May 29, 2019 11:17 PM
>> To: Tony Chuang; kvalo@codeaurora.org
>> Cc: linux-wireless@vger.kernel.org
>> Subject: Re: [PATCH 01/11] rtw88: resolve order of tx power setting routines
>>
>> On 5/29/19 2:54 AM, yhchuang@realtek.com wrote:
>>> From: Yan-Hsuan Chuang <yhchuang@realtek.com>
>>>
>>> Some functions that should be static are unnecessarily exposed, remove
>>> their declaration in header file phy.h.
>>>
>>> After resolving their declaration order, they can be declared as static.
>>> So this commit changes nothing except the order and marking them static.
>>>
>>> Signed-off-by: Yan-Hsuan Chuang <yhchuang@realtek.com>
>>
>> This patch does not apply. Using quilt to see what is wrong, there are 6
>> changes
>> that have already been applied.
>>
>> Larry
>>
>
>
> These patches are based on
>
> https://kernel.googlesource.com/pub/scm/linux/kernel/git/kvalo/wireless-drivers
> branch master
>
> commit 6aca09771db4277a78853d6ac680d8d5f0d915e3
> Author: YueHaibing <yuehaibing@huawei.com>
> Date: Sat May 4 18:32:24 2019 +0800
>
> rtw88: Make some symbols static
>
>
> It should apply, did I miss something?
I was trying to apply the patch to wireless-drivers-next. That may be my
problem. The other 10 applied OK.
Larry
^ permalink raw reply
* RE: [PATCH] rtw88: Remove set but not used variable 'ip_sel' and 'orig'
From: Tony Chuang @ 2019-05-30 2:32 UTC (permalink / raw)
To: YueHaibing, kvalo@codeaurora.org, davem@davemloft.net
Cc: linux-kernel@vger.kernel.org, netdev@vger.kernel.org,
linux-wireless@vger.kernel.org
In-Reply-To: <20190529145740.22804-1-yuehaibing@huawei.com>
> -----Original Message-----
> From: YueHaibing [mailto:yuehaibing@huawei.com]
> Sent: Wednesday, May 29, 2019 10:58 PM
> To: Tony Chuang; kvalo@codeaurora.org; davem@davemloft.net
> Cc: linux-kernel@vger.kernel.org; netdev@vger.kernel.org;
> linux-wireless@vger.kernel.org; YueHaibing
> Subject: [PATCH] rtw88: Remove set but not used variable 'ip_sel' and 'orig'
>
> Fixes gcc '-Wunused-but-set-variable' warnings:
>
> drivers/net/wireless/realtek/rtw88/pci.c: In function rtw_pci_phy_cfg:
> drivers/net/wireless/realtek/rtw88/pci.c:978:6: warning: variable ip_sel set
> but not used [-Wunused-but-set-variable]
> drivers/net/wireless/realtek/rtw88/phy.c: In function
> phy_tx_power_limit_config:
> drivers/net/wireless/realtek/rtw88/phy.c:1607:11: warning: variable orig set
> but not used [-Wunused-but-set-variable]
>
> They are never used, so can be removed.
>
> Signed-off-by: YueHaibing <yuehaibing@huawei.com>
> ---
> drivers/net/wireless/realtek/rtw88/pci.c | 3 ---
> drivers/net/wireless/realtek/rtw88/phy.c | 3 +--
> 2 files changed, 1 insertion(+), 5 deletions(-)
>
> diff --git a/drivers/net/wireless/realtek/rtw88/pci.c
> b/drivers/net/wireless/realtek/rtw88/pci.c
> index 353871c27779..8329f4e447b7 100644
> --- a/drivers/net/wireless/realtek/rtw88/pci.c
> +++ b/drivers/net/wireless/realtek/rtw88/pci.c
> @@ -977,7 +977,6 @@ static void rtw_pci_phy_cfg(struct rtw_dev *rtwdev)
> u16 cut;
> u16 value;
> u16 offset;
> - u16 ip_sel;
> int i;
>
> cut = BIT(0) << rtwdev->hal.cut_version;
> @@ -990,7 +989,6 @@ static void rtw_pci_phy_cfg(struct rtw_dev *rtwdev)
> break;
> offset = para->offset;
> value = para->value;
> - ip_sel = para->ip_sel;
> if (para->ip_sel == RTW_IP_SEL_PHY)
> rtw_mdio_write(rtwdev, offset, value, true);
> else
> @@ -1005,7 +1003,6 @@ static void rtw_pci_phy_cfg(struct rtw_dev
> *rtwdev)
> break;
> offset = para->offset;
> value = para->value;
> - ip_sel = para->ip_sel;
> if (para->ip_sel == RTW_IP_SEL_PHY)
> rtw_mdio_write(rtwdev, offset, value, false);
> else
> diff --git a/drivers/net/wireless/realtek/rtw88/phy.c
> b/drivers/net/wireless/realtek/rtw88/phy.c
> index 404d89432c96..c3e75ffe27b5 100644
> --- a/drivers/net/wireless/realtek/rtw88/phy.c
> +++ b/drivers/net/wireless/realtek/rtw88/phy.c
> @@ -1604,12 +1604,11 @@ void rtw_phy_tx_power_by_rate_config(struct
> rtw_hal *hal)
> static void
> phy_tx_power_limit_config(struct rtw_hal *hal, u8 regd, u8 bw, u8 rs)
> {
> - s8 base, orig;
> + s8 base;
> u8 ch;
>
> for (ch = 0; ch < RTW_MAX_CHANNEL_NUM_2G; ch++) {
> base = hal->tx_pwr_by_rate_base_2g[0][rs];
> - orig = hal->tx_pwr_limit_2g[regd][bw][rs][ch];
> hal->tx_pwr_limit_2g[regd][bw][rs][ch] -= base;
> }
>
Hi Haibing
I have submitted a patch fix the unused variable in phy.c
Which is,
> drivers/net/wireless/realtek/rtw88/phy.c: In function
> phy_tx_power_limit_config:
> drivers/net/wireless/realtek/rtw88/phy.c:1607:11: warning: variable orig set
> but not used [-Wunused-but-set-variable]
Can you drop the changes in phy.c and remain the changes in pci.c?
Thanks.
Yan-Hsuan
^ permalink raw reply
* RE: [PATCH 01/11] rtw88: resolve order of tx power setting routines
From: Tony Chuang @ 2019-05-30 2:29 UTC (permalink / raw)
To: Larry Finger, kvalo@codeaurora.org; +Cc: linux-wireless@vger.kernel.org
In-Reply-To: <f5bd9ab0-c32c-dcc6-9451-09e6b7f50a96@lwfinger.net>
> -----Original Message-----
> From: Larry Finger [mailto:larry.finger@gmail.com] On Behalf Of Larry Finger
> Sent: Wednesday, May 29, 2019 11:17 PM
> To: Tony Chuang; kvalo@codeaurora.org
> Cc: linux-wireless@vger.kernel.org
> Subject: Re: [PATCH 01/11] rtw88: resolve order of tx power setting routines
>
> On 5/29/19 2:54 AM, yhchuang@realtek.com wrote:
> > From: Yan-Hsuan Chuang <yhchuang@realtek.com>
> >
> > Some functions that should be static are unnecessarily exposed, remove
> > their declaration in header file phy.h.
> >
> > After resolving their declaration order, they can be declared as static.
> > So this commit changes nothing except the order and marking them static.
> >
> > Signed-off-by: Yan-Hsuan Chuang <yhchuang@realtek.com>
>
> This patch does not apply. Using quilt to see what is wrong, there are 6
> changes
> that have already been applied.
>
> Larry
>
These patches are based on
https://kernel.googlesource.com/pub/scm/linux/kernel/git/kvalo/wireless-drivers
branch master
commit 6aca09771db4277a78853d6ac680d8d5f0d915e3
Author: YueHaibing <yuehaibing@huawei.com>
Date: Sat May 4 18:32:24 2019 +0800
rtw88: Make some symbols static
It should apply, did I miss something?
Yan-Hsuan
^ permalink raw reply
* [PATCH] ath10k: fix PCIE device wake up failed
From: Miaoqing Pan @ 2019-05-30 1:49 UTC (permalink / raw)
To: ath10k; +Cc: linux-wireless, Miaoqing Pan
Observed PCIE device wake up failed after ~120 iterations of
soft-reboot test. The error message is
"ath10k_pci 0000:01:00.0: failed to wake up device : -110"
The call trace as below:
ath10k_pci_probe -> ath10k_pci_force_wake -> ath10k_pci_wake_wait ->
ath10k_pci_is_awake
Once trigger the device to wake up, we will continuously check the RTC
state until it returns RTC_STATE_V_ON or timeout.
But for QCA99x0 chips, we use wrong value for RTC_STATE_V_ON.
Occasionally, we get 0x7 on the fist read, we thought as a failure
case, but actually is the right value, also verified with the spec.
So fix the issue by changing RTC_STATE_V_ON from 0x5 to 0x7, passed
~2000 iterations.
Tested HW: QCA9984
Signed-off-by: Miaoqing Pan <miaoqing@codeaurora.org>
---
drivers/net/wireless/ath/ath10k/hw.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/net/wireless/ath/ath10k/hw.c b/drivers/net/wireless/ath/ath10k/hw.c
index ad082b7..b242085 100644
--- a/drivers/net/wireless/ath/ath10k/hw.c
+++ b/drivers/net/wireless/ath/ath10k/hw.c
@@ -158,7 +158,7 @@
};
const struct ath10k_hw_values qca99x0_values = {
- .rtc_state_val_on = 5,
+ .rtc_state_val_on = 7,
.ce_count = 12,
.msi_assign_ce_max = 12,
.num_target_ce_config_wlan = 10,
--
1.9.1
^ permalink raw reply related
* [PATCH wireless-drivers] mt76: usb: fix buffer allocation for scatter-gather capable devices
From: Lorenzo Bianconi @ 2019-05-29 21:01 UTC (permalink / raw)
To: kvalo; +Cc: linux-wireless, nbd, lorenzo.bianconi, sgruszka
Partially revert commit f8f527b16db5 ("mt76: usb: use EP max packet
aligned buffer sizes for rx") since it breaks A-MSDU support.
When A-MSDU is enable the device can receive frames up to
q->buf_size but they will be discarded in mt76u_process_rx_entry
since there is no enough room for skb_shared_info.
Fix it by introducing q->data_size and take info account
skb_shared_info size in q->buf_size
Moreover increase buffer size even for legacy mode (scatter-gather not
available)
Fixes: f8f527b16db5 ("mt76: usb: use EP max packet aligned buffer sizes for rx")
Signed-off-by: Lorenzo Bianconi <lorenzo@kernel.org>
---
drivers/net/wireless/mediatek/mt76/mt76.h | 4 ++++
drivers/net/wireless/mediatek/mt76/usb.c | 26 ++++++++++++-----------
2 files changed, 18 insertions(+), 12 deletions(-)
diff --git a/drivers/net/wireless/mediatek/mt76/mt76.h b/drivers/net/wireless/mediatek/mt76/mt76.h
index 8ecbf81a906f..f118919ca5ff 100644
--- a/drivers/net/wireless/mediatek/mt76/mt76.h
+++ b/drivers/net/wireless/mediatek/mt76/mt76.h
@@ -31,6 +31,9 @@
#define MT_MCU_RING_SIZE 32
#define MT_RX_BUF_SIZE 2048
+#define MT_BUF_WITH_OVERHEAD(x) \
+ ((x) + SKB_DATA_ALIGN(sizeof(struct skb_shared_info)))
+
struct mt76_dev;
struct mt76_wcid;
@@ -124,6 +127,7 @@ struct mt76_queue {
u16 tail;
int ndesc;
int queued;
+ int data_size;
int buf_size;
bool stopped;
diff --git a/drivers/net/wireless/mediatek/mt76/usb.c b/drivers/net/wireless/mediatek/mt76/usb.c
index bbaa1365bbda..9e328e4532b3 100644
--- a/drivers/net/wireless/mediatek/mt76/usb.c
+++ b/drivers/net/wireless/mediatek/mt76/usb.c
@@ -299,7 +299,7 @@ mt76u_fill_rx_sg(struct mt76_dev *dev, struct mt76_queue *q, struct urb *urb,
page = virt_to_head_page(data);
offset = data - page_address(page);
- sg_set_page(&urb->sg[i], page, q->buf_size, offset);
+ sg_set_page(&urb->sg[i], page, q->data_size, offset);
}
if (i < nsgs) {
@@ -311,7 +311,7 @@ mt76u_fill_rx_sg(struct mt76_dev *dev, struct mt76_queue *q, struct urb *urb,
}
urb->num_sgs = max_t(int, i, urb->num_sgs);
- urb->transfer_buffer_length = urb->num_sgs * q->buf_size,
+ urb->transfer_buffer_length = urb->num_sgs * q->data_size;
sg_init_marker(urb->sg, urb->num_sgs);
return i ? : -ENOMEM;
@@ -322,14 +322,13 @@ mt76u_refill_rx(struct mt76_dev *dev, struct urb *urb, int nsgs, gfp_t gfp)
{
struct mt76_queue *q = &dev->q_rx[MT_RXQ_MAIN];
- if (dev->usb.sg_en) {
+ if (dev->usb.sg_en)
return mt76u_fill_rx_sg(dev, q, urb, nsgs, gfp);
- } else {
- urb->transfer_buffer_length = q->buf_size;
- urb->transfer_buffer = page_frag_alloc(&q->rx_page,
- q->buf_size, gfp);
- return urb->transfer_buffer ? 0 : -ENOMEM;
- }
+
+ urb->transfer_buffer_length = q->data_size;
+ urb->transfer_buffer = page_frag_alloc(&q->rx_page, q->buf_size, gfp);
+
+ return urb->transfer_buffer ? 0 : -ENOMEM;
}
static int
@@ -446,8 +445,9 @@ mt76u_process_rx_entry(struct mt76_dev *dev, struct urb *urb)
return 0;
data_len = min_t(int, len, data_len - MT_DMA_HDR_LEN);
- if (MT_DMA_HDR_LEN + data_len > SKB_WITH_OVERHEAD(q->buf_size)) {
- dev_err_ratelimited(dev->dev, "rx data too big %d\n", data_len);
+ if (MT_DMA_HDR_LEN + data_len > q->data_size) {
+ dev_err_ratelimited(dev->dev, "rx data too big %d\n",
+ data_len);
return 0;
}
@@ -577,8 +577,10 @@ static int mt76u_alloc_rx(struct mt76_dev *dev)
if (!q->entry)
return -ENOMEM;
- q->buf_size = dev->usb.sg_en ? MT_RX_BUF_SIZE : PAGE_SIZE;
+ q->data_size = dev->usb.sg_en ? MT_RX_BUF_SIZE : PAGE_SIZE;
+ q->buf_size = MT_BUF_WITH_OVERHEAD(q->data_size);
q->ndesc = MT_NUM_RX_ENTRIES;
+
for (i = 0; i < q->ndesc; i++) {
err = mt76u_rx_urb_alloc(dev, &q->entry[i]);
if (err < 0)
--
2.21.0
^ permalink raw reply related
* Re: cellular modem APIs - take 2
From: Denis Kenzior @ 2019-05-29 20:44 UTC (permalink / raw)
To: Johannes Berg, Marcel Holtmann
Cc: netdev, linux-wireless, Subash Abhinov Kasiviswanathan,
Dan Williams, Sean Tranchetti, Daniele Palmas, Aleksander Morgado,
Bjørn Mork
In-Reply-To: <acf18b398fd63f2dfece5981ebd5057141529e6a.camel@sipsolutions.net>
Hi Johannes,
>
> After all, I'm not really proposing that we put oFono or something like
> it into the kernel - far from it! I'm only proposing that we kill the
> many various ways of creating and managing the necessary netdevs (VLANs,
> sysfs, rmnet, ...) from a piece of software like oFono (or libmbim or
> whatever else).
I do like the concept of unifying this if possible. The question is, is
it actually possible :) I think Dan covered most of the aspects of what
userspace has to deal with already. But the basic issue is that there's
a heck of a lot of different ways of doing it.
>
> Apart from CAIF and phonet, oFono doesn't even try to do this though,
> afaict, so I guess it relies on the default netdev created, or some out-
> of-band configuration is still needed?
Actually it can. We can drive modems which provide only a single serial
port and run multiplexing over that. So we fully control the number of
control channels created, the number of netdevs created and even
create/destroy them on as needed basis. And these netdevs can be PPP
encapsulated or pure IP or whatever else.
Regards,
-Denis
^ permalink raw reply
* Re: cellular modem APIs - take 2
From: Denis Kenzior @ 2019-05-29 20:35 UTC (permalink / raw)
To: Johannes Berg, Dan Williams, netdev, linux-wireless
Cc: Subash Abhinov Kasiviswanathan, Sean Tranchetti, Daniele Palmas,
Aleksander Morgado, Bjørn Mork
In-Reply-To: <58bc88b7eda912133ad0fc4718ac917adc8fa63b.camel@sipsolutions.net>
Hi Johannes,
>
> It just seemed that people do want to have a netdev (if only to see that
> their driver loaded and use ethtool to dump the firmware version), and
> then you can reassign it to some actual context later?
I can see that this is useful for developers, but really
counterproductive in production. You have a bunch of services (systemd,
NM, ConnMan, dhcpcd, etc, etc, etc) all observing the newly created
devices and trying to 'own' them. Which makes no freaking sense to do
until those devices are really usable. Just because it is a netdev,
doesn't mean it is ethernet or behaves like it.
That also leads to expectations from users that want stuff like
udev-consistent-naming to work, even though udev has no idea wtf a given
device is, when it is ready or not ready, etc. And the flip side, there
may be systems that do not use systemd/udevd, so the daemons responsible
for management of such devices cannot sanely assume anything. It is
just pure chaos.
And then there's hotplug/unplug to worry about ;)
So I would like to reiterate Marcel's point: creating unmanaged devices
should not be the default behavior.
>
> It doesn't really matter much. If you have a control channel and higher-
> level abstraction (wwan device) then having the netdev is probably more
> of a nuisance and mostly irrelevant, just might be useful for legacy
> reasons.
Which we should be trying to eradicate, not encourage ;)
>> Should you really need to account for these specially, or would some
>> kind of sysfs linkage like SET_NETDEV_DEV() be more appropriate?
>>
>> Really all you want to do is (a) identify which WWAN device a given
>> control/data channel is for and (b) perhaps tag different control/data
>> channels with attributes like primary/secondary/gps/sim/etc often
>> through USB attributes or hardcoded data on SoCs.
Userspace can also choose to do its own multiplexing, so you can't even
really assume the above is what you 'want'.
Regards,
-Denis
^ permalink raw reply
* Re: cellular modem APIs - take 2
From: Johannes Berg @ 2019-05-29 20:16 UTC (permalink / raw)
To: Dan Williams, netdev, linux-wireless
Cc: Subash Abhinov Kasiviswanathan, Sean Tranchetti, Daniele Palmas,
Aleksander Morgado, Bjørn Mork
In-Reply-To: <cb2ef612be9e347a7cbceb26831f8d5ebea4eacf.camel@redhat.com>
Hi Dan,
> > Quite possibly there might be some additional (vendor-dependent?)
> > configuration for when such netdevs are created, but we need to
> > figure out if that really needs to be at creation time, or just
> > ethtool later or something like that. I guess it depends on how
> > generic it needs to be.
>
> I'm pretty sure it would have to be async via netlink or ethtool or
> whatever later, because the control plane (ModemManager,
> libmbim/libqmi, oFono, etc) is what sets up the PDP/EPS context and
> thus the data channel. A netdev (or a queue on that netdev) would be a
> representation of that data channel, but that's not something the
> kernel knows beforehand.
Right.
It just seemed that people do want to have a netdev (if only to see that
their driver loaded and use ethtool to dump the firmware version), and
then you can reassign it to some actual context later?
It doesn't really matter much. If you have a control channel and higher-
level abstraction (wwan device) then having the netdev is probably more
of a nuisance and mostly irrelevant, just might be useful for legacy
reasons.
> > 3) Separately, we need to have an ability to create "generalized control
> > channels". I'm thinking there would be a general command "create
> > control channel" with a given type (e.g. ATCMD, RPC, MBIM, GNSS) plus
> > a list of vendor-specific channels (e.g. for tracing).
> >
> > I'm unsure where this channel should really go - somehow it seems to
> > me that for many (most?) of these registering them as a serial line
> > would be most appropriate, but some, especially vendor-defined
> > channels like tracing, would probably better use a transport that's
> > higher bandwidth than, e.g. netdevs.
>
> There's only a couple protocols that are run over serial transport,
> namely AT, DM/DIAG, and Sierra's CnS.
Right.
> Most of the rest like QMI and MBIM are packet-based protocols that can
> use different transports. QMI for example can use CDC-WDM for USB
> devices but on SoCs will use Qualcomm's SMD I believe.
Right, though transport and protocol are sort of different issues.
> Should you really need to account for these specially, or would some
> kind of sysfs linkage like SET_NETDEV_DEV() be more appropriate?
>
> Really all you want to do is (a) identify which WWAN device a given
> control/data channel is for and (b) perhaps tag different control/data
> channels with attributes like primary/secondary/gps/sim/etc often
> through USB attributes or hardcoded data on SoCs.
Ah, that brings up something I completely forgot.
I was thinking only of the case that the control channel(s) to the
device is/are all managed by the *kernel* driver, i.e. you'd have some
device-specific driver that has an interface into userspace to talk to
the modem's control channel (and that we could abstract).
However, yes, that's not true - many will be like USB where the control
channel is driven by a generic kernel driver (e.g. maybe usb-serial) or
no kernel driver at all, and then this linkage is probably the right
approach. Need to think about it.
OTOH, there will be device-specific ways to add more control channels
(say for debug/trace purposes etc.) and those would not have a "natural"
interface to userspace like control channels with generic/no drivers.
> > One way I thought of doing this was to create an abstraction in the
> > wwan framework that lets the driver use SKBs anyway (i.e. TX and RX
> > on these channels using SKBs) and then translate them to some channel
> > in the framework - that way, we can even select at runtime if we want
> > a netdev (not really plugged into the network stack, ARPHDR_VOID?) or
> > some other kind of transport. Building that would allow us to add
> > transport types in the future too.
>
> I'm not quite sure what you mean here, can you explain?
I was just thinking of the mechanics of doing this in the driver (while,
like I said above, completely forgetting about the non-monolithic driver
case). It's not really that important.
> > I guess such a channel should also be created by default, if it's
> > not already created by the driver in some out-of-band way anyway (and
> > most likely it shouldn't be, but I guess drivers might have some
> > entirely different communication channels for AT CMDs?)
>
> For existing devices we're not talking about monolithic drivers here
> (excepting 'hso') which I guess is part of the issue.
Right, and doesn't help I forgot about this ;-)
> A given device
> might be driven by 2 or 3 different kernel drivers (usb-serial derived,
> netdev, cdc-wdm) and they all expose different channels and none of
> them coordinate. You have to do sysfs matching up the family tree to
> find out they are associated with each other. If the kernel can take
> over some of that responsibility great.
Right. I guess it's hard for the kernel to take responsibility unless we
teach all the components (usb-serial, ...) that certain devices are
modems and should get some (additional?) registration?
> But the diversity is large. Any given TTY representing an AT channel
> could be from USB drivers (usb-serial, cdc-wdm, vendor-specific driver
> like sierra/hso, option) or PCI (nozomi) or platform stuff (Qualcomm
> SoC ones). You can also do AT-over-QMI if you want.
Right. The linkage here is the interesting part - for platform stuff
that might be easier (devicetree?) but not sure how we could teach that
to e.g. usb-serial, and nozomi is interesting because ... where is the
data plane even?
> I think it's worth discussing how this could be better unified but
> maybe small steps to get there are more feasible.
Fair point.
> > 4) There was a question about something like pure IP channels that
> > belong to another PDN and apparently now separate netdevs might be
> > used, but it seems to me that could just be a queue reserved on the
> > regular netdevs and then when you say ("enable video streaming on
> > wwan1 interface") that can do some magic to classify the video
> > packets (DSCP?) to another hardware queue for better QoS.
>
> Most stuff is pure IP these days (both for QMI/rmnet and MBIM at least)
> and having ethernet encapsulation is kinda pointless.
Yeah, true, not really sure why I was distinguishing this in these
terms. I think the use case really was just giving some packets higher
priority, putting them into a different *hardware* queue so the device
can see them even if the "normal" hardware queue is completely
backlogged.
Kinda a typical multi-queue TX use case.
> But anyway you'd
> need some mechanism to map that particular queue to a given channel/PDN
> created by the control channel.
Well, I was thinking that mechanism was creating the netdev, but then
*within* that some QoS seems to be desired.
> But classification is mostly done in the hardware/network because
> setting different QoS for a given PDP/EPS context is basically saying
> how much airtime the queue gets. No amount of kernel involvement is
> going to change what the network lets you transmit.
Right.
> And I honestly
> don't know how the firmware responds when its internal queues for a
> given context are full that would tell a kernel queue to stop sending
> more.
Well, at the very least it'll stop pulling packets from DMA/whatever, so
the kernel has to back off, right?
johannes
^ permalink raw reply
* Re: cellular modem APIs - take 2
From: Johannes Berg @ 2019-05-29 19:59 UTC (permalink / raw)
To: Marcel Holtmann
Cc: netdev, linux-wireless, Subash Abhinov Kasiviswanathan,
Dan Williams, Sean Tranchetti, Daniele Palmas, Aleksander Morgado,
Bjørn Mork
In-Reply-To: <662BBC5C-D0C7-4B2C-A001-D6F490D0F36F@holtmann.org>
Hi Marcel,
> Have you actually looked at Phonet or CAIF.
Phonet is just a specific protocol spoken by a specific modem (family)
for their control plane. Not all modems are going to be speaking this.
Same for CAIF, really. I don't really see all that much that's generic
(enough) here. It's unfortunate that in all this time nobody ever
bothered to even try though, and just invented all their own mechanisms
to precisely mirror the hardware and firmware they were working with.
Theoretically now, you could build a driver that still speaks CAIF or
phonet and then translates to a specific modem, but what would the point
be?
Now, I'm looking more at the data plan than the control plane, so I
guess you could argue I should've not just mentioned MBIM and AT
commands, but also something like Phonet/CAIF.
> And netdev by default seems like repeating the same mistake we have
> done with WiFi. Your default context in LTE cases is only available
> when actually registered to the LTE bearer. It is pretty much
> pointless to have a netdev if you are not registered to the network.
I partially agree with this.
Of course, for WiFi, that's just wrong since the control path is on the
netdev. Without a netdev, nothing works, and so not having one by
default just adds something pointless to the setup necessary to bring up
anything at all. It can be argued whether not allowing to remove it is
right, but that just detracts from the discussion at hand and there's
also a lot of history here.
I do understand (and mostly agree) that having a netdev by default
that's not connected to anything and has no functionality until you've
done some out-of-band (as far as the netdev is concerned) setup is
fairly much pointless, but OTOH having a netdev is something that people
seem to want for various reasons (discovery, ethtool, ...).
> You have to do a lot of initial modem setup before you ever get to the
> having your default context connected. Have a look at oFono and what
> it does to bring up the modem.
Sure.
> > 2) Clearly, one needs to be able to create PDN netdevs, with the PDN
> > given to the command. Here's another advantage: If these are created
> > on top of another abstraction, not another netdev, they can have
> > their own queues, multiqueue RX etc. much more easily.
[...]
> I think you need to provide actually a lot more details on how queue
> control inside Linux would be helpful and actually work in the first
> place. I don’t see how Linux will be ever in charge and not the modem
> do this all for you.
I think you misunderstand. I wasn't really talking about *queue control*
(packet dequeue etc.) although this is *also* something that could be
interesting, since the modem can only control packets that ever made it
to the hardware.
I was more thinking of what I actually wrote - "have their own queues,
multiqueue RX etc." - i.e. control the software layer of the queues in
the driver, rather than having all of that managed in some stacked
netdev like VLAN.
For example, with stacked netdevs like VLANs it gets difficult (and
awkward from a network stack perspective) to put frames for different
connections (stacked netdevs) into different hardware queues and manage
flow control correctly.
Similarly, if one connection has maybe multiple hardware queues (say for
a specific video stream) disentangling that when you have stacked
netdevs is much harder than when they're all separate.
(And, of course, there's little point in having the underlying netdev to
start with since it speaks a device-specific protocol the network stack
will never understand.)
> > 3) Separately, we need to have an ability to create "generalized control
> > channels". I'm thinking there would be a general command "create
> > control channel" with a given type (e.g. ATCMD, RPC, MBIM, GNSS) plus
> > a list of vendor-specific channels (e.g. for tracing).
[...]
> > I guess such a channel should also be created by default, if it's
> > not already created by the driver in some out-of-band way anyway (and
> > most likely it shouldn't be, but I guess drivers might have some
> > entirely different communication channels for AT CMDs?)
>
> I would just use sockets like Phonet and CAIF.
It was in fact one of the options I considered, but it seems to have
very little traction with any other userspace, and having a separate
socket family yet again also seems a bit pointless. I guess for devices
that do already speak Phonet or CAIF that would make sense. Possible to
some extent, but not really useful, would be to terminate the Phonet or
CAIF protocol inside the driver or stack, but then you end up having to
emulate some specific firmware behaviour ...
So ultimately it boils down to what protocols you want to support and
what kind of API they typically use. I guess I don't have enough hubris
to propose unifying the whole command set and how you talk to your
device ;-)
I suppose you could have a socket type for AT commands, but is that
really all that useful? Or a socket type that muxes the different
control channels you might have, which gets close to phonet/caif.
> Frankly I have a problem if this is designed from the hardware point
> of view. Unless you are familiar with how 3GPP works and what a
> telephony stack like oFono has to do, there is really no point in
> trying to create a WWAN subsystem.
>
> Anything defined needs to be defined in terms of 3GPP and not what
> current drivers have hacked together.
That objection makes no sense to me. 3GPP doesn't define how the data
plane is implemented in your device/OS. There's a data plane, it carries
IP packets, but how you get those to your applications?
After all, I'm not really proposing that we put oFono or something like
it into the kernel - far from it! I'm only proposing that we kill the
many various ways of creating and managing the necessary netdevs (VLANs,
sysfs, rmnet, ...) from a piece of software like oFono (or libmbim or
whatever else).
Apart from CAIF and phonet, oFono doesn't even try to do this though,
afaict, so I guess it relies on the default netdev created, or some out-
of-band configuration is still needed?
johannes
^ permalink raw reply
* Re: cellular modem APIs - take 2
From: Dan Williams @ 2019-05-29 19:59 UTC (permalink / raw)
To: Johannes Berg, netdev, linux-wireless
Cc: Subash Abhinov Kasiviswanathan, Sean Tranchetti, Daniele Palmas,
Aleksander Morgado, Bjørn Mork
In-Reply-To: <b59be15f1d0caa4eaa4476bbd8457afc44d57089.camel@sipsolutions.net>
On Mon, 2019-05-27 at 15:20 +0200, Johannes Berg wrote:
> Hi all,
>
> Sorry for the long delay in getting back to this. I'm meaning to write
> some code soon also for this, to illustrate better, but I figured I'd
> still get some thoughts out before I do that.
>
> After more discussion (@Intel) and the previous thread(s), I've pretty
> much come to the conclusion that we should have a small subsystem for
> WWAN, rather than fudging everything like we previously did.
>
> We can debate whether or not that should use 'real' netlink or generic
> netlink - personally I know the latter better and I think it has some
> real advantages like easier message parsing (it's automatic more or
> less) including strict checking and automatic policy introspection (I
> recently wrote the code for this and it's plugged into generic netlink
> family, for other netlink families it needs more hand-written code). But
> I could possibly be convinced of doing something else, and/or perhaps
> building more infrastructure for 'real' netlink to realize those
> benefits there as well.
>
>
> In terms of what I APIs are needed, the kernel-driver side and userspace
> side go pretty much hand in hand (the wwan subsystem just providing the
> glue), so what I say below is pretty much both a method/function call
> (kernel internal API) or a netlink message (userspace API).
>
> 1) I think a generic abstraction of WWAN device that is not a netdev
> is needed. Yes, on the one hand it's quite nice to be able to work on
> top of a given netdev, but it's also limiting because it requires the
> data flow to go through there, and packets that are tagged in some
> way be exchanged there.
> For VLANs this can be out-of-band (in a sense) with hw-accel, but for
> rmnet-style it's in-band, and that limits what we can do.
>
> Now, of course this doesn't mean there shouldn't be a netdev created
> by default in most cases, but it gives us a way to do additional
> things that we cannot do with *just* a netdev.
>
> From a driver POV though, it would register a new "wwan_device", and
> then get some generic callback to create a netdev on top, maybe by
> default from the subsystem or from the user.
>
> 2) Clearly, one needs to be able to create PDN netdevs, with the PDN
> given to the command. Here's another advantage: If these are created
> on top of another abstraction, not another netdev, they can have
> their own queues, multiqueue RX etc. much more easily.
>
> Also, things like the "if I have just a single channel, drop the mux
> headers" can then be entirely done in the driver, and the default
> netdev no longer has the possibility of muxed and IP frames on the
> same datapath.
>
> This also enables more things like handling checksum offload directly
> in the driver, which doesn't behave so well with VLANs I think.
>
> All of that will just be easier for 5G too, I believe, with
> acceleration being handled per PDN, multi-queue working without
> ndo_select_queue, etc.
>
> Quite possibly there might be some additional (vendor-dependent?)
> configuration for when such netdevs are created, but we need to
> figure out if that really needs to be at creation time, or just
> ethtool later or something like that. I guess it depends on how
> generic it needs to be.
I'm pretty sure it would have to be async via netlink or ethtool or
whatever later, because the control plane (ModemManager,
libmbim/libqmi, oFono, etc) is what sets up the PDP/EPS context and
thus the data channel. A netdev (or a queue on that netdev) would be a
representation of that data channel, but that's not something the
kernel knows beforehand.
> 3) Separately, we need to have an ability to create "generalized control
> channels". I'm thinking there would be a general command "create
> control channel" with a given type (e.g. ATCMD, RPC, MBIM, GNSS) plus
> a list of vendor-specific channels (e.g. for tracing).
>
> I'm unsure where this channel should really go - somehow it seems to
> me that for many (most?) of these registering them as a serial line
> would be most appropriate, but some, especially vendor-defined
> channels like tracing, would probably better use a transport that's
> higher bandwidth than, e.g. netdevs.
There's only a couple protocols that are run over serial transport,
namely AT, DM/DIAG, and Sierra's CnS.
Most of the rest like QMI and MBIM are packet-based protocols that can
use different transports. QMI for example can use CDC-WDM for USB
devices but on SoCs will use Qualcomm's SMD I believe.
Should you really need to account for these specially, or would some
kind of sysfs linkage like SET_NETDEV_DEV() be more appropriate?
Really all you want to do is (a) identify which WWAN device a given
control/data channel is for and (b) perhaps tag different control/data
channels with attributes like primary/secondary/gps/sim/etc often
through USB attributes or hardcoded data on SoCs.
> One way I thought of doing this was to create an abstraction in the
> wwan framework that lets the driver use SKBs anyway (i.e. TX and RX
> on these channels using SKBs) and then translate them to some channel
> in the framework - that way, we can even select at runtime if we want
> a netdev (not really plugged into the network stack, ARPHDR_VOID?) or
> some other kind of transport. Building that would allow us to add
> transport types in the future too.
I'm not quite sure what you mean here, can you explain?
> I guess such a channel should also be created by default, if it's
> not already created by the driver in some out-of-band way anyway (and
> most likely it shouldn't be, but I guess drivers might have some
> entirely different communication channels for AT CMDs?)
For existing devices we're not talking about monolithic drivers here
(excepting 'hso') which I guess is part of the issue. A given device
might be driven by 2 or 3 different kernel drivers (usb-serial derived,
netdev, cdc-wdm) and they all expose different channels and none of
them coordinate. You have to do sysfs matching up the family tree to
find out they are associated with each other. If the kernel can take
over some of that responsibility great.
But the diversity is large. Any given TTY representing an AT channel
could be from USB drivers (usb-serial, cdc-wdm, vendor-specific driver
like sierra/hso, option) or PCI (nozomi) or platform stuff (Qualcomm
SoC ones). You can also do AT-over-QMI if you want.
I think it's worth discussing how this could be better unified but
maybe small steps to get there are more feasible.
> 4) There was a question about something like pure IP channels that
> belong to another PDN and apparently now separate netdevs might be
> used, but it seems to me that could just be a queue reserved on the
> regular netdevs and then when you say ("enable video streaming on
> wwan1 interface") that can do some magic to classify the video
> packets (DSCP?) to another hardware queue for better QoS.
Most stuff is pure IP these days (both for QMI/rmnet and MBIM at least)
and having ethernet encapsulation is kinda pointless. But anyway you'd
need some mechanism to map that particular queue to a given channel/PDN
created by the control channel.
But classification is mostly done in the hardware/network because
setting different QoS for a given PDP/EPS context is basically saying
how much airtime the queue gets. No amount of kernel involvement is
going to change what the network lets you transmit. And I honestly
don't know how the firmware responds when its internal queues for a
given context are full that would tell a kernel queue to stop sending
more.
Dan
>
>
> Anyway, if all of this doesn't seem completely outlandish I'll try to
> write some code to illustrate it (sooner, rather than later).
>
> Thanks,
> johannes
>
^ permalink raw reply
* Re: cellular modem APIs - take 2
From: Marcel Holtmann @ 2019-05-29 19:05 UTC (permalink / raw)
To: Johannes Berg
Cc: netdev, linux-wireless, Subash Abhinov Kasiviswanathan,
Dan Williams, Sean Tranchetti, Daniele Palmas, Aleksander Morgado,
Bjørn Mork
In-Reply-To: <b59be15f1d0caa4eaa4476bbd8457afc44d57089.camel@sipsolutions.net>
Hi Johannes,
> Sorry for the long delay in getting back to this. I'm meaning to write
> some code soon also for this, to illustrate better, but I figured I'd
> still get some thoughts out before I do that.
>
> After more discussion (@Intel) and the previous thread(s), I've pretty
> much come to the conclusion that we should have a small subsystem for
> WWAN, rather than fudging everything like we previously did.
>
> We can debate whether or not that should use 'real' netlink or generic
> netlink - personally I know the latter better and I think it has some
> real advantages like easier message parsing (it's automatic more or
> less) including strict checking and automatic policy introspection (I
> recently wrote the code for this and it's plugged into generic netlink
> family, for other netlink families it needs more hand-written code). But
> I could possibly be convinced of doing something else, and/or perhaps
> building more infrastructure for 'real' netlink to realize those
> benefits there as well.
>
>
> In terms of what I APIs are needed, the kernel-driver side and userspace
> side go pretty much hand in hand (the wwan subsystem just providing the
> glue), so what I say below is pretty much both a method/function call
> (kernel internal API) or a netlink message (userspace API).
>
> 1) I think a generic abstraction of WWAN device that is not a netdev
> is needed. Yes, on the one hand it's quite nice to be able to work on
> top of a given netdev, but it's also limiting because it requires the
> data flow to go through there, and packets that are tagged in some
> way be exchanged there.
> For VLANs this can be out-of-band (in a sense) with hw-accel, but for
> rmnet-style it's in-band, and that limits what we can do.
>
> Now, of course this doesn't mean there shouldn't be a netdev created
> by default in most cases, but it gives us a way to do additional
> things that we cannot do with *just* a netdev.
>
> From a driver POV though, it would register a new "wwan_device", and
> then get some generic callback to create a netdev on top, maybe by
> default from the subsystem or from the user.
Have you actually looked at Phonet or CAIF.
And netdev by default seems like repeating the same mistake we have done with WiFi. Your default context in LTE cases is only available when actually registered to the LTE bearer. It is pretty much pointless to have a netdev if you are not registered to the network.
You have to do a lot of initial modem setup before you ever get to the having your default context connected. Have a look at oFono and what it does to bring up the modem.
> 2) Clearly, one needs to be able to create PDN netdevs, with the PDN
> given to the command. Here's another advantage: If these are created
> on top of another abstraction, not another netdev, they can have
> their own queues, multiqueue RX etc. much more easily.
>
> Also, things like the "if I have just a single channel, drop the mux
> headers" can then be entirely done in the driver, and the default
> netdev no longer has the possibility of muxed and IP frames on the
> same datapath.
>
> This also enables more things like handling checksum offload directly
> in the driver, which doesn't behave so well with VLANs I think.
>
> All of that will just be easier for 5G too, I believe, with
> acceleration being handled per PDN, multi-queue working without
> ndo_select_queue, etc.
>
> Quite possibly there might be some additional (vendor-dependent?)
> configuration for when such netdevs are created, but we need to
> figure out if that really needs to be at creation time, or just
> ethtool later or something like that. I guess it depends on how
> generic it needs to be.
I think you need to provide actually a lot more details on how queue control inside Linux would be helpful and actually work in the first place. I don’t see how Linux will be ever in charge and not the modem do this all for you.
> 3) Separately, we need to have an ability to create "generalized control
> channels". I'm thinking there would be a general command "create
> control channel" with a given type (e.g. ATCMD, RPC, MBIM, GNSS) plus
> a list of vendor-specific channels (e.g. for tracing).
>
> I'm unsure where this channel should really go - somehow it seems to
> me that for many (most?) of these registering them as a serial line
> would be most appropriate, but some, especially vendor-defined
> channels like tracing, would probably better use a transport that's
> higher bandwidth than, e.g. netdevs.
>
> One way I thought of doing this was to create an abstraction in the
> wwan framework that lets the driver use SKBs anyway (i.e. TX and RX
> on these channels using SKBs) and then translate them to some channel
> in the framework - that way, we can even select at runtime if we want
> a netdev (not really plugged into the network stack, ARPHDR_VOID?) or
> some other kind of transport. Building that would allow us to add
> transport types in the future too.
>
> I guess such a channel should also be created by default, if it's
> not already created by the driver in some out-of-band way anyway (and
> most likely it shouldn't be, but I guess drivers might have some
> entirely different communication channels for AT CMDs?)
I would just use sockets like Phonet and CAIF.
> 4) There was a question about something like pure IP channels that
> belong to another PDN and apparently now separate netdevs might be
> used, but it seems to me that could just be a queue reserved on the
> regular netdevs and then when you say ("enable video streaming on
> wwan1 interface") that can do some magic to classify the video
> packets (DSCP?) to another hardware queue for better QoS.
>
>
>
> Anyway, if all of this doesn't seem completely outlandish I'll try to
> write some code to illustrate it (sooner, rather than later).
Frankly I have a problem if this is designed from the hardware point of view. Unless you are familiar with how 3GPP works and what a telephony stack like oFono has to do, there is really no point in trying to create a WWAN subsystem.
Anything defined needs to be defined in terms of 3GPP and not what current drivers have hacked together.
Regards
Marcel
^ permalink raw reply
* Re: [RFC PATCH v3] rtl8xxxu: Improve TX performance of RTL8723BU on rtl8xxxu driver
From: Larry Finger @ 2019-05-29 18:22 UTC (permalink / raw)
To: Chris Chiu, jes.sorensen, kvalo, davem
Cc: linux-wireless, netdev, linux-kernel, linux
In-Reply-To: <20190529050335.72061-1-chiu@endlessm.com>
On 5/29/19 12:03 AM, Chris Chiu wrote:
> We have 3 laptops which connect the wifi by the same RTL8723BU.
> The PCI VID/PID of the wifi chip is 10EC:B720 which is supported.
> They have the same problem with the in-kernel rtl8xxxu driver, the
> iperf (as a client to an ethernet-connected server) gets ~1Mbps.
> Nevertheless, the signal strength is reported as around -40dBm,
> which is quite good. From the wireshark capture, the tx rate for each
> data and qos data packet is only 1Mbps. Compare to the driver from
> https://github.com/lwfinger/rtl8723bu, the same iperf test gets ~12
> Mbps or more. The signal strength is reported similarly around
> -40dBm. That's why we want to improve.
The driver at GitHub was written by Realtek. I only published it in a prominent
location, and fix it for kernel API changes. I would say "the Realtek driver at
https://...", and every mention of "Larry's driver" should say "Realtek's
driver". That attribution is more correct.
>
> After reading the source code of the rtl8xxxu driver and Larry's, the
> major difference is that Larry's driver has a watchdog which will keep
> monitoring the signal quality and updating the rate mask just like the
> rtl8xxxu_gen2_update_rate_mask() does if signal quality changes.
> And this kind of watchdog also exists in rtlwifi driver of some specific
> chips, ex rtl8192ee, rtl8188ee, rtl8723ae, rtl8821ae...etc. They have
> the same member function named dm_watchdog and will invoke the
> corresponding dm_refresh_rate_adaptive_mask to adjust the tx rate
> mask.
>
> With this commit, the tx rate of each data and qos data packet will
> be 39Mbps (MCS4) with the 0xF00000 as the tx rate mask. The 20th bit
> to 23th bit means MCS4 to MCS7. It means that the firmware still picks
> the lowest rate from the rate mask and explains why the tx rate of
> data and qos data is always lowest 1Mbps because the default rate mask
> passed is always 0xFFFFFFF ranges from the basic CCK rate, OFDM rate,
> and MCS rate. However, with Larry's driver, the tx rate observed from
> wireshark under the same condition is almost 65Mbps or 72Mbps.
>
> I believe the firmware of RTL8723BU may need fix. And I think we
> can still bring in the dm_watchdog as rtlwifi to improve from the
> driver side. Please leave precious comments for my commits and
> suggest what I can do better. Or suggest if there's any better idea
> to fix this. Thanks.
>
> Signed-off-by: Chris Chiu <chiu@endlessm.com>
I have not tested this patch, but I plan to soon.
Larry
^ permalink raw reply
* Re: [RFC PATCH v3] rtl8xxxu: Improve TX performance of RTL8723BU on rtl8xxxu driver
From: Daniel Drake @ 2019-05-29 18:11 UTC (permalink / raw)
To: Chris Chiu
Cc: Jes Sorensen, Kalle Valo, David Miller, linux-wireless, netdev,
Linux Kernel, Linux Upstreaming Team
In-Reply-To: <20190529050335.72061-1-chiu@endlessm.com>
Hi Chris,
On Tue, May 28, 2019 at 11:03 PM Chris Chiu <chiu@endlessm.com> wrote:
> + /*
> + * Single virtual interface permitted since the driver supports STATION
> + * mode only.
I think you can be a bit more explicit by saying e.g.:
Only one virtual interface permitted because only STA mode is
supported and no iface_combinations are provided.
> diff --git a/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_core.c b/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_core.c
> index 039e5ca9d2e4..2d612c2df5b2 100644
> --- a/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_core.c
> +++ b/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_core.c
> @@ -4345,7 +4345,7 @@ void rtl8xxxu_gen2_update_rate_mask(struct rtl8xxxu_priv *priv,
> h2c.b_macid_cfg.ramask3 = (ramask >> 24) & 0xff;
>
> h2c.ramask.arg = 0x80;
> - h2c.b_macid_cfg.data1 = 0;
> + h2c.b_macid_cfg.data1 = priv->ratr_index;
I think ratr_index can be moved to be a function parameter of the
update_rate_mask function. It looks like all callsites already know
which value they want to set. Then you don't have to store it in the
priv structure.
> @@ -5471,6 +5509,10 @@ static int rtl8xxxu_add_interface(struct ieee80211_hw *hw,
>
> switch (vif->type) {
> case NL80211_IFTYPE_STATION:
> + if (!priv->vif)
> + priv->vif = vif;
> + else
> + return -EOPNOTSUPP;
> rtl8xxxu_stop_tx_beacon(priv);
rtl8xxxu_remove_interface should also set priv->vif back to NULL.
> @@ -6183,6 +6259,8 @@ static void rtl8xxxu_disconnect(struct usb_interface *interface)
> mutex_destroy(&priv->usb_buf_mutex);
> mutex_destroy(&priv->h2c_mutex);
>
> + cancel_delayed_work_sync(&priv->ra_watchdog);
Given that the work was started in rtl8xxxu_start, I think it should
be cancelled in rtl8xxxu_stop() instead.
Daniel
^ permalink raw reply
* Re: [PATCH 02/10] cfg80211: use BIT_ULL in cfg80211_parse_mbssid_data()
From: Kalle Valo @ 2019-05-29 16:28 UTC (permalink / raw)
To: Luca Coelho; +Cc: johannes, linux-wireless, Luca Coelho, Dan Carpented
In-Reply-To: <20190529122537.8564-3-luca@coelho.fi>
Luca Coelho <luca@coelho.fi> writes:
> From: Luca Coelho <luciano.coelho@intel.com>
>
> The seen_indices variable is u64 and in other parts of the code we
> assume mbssid_index_ie[2] can be up to 45, so we should use the 64-bit
> versions of BIT, namely, BIT_ULL().
>
> Reported-by: Dan Carpented <dan.carpenter@oracle.com>
s/ted/ter/ :)
--
Kalle Valo
^ permalink raw reply
* Re: FYI: vendor specific nl80211 API upstream
From: Denis Kenzior @ 2019-05-29 15:49 UTC (permalink / raw)
To: Johannes Berg, linux-wireless
In-Reply-To: <f3847d4efe6822bba3948993ddc4cde31615436f.camel@sipsolutions.net>
Hi Johannes,
On 05/29/2019 04:09 AM, Johannes Berg wrote:
> On Tue, 2019-05-28 at 12:36 -0500, Denis Kenzior wrote:
>>
>> I'm guessing that you guys considered and rejected the idea of pushing
>> these out to a separate, vendor specific genl family instead?
>
> We do actually use that internally (though mostly for cases where we
> don't have a cfg80211 connection like manufacturing support), but vendor
> commands are there and people do like to use them :-)
And herein lies the danger. If you make it too easy to add vendor APIs,
there's no incentive for the vendors to do anything else. In the end
this all becomes a mess for userspace to deal with.
One idea off the top of my head is to introduce a concept of
'experimental' APIs in NL80211, ones that are not guaranteed to be ABI
stable going forward. Specifically for dealing with such 'vendor' APIs.
The semantic difference might be subtle, but I think the effect will
be drastically different. E.g. people will approach this more seriously
and you will get more people reviewing the API.
>
> The idea with formalizing this is that they actually get more
> visibility, and I hope that this will lead to more forming of real
> nl80211 API too.
What about ABI guarantees (to tie it in with the discussion above) ?
If the vendor wants to change their API, can they? Are NL80211 APIs
stable unless they are vendor APIs?
Anyhow, speaking from experience with oFono, which has to deal with a
bazillion of wwan modem vendors, I suspect that the opposite will
actually happen. Any time we let through a vendor API, the vendor lost
any interest in generalizing it further. And it becomes a huge pain to
implement a proper generic one later. I get that there are cases where
something just cannot be generalized. In that case it belongs on a
separate genl family (or whatever) altogether.
So I would highly encourage you to reconsider this decision and
deprecate vendor APIs altogether. If someone really cares, they can
implement their own genl family. It is really not that hard. And then
they control the API, API stability policy, etc.
Regards,
-Denis
^ permalink raw reply
* Re: [PATCH] rtlwifi: Fix null-pointer dereferences in error handling code of rtl_pci_probe()
From: Larry Finger @ 2019-05-29 15:48 UTC (permalink / raw)
To: Jia-Ju Bai, Kalle Valo
Cc: pkshih, davem, linux-wireless, netdev, linux-kernel
In-Reply-To: <357682ba-d1ae-23b1-2372-b1a33d2ba1ac@gmail.com>
On 5/29/19 5:30 AM, Jia-Ju Bai wrote:
>
>
> On 2019/5/28 21:00, Larry Finger wrote:
>> On 5/28/19 6:55 AM, Kalle Valo wrote:
>>> Jia-Ju Bai <baijiaju1990@gmail.com> wrote:
>>>
>>>> *BUG 1:
>>>> In rtl_pci_probe(), when rtlpriv->cfg->ops->init_sw_vars() fails,
>>>> rtl_deinit_core() in the error handling code is executed.
>>>> rtl_deinit_core() calls rtl_free_entries_from_scan_list(), which uses
>>>> rtlpriv->scan_list.list in list_for_each_entry_safe(), but it has been
>>>> initialized. Thus a null-pointer dereference occurs.
>>>> The reason is that rtlpriv->scan_list.list is initialized by
>>>> INIT_LIST_HEAD() in rtl_init_core(), which has not been called.
>>>>
>>>> To fix this bug, rtl_deinit_core() should not be called when
>>>> rtlpriv->cfg->ops->init_sw_vars() fails.
>>>>
>>>> *BUG 2:
>>>> In rtl_pci_probe(), rtl_init_core() can fail when rtl_regd_init() in
>>>> this function fails, and rtlpriv->scan_list.list has not been
>>>> initialized by INIT_LIST_HEAD(). Then, rtl_deinit_core() in the error
>>>> handling code of rtl_pci_probe() is executed. Finally, a null-pointer
>>>> dereference occurs due to the same reason of the above bug.
>>>>
>>>> To fix this bug, the initialization of lists in rtl_init_core() are
>>>> performed before the call to rtl_regd_init().
>>>>
>>>> These bugs are found by a runtime fuzzing tool named FIZZER written by
>>>> us.
>>>>
>>>> Signed-off-by: Jia-Ju Bai <baijiaju1990@gmail.com>
>>>
>>> Ping & Larry, is this ok to take?
>>>
>>
>> Kalle,
>>
>> Not at the moment. In reviewing the code, I was unable to see how this
>> situation could develop, and his backtrace did not mention any rtlwifi code.
>> For that reason, I asked him to add printk stat4ements to show the last part
>> of rtl_pci that executed correctly. In
>> https://marc.info/?l=linux-wireless&m=155788322631134&w=2, he promised to do
>> that, but I have not seen the result.
>>
>
> Hi Larry,
>
> This patch is not related to the message you mentioned.
> That message is about an occasional crash that I reported.
> That crash occurred when request_irq() in rtl_pci_intr_mode_legacy() in
> rtl_pci_intr_mode_decide() fails.
> I have added printk statements and try to reproduce and debug that crash, but
> that crash does not always occur, and I still do not know the root cause of that
> crash.
>
> The null-pointer dereferences fixed by this patch are different from that crash,
> and they always occur when the related functions fail.
> So please review these null-pointer dereferences, thanks :)
>
>
> Best wishes,
> Jia-Ju Bai
Sorry if I got confused. Kalle has dropped this patch, thus you will need to
submit a new version.
Larry
^ permalink raw reply
* Re: [PATCH 01/11] rtw88: resolve order of tx power setting routines
From: Larry Finger @ 2019-05-29 15:16 UTC (permalink / raw)
To: yhchuang, kvalo; +Cc: linux-wireless
In-Reply-To: <1559116487-5244-2-git-send-email-yhchuang@realtek.com>
On 5/29/19 2:54 AM, yhchuang@realtek.com wrote:
> From: Yan-Hsuan Chuang <yhchuang@realtek.com>
>
> Some functions that should be static are unnecessarily exposed, remove
> their declaration in header file phy.h.
>
> After resolving their declaration order, they can be declared as static.
> So this commit changes nothing except the order and marking them static.
>
> Signed-off-by: Yan-Hsuan Chuang <yhchuang@realtek.com>
This patch does not apply. Using quilt to see what is wrong, there are 6 changes
that have already been applied.
Larry
^ permalink raw reply
* [PATCH] rtw88: Remove set but not used variable 'ip_sel' and 'orig'
From: YueHaibing @ 2019-05-29 14:57 UTC (permalink / raw)
To: yhchuang, kvalo, davem; +Cc: linux-kernel, netdev, linux-wireless, YueHaibing
Fixes gcc '-Wunused-but-set-variable' warnings:
drivers/net/wireless/realtek/rtw88/pci.c: In function rtw_pci_phy_cfg:
drivers/net/wireless/realtek/rtw88/pci.c:978:6: warning: variable ip_sel set but not used [-Wunused-but-set-variable]
drivers/net/wireless/realtek/rtw88/phy.c: In function phy_tx_power_limit_config:
drivers/net/wireless/realtek/rtw88/phy.c:1607:11: warning: variable orig set but not used [-Wunused-but-set-variable]
They are never used, so can be removed.
Signed-off-by: YueHaibing <yuehaibing@huawei.com>
---
drivers/net/wireless/realtek/rtw88/pci.c | 3 ---
drivers/net/wireless/realtek/rtw88/phy.c | 3 +--
2 files changed, 1 insertion(+), 5 deletions(-)
diff --git a/drivers/net/wireless/realtek/rtw88/pci.c b/drivers/net/wireless/realtek/rtw88/pci.c
index 353871c27779..8329f4e447b7 100644
--- a/drivers/net/wireless/realtek/rtw88/pci.c
+++ b/drivers/net/wireless/realtek/rtw88/pci.c
@@ -977,7 +977,6 @@ static void rtw_pci_phy_cfg(struct rtw_dev *rtwdev)
u16 cut;
u16 value;
u16 offset;
- u16 ip_sel;
int i;
cut = BIT(0) << rtwdev->hal.cut_version;
@@ -990,7 +989,6 @@ static void rtw_pci_phy_cfg(struct rtw_dev *rtwdev)
break;
offset = para->offset;
value = para->value;
- ip_sel = para->ip_sel;
if (para->ip_sel == RTW_IP_SEL_PHY)
rtw_mdio_write(rtwdev, offset, value, true);
else
@@ -1005,7 +1003,6 @@ static void rtw_pci_phy_cfg(struct rtw_dev *rtwdev)
break;
offset = para->offset;
value = para->value;
- ip_sel = para->ip_sel;
if (para->ip_sel == RTW_IP_SEL_PHY)
rtw_mdio_write(rtwdev, offset, value, false);
else
diff --git a/drivers/net/wireless/realtek/rtw88/phy.c b/drivers/net/wireless/realtek/rtw88/phy.c
index 404d89432c96..c3e75ffe27b5 100644
--- a/drivers/net/wireless/realtek/rtw88/phy.c
+++ b/drivers/net/wireless/realtek/rtw88/phy.c
@@ -1604,12 +1604,11 @@ void rtw_phy_tx_power_by_rate_config(struct rtw_hal *hal)
static void
phy_tx_power_limit_config(struct rtw_hal *hal, u8 regd, u8 bw, u8 rs)
{
- s8 base, orig;
+ s8 base;
u8 ch;
for (ch = 0; ch < RTW_MAX_CHANNEL_NUM_2G; ch++) {
base = hal->tx_pwr_by_rate_base_2g[0][rs];
- orig = hal->tx_pwr_limit_2g[regd][bw][rs][ch];
hal->tx_pwr_limit_2g[regd][bw][rs][ch] -= base;
}
--
2.17.1
^ permalink raw reply related
* [PATCH] mt76: Remove set but not used variables 'pid' and 'final_mpdu'
From: YueHaibing @ 2019-05-29 14:53 UTC (permalink / raw)
To: nbd, lorenzo.bianconi83, ryder.lee, royluo, kvalo, matthias.bgg,
sgruszka
Cc: linux-kernel, netdev, linux-wireless, linux-arm-kernel,
linux-mediatek, davem, YueHaibing
Fixes gcc '-Wunused-but-set-variable' warnings:
drivers/net/wireless/mediatek/mt76/mt7603/mac.c: In function mt7603_fill_txs:
drivers/net/wireless/mediatek/mt76/mt7603/mac.c:969:5: warning: variable pid set but not used [-Wunused-but-set-variable]
drivers/net/wireless/mediatek/mt76/mt7603/mac.c:961:7: warning: variable final_mpdu set but not used [-Wunused-but-set-variable]
drivers/net/wireless/mediatek/mt76/mt7615/mac.c: In function mt7615_fill_txs:
drivers/net/wireless/mediatek/mt76/mt7615/mac.c:555:5: warning: variable pid set but not used [-Wunused-but-set-variable]
drivers/net/wireless/mediatek/mt76/mt7615/mac.c:552:19: warning: variable final_mpdu set but not used [-Wunused-but-set-variable]
They are never used, so can be removed.
Signed-off-by: YueHaibing <yuehaibing@huawei.com>
---
drivers/net/wireless/mediatek/mt76/mt7603/mac.c | 4 ----
drivers/net/wireless/mediatek/mt76/mt7615/mac.c | 5 +----
2 files changed, 1 insertion(+), 8 deletions(-)
diff --git a/drivers/net/wireless/mediatek/mt76/mt7603/mac.c b/drivers/net/wireless/mediatek/mt76/mt7603/mac.c
index 6d506e34c3ee..5182a36276fc 100644
--- a/drivers/net/wireless/mediatek/mt76/mt7603/mac.c
+++ b/drivers/net/wireless/mediatek/mt76/mt7603/mac.c
@@ -958,7 +958,6 @@ mt7603_fill_txs(struct mt7603_dev *dev, struct mt7603_sta *sta,
int final_idx = 0;
u32 final_rate;
u32 final_rate_flags;
- bool final_mpdu;
bool ack_timeout;
bool fixed_rate;
bool probe;
@@ -966,7 +965,6 @@ mt7603_fill_txs(struct mt7603_dev *dev, struct mt7603_sta *sta,
bool cck = false;
int count;
u32 txs;
- u8 pid;
int idx;
int i;
@@ -974,9 +972,7 @@ mt7603_fill_txs(struct mt7603_dev *dev, struct mt7603_sta *sta,
probe = !!(info->flags & IEEE80211_TX_CTL_RATE_CTRL_PROBE);
txs = le32_to_cpu(txs_data[4]);
- final_mpdu = txs & MT_TXS4_ACKED_MPDU;
ampdu = !fixed_rate && (txs & MT_TXS4_AMPDU);
- pid = FIELD_GET(MT_TXS4_PID, txs);
count = FIELD_GET(MT_TXS4_TX_COUNT, txs);
txs = le32_to_cpu(txs_data[0]);
diff --git a/drivers/net/wireless/mediatek/mt76/mt7615/mac.c b/drivers/net/wireless/mediatek/mt76/mt7615/mac.c
index b8f48d10f27a..a51bfb6990b3 100644
--- a/drivers/net/wireless/mediatek/mt76/mt7615/mac.c
+++ b/drivers/net/wireless/mediatek/mt76/mt7615/mac.c
@@ -549,23 +549,20 @@ static bool mt7615_fill_txs(struct mt7615_dev *dev, struct mt7615_sta *sta,
{
struct ieee80211_supported_band *sband;
int i, idx, count, final_idx = 0;
- bool fixed_rate, final_mpdu, ack_timeout;
+ bool fixed_rate, ack_timeout;
bool probe, ampdu, cck = false;
u32 final_rate, final_rate_flags, final_nss, txs;
- u8 pid;
fixed_rate = info->status.rates[0].count;
probe = !!(info->flags & IEEE80211_TX_CTL_RATE_CTRL_PROBE);
txs = le32_to_cpu(txs_data[1]);
- final_mpdu = txs & MT_TXS1_ACKED_MPDU;
ampdu = !fixed_rate && (txs & MT_TXS1_AMPDU);
txs = le32_to_cpu(txs_data[3]);
count = FIELD_GET(MT_TXS3_TX_COUNT, txs);
txs = le32_to_cpu(txs_data[0]);
- pid = FIELD_GET(MT_TXS0_PID, txs);
final_rate = FIELD_GET(MT_TXS0_TX_RATE, txs);
ack_timeout = txs & MT_TXS0_ACK_TIMEOUT;
--
2.17.1
^ permalink raw reply related
* Re: [PATCH 1/3] brcmfmac: re-enable command decode in sdio_aos for BRCM 4354
From: Kalle Valo @ 2019-05-29 14:51 UTC (permalink / raw)
To: Doug Anderson
Cc: Arend van Spriel, Madhan Mohan R, Ulf Hansson, LKML,
Hante Meuleman, David S. Miller, netdev, Chi-Hsien Lin,
Brian Norris, linux-wireless, YueHaibing, Adrian Hunter,
open list:ARM/Rockchip SoC..., brcm80211-dev-list.pdl,
Matthias Kaehlcke, Naveen Gupta, Wright Feng, brcm80211-dev-list,
Double Lo, Franky Lin
In-Reply-To: <CAD=FV=VtxdEeFQsdF=U7-_7R+TXfVmA2_JMB_-WYidGHTLDgLw@mail.gmail.com>
Doug Anderson <dianders@chromium.org> writes:
> Hi,
>
> On Tue, May 28, 2019 at 5:18 AM Kalle Valo <kvalo@codeaurora.org> wrote:
>>
>> Douglas Anderson <dianders@chromium.org> wrote:
>>
>> > In commit 29f6589140a1 ("brcmfmac: disable command decode in
>> > sdio_aos") we disabled something called "command decode in sdio_aos"
>> > for a whole bunch of Broadcom SDIO WiFi parts.
>> >
>> > After that patch landed I find that my kernel log on
>> > rk3288-veyron-minnie and rk3288-veyron-speedy is filled with:
>> > brcmfmac: brcmf_sdio_bus_sleep: error while changing bus sleep state -110
>> >
>> > This seems to happen every time the Broadcom WiFi transitions out of
>> > sleep mode. Reverting the part of the commit that affects the WiFi on
>> > my boards fixes the problem for me, so that's what this patch does.
>> >
>> > Note that, in general, the justification in the original commit seemed
>> > a little weak. It looked like someone was testing on a SD card
>> > controller that would sometimes die if there were CRC errors on the
>> > bus. This used to happen back in early days of dw_mmc (the controller
>> > on my boards), but we fixed it. Disabling a feature on all boards
>> > just because one SD card controller is broken seems bad. ...so
>> > instead of just this patch possibly the right thing to do is to fully
>> > revert the original commit.
>> >
>> > Fixes: 29f6589140a1 ("brcmfmac: disable command decode in sdio_aos")
>> > Signed-off-by: Douglas Anderson <dianders@chromium.org>
>>
>> I don't see patch 2 in patchwork and I assume discussion continues.
>
> Apologies. I made sure to CC you individually on all the patches but
> didn't think about the fact that you use patchwork to manage and so
> didn't ensure all patches made it to all lists (by default each patch
> gets recipients individually from get_maintainer). I'll make sure to
> fix for patch set #2. If you want to see all the patches, you can at
> least find them on lore.kernel.org linked from the cover:
>
> https://lore.kernel.org/patchwork/cover/1075373/
No worries, I had the thread on my email but was just too busy to check.
So I instead wrote down my thought process so that somebode can correct
me in case I have misunderstood. I usually do that when it's not clear
what the next action should be.
--
Kalle Valo
^ permalink raw reply
* Re: [PATCH] rtlwifi: rtl8192cu: fix error handle when usb probe failed
From: Larry Finger @ 2019-05-29 14:51 UTC (permalink / raw)
To: pkshih, kvalo; +Cc: linux-wireless, andreyknvl
In-Reply-To: <20190529065730.25951-1-pkshih@realtek.com>
On 5/29/19 1:57 AM, pkshih@realtek.com wrote:
> From: Ping-Ke Shih <pkshih@realtek.com>
>
> rtl_usb_probe() must do error handle rtl_deinit_core() only if
> rtl_init_core() is done, otherwise goto error_out2.
>
> | usb 1-1: New USB device strings: Mfr=0, Product=0, SerialNumber=0
> | rtl_usb: reg 0xf0, usbctrl_vendorreq TimeOut! status:0xffffffb9 value=0x0
> | rtl8192cu: Chip version 0x10
> | rtl_usb: reg 0xa, usbctrl_vendorreq TimeOut! status:0xffffffb9 value=0x0
> | rtl_usb: Too few input end points found
> | INFO: trying to register non-static key.
> | the code is fine but needs lockdep annotation.
> | turning off the locking correctness validator.
> | CPU: 0 PID: 12 Comm: kworker/0:1 Not tainted 5.1.0-rc4-319354-g9a33b36 #3
> | Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS
> | Google 01/01/2011
> | Workqueue: usb_hub_wq hub_event
> | Call Trace:
> | __dump_stack lib/dump_stack.c:77 [inline]
> | dump_stack+0xe8/0x16e lib/dump_stack.c:113
> | assign_lock_key kernel/locking/lockdep.c:786 [inline]
> | register_lock_class+0x11b8/0x1250 kernel/locking/lockdep.c:1095
> | __lock_acquire+0xfb/0x37c0 kernel/locking/lockdep.c:3582
> | lock_acquire+0x10d/0x2f0 kernel/locking/lockdep.c:4211
> | __raw_spin_lock_irqsave include/linux/spinlock_api_smp.h:110 [inline]
> | _raw_spin_lock_irqsave+0x44/0x60 kernel/locking/spinlock.c:152
> | rtl_c2hcmd_launcher+0xd1/0x390
> | drivers/net/wireless/realtek/rtlwifi/base.c:2344
> | rtl_deinit_core+0x25/0x2d0 drivers/net/wireless/realtek/rtlwifi/base.c:574
> | rtl_usb_probe.cold+0x861/0xa70
> | drivers/net/wireless/realtek/rtlwifi/usb.c:1093
> | usb_probe_interface+0x31d/0x820 drivers/usb/core/driver.c:361
> | really_probe+0x2da/0xb10 drivers/base/dd.c:509
> | driver_probe_device+0x21d/0x350 drivers/base/dd.c:671
> | __device_attach_driver+0x1d8/0x290 drivers/base/dd.c:778
> | bus_for_each_drv+0x163/0x1e0 drivers/base/bus.c:454
> | __device_attach+0x223/0x3a0 drivers/base/dd.c:844
> | bus_probe_device+0x1f1/0x2a0 drivers/base/bus.c:514
> | device_add+0xad2/0x16e0 drivers/base/core.c:2106
> | usb_set_configuration+0xdf7/0x1740 drivers/usb/core/message.c:2021
> | generic_probe+0xa2/0xda drivers/usb/core/generic.c:210
> | usb_probe_device+0xc0/0x150 drivers/usb/core/driver.c:266
> | really_probe+0x2da/0xb10 drivers/base/dd.c:509
> | driver_probe_device+0x21d/0x350 drivers/base/dd.c:671
> | __device_attach_driver+0x1d8/0x290 drivers/base/dd.c:778
> | bus_for_each_drv+0x163/0x1e0 drivers/base/bus.c:454
> | __device_attach+0x223/0x3a0 drivers/base/dd.c:844
> | bus_probe_device+0x1f1/0x2a0 drivers/base/bus.c:514
> | device_add+0xad2/0x16e0 drivers/base/core.c:2106
> | usb_new_device.cold+0x537/0xccf drivers/usb/core/hub.c:2534
> | hub_port_connect drivers/usb/core/hub.c:5089 [inline]
> | hub_port_connect_change drivers/usb/core/hub.c:5204 [inline]
> | port_event drivers/usb/core/hub.c:5350 [inline]
> | hub_event+0x138e/0x3b00 drivers/usb/core/hub.c:5432
> | process_one_work+0x90f/0x1580 kernel/workqueue.c:2269
> | worker_thread+0x9b/0xe20 kernel/workqueue.c:2415
> | kthread+0x313/0x420 kernel/kthread.c:253
> | ret_from_fork+0x3a/0x50 arch/x86/entry/entry_64.S:352
>
> Reported-by: syzbot+1fcc5ef45175fc774231@syzkaller.appspotmail.com
> Signed-off-by: Ping-Ke Shih <pkshih@realtek.com>
I agree that this is a good fix.
Acked-by: Larry Finger <Larry.Finger@lwfinger.net>
Larry
> ---
> drivers/net/wireless/realtek/rtlwifi/usb.c | 5 +++--
> 1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/net/wireless/realtek/rtlwifi/usb.c b/drivers/net/wireless/realtek/rtlwifi/usb.c
> index e24fda5e9087..34d68dbf4b4c 100644
> --- a/drivers/net/wireless/realtek/rtlwifi/usb.c
> +++ b/drivers/net/wireless/realtek/rtlwifi/usb.c
> @@ -1064,13 +1064,13 @@ int rtl_usb_probe(struct usb_interface *intf,
> rtlpriv->cfg->ops->read_eeprom_info(hw);
> err = _rtl_usb_init(hw);
> if (err)
> - goto error_out;
> + goto error_out2;
> rtl_usb_init_sw(hw);
> /* Init mac80211 sw */
> err = rtl_init_core(hw);
> if (err) {
> pr_err("Can't allocate sw for mac80211\n");
> - goto error_out;
> + goto error_out2;
> }
> if (rtlpriv->cfg->ops->init_sw_vars(hw)) {
> pr_err("Can't init_sw_vars\n");
> @@ -1091,6 +1091,7 @@ int rtl_usb_probe(struct usb_interface *intf,
>
> error_out:
> rtl_deinit_core(hw);
> +error_out2:
> _rtl_usb_io_handler_release(hw);
> usb_put_dev(udev);
> complete(&rtlpriv->firmware_loading_complete);
>
^ permalink raw reply
* Re: [PATCH 4/7] iwlwifi: print fseq info upon fw assert
From: Kalle Valo @ 2019-05-29 14:39 UTC (permalink / raw)
To: Luca Coelho; +Cc: linux-wireless, Shahar S Matityahu, Luca Coelho
In-Reply-To: <20190529133955.31082-5-luca@coelho.fi>
Luca Coelho <luca@coelho.fi> writes:
> From: Shahar S Matityahu <shahar.s.matityahu@intel.com>
>
> Read fseq info from FW registers and print it upon fw assert.
> The print is needed since the fseq version coming from the TLV might
> not be the actual version that is used.
>
> Signed-off-by: Shahar S Matityahu <shahar.s.matityahu@intel.com>
> Signed-off-by: Luca Coelho <luciano.coelho@intel.com>
[...]
> +void iwl_fw_error_print_fseq_regs(struct iwl_fw_runtime *fwrt)
> +{
> + struct iwl_trans *trans = fwrt->trans;
> + unsigned long flags;
> + int i;
> + struct {
> + u32 addr;
> + const char *str;
> + } fseq_regs[] = {
> + FSEQ_REG(FSEQ_ERROR_CODE),
> + FSEQ_REG(FSEQ_TOP_INIT_VERSION),
> + FSEQ_REG(FSEQ_CNVIO_INIT_VERSION),
> + FSEQ_REG(FSEQ_OTP_VERSION),
> + FSEQ_REG(FSEQ_TOP_CONTENT_VERSION),
> + FSEQ_REG(FSEQ_ALIVE_TOKEN),
> + FSEQ_REG(FSEQ_CNVI_ID),
> + FSEQ_REG(FSEQ_CNVR_ID),
> + FSEQ_REG(CNVI_AUX_MISC_CHIP),
> + FSEQ_REG(CNVR_AUX_MISC_CHIP),
> + FSEQ_REG(CNVR_SCU_SD_REGS_SD_REG_DIG_DCDC_VTRIM),
> + FSEQ_REG(CNVR_SCU_SD_REGS_SD_REG_ACTIVE_VDIG_MIRROR),
> + };
Can fseq_regs be static const?
--
Kalle Valo
^ 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