* Re: [PATCH 00/16] wilc1000: move out of staging
From: Ajay.Kathat @ 2019-05-23 16:45 UTC (permalink / raw)
To: kvalo
Cc: linux-wireless, johannes, gregkh, Adham.Abozaeid,
Venkateswara.Kaja, Nicolas.Ferre, Claudiu.Beznea
In-Reply-To: <87sgt5cl8q.fsf@kamboji.qca.qualcomm.com>
Hi Kalle,
On 5/23/2019 7:43 PM, Kalle Valo wrote:
> <Ajay.Kathat@microchip.com> writes:
>
>> Hi Kalle,
>>
>> On 3/21/2019 6:24 PM, Kalle Valo wrote:
>>>
>>> <Ajay.Kathat@microchip.com> writes:
>>>
>>>> Hi Kalle/Johannes,
>>>>
>>>> On 2/9/2019 12:42 PM, Ajay Kathat - I15481 wrote:
>>>>> From: Ajay Singh <ajay.kathat@microchip.com>
>>>>>
>>>>> This patch series is to review and move wilc1000 driver out of staging.
>>>>> Implemented the initial received review comments[1] and submitting the
>>>>> driver again. During this cleanup deleted around 3.3k lines of code.
>>>>>
>>>>> Below are the major items fixed in recent cleanup:
>>>>> - remove use of shadow buffer to keep scan result.
>>>>> - remove internal messaging flow to handle cfg80211_ops.
>>>>> - make use of cfg80211 provided API.
>>>>> - use structure for packing firmware commands.
>>>>> - make use of kernel provided API and macros.
>>>>> - remove unnecessary logs messages.
>>>>>
>>>>> Pending action item:
>>>>> - dynamically add/remove p2p interface.
>>>>>
>>>>> This item will take some time, we are planning to take it up after
>>>>> mainline.
>>>>>
>>>>> We hope it can be move out staging in v5.1.
>>>>> Please review and confirm if it is good to move out.
>>>>
>>>> Do you have any update for this series. Please provide your inputs for
>>>> next step.
>>>
>>> rtw88 is taking priority in the new drivers "queue" so I doubt I can
>>> take a look at this in the next few weeks.
>>>
>>
>> Is there any update for wilc1000 driver review. Please let me know your
>> inputs.
>
> So is the driver in good shape now? I really do not want to use a lot of
> time reviewing it just to find out that there's a lot of work to do. For
> example, has someone else reviewed it?
>
The first series for complete driver review was submitted earlier [1].
It was reviewed by Johannes and he looked into driver integration with
the cfg80211 stack part. We worked on the review comments and submitted
the changes to staging.
We need further review to identify if there is any blocker to move to
mainline.
[1]. https://www.spinics.net/lists/linux-wireless/msg177878.html
Regards,
Ajay
^ permalink raw reply
* Re: [PATCH] ath10k: add missing error handling
From: Brian Norris @ 2019-05-23 16:42 UTC (permalink / raw)
To: Claire Chang; +Cc: Kalle Valo, ath10k, linux-wireless, Wen Gong
In-Reply-To: <20190523071534.254611-1-tientzu@chromium.org>
On Thu, May 23, 2019 at 12:15 AM Claire Chang <tientzu@chromium.org> wrote:
> --- a/drivers/net/wireless/ath/ath10k/sdio.c
> +++ b/drivers/net/wireless/ath/ath10k/sdio.c
> @@ -607,6 +607,10 @@ static int ath10k_sdio_mbox_rx_alloc(struct ath10k *ar,
> full_len,
> last_in_bundle,
> last_in_bundle);
> + if (ret) {
IIUC, you have basically the same failure case a few lines up, where
ath10k_sdio_mbox_alloc_pkt_bundle() may fail. Do the same there?
This (including the error label to which it's jumping) looks fine to me though:
Reviewed-by: Brian Norris <briannorris@chromium.org>
> + ath10k_warn(ar, "alloc_rx_pkt error %d\n", ret);
> + goto err;
> + }
> }
>
> ar_sdio->n_rx_pkts = i;
^ permalink raw reply
* [PATCH v2 5.2] rsi: Properly initialize data in rsi_sdio_ta_reset
From: Nathan Chancellor @ 2019-05-23 15:30 UTC (permalink / raw)
To: Amitkumar Karwar, Siva Rebbagondla, Kalle Valo
Cc: linux-wireless, netdev, linux-kernel, Arnd Bergmann,
Nick Desaulniers, clang-built-linux, Nathan Chancellor
In-Reply-To: <20190502151548.11143-1-natechancellor@gmail.com>
When building with -Wuninitialized, Clang warns:
drivers/net/wireless/rsi/rsi_91x_sdio.c:940:43: warning: variable 'data'
is uninitialized when used here [-Wuninitialized]
put_unaligned_le32(TA_HOLD_THREAD_VALUE, data);
^~~~
drivers/net/wireless/rsi/rsi_91x_sdio.c:930:10: note: initialize the
variable 'data' to silence this warning
u8 *data;
^
= NULL
1 warning generated.
Using Clang's suggestion of initializing data to NULL wouldn't work out
because data will be dereferenced by put_unaligned_le32. Use kzalloc to
properly initialize data, which matches a couple of other places in this
driver.
Fixes: e5a1ecc97e5f ("rsi: add firmware loading for 9116 device")
Link: https://github.com/ClangBuiltLinux/linux/issues/464
Signed-off-by: Nathan Chancellor <natechancellor@gmail.com>
---
v1 -> v2:
* Use RSI_9116_REG_SIZE instead of sizeof(u32) for kzalloc thanks to
review from Arnd.
drivers/net/wireless/rsi/rsi_91x_sdio.c | 21 ++++++++++++++-------
1 file changed, 14 insertions(+), 7 deletions(-)
diff --git a/drivers/net/wireless/rsi/rsi_91x_sdio.c b/drivers/net/wireless/rsi/rsi_91x_sdio.c
index f9c67ed473d1..b42cd50b837e 100644
--- a/drivers/net/wireless/rsi/rsi_91x_sdio.c
+++ b/drivers/net/wireless/rsi/rsi_91x_sdio.c
@@ -929,11 +929,15 @@ static int rsi_sdio_ta_reset(struct rsi_hw *adapter)
u32 addr;
u8 *data;
+ data = kzalloc(RSI_9116_REG_SIZE, GFP_KERNEL);
+ if (!data)
+ return -ENOMEM;
+
status = rsi_sdio_master_access_msword(adapter, TA_BASE_ADDR);
if (status < 0) {
rsi_dbg(ERR_ZONE,
"Unable to set ms word to common reg\n");
- return status;
+ goto err;
}
rsi_dbg(INIT_ZONE, "%s: Bring TA out of reset\n", __func__);
@@ -944,7 +948,7 @@ static int rsi_sdio_ta_reset(struct rsi_hw *adapter)
RSI_9116_REG_SIZE);
if (status < 0) {
rsi_dbg(ERR_ZONE, "Unable to hold TA threads\n");
- return status;
+ goto err;
}
put_unaligned_le32(TA_SOFT_RST_CLR, data);
@@ -954,7 +958,7 @@ static int rsi_sdio_ta_reset(struct rsi_hw *adapter)
RSI_9116_REG_SIZE);
if (status < 0) {
rsi_dbg(ERR_ZONE, "Unable to get TA out of reset\n");
- return status;
+ goto err;
}
put_unaligned_le32(TA_PC_ZERO, data);
@@ -964,7 +968,8 @@ static int rsi_sdio_ta_reset(struct rsi_hw *adapter)
RSI_9116_REG_SIZE);
if (status < 0) {
rsi_dbg(ERR_ZONE, "Unable to Reset TA PC value\n");
- return -EINVAL;
+ status = -EINVAL;
+ goto err;
}
put_unaligned_le32(TA_RELEASE_THREAD_VALUE, data);
@@ -974,17 +979,19 @@ static int rsi_sdio_ta_reset(struct rsi_hw *adapter)
RSI_9116_REG_SIZE);
if (status < 0) {
rsi_dbg(ERR_ZONE, "Unable to release TA threads\n");
- return status;
+ goto err;
}
status = rsi_sdio_master_access_msword(adapter, MISC_CFG_BASE_ADDR);
if (status < 0) {
rsi_dbg(ERR_ZONE, "Unable to set ms word to common reg\n");
- return status;
+ goto err;
}
rsi_dbg(INIT_ZONE, "***** TA Reset done *****\n");
- return 0;
+err:
+ kfree(data);
+ return status;
}
static struct rsi_host_intf_ops sdio_host_intf_ops = {
--
2.22.0.rc1
^ permalink raw reply related
* Re: [PATCH] iw: print HE capabilities
From: Sven Eckelmann @ 2019-05-23 15:22 UTC (permalink / raw)
To: John Crispin; +Cc: Johannes Berg, linux-wireless, Shashidhar Lakkavalli
In-Reply-To: <20190520105416.27185-1-john@phrozen.org>
[-- Attachment #1: Type: text/plain, Size: 568 bytes --]
On Monday, 20 May 2019 12:54:16 CEST John Crispin wrote:
> + char *iftypes[NUM_NL80211_IFTYPES] = {
> + "Unspec", "Adhoc", "Station", "AP", "AP/VLAN", "WDS", "Monitor"
> + "Mesh", "P2P/Client", "P2P/Go", "P2P/Device", "OCB", "NAN",
> + };
Noticed during tests with hwsim that this list is slightly incorrect - because
the "," is missing after "Monitor". Thus you will now have an entry
"MonitorMesh" instead of two entries "Monitor" + "Mesh". So everything after
NL80211_IFTYPE_MESH_POINT is off by one.
Kind regards,
Sven
[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: [PATCH v2] sdio: Fix a memory leaking bug in wl1271_probe()
From: Kalle Valo @ 2019-05-23 15:00 UTC (permalink / raw)
To: Gen Zhang; +Cc: eyalreizer, netdev, linux-kernel, linux-wireless
In-Reply-To: <20190523144425.GA26766@zhanggen-UX430UQ>
+ linux-wireless
Gen Zhang <blackgod016574@gmail.com> writes:
> In wl1271_probe(), 'glue->core' is allocated by platform_device_alloc(),
> when this allocation fails, ENOMEM is returned. However, 'pdev_data'
> and 'glue' are allocated by devm_kzalloc() before 'glue->core'. When
> platform_device_alloc() returns NULL, we should also free 'pdev_data'
> and 'glue' before wl1271_probe() ends to prevent leaking memory.
>
> Similarly, we should free 'pdev_data' when 'glue' is NULL. And we
> should free 'pdev_data' and 'glue' when 'ret' is error.
>
> Further, we shoulf free 'glue->dev', 'pdev_data' and 'glue' when this
> function normally ends to prevent memory leaking.
>
> Signed-off-by: Gen Zhang <blackgod016574@gmail.com>
> ---
> diff --git a/drivers/net/wireless/ti/wlcore/sdio.c b/drivers/net/wireless/ti/wlcore/sdio.c
> index 4d4b0770..232ce5f 100644
> --- a/drivers/net/wireless/ti/wlcore/sdio.c
> +++ b/drivers/net/wireless/ti/wlcore/sdio.c
You need to CC linux-wireless, otherwise patchwork won't see the patch
and it will not be applied. Also you should use prefix "wlcore:".
https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches#commit_title_is_wrong
--
Kalle Valo
^ permalink raw reply
* Re: [PATCH 00/16] wilc1000: move out of staging
From: Kalle Valo @ 2019-05-23 14:13 UTC (permalink / raw)
To: Ajay.Kathat
Cc: linux-wireless, johannes, gregkh, Adham.Abozaeid,
Venkateswara.Kaja, Nicolas.Ferre, Claudiu.Beznea
In-Reply-To: <2ba91fc2-af3a-b235-5cb0-2ba06444ea30@microchip.com>
<Ajay.Kathat@microchip.com> writes:
> Hi Kalle,
>
> On 3/21/2019 6:24 PM, Kalle Valo wrote:
>>
>> <Ajay.Kathat@microchip.com> writes:
>>
>>> Hi Kalle/Johannes,
>>>
>>> On 2/9/2019 12:42 PM, Ajay Kathat - I15481 wrote:
>>>> From: Ajay Singh <ajay.kathat@microchip.com>
>>>>
>>>> This patch series is to review and move wilc1000 driver out of staging.
>>>> Implemented the initial received review comments[1] and submitting the
>>>> driver again. During this cleanup deleted around 3.3k lines of code.
>>>>
>>>> Below are the major items fixed in recent cleanup:
>>>> - remove use of shadow buffer to keep scan result.
>>>> - remove internal messaging flow to handle cfg80211_ops.
>>>> - make use of cfg80211 provided API.
>>>> - use structure for packing firmware commands.
>>>> - make use of kernel provided API and macros.
>>>> - remove unnecessary logs messages.
>>>>
>>>> Pending action item:
>>>> - dynamically add/remove p2p interface.
>>>>
>>>> This item will take some time, we are planning to take it up after
>>>> mainline.
>>>>
>>>> We hope it can be move out staging in v5.1.
>>>> Please review and confirm if it is good to move out.
>>>
>>> Do you have any update for this series. Please provide your inputs for
>>> next step.
>>
>> rtw88 is taking priority in the new drivers "queue" so I doubt I can
>> take a look at this in the next few weeks.
>>
>
> Is there any update for wilc1000 driver review. Please let me know your
> inputs.
So is the driver in good shape now? I really do not want to use a lot of
time reviewing it just to find out that there's a lot of work to do. For
example, has someone else reviewed it?
--
Kalle Valo
^ permalink raw reply
* Re: [PATCH 00/16] wilc1000: move out of staging
From: Ajay.Kathat @ 2019-05-23 13:32 UTC (permalink / raw)
To: kvalo
Cc: linux-wireless, johannes, gregkh, Adham.Abozaeid,
Venkateswara.Kaja, Nicolas.Ferre, Claudiu.Beznea
In-Reply-To: <871s30pevc.fsf@codeaurora.org>
Hi Kalle,
On 3/21/2019 6:24 PM, Kalle Valo wrote:
>
> <Ajay.Kathat@microchip.com> writes:
>
>> Hi Kalle/Johannes,
>>
>> On 2/9/2019 12:42 PM, Ajay Kathat - I15481 wrote:
>>> From: Ajay Singh <ajay.kathat@microchip.com>
>>>
>>> This patch series is to review and move wilc1000 driver out of staging.
>>> Implemented the initial received review comments[1] and submitting the
>>> driver again. During this cleanup deleted around 3.3k lines of code.
>>>
>>> Below are the major items fixed in recent cleanup:
>>> - remove use of shadow buffer to keep scan result.
>>> - remove internal messaging flow to handle cfg80211_ops.
>>> - make use of cfg80211 provided API.
>>> - use structure for packing firmware commands.
>>> - make use of kernel provided API and macros.
>>> - remove unnecessary logs messages.
>>>
>>> Pending action item:
>>> - dynamically add/remove p2p interface.
>>>
>>> This item will take some time, we are planning to take it up after
>>> mainline.
>>>
>>> We hope it can be move out staging in v5.1.
>>> Please review and confirm if it is good to move out.
>>
>> Do you have any update for this series. Please provide your inputs for
>> next step.
>
> rtw88 is taking priority in the new drivers "queue" so I doubt I can
> take a look at this in the next few weeks.
>
Is there any update for wilc1000 driver review. Please let me know your
inputs.
Regards,
Ajay
^ permalink raw reply
* Re: [PATCH 1/7] brcm80211: switch common header files to using SPDX license identifier
From: Arend Van Spriel @ 2019-05-23 10:53 UTC (permalink / raw)
To: Kalle Valo; +Cc: linux-wireless
In-Reply-To: <871s0pedih.fsf@kamboji.qca.qualcomm.com>
On May 23, 2019 11:17:22 AM Kalle Valo <kvalo@codeaurora.org> wrote:
> Arend Van Spriel <arend.vanspriel@broadcom.com> writes:
>> Or do you want me to resend the whole series without patch 3?
>
> If the series applies without patch 3 (and I assume it does) no need to
> resend the whole series.
Thanks, Kalle
I assume the same.
Regards,
Arend
^ permalink raw reply
* [PATCH] iw: add HE support to station dump call
From: John Crispin @ 2019-05-23 10:50 UTC (permalink / raw)
To: Johannes Berg; +Cc: linux-wireless, John Crispin, Shashidhar Lakkavalli
Signed-off-by: Shashidhar Lakkavalli <slakkavalli@datto.com>
Signed-off-by: John Crispin <john@phrozen.org>
---
station.c | 12 ++++++++++++
1 file changed, 12 insertions(+)
diff --git a/station.c b/station.c
index 0454f87..aaad079 100644
--- a/station.c
+++ b/station.c
@@ -243,6 +243,18 @@ void parse_bitrate(struct nlattr *bitrate_attr, char *buf, int buflen)
if (rinfo[NL80211_RATE_INFO_VHT_NSS])
pos += snprintf(pos, buflen - (pos - buf),
" VHT-NSS %d", nla_get_u8(rinfo[NL80211_RATE_INFO_VHT_NSS]));
+ if (rinfo[NL80211_RATE_INFO_HE_MCS])
+ pos += snprintf(pos, buflen - (pos - buf),
+ " HE-MCS %d", nla_get_u8(rinfo[NL80211_RATE_INFO_HE_MCS]));
+ if (rinfo[NL80211_RATE_INFO_HE_NSS])
+ pos += snprintf(pos, buflen - (pos - buf),
+ " HE-NSS %d", nla_get_u8(rinfo[NL80211_RATE_INFO_HE_NSS]));
+ if (rinfo[NL80211_RATE_INFO_HE_GI])
+ pos += snprintf(pos, buflen - (pos - buf),
+ " HE-GI %d", nla_get_u8(rinfo[NL80211_RATE_INFO_HE_GI]));
+ if (rinfo[NL80211_RATE_INFO_HE_DCM])
+ pos += snprintf(pos, buflen - (pos - buf),
+ " HE-DCM %d", nla_get_u8(rinfo[NL80211_RATE_INFO_HE_DCM]));
}
static char *get_chain_signal(struct nlattr *attr_list)
--
2.20.1
^ permalink raw reply related
* Re: [PATCH 1/7] brcm80211: switch common header files to using SPDX license identifier
From: Kalle Valo @ 2019-05-23 9:17 UTC (permalink / raw)
To: Arend Van Spriel; +Cc: linux-wireless
In-Reply-To: <f4d582e8-3d65-959f-78bd-88b7c47131a5@broadcom.com>
Arend Van Spriel <arend.vanspriel@broadcom.com> writes:
> On 5/20/2019 6:54 PM, Kalle Valo wrote:
>> Arend Van Spriel <arend.vanspriel@broadcom.com> writes:
>>
>>> On 5/16/2019 10:57 PM, Arend Van Spriel wrote:
>>>> On 5/16/2019 2:04 PM, Arend van Spriel wrote:
>>>>> With ISC license text in place under the LICENSES folder switch
>>>>> to using the SPDX license identifier to refer to the ISC license.
>>>>
>>>> Hi Kalle,
>>>>
>>>> Given the feedback on checkpatch (or spdxcheck) failures let me
>>>> respin this series.
>>>
>>> Actually let's *NOT* respin and leave this series as is and ignore the
>>> warning for the header files as Thomas wrote in his response: " So we
>>> can fixup the documentation and allow // style for headers as well.".
>>
>> What about patch 3, should I drop that patch? Wasn't the conclusion that
>> you need separately change led.c?
>
> Okay. Let's do that.
Dropped it now.
> Or do you want me to resend the whole series without patch 3?
If the series applies without patch 3 (and I assume it does) no need to
resend the whole series.
--
Kalle Valo
^ permalink raw reply
* Re: [PATCH 3/7] brcmsmac: switch source files to using SPDX license identifier
From: Kalle Valo @ 2019-05-23 9:16 UTC (permalink / raw)
To: Arend van Spriel
Cc: linux-wireless, Arend van Spriel, Thomas Gleixner,
Greg Kroah-Hartman
In-Reply-To: <1558008251-13692-4-git-send-email-arend.vanspriel@broadcom.com>
Arend van Spriel <arend.vanspriel@broadcom.com> wrote:
> With ISC license text in place under the LICENSES folder switch
> to using the SPDX license identifier to refer to the ISC license.
>
> Cc: Thomas Gleixner <tglx@linutronix.de>
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Reviewed-by: Hante Meuleman <hante.meuleman@broadcom.com>
> Reviewed-by: Pieter-Paul Giesberts <pieter-paul.giesberts@broadcom.com>
> Reviewed-by: Franky Lin <franky.lin@broadcom.com>
> Signed-off-by: Arend van Spriel <arend.vanspriel@broadcom.com>
Dropped per discussion
Patch set to Changes Requested.
--
https://patchwork.kernel.org/patch/10946471/
https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches
^ permalink raw reply
* Re: [PATCH] wireless: fix intel typos in code comments
From: Kalle Valo @ 2019-05-23 9:14 UTC (permalink / raw)
To: Weitao Hou; +Cc: stas.yakovlev, davem, linux-wireless, netdev, linux-kernel
In-Reply-To: <20190519033114.20271-1-houweitaoo@gmail.com>
Weitao Hou <houweitaoo@gmail.com> writes:
> fix lengh to length
>
> Signed-off-by: Weitao Hou <houweitaoo@gmail.com>
> ---
> drivers/net/wireless/intel/ipw2x00/ipw2200.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
Correct prefix is "ipw2x00:".
https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches#commit_title_is_wrong
--
Kalle Valo
^ permalink raw reply
* Re: [PATCH] net: fix typos in code comments
From: Kalle Valo @ 2019-05-23 9:12 UTC (permalink / raw)
To: Weitao Hou
Cc: nbd, lorenzo.bianconi83, ryder.lee, royluo, davem, matthias.bgg,
linux-wireless, netdev, linux-arm-kernel, linux-mediatek,
linux-kernel
In-Reply-To: <20190519030923.18343-1-houweitaoo@gmail.com>
Weitao Hou <houweitaoo@gmail.com> writes:
> fix lenght to length
>
> Signed-off-by: Weitao Hou <houweitaoo@gmail.com>
> ---
> drivers/net/wireless/mediatek/mt76/mt76x02_usb_core.c | 2 +-
Please use correct prefix "mt76:":
https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches#commit_title_is_wrong
--
Kalle Valo
^ permalink raw reply
* Re: [PATCH] rsi: Properly initialize data in rsi_sdio_ta_reset
From: Kalle Valo @ 2019-05-23 8:56 UTC (permalink / raw)
To: Arnd Bergmann
Cc: Nathan Chancellor, Nick Desaulniers, Amitkumar Karwar,
Siva Rebbagondla, linux-wireless, Networking, LKML,
clang-built-linux
In-Reply-To: <CAK8P3a001V5qQo4vGfpugtmrnFfUNeP_q4KY-YS7rP_L91HY1A@mail.gmail.com>
Arnd Bergmann <arnd@arndb.de> writes:
>> > @@ -937,7 +937,7 @@ static int rsi_sdio_ta_reset(struct rsi_hw *adapter)
>> > }
>> >
>> > rsi_dbg(INIT_ZONE, "%s: Bring TA out of reset\n", __func__);
>> > - put_unaligned_le32(TA_HOLD_THREAD_VALUE, data);
>> > + put_unaligned_le32(TA_HOLD_THREAD_VALUE, &data);
>> > addr = TA_HOLD_THREAD_REG | RSI_SD_REQUEST_MASTER;
>> > status = rsi_sdio_write_register_multiple(adapter, addr,
>> > (u8 *)&data,
>
> This is clearly not ok, as put_unaligned_le32() stores four bytes, and
> the local variable is only one byte!
>
> Also, sdio does use DMA for transfers, so the variable has to be
> dynamically allocated. I think your original patch was correct.
> The only change I'd possibly make would be to use
> RSI_9116_REG_SIZE instead of sizeof(u32).
Good point. Nathan please fix this and submit v2.
>> Did any of the maintainers have any comments on what the correct
>> solution is here to resolve this warning? It is one of the few left
>> before we can turn on -Wuninitialized for the whole kernel.
>
> I would argue that this should not stop us from turning it on, as the
> warning is for a clear bug in the code that absolutely needs to be
> fixed, rather than a false-positive.
I can queue v2 for v5.2, just remind me by adding "[PATCH v2 5.2]" to
the subject.
--
Kalle Valo
^ permalink raw reply
* Re: [PATCH v3] ath10k: add support for firmware crash recovery on SDIO chip
From: Claire Chang @ 2019-05-23 8:53 UTC (permalink / raw)
To: Wen Gong; +Cc: ath10k, open list:NETWORKING DRIVERS (WIRELESS)
In-Reply-To: <1558506776-19702-1-git-send-email-wgong@codeaurora.org>
Tested-by: Claire Chang <tientzu@chromium.org>
^ permalink raw reply
* Re: [PATCH] rsi: Properly initialize data in rsi_sdio_ta_reset
From: Kalle Valo @ 2019-05-23 8:52 UTC (permalink / raw)
To: Nathan Chancellor
Cc: Nick Desaulniers, Amitkumar Karwar, Siva Rebbagondla,
linux-wireless, netdev, LKML, clang-built-linux
In-Reply-To: <20190523015415.GA17819@archlinux-epyc>
Nathan Chancellor <natechancellor@gmail.com> writes:
> On Thu, May 02, 2019 at 08:17:18PM -0700, Nathan Chancellor wrote:
>> On Thu, May 02, 2019 at 11:18:01AM -0700, Nick Desaulniers wrote:
>> > On Thu, May 2, 2019 at 8:16 AM Nathan Chancellor
>> > <natechancellor@gmail.com> wrote:
>> > >
>> > > When building with -Wuninitialized, Clang warns:
>> > >
>> > > drivers/net/wireless/rsi/rsi_91x_sdio.c:940:43: warning: variable 'data'
>> > > is uninitialized when used here [-Wuninitialized]
>> > > put_unaligned_le32(TA_HOLD_THREAD_VALUE, data);
>> > > ^~~~
>> > > drivers/net/wireless/rsi/rsi_91x_sdio.c:930:10: note: initialize the
>> > > variable 'data' to silence this warning
>> > > u8 *data;
>> > > ^
>> > > = NULL
>> > > 1 warning generated.
>> > >
>> > > Using Clang's suggestion of initializing data to NULL wouldn't work out
>> > > because data will be dereferenced by put_unaligned_le32. Use kzalloc to
>> > > properly initialize data, which matches a couple of other places in this
>> > > driver.
>> > >
>> > > Fixes: e5a1ecc97e5f ("rsi: add firmware loading for 9116 device")
>> > > Link: https://github.com/ClangBuiltLinux/linux/issues/464
>> > > Signed-off-by: Nathan Chancellor <natechancellor@gmail.com>
>> > > ---
>> > > drivers/net/wireless/rsi/rsi_91x_sdio.c | 21 ++++++++++++++-------
>> > > 1 file changed, 14 insertions(+), 7 deletions(-)
>> > >
>> > > diff --git a/drivers/net/wireless/rsi/rsi_91x_sdio.c b/drivers/net/wireless/rsi/rsi_91x_sdio.c
>> > > index f9c67ed473d1..b35728564c7b 100644
>> > > --- a/drivers/net/wireless/rsi/rsi_91x_sdio.c
>> > > +++ b/drivers/net/wireless/rsi/rsi_91x_sdio.c
>> > > @@ -929,11 +929,15 @@ static int rsi_sdio_ta_reset(struct rsi_hw *adapter)
>> > > u32 addr;
>> > > u8 *data;
>> > >
>> > > + data = kzalloc(sizeof(u32), GFP_KERNEL);
>> >
>> > Something fishy is going on here. We allocate 4 B but declare data as
>> > a u8* (pointer to individual bytes)? In general, dynamically
>> > allocating that few bytes is a code smell; either you meant to just
>> > use the stack, or this memory's lifetime extends past the lifetime of
>> > this stackframe, at which point you probably just meant to stack
>> > allocate space in a higher parent frame and pass this preallocated
>> > memory down to the child frame to get filled in.
>> >
>> > Reading through this code, I don't think that the memory is meant to
>> > outlive the stack frame. Is there a reason why we can't just declare
>> > data as:
>> >
>> > u8 data [4];
>>
>> data was __le32 in rsi_reset_chip() before commit f700546682a6 ("rsi:
>> fix nommu_map_sg overflow kernel panic").
>>
>> I wonder if this would be okay for this function:
>>
>> -------------------------------------------------
>>
>> diff --git a/drivers/net/wireless/rsi/rsi_91x_sdio.c b/drivers/net/wireless/rsi/rsi_91x_sdio.c
>> index f9c67ed473d1..0330c50ab99c 100644
>> --- a/drivers/net/wireless/rsi/rsi_91x_sdio.c
>> +++ b/drivers/net/wireless/rsi/rsi_91x_sdio.c
>> @@ -927,7 +927,7 @@ static int rsi_sdio_ta_reset(struct rsi_hw *adapter)
>> {
>> int status;
>> u32 addr;
>> - u8 *data;
>> + u8 data;
>>
>> status = rsi_sdio_master_access_msword(adapter, TA_BASE_ADDR);
>> if (status < 0) {
>> @@ -937,7 +937,7 @@ static int rsi_sdio_ta_reset(struct rsi_hw *adapter)
>> }
>>
>> rsi_dbg(INIT_ZONE, "%s: Bring TA out of reset\n", __func__);
>> - put_unaligned_le32(TA_HOLD_THREAD_VALUE, data);
>> + put_unaligned_le32(TA_HOLD_THREAD_VALUE, &data);
>> addr = TA_HOLD_THREAD_REG | RSI_SD_REQUEST_MASTER;
>> status = rsi_sdio_write_register_multiple(adapter, addr,
>> (u8 *)&data,
>> @@ -947,7 +947,7 @@ static int rsi_sdio_ta_reset(struct rsi_hw *adapter)
>> return status;
>> }
>>
>> - put_unaligned_le32(TA_SOFT_RST_CLR, data);
>> + put_unaligned_le32(TA_SOFT_RST_CLR, &data);
>> addr = TA_SOFT_RESET_REG | RSI_SD_REQUEST_MASTER;
>> status = rsi_sdio_write_register_multiple(adapter, addr,
>> (u8 *)&data,
>> @@ -957,7 +957,7 @@ static int rsi_sdio_ta_reset(struct rsi_hw *adapter)
>> return status;
>> }
>>
>> - put_unaligned_le32(TA_PC_ZERO, data);
>> + put_unaligned_le32(TA_PC_ZERO, &data);
>> addr = TA_TH0_PC_REG | RSI_SD_REQUEST_MASTER;
>> status = rsi_sdio_write_register_multiple(adapter, addr,
>> (u8 *)&data,
>> @@ -967,7 +967,7 @@ static int rsi_sdio_ta_reset(struct rsi_hw *adapter)
>> return -EINVAL;
>> }
>>
>> - put_unaligned_le32(TA_RELEASE_THREAD_VALUE, data);
>> + put_unaligned_le32(TA_RELEASE_THREAD_VALUE, &data);
>> addr = TA_RELEASE_THREAD_REG | RSI_SD_REQUEST_MASTER;
>> status = rsi_sdio_write_register_multiple(adapter, addr,
>> (u8 *)&data,
>>
>>
>> >
>> > then use ARRAY_SIZE(data) or RSI_9116_REG_SIZE in rsi_reset_chip(),
>> > getting rid of the kzalloc/kfree?
>> >
>> > (Sorry, I hate when a simple fixup becomes a "hey let's rewrite all
>> > this code" thus becoming "that guy.")
>>
>> If we aren't actually improving the code, then why bother? :)
>>
>> Thank you for the review!
>
> Did any of the maintainers have any comments on what the correct
> solution is here to resolve this warning? It is one of the few left
> before we can turn on -Wuninitialized for the whole kernel.
I don't have any strong opinion, but as the commit log says that
kzalloc() is also used in similar cases in the same driver I would happy
to take this patch as is.
--
Kalle Valo
^ permalink raw reply
* Re: [PATCH] rsi: Properly initialize data in rsi_sdio_ta_reset
From: Arnd Bergmann @ 2019-05-23 8:51 UTC (permalink / raw)
To: Nathan Chancellor
Cc: Nick Desaulniers, Amitkumar Karwar, Siva Rebbagondla, Kalle Valo,
linux-wireless, Networking, LKML, clang-built-linux
In-Reply-To: <20190523015415.GA17819@archlinux-epyc>
On Thu, May 23, 2019 at 3:54 AM Nathan Chancellor
<natechancellor@gmail.com> wrote:
>
> On Thu, May 02, 2019 at 08:17:18PM -0700, Nathan Chancellor wrote:
> > On Thu, May 02, 2019 at 11:18:01AM -0700, Nick Desaulniers wrote:
> > > On Thu, May 2, 2019 at 8:16 AM Nathan Chancellor
> > > u8 data [4];
> >
> > data was __le32 in rsi_reset_chip() before commit f700546682a6 ("rsi:
> > fix nommu_map_sg overflow kernel panic").
> >
> > I wonder if this would be okay for this function:
> >
> > -------------------------------------------------
> >
> > diff --git a/drivers/net/wireless/rsi/rsi_91x_sdio.c b/drivers/net/wireless/rsi/rsi_91x_sdio.c
> > index f9c67ed473d1..0330c50ab99c 100644
> > --- a/drivers/net/wireless/rsi/rsi_91x_sdio.c
> > +++ b/drivers/net/wireless/rsi/rsi_91x_sdio.c
> > @@ -927,7 +927,7 @@ static int rsi_sdio_ta_reset(struct rsi_hw *adapter)
> > {
> > int status;
> > u32 addr;
> > - u8 *data;
> > + u8 data;
> >
> > status = rsi_sdio_master_access_msword(adapter, TA_BASE_ADDR);
> > if (status < 0) {
> > @@ -937,7 +937,7 @@ static int rsi_sdio_ta_reset(struct rsi_hw *adapter)
> > }
> >
> > rsi_dbg(INIT_ZONE, "%s: Bring TA out of reset\n", __func__);
> > - put_unaligned_le32(TA_HOLD_THREAD_VALUE, data);
> > + put_unaligned_le32(TA_HOLD_THREAD_VALUE, &data);
> > addr = TA_HOLD_THREAD_REG | RSI_SD_REQUEST_MASTER;
> > status = rsi_sdio_write_register_multiple(adapter, addr,
> > (u8 *)&data,
This is clearly not ok, as put_unaligned_le32() stores four bytes, and
the local variable is only one byte!
Also, sdio does use DMA for transfers, so the variable has to be
dynamically allocated. I think your original patch was correct.
The only change I'd possibly make would be to use
RSI_9116_REG_SIZE instead of sizeof(u32).
> Did any of the maintainers have any comments on what the correct
> solution is here to resolve this warning? It is one of the few left
> before we can turn on -Wuninitialized for the whole kernel.
I would argue that this should not stop us from turning it on, as the
warning is for a clear bug in the code that absolutely needs to be
fixed, rather than a false-positive.
Arnd
^ permalink raw reply
* [PATCH V2] mac80211: fix rate reporting inside cfg80211_calculate_bitrate_he()
From: John Crispin @ 2019-05-23 8:27 UTC (permalink / raw)
To: Johannes Berg; +Cc: linux-wireless, John Crispin, Shashidhar Lakkavalli
The reported rate is not scaled down correctly. After applying this patch,
the function will behave just like the v/ht equivalents.
Signed-off-by: Shashidhar Lakkavalli <slakkavalli@datto.com>
Signed-off-by: John Crispin <john@phrozen.org>
---
net/wireless/util.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/net/wireless/util.c b/net/wireless/util.c
index 75899b62bdc9..5a03f38788e7 100644
--- a/net/wireless/util.c
+++ b/net/wireless/util.c
@@ -1237,7 +1237,7 @@ static u32 cfg80211_calculate_bitrate_he(struct rate_info *rate)
if (rate->he_dcm)
result /= 2;
- return result;
+ return result / 10000;
}
u32 cfg80211_calculate_bitrate(struct rate_info *rate)
--
2.20.1
^ permalink raw reply related
* Re: [PATCH] mac80211: fix rate reporting inside cfg80211_calculate_bitrate_he()
From: John Crispin @ 2019-05-23 8:27 UTC (permalink / raw)
To: Johannes Berg; +Cc: linux-wireless, Shashidhar Lakkavalli
In-Reply-To: <20190523080332.29173-1-john@phrozen.org>
On 23/05/2019 10:03, John Crispin wrote:
> The reported rate is not scaled done correctly. After applying this patch, the
> function will behave just like the v/ht equivalents.
s/done/down/ ... sorry aspell obviously did not catch that one :-)
John
> Signed-off-by: Shashidhar Lakkavalli <slakkavalli@datto.com>
> Signed-off-by: John Crispin <john@phrozen.org>
> ---
> net/wireless/util.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/net/wireless/util.c b/net/wireless/util.c
> index 75899b62bdc9..5a03f38788e7 100644
> --- a/net/wireless/util.c
> +++ b/net/wireless/util.c
> @@ -1237,7 +1237,7 @@ static u32 cfg80211_calculate_bitrate_he(struct rate_info *rate)
> if (rate->he_dcm)
> result /= 2;
>
> - return result;
> + return result / 10000;
> }
>
> u32 cfg80211_calculate_bitrate(struct rate_info *rate)
^ permalink raw reply
* [PATCH] mac80211: fix rate reporting inside cfg80211_calculate_bitrate_he()
From: John Crispin @ 2019-05-23 8:03 UTC (permalink / raw)
To: Johannes Berg; +Cc: linux-wireless, John Crispin, Shashidhar Lakkavalli
The reported rate is not scaled done correctly. After applying this patch, the
function will behave just like the v/ht equivalents.
Signed-off-by: Shashidhar Lakkavalli <slakkavalli@datto.com>
Signed-off-by: John Crispin <john@phrozen.org>
---
net/wireless/util.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/net/wireless/util.c b/net/wireless/util.c
index 75899b62bdc9..5a03f38788e7 100644
--- a/net/wireless/util.c
+++ b/net/wireless/util.c
@@ -1237,7 +1237,7 @@ static u32 cfg80211_calculate_bitrate_he(struct rate_info *rate)
if (rate->he_dcm)
result /= 2;
- return result;
+ return result / 10000;
}
u32 cfg80211_calculate_bitrate(struct rate_info *rate)
--
2.20.1
^ permalink raw reply related
* Re: ath10k QCA9377 firmware crashes and fails to recover
From: Kalle Valo @ 2019-05-23 7:35 UTC (permalink / raw)
To: Daniel Drake; +Cc: ath10k, linux-wireless, Endless Linux Upstreaming Team
In-Reply-To: <CAD8Lp45wxQ3vL_ttq-yKYDxscjn2KyJVCx_vJBCn+u8Yc5QtOQ@mail.gmail.com>
Daniel Drake <drake@endlessm.com> writes:
> We are experiencing failures with QCA9377 wifi, using Linux 4.18 and
> Linux 5.0 with the latest firmware version:
>
> ath10k_pci 0000:02:00.0: firmware crashed! (guid
> 54a4649a-1240-4459-9442-9d498c49de79)
> ath10k_pci 0000:02:00.0: qca9377 hw1.1 target 0x05020001 chip_id
> 0x003821ff sub 1a3b:2b31
> ath10k_pci 0000:02:00.0: kconfig debug 0 debugfs 1 tracing 1 dfs 0 testmode 0
> ath10k_pci 0000:02:00.0: firmware ver WLAN.TF.1.0-00002-QCATFSWPZ-5
> api 5 features ignore-otp crc32 c3e0d04f
Is this a regression? For example, have you tried older firmware
versions?
--
Kalle Valo
^ permalink raw reply
* [PATCH] ath10k: add missing error handling
From: Claire Chang @ 2019-05-23 7:15 UTC (permalink / raw)
To: kvalo; +Cc: ath10k, linux-wireless, wgong, Claire Chang
In function ath10k_sdio_mbox_rx_alloc() [sdio.c],
ath10k_sdio_mbox_alloc_rx_pkt() is called without handling the error cases.
This will make the driver think the allocation for skb is successful and
try to access the skb. If we enable failslab, system will easily crash with
NULL pointer dereferencing.
Call trace of CONFIG_FAILSLAB:
ath10k_sdio_irq_handler+0x570/0xa88 [ath10k_sdio]
process_sdio_pending_irqs+0x4c/0x174
sdio_run_irqs+0x3c/0x64
sdio_irq_work+0x1c/0x28
Fixes: d96db25d2025 ("ath10k: add initial SDIO support")
Signed-off-by: Claire Chang <tientzu@chromium.org>
---
drivers/net/wireless/ath/ath10k/sdio.c | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/drivers/net/wireless/ath/ath10k/sdio.c b/drivers/net/wireless/ath/ath10k/sdio.c
index 9bbd5b54b8ca..4b7f1c6f13e2 100644
--- a/drivers/net/wireless/ath/ath10k/sdio.c
+++ b/drivers/net/wireless/ath/ath10k/sdio.c
@@ -607,6 +607,10 @@ static int ath10k_sdio_mbox_rx_alloc(struct ath10k *ar,
full_len,
last_in_bundle,
last_in_bundle);
+ if (ret) {
+ ath10k_warn(ar, "alloc_rx_pkt error %d\n", ret);
+ goto err;
+ }
}
ar_sdio->n_rx_pkts = i;
--
2.21.0.1020.gf2820cf01a-goog
^ permalink raw reply related
* ath10k QCA9377 firmware crashes and fails to recover
From: Daniel Drake @ 2019-05-23 3:19 UTC (permalink / raw)
To: Kalle Valo, ath10k; +Cc: linux-wireless, Endless Linux Upstreaming Team
Hi,
We are experiencing failures with QCA9377 wifi, using Linux 4.18 and
Linux 5.0 with the latest firmware version:
ath10k_pci 0000:02:00.0: firmware crashed! (guid
54a4649a-1240-4459-9442-9d498c49de79)
ath10k_pci 0000:02:00.0: qca9377 hw1.1 target 0x05020001 chip_id
0x003821ff sub 1a3b:2b31
ath10k_pci 0000:02:00.0: kconfig debug 0 debugfs 1 tracing 1 dfs 0 testmode 0
ath10k_pci 0000:02:00.0: firmware ver WLAN.TF.1.0-00002-QCATFSWPZ-5
api 5 features ignore-otp crc32 c3e0d04f
ath10k_pci 0000:02:00.0: board_file api 2 bmi_id N/A crc32 8aedfa4a
ath10k_pci 0000:02:00.0: htt-ver 3.44 wmi-op 4 htt-op 3 cal otp
max-sta 32 raw 0 hwcrypto 1
ath10k_pci 0000:02:00.0: firmware register dump:
ath10k_pci 0000:02:00.0: [00]: 0x05020001 0x00000000 0x1D008136 0x00000000
ath10k_pci 0000:02:00.0: [04]: 0x1D008136 0x00060330 0x00000004 0x00000000
ath10k_pci 0000:02:00.0: [08]: 0x00000001 0x0042B0EC 0x0040E84C 0x00433E18
ath10k_pci 0000:02:00.0: [12]: 0x00000000 0xFFFFFFFF 0x00952F6C 0x00952F77
ath10k_pci 0000:02:00.0: [16]: 0x0127099E 0x7DBF8AE8 0x00000000 0x00000000
ath10k_pci 0000:02:00.0: [20]: 0x0000013C 0x00433E18 0x0040E7E8 0x00436ED0
ath10k_pci 0000:02:00.0: [24]: 0x800B1A0B 0x0040E7E8 0x00433E18 0x00436FA0
ath10k_pci 0000:02:00.0: [28]: 0x00000001 0x00433E18 0x00436FA0 0x00429558
ath10k_pci 0000:02:00.0: [32]: 0x00000000 0x00433E18 0x00400000 0x000000F4
ath10k_pci 0000:02:00.0: [36]: 0x809ED9B2 0x0040E898 0x00000201 0x0040E900
ath10k_pci 0000:02:00.0: [40]: 0x00800000 0x00000000 0x00000000 0x00000000
ath10k_pci 0000:02:00.0: [44]: 0x00000000 0x00000000 0x00000001 0x00000000
ath10k_pci 0000:02:00.0: [48]: 0x00000018 0x00436FA0 0x00000000 0x00000000
ath10k_pci 0000:02:00.0: [52]: 0x004089D0 0x00000000 0x00000000 0x00000000
ath10k_pci 0000:02:00.0: [56]: 0x00000127 0x00000000 0x00000040 0x80168040
ath10k_pci 0000:02:00.0: Copy Engine register dump:
ath10k_pci 0000:02:00.0: [00]: 0x00034400 2 2 3 3
ath10k_pci 0000:02:00.0: [01]: 0x00034800 21 21 98 99
ath10k_pci 0000:02:00.0: [02]: 0x00034c00 12 12 75 76
ath10k_pci 0000:02:00.0: [03]: 0x00035000 7 7 9 7
ath10k_pci 0000:02:00.0: [04]: 0x00035400 7033 7033 253 190
ath10k_pci 0000:02:00.0: [05]: 0x00035800 0 0 64 0
ath10k_pci 0000:02:00.0: [06]: 0x00035c00 8 8 0 0
ath10k_pci 0000:02:00.0: [07]: 0x00036000 1 1 1 1
ieee80211 phy0: Hardware restart was requested
ath10k_pci 0000:02:00.0: firmware crashed! (guid
4f81011e-ff0e-42d1-8f97-74a576157bc7)
ath10k_pci 0000:02:00.0: qca9377 hw1.1 target 0x05020001 chip_id
0x003821ff sub 1a3b:2b31
ath10k_pci 0000:02:00.0: kconfig debug 0 debugfs 1 tracing 1 dfs 0 testmode 0
ath10k_pci 0000:02:00.0: firmware ver WLAN.TF.1.0-00002-QCATFSWPZ-5
api 5 features ignore-otp crc32 c3e0d04f
ath10k_pci 0000:02:00.0: board_file api 2 bmi_id N/A crc32 8aedfa4a
ath10k_pci 0000:02:00.0: htt-ver 3.44 wmi-op 4 htt-op 3 cal otp
max-sta 32 raw 0 hwcrypto 1
ath10k_pci 0000:02:00.0: firmware register dump:
ath10k_pci 0000:02:00.0: [00]: 0x05020001 0x00000000 0x809FF705 0x00000000
ath10k_pci 0000:02:00.0: [04]: 0x809FF705 0x0040EA78 0x0043DA00 0x0042C514
ath10k_pci 0000:02:00.0: [08]: 0x004301B0 0x004301C0 0x00000064 0x00000000
ath10k_pci 0000:02:00.0: [12]: 0x809B140B 0x004301C0 0x004301C8 0x00000064
ath10k_pci 0000:02:00.0: [16]: 0x00000000 0x0040EABC 0x00000064 0x0040EAB0
ath10k_pci 0000:02:00.0: [20]: 0x000FFFFF 0x0040EAE8 0x00000011 0x004041E4
ath10k_pci 0000:02:00.0: [24]: 0x00020000 0x00020000 0x00400000 0x00000001
ath10k_pci 0000:02:00.0: [28]: 0x809FCDD8 0x0040EA98 0x0043DA00 0x00000001
ath10k_pci 0000:02:00.0: [32]: 0x00000001 0x004122C0 0x00426BD0 0x00035428
ath10k_pci 0000:02:00.0: [36]: 0x00000000 0x00000000 0x00000000 0x00000000
ath10k_pci 0000:02:00.0: [40]: 0x00000000 0x00000000 0x00000000 0x00000000
ath10k_pci 0000:02:00.0: [44]: 0x00000000 0x00000000 0x00000000 0x00000000
ath10k_pci 0000:02:00.0: [48]: 0x00000000 0x00000000 0x00000000 0x00000000
ath10k_pci 0000:02:00.0: [52]: 0x00000000 0x00000000 0x00000000 0x00000000
ath10k_pci 0000:02:00.0: [56]: 0x00000000 0x00000000 0x00000000 0x00000000
ath10k_pci 0000:02:00.0: Copy Engine register dump:
ath10k_pci 0000:02:00.0: [00]: 0x00034400 2 2 3 3
ath10k_pci 0000:02:00.0: [01]: 0x00034800 3 3 16 17
ath10k_pci 0000:02:00.0: [02]: 0x00034c00 1 1 0 1
ath10k_pci 0000:02:00.0: [03]: 0x00035000 1 1 2 1
ath10k_pci 0000:02:00.0: [04]: 0x00035400 14 14 64 6
ath10k_pci 0000:02:00.0: [05]: 0x00035800 0 0 0 0
ath10k_pci 0000:02:00.0: [06]: 0x00035c00 0 0 0 0
ath10k_pci 0000:02:00.0: [07]: 0x00036000 1 1 1 1
ath10k_pci 0000:02:00.0: wmi unified ready event not received
ath10k_pci 0000:02:00.0: device has crashed during init
ath10k_pci 0000:02:00.0: device has crashed during init
ath10k_pci 0000:02:00.0: failed to wait for target init: -70
ath10k_pci 0000:02:00.0: Could not init core: -110
------------[ cut here ]------------
Hardware became unavailable during restart.
ath10k_pci 0000:02:00.0: cannot restart a device that hasn't been started
WARNING: CPU: 2 PID: 316 at
/usr/src/packages/BUILD/net/mac80211/util.c:1907
ieee80211_reconfig+0x235/0xf70 [mac80211]
Modules linked in: uas usb_storage ccm rfcomm arc4 joydev cmac bnep
hid_multitouch hid_generic spi_pxa2xx_platform 8250_dw intel_rapl
intel_telemetry_pltdrv intel_punit_ipc intel_telemetry_core
intel_pmc_ipc x86_pkg_temp_thermal intel_powerclamp coretemp kvm_intel
snd_soc_skl kvm irqbypass crct10dif_pclmul snd_soc_skl_ipc
snd_soc_sst_ipc snd_soc_sst_dsp crc32_pclmul snd_hda_ext_core
snd_hda_codec_hdmi ghash_clmulni_intel snd_soc_acpi pcbc snd_soc_core
snd_hda_codec_realtek snd_hda_codec_generic snd_compress uvcvideo
videobuf2_vmalloc ac97_bus aesni_intel videobuf2_memops videobuf2_v4l2
aes_x86_64 snd_pcm_dmaengine crypto_simd videobuf2_common cryptd
glue_helper videodev snd_hda_intel ath10k_pci intel_cstate
snd_hda_codec intel_rapl_perf media ath10k_core snd_hda_core snd_hwdep
btusb btrtl
btbcm btintel ath snd_pcm asus_nb_wmi asus_wmi mac80211 snd_timer
sparse_keymap bluetooth input_leds wmi_bmof bfq snd soundcore
ecdh_generic rtsx_pci_ms memstick cfg80211 idma64 virt_dma
intel_lpss_pci intel_lpss mei_me processor_thermal_device mei
intel_soc_dts_iosf mac_hid int3400_thermal int3403_thermal
acpi_thermal_rel int340x_thermal_zone pinctrl_geminilake asus_wireless
zram ip_tables x_tables mmc_block rtsx_pci_sdmmc i915 i2c_algo_bit
drm_kms_helper syscopyarea sysfillrect sysimgblt fb_sys_fops serio_raw
ahci sdhci_pci rtsx_pci drm cqhci sdhci libahci wmi i2c_hid hid video
CPU: 2 PID: 316 Comm: kworker/2:5 Not tainted 4.18.0-15-generic
#16+dev706.ddb1008beos3.5.2
Hardware name: ASUSTeK COMPUTER INC. VivoBook_ASUS Laptop
E406MA_E406MA/E406MA, BIOS E406MA.311 03/14/2019
Workqueue: events_freezable ieee80211_restart_work [mac80211]
RIP: 0010:ieee80211_reconfig+0x235/0xf70 [mac80211]
Code: c6 45 d6 00 c6 83 94 04 00 00 00 48 89 df e8 f2 a9 fc ff 85 c0
41 89 c6 0f 84 40 01 00 00 48 c7 c7 b8 b1 a6 c0 e8 fb b4 c6 dc <0f> 0b
e9 48 fe ff ff 4c 89 e7 e8 dc 09 02 00 e9 13 ff ff ff c6 83
RSP: 0018:ffffad7c80a57de8 EFLAGS: 00010286
RAX: 0000000000000000 RBX: ffff8870af2b0760 RCX: 0000000000000006
RDX: 0000000000000007 RSI: 0000000000000082 RDI: ffff8870bfd16540
RBP: ffffad7c80a57e30 R08: 0000000000000001 R09: 00000000000003a9
R10: 00000000000000c9 R11: 00000000000003a9 R12: ffff8870af2b0f90
R13: ffff8870af2b13a0 R14: 00000000ffffff92 R15: 0000000000000000
FS: 0000000000000000(0000) GS:ffff8870bfd00000(0000) knlGS:0000000000000000
CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
CR2: 00003eb20551d000 CR3: 0000000150908000 CR4: 0000000000340ee0
Call Trace:
ieee80211_restart_work+0xbc/0xf0 [mac80211]
process_one_work+0x1fa/0x3f0
worker_thread+0x4d/0x410
kthread+0x105/0x140
? rescuer_thread+0x370/0x370
? kthread_create_worker_on_cpu+0x70/0x70
ret_from_fork+0x1f/0x40
---[ end trace bfb7f12d31b47270 ]---
followed by many more similar warnings.
This problem can be easily reproduced by playing youtube videos for a
few minutes. When it happens, the system does not recover and the wifi
is unusable til reboot.
We have confirmed that this problem only affects specific cards. Other
identical-looking QCA9377 cards found in other instances of the same
laptop products do not suffer the problem. If we move an affected card
to another laptop, the problem will then readily occur on that laptop
too, i.e. the problem follows the card.
Having seen multiple cards that follow this pattern, it does not
appear to be a one-off.
It also does not appear to be an actual defect with the hardware
because we've tested the same cards extensively under Windows 10 and
no problems are seen there.
Any suggestions for further debugging?
We would be happy to ship affected devices to Qualcomm engineers if that helps.
Thanks
Daniel
^ permalink raw reply
* Re: [PATCH] rsi: Properly initialize data in rsi_sdio_ta_reset
From: Nathan Chancellor @ 2019-05-23 1:54 UTC (permalink / raw)
To: Nick Desaulniers
Cc: Amitkumar Karwar, Siva Rebbagondla, Kalle Valo, linux-wireless,
netdev, LKML, clang-built-linux
In-Reply-To: <20190503031718.GB6969@archlinux-i9>
On Thu, May 02, 2019 at 08:17:18PM -0700, Nathan Chancellor wrote:
> On Thu, May 02, 2019 at 11:18:01AM -0700, Nick Desaulniers wrote:
> > On Thu, May 2, 2019 at 8:16 AM Nathan Chancellor
> > <natechancellor@gmail.com> wrote:
> > >
> > > When building with -Wuninitialized, Clang warns:
> > >
> > > drivers/net/wireless/rsi/rsi_91x_sdio.c:940:43: warning: variable 'data'
> > > is uninitialized when used here [-Wuninitialized]
> > > put_unaligned_le32(TA_HOLD_THREAD_VALUE, data);
> > > ^~~~
> > > drivers/net/wireless/rsi/rsi_91x_sdio.c:930:10: note: initialize the
> > > variable 'data' to silence this warning
> > > u8 *data;
> > > ^
> > > = NULL
> > > 1 warning generated.
> > >
> > > Using Clang's suggestion of initializing data to NULL wouldn't work out
> > > because data will be dereferenced by put_unaligned_le32. Use kzalloc to
> > > properly initialize data, which matches a couple of other places in this
> > > driver.
> > >
> > > Fixes: e5a1ecc97e5f ("rsi: add firmware loading for 9116 device")
> > > Link: https://github.com/ClangBuiltLinux/linux/issues/464
> > > Signed-off-by: Nathan Chancellor <natechancellor@gmail.com>
> > > ---
> > > drivers/net/wireless/rsi/rsi_91x_sdio.c | 21 ++++++++++++++-------
> > > 1 file changed, 14 insertions(+), 7 deletions(-)
> > >
> > > diff --git a/drivers/net/wireless/rsi/rsi_91x_sdio.c b/drivers/net/wireless/rsi/rsi_91x_sdio.c
> > > index f9c67ed473d1..b35728564c7b 100644
> > > --- a/drivers/net/wireless/rsi/rsi_91x_sdio.c
> > > +++ b/drivers/net/wireless/rsi/rsi_91x_sdio.c
> > > @@ -929,11 +929,15 @@ static int rsi_sdio_ta_reset(struct rsi_hw *adapter)
> > > u32 addr;
> > > u8 *data;
> > >
> > > + data = kzalloc(sizeof(u32), GFP_KERNEL);
> >
> > Something fishy is going on here. We allocate 4 B but declare data as
> > a u8* (pointer to individual bytes)? In general, dynamically
> > allocating that few bytes is a code smell; either you meant to just
> > use the stack, or this memory's lifetime extends past the lifetime of
> > this stackframe, at which point you probably just meant to stack
> > allocate space in a higher parent frame and pass this preallocated
> > memory down to the child frame to get filled in.
> >
> > Reading through this code, I don't think that the memory is meant to
> > outlive the stack frame. Is there a reason why we can't just declare
> > data as:
> >
> > u8 data [4];
>
> data was __le32 in rsi_reset_chip() before commit f700546682a6 ("rsi:
> fix nommu_map_sg overflow kernel panic").
>
> I wonder if this would be okay for this function:
>
> -------------------------------------------------
>
> diff --git a/drivers/net/wireless/rsi/rsi_91x_sdio.c b/drivers/net/wireless/rsi/rsi_91x_sdio.c
> index f9c67ed473d1..0330c50ab99c 100644
> --- a/drivers/net/wireless/rsi/rsi_91x_sdio.c
> +++ b/drivers/net/wireless/rsi/rsi_91x_sdio.c
> @@ -927,7 +927,7 @@ static int rsi_sdio_ta_reset(struct rsi_hw *adapter)
> {
> int status;
> u32 addr;
> - u8 *data;
> + u8 data;
>
> status = rsi_sdio_master_access_msword(adapter, TA_BASE_ADDR);
> if (status < 0) {
> @@ -937,7 +937,7 @@ static int rsi_sdio_ta_reset(struct rsi_hw *adapter)
> }
>
> rsi_dbg(INIT_ZONE, "%s: Bring TA out of reset\n", __func__);
> - put_unaligned_le32(TA_HOLD_THREAD_VALUE, data);
> + put_unaligned_le32(TA_HOLD_THREAD_VALUE, &data);
> addr = TA_HOLD_THREAD_REG | RSI_SD_REQUEST_MASTER;
> status = rsi_sdio_write_register_multiple(adapter, addr,
> (u8 *)&data,
> @@ -947,7 +947,7 @@ static int rsi_sdio_ta_reset(struct rsi_hw *adapter)
> return status;
> }
>
> - put_unaligned_le32(TA_SOFT_RST_CLR, data);
> + put_unaligned_le32(TA_SOFT_RST_CLR, &data);
> addr = TA_SOFT_RESET_REG | RSI_SD_REQUEST_MASTER;
> status = rsi_sdio_write_register_multiple(adapter, addr,
> (u8 *)&data,
> @@ -957,7 +957,7 @@ static int rsi_sdio_ta_reset(struct rsi_hw *adapter)
> return status;
> }
>
> - put_unaligned_le32(TA_PC_ZERO, data);
> + put_unaligned_le32(TA_PC_ZERO, &data);
> addr = TA_TH0_PC_REG | RSI_SD_REQUEST_MASTER;
> status = rsi_sdio_write_register_multiple(adapter, addr,
> (u8 *)&data,
> @@ -967,7 +967,7 @@ static int rsi_sdio_ta_reset(struct rsi_hw *adapter)
> return -EINVAL;
> }
>
> - put_unaligned_le32(TA_RELEASE_THREAD_VALUE, data);
> + put_unaligned_le32(TA_RELEASE_THREAD_VALUE, &data);
> addr = TA_RELEASE_THREAD_REG | RSI_SD_REQUEST_MASTER;
> status = rsi_sdio_write_register_multiple(adapter, addr,
> (u8 *)&data,
>
>
> >
> > then use ARRAY_SIZE(data) or RSI_9116_REG_SIZE in rsi_reset_chip(),
> > getting rid of the kzalloc/kfree?
> >
> > (Sorry, I hate when a simple fixup becomes a "hey let's rewrite all
> > this code" thus becoming "that guy.")
>
> If we aren't actually improving the code, then why bother? :)
>
> Thank you for the review!
> Nathan
>
> > --
> > Thanks,
> > ~Nick Desaulniers
Hi all,
Did any of the maintainers have any comments on what the correct
solution is here to resolve this warning? It is one of the few left
before we can turn on -Wuninitialized for the whole kernel.
Thanks,
Nathan
^ permalink raw reply
* [PATCH AUTOSEL 5.1 047/375] mt76: remove mt76_queue dependency from tx_queue_skb function pointer
From: Sasha Levin @ 2019-05-22 19:15 UTC (permalink / raw)
To: linux-kernel, stable
Cc: Lorenzo Bianconi, Felix Fietkau, Sasha Levin, linux-wireless,
netdev
In-Reply-To: <20190522192115.22666-1-sashal@kernel.org>
From: Lorenzo Bianconi <lorenzo@kernel.org>
[ Upstream commit 89a37842b0c13c9e568bf12f4fcbe6507147e41d ]
Remove mt76_queue dependency from tx_queue_skb function pointer and
rely on mt76_tx_qid instead. This is a preliminary patch to introduce
mt76_sw_queue support
Signed-off-by: Lorenzo Bianconi <lorenzo@kernel.org>
Signed-off-by: Felix Fietkau <nbd@nbd.name>
Signed-off-by: Sasha Levin <sashal@kernel.org>
---
drivers/net/wireless/mediatek/mt76/dma.c | 3 ++-
drivers/net/wireless/mediatek/mt76/mt76.h | 4 ++--
drivers/net/wireless/mediatek/mt76/mt7603/beacon.c | 6 +++---
drivers/net/wireless/mediatek/mt76/mt76x02_mmio.c | 4 ++--
drivers/net/wireless/mediatek/mt76/tx.c | 10 +++++-----
drivers/net/wireless/mediatek/mt76/usb.c | 3 ++-
6 files changed, 16 insertions(+), 14 deletions(-)
diff --git a/drivers/net/wireless/mediatek/mt76/dma.c b/drivers/net/wireless/mediatek/mt76/dma.c
index 76629b98c78d7..8c7ee8302fb87 100644
--- a/drivers/net/wireless/mediatek/mt76/dma.c
+++ b/drivers/net/wireless/mediatek/mt76/dma.c
@@ -271,10 +271,11 @@ mt76_dma_tx_queue_skb_raw(struct mt76_dev *dev, enum mt76_txq_id qid,
return 0;
}
-int mt76_dma_tx_queue_skb(struct mt76_dev *dev, struct mt76_queue *q,
+int mt76_dma_tx_queue_skb(struct mt76_dev *dev, enum mt76_txq_id qid,
struct sk_buff *skb, struct mt76_wcid *wcid,
struct ieee80211_sta *sta)
{
+ struct mt76_queue *q = &dev->q_tx[qid];
struct mt76_queue_entry e;
struct mt76_txwi_cache *t;
struct mt76_queue_buf buf[32];
diff --git a/drivers/net/wireless/mediatek/mt76/mt76.h b/drivers/net/wireless/mediatek/mt76/mt76.h
index bcbfd3c4a44b6..eb882b2cbc0ec 100644
--- a/drivers/net/wireless/mediatek/mt76/mt76.h
+++ b/drivers/net/wireless/mediatek/mt76/mt76.h
@@ -156,7 +156,7 @@ struct mt76_queue_ops {
struct mt76_queue_buf *buf, int nbufs, u32 info,
struct sk_buff *skb, void *txwi);
- int (*tx_queue_skb)(struct mt76_dev *dev, struct mt76_queue *q,
+ int (*tx_queue_skb)(struct mt76_dev *dev, enum mt76_txq_id qid,
struct sk_buff *skb, struct mt76_wcid *wcid,
struct ieee80211_sta *sta);
@@ -645,7 +645,7 @@ static inline struct mt76_tx_cb *mt76_tx_skb_cb(struct sk_buff *skb)
return ((void *) IEEE80211_SKB_CB(skb)->status.status_driver_data);
}
-int mt76_dma_tx_queue_skb(struct mt76_dev *dev, struct mt76_queue *q,
+int mt76_dma_tx_queue_skb(struct mt76_dev *dev, enum mt76_txq_id qid,
struct sk_buff *skb, struct mt76_wcid *wcid,
struct ieee80211_sta *sta);
diff --git a/drivers/net/wireless/mediatek/mt76/mt7603/beacon.c b/drivers/net/wireless/mediatek/mt76/mt7603/beacon.c
index 4dcb465095d19..99c0a3ba37cb7 100644
--- a/drivers/net/wireless/mediatek/mt76/mt7603/beacon.c
+++ b/drivers/net/wireless/mediatek/mt76/mt7603/beacon.c
@@ -23,7 +23,7 @@ mt7603_update_beacon_iter(void *priv, u8 *mac, struct ieee80211_vif *vif)
if (!skb)
return;
- mt76_dma_tx_queue_skb(&dev->mt76, &dev->mt76.q_tx[MT_TXQ_BEACON], skb,
+ mt76_dma_tx_queue_skb(&dev->mt76, MT_TXQ_BEACON, skb,
&mvif->sta.wcid, NULL);
spin_lock_bh(&dev->ps_lock);
@@ -118,8 +118,8 @@ void mt7603_pre_tbtt_tasklet(unsigned long arg)
struct ieee80211_vif *vif = info->control.vif;
struct mt7603_vif *mvif = (struct mt7603_vif *)vif->drv_priv;
- mt76_dma_tx_queue_skb(&dev->mt76, q, skb, &mvif->sta.wcid,
- NULL);
+ mt76_dma_tx_queue_skb(&dev->mt76, MT_TXQ_CAB, skb,
+ &mvif->sta.wcid, NULL);
}
mt76_queue_kick(dev, q);
spin_unlock_bh(&q->lock);
diff --git a/drivers/net/wireless/mediatek/mt76/mt76x02_mmio.c b/drivers/net/wireless/mediatek/mt76/mt76x02_mmio.c
index daaed1220147e..952fe19cba9b6 100644
--- a/drivers/net/wireless/mediatek/mt76/mt76x02_mmio.c
+++ b/drivers/net/wireless/mediatek/mt76/mt76x02_mmio.c
@@ -146,8 +146,8 @@ static void mt76x02_pre_tbtt_tasklet(unsigned long arg)
struct ieee80211_vif *vif = info->control.vif;
struct mt76x02_vif *mvif = (struct mt76x02_vif *)vif->drv_priv;
- mt76_dma_tx_queue_skb(&dev->mt76, q, skb, &mvif->group_wcid,
- NULL);
+ mt76_dma_tx_queue_skb(&dev->mt76, MT_TXQ_PSD, skb,
+ &mvif->group_wcid, NULL);
}
spin_unlock_bh(&q->lock);
}
diff --git a/drivers/net/wireless/mediatek/mt76/tx.c b/drivers/net/wireless/mediatek/mt76/tx.c
index 2585df5123350..0c1036da9a92a 100644
--- a/drivers/net/wireless/mediatek/mt76/tx.c
+++ b/drivers/net/wireless/mediatek/mt76/tx.c
@@ -286,7 +286,7 @@ mt76_tx(struct mt76_dev *dev, struct ieee80211_sta *sta,
q = &dev->q_tx[qid];
spin_lock_bh(&q->lock);
- dev->queue_ops->tx_queue_skb(dev, q, skb, wcid, sta);
+ dev->queue_ops->tx_queue_skb(dev, qid, skb, wcid, sta);
dev->queue_ops->kick(dev, q);
if (q->queued > q->ndesc - 8 && !q->stopped) {
@@ -327,7 +327,6 @@ mt76_queue_ps_skb(struct mt76_dev *dev, struct ieee80211_sta *sta,
{
struct mt76_wcid *wcid = (struct mt76_wcid *) sta->drv_priv;
struct ieee80211_tx_info *info = IEEE80211_SKB_CB(skb);
- struct mt76_queue *hwq = &dev->q_tx[MT_TXQ_PSD];
info->control.flags |= IEEE80211_TX_CTRL_PS_RESPONSE;
if (last)
@@ -335,7 +334,7 @@ mt76_queue_ps_skb(struct mt76_dev *dev, struct ieee80211_sta *sta,
IEEE80211_TX_CTL_REQ_TX_STATUS;
mt76_skb_set_moredata(skb, !last);
- dev->queue_ops->tx_queue_skb(dev, hwq, skb, wcid, sta);
+ dev->queue_ops->tx_queue_skb(dev, MT_TXQ_PSD, skb, wcid, sta);
}
void
@@ -390,6 +389,7 @@ mt76_txq_send_burst(struct mt76_dev *dev, struct mt76_queue *hwq,
struct mt76_txq *mtxq, bool *empty)
{
struct ieee80211_txq *txq = mtxq_to_txq(mtxq);
+ enum mt76_txq_id qid = mt76_txq_get_qid(txq);
struct ieee80211_tx_info *info;
struct mt76_wcid *wcid = mtxq->wcid;
struct sk_buff *skb;
@@ -423,7 +423,7 @@ mt76_txq_send_burst(struct mt76_dev *dev, struct mt76_queue *hwq,
if (ampdu)
mt76_check_agg_ssn(mtxq, skb);
- idx = dev->queue_ops->tx_queue_skb(dev, hwq, skb, wcid, txq->sta);
+ idx = dev->queue_ops->tx_queue_skb(dev, qid, skb, wcid, txq->sta);
if (idx < 0)
return idx;
@@ -458,7 +458,7 @@ mt76_txq_send_burst(struct mt76_dev *dev, struct mt76_queue *hwq,
if (cur_ampdu)
mt76_check_agg_ssn(mtxq, skb);
- idx = dev->queue_ops->tx_queue_skb(dev, hwq, skb, wcid,
+ idx = dev->queue_ops->tx_queue_skb(dev, qid, skb, wcid,
txq->sta);
if (idx < 0)
return idx;
diff --git a/drivers/net/wireless/mediatek/mt76/usb.c b/drivers/net/wireless/mediatek/mt76/usb.c
index 4c1abd4924054..b1551419338f0 100644
--- a/drivers/net/wireless/mediatek/mt76/usb.c
+++ b/drivers/net/wireless/mediatek/mt76/usb.c
@@ -726,10 +726,11 @@ mt76u_tx_build_sg(struct mt76_dev *dev, struct sk_buff *skb,
}
static int
-mt76u_tx_queue_skb(struct mt76_dev *dev, struct mt76_queue *q,
+mt76u_tx_queue_skb(struct mt76_dev *dev, enum mt76_txq_id qid,
struct sk_buff *skb, struct mt76_wcid *wcid,
struct ieee80211_sta *sta)
{
+ struct mt76_queue *q = &dev->q_tx[qid];
struct mt76u_buf *buf;
u16 idx = q->tail;
int err;
--
2.20.1
^ permalink raw reply related
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