* [RFC] rtlwifi: rtl8821ae: Use inline routines rather than macros for descriptor word 0
From: Larry Finger @ 2019-05-06 17:39 UTC (permalink / raw)
To: kvalo; +Cc: linux-wireless, pkshih, Larry Finger
The driver uses complicated macros to set parts of word 0 of the TX and RX
descriptors. These are changed into inline routines.
Signed-off-by: Larry Finger <Larry.Finger@lwfinger.net>
---
Kalle,
Based on your comment on how much you dislike those "byte macros", I have
converted a few of them from rtl8821ae into static inline functions.
Is this what you had in mind, and do you consider these changes to
improve the code?
These routines still need to mask the value before the ! operation with
the masked original value.
Thanks,
Larry
---
.../wireless/realtek/rtlwifi/rtl8821ae/trx.c | 33 ++++----
.../wireless/realtek/rtlwifi/rtl8821ae/trx.h | 80 ++++++++++++++-----
2 files changed, 78 insertions(+), 35 deletions(-)
diff --git a/drivers/net/wireless/realtek/rtlwifi/rtl8821ae/trx.c b/drivers/net/wireless/realtek/rtlwifi/rtl8821ae/trx.c
index 7b6faf38e09c..2ad33cfb1656 100644
--- a/drivers/net/wireless/realtek/rtlwifi/rtl8821ae/trx.c
+++ b/drivers/net/wireless/realtek/rtlwifi/rtl8821ae/trx.c
@@ -703,8 +703,8 @@ void rtl8821ae_tx_fill_desc(struct ieee80211_hw *hw,
if (firstseg) {
if (rtlhal->earlymode_enable) {
SET_TX_DESC_PKT_OFFSET(pdesc, 1);
- SET_TX_DESC_OFFSET(pdesc, USB_HWDESC_HEADER_LEN +
- EM_HDR_LEN);
+ set_tx_desc_offset((__le32 *)pdesc,
+ USB_HWDESC_HEADER_LEN + EM_HDR_LEN);
if (ptcb_desc->empkt_num) {
RT_TRACE(rtlpriv, COMP_SEND, DBG_TRACE,
"Insert 8 byte.pTcb->EMPktNum:%d\n",
@@ -713,7 +713,8 @@ void rtl8821ae_tx_fill_desc(struct ieee80211_hw *hw,
(u8 *)(skb->data));
}
} else {
- SET_TX_DESC_OFFSET(pdesc, USB_HWDESC_HEADER_LEN);
+ set_tx_desc_offset((__le32 *)pdesc,
+ USB_HWDESC_HEADER_LEN);
}
@@ -752,8 +753,8 @@ void rtl8821ae_tx_fill_desc(struct ieee80211_hw *hw,
SET_TX_DESC_TX_SUB_CARRIER(pdesc,
rtl8821ae_sc_mapping(hw, ptcb_desc));
- SET_TX_DESC_LINIP(pdesc, 0);
- SET_TX_DESC_PKT_SIZE(pdesc, (u16)skb_len);
+ set_tx_desc_linip((__le32 *)pdesc, 0);
+ set_tx_desc_pkt_size((__le32 *)pdesc, (u16)skb_len);
if (sta) {
u8 ampdu_density = sta->ht_cap.ampdu_density;
@@ -789,15 +790,15 @@ void rtl8821ae_tx_fill_desc(struct ieee80211_hw *hw,
RT_TRACE(rtlpriv, COMP_SEND, DBG_TRACE,
"Enable RDG function.\n");
SET_TX_DESC_RDG_ENABLE(pdesc, 1);
- SET_TX_DESC_HTC(pdesc, 1);
+ set_tx_desc_htc((__le32 *)pdesc, 1);
}
}
/* tx report */
rtl_set_tx_report(ptcb_desc, pdesc, hw, tx_info);
}
- SET_TX_DESC_FIRST_SEG(pdesc, (firstseg ? 1 : 0));
- SET_TX_DESC_LAST_SEG(pdesc, (lastseg ? 1 : 0));
+ set_tx_desc_first_seg((__le32 *)pdesc, (firstseg ? 1 : 0));
+ set_tx_desc_last_seg((__le32 *)pdesc, (lastseg ? 1 : 0));
SET_TX_DESC_TX_BUFFER_SIZE(pdesc, (u16)buf_len);
SET_TX_DESC_TX_BUFFER_ADDRESS(pdesc, mapping);
/* if (rtlpriv->dm.useramask) { */
@@ -815,7 +816,7 @@ void rtl8821ae_tx_fill_desc(struct ieee80211_hw *hw,
SET_TX_DESC_MORE_FRAG(pdesc, (lastseg ? 0 : 1));
if (is_multicast_ether_addr(ieee80211_get_DA(hdr)) ||
is_broadcast_ether_addr(ieee80211_get_DA(hdr))) {
- SET_TX_DESC_BMC(pdesc, 1);
+ set_tx_desc_bmc((__le32 *)pdesc, 1);
}
rtl8821ae_dm_set_tx_ant_by_tx_info(hw, pdesc, ptcb_desc->mac_id);
@@ -841,12 +842,12 @@ void rtl8821ae_tx_fill_cmddesc(struct ieee80211_hw *hw,
}
CLEAR_PCI_TX_DESC_CONTENT(pdesc, TX_DESC_SIZE);
- SET_TX_DESC_FIRST_SEG(pdesc, 1);
- SET_TX_DESC_LAST_SEG(pdesc, 1);
+ set_tx_desc_first_seg((__le32 *)pdesc, 1);
+ set_tx_desc_last_seg((__le32 *)pdesc, 1);
- SET_TX_DESC_PKT_SIZE((u8 *)pdesc, (u16)(skb->len));
+ set_tx_desc_pkt_size((__le32 *)pdesc, (u16)(skb->len));
- SET_TX_DESC_OFFSET(pdesc, USB_HWDESC_HEADER_LEN);
+ set_tx_desc_offset((__le32 *)pdesc, USB_HWDESC_HEADER_LEN);
SET_TX_DESC_USE_RATE(pdesc, 1);
SET_TX_DESC_TX_RATE(pdesc, DESC_RATE1M);
@@ -864,7 +865,7 @@ void rtl8821ae_tx_fill_cmddesc(struct ieee80211_hw *hw,
SET_TX_DESC_MACID(pdesc, 0);
- SET_TX_DESC_OWN(pdesc, 1);
+ set_tx_desc_own((__le32 *)pdesc, 1);
RT_PRINT_DATA(rtlpriv, COMP_CMD, DBG_LOUD,
"H2C Tx Cmd Content\n",
@@ -877,7 +878,7 @@ void rtl8821ae_set_desc(struct ieee80211_hw *hw, u8 *pdesc,
if (istx) {
switch (desc_name) {
case HW_DESC_OWN:
- SET_TX_DESC_OWN(pdesc, 1);
+ set_tx_desc_own((__le32 *)pdesc, 1);
break;
case HW_DESC_TX_NEXTDESC_ADDR:
SET_TX_DESC_NEXT_DESC_ADDRESS(pdesc, *(u32 *)val);
@@ -919,7 +920,7 @@ u64 rtl8821ae_get_desc(struct ieee80211_hw *hw,
if (istx) {
switch (desc_name) {
case HW_DESC_OWN:
- ret = GET_TX_DESC_OWN(pdesc);
+ ret = get_tx_desc_own((__le32 *)pdesc);
break;
case HW_DESC_TXBUFF_ADDR:
ret = GET_TX_DESC_TX_BUFFER_ADDRESS(pdesc);
diff --git a/drivers/net/wireless/realtek/rtlwifi/rtl8821ae/trx.h b/drivers/net/wireless/realtek/rtlwifi/rtl8821ae/trx.h
index 861d78a24d05..64deaf4dab23 100644
--- a/drivers/net/wireless/realtek/rtlwifi/rtl8821ae/trx.h
+++ b/drivers/net/wireless/realtek/rtlwifi/rtl8821ae/trx.h
@@ -14,25 +14,67 @@
#define USB_HWDESC_HEADER_LEN 40
#define CRCLENGTH 4
-#define SET_TX_DESC_PKT_SIZE(__pdesc, __val) \
- SET_BITS_TO_LE_4BYTE(__pdesc, 0, 16, __val)
-#define SET_TX_DESC_OFFSET(__pdesc, __val) \
- SET_BITS_TO_LE_4BYTE(__pdesc, 16, 8, __val)
-#define SET_TX_DESC_BMC(__pdesc, __val) \
- SET_BITS_TO_LE_4BYTE(__pdesc, 24, 1, __val)
-#define SET_TX_DESC_HTC(__pdesc, __val) \
- SET_BITS_TO_LE_4BYTE(__pdesc, 25, 1, __val)
-#define SET_TX_DESC_LAST_SEG(__pdesc, __val) \
- SET_BITS_TO_LE_4BYTE(__pdesc, 26, 1, __val)
-#define SET_TX_DESC_FIRST_SEG(__pdesc, __val) \
- SET_BITS_TO_LE_4BYTE(__pdesc, 27, 1, __val)
-#define SET_TX_DESC_LINIP(__pdesc, __val) \
- SET_BITS_TO_LE_4BYTE(__pdesc, 28, 1, __val)
-#define SET_TX_DESC_OWN(__pdesc, __val) \
- SET_BITS_TO_LE_4BYTE(__pdesc, 31, 1, __val)
-
-#define GET_TX_DESC_OWN(__pdesc) \
- LE_BITS_TO_4BYTE(__pdesc, 31, 1)
+/* Set packet size (16 bits) in TX descriptor word 0 */
+static inline void set_tx_desc_pkt_size(__le32 *__pdesc, u16 __val)
+{
+ *__pdesc = cpu_to_le32((le32_to_cpu(*__pdesc) & ~GENMASK(0, 15)) |
+ __val);
+}
+
+/* Set offset (8 bits) in TX descriptor word 0 */
+static inline void set_tx_desc_offset(__le32 *__pdesc, u8 __val)
+{
+ *__pdesc = cpu_to_le32((le32_to_cpu(*__pdesc) & ~GENMASK(16, 23)) |
+ __val << 16);
+}
+
+/* Set bmc (1 bit) in TX descriptor word 0 */
+static inline void set_tx_desc_bmc(__le32 *__pdesc, u8 __val)
+{
+ *__pdesc = cpu_to_le32((le32_to_cpu(*__pdesc) & ~GENMASK(24, 24)) |
+ __val << 24);
+}
+
+/* Set htc (1 bit) in TX descriptor word 0 */
+static inline void set_tx_desc_htc(__le32 *__pdesc, u8 __val)
+{
+ *__pdesc = cpu_to_le32((le32_to_cpu(*__pdesc) & ~GENMASK(25, 25)) |
+ __val << 25);
+}
+
+/* Set last segment flag (1 bit) in TX descriptor word 0 */
+static inline void set_tx_desc_last_seg(__le32 *__pdesc, u8 __val)
+{
+ *__pdesc = cpu_to_le32((le32_to_cpu(*__pdesc) & ~GENMASK(26, 26)) |
+ __val << 26);
+}
+
+/* Set first segment iflag (1 bit) in TX descriptor word 0 */
+static inline void set_tx_desc_first_seg(__le32 *__pdesc, u8 __val)
+{
+ *__pdesc = cpu_to_le32((le32_to_cpu(*__pdesc) & ~GENMASK(27, 27)) |
+ __val << 27);
+}
+
+/* Set linip (1 bit) in TX descriptor word 0 */
+static inline void set_tx_desc_linip(__le32 *__pdesc, u8 __val)
+{
+ *__pdesc = cpu_to_le32((le32_to_cpu(*__pdesc) & ~GENMASK(28, 28)) |
+ __val << 18);
+}
+
+/* Set own flag (1 bit) in TX descriptor word 0 */
+static inline void set_tx_desc_own(__le32 *__pdesc, u8 __val)
+{
+ *__pdesc = cpu_to_le32((le32_to_cpu(*__pdesc) & ~GENMASK(31, 31)) |
+ __val << 31);
+}
+
+/* Get own flag (1 bit) from TX descriptor word 0 */
+static inline u8 get_tx_desc_own(__le32 *__pdesc)
+{
+ return (le32_to_cpu(*__pdesc) & ~GENMASK(31, 31)) >> 31;
+}
#define SET_TX_DESC_MACID(__pdesc, __val) \
SET_BITS_TO_LE_4BYTE(__pdesc+4, 0, 7, __val)
--
2.21.0
^ permalink raw reply related
* Re: [PATCH v2 1/1] ath10k: Enable MSA region dump support for WCN3990
From: Brian Norris @ 2019-05-06 16:31 UTC (permalink / raw)
To: Govind Singh; +Cc: ath10k, linux-wireless
In-Reply-To: <20190506142603.1746-1-govinds@codeaurora.org>
On Mon, May 6, 2019 at 7:26 AM Govind Singh <govinds@codeaurora.org> wrote:
> --- a/drivers/net/wireless/ath/ath10k/snoc.c
> +++ b/drivers/net/wireless/ath/ath10k/snoc.c
> @@ -1586,6 +1587,72 @@ static int ath10k_hw_power_off(struct ath10k *ar)
> return ret;
> }
>
> +static void ath10k_msa_dump_memory(struct ath10k *ar,
> + struct ath10k_fw_crash_data *crash_data)
> +{
> + struct ath10k_snoc *ar_snoc = ath10k_snoc_priv(ar);
> + const struct ath10k_hw_mem_layout *mem_layout;
> + const struct ath10k_mem_region *current_region;
> + struct ath10k_dump_ram_data_hdr *hdr;
> + size_t buf_len;
> + u8 *buf;
> +
> + lockdep_assert_held(&ar->data_lock);
I believe that's the wrong lock now. See below.
> +
> + if (!crash_data && !crash_data->ramdump_buf)
> + return;
> +
> + mem_layout = ath10k_coredump_get_mem_layout(ar);
> + if (!mem_layout)
> + return;
> +
> + current_region = &mem_layout->region_table.regions[0];
> +
> + buf = crash_data->ramdump_buf;
> + buf_len = crash_data->ramdump_buf_len;
> + memset(buf, 0, buf_len);
> +
> + /* Reserve space for the header. */
> + hdr = (void *)buf;
> + buf += sizeof(*hdr);
> + buf_len -= sizeof(*hdr);
> +
> + hdr->region_type = cpu_to_le32(current_region->type);
> + hdr->start = cpu_to_le32(ar_snoc->qmi->msa_va);
> + hdr->length = cpu_to_le32(ar_snoc->qmi->msa_mem_size);
> +
> + if (current_region->len < ar_snoc->qmi->msa_mem_size) {
> + memcpy(buf, ar_snoc->qmi->msa_va, current_region->len);
> + ath10k_warn(ar, "msa dump length is less than msa size %x, %x\n",
> + current_region->len, ar_snoc->qmi->msa_mem_size);
> + } else {
> + memcpy(buf, ar_snoc->qmi->msa_va, ar_snoc->qmi->msa_mem_size);
> + }
> +}
> +
> +void ath10k_snoc_fw_crashed_dump(struct ath10k *ar)
> +{
> + struct ath10k_fw_crash_data *crash_data;
> + char guid[UUID_STRING_LEN + 1];
> +
> + spin_lock_bh(&ar->data_lock);
> +
> + ar->stats.fw_crash_counter++;
> +
> + crash_data = ath10k_coredump_new(ar);
This will (for good reason) spit a lockdep warning after this, I think:
38faed150438 ath10k: perform crash dump collection in workqueue
You need to hold 'dump_mutex' now. I believe you only need to hold
'data_lock' for the sake of the crash counter.
Brian
> +
> + if (crash_data)
> + scnprintf(guid, sizeof(guid), "%pUl", &crash_data->guid);
> + else
> + scnprintf(guid, sizeof(guid), "n/a");
> +
> + ath10k_err(ar, "firmware crashed! (guid %s)\n", guid);
> + ath10k_print_driver_info(ar);
> + ath10k_msa_dump_memory(ar, crash_data);
> +
> + spin_unlock_bh(&ar->data_lock);
> +}
^ permalink raw reply
* Re: [PATCH RFC] brcmfmac: sanitize DMI strings v2
From: Hans de Goede @ 2019-05-06 16:05 UTC (permalink / raw)
To: Victor Bravo, Kalle Valo
Cc: Arend Van Spriel, Franky Lin, Hante Meuleman, Chi-Hsien Lin,
Wright Feng, David S. Miller, linux-wireless,
brcm80211-dev-list.pdl, brcm80211-dev-list, linux-kernel
In-Reply-To: <20190506152441.ifjcdi73elxuq5it@localhost>
Hi,
On 06-05-19 17:24, Victor Bravo wrote:
> On Mon, May 06, 2019 at 03:26:28PM +0300, Kalle Valo wrote:
>> Hans de Goede <hdegoede@redhat.com> writes:
>>
>>> If we're going to do some filtering, then I suggest we play it safe and also
>>> disallow other chars which may be used as a separator somewhere, specifically
>>> ':' and ','.
>>>
>>> Currently upstream linux-firmware has these files which rely on the DMI
>>> matching:
>>>
>>> brcmfmac4330-sdio.Prowise-PT301.txt
>>> brcmfmac43430-sdio.Hampoo-D2D3_Vi8A1.txt
>>> brcmfmac43430a0-sdio.ONDA-V80 PLUS.txt
>>>
>>> The others are either part of the DMI override table for devices with unsuitable
>>> DMI strings like "Default String"; or are device-tree based.
>>>
>>> So as long as we don't break those 3 (or break the ONDA one but get a symlink
>>> in place) we can sanitize a bit more then just non-printable and '/'.
>>>
>>> Kalle, Arend, what is your opinion on this?
>>>
>>> Note I do not expect the ONDA V80 Plus to have a lot of Linux users,
>>> but it definitely has some.
>>
>> To me having spaces in filenames is a bad idea, but on the other hand we
>> do have the "don't break existing setups" rule, so it's not so simple. I
>> vote for not allowing spaces, I think that's the best for the long run,
>> but don't know what Arend thinks.
>
> I have found a fresh judicate on this:
> https://lkml.org/lkml/2018/12/22/221
>
> It seems clear that we have to support at least spaces for some time
> (maybe wih separate config option which will be deprecated but on by
> defaut until old files are considered gone).
Ah that issue, well that is not really comparable in that case a lot of
peoples setups were completely broken by that commit and it was a
quite surprising behavior change in a userspace facing API.
The nvram loading path already does 2 tries, I really don't want to
unnecessary complicate it with a third try.
The Onda V80 Plus is a X86 based Windows / Android dual boot tablet,
as said before I do not expect a ton of users to be running regular
Linux on it.
Given Kalle's clear preference for getting rid of the spaces lets
just do that. But first we must get a symlink added to linux-firmware
using the name with the _, newer kernels requiring a newer linux-firmware
to match is not unheard of AFAIK, so combined with the limited amount
of users I think this is a reasonable compromise.
Kalle, do you agree with getting the symlink added to linux-firmware
ASAP as a fix for the V80 Plus issue; or do you want to see a fallback
to the un-cleaned name as you suggested before ?
Regards,
Hans
^ permalink raw reply
* Re: [PATCH RFC] brcmfmac: sanitize DMI strings v2
From: Victor Bravo @ 2019-05-06 15:24 UTC (permalink / raw)
To: Kalle Valo
Cc: Hans de Goede, Arend Van Spriel, Franky Lin, Hante Meuleman,
Chi-Hsien Lin, Wright Feng, David S. Miller, linux-wireless,
brcm80211-dev-list.pdl, brcm80211-dev-list, linux-kernel
In-Reply-To: <87d0kvvkej.fsf@codeaurora.org>
On Mon, May 06, 2019 at 03:26:28PM +0300, Kalle Valo wrote:
> Hans de Goede <hdegoede@redhat.com> writes:
>
> > If we're going to do some filtering, then I suggest we play it safe and also
> > disallow other chars which may be used as a separator somewhere, specifically
> > ':' and ','.
> >
> > Currently upstream linux-firmware has these files which rely on the DMI
> > matching:
> >
> > brcmfmac4330-sdio.Prowise-PT301.txt
> > brcmfmac43430-sdio.Hampoo-D2D3_Vi8A1.txt
> > brcmfmac43430a0-sdio.ONDA-V80 PLUS.txt
> >
> > The others are either part of the DMI override table for devices with unsuitable
> > DMI strings like "Default String"; or are device-tree based.
> >
> > So as long as we don't break those 3 (or break the ONDA one but get a symlink
> > in place) we can sanitize a bit more then just non-printable and '/'.
> >
> > Kalle, Arend, what is your opinion on this?
> >
> > Note I do not expect the ONDA V80 Plus to have a lot of Linux users,
> > but it definitely has some.
>
> To me having spaces in filenames is a bad idea, but on the other hand we
> do have the "don't break existing setups" rule, so it's not so simple. I
> vote for not allowing spaces, I think that's the best for the long run,
> but don't know what Arend thinks.
I have found a fresh judicate on this:
https://lkml.org/lkml/2018/12/22/221
It seems clear that we have to support at least spaces for some time
(maybe wih separate config option which will be deprecated but on by
defaut until old files are considered gone).
> Maybe we could do some kind of fallback mechanism, like first trying the
> sanitised filename and if that's not found then we try the old filename
> with spaces? And if that old filename works we print a big fat warning
> that the user should update the file and that the old "filename with
> spaces" support is going away soon?
In case of parametric sanitizing function, this might be achievable
by sanitizing using "final" character set first, and falling back
to "compatible" character set on file not found. So this may actually
bring another requirement on the sanitizing function.
Regards,
v.
^ permalink raw reply
* [PATCH v2 1/1] ath10k: Enable MSA region dump support for WCN3990
From: Govind Singh @ 2019-05-06 14:26 UTC (permalink / raw)
To: ath10k; +Cc: linux-wireless, Govind Singh
MSA memory region caries the hw descriptors information.
Dump MSA region in core dump as this is very helpful in debugging
hw issues.
Testing: Tested on WCN3990 HW
Tested FW: WLAN.HL.3.1-00959-QCAHLSWMTPLZ-1
Signed-off-by: Govind Singh <govinds@codeaurora.org>
---
drivers/net/wireless/ath/ath10k/coredump.c | 21 +++++++
drivers/net/wireless/ath/ath10k/coredump.h | 1 +
drivers/net/wireless/ath/ath10k/qmi.c | 6 ++
drivers/net/wireless/ath/ath10k/snoc.c | 67 ++++++++++++++++++++++
drivers/net/wireless/ath/ath10k/snoc.h | 1 +
5 files changed, 96 insertions(+)
diff --git a/drivers/net/wireless/ath/ath10k/coredump.c b/drivers/net/wireless/ath/ath10k/coredump.c
index eadae2f9206b..56d62035c988 100644
--- a/drivers/net/wireless/ath/ath10k/coredump.c
+++ b/drivers/net/wireless/ath/ath10k/coredump.c
@@ -962,6 +962,19 @@ static const struct ath10k_mem_region qca4019_hw10_mem_regions[] = {
},
};
+static const struct ath10k_mem_region wcn399x_hw10_mem_regions[] = {
+ {
+ /* MSA region start is not fixed, hence it is assigned at runtime */
+ .type = ATH10K_MEM_REGION_TYPE_MSA,
+ .len = 0x100000,
+ .name = "DRAM",
+ .section_table = {
+ .sections = NULL,
+ .size = 0,
+ },
+ },
+};
+
static const struct ath10k_hw_mem_layout hw_mem_layouts[] = {
{
.hw_id = QCA6174_HW_1_0_VERSION,
@@ -1059,6 +1072,14 @@ static const struct ath10k_hw_mem_layout hw_mem_layouts[] = {
.size = ARRAY_SIZE(qca4019_hw10_mem_regions),
},
},
+ {
+ .hw_id = WCN3990_HW_1_0_DEV_VERSION,
+ .hw_rev = ATH10K_HW_WCN3990,
+ .region_table = {
+ .regions = wcn399x_hw10_mem_regions,
+ .size = ARRAY_SIZE(wcn399x_hw10_mem_regions),
+ },
+ },
};
static u32 ath10k_coredump_get_ramdump_size(struct ath10k *ar)
diff --git a/drivers/net/wireless/ath/ath10k/coredump.h b/drivers/net/wireless/ath/ath10k/coredump.h
index 5dac653e1649..9802e90483f4 100644
--- a/drivers/net/wireless/ath/ath10k/coredump.h
+++ b/drivers/net/wireless/ath/ath10k/coredump.h
@@ -126,6 +126,7 @@ enum ath10k_mem_region_type {
ATH10K_MEM_REGION_TYPE_IRAM2 = 5,
ATH10K_MEM_REGION_TYPE_IOSRAM = 6,
ATH10K_MEM_REGION_TYPE_IOREG = 7,
+ ATH10K_MEM_REGION_TYPE_MSA = 8,
};
/* Define a section of the region which should be copied. As not all parts
diff --git a/drivers/net/wireless/ath/ath10k/qmi.c b/drivers/net/wireless/ath/ath10k/qmi.c
index ba8f5a8f83d1..47da492a326b 100644
--- a/drivers/net/wireless/ath/ath10k/qmi.c
+++ b/drivers/net/wireless/ath/ath10k/qmi.c
@@ -817,9 +817,15 @@ ath10k_qmi_driver_event_post(struct ath10k_qmi *qmi,
static void ath10k_qmi_event_server_exit(struct ath10k_qmi *qmi)
{
struct ath10k *ar = qmi->ar;
+ struct ath10k_snoc *ar_snoc = ath10k_snoc_priv(ar);
ath10k_qmi_remove_msa_permission(qmi);
ath10k_core_free_board_files(ar);
+ if (!test_bit(ATH10K_SNOC_FLAG_UNREGISTERING, &ar_snoc->flags)) {
+ ath10k_qmi_remove_msa_permission(qmi);
+ ath10k_snoc_fw_crashed_dump(ar);
+ ath10k_qmi_setup_msa_permissions(qmi);
+ }
ath10k_snoc_fw_indication(ar, ATH10K_QMI_EVENT_FW_DOWN_IND);
ath10k_dbg(ar, ATH10K_DBG_QMI, "wifi fw qmi service disconnected\n");
}
diff --git a/drivers/net/wireless/ath/ath10k/snoc.c b/drivers/net/wireless/ath/ath10k/snoc.c
index 0be12996beba..252dd4ee782d 100644
--- a/drivers/net/wireless/ath/ath10k/snoc.c
+++ b/drivers/net/wireless/ath/ath10k/snoc.c
@@ -24,6 +24,7 @@
#include <linux/regulator/consumer.h>
#include "ce.h"
+#include "coredump.h"
#include "debug.h"
#include "hif.h"
#include "htc.h"
@@ -1586,6 +1587,72 @@ static int ath10k_hw_power_off(struct ath10k *ar)
return ret;
}
+static void ath10k_msa_dump_memory(struct ath10k *ar,
+ struct ath10k_fw_crash_data *crash_data)
+{
+ struct ath10k_snoc *ar_snoc = ath10k_snoc_priv(ar);
+ const struct ath10k_hw_mem_layout *mem_layout;
+ const struct ath10k_mem_region *current_region;
+ struct ath10k_dump_ram_data_hdr *hdr;
+ size_t buf_len;
+ u8 *buf;
+
+ lockdep_assert_held(&ar->data_lock);
+
+ if (!crash_data && !crash_data->ramdump_buf)
+ return;
+
+ mem_layout = ath10k_coredump_get_mem_layout(ar);
+ if (!mem_layout)
+ return;
+
+ current_region = &mem_layout->region_table.regions[0];
+
+ buf = crash_data->ramdump_buf;
+ buf_len = crash_data->ramdump_buf_len;
+ memset(buf, 0, buf_len);
+
+ /* Reserve space for the header. */
+ hdr = (void *)buf;
+ buf += sizeof(*hdr);
+ buf_len -= sizeof(*hdr);
+
+ hdr->region_type = cpu_to_le32(current_region->type);
+ hdr->start = cpu_to_le32(ar_snoc->qmi->msa_va);
+ hdr->length = cpu_to_le32(ar_snoc->qmi->msa_mem_size);
+
+ if (current_region->len < ar_snoc->qmi->msa_mem_size) {
+ memcpy(buf, ar_snoc->qmi->msa_va, current_region->len);
+ ath10k_warn(ar, "msa dump length is less than msa size %x, %x\n",
+ current_region->len, ar_snoc->qmi->msa_mem_size);
+ } else {
+ memcpy(buf, ar_snoc->qmi->msa_va, ar_snoc->qmi->msa_mem_size);
+ }
+}
+
+void ath10k_snoc_fw_crashed_dump(struct ath10k *ar)
+{
+ struct ath10k_fw_crash_data *crash_data;
+ char guid[UUID_STRING_LEN + 1];
+
+ spin_lock_bh(&ar->data_lock);
+
+ ar->stats.fw_crash_counter++;
+
+ crash_data = ath10k_coredump_new(ar);
+
+ if (crash_data)
+ scnprintf(guid, sizeof(guid), "%pUl", &crash_data->guid);
+ else
+ scnprintf(guid, sizeof(guid), "n/a");
+
+ ath10k_err(ar, "firmware crashed! (guid %s)\n", guid);
+ ath10k_print_driver_info(ar);
+ ath10k_msa_dump_memory(ar, crash_data);
+
+ spin_unlock_bh(&ar->data_lock);
+}
+
static const struct of_device_id ath10k_snoc_dt_match[] = {
{ .compatible = "qcom,wcn3990-wifi",
.data = &drv_priv,
diff --git a/drivers/net/wireless/ath/ath10k/snoc.h b/drivers/net/wireless/ath/ath10k/snoc.h
index 25383de8f17d..6d28a6290a94 100644
--- a/drivers/net/wireless/ath/ath10k/snoc.h
+++ b/drivers/net/wireless/ath/ath10k/snoc.h
@@ -101,5 +101,6 @@ static inline struct ath10k_snoc *ath10k_snoc_priv(struct ath10k *ar)
}
int ath10k_snoc_fw_indication(struct ath10k *ar, u64 type);
+void ath10k_snoc_fw_crashed_dump(struct ath10k *ar);
#endif /* _SNOC_H_ */
--
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project
^ permalink raw reply related
* Re: [PATCH RFC] brcmfmac: sanitize DMI strings v2
From: Victor Bravo @ 2019-05-06 14:06 UTC (permalink / raw)
To: Kalle Valo
Cc: Hans de Goede, Arend Van Spriel, Franky Lin, Hante Meuleman,
Chi-Hsien Lin, Wright Feng, David S. Miller, linux-wireless,
brcm80211-dev-list.pdl, brcm80211-dev-list, linux-kernel
In-Reply-To: <878svjvk9q.fsf@codeaurora.org>
On Mon, May 06, 2019 at 03:29:21PM +0300, Kalle Valo wrote:
> Victor Bravo <1905@spmblk.com> writes:
>
> > On Mon, May 06, 2019 at 11:42:06AM +0300, Kalle Valo wrote:
> >> Hans de Goede <hdegoede@redhat.com> writes:
> >>
> >> >> @@ -99,6 +107,15 @@ static const struct dmi_system_id dmi_platform_data[] = {
> >> >> {}
> >> >> };
> >> >> +void brcmf_dmi_sanitize(char *dst, const unsigned char *allowed,
> >> >> char safe)
> >> >> +{
> >> >> + while (*dst) {
> >> >> + if ((*dst < 0) || !(allowed[*dst / 8] & (1 << (*dst % 8))))
> >> >
> >> > At a first look I have no clue what this code is doing and I honestly do not feel
> >> > like figuring it out, this is clever, but IMHO not readable.
> >> >
> >> > Please just write this as if (*dst < 0x21 || (*dst > foo && < bar) || etc,
> >> > so that a human can actually see in one look what the code is doing.
> >>
> >> Is there an existing function for sanitising filenames so that we don't
> >> need to reinvent the wheel, maybe something like isalnum()?
> >
> > I would definitely prefer to use existing function, but I didn't find
> > any suitable one. Suggestions are welcome.
>
> I didn't find anything either, but hopefully someone knows.
>
> > As for implementation details, the one I posted was optimized for both
> > speed and size, and at least in my opinion this bit array driven
> > parametric implementation is exactly what is needed here (using a string
> > of allowed characters with strchr-style lookups would bring much worse
> > complexity, and checking the characters using series of hardcoded if
> > conditions could quickly grow to more than those 16 bytes used by the
> > array).
>
> But is this really something which should be optimised? This is driver
> initialisation, not in some hot path, right? Can you even measure the
> difference?
I didn't measure it, but according to my understanding of "Linux Kernel
Contributor Covenant Code of Conduct Interpretation" this actually
seems to be right according to "everyone involved wants to see the best
possible solution for the overall success of Linux" (but it also depends
on defintion of "best possible" of course).
Honestly, my exact thought behind this was that switch() or if
conditions shouldn't be used here (except simple case handling few
characters) because of above rule.
I also definitely want to avoid reinventing the wheel or even having
duplicate code in the kernel, so I tried to write the routine in a way
that allows it to be used as kernel library function usable in any
driver unless we manage to find existing wheel of similar parameters
elsewhere. That's why allowed and safe are passed as arguments even if
they are available as global varible / macro.
The bitmask approach also still seems useful as it allows to easily
handle any set of allowed characters, while it seems that the set
of unwanted characters may be extended to even more than my patch
handles.
So *in case no suitable existing sanitizing function is found*, the
approach IMHO might be also
1) fine-tuning this kind of sanitizing routine and making it a kernel
"library" function outside the driver, possibly with some useful macros
- e.g. parametric macro to check whether a value is present in a set
(which could also help to make this check more readable as suggested by
Hans), and I would also really like a macro for compile-time initializon
of the bit array from string literal containing allowed characters
(which unfortunately doesn't seem possible in C yet).
2) having only brcmf_dmi_allowed_chars and BRCMF_DMI_SAFE_CHAR in the
driver and use these with the "library" function.
Unfortunately I'm new to kernel development and have no clue whether
it's a good idea to start with such a function inside driver and
eventually moving it later, or start with getting the function in final
location first and delay driver modificatios until it's ready.
Regards,
v.
^ permalink raw reply
* [PATCH] iwlwifi: remove some unnecessary NULL checks
From: Dan Carpenter @ 2019-05-06 12:54 UTC (permalink / raw)
To: Johannes Berg
Cc: Emmanuel Grumbach, Luca Coelho, Intel Linux Wireless, Kalle Valo,
Naftali Goldstein, Sara Sharon, Shaul Triebitz,
Mordechay Goodstein, Liad Kaufman, Gregory Greenman,
linux-wireless, kernel-janitors
These pointers are an offset into the "sta" struct. They're assigned
like this:
const struct ieee80211_sta_vht_cap *vht_cap = &sta->vht_cap;
They're not the first member of the struct (->supp_rates[] is first) so
they can't be NULL.
Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
---
.../net/wireless/intel/iwlwifi/mvm/rs-fw.c | 23 +++++++++----------
1 file changed, 11 insertions(+), 12 deletions(-)
diff --git a/drivers/net/wireless/intel/iwlwifi/mvm/rs-fw.c b/drivers/net/wireless/intel/iwlwifi/mvm/rs-fw.c
index 659e21b2d4e7..b6fb670d249c 100644
--- a/drivers/net/wireless/intel/iwlwifi/mvm/rs-fw.c
+++ b/drivers/net/wireless/intel/iwlwifi/mvm/rs-fw.c
@@ -101,7 +101,7 @@ static u8 rs_fw_sgi_cw_support(struct ieee80211_sta *sta)
struct ieee80211_sta_he_cap *he_cap = &sta->he_cap;
u8 supp = 0;
- if (he_cap && he_cap->has_he)
+ if (he_cap->has_he)
return 0;
if (ht_cap->cap & IEEE80211_HT_CAP_SGI_20)
@@ -123,12 +123,12 @@ static u16 rs_fw_get_config_flags(struct iwl_mvm *mvm,
struct ieee80211_sta_ht_cap *ht_cap = &sta->ht_cap;
struct ieee80211_sta_vht_cap *vht_cap = &sta->vht_cap;
struct ieee80211_sta_he_cap *he_cap = &sta->he_cap;
- bool vht_ena = vht_cap && vht_cap->vht_supported;
+ bool vht_ena = vht_cap->vht_supported;
u16 flags = 0;
if (mvm->cfg->ht_params->stbc &&
(num_of_ant(iwl_mvm_get_valid_tx_ant(mvm)) > 1)) {
- if (he_cap && he_cap->has_he) {
+ if (he_cap->has_he) {
if (he_cap->he_cap_elem.phy_cap_info[2] &
IEEE80211_HE_PHY_CAP2_STBC_RX_UNDER_80MHZ)
flags |= IWL_TLC_MNG_CFG_FLAGS_STBC_MSK;
@@ -136,15 +136,14 @@ static u16 rs_fw_get_config_flags(struct iwl_mvm *mvm,
if (he_cap->he_cap_elem.phy_cap_info[7] &
IEEE80211_HE_PHY_CAP7_STBC_RX_ABOVE_80MHZ)
flags |= IWL_TLC_MNG_CFG_FLAGS_HE_STBC_160MHZ_MSK;
- } else if ((ht_cap &&
- (ht_cap->cap & IEEE80211_HT_CAP_RX_STBC)) ||
+ } else if ((ht_cap->cap & IEEE80211_HT_CAP_RX_STBC) ||
(vht_ena &&
(vht_cap->cap & IEEE80211_VHT_CAP_RXSTBC_MASK)))
flags |= IWL_TLC_MNG_CFG_FLAGS_STBC_MSK;
}
if (mvm->cfg->ht_params->ldpc &&
- ((ht_cap && (ht_cap->cap & IEEE80211_HT_CAP_LDPC_CODING)) ||
+ ((ht_cap->cap & IEEE80211_HT_CAP_LDPC_CODING) ||
(vht_ena && (vht_cap->cap & IEEE80211_VHT_CAP_RXLDPC))))
flags |= IWL_TLC_MNG_CFG_FLAGS_LDPC_MSK;
@@ -154,7 +153,7 @@ static u16 rs_fw_get_config_flags(struct iwl_mvm *mvm,
IEEE80211_HE_PHY_CAP1_LDPC_CODING_IN_PAYLOAD))
flags &= ~IWL_TLC_MNG_CFG_FLAGS_LDPC_MSK;
- if (he_cap && he_cap->has_he &&
+ if (he_cap->has_he &&
(he_cap->he_cap_elem.phy_cap_info[3] &
IEEE80211_HE_PHY_CAP3_DCM_MAX_CONST_RX_MASK))
flags |= IWL_TLC_MNG_CFG_FLAGS_HE_DCM_NSS_1_MSK;
@@ -293,13 +292,13 @@ static void rs_fw_set_supp_rates(struct ieee80211_sta *sta,
cmd->mode = IWL_TLC_MNG_MODE_NON_HT;
/* HT/VHT rates */
- if (he_cap && he_cap->has_he) {
+ if (he_cap->has_he) {
cmd->mode = IWL_TLC_MNG_MODE_HE;
rs_fw_he_set_enabled_rates(sta, sband, cmd);
- } else if (vht_cap && vht_cap->vht_supported) {
+ } else if (vht_cap->vht_supported) {
cmd->mode = IWL_TLC_MNG_MODE_VHT;
rs_fw_vht_set_enabled_rates(sta, vht_cap, cmd);
- } else if (ht_cap && ht_cap->ht_supported) {
+ } else if (ht_cap->ht_supported) {
cmd->mode = IWL_TLC_MNG_MODE_HT;
cmd->ht_rates[0][0] = cpu_to_le16(ht_cap->mcs.rx_mask[0]);
cmd->ht_rates[1][0] = cpu_to_le16(ht_cap->mcs.rx_mask[1]);
@@ -381,7 +380,7 @@ static u16 rs_fw_get_max_amsdu_len(struct ieee80211_sta *sta)
const struct ieee80211_sta_vht_cap *vht_cap = &sta->vht_cap;
const struct ieee80211_sta_ht_cap *ht_cap = &sta->ht_cap;
- if (vht_cap && vht_cap->vht_supported) {
+ if (vht_cap->vht_supported) {
switch (vht_cap->cap & IEEE80211_VHT_CAP_MAX_MPDU_MASK) {
case IEEE80211_VHT_CAP_MAX_MPDU_LENGTH_11454:
return IEEE80211_MAX_MPDU_LEN_VHT_11454;
@@ -391,7 +390,7 @@ static u16 rs_fw_get_max_amsdu_len(struct ieee80211_sta *sta)
return IEEE80211_MAX_MPDU_LEN_VHT_3895;
}
- } else if (ht_cap && ht_cap->ht_supported) {
+ } else if (ht_cap->ht_supported) {
if (ht_cap->cap & IEEE80211_HT_CAP_MAX_AMSDU)
/*
* agg is offloaded so we need to assume that agg
--
2.18.0
^ permalink raw reply related
* Re: [PATCH v2 4/5] rtw88: fix unassigned rssi_level in rtw_sta_info
From: Kalle Valo @ 2019-05-06 12:35 UTC (permalink / raw)
To: Tony Chuang; +Cc: linux-wireless@vger.kernel.org
In-Reply-To: <F7CD281DE3E379468C6D07993EA72F84D17EB4C9@RTITMBSVM04.realtek.com.tw>
Tony Chuang <yhchuang@realtek.com> writes:
>> -----Original Message-----
>> From: Kalle Valo [mailto:kvalo@codeaurora.org]
>> Sent: Monday, May 06, 2019 4:49 PM
>> To: Tony Chuang
>> Cc: linux-wireless@vger.kernel.org
>> Subject: Re: [PATCH v2 4/5] rtw88: fix unassigned rssi_level in rtw_sta_info
>>
>> <yhchuang@realtek.com> writes:
>>
>> > From: Yan-Hsuan Chuang <yhchuang@realtek.com>
>> >
>> > The new rssi_level should be stored in si, otherwise the rssi_level will
>> > never be updated and get a wrong RA mask, which is calculated by the
>> > rssi level
>> >
>> > Signed-off-by: Yan-Hsuan Chuang <yhchuang@realtek.com>
>>
>> Stanislaw suggested that this should go to 5.2. So what breaks from
>> user's point of view if this is not applied?
>>
>
> If the rssi level remains unchanged, then we could choose wrong ra_mask.
> And some *bad rates* we be chosen by firmware.
> The most hurtful scene would be *noisy environment* such as office, or public.
> The latency would be high and overall throughput would be only half.
> (This was tested, such as 4x Mbps -> 1x Mbps)
Yeah, then this is definitely suitable for 5.2. Could you please resend
the patch and mention the symtomps in the commit log? And mark the patch
as "[PATCH 5.2 v3]" so that I can easily see it's for v5.2, please.
--
Kalle Valo
^ permalink raw reply
* Re: [PATCH RFC] brcmfmac: sanitize DMI strings v2
From: Kalle Valo @ 2019-05-06 12:29 UTC (permalink / raw)
To: Victor Bravo
Cc: Hans de Goede, Arend Van Spriel, Franky Lin, Hante Meuleman,
Chi-Hsien Lin, Wright Feng, David S. Miller, linux-wireless,
brcm80211-dev-list.pdl, brcm80211-dev-list, linux-kernel
In-Reply-To: <20190506091441.wqtccm4n6xxhxom2@localhost>
Victor Bravo <1905@spmblk.com> writes:
> On Mon, May 06, 2019 at 11:42:06AM +0300, Kalle Valo wrote:
>> Hans de Goede <hdegoede@redhat.com> writes:
>>
>> >> @@ -99,6 +107,15 @@ static const struct dmi_system_id dmi_platform_data[] = {
>> >> {}
>> >> };
>> >> +void brcmf_dmi_sanitize(char *dst, const unsigned char *allowed,
>> >> char safe)
>> >> +{
>> >> + while (*dst) {
>> >> + if ((*dst < 0) || !(allowed[*dst / 8] & (1 << (*dst % 8))))
>> >
>> > At a first look I have no clue what this code is doing and I honestly do not feel
>> > like figuring it out, this is clever, but IMHO not readable.
>> >
>> > Please just write this as if (*dst < 0x21 || (*dst > foo && < bar) || etc,
>> > so that a human can actually see in one look what the code is doing.
>>
>> Is there an existing function for sanitising filenames so that we don't
>> need to reinvent the wheel, maybe something like isalnum()?
>
> I would definitely prefer to use existing function, but I didn't find
> any suitable one. Suggestions are welcome.
I didn't find anything either, but hopefully someone knows.
> As for implementation details, the one I posted was optimized for both
> speed and size, and at least in my opinion this bit array driven
> parametric implementation is exactly what is needed here (using a string
> of allowed characters with strchr-style lookups would bring much worse
> complexity, and checking the characters using series of hardcoded if
> conditions could quickly grow to more than those 16 bytes used by the
> array).
But is this really something which should be optimised? This is driver
initialisation, not in some hot path, right? Can you even measure the
difference?
--
Kalle Valo
^ permalink raw reply
* Re: [PATCH RFC] brcmfmac: sanitize DMI strings v2
From: Kalle Valo @ 2019-05-06 12:26 UTC (permalink / raw)
To: Hans de Goede
Cc: Victor Bravo, Arend Van Spriel, Franky Lin, Hante Meuleman,
Chi-Hsien Lin, Wright Feng, David S. Miller, linux-wireless,
brcm80211-dev-list.pdl, brcm80211-dev-list, linux-kernel
In-Reply-To: <fb07ae01-4cca-98e7-1c2d-dfdf44909900@redhat.com>
Hans de Goede <hdegoede@redhat.com> writes:
> If we're going to do some filtering, then I suggest we play it safe and also
> disallow other chars which may be used as a separator somewhere, specifically
> ':' and ','.
>
> Currently upstream linux-firmware has these files which rely on the DMI
> matching:
>
> brcmfmac4330-sdio.Prowise-PT301.txt
> brcmfmac43430-sdio.Hampoo-D2D3_Vi8A1.txt
> brcmfmac43430a0-sdio.ONDA-V80 PLUS.txt
>
> The others are either part of the DMI override table for devices with unsuitable
> DMI strings like "Default String"; or are device-tree based.
>
> So as long as we don't break those 3 (or break the ONDA one but get a symlink
> in place) we can sanitize a bit more then just non-printable and '/'.
>
> Kalle, Arend, what is your opinion on this?
>
> Note I do not expect the ONDA V80 Plus to have a lot of Linux users,
> but it definitely has some.
To me having spaces in filenames is a bad idea, but on the other hand we
do have the "don't break existing setups" rule, so it's not so simple. I
vote for not allowing spaces, I think that's the best for the long run,
but don't know what Arend thinks.
Maybe we could do some kind of fallback mechanism, like first trying the
sanitised filename and if that's not found then we try the old filename
with spaces? And if that old filename works we print a big fat warning
that the user should update the file and that the old "filename with
spaces" support is going away soon?
--
Kalle Valo
^ permalink raw reply
* KASAN: use-after-free Read in p54u_load_firmware_cb
From: syzbot @ 2019-05-06 11:16 UTC (permalink / raw)
To: andreyknvl, chunkeey, davem, kvalo, linux-kernel, linux-usb,
linux-wireless, netdev, syzkaller-bugs
Hello,
syzbot found the following crash on:
HEAD commit: 43151d6c usb-fuzzer: main usb gadget fuzzer driver
git tree: https://github.com/google/kasan.git usb-fuzzer
console output: https://syzkaller.appspot.com/x/log.txt?x=142b312ca00000
kernel config: https://syzkaller.appspot.com/x/.config?x=4183eeef650d1234
dashboard link: https://syzkaller.appspot.com/bug?extid=200d4bb11b23d929335f
compiler: gcc (GCC) 9.0.0 20181231 (experimental)
Unfortunately, I don't have any reproducer for this crash yet.
IMPORTANT: if you fix the bug, please add the following tag to the commit:
Reported-by: syzbot+200d4bb11b23d929335f@syzkaller.appspotmail.com
usb 4-1: Direct firmware load for isl3887usb failed with error -2
usb 4-1: Firmware not found.
==================================================================
BUG: KASAN: use-after-free in p54u_load_firmware_cb.cold+0x97/0x13a
drivers/net/wireless/intersil/p54/p54usb.c:936
Read of size 8 at addr ffff888098bf3588 by task kworker/0:1/12
CPU: 0 PID: 12 Comm: kworker/0:1 Not tainted 5.1.0-rc3-319004-g43151d6 #6
Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS
Google 01/01/2011
Workqueue: events request_firmware_work_func
Call Trace:
__dump_stack lib/dump_stack.c:77 [inline]
dump_stack+0xe8/0x16e lib/dump_stack.c:113
print_address_description+0x6c/0x236 mm/kasan/report.c:187
kasan_report.cold+0x1a/0x3c mm/kasan/report.c:317
p54u_load_firmware_cb.cold+0x97/0x13a
drivers/net/wireless/intersil/p54/p54usb.c:936
request_firmware_work_func+0x12d/0x249
drivers/base/firmware_loader/main.c:785
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
The buggy address belongs to the page:
page:ffffea000262fcc0 count:0 mapcount:0 mapping:0000000000000000 index:0x0
flags: 0xfff00000000000()
raw: 00fff00000000000 0000000000000000 ffffffff02620101 0000000000000000
raw: 0000000000000000 0000000000000000 00000000ffffffff 0000000000000000
page dumped because: kasan: bad access detected
Memory state around the buggy address:
ffff888098bf3480: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff
ffff888098bf3500: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff
> ffff888098bf3580: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff
^
ffff888098bf3600: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff
ffff888098bf3680: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff
==================================================================
---
This bug is generated by a bot. It may contain errors.
See https://goo.gl/tpsmEJ for more information about syzbot.
syzbot engineers can be reached at syzkaller@googlegroups.com.
syzbot will keep track of this bug report. See:
https://goo.gl/tpsmEJ#status for how to communicate with syzbot.
^ permalink raw reply
* KASAN: slab-out-of-bounds Read in p54u_load_firmware_cb
From: syzbot @ 2019-05-06 11:16 UTC (permalink / raw)
To: andreyknvl, chunkeey, davem, kvalo, linux-kernel, linux-usb,
linux-wireless, netdev, syzkaller-bugs
Hello,
syzbot found the following crash on:
HEAD commit: 43151d6c usb-fuzzer: main usb gadget fuzzer driver
git tree: https://github.com/google/kasan.git usb-fuzzer
console output: https://syzkaller.appspot.com/x/log.txt?x=12c0112ca00000
kernel config: https://syzkaller.appspot.com/x/.config?x=4183eeef650d1234
dashboard link: https://syzkaller.appspot.com/bug?extid=6d237e74cdc13f036473
compiler: gcc (GCC) 9.0.0 20181231 (experimental)
Unfortunately, I don't have any reproducer for this crash yet.
IMPORTANT: if you fix the bug, please add the following tag to the commit:
Reported-by: syzbot+6d237e74cdc13f036473@syzkaller.appspotmail.com
cx231xx 2-1:0.1: New device @ 480 Mbps (2040:b111) with 1 interfaces
cx231xx 2-1:0.1: Not found matching IAD interface
usb 6-1: Direct firmware load for isl3887usb failed with error -2
usb 6-1: Firmware not found.
==================================================================
BUG: KASAN: slab-out-of-bounds in p54u_load_firmware_cb.cold+0x97/0x13a
drivers/net/wireless/intersil/p54/p54usb.c:936
Read of size 8 at addr ffff888099763588 by task kworker/0:1/12
CPU: 0 PID: 12 Comm: kworker/0:1 Not tainted 5.1.0-rc3-319004-g43151d6 #6
Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS
Google 01/01/2011
Workqueue: events request_firmware_work_func
Call Trace:
__dump_stack lib/dump_stack.c:77 [inline]
dump_stack+0xe8/0x16e lib/dump_stack.c:113
print_address_description+0x6c/0x236 mm/kasan/report.c:187
kasan_report.cold+0x1a/0x3c mm/kasan/report.c:317
p54u_load_firmware_cb.cold+0x97/0x13a
drivers/net/wireless/intersil/p54/p54usb.c:936
request_firmware_work_func+0x12d/0x249
drivers/base/firmware_loader/main.c:785
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
Allocated by task 7931:
set_track mm/kasan/common.c:87 [inline]
__kasan_kmalloc mm/kasan/common.c:497 [inline]
__kasan_kmalloc.constprop.0+0xbf/0xd0 mm/kasan/common.c:470
slab_post_alloc_hook mm/slab.h:437 [inline]
slab_alloc_node mm/slub.c:2756 [inline]
__kmalloc_node_track_caller+0xf3/0x320 mm/slub.c:4372
__kmalloc_reserve.isra.0+0x3e/0xf0 net/core/skbuff.c:140
__alloc_skb+0xf4/0x5a0 net/core/skbuff.c:208
alloc_skb include/linux/skbuff.h:1058 [inline]
alloc_uevent_skb+0x7b/0x210 lib/kobject_uevent.c:289
uevent_net_broadcast_untagged lib/kobject_uevent.c:325 [inline]
kobject_uevent_net_broadcast lib/kobject_uevent.c:408 [inline]
kobject_uevent_env+0x865/0x13d0 lib/kobject_uevent.c:589
udc_bind_to_driver+0x33e/0x5e0 drivers/usb/gadget/udc/core.c:1357
usb_gadget_probe_driver+0x23f/0x380 drivers/usb/gadget/udc/core.c:1408
fuzzer_ioctl_run drivers/usb/gadget/fuzzer/fuzzer.c:718 [inline]
fuzzer_ioctl+0x15be/0x1d90 drivers/usb/gadget/fuzzer/fuzzer.c:1130
full_proxy_unlocked_ioctl+0x11b/0x180 fs/debugfs/file.c:205
vfs_ioctl fs/ioctl.c:46 [inline]
file_ioctl fs/ioctl.c:509 [inline]
do_vfs_ioctl+0xced/0x12f0 fs/ioctl.c:696
ksys_ioctl+0xa0/0xc0 fs/ioctl.c:713
__do_sys_ioctl fs/ioctl.c:720 [inline]
__se_sys_ioctl fs/ioctl.c:718 [inline]
__x64_sys_ioctl+0x74/0xb0 fs/ioctl.c:718
do_syscall_64+0xcf/0x4f0 arch/x86/entry/common.c:290
entry_SYSCALL_64_after_hwframe+0x49/0xbe
Freed by task 805:
set_track mm/kasan/common.c:87 [inline]
__kasan_slab_free+0x130/0x180 mm/kasan/common.c:459
slab_free_hook mm/slub.c:1429 [inline]
slab_free_freelist_hook+0x5e/0x140 mm/slub.c:1456
slab_free mm/slub.c:3003 [inline]
kfree+0xce/0x290 mm/slub.c:3958
skb_free_head+0x90/0xb0 net/core/skbuff.c:557
skb_release_data+0x543/0x8b0 net/core/skbuff.c:577
skb_release_all+0x4b/0x60 net/core/skbuff.c:631
__kfree_skb net/core/skbuff.c:645 [inline]
consume_skb net/core/skbuff.c:705 [inline]
consume_skb+0xc5/0x2f0 net/core/skbuff.c:699
skb_free_datagram+0x1b/0xf0 net/core/datagram.c:329
netlink_recvmsg+0x663/0xea0 net/netlink/af_netlink.c:2004
sock_recvmsg_nosec net/socket.c:881 [inline]
sock_recvmsg net/socket.c:888 [inline]
sock_recvmsg+0xd1/0x110 net/socket.c:884
___sys_recvmsg+0x278/0x5a0 net/socket.c:2422
__sys_recvmsg+0xee/0x1b0 net/socket.c:2471
do_syscall_64+0xcf/0x4f0 arch/x86/entry/common.c:290
entry_SYSCALL_64_after_hwframe+0x49/0xbe
The buggy address belongs to the object at ffff888099763180
which belongs to the cache kmalloc-1k of size 1024
The buggy address is located 8 bytes to the right of
1024-byte region [ffff888099763180, ffff888099763580)
The buggy address belongs to the page:
page:ffffea000265d800 count:1 mapcount:0 mapping:ffff88812c3f4a00 index:0x0
compound_mapcount: 0
flags: 0xfff00000010200(slab|head)
raw: 00fff00000010200 dead000000000100 dead000000000200 ffff88812c3f4a00
raw: 0000000000000000 00000000000e000e 00000001ffffffff 0000000000000000
page dumped because: kasan: bad access detected
Memory state around the buggy address:
ffff888099763480: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb
ffff888099763500: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb
> ffff888099763580: fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc
^
ffff888099763600: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb
ffff888099763680: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb
==================================================================
---
This bug is generated by a bot. It may contain errors.
See https://goo.gl/tpsmEJ for more information about syzbot.
syzbot engineers can be reached at syzkaller@googlegroups.com.
syzbot will keep track of this bug report. See:
https://goo.gl/tpsmEJ#status for how to communicate with syzbot.
^ permalink raw reply
* Re: [PATCH RFC] brcmfmac: sanitize DMI strings v2
From: Hans de Goede @ 2019-05-06 10:34 UTC (permalink / raw)
To: Victor Bravo
Cc: Arend Van Spriel, Franky Lin, Hante Meuleman, Chi-Hsien Lin,
Wright Feng, Kalle Valo, David S. Miller, linux-wireless,
brcm80211-dev-list.pdl, brcm80211-dev-list, linux-kernel
In-Reply-To: <20190506102032.3ximjecado4mz62j@localhost>
Hi,
On 06-05-19 12:20, Victor Bravo wrote:
> On Mon, May 06, 2019 at 11:33:20AM +0200, Hans de Goede wrote:
>> Hi,
>>
>> On 06-05-19 11:06, Victor Bravo wrote:
>>> On Mon, May 06, 2019 at 10:13:38AM +0200, Hans de Goede wrote:
>>>> Hi,
>>>
>>> Hi,
>>>
>>>> On 05-05-19 17:03, Victor Bravo wrote:
>>>>> Sanitize DMI strings in brcmfmac driver to make resulting filenames
>>>>> contain only safe characters. This version replaces all non-printable
>>>>> characters incl. delete (0-31, 127-255), spaces and slashes with
>>>>> underscores.
>>>>>
>>>>> This change breaks backward compatibility, but adds control over strings
>>>>> passed to firmware loader and compatibility with CONFIG_EXTRA_FIRMWARE
>>>>> which doesn't support spaces in filenames.
>>>>>
>>>>> Changes from v1: don't revert fresh commit by someone else
>>>>>
>>>>> Signed-off-by: Victor Bravo <1905@spmblk.com>
>>>>
>>>> Thank you for the patch, but I'm sorry to say this patch cannot go in as is,
>>>> because it will break existing systems.
>>>>
>>>> If you look here:
>>>>
>>>> https://git.kernel.org/pub/scm/linux/kernel/git/firmware/linux-firmware.git/tree/brcm
>>>>
>>>> You will see a file named: "brcmfmac43430a0-sdio.ONDA-V80 PLUS.txt" there, which
>>>> has a space in its name (and which works fine).
>>>
>>> Thanks for the updates. Spaces are actually a problem as files with spaces
>>> don't work when built-in with CONFIG_EXTRA_FIRMWARE (which is used with
>>> non-modular kernel containing brcmfmac driver).
>>>
>>> If the DMI string contains slashes, they will cause problems
>>> for obvious reasons too.
>>
>> Right, as said I'm fine with sanitizing the names, so dropping e.g. / chars,
>> but replacing space with _ will cause wifi to stop working on Onda V80 Plus devices and
>> we have a clear no regressions policy in the kernel.
>
> Please keep in mind that dropping slashes and non-printable characters
> might be a regression too, as some users may already be using
> intermediate directories or filenames with tabs etc. if their DMI
> strings contain these. If such users exist, they will have to rename
> their nvram files anyway.
I consider it very unlikely that such users exist OTOH the Onda V80 PLus
nvram file is actually in linux-firmware upstream, so that e.g. Fedora 30
has working wifi OOTB on the V80 Plus, if we go with your proposal to
also replace spaces, then users with a V80 Plus will all of a sudden have
their wifi stop working.
If Kalle and Arend are in favor of including space in the "bad character"
list then at a minimum we must first get a patch added to linux-firmware
adding an ONDA-V80_PLUS.txt symlink to the ONDA-V80 PLUS.txt file.
> On the other hand, removing just slashes and non-printable characters
> (i.e. (*dst < 0x20) || (dst == '/') || (dst == 0x7f)) would allow
> the sanitize function to the bit array and go with hardcoded conditions
> (especially if those are final and won't need further adjustments in
> set of allowed characters
If we're going to do some filtering, then I suggest we play it safe and also
disallow other chars which may be used as a separator somewhere, specifically
':' and ','.
Currently upstream linux-firmware has these files which rely on the DMI
matching:
brcmfmac4330-sdio.Prowise-PT301.txt
brcmfmac43430-sdio.Hampoo-D2D3_Vi8A1.txt
brcmfmac43430a0-sdio.ONDA-V80 PLUS.txt
The others are either part of the DMI override table for devices with unsuitable
DMI strings like "Default String"; or are device-tree based.
So as long as we don't break those 3 (or break the ONDA one but get a symlink
in place) we can sanitize a bit more then just non-printable and '/'.
Kalle, Arend, what is your opinion on this?
Note I do not expect the ONDA V80 Plus to have a lot of Linux users, but it definitely has some.
Regards,
Hans
>
>>>> I'm fine with doing some sanitizing of the strings, but replacing spaces with _
>>>> breaks existing use-cases (will cause a regression for them) and a space is absolutely
>>>> a valid character in a filename and the firmware-loader can deal with this just fine.
>>>>
>>>> If the code for building firmwares into the kernel cannot deal with spaces then IMHO
>>>> that code should be fixed instead. Have you looked into fixing that?
>>>
>>> Yes, but updating CONFIG_EXTRA_FIRMWARE to support spaces because of
>>> this looks much like
>>
>> <snip off-topic remark>
>>
>>> Do you really think it's a good idea to propose that in
>>> this case?
>>
>> I think expecting spaces in filenames to just work is quite reasonable, after all
>> its been a long time since we've left DOS-es 8.3 filename limitations.
>>
>> Have you actually looked at how hard it would be to make filenames with spaces work
>> with CONFIG_EXTRA_FIRMWARE ?
>>
>> No matter how you spin it, the space problem is a CONFIG_EXTRA_FIRMWARE bug, not an
>> issue with the brcmfmac code.
>
> Well CONFIG_EXTRA_FIRMWARE is defined to use space as filename
> separator, so I don't really dare to call that a bug. Also brcmfmac
> seems to be only driver requiring support for spaces etc. in firmware
> file names, and this requirement seems to be quite fresh.
>
> So to me the proper way to fix this via CONFIG_EXTRA_FIRMWARE seems to
> be
>
> 1) wait some time until brcfmac's fw filenames with spaces become
> de-facto standard and distros are literally full of these.
>
> 2) after this happens, propose update of CONFIG_EXTRA_FIRMWARE to
> support spaces in filenames, arguing with status quo which coming from 1)
>
> Unfortunately I feel more like a programmer and this seems more like
> politics, so I can promise participation in the wait part only right
> now.
>
>>>> As for your T100HA example from earlier in this thread, the brcmfmac driver now
>>>> also supports getting the firmware from a special EFI nvram variable, which the
>>>> T100HA sets, so you do not need to provide a nvram file on the T100HA and things
>>>> will still work.
>>>
>>> I don't really get this. Can you please suggest how do I make the driver
>>> use something different than "brcmfmac43340-sdio.txt" or
>>> "brcmfmac43340-sdio.ASUSTeK COMPUTER INC.-T100HAN.txt" on T100HAN?
>>
>> If you leave out either file, then with a recent kernel you should see this
>> brcm_info trigger:
>>
>> brcmf_info("Using nvram EFI variable\n");
>>
>> So you should see this message when you do:
>>
>> dmesg | grep "Using nvram EFI variable"
>>
>> And the wifi on the T100HAN should just work, without needing to do any
>> manual config / provide an nvram file in anyway.
>>
>> I always strive to make hardware just work with Linux and any UEFI x86 machine
>> using brcmfmac which provides the necessary nvram EFI variable in its firmware
>> should now just work when booting say a Fedora 30 livecd.
>>
>> The EFI nvram var support has been tested successfully on the following models:
>>
>> Acer Iconia Tab8 w1-8
>> Acer One 10
>> Asus T100CHI
>> Asus T100HA
>> Asus T100TA
>> Asus T200TA
>> Lenovo Mixx 2 8
>> Lenovo Yoga2 tablet 10
>
> Thanks! Wasn't aware of this, will try in the evening.
>
> Regards,
> v.
>
>> Regards,
>>
>> Hans
>>
>>
>>
>>>>> diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/dmi.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/dmi.c
>>>>> index 7535cb0d4ac0..84571e09b465 100644
>>>>> --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/dmi.c
>>>>> +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/dmi.c
>>>>> @@ -23,6 +23,14 @@
>>>>> /* The DMI data never changes so we can use a static buf for this */
>>>>> static char dmi_board_type[128];
>>>>> +/* Array of 128 bits representing 7-bit characters allowed in DMI strings. */
>>>>> +static unsigned char brcmf_dmi_allowed_chars[] = {
>>>>> + 0x00, 0x00, 0x00, 0x00, 0xfe, 0x7f, 0xff, 0xff,
>>>>> + 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0x7f
>>>>> +};
>>>>> +
>>>>> +#define BRCMF_DMI_SAFE_CHAR '_'
>>>>> +
>>>>> struct brcmf_dmi_data {
>>>>> u32 chip;
>>>>> u32 chiprev;
>>>>> @@ -99,6 +107,15 @@ static const struct dmi_system_id dmi_platform_data[] = {
>>>>> {}
>>>>> };
>>>>> +void brcmf_dmi_sanitize(char *dst, const unsigned char *allowed, char safe)
>>>>> +{
>>>>> + while (*dst) {
>>>>> + if ((*dst < 0) || !(allowed[*dst / 8] & (1 << (*dst % 8))))
>>>>
>>>> At a first look I have no clue what this code is doing and I honestly do not feel
>>>> like figuring it out, this is clever, but IMHO not readable.
>>>
>>> Understood. The cluless part actually checks corresponding bit
>>> in allowed array, which is a bit mask describing what characters
>>> are allowed or not.
>>>
>>>> Please just write this as if (*dst < 0x21 || (*dst > foo && < bar) || etc,
>>>> so that a human can actually see in one look what the code is doing.
>>>>
>>>> You may want to wait for Arend to give his opinion before changing this though,
>>>> maybe he likes the code as is.
>>>>
>>>> Also note that that should be < 0x20 of course, since we need to preserve spaces
>>>> as is to avoid a regression.
>>>
>>> This has been already discussed, spaces are a problem. There even was an
>>> opinion that adding the code that doesn't bother with spaces and slashes
>>> might be a regression as well.
>>>
>>> Regards,
>>>
>>> v.
>>>
>>>> Regards,
>>>>
>>>> Hans
>>>>
>>>>
>>>>
>>>>
>>>>
>>>>> + *dst = safe;
>>>>> + dst++;
>>>>> + }
>>>>> +}
>>>>> +
>>>>> void brcmf_dmi_probe(struct brcmf_mp_device *settings, u32 chip, u32 chiprev)
>>>>> {
>>>>> const struct dmi_system_id *match;
>>>>> @@ -126,6 +143,9 @@ void brcmf_dmi_probe(struct brcmf_mp_device *settings, u32 chip, u32 chiprev)
>>>>> if (sys_vendor && product_name) {
>>>>> snprintf(dmi_board_type, sizeof(dmi_board_type), "%s-%s",
>>>>> sys_vendor, product_name);
>>>>> + brcmf_dmi_sanitize(dmi_board_type,
>>>>> + brcmf_dmi_allowed_chars,
>>>>> + BRCMF_DMI_SAFE_CHAR);
>>>>> settings->board_type = dmi_board_type;
>>>>> }
>>>>> }
>>>>>
>>>>
>>
^ permalink raw reply
* Re: [PATCH RFC] brcmfmac: sanitize DMI strings v2
From: Victor Bravo @ 2019-05-06 10:20 UTC (permalink / raw)
To: Hans de Goede
Cc: Arend Van Spriel, Franky Lin, Hante Meuleman, Chi-Hsien Lin,
Wright Feng, Kalle Valo, David S. Miller, linux-wireless,
brcm80211-dev-list.pdl, brcm80211-dev-list, linux-kernel
In-Reply-To: <70677dff-4336-28d5-7ab9-7ba7c3d74ebc@redhat.com>
On Mon, May 06, 2019 at 11:33:20AM +0200, Hans de Goede wrote:
> Hi,
>
> On 06-05-19 11:06, Victor Bravo wrote:
> > On Mon, May 06, 2019 at 10:13:38AM +0200, Hans de Goede wrote:
> > > Hi,
> >
> > Hi,
> >
> > > On 05-05-19 17:03, Victor Bravo wrote:
> > > > Sanitize DMI strings in brcmfmac driver to make resulting filenames
> > > > contain only safe characters. This version replaces all non-printable
> > > > characters incl. delete (0-31, 127-255), spaces and slashes with
> > > > underscores.
> > > >
> > > > This change breaks backward compatibility, but adds control over strings
> > > > passed to firmware loader and compatibility with CONFIG_EXTRA_FIRMWARE
> > > > which doesn't support spaces in filenames.
> > > >
> > > > Changes from v1: don't revert fresh commit by someone else
> > > >
> > > > Signed-off-by: Victor Bravo <1905@spmblk.com>
> > >
> > > Thank you for the patch, but I'm sorry to say this patch cannot go in as is,
> > > because it will break existing systems.
> > >
> > > If you look here:
> > >
> > > https://git.kernel.org/pub/scm/linux/kernel/git/firmware/linux-firmware.git/tree/brcm
> > >
> > > You will see a file named: "brcmfmac43430a0-sdio.ONDA-V80 PLUS.txt" there, which
> > > has a space in its name (and which works fine).
> >
> > Thanks for the updates. Spaces are actually a problem as files with spaces
> > don't work when built-in with CONFIG_EXTRA_FIRMWARE (which is used with
> > non-modular kernel containing brcmfmac driver).
> >
> > If the DMI string contains slashes, they will cause problems
> > for obvious reasons too.
>
> Right, as said I'm fine with sanitizing the names, so dropping e.g. / chars,
> but replacing space with _ will cause wifi to stop working on Onda V80 Plus devices and
> we have a clear no regressions policy in the kernel.
Please keep in mind that dropping slashes and non-printable characters
might be a regression too, as some users may already be using
intermediate directories or filenames with tabs etc. if their DMI
strings contain these. If such users exist, they will have to rename
their nvram files anyway.
On the other hand, removing just slashes and non-printable characters
(i.e. (*dst < 0x20) || (dst == '/') || (dst == 0x7f)) would allow
the sanitize function to the bit array and go with hardcoded conditions
(especially if those are final and won't need further adjustments in
set of allowed characters
> > > I'm fine with doing some sanitizing of the strings, but replacing spaces with _
> > > breaks existing use-cases (will cause a regression for them) and a space is absolutely
> > > a valid character in a filename and the firmware-loader can deal with this just fine.
> > >
> > > If the code for building firmwares into the kernel cannot deal with spaces then IMHO
> > > that code should be fixed instead. Have you looked into fixing that?
> >
> > Yes, but updating CONFIG_EXTRA_FIRMWARE to support spaces because of
> > this looks much like
>
> <snip off-topic remark>
>
> > Do you really think it's a good idea to propose that in
> > this case?
>
> I think expecting spaces in filenames to just work is quite reasonable, after all
> its been a long time since we've left DOS-es 8.3 filename limitations.
>
> Have you actually looked at how hard it would be to make filenames with spaces work
> with CONFIG_EXTRA_FIRMWARE ?
>
> No matter how you spin it, the space problem is a CONFIG_EXTRA_FIRMWARE bug, not an
> issue with the brcmfmac code.
Well CONFIG_EXTRA_FIRMWARE is defined to use space as filename
separator, so I don't really dare to call that a bug. Also brcmfmac
seems to be only driver requiring support for spaces etc. in firmware
file names, and this requirement seems to be quite fresh.
So to me the proper way to fix this via CONFIG_EXTRA_FIRMWARE seems to
be
1) wait some time until brcfmac's fw filenames with spaces become
de-facto standard and distros are literally full of these.
2) after this happens, propose update of CONFIG_EXTRA_FIRMWARE to
support spaces in filenames, arguing with status quo which coming from 1)
Unfortunately I feel more like a programmer and this seems more like
politics, so I can promise participation in the wait part only right
now.
> > > As for your T100HA example from earlier in this thread, the brcmfmac driver now
> > > also supports getting the firmware from a special EFI nvram variable, which the
> > > T100HA sets, so you do not need to provide a nvram file on the T100HA and things
> > > will still work.
> >
> > I don't really get this. Can you please suggest how do I make the driver
> > use something different than "brcmfmac43340-sdio.txt" or
> > "brcmfmac43340-sdio.ASUSTeK COMPUTER INC.-T100HAN.txt" on T100HAN?
>
> If you leave out either file, then with a recent kernel you should see this
> brcm_info trigger:
>
> brcmf_info("Using nvram EFI variable\n");
>
> So you should see this message when you do:
>
> dmesg | grep "Using nvram EFI variable"
>
> And the wifi on the T100HAN should just work, without needing to do any
> manual config / provide an nvram file in anyway.
>
> I always strive to make hardware just work with Linux and any UEFI x86 machine
> using brcmfmac which provides the necessary nvram EFI variable in its firmware
> should now just work when booting say a Fedora 30 livecd.
>
> The EFI nvram var support has been tested successfully on the following models:
>
> Acer Iconia Tab8 w1-8
> Acer One 10
> Asus T100CHI
> Asus T100HA
> Asus T100TA
> Asus T200TA
> Lenovo Mixx 2 8
> Lenovo Yoga2 tablet 10
Thanks! Wasn't aware of this, will try in the evening.
Regards,
v.
> Regards,
>
> Hans
>
>
>
> > > > diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/dmi.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/dmi.c
> > > > index 7535cb0d4ac0..84571e09b465 100644
> > > > --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/dmi.c
> > > > +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/dmi.c
> > > > @@ -23,6 +23,14 @@
> > > > /* The DMI data never changes so we can use a static buf for this */
> > > > static char dmi_board_type[128];
> > > > +/* Array of 128 bits representing 7-bit characters allowed in DMI strings. */
> > > > +static unsigned char brcmf_dmi_allowed_chars[] = {
> > > > + 0x00, 0x00, 0x00, 0x00, 0xfe, 0x7f, 0xff, 0xff,
> > > > + 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0x7f
> > > > +};
> > > > +
> > > > +#define BRCMF_DMI_SAFE_CHAR '_'
> > > > +
> > > > struct brcmf_dmi_data {
> > > > u32 chip;
> > > > u32 chiprev;
> > > > @@ -99,6 +107,15 @@ static const struct dmi_system_id dmi_platform_data[] = {
> > > > {}
> > > > };
> > > > +void brcmf_dmi_sanitize(char *dst, const unsigned char *allowed, char safe)
> > > > +{
> > > > + while (*dst) {
> > > > + if ((*dst < 0) || !(allowed[*dst / 8] & (1 << (*dst % 8))))
> > >
> > > At a first look I have no clue what this code is doing and I honestly do not feel
> > > like figuring it out, this is clever, but IMHO not readable.
> >
> > Understood. The cluless part actually checks corresponding bit
> > in allowed array, which is a bit mask describing what characters
> > are allowed or not.
> >
> > > Please just write this as if (*dst < 0x21 || (*dst > foo && < bar) || etc,
> > > so that a human can actually see in one look what the code is doing.
> > >
> > > You may want to wait for Arend to give his opinion before changing this though,
> > > maybe he likes the code as is.
> > >
> > > Also note that that should be < 0x20 of course, since we need to preserve spaces
> > > as is to avoid a regression.
> >
> > This has been already discussed, spaces are a problem. There even was an
> > opinion that adding the code that doesn't bother with spaces and slashes
> > might be a regression as well.
> >
> > Regards,
> >
> > v.
> >
> > > Regards,
> > >
> > > Hans
> > >
> > >
> > >
> > >
> > >
> > > > + *dst = safe;
> > > > + dst++;
> > > > + }
> > > > +}
> > > > +
> > > > void brcmf_dmi_probe(struct brcmf_mp_device *settings, u32 chip, u32 chiprev)
> > > > {
> > > > const struct dmi_system_id *match;
> > > > @@ -126,6 +143,9 @@ void brcmf_dmi_probe(struct brcmf_mp_device *settings, u32 chip, u32 chiprev)
> > > > if (sys_vendor && product_name) {
> > > > snprintf(dmi_board_type, sizeof(dmi_board_type), "%s-%s",
> > > > sys_vendor, product_name);
> > > > + brcmf_dmi_sanitize(dmi_board_type,
> > > > + brcmf_dmi_allowed_chars,
> > > > + BRCMF_DMI_SAFE_CHAR);
> > > > settings->board_type = dmi_board_type;
> > > > }
> > > > }
> > > >
> > >
>
^ permalink raw reply
* Re: [PATCH RFC] brcmfmac: sanitize DMI strings v2
From: Hans de Goede @ 2019-05-06 9:33 UTC (permalink / raw)
To: Victor Bravo
Cc: Arend Van Spriel, Franky Lin, Hante Meuleman, Chi-Hsien Lin,
Wright Feng, Kalle Valo, David S. Miller, linux-wireless,
brcm80211-dev-list.pdl, brcm80211-dev-list, linux-kernel
In-Reply-To: <20190506090609.msudhncj7e5vdtzw@localhost>
Hi,
On 06-05-19 11:06, Victor Bravo wrote:
> On Mon, May 06, 2019 at 10:13:38AM +0200, Hans de Goede wrote:
>> Hi,
>
> Hi,
>
>> On 05-05-19 17:03, Victor Bravo wrote:
>>> Sanitize DMI strings in brcmfmac driver to make resulting filenames
>>> contain only safe characters. This version replaces all non-printable
>>> characters incl. delete (0-31, 127-255), spaces and slashes with
>>> underscores.
>>>
>>> This change breaks backward compatibility, but adds control over strings
>>> passed to firmware loader and compatibility with CONFIG_EXTRA_FIRMWARE
>>> which doesn't support spaces in filenames.
>>>
>>> Changes from v1: don't revert fresh commit by someone else
>>>
>>> Signed-off-by: Victor Bravo <1905@spmblk.com>
>>
>> Thank you for the patch, but I'm sorry to say this patch cannot go in as is,
>> because it will break existing systems.
>>
>> If you look here:
>>
>> https://git.kernel.org/pub/scm/linux/kernel/git/firmware/linux-firmware.git/tree/brcm
>>
>> You will see a file named: "brcmfmac43430a0-sdio.ONDA-V80 PLUS.txt" there, which
>> has a space in its name (and which works fine).
>
> Thanks for the updates. Spaces are actually a problem as files with spaces
> don't work when built-in with CONFIG_EXTRA_FIRMWARE (which is used with
> non-modular kernel containing brcmfmac driver).
>
> If the DMI string contains slashes, they will cause problems
> for obvious reasons too.
Right, as said I'm fine with sanitizing the names, so dropping e.g. / chars,
but replacing space with _ will cause wifi to stop working on Onda V80 Plus devices and
we have a clear no regressions policy in the kernel.
>> I'm fine with doing some sanitizing of the strings, but replacing spaces with _
>> breaks existing use-cases (will cause a regression for them) and a space is absolutely
>> a valid character in a filename and the firmware-loader can deal with this just fine.
>>
>> If the code for building firmwares into the kernel cannot deal with spaces then IMHO
>> that code should be fixed instead. Have you looked into fixing that?
>
> Yes, but updating CONFIG_EXTRA_FIRMWARE to support spaces because of
> this looks much like
<snip off-topic remark>
> Do you really think it's a good idea to propose that in
> this case?
I think expecting spaces in filenames to just work is quite reasonable, after all
its been a long time since we've left DOS-es 8.3 filename limitations.
Have you actually looked at how hard it would be to make filenames with spaces work
with CONFIG_EXTRA_FIRMWARE ?
No matter how you spin it, the space problem is a CONFIG_EXTRA_FIRMWARE bug, not an
issue with the brcmfmac code.
>> As for your T100HA example from earlier in this thread, the brcmfmac driver now
>> also supports getting the firmware from a special EFI nvram variable, which the
>> T100HA sets, so you do not need to provide a nvram file on the T100HA and things
>> will still work.
>
> I don't really get this. Can you please suggest how do I make the driver
> use something different than "brcmfmac43340-sdio.txt" or
> "brcmfmac43340-sdio.ASUSTeK COMPUTER INC.-T100HAN.txt" on T100HAN?
If you leave out either file, then with a recent kernel you should see this
brcm_info trigger:
brcmf_info("Using nvram EFI variable\n");
So you should see this message when you do:
dmesg | grep "Using nvram EFI variable"
And the wifi on the T100HAN should just work, without needing to do any
manual config / provide an nvram file in anyway.
I always strive to make hardware just work with Linux and any UEFI x86 machine
using brcmfmac which provides the necessary nvram EFI variable in its firmware
should now just work when booting say a Fedora 30 livecd.
The EFI nvram var support has been tested successfully on the following models:
Acer Iconia Tab8 w1-8
Acer One 10
Asus T100CHI
Asus T100HA
Asus T100TA
Asus T200TA
Lenovo Mixx 2 8
Lenovo Yoga2 tablet 10
Regards,
Hans
>>> diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/dmi.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/dmi.c
>>> index 7535cb0d4ac0..84571e09b465 100644
>>> --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/dmi.c
>>> +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/dmi.c
>>> @@ -23,6 +23,14 @@
>>> /* The DMI data never changes so we can use a static buf for this */
>>> static char dmi_board_type[128];
>>> +/* Array of 128 bits representing 7-bit characters allowed in DMI strings. */
>>> +static unsigned char brcmf_dmi_allowed_chars[] = {
>>> + 0x00, 0x00, 0x00, 0x00, 0xfe, 0x7f, 0xff, 0xff,
>>> + 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0x7f
>>> +};
>>> +
>>> +#define BRCMF_DMI_SAFE_CHAR '_'
>>> +
>>> struct brcmf_dmi_data {
>>> u32 chip;
>>> u32 chiprev;
>>> @@ -99,6 +107,15 @@ static const struct dmi_system_id dmi_platform_data[] = {
>>> {}
>>> };
>>> +void brcmf_dmi_sanitize(char *dst, const unsigned char *allowed, char safe)
>>> +{
>>> + while (*dst) {
>>> + if ((*dst < 0) || !(allowed[*dst / 8] & (1 << (*dst % 8))))
>>
>> At a first look I have no clue what this code is doing and I honestly do not feel
>> like figuring it out, this is clever, but IMHO not readable.
>
> Understood. The cluless part actually checks corresponding bit
> in allowed array, which is a bit mask describing what characters
> are allowed or not.
>
>> Please just write this as if (*dst < 0x21 || (*dst > foo && < bar) || etc,
>> so that a human can actually see in one look what the code is doing.
>>
>> You may want to wait for Arend to give his opinion before changing this though,
>> maybe he likes the code as is.
>>
>> Also note that that should be < 0x20 of course, since we need to preserve spaces
>> as is to avoid a regression.
>
> This has been already discussed, spaces are a problem. There even was an
> opinion that adding the code that doesn't bother with spaces and slashes
> might be a regression as well.
>
> Regards,
>
> v.
>
>> Regards,
>>
>> Hans
>>
>>
>>
>>
>>
>>> + *dst = safe;
>>> + dst++;
>>> + }
>>> +}
>>> +
>>> void brcmf_dmi_probe(struct brcmf_mp_device *settings, u32 chip, u32 chiprev)
>>> {
>>> const struct dmi_system_id *match;
>>> @@ -126,6 +143,9 @@ void brcmf_dmi_probe(struct brcmf_mp_device *settings, u32 chip, u32 chiprev)
>>> if (sys_vendor && product_name) {
>>> snprintf(dmi_board_type, sizeof(dmi_board_type), "%s-%s",
>>> sys_vendor, product_name);
>>> + brcmf_dmi_sanitize(dmi_board_type,
>>> + brcmf_dmi_allowed_chars,
>>> + BRCMF_DMI_SAFE_CHAR);
>>> settings->board_type = dmi_board_type;
>>> }
>>> }
>>>
>>
^ permalink raw reply
* Re: [PATCH RFC] brcmfmac: sanitize DMI strings v2
From: Victor Bravo @ 2019-05-06 9:14 UTC (permalink / raw)
To: Kalle Valo
Cc: Hans de Goede, Arend Van Spriel, Franky Lin, Hante Meuleman,
Chi-Hsien Lin, Wright Feng, David S. Miller, linux-wireless,
brcm80211-dev-list.pdl, brcm80211-dev-list, linux-kernel
In-Reply-To: <87o94gug81.fsf@codeaurora.org>
On Mon, May 06, 2019 at 11:42:06AM +0300, Kalle Valo wrote:
> Hans de Goede <hdegoede@redhat.com> writes:
>
> >> @@ -99,6 +107,15 @@ static const struct dmi_system_id dmi_platform_data[] = {
> >> {}
> >> };
> >> +void brcmf_dmi_sanitize(char *dst, const unsigned char *allowed,
> >> char safe)
> >> +{
> >> + while (*dst) {
> >> + if ((*dst < 0) || !(allowed[*dst / 8] & (1 << (*dst % 8))))
> >
> > At a first look I have no clue what this code is doing and I honestly do not feel
> > like figuring it out, this is clever, but IMHO not readable.
> >
> > Please just write this as if (*dst < 0x21 || (*dst > foo && < bar) || etc,
> > so that a human can actually see in one look what the code is doing.
>
> Is there an existing function for sanitising filenames so that we don't
> need to reinvent the wheel, maybe something like isalnum()?
I would definitely prefer to use existing function, but I didn't find
any suitable one. Suggestions are welcome.
As for implementation details, the one I posted was optimized for both
speed and size, and at least in my opinion this bit array driven
parametric implementation is exactly what is needed here (using a string
of allowed characters with strchr-style lookups would bring much worse
complexity, and checking the characters using series of hardcoded if
conditions could quickly grow to more than those 16 bytes used by the
array).
Regards,
v.
^ permalink raw reply
* Re: [PATCH RFC] brcmfmac: sanitize DMI strings v2
From: Victor Bravo @ 2019-05-06 9:06 UTC (permalink / raw)
To: Hans de Goede
Cc: Arend Van Spriel, Franky Lin, Hante Meuleman, Chi-Hsien Lin,
Wright Feng, Kalle Valo, David S. Miller, linux-wireless,
brcm80211-dev-list.pdl, brcm80211-dev-list, linux-kernel
In-Reply-To: <0f75a3d4-94af-5503-94c3-e8af2364448d@redhat.com>
On Mon, May 06, 2019 at 10:13:38AM +0200, Hans de Goede wrote:
> Hi,
Hi,
> On 05-05-19 17:03, Victor Bravo wrote:
> > Sanitize DMI strings in brcmfmac driver to make resulting filenames
> > contain only safe characters. This version replaces all non-printable
> > characters incl. delete (0-31, 127-255), spaces and slashes with
> > underscores.
> >
> > This change breaks backward compatibility, but adds control over strings
> > passed to firmware loader and compatibility with CONFIG_EXTRA_FIRMWARE
> > which doesn't support spaces in filenames.
> >
> > Changes from v1: don't revert fresh commit by someone else
> >
> > Signed-off-by: Victor Bravo <1905@spmblk.com>
>
> Thank you for the patch, but I'm sorry to say this patch cannot go in as is,
> because it will break existing systems.
>
> If you look here:
>
> https://git.kernel.org/pub/scm/linux/kernel/git/firmware/linux-firmware.git/tree/brcm
>
> You will see a file named: "brcmfmac43430a0-sdio.ONDA-V80 PLUS.txt" there, which
> has a space in its name (and which works fine).
Thanks for the updates. Spaces are actually a problem as files with spaces
don't work when built-in with CONFIG_EXTRA_FIRMWARE (which is used with
non-modular kernel containing brcmfmac driver).
If the DMI string contains slashes, they will cause problems
for obvious reasons too.
> I'm fine with doing some sanitizing of the strings, but replacing spaces with _
> breaks existing use-cases (will cause a regression for them) and a space is absolutely
> a valid character in a filename and the firmware-loader can deal with this just fine.
>
> If the code for building firmwares into the kernel cannot deal with spaces then IMHO
> that code should be fixed instead. Have you looked into fixing that?
Yes, but updating CONFIG_EXTRA_FIRMWARE to support spaces because of
this looks much like fixing systemd-caused unitialized urandom reads on
kernel side. Do you really think it's a good idea to propose that in
this case?
> As for your T100HA example from earlier in this thread, the brcmfmac driver now
> also supports getting the firmware from a special EFI nvram variable, which the
> T100HA sets, so you do not need to provide a nvram file on the T100HA and things
> will still work.
I don't really get this. Can you please suggest how do I make the driver
use something different than "brcmfmac43340-sdio.txt" or
"brcmfmac43340-sdio.ASUSTeK COMPUTER INC.-T100HAN.txt" on T100HAN?
> > diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/dmi.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/dmi.c
> > index 7535cb0d4ac0..84571e09b465 100644
> > --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/dmi.c
> > +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/dmi.c
> > @@ -23,6 +23,14 @@
> > /* The DMI data never changes so we can use a static buf for this */
> > static char dmi_board_type[128];
> > +/* Array of 128 bits representing 7-bit characters allowed in DMI strings. */
> > +static unsigned char brcmf_dmi_allowed_chars[] = {
> > + 0x00, 0x00, 0x00, 0x00, 0xfe, 0x7f, 0xff, 0xff,
> > + 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0x7f
> > +};
> > +
> > +#define BRCMF_DMI_SAFE_CHAR '_'
> > +
> > struct brcmf_dmi_data {
> > u32 chip;
> > u32 chiprev;
> > @@ -99,6 +107,15 @@ static const struct dmi_system_id dmi_platform_data[] = {
> > {}
> > };
> > +void brcmf_dmi_sanitize(char *dst, const unsigned char *allowed, char safe)
> > +{
> > + while (*dst) {
> > + if ((*dst < 0) || !(allowed[*dst / 8] & (1 << (*dst % 8))))
>
> At a first look I have no clue what this code is doing and I honestly do not feel
> like figuring it out, this is clever, but IMHO not readable.
Understood. The cluless part actually checks corresponding bit
in allowed array, which is a bit mask describing what characters
are allowed or not.
> Please just write this as if (*dst < 0x21 || (*dst > foo && < bar) || etc,
> so that a human can actually see in one look what the code is doing.
>
> You may want to wait for Arend to give his opinion before changing this though,
> maybe he likes the code as is.
>
> Also note that that should be < 0x20 of course, since we need to preserve spaces
> as is to avoid a regression.
This has been already discussed, spaces are a problem. There even was an
opinion that adding the code that doesn't bother with spaces and slashes
might be a regression as well.
Regards,
v.
> Regards,
>
> Hans
>
>
>
>
>
> > + *dst = safe;
> > + dst++;
> > + }
> > +}
> > +
> > void brcmf_dmi_probe(struct brcmf_mp_device *settings, u32 chip, u32 chiprev)
> > {
> > const struct dmi_system_id *match;
> > @@ -126,6 +143,9 @@ void brcmf_dmi_probe(struct brcmf_mp_device *settings, u32 chip, u32 chiprev)
> > if (sys_vendor && product_name) {
> > snprintf(dmi_board_type, sizeof(dmi_board_type), "%s-%s",
> > sys_vendor, product_name);
> > + brcmf_dmi_sanitize(dmi_board_type,
> > + brcmf_dmi_allowed_chars,
> > + BRCMF_DMI_SAFE_CHAR);
> > settings->board_type = dmi_board_type;
> > }
> > }
> >
>
^ permalink raw reply
* RE: [PATCH v2 4/5] rtw88: fix unassigned rssi_level in rtw_sta_info
From: Tony Chuang @ 2019-05-06 8:54 UTC (permalink / raw)
To: Kalle Valo; +Cc: linux-wireless@vger.kernel.org
In-Reply-To: <874l68vuhi.fsf@purkki.adurom.net>
> -----Original Message-----
> From: Kalle Valo [mailto:kvalo@codeaurora.org]
> Sent: Monday, May 06, 2019 4:49 PM
> To: Tony Chuang
> Cc: linux-wireless@vger.kernel.org
> Subject: Re: [PATCH v2 4/5] rtw88: fix unassigned rssi_level in rtw_sta_info
>
> <yhchuang@realtek.com> writes:
>
> > From: Yan-Hsuan Chuang <yhchuang@realtek.com>
> >
> > The new rssi_level should be stored in si, otherwise the rssi_level will
> > never be updated and get a wrong RA mask, which is calculated by the
> > rssi level
> >
> > Signed-off-by: Yan-Hsuan Chuang <yhchuang@realtek.com>
>
> Stanislaw suggested that this should go to 5.2. So what breaks from
> user's point of view if this is not applied?
>
If the rssi level remains unchanged, then we could choose wrong ra_mask.
And some *bad rates* we be chosen by firmware.
The most hurtful scene would be *noisy environment* such as office, or public.
The latency would be high and overall throughput would be only half.
(This was tested, such as 4x Mbps -> 1x Mbps)
Yan-Hsuan
^ permalink raw reply
* Re: [PATCH v2 4/5] rtw88: fix unassigned rssi_level in rtw_sta_info
From: Kalle Valo @ 2019-05-06 8:48 UTC (permalink / raw)
To: yhchuang; +Cc: linux-wireless
In-Reply-To: <1556884415-23474-5-git-send-email-yhchuang@realtek.com>
<yhchuang@realtek.com> writes:
> From: Yan-Hsuan Chuang <yhchuang@realtek.com>
>
> The new rssi_level should be stored in si, otherwise the rssi_level will
> never be updated and get a wrong RA mask, which is calculated by the
> rssi level
>
> Signed-off-by: Yan-Hsuan Chuang <yhchuang@realtek.com>
Stanislaw suggested that this should go to 5.2. So what breaks from
user's point of view if this is not applied?
--
Kalle Valo
^ permalink raw reply
* Re: [PATCH RFC] brcmfmac: sanitize DMI strings v2
From: Kalle Valo @ 2019-05-06 8:44 UTC (permalink / raw)
To: Victor Bravo
Cc: Arend Van Spriel, Franky Lin, Hante Meuleman, Chi-Hsien Lin,
Wright Feng, David S. Miller, linux-wireless,
brcm80211-dev-list.pdl, brcm80211-dev-list, linux-kernel,
Hans de Goede
In-Reply-To: <20190505150355.3fbng4ny34x255vk@localhost>
Victor Bravo <1905@spmblk.com> writes:
> Sanitize DMI strings in brcmfmac driver to make resulting filenames
> contain only safe characters. This version replaces all non-printable
> characters incl. delete (0-31, 127-255), spaces and slashes with
> underscores.
>
> This change breaks backward compatibility, but adds control over strings
> passed to firmware loader and compatibility with CONFIG_EXTRA_FIRMWARE
> which doesn't support spaces in filenames.
>
> Changes from v1: don't revert fresh commit by someone else
>
> Signed-off-by: Victor Bravo <1905@spmblk.com>
The version should be in brackets "[PATCH RFC v2]" and the change log
after "---" line:
https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches
--
Kalle Valo
^ permalink raw reply
* Re: [PATCH 5.1] rtw88: fix subscript above array bounds compiler warning
From: Stanislaw Gruszka @ 2019-05-06 8:42 UTC (permalink / raw)
To: linux-wireless, Kalle Valo; +Cc: Yan-Hsuan Chuang
In-Reply-To: <20190506073917.10106-1-sgruszka@redhat.com>
This is for 5.2 and v2 obviously.
Stanislaw
On Mon, May 06, 2019 at 09:39:17AM +0200, Stanislaw Gruszka wrote:
> My compiler complains about:
>
> drivers/net/wireless/realtek/rtw88/phy.c: In function ???rtw_phy_rf_power_2_rssi???:
> drivers/net/wireless/realtek/rtw88/phy.c:430:26: warning: array subscript is above array bounds [-Warray-bounds]
> linear = db_invert_table[i][j];
>
> According to comment power_db should be in range 1 ~ 96 .
> To fix add check for boundaries before access the array.
>
> Signed-off-by: Stanislaw Gruszka <sgruszka@redhat.com>
> ---
> RFC -> v1
> - add check before accessing the array insted of
> rtw_phy_power_2_db() change.
> v1 -> v2:
> - return 1 for power_db < 1
>
> drivers/net/wireless/realtek/rtw88/phy.c | 5 +++++
> 1 file changed, 5 insertions(+)
>
> diff --git a/drivers/net/wireless/realtek/rtw88/phy.c b/drivers/net/wireless/realtek/rtw88/phy.c
> index 4381b360b5b5..9ca52a4d025a 100644
> --- a/drivers/net/wireless/realtek/rtw88/phy.c
> +++ b/drivers/net/wireless/realtek/rtw88/phy.c
> @@ -423,6 +423,11 @@ static u64 rtw_phy_db_2_linear(u8 power_db)
> u8 i, j;
> u64 linear;
>
> + if (power_db > 96)
> + power_db = 96;
> + else if (power_db < 1)
> + return 1;
> +
> /* 1dB ~ 96dB */
> i = (power_db - 1) >> 3;
> j = (power_db - 1) - (i << 3);
> --
> 2.20.1
>
^ permalink raw reply
* Re: [PATCH RFC] brcmfmac: sanitize DMI strings v2
From: Kalle Valo @ 2019-05-06 8:42 UTC (permalink / raw)
To: Hans de Goede
Cc: Victor Bravo, Arend Van Spriel, Franky Lin, Hante Meuleman,
Chi-Hsien Lin, Wright Feng, David S. Miller, linux-wireless,
brcm80211-dev-list.pdl, brcm80211-dev-list, linux-kernel
In-Reply-To: <0f75a3d4-94af-5503-94c3-e8af2364448d@redhat.com>
Hans de Goede <hdegoede@redhat.com> writes:
>> @@ -99,6 +107,15 @@ static const struct dmi_system_id dmi_platform_data[] = {
>> {}
>> };
>> +void brcmf_dmi_sanitize(char *dst, const unsigned char *allowed,
>> char safe)
>> +{
>> + while (*dst) {
>> + if ((*dst < 0) || !(allowed[*dst / 8] & (1 << (*dst % 8))))
>
> At a first look I have no clue what this code is doing and I honestly do not feel
> like figuring it out, this is clever, but IMHO not readable.
>
> Please just write this as if (*dst < 0x21 || (*dst > foo && < bar) || etc,
> so that a human can actually see in one look what the code is doing.
Is there an existing function for sanitising filenames so that we don't
need to reinvent the wheel, maybe something like isalnum()?
--
Kalle Valo
^ permalink raw reply
* Re: [PATCH v2 0/5] rtw88: minor fixes from suggestions during review
From: Stanislaw Gruszka @ 2019-05-06 8:40 UTC (permalink / raw)
To: Kalle Valo; +Cc: yhchuang, linux-wireless
In-Reply-To: <87ftpvkal1.fsf@kamboji.qca.qualcomm.com>
On Fri, May 03, 2019 at 03:04:58PM +0300, Kalle Valo wrote:
> <yhchuang@realtek.com> writes:
>
> > From: Yan-Hsuan Chuang <yhchuang@realtek.com>
> >
> > The series fix some small problems for rtw88, most of the suggestions
> > are from the review process.
> >
> >
> > v1 -> v2
> >
> > - modify description for LPS, ", turn off" -> ", to turn off"
> > - drop patch "rtw88: mac: remove dangerous while (1)",
> > should re-write the power sequence parsing code to make sense of avoiding
> > infinite loop
> > - unify Makefile license to Dual GPL/BSD
> >
> >
> > Yan-Hsuan Chuang (5):
> > rtw88: add license for Makefile
> > rtw88: pci: use ieee80211_ac_numbers instead of 0-3
> > rtw88: pci: check if queue mapping exceeds size of ac_to_hwq
> > rtw88: fix unassigned rssi_level in rtw_sta_info
> > rtw88: more descriptions about LPS
>
> I was just in the next few minutes about to tag the last -next pull for
> 5.2. I'll apply patch 1 now so that we have consistent licenses for 5.2
> but the rest have to wait for 5.3.
I think '[PATCH v2 4/5] rtw88: fix unassigned rssi_level in rtw_sta_inf'
should go to 5.2 .
Stanislaw
^ permalink raw reply
* Re: pull-request: wireless-drivers 2019-04-30
From: Kalle Valo @ 2019-05-06 8:29 UTC (permalink / raw)
To: David Miller; +Cc: linux-wireless, netdev, linux-kernel
In-Reply-To: <20190505.005130.1921658214241614481.davem@davemloft.net>
David Miller <davem@davemloft.net> writes:
> From: Kalle Valo <kvalo@codeaurora.org>
> Date: Tue, 30 Apr 2019 19:55:45 +0300
>
>> David Miller <davem@davemloft.net> writes:
>>
>>> Thanks for the conflict resolution information, it is very helpful.
>>>
>>> However, can you put it into the merge commit text next time as well?
>>> I cut and pasted it in there when I pulled this stuff in.
>>
>> A good idea, I'll do that. Just to be sure, do you mean that I should
>> add it only with conflicts between net and net-next (like in this case)?
>> Or should I add it everytime I see a conflict, for example between
>> wireless-drivers-next and net-next? I hope my question is not too
>> confusing...
>
> When there is a major conflict for me to resolve when I pull in your
> pull reqeust, please place the conflict resolution help text into the
> merge commit message.
>
> I hope this is now clear :-)
Got it now, thanks!
--
Kalle Valo
^ permalink raw reply
* RE: [PATCH 5.1] rtw88: fix subscript above array bounds compiler warning
From: Tony Chuang @ 2019-05-06 8:29 UTC (permalink / raw)
To: Stanislaw Gruszka, linux-wireless@vger.kernel.org
In-Reply-To: <20190506073917.10106-1-sgruszka@redhat.com>
> Subject: [PATCH 5.1] rtw88: fix subscript above array bounds compiler warning
>
> My compiler complains about:
>
> drivers/net/wireless/realtek/rtw88/phy.c: In function
> ‘rtw_phy_rf_power_2_rssi’:
> drivers/net/wireless/realtek/rtw88/phy.c:430:26: warning: array subscript is
> above array bounds [-Warray-bounds]
> linear = db_invert_table[i][j];
>
> According to comment power_db should be in range 1 ~ 96 .
> To fix add check for boundaries before access the array.
>
> Signed-off-by: Stanislaw Gruszka <sgruszka@redhat.com>
> ---
> RFC -> v1
> - add check before accessing the array insted of
> rtw_phy_power_2_db() change.
> v1 -> v2:
> - return 1 for power_db < 1
>
> drivers/net/wireless/realtek/rtw88/phy.c | 5 +++++
> 1 file changed, 5 insertions(+)
>
> diff --git a/drivers/net/wireless/realtek/rtw88/phy.c
> b/drivers/net/wireless/realtek/rtw88/phy.c
> index 4381b360b5b5..9ca52a4d025a 100644
> --- a/drivers/net/wireless/realtek/rtw88/phy.c
> +++ b/drivers/net/wireless/realtek/rtw88/phy.c
> @@ -423,6 +423,11 @@ static u64 rtw_phy_db_2_linear(u8 power_db)
> u8 i, j;
> u64 linear;
>
> + if (power_db > 96)
> + power_db = 96;
> + else if (power_db < 1)
> + return 1;
> +
> /* 1dB ~ 96dB */
> i = (power_db - 1) >> 3;
> j = (power_db - 1) - (i << 3);
> --
Thanks. For this patch.
Acked-by: Yan-Hsuan Chuang <yhchuang@realtek.com>
Yan-Hsuan
^ 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