* Re: [PATCH] broadcom/brcm80211/brcmfmac/cfg80211 driver, bad regulatory domain frequency value
From: Arend Van Spriel @ 2016-11-16 10:38 UTC (permalink / raw)
To: Kalle Valo, Gianfranco Costamagna
Cc: brcm80211-dev-list@broadcom.com, linux-wireless@vger.kernel.org,
nsmaldone@tierratelematics.com, Marco.Arlone@roj.com
In-Reply-To: <87r36dov6u.fsf@purkki.adurom.net>
On 15-11-2016 12:10, Kalle Valo wrote:
> Gianfranco Costamagna <locutusofborg@debian.org> writes:
>
>>> Well, not before you pointed it out ;-). You are welcome to send a patch
>>> fixing it. Otherwise, I will take care of it.
>>
>> attaching a format-patch like version.
>> I don't think we need a Tested-by or whatever, because it is just a typo in a comment.
>> (this is my first contribution, feel free to rebase or change whatever you prefer
>> to make it in line with other styles)
>>
>> (I gave authorship to Marco, the first one who discovered such typo)
>
> [...]
>
>> Signed-off-by: Gianfranco Costamagna <gianfranco.costamagna@abinsula.com>
>> Signed-off-by: Nicola Smaldone <nicola.smaldone@tierraservice.com>
>> Signed-off-by: Arlone Marco <marco.arlone@roj.com>
>
> Please note that you cannot add Signed-off-by for other people without
> their explicit approval (see Documentation/SubmittingPatches). I don't
> know if they did it in this case or not, but wanted to point out this
> anyway.
Thanks, Kalle
It was on my list to give a contrib boot camp here. So let's go over
things here albeit it late.
Hi Gianfrance,
A couple of things from which you may benefit for future contributions.
Basic info can be found in Documentation/SubmittingPatches as Kalle
indicated.
Patches should not be submitted as attachments. After doing 'git
format-patch' the easiest way is using 'git send-email' (which requires
you to have sendmail on your system properly setup). I tend to send
patches to Kalle and Cc: linux-wireless, but that is my personal
preference. Sending it to the list should be sufficient as patchwork
will pick it up [1].
When it comes to attribution of others you should have their approval
for Signed-off-by. However, reading your remarks it seems a proper way
is below:
Reported-by: Arlone Marco <marco.arlone@roj.com>
Tested-by: Nicola Smaldone <nicola.smaldone@tierraservice.com>
Signed-off-by: Gianfranco Costamagna <gianfranco.costamagna@abinsula.com>
Regards,
Arend
[1] https://patchwork.kernel.org/project/linux-wireless/list/
^ permalink raw reply
* [PATCH v2 04/10] rt2800: do not overwrite WPDMA_GLO_CFG_WP_DMA_BURST_SIZE
From: Stanislaw Gruszka @ 2016-11-16 11:53 UTC (permalink / raw)
To: linux-wireless; +Cc: Helmut Schaa, Mathias Kresin
In-Reply-To: <1478095865-8651-5-git-send-email-sgruszka@redhat.com>
We already initlized WPDMA_GLO_CFG_WP_DMA_BURST_SIZE to 3 on
rt2800_init_registers() for USB devices. For PCI devices we will use
HW default setting, which is 2, so patch does not change behaviour
on PCI devices.
Signed-off-by: Stanislaw Gruszka <sgruszka@redhat.com>
---
v1 -> v2 Changelog fixes
drivers/net/wireless/ralink/rt2x00/rt2800lib.c | 1 -
1 files changed, 0 insertions(+), 1 deletions(-)
diff --git a/drivers/net/wireless/ralink/rt2x00/rt2800lib.c b/drivers/net/wireless/ralink/rt2x00/rt2800lib.c
index feceb13..9ecdc4c 100644
--- a/drivers/net/wireless/ralink/rt2x00/rt2800lib.c
+++ b/drivers/net/wireless/ralink/rt2x00/rt2800lib.c
@@ -6756,7 +6756,6 @@ int rt2800_enable_radio(struct rt2x00_dev *rt2x00dev)
rt2800_register_read(rt2x00dev, WPDMA_GLO_CFG, ®);
rt2x00_set_field32(®, WPDMA_GLO_CFG_ENABLE_TX_DMA, 1);
rt2x00_set_field32(®, WPDMA_GLO_CFG_ENABLE_RX_DMA, 1);
- rt2x00_set_field32(®, WPDMA_GLO_CFG_WP_DMA_BURST_SIZE, 2);
rt2x00_set_field32(®, WPDMA_GLO_CFG_TX_WRITEBACK_DONE, 1);
rt2800_register_write(rt2x00dev, WPDMA_GLO_CFG, reg);
--
1.7.1
^ permalink raw reply related
* Re: [PATCH 05/10] rt2800: make ba_size depend on ampdu_factor
From: Stanislaw Gruszka @ 2016-11-16 12:02 UTC (permalink / raw)
To: Mathias Kresin; +Cc: linux-wireless, Helmut Schaa, Felix Fietkau
In-Reply-To: <e4dfaa30-7dd6-c485-3e30-71bfafbc1502@kresin.me>
On Wed, Nov 16, 2016 at 09:07:00AM +0100, Mathias Kresin wrote:
> Here are the results of the requested tests. Please keep in mind, I'm not in
> a lab environment:
>
> LEDE head
> connect
> action 0 sta 9c:f3:87:bc:AA:BB tid 6 buf_size 64 ampdu_factor 3
> action 2 sta 9c:f3:87:bc:AA:BB tid 0 buf_size 0 ampdu_factor 3
> action 6 sta 9c:f3:87:bc:AA:BB tid 0 buf_size 64 ampdu_factor 3
> action 0 sta 9c:f3:87:bc:AA:BB tid 0 buf_size 64 ampdu_factor 3
No problem here - buf_size corresponds to ampdu_factor.
> ieee80211 phy0: rt2x00queue_flush_queue: Warning - Queue 2 failed
> to flush
> ieee80211 phy0: rt2x00queue_flush_queue: Warning - Queue 2 failed
> to flush
I think we do not give device enough time to post AMPDU consisted
with bigger amount of frames. If we want to increase ba_size we will
need also some other changes in the driver. Anyway I already request
Kalle to drop this patch. I assume other patches do not cause
regression for you, correct?
Thanks for testing.
Stanislaw
^ permalink raw reply
* Re: [PATCH 0/8] iw: Add common chandef parser and new DFS related commands
From: Johannes Berg @ 2016-11-16 12:41 UTC (permalink / raw)
To: Benjamin Berg; +Cc: linux-wireless, sw, Benjamin Berg
In-Reply-To: <20161107145943.16761-1-benjamin@sipsolutions.net>
On Mon, 2016-11-07 at 15:59 +0100, Benjamin Berg wrote:
> From: Benjamin Berg <benjamin.berg@open-mesh.com>
>
> Hi,
>
> This patchset adds commands for doing a CAC and sending CSA.
>
> Another change is that parsing of command line frequency and channel
> definitions is split out so that it can be shared between the
> different users.
That's nice :)
I've applied all except for patch 7 - I don't see any justification for
adding that - quite clearly iw will not listen to any radar detection
events, so even for testing/certification this doesn't seem useful
since then you'd want to check/prove that you *do* listen to those ...
johannes
^ permalink raw reply
* RE: [PATCH 5/5] mwifiex: wait for firmware dump completion in remove_card
From: Amitkumar Karwar @ 2016-11-16 13:07 UTC (permalink / raw)
To: Brian Norris
Cc: Kalle Valo, Dmitry Torokhov, linux-wireless@vger.kernel.org,
Cathy Luo, Nishant Sarmukadam, Xinming Hu
In-Reply-To: <20161109203710.GA118631@google.com>
Hi Brian,
> From: Brian Norris [mailto:briannorris@chromium.org]
> Sent: Thursday, November 10, 2016 2:07 AM
> To: Amitkumar Karwar
> Cc: Kalle Valo; Dmitry Torokhov; linux-wireless@vger.kernel.org; Cathy
> Luo; Nishant Sarmukadam; Xinming Hu
> Subject: Re: [PATCH 5/5] mwifiex: wait for firmware dump completion in
> remove_card
>
> On Wed, Nov 09, 2016 at 12:35:22PM +0000, Amitkumar Karwar wrote:
> > Hi Brian,
>
> Hi,
>
> > > From: Brian Norris [mailto:briannorris@chromium.org]
> > > Sent: Thursday, November 03, 2016 2:16 AM
> > > To: Kalle Valo
> > > Cc: Dmitry Torokhov; Amitkumar Karwar;
> > > linux-wireless@vger.kernel.org; Cathy Luo; Nishant Sarmukadam;
> > > Xinming Hu
> > > Subject: Re: [PATCH 5/5] mwifiex: wait for firmware dump completion
> > > in remove_card
> > >
> > > On Thu, Oct 27, 2016 at 04:20:25PM +0300, Kalle Valo wrote:
> > > > Dmitry Torokhov <dmitry.torokhov@gmail.com> writes:
> > > >
> > > > >> +/* reset_trigger variable is used to identify if
> > > > >> +mwifiex_sdio_remove()
> > > > >> + * is called by sdio_work during reset or the call is from
> > > > >> +sdio
> > > subsystem.
> > > > >> + * We will cancel sdio_work only if the call is from sdio
> > > subsystem.
> > > > >> + */
> > > > >> +static u8 reset_triggered;
> > > > >
> > > > > It would be really great if the driver supported multiple
> devices.
> > > > > IOW please avoid module-globals.
> > > >
> > > > Good catch, it's a hard requirement to support multiple devices
> at
> > > > the same time.
> > >
> > > BTW, this problem is repeated in several places throughout this
> driver.
> > > For instance, look for 'user_rmmod' (why? you shouldn't need to
> > > treat module unload differently...)
> >
> > We have 2 kinds of teardown cases.
> > 1) Chip is going to be powered off.
> > a) System reboot
> > b) Someone manually removed wifi card from system
> > 2) User unloaded the driver.
> >
> > In case 1. b), we can't have logic to terminate WIFI connection and
> > download SHUTDOWN command to firmware, as hardware itself is not
> > present.
>
> That seems like a poor way of determining the difference between hot
> unplug and clean shutdown. What if unplug happens concurrently with
> rmmod? Seems like it'd be better to identify hardware failures as such,
> and just skip talking to HW. Also, for interfaces that support unplug
> well (like USB), shouldn't you be able to get hotplug info from the
> driver core, instead of having to guess the inverse of that ("this was
> not a hotplug") from static variables like that?
>
> > This logic is useful when user just unloads and loads the driver.
> > Firmware download will be skipped in this case, as it's already
> > running. SHUTDOWN command sent during unload has cleared firmware's
> > state.
>
> Why is that useful?
>
> > 'user_rmmod' flag doesn't create problem for supporting multiple
> > devices. The flag is true during module unload OR reboot. It's
> > applicable for all devices.
> >
> > > and the work structs (and corresponding 'saved_adapter' and
> > > 'iface_flags') used for PCIe function-level reset and SDIO reset.
> >
> > We are working on the v3 of this patch series. We will try to get rid
> > of these variables along with global "work_struct" as you suggested.
I observed some crash issues while testing with PCIe/SDIO chipsets after removing user_rmmod. We are still checking them. I will not include user_rmmod removal patch in v3 series. Other pcie_work related global variables are removed in v3 series.
Card is freed and recreated during mwifiex_sdio_card_reset_work(). It is one of the activities in sdio_work. So moving sdio_work inside card won't be straight forward.
Regards,
Amitkumar
^ permalink raw reply
* RE: [PATCH v2 1/5] mwifiex: remove redundant condition in main process
From: Amitkumar Karwar @ 2016-11-16 13:08 UTC (permalink / raw)
To: Brian Norris, Kalle Valo
Cc: Xinming Hu, linux-wireless@vger.kernel.org, Cathy Luo,
Nishant Sarmukadam, rajatja@google.com, dmitry.torokhov@gmail.com
In-Reply-To: <20161110194646.GA85922@google.com>
Hi Brian,
> From: Brian Norris [mailto:briannorris@chromium.org]
> Sent: Friday, November 11, 2016 1:17 AM
> To: Kalle Valo
> Cc: Xinming Hu; Amitkumar Karwar; linux-wireless@vger.kernel.org; Cathy
> Luo; Nishant Sarmukadam; rajatja@google.com; dmitry.torokhov@gmail.com
> Subject: Re: [PATCH v2 1/5] mwifiex: remove redundant condition in main
> process
>
> On Mon, Nov 07, 2016 at 08:46:41PM +0200, Kalle Valo wrote:
> > Xinming Hu <huxm@marvell.com> writes:
> >
> > > We have include below change in latest submit
> > > https://patchwork.kernel.org/patch/9407283/
> > > Please drop this patch.
> >
> > When making changes please resend the whole patchset. I do not want
> to
> > individually pick patches from different places, that will eventually
> > go wrong anyway. So I just apply full patchsets.
>
> I'm going to assume that Xinming intended that the entire series be
> resent, possibly without this patch. There were enough other issues in
> the series anyway.
I will submit v3 series shortly.
Regards,
Amitkumar
^ permalink raw reply
* [PATCH v3 1/5] mwifiex: don't wait for main_process in shutdown_drv
From: Amitkumar Karwar @ 2016-11-16 13:09 UTC (permalink / raw)
To: linux-wireless
Cc: Cathy Luo, Nishant Sarmukadam, rajatja, briannorris,
dmitry.torokhov, Xinming Hu, Amitkumar Karwar
From: Xinming Hu <huxm@marvell.com>
main_process is not expected to be running when shutdown_drv function
is called. currently we wait for main_process completion in the
function.
Actually the caller has already made sure main_process is completed by
performing below actions.
(1) disable interrupts in if_ops->disable_int.
(2) set adapter->surprise_removed = true, main_process wont be queued.
(3) mwifiex_terminate_workqueue(adapter), wait for workqueue to be
completed.
This patch removes redundant wait code and takes care of related
cleanup.
Signed-off-by: Xinming Hu <huxm@marvell.com>
Signed-off-by: Amitkumar Karwar <akarwar@marvell.com>
---
v3: a) This patch is introduced in v3. It replaces "[v2,2/5] mwifiex: use spinlock for
'mwifiex_processing' in shutdown_drv" patch.
b) "[v2,1/5] mwifiex: remove redundant condition in main process" is dropped in
this patch series.
---
drivers/net/wireless/marvell/mwifiex/init.c | 19 ++--------
drivers/net/wireless/marvell/mwifiex/main.c | 55 ++++++++++-------------------
drivers/net/wireless/marvell/mwifiex/main.h | 5 +--
drivers/net/wireless/marvell/mwifiex/util.c | 15 --------
4 files changed, 22 insertions(+), 72 deletions(-)
diff --git a/drivers/net/wireless/marvell/mwifiex/init.c b/drivers/net/wireless/marvell/mwifiex/init.c
index b36cb3f..f581a15 100644
--- a/drivers/net/wireless/marvell/mwifiex/init.c
+++ b/drivers/net/wireless/marvell/mwifiex/init.c
@@ -656,10 +656,9 @@ void mwifiex_free_priv(struct mwifiex_private *priv)
* - Free the adapter
* - Notify completion
*/
-int
+void
mwifiex_shutdown_drv(struct mwifiex_adapter *adapter)
{
- int ret = -EINPROGRESS;
struct mwifiex_private *priv;
s32 i;
unsigned long flags;
@@ -667,15 +666,7 @@ void mwifiex_free_priv(struct mwifiex_private *priv)
/* mwifiex already shutdown */
if (adapter->hw_status == MWIFIEX_HW_STATUS_NOT_READY)
- return 0;
-
- adapter->hw_status = MWIFIEX_HW_STATUS_CLOSING;
- /* wait for mwifiex_process to complete */
- if (adapter->mwifiex_processing) {
- mwifiex_dbg(adapter, WARN,
- "main process is still running\n");
- return ret;
- }
+ return;
/* cancel current command */
if (adapter->curr_cmd) {
@@ -726,11 +717,7 @@ void mwifiex_free_priv(struct mwifiex_private *priv)
mwifiex_adapter_cleanup(adapter);
spin_unlock(&adapter->mwifiex_lock);
-
- /* Notify completion */
- ret = mwifiex_shutdown_fw_complete(adapter);
-
- return ret;
+ adapter->hw_status = MWIFIEX_HW_STATUS_NOT_READY;
}
/*
diff --git a/drivers/net/wireless/marvell/mwifiex/main.c b/drivers/net/wireless/marvell/mwifiex/main.c
index eac44fe..379e084 100644
--- a/drivers/net/wireless/marvell/mwifiex/main.c
+++ b/drivers/net/wireless/marvell/mwifiex/main.c
@@ -248,15 +248,14 @@ int mwifiex_main_process(struct mwifiex_adapter *adapter)
if (adapter->mwifiex_processing || adapter->main_locked) {
adapter->more_task_flag = true;
spin_unlock_irqrestore(&adapter->main_proc_lock, flags);
- goto exit_main_proc;
+ return 0;
} else {
adapter->mwifiex_processing = true;
spin_unlock_irqrestore(&adapter->main_proc_lock, flags);
}
process_start:
do {
- if ((adapter->hw_status == MWIFIEX_HW_STATUS_CLOSING) ||
- (adapter->hw_status == MWIFIEX_HW_STATUS_NOT_READY))
+ if (adapter->hw_status == MWIFIEX_HW_STATUS_NOT_READY)
break;
/* For non-USB interfaces, If we process interrupts first, it
@@ -464,9 +463,6 @@ int mwifiex_main_process(struct mwifiex_adapter *adapter)
adapter->mwifiex_processing = false;
spin_unlock_irqrestore(&adapter->main_proc_lock, flags);
-exit_main_proc:
- if (adapter->hw_status == MWIFIEX_HW_STATUS_CLOSING)
- mwifiex_shutdown_drv(adapter);
return ret;
}
EXPORT_SYMBOL_GPL(mwifiex_main_process);
@@ -648,16 +644,14 @@ static void mwifiex_fw_dpc(const struct firmware *firmware, void *context)
if (adapter->if_ops.unregister_dev)
adapter->if_ops.unregister_dev(adapter);
+ adapter->surprise_removed = true;
+ mwifiex_terminate_workqueue(adapter);
+
if (adapter->hw_status == MWIFIEX_HW_STATUS_READY) {
pr_debug("info: %s: shutdown mwifiex\n", __func__);
- adapter->init_wait_q_woken = false;
-
- if (mwifiex_shutdown_drv(adapter) == -EINPROGRESS)
- wait_event_interruptible(adapter->init_wait_q,
- adapter->init_wait_q_woken);
+ mwifiex_shutdown_drv(adapter);
}
- adapter->surprise_removed = true;
- mwifiex_terminate_workqueue(adapter);
+
init_failed = true;
done:
if (adapter->cal_data) {
@@ -1402,11 +1396,8 @@ static void mwifiex_main_work_queue(struct work_struct *work)
}
mwifiex_dbg(adapter, CMD, "cmd: calling mwifiex_shutdown_drv...\n");
- adapter->init_wait_q_woken = false;
- if (mwifiex_shutdown_drv(adapter) == -EINPROGRESS)
- wait_event_interruptible(adapter->init_wait_q,
- adapter->init_wait_q_woken);
+ mwifiex_shutdown_drv(adapter);
if (adapter->if_ops.down_dev)
adapter->if_ops.down_dev(adapter);
@@ -1512,19 +1503,16 @@ static void mwifiex_main_work_queue(struct work_struct *work)
mwifiex_dbg(adapter, ERROR, "info: %s: unregister device\n", __func__);
if (adapter->if_ops.unregister_dev)
adapter->if_ops.unregister_dev(adapter);
+
+err_kmalloc:
+ adapter->surprise_removed = true;
+ mwifiex_terminate_workqueue(adapter);
if (adapter->hw_status == MWIFIEX_HW_STATUS_READY) {
mwifiex_dbg(adapter, ERROR,
"info: %s: shutdown mwifiex\n", __func__);
- adapter->init_wait_q_woken = false;
-
- if (mwifiex_shutdown_drv(adapter) == -EINPROGRESS)
- wait_event_interruptible(adapter->init_wait_q,
- adapter->init_wait_q_woken);
+ mwifiex_shutdown_drv(adapter);
}
-err_kmalloc:
- mwifiex_terminate_workqueue(adapter);
- adapter->surprise_removed = true;
complete_all(adapter->fw_done);
mwifiex_dbg(adapter, INFO, "%s, error\n", __func__);
@@ -1684,17 +1672,13 @@ static void mwifiex_probe_of(struct mwifiex_adapter *adapter)
pr_debug("info: %s: unregister device\n", __func__);
if (adapter->if_ops.unregister_dev)
adapter->if_ops.unregister_dev(adapter);
- if (adapter->hw_status == MWIFIEX_HW_STATUS_READY) {
- pr_debug("info: %s: shutdown mwifiex\n", __func__);
- adapter->init_wait_q_woken = false;
-
- if (mwifiex_shutdown_drv(adapter) == -EINPROGRESS)
- wait_event_interruptible(adapter->init_wait_q,
- adapter->init_wait_q_woken);
- }
err_registerdev:
adapter->surprise_removed = true;
mwifiex_terminate_workqueue(adapter);
+ if (adapter->hw_status == MWIFIEX_HW_STATUS_READY) {
+ pr_debug("info: %s: shutdown mwifiex\n", __func__);
+ mwifiex_shutdown_drv(adapter);
+ }
err_kmalloc:
mwifiex_free_adapter(adapter);
@@ -1744,11 +1728,8 @@ int mwifiex_remove_card(struct mwifiex_adapter *adapter)
mwifiex_dbg(adapter, CMD,
"cmd: calling mwifiex_shutdown_drv...\n");
- adapter->init_wait_q_woken = false;
- if (mwifiex_shutdown_drv(adapter) == -EINPROGRESS)
- wait_event_interruptible(adapter->init_wait_q,
- adapter->init_wait_q_woken);
+ mwifiex_shutdown_drv(adapter);
mwifiex_dbg(adapter, CMD,
"cmd: mwifiex_shutdown_drv done\n");
if (atomic_read(&adapter->rx_pending) ||
diff --git a/drivers/net/wireless/marvell/mwifiex/main.h b/drivers/net/wireless/marvell/mwifiex/main.h
index 5c9bd94..fd56b5d 100644
--- a/drivers/net/wireless/marvell/mwifiex/main.h
+++ b/drivers/net/wireless/marvell/mwifiex/main.h
@@ -248,7 +248,6 @@ enum MWIFIEX_HARDWARE_STATUS {
MWIFIEX_HW_STATUS_INITIALIZING,
MWIFIEX_HW_STATUS_INIT_DONE,
MWIFIEX_HW_STATUS_RESET,
- MWIFIEX_HW_STATUS_CLOSING,
MWIFIEX_HW_STATUS_NOT_READY
};
@@ -1041,9 +1040,7 @@ void mwifiex_wake_up_net_dev_queue(struct net_device *netdev,
int mwifiex_init_fw_complete(struct mwifiex_adapter *adapter);
-int mwifiex_shutdown_drv(struct mwifiex_adapter *adapter);
-
-int mwifiex_shutdown_fw_complete(struct mwifiex_adapter *adapter);
+void mwifiex_shutdown_drv(struct mwifiex_adapter *adapter);
int mwifiex_dnld_fw(struct mwifiex_adapter *, struct mwifiex_fw_image *);
diff --git a/drivers/net/wireless/marvell/mwifiex/util.c b/drivers/net/wireless/marvell/mwifiex/util.c
index 18fbb96..b1ab8da 100644
--- a/drivers/net/wireless/marvell/mwifiex/util.c
+++ b/drivers/net/wireless/marvell/mwifiex/util.c
@@ -146,21 +146,6 @@ int mwifiex_init_fw_complete(struct mwifiex_adapter *adapter)
}
/*
- * Firmware shutdown complete callback handler.
- *
- * This function sets the hardware status to not ready and wakes up
- * the function waiting on the init wait queue for the firmware
- * shutdown to complete.
- */
-int mwifiex_shutdown_fw_complete(struct mwifiex_adapter *adapter)
-{
- adapter->hw_status = MWIFIEX_HW_STATUS_NOT_READY;
- adapter->init_wait_q_woken = true;
- wake_up_interruptible(&adapter->init_wait_q);
- return 0;
-}
-
-/*
* This function sends init/shutdown command
* to firmware.
*/
--
1.9.1
^ permalink raw reply related
* [PATCH v3 2/5] mwifiex: do not free firmware dump memory in shutdown_drv
From: Amitkumar Karwar @ 2016-11-16 13:09 UTC (permalink / raw)
To: linux-wireless
Cc: Cathy Luo, Nishant Sarmukadam, rajatja, briannorris,
dmitry.torokhov, Xinming Hu, Amitkumar Karwar
In-Reply-To: <1479301749-14803-1-git-send-email-akarwar@marvell.com>
From: Xinming Hu <huxm@marvell.com>
mwifiex_upload_device_dump() already takes care of freeing firmware dump
memory. Doing the same thing in mwifiex_shutdown_drv() is redundant.
Signed-off-by: Xinming Hu <huxm@marvell.com>
Signed-off-by: Amitkumar Karwar <akarwar@marvell.com>
---
v2: Same as v1.
drv_info_dump and drv_info_size need not be in adapter structure. Saparate
patch is created for this (Dmitry Torokhov)
v3: Same as [v2,3/5]
---
drivers/net/wireless/marvell/mwifiex/init.c | 19 -------------------
1 file changed, 19 deletions(-)
diff --git a/drivers/net/wireless/marvell/mwifiex/init.c b/drivers/net/wireless/marvell/mwifiex/init.c
index f581a15..87cda4f 100644
--- a/drivers/net/wireless/marvell/mwifiex/init.c
+++ b/drivers/net/wireless/marvell/mwifiex/init.c
@@ -408,8 +408,6 @@ static void mwifiex_free_lock_list(struct mwifiex_adapter *adapter)
static void
mwifiex_adapter_cleanup(struct mwifiex_adapter *adapter)
{
- int idx;
-
if (!adapter) {
pr_err("%s: adapter is NULL\n", __func__);
return;
@@ -427,23 +425,6 @@ static void mwifiex_free_lock_list(struct mwifiex_adapter *adapter)
mwifiex_dbg(adapter, INFO, "info: free cmd buffer\n");
mwifiex_free_cmd_buffer(adapter);
- for (idx = 0; idx < adapter->num_mem_types; idx++) {
- struct memory_type_mapping *entry =
- &adapter->mem_type_mapping_tbl[idx];
-
- if (entry->mem_ptr) {
- vfree(entry->mem_ptr);
- entry->mem_ptr = NULL;
- }
- entry->mem_size = 0;
- }
-
- if (adapter->drv_info_dump) {
- vfree(adapter->drv_info_dump);
- adapter->drv_info_dump = NULL;
- adapter->drv_info_size = 0;
- }
-
if (adapter->sleep_cfm)
dev_kfree_skb_any(adapter->sleep_cfm);
}
--
1.9.1
^ permalink raw reply related
* [PATCH v3 3/5] mwifiex: get rid of drv_info* adapter variables
From: Amitkumar Karwar @ 2016-11-16 13:09 UTC (permalink / raw)
To: linux-wireless
Cc: Cathy Luo, Nishant Sarmukadam, rajatja, briannorris,
dmitry.torokhov, Xinming Hu, Amitkumar Karwar
In-Reply-To: <1479301749-14803-1-git-send-email-akarwar@marvell.com>
From: Xinming Hu <huxm@marvell.com>
We can avoid drv_info_dump and drv_info_size adapter variables.
This info can be passed to mwifiex_upload_device_dump() as parameters
Signed-off-by: Xinming Hu <huxm@marvell.com>
Signed-off-by: Amitkumar Karwar <akarwar@marvell.com>
---
v2: This patch is introduced in v2.
Also, I have dropped "v1 4/5 mwifiex: firmware dump code rearrangement
in pcie.c" patch in this series to avoid rebase efforts for other patches
from Rajat/Brian in queue. I will submit this later.
v3: same as [v2,4/5]
---
drivers/net/wireless/marvell/mwifiex/main.c | 42 ++++++++++++-----------------
drivers/net/wireless/marvell/mwifiex/main.h | 7 +++--
drivers/net/wireless/marvell/mwifiex/pcie.c | 7 +++--
drivers/net/wireless/marvell/mwifiex/sdio.c | 6 +++--
4 files changed, 29 insertions(+), 33 deletions(-)
diff --git a/drivers/net/wireless/marvell/mwifiex/main.c b/drivers/net/wireless/marvell/mwifiex/main.c
index 379e084..55ac0cf 100644
--- a/drivers/net/wireless/marvell/mwifiex/main.c
+++ b/drivers/net/wireless/marvell/mwifiex/main.c
@@ -1029,7 +1029,7 @@ void mwifiex_multi_chan_resync(struct mwifiex_adapter *adapter)
}
EXPORT_SYMBOL_GPL(mwifiex_multi_chan_resync);
-void mwifiex_drv_info_dump(struct mwifiex_adapter *adapter)
+int mwifiex_drv_info_dump(struct mwifiex_adapter *adapter, void **drv_info)
{
void *p;
char drv_version[64];
@@ -1039,21 +1039,17 @@ void mwifiex_drv_info_dump(struct mwifiex_adapter *adapter)
int i, idx;
struct netdev_queue *txq;
struct mwifiex_debug_info *debug_info;
-
- if (adapter->drv_info_dump) {
- vfree(adapter->drv_info_dump);
- adapter->drv_info_dump = NULL;
- adapter->drv_info_size = 0;
- }
+ void *drv_info_dump;
mwifiex_dbg(adapter, MSG, "===mwifiex driverinfo dump start===\n");
- adapter->drv_info_dump = vzalloc(MWIFIEX_DRV_INFO_SIZE_MAX);
+ /* memory allocate here should be free in mwifiex_upload_device_dump*/
+ drv_info_dump = vzalloc(MWIFIEX_DRV_INFO_SIZE_MAX);
- if (!adapter->drv_info_dump)
- return;
+ if (!drv_info_dump)
+ return 0;
- p = (char *)(adapter->drv_info_dump);
+ p = (char *)(drv_info_dump);
p += sprintf(p, "driver_name = " "\"mwifiex\"\n");
mwifiex_drv_get_driver_version(adapter, drv_version,
@@ -1137,18 +1133,20 @@ void mwifiex_drv_info_dump(struct mwifiex_adapter *adapter)
kfree(debug_info);
}
- adapter->drv_info_size = p - adapter->drv_info_dump;
mwifiex_dbg(adapter, MSG, "===mwifiex driverinfo dump end===\n");
+ *drv_info = drv_info_dump;
+ return p - drv_info_dump;
}
EXPORT_SYMBOL_GPL(mwifiex_drv_info_dump);
-void mwifiex_upload_device_dump(struct mwifiex_adapter *adapter)
+void mwifiex_upload_device_dump(struct mwifiex_adapter *adapter, void *drv_info,
+ int drv_info_size)
{
u8 idx, *dump_data, *fw_dump_ptr;
u32 dump_len;
dump_len = (strlen("========Start dump driverinfo========\n") +
- adapter->drv_info_size +
+ drv_info_size +
strlen("\n========End dump========\n"));
for (idx = 0; idx < adapter->num_mem_types; idx++) {
@@ -1178,8 +1176,8 @@ void mwifiex_upload_device_dump(struct mwifiex_adapter *adapter)
strcpy(fw_dump_ptr, "========Start dump driverinfo========\n");
fw_dump_ptr += strlen("========Start dump driverinfo========\n");
- memcpy(fw_dump_ptr, adapter->drv_info_dump, adapter->drv_info_size);
- fw_dump_ptr += adapter->drv_info_size;
+ memcpy(fw_dump_ptr, drv_info, drv_info_size);
+ fw_dump_ptr += drv_info_size;
strcpy(fw_dump_ptr, "\n========End dump========\n");
fw_dump_ptr += strlen("\n========End dump========\n");
@@ -1217,18 +1215,12 @@ void mwifiex_upload_device_dump(struct mwifiex_adapter *adapter)
struct memory_type_mapping *entry =
&adapter->mem_type_mapping_tbl[idx];
- if (entry->mem_ptr) {
- vfree(entry->mem_ptr);
- entry->mem_ptr = NULL;
- }
+ vfree(entry->mem_ptr);
+ entry->mem_ptr = NULL;
entry->mem_size = 0;
}
- if (adapter->drv_info_dump) {
- vfree(adapter->drv_info_dump);
- adapter->drv_info_dump = NULL;
- adapter->drv_info_size = 0;
- }
+ vfree(drv_info);
}
EXPORT_SYMBOL_GPL(mwifiex_upload_device_dump);
diff --git a/drivers/net/wireless/marvell/mwifiex/main.h b/drivers/net/wireless/marvell/mwifiex/main.h
index fd56b5d..d501d03 100644
--- a/drivers/net/wireless/marvell/mwifiex/main.h
+++ b/drivers/net/wireless/marvell/mwifiex/main.h
@@ -994,8 +994,6 @@ struct mwifiex_adapter {
u8 key_api_major_ver, key_api_minor_ver;
struct memory_type_mapping *mem_type_mapping_tbl;
u8 num_mem_types;
- void *drv_info_dump;
- u32 drv_info_size;
bool scan_chan_gap_enabled;
struct sk_buff_head rx_data_q;
bool mfg_mode;
@@ -1641,8 +1639,9 @@ void mwifiex_hist_data_add(struct mwifiex_private *priv,
u8 mwifiex_adjust_data_rate(struct mwifiex_private *priv,
u8 rx_rate, u8 ht_info);
-void mwifiex_drv_info_dump(struct mwifiex_adapter *adapter);
-void mwifiex_upload_device_dump(struct mwifiex_adapter *adapter);
+int mwifiex_drv_info_dump(struct mwifiex_adapter *adapter, void **drv_info);
+void mwifiex_upload_device_dump(struct mwifiex_adapter *adapter, void *drv_info,
+ int drv_info_size);
void *mwifiex_alloc_dma_align_buf(int rx_len, gfp_t flags);
void mwifiex_queue_main_work(struct mwifiex_adapter *adapter);
int mwifiex_get_wakeup_reason(struct mwifiex_private *priv, u16 action,
diff --git a/drivers/net/wireless/marvell/mwifiex/pcie.c b/drivers/net/wireless/marvell/mwifiex/pcie.c
index e4f1f55..dd8f7aa 100644
--- a/drivers/net/wireless/marvell/mwifiex/pcie.c
+++ b/drivers/net/wireless/marvell/mwifiex/pcie.c
@@ -2705,9 +2705,12 @@ static void mwifiex_pcie_fw_dump(struct mwifiex_adapter *adapter)
static void mwifiex_pcie_device_dump_work(struct mwifiex_adapter *adapter)
{
- mwifiex_drv_info_dump(adapter);
+ int drv_info_size;
+ void *drv_info;
+
+ drv_info_size = mwifiex_drv_info_dump(adapter, &drv_info);
mwifiex_pcie_fw_dump(adapter);
- mwifiex_upload_device_dump(adapter);
+ mwifiex_upload_device_dump(adapter, drv_info, drv_info_size);
}
static unsigned long iface_work_flags;
diff --git a/drivers/net/wireless/marvell/mwifiex/sdio.c b/drivers/net/wireless/marvell/mwifiex/sdio.c
index 1235f04..16d1d30 100644
--- a/drivers/net/wireless/marvell/mwifiex/sdio.c
+++ b/drivers/net/wireless/marvell/mwifiex/sdio.c
@@ -2547,13 +2547,15 @@ static void mwifiex_sdio_generic_fw_dump(struct mwifiex_adapter *adapter)
static void mwifiex_sdio_device_dump_work(struct mwifiex_adapter *adapter)
{
struct sdio_mmc_card *card = adapter->card;
+ int drv_info_size;
+ void *drv_info;
- mwifiex_drv_info_dump(adapter);
+ drv_info_size = mwifiex_drv_info_dump(adapter, &drv_info);
if (card->fw_dump_enh)
mwifiex_sdio_generic_fw_dump(adapter);
else
mwifiex_sdio_fw_dump(adapter);
- mwifiex_upload_device_dump(adapter);
+ mwifiex_upload_device_dump(adapter, drv_info, drv_info_size);
}
static void mwifiex_sdio_work(struct work_struct *work)
--
1.9.1
^ permalink raw reply related
* [PATCH v3 4/5] mwifiex: wait firmware dump complete during card remove process
From: Amitkumar Karwar @ 2016-11-16 13:09 UTC (permalink / raw)
To: linux-wireless
Cc: Cathy Luo, Nishant Sarmukadam, rajatja, briannorris,
dmitry.torokhov, Xinming Hu, Amitkumar Karwar
In-Reply-To: <1479301749-14803-1-git-send-email-akarwar@marvell.com>
From: Xinming Hu <huxm@marvell.com>
Wait for firmware dump complete in card remove function.
For sdio interface, there are two diffenrent cases,
card reset trigger sdio_work and firmware dump trigger sdio_work.
Do code rearrangement for distinguish between these two cases.
Signed-off-by: Xinming Hu <huxm@marvell.com>
Signed-off-by: Amitkumar Karwar <akarwar@marvell.com>
---
v2: 1. Get rid of reset_triggered flag. Instead split the code and use
__mwifiex_sdio_remove() (Brian Norris/Dmitry Torokhov)
2. "v1 4/5 mwifiex: firmware dump code rearrangement.." is dropped. So
rebased accordingly.
v3: same as [v2,5/5]. The improvement of 'moving pcie_work to card struct'
suggested by Brian is taken care in next patch.
---
drivers/net/wireless/marvell/mwifiex/pcie.c | 6 +++++-
drivers/net/wireless/marvell/mwifiex/sdio.c | 15 ++++++++++++---
2 files changed, 17 insertions(+), 4 deletions(-)
diff --git a/drivers/net/wireless/marvell/mwifiex/pcie.c b/drivers/net/wireless/marvell/mwifiex/pcie.c
index dd8f7aa..c8e69a4 100644
--- a/drivers/net/wireless/marvell/mwifiex/pcie.c
+++ b/drivers/net/wireless/marvell/mwifiex/pcie.c
@@ -51,6 +51,9 @@ static int mwifiex_pcie_probe_of(struct device *dev)
return 0;
}
+static void mwifiex_pcie_work(struct work_struct *work);
+static DECLARE_WORK(pcie_work, mwifiex_pcie_work);
+
static int
mwifiex_map_pci_memory(struct mwifiex_adapter *adapter, struct sk_buff *skb,
size_t size, int flags)
@@ -254,6 +257,8 @@ static void mwifiex_pcie_remove(struct pci_dev *pdev)
if (!adapter || !adapter->priv_num)
return;
+ cancel_work_sync(&pcie_work);
+
if (user_rmmod && !adapter->mfg_mode) {
mwifiex_deauthenticate_all(adapter);
@@ -2722,7 +2727,6 @@ static void mwifiex_pcie_work(struct work_struct *work)
mwifiex_pcie_device_dump_work(save_adapter);
}
-static DECLARE_WORK(pcie_work, mwifiex_pcie_work);
/* This function dumps FW information */
static void mwifiex_pcie_device_dump(struct mwifiex_adapter *adapter)
{
diff --git a/drivers/net/wireless/marvell/mwifiex/sdio.c b/drivers/net/wireless/marvell/mwifiex/sdio.c
index 16d1d30..78f2cc9 100644
--- a/drivers/net/wireless/marvell/mwifiex/sdio.c
+++ b/drivers/net/wireless/marvell/mwifiex/sdio.c
@@ -46,6 +46,9 @@
*/
static u8 user_rmmod;
+static void mwifiex_sdio_work(struct work_struct *work);
+static DECLARE_WORK(sdio_work, mwifiex_sdio_work);
+
static struct mwifiex_if_ops sdio_ops;
static unsigned long iface_work_flags;
@@ -220,7 +223,7 @@ static int mwifiex_sdio_resume(struct device *dev)
* This function removes the interface and frees up the card structure.
*/
static void
-mwifiex_sdio_remove(struct sdio_func *func)
+__mwifiex_sdio_remove(struct sdio_func *func)
{
struct sdio_mmc_card *card;
struct mwifiex_adapter *adapter;
@@ -249,6 +252,13 @@ static int mwifiex_sdio_resume(struct device *dev)
mwifiex_remove_card(adapter);
}
+static void
+mwifiex_sdio_remove(struct sdio_func *func)
+{
+ cancel_work_sync(&sdio_work);
+ __mwifiex_sdio_remove(func);
+}
+
/*
* SDIO suspend.
*
@@ -2227,7 +2237,7 @@ static void mwifiex_recreate_adapter(struct sdio_mmc_card *card)
* discovered and initializes them from scratch.
*/
- mwifiex_sdio_remove(func);
+ __mwifiex_sdio_remove(func);
/*
* Normally, we would let the driver core take care of releasing these.
@@ -2568,7 +2578,6 @@ static void mwifiex_sdio_work(struct work_struct *work)
mwifiex_sdio_card_reset_work(save_adapter);
}
-static DECLARE_WORK(sdio_work, mwifiex_sdio_work);
/* This function resets the card */
static void mwifiex_sdio_card_reset(struct mwifiex_adapter *adapter)
{
--
1.9.1
^ permalink raw reply related
* [PATCH v3 5/5] mwifiex: move pcie_work and related variables inside card
From: Amitkumar Karwar @ 2016-11-16 13:09 UTC (permalink / raw)
To: linux-wireless
Cc: Cathy Luo, Nishant Sarmukadam, rajatja, briannorris,
dmitry.torokhov, Ganapathi Bhat, Amitkumar Karwar
In-Reply-To: <1479301749-14803-1-git-send-email-akarwar@marvell.com>
From: Ganapathi Bhat <gbhat@marvell.com>
Currently pcie_work and related variables are global. It may create
problem while supporting multiple devices simultaneously. Let's move
it inside card structure so that separate instance will be created/
cancelled in init/teardown threads of each connected devices.
Signed-off-by: Ganapathi Bhat <gbhat@marvell.com>
Signed-off-by: Amitkumar Karwar <akarwar@marvell.com>
---
v3: This patch is introduced in v3
---
drivers/net/wireless/marvell/mwifiex/pcie.c | 23 ++++++++++++-----------
drivers/net/wireless/marvell/mwifiex/pcie.h | 2 ++
2 files changed, 14 insertions(+), 11 deletions(-)
diff --git a/drivers/net/wireless/marvell/mwifiex/pcie.c b/drivers/net/wireless/marvell/mwifiex/pcie.c
index c8e69a4..32fa4ed 100644
--- a/drivers/net/wireless/marvell/mwifiex/pcie.c
+++ b/drivers/net/wireless/marvell/mwifiex/pcie.c
@@ -52,7 +52,6 @@ static int mwifiex_pcie_probe_of(struct device *dev)
}
static void mwifiex_pcie_work(struct work_struct *work);
-static DECLARE_WORK(pcie_work, mwifiex_pcie_work);
static int
mwifiex_map_pci_memory(struct mwifiex_adapter *adapter, struct sk_buff *skb,
@@ -222,6 +221,7 @@ static int mwifiex_pcie_probe(struct pci_dev *pdev,
card->pcie.mem_type_mapping_tbl = data->mem_type_mapping_tbl;
card->pcie.num_mem_types = data->num_mem_types;
card->pcie.can_ext_scan = data->can_ext_scan;
+ INIT_WORK(&card->work, mwifiex_pcie_work);
}
/* device tree node parsing and platform specific configuration*/
@@ -257,7 +257,7 @@ static void mwifiex_pcie_remove(struct pci_dev *pdev)
if (!adapter || !adapter->priv_num)
return;
- cancel_work_sync(&pcie_work);
+ cancel_work_sync(&card->work);
if (user_rmmod && !adapter->mfg_mode) {
mwifiex_deauthenticate_all(adapter);
@@ -2718,25 +2718,27 @@ static void mwifiex_pcie_device_dump_work(struct mwifiex_adapter *adapter)
mwifiex_upload_device_dump(adapter, drv_info, drv_info_size);
}
-static unsigned long iface_work_flags;
-static struct mwifiex_adapter *save_adapter;
static void mwifiex_pcie_work(struct work_struct *work)
{
+ struct pcie_service_card *card =
+ container_of(work, struct pcie_service_card, work);
+
if (test_and_clear_bit(MWIFIEX_IFACE_WORK_DEVICE_DUMP,
- &iface_work_flags))
- mwifiex_pcie_device_dump_work(save_adapter);
+ &card->work_flags))
+ mwifiex_pcie_device_dump_work(card->adapter);
}
/* This function dumps FW information */
static void mwifiex_pcie_device_dump(struct mwifiex_adapter *adapter)
{
- save_adapter = adapter;
- if (test_bit(MWIFIEX_IFACE_WORK_DEVICE_DUMP, &iface_work_flags))
+ struct pcie_service_card *card = adapter->card;
+
+ if (test_bit(MWIFIEX_IFACE_WORK_DEVICE_DUMP, &card->work_flags))
return;
- set_bit(MWIFIEX_IFACE_WORK_DEVICE_DUMP, &iface_work_flags);
+ set_bit(MWIFIEX_IFACE_WORK_DEVICE_DUMP, &card->work_flags);
- schedule_work(&pcie_work);
+ schedule_work(&card->work);
}
/*
@@ -3202,7 +3204,6 @@ static void mwifiex_pcie_cleanup_module(void)
/* Set the flag as user is removing this module. */
user_rmmod = 1;
- cancel_work_sync(&pcie_work);
pci_unregister_driver(&mwifiex_pcie);
}
diff --git a/drivers/net/wireless/marvell/mwifiex/pcie.h b/drivers/net/wireless/marvell/mwifiex/pcie.h
index ae3365d..21ba5e6 100644
--- a/drivers/net/wireless/marvell/mwifiex/pcie.h
+++ b/drivers/net/wireless/marvell/mwifiex/pcie.h
@@ -386,6 +386,8 @@ struct pcie_service_card {
#endif
struct mwifiex_msix_context msix_ctx[MWIFIEX_NUM_MSIX_VECTORS];
struct mwifiex_msix_context share_irq_ctx;
+ struct work_struct work;
+ unsigned long work_flags;
};
static inline int
--
1.9.1
^ permalink raw reply related
* Re: [RFC 02/12] ath10k: htc: rx trailer lookahead support
From: Valo, Kalle @ 2016-11-16 13:53 UTC (permalink / raw)
To: Erik Stromdahl
Cc: michal.kazior@tieto.com, linux-wireless,
ath10k@lists.infradead.org
In-Reply-To: <ac58b3f6-4432-b0b6-8b50-c733f898eded@gmail.com>
Erik Stromdahl <erik.stromdahl@gmail.com> writes:
> On 11/15/2016 10:57 AM, Michal Kazior wrote:
>> On 14 November 2016 at 17:33, Erik Stromdahl <erik.stromdahl@gmail.com> =
wrote:
>>> The RX trailer parsing is now capable of parsing lookahead reports.
>>> This is needed by SDIO/mbox.
>>=20
>> It'd be useful to know what exactly lookahead will be used for. What
>> payload does it carry.
>>=20
> It carries the 4 first bytes of the next RX message, i.e. the first 4
> bytes of an HTC header.
>
> It is used by the sdio interrupt handler to know if the next packet is a
> part of an RX bundle, which endpoint it belongs to and how long it is
> (so we know how many bytes to allocate).
Please add that to the commit log, it's useful information. Or maybe a
code comment would be better?
>>> +static int
>>> +ath10k_htc_process_lookahead(struct ath10k_htc *htc,
>>> + const struct ath10k_htc_lookahead_report *=
report,
>>> + int len,
>>> + enum ath10k_htc_ep_id eid,
>>> + u32 *next_lk_ahds,
>>=20
>> next_lk_ahds should be u8 or void. From what I understand by glancing
>> through the code it is an agnostic buffer that carries payload which
>> is later interpreted either as eid or htc header, right? void is
>> probably most suitable in this case for passing around and u8 for
>> inline-based storage.
>>=20
> Sounds reasonable, I'll change to void pointer.
>
>> I'd also avoid silly abbreviations. Probably "lookahead" alone is enough=
.
>>=20
> Ok, this abbreviation was actually taken from the ath6kl code. I think
> the intention was to reduce line lengths.
Most likely that comes from the staging ath6kl driver which again is a
fork of the Atheros internal driver. I agree with Michal, better to
avoid silly abbreviations as much as possible. Readability is most
important.
--=20
Kalle Valo=
^ permalink raw reply
* Re: [05/10] rt2800: make ba_size depend on ampdu_factor
From: Kalle Valo @ 2016-11-16 13:59 UTC (permalink / raw)
To: Stanislaw Gruszka; +Cc: linux-wireless, Helmut Schaa, Stanislaw Gruszka
In-Reply-To: <1478095865-8651-6-git-send-email-sgruszka@redhat.com>
Stanislaw Gruszka <sgruszka@redhat.com> wrote:
> We can calculate BA window size (max number of pending frames not
> yet block acked) of remote station using Maximum A-MPDU length factor
> for that station.
>
> Signed-off-by: Stanislaw Gruszka <sgruszka@redhat.com>
Dropping as requested by Stanislaw.
Patch set to Changes Requested.
--
https://patchwork.kernel.org/patch/9409241/
Documentation about submitting wireless patches and checking status
from patchwork:
https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches
^ permalink raw reply
* Re: [PATCH] brcmfmac: update beacon IE when bss up and clear when stopped
From: Kalle Valo @ 2016-11-16 14:03 UTC (permalink / raw)
To: Wright Feng
Cc: arend.vanspriel, franky.lin, hante.meuleman, pieterpg,
brcm80211-dev-list.pdl, linux-wireless, Chi-Hsien Lin,
wright.feng
In-Reply-To: <20161115022322.GA5611@cypress.com>
Wright Feng <wefe@cypress.com> writes:
> Firmware doesn't update beacon vendor IEs when bss is down, so move brcmf_config_ap_mgmt_ie after BSS up. And host driver should clear IEs when AP stopped so that the IEs in host side will be synced with in firmware side.
>
> Signed-off-by: Wright Feng <wright.feng@cypress.com>
Please word wrap the commit log, see Documentation/SubmittingPatches for
more info.
--
Kalle Valo
^ permalink raw reply
* Re: [PATCH] ath9k: fix ath9k_hw_gpio_get() to return 0 or 1 on success
From: Kalle Valo @ 2016-11-16 14:08 UTC (permalink / raw)
To: Matthias Schiffer; +Cc: ath9k-devel, linux-wireless, ath9k-devel, miaoqing
In-Reply-To: <44116a7bca8524a187083916bdea535145e7a23a.1479232041.git.mschiffer@universe-factory.net>
Matthias Schiffer <mschiffer@universe-factory.net> writes:
> Commit b2d70d4944c1 ("ath9k: make GPIO API to support both of WMAC and
> SOC") refactored ath9k_hw_gpio_get() to support both WMAC and SOC GPIOs,
> changing the return on success from 1 to BIT(gpio). This broke some callers
> like ath_is_rfkill_set().
>
> Instead of fixing all callers, change ath9k_hw_gpio_get() back to only
> return 0 or 1.
>
> Fixes: b2d70d4944c1 ("ath9k: make GPIO API to support both of WMAC and SOC")
> Cc: <stable@vger.kernel.org> # v4.7+
> Signed-off-by: Matthias Schiffer <mschiffer@universe-factory.net>
Can you describe more about the symptoms, how did this break from user's
point of view? I can add that to the commit log.
--
Kalle Valo
^ permalink raw reply
* Re: [RFC 03/12] ath10k: htc: Changed order of wait target and ep connect
From: Michal Kazior @ 2016-11-16 14:29 UTC (permalink / raw)
To: Erik Stromdahl; +Cc: Kalle Valo, linux-wireless, ath10k@lists.infradead.org
In-Reply-To: <33589cf2-a7e6-fe9a-ea2b-a2252ac00355@gmail.com>
On 15 November 2016 at 18:07, Erik Stromdahl <erik.stromdahl@gmail.com> wro=
te:
> On 11/15/2016 11:13 AM, Michal Kazior wrote:
>> On 14 November 2016 at 17:33, Erik Stromdahl <erik.stromdahl@gmail.com> =
wrote:
>>> This patch changes the order in which the driver waits for the
>>> target to become ready and the service connect of the HTC
>>> control service.
>>>
>>> The HTC control service is connected before the driver starts
>>> waiting for the HTC ready control message.
>>>
>>> The reason for this is that the HTC ready control message is
>>> transmitted on EP 0 and that sdio/mbox based systems will ignore
>>> messages received on unconnected endpoints.
>>>
>>> Signed-off-by: Erik Stromdahl <erik.stromdahl@gmail.com>
>>> ---
>>> drivers/net/wireless/ath/ath10k/htc.c | 32 ++++++++++++++++---------=
-------
>>> 1 file changed, 16 insertions(+), 16 deletions(-)
>>>
>>> diff --git a/drivers/net/wireless/ath/ath10k/htc.c b/drivers/net/wirele=
ss/ath/ath10k/htc.c
>>> index e3f7bf4..7257366 100644
>>> --- a/drivers/net/wireless/ath/ath10k/htc.c
>>> +++ b/drivers/net/wireless/ath/ath10k/htc.c
>>> @@ -606,6 +606,22 @@ int ath10k_htc_wait_target(struct ath10k_htc *htc)
>>> u16 credit_count;
>>> u16 credit_size;
>>>
>>> + /* setup our pseudo HTC control endpoint connection */
>>> + memset(&conn_req, 0, sizeof(conn_req));
>>> + memset(&conn_resp, 0, sizeof(conn_resp));
>>> + conn_req.ep_ops.ep_tx_complete =3D ath10k_htc_control_tx_comple=
te;
>>> + conn_req.ep_ops.ep_rx_complete =3D ath10k_htc_control_rx_comple=
te;
>>> + conn_req.max_send_queue_depth =3D ATH10K_NUM_CONTROL_TX_BUFFERS=
;
>>> + conn_req.service_id =3D ATH10K_HTC_SVC_ID_RSVD_CTRL;
>>> +
>>> + /* connect fake service */
>>> + status =3D ath10k_htc_connect_service(htc, &conn_req, &conn_res=
p);
>>> + if (status) {
>>> + ath10k_err(ar, "could not connect to htc service (%d)\n=
",
>>> + status);
>>> + return status;
>>> + }
>>> +
>>
>> How is this supposed to work? ath10k_htc_connect_service() requires
>> htc->target_credit_size to compute tx_credits_per_max_message. Or am I
>> missing something? Applying this patch alone results in:
>>
>> [ 6.680101] divide error: 0000 [#1] SMP
>> [ 6.681342] Modules linked in: ath10k_pci(O) ath10k_core(O) ath
>> mac80211 cfg80211
>> [ 6.684876] CPU: 3 PID: 823 Comm: kworker/u8:2 Tainted: G W
>> O 4.9.0-rc4-wt-ath+ #79
>> [ 6.688051] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996),
>> BIOS 1.7.5-20140531_083030-gandalf 04/01/2014
>> [ 6.691644] Workqueue: ath10k_wq ath10k_core_register_work [ath10k_co=
re]
>> [ 6.694309] task: ffff88000a190000 task.stack: ffffc900006d4000
>> [ 6.695458] RIP: 0010:[<ffffffffa01ae46b>] [<ffffffffa01ae46b>]
>> ath10k_htc_connect_service+0x21b/0x420 [ath10k_core]
>>
>>
>> Micha=C5=82
>>
>
> You're right. I have totally missed this. What is strange is that my
> compiler (ARM linaro) seems to optimize the code in a way that removes
> the tx_credits_per_max_message value.
>
> If I add a printk in ath10k_htc_connect_service (printing the value) I
> get a similar oops.
>
> I think it has to do with the fact the this value isn't really used at
> all. grepping the code reveals that tx_credits_per_max_message is only
> used inside ath10k_htc_connect_service (only written, never read).
>
> Removing it doesn't seem to break anything, so perhaps it should be remov=
ed?
I think it's safe to remove now. This is legacy inherited from the
internal driver.
Otherwise this looks okay except commit log which is a bit unclear.
I'm still not sure *why* you need to reorder this. FWIW this can be a
general clean up thing that ends up moving the if (eid =3D=3D EP0) {code}
to control endpoint handler in patch 4.
Micha=C5=82
^ permalink raw reply
* Re: [PATCH] ath9k: fix ath9k_hw_gpio_get() to return 0 or 1 on success
From: Matthias Schiffer @ 2016-11-16 14:40 UTC (permalink / raw)
To: Kalle Valo; +Cc: ath9k-devel, linux-wireless, ath9k-devel, miaoqing
In-Reply-To: <8760nno6th.fsf@kamboji.qca.qualcomm.com>
[-- Attachment #1.1: Type: text/plain, Size: 1402 bytes --]
On 11/16/2016 03:08 PM, Kalle Valo wrote:
> Matthias Schiffer <mschiffer@universe-factory.net> writes:
>
>> Commit b2d70d4944c1 ("ath9k: make GPIO API to support both of WMAC and
>> SOC") refactored ath9k_hw_gpio_get() to support both WMAC and SOC GPIOs,
>> changing the return on success from 1 to BIT(gpio). This broke some callers
>> like ath_is_rfkill_set().
>>
>> Instead of fixing all callers, change ath9k_hw_gpio_get() back to only
>> return 0 or 1.
>>
>> Fixes: b2d70d4944c1 ("ath9k: make GPIO API to support both of WMAC and SOC")
>> Cc: <stable@vger.kernel.org> # v4.7+
>> Signed-off-by: Matthias Schiffer <mschiffer@universe-factory.net>
>
> Can you describe more about the symptoms, how did this break from user's
> point of view? I can add that to the commit log.
>
I did actually not experience an issue myself with a mainline kernel.
Looking at the functions ath_is_rfkill_set() and ath9k_rfkill_poll_state()
in gpio.c, this issue causes wiphy_rfkill_set_hw_state() always to be
passed false when ah->rfkill_polarity==1, breaking rfkill. I don't know how
common devices with ah->rfkill_polarity==1 are.
I became aware of this issue when rebasing an out-of-tree patch in LEDE
which uses the WMAC GPIOs to configure some kind of bandpass filter found
in Ubiquiti hardware. (I hope to find time to get this patch upsteam at
some point...)
Matthias
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 801 bytes --]
^ permalink raw reply
* [PATCH] staging: wilc1000: simplify vif[i]->ndev accesses
From: Arnd Bergmann @ 2016-11-16 15:07 UTC (permalink / raw)
To: Greg Kroah-Hartman
Cc: Arnd Bergmann, Aditya Shankar, Ganesh Krishna, Glen Lee, Leo Kim,
Binoy Jayan, Chris Park, linux-wireless, devel, linux-kernel
With gcc-7, I got a new warning for this driver:
wilc1000/linux_wlan.c: In function 'wilc_netdev_cleanup':
wilc1000/linux_wlan.c:1224:15: error: 'vif[1]' may be used uninitialized in this function [-Werror=maybe-uninitialized]
wilc1000/linux_wlan.c:1224:15: error: 'vif[0]' may be used uninitialized in this function [-Werror=maybe-uninitialized]
A closer look at the function reveals that it's more complex than
it needs to be, given that based on how the device is created
we always get
netdev_priv(vif->ndev) == vif
Based on this assumption, I found a few other places in the same file
that can be simplified. That code appears to be a relic from times
when the assumption above was not valid.
Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---
drivers/staging/wilc1000/linux_wlan.c | 34 +++++++++-------------------------
1 file changed, 9 insertions(+), 25 deletions(-)
diff --git a/drivers/staging/wilc1000/linux_wlan.c b/drivers/staging/wilc1000/linux_wlan.c
index 82c602ba3289..3775706578b2 100644
--- a/drivers/staging/wilc1000/linux_wlan.c
+++ b/drivers/staging/wilc1000/linux_wlan.c
@@ -260,23 +260,12 @@ static struct net_device *get_if_handler(struct wilc *wilc, u8 *mac_header)
int wilc_wlan_set_bssid(struct net_device *wilc_netdev, u8 *bssid, u8 mode)
{
- int i = 0;
- int ret = -1;
- struct wilc_vif *vif;
- struct wilc *wilc;
-
- vif = netdev_priv(wilc_netdev);
- wilc = vif->wilc;
+ struct wilc_vif *vif = netdev_priv(wilc_netdev);
- for (i = 0; i < wilc->vif_num; i++)
- if (wilc->vif[i]->ndev == wilc_netdev) {
- memcpy(wilc->vif[i]->bssid, bssid, 6);
- wilc->vif[i]->mode = mode;
- ret = 0;
- break;
- }
+ memcpy(vif->bssid, bssid, 6);
+ vif->mode = mode;
- return ret;
+ return 0;
}
int wilc_wlan_get_num_conn_ifcs(struct wilc *wilc)
@@ -1203,16 +1192,11 @@ void WILC_WFI_mgmt_rx(struct wilc *wilc, u8 *buff, u32 size)
void wilc_netdev_cleanup(struct wilc *wilc)
{
- int i = 0;
- struct wilc_vif *vif[NUM_CONCURRENT_IFC];
+ int i;
- if (wilc && (wilc->vif[0]->ndev || wilc->vif[1]->ndev)) {
+ if (wilc && (wilc->vif[0]->ndev || wilc->vif[1]->ndev))
unregister_inetaddr_notifier(&g_dev_notifier);
- for (i = 0; i < NUM_CONCURRENT_IFC; i++)
- vif[i] = netdev_priv(wilc->vif[i]->ndev);
- }
-
if (wilc && wilc->firmware) {
release_firmware(wilc->firmware);
wilc->firmware = NULL;
@@ -1221,7 +1205,7 @@ void wilc_netdev_cleanup(struct wilc *wilc)
if (wilc && (wilc->vif[0]->ndev || wilc->vif[1]->ndev)) {
for (i = 0; i < NUM_CONCURRENT_IFC; i++)
if (wilc->vif[i]->ndev)
- if (vif[i]->mac_opened)
+ if (wilc->vif[i]->mac_opened)
wilc_mac_close(wilc->vif[i]->ndev);
for (i = 0; i < NUM_CONCURRENT_IFC; i++) {
@@ -1269,9 +1253,9 @@ int wilc_netdev_init(struct wilc **wilc, struct device *dev, int io_type,
vif->idx = wl->vif_num;
vif->wilc = *wilc;
+ vif->ndev = ndev;
wl->vif[i] = vif;
- wl->vif[wl->vif_num]->ndev = ndev;
- wl->vif_num++;
+ wl->vif_num = i;
ndev->netdev_ops = &wilc_netdev_ops;
{
--
2.9.0
^ permalink raw reply related
* RE: [PATCH v2 5/5] mwifiex: wait firmware dump complete during card remove process
From: Amitkumar Karwar @ 2016-11-16 15:27 UTC (permalink / raw)
To: Brian Norris
Cc: linux-wireless@vger.kernel.org, Cathy Luo, Nishant Sarmukadam,
rajatja@google.com, briannorris@google.com,
dmitry.torokhov@gmail.com, Xinming Hu
In-Reply-To: <20161027184851.GB28717@localhost>
Hi Brian,
> > +
> > static int
> > mwifiex_map_pci_memory(struct mwifiex_adapter *adapter, struct
> sk_buff *skb,
> > size_t size, int flags)
> > @@ -249,6 +252,8 @@ static void mwifiex_pcie_remove(struct pci_dev
> *pdev)
> > if (!adapter || !adapter->priv_num)
> > return;
> >
> > + cancel_work_sync(&pcie_work);
>
> Hmm, actually what happens if we have !adapter? Then that means we've
> already torn down the device (e.g., unregister_dev() -- except we
> haven't quite fixed that bug, see the other patch series you sent out),
> and so we'll never reach this. But that also means we haven't
> synchronized any outstanding work.
There won't be any outstanding work in that case where init failure thread has already cleared "adapter"
Pcie/sdio Work(firmware dump + card reset) is scheduled in below two scenarios
1) Command timeout -- We have a check to skip triggering work when hw_status shows it's INITIALIZING.
2) Tx data timeout -- Tx data is applicable only when wifi connection is alive. In above mentioned case, device itself has failed to initialize.
>
> So this really belongs in one of the earlier mwifiex callbacks
> (unregister_dev()?), and not in the device remove() callback.
>
Regards,
Amitkumar
^ permalink raw reply
* Re: [PATCH 05/10] rt2800: make ba_size depend on ampdu_factor
From: Mathias Kresin @ 2016-11-16 15:54 UTC (permalink / raw)
To: Stanislaw Gruszka; +Cc: linux-wireless, Helmut Schaa, Felix Fietkau
In-Reply-To: <20161116120213.GB26475@redhat.com>
2016-11-16 13:02 GMT+01:00 Stanislaw Gruszka <sgruszka@redhat.com>:
> On Wed, Nov 16, 2016 at 09:07:00AM +0100, Mathias Kresin wrote:
>> Here are the results of the requested tests. Please keep in mind, I'm not in
>> a lab environment:
>>
>> LEDE head
>> connect
>> action 0 sta 9c:f3:87:bc:AA:BB tid 6 buf_size 64 ampdu_factor 3
>> action 2 sta 9c:f3:87:bc:AA:BB tid 0 buf_size 0 ampdu_factor 3
>> action 6 sta 9c:f3:87:bc:AA:BB tid 0 buf_size 64 ampdu_factor 3
>> action 0 sta 9c:f3:87:bc:AA:BB tid 0 buf_size 64 ampdu_factor 3
>
> No problem here - buf_size corresponds to ampdu_factor.
>
>> ieee80211 phy0: rt2x00queue_flush_queue: Warning - Queue 2 failed
>> to flush
>> ieee80211 phy0: rt2x00queue_flush_queue: Warning - Queue 2 failed
>> to flush
>
> I think we do not give device enough time to post AMPDU consisted
> with bigger amount of frames. If we want to increase ba_size we will
> need also some other changes in the driver. Anyway I already request
> Kalle to drop this patch. I assume other patches do not cause
> regression for you, correct?
Correct. Just 05/10 caused issues.
^ permalink raw reply
* Re: [PATCH] ath9k: fix ath9k_hw_gpio_get() to return 0 or 1 on success
From: Kalle Valo @ 2016-11-16 15:59 UTC (permalink / raw)
To: Matthias Schiffer; +Cc: ath9k-devel, linux-wireless, ath9k-devel, miaoqing
In-Reply-To: <6d1da72b-33b5-b99e-69ac-8b8307ce60f0@universe-factory.net>
Matthias Schiffer <mschiffer@universe-factory.net> writes:
> On 11/16/2016 03:08 PM, Kalle Valo wrote:
>> Matthias Schiffer <mschiffer@universe-factory.net> writes:
>>
>>> Commit b2d70d4944c1 ("ath9k: make GPIO API to support both of WMAC and
>>> SOC") refactored ath9k_hw_gpio_get() to support both WMAC and SOC GPIOs,
>>> changing the return on success from 1 to BIT(gpio). This broke some callers
>>> like ath_is_rfkill_set().
>>>
>>> Instead of fixing all callers, change ath9k_hw_gpio_get() back to only
>>> return 0 or 1.
>>>
>>> Fixes: b2d70d4944c1 ("ath9k: make GPIO API to support both of WMAC and SOC")
>>> Cc: <stable@vger.kernel.org> # v4.7+
>>> Signed-off-by: Matthias Schiffer <mschiffer@universe-factory.net>
>>
>> Can you describe more about the symptoms, how did this break from user's
>> point of view? I can add that to the commit log.
>>
>
> I did actually not experience an issue myself with a mainline kernel.
>
> Looking at the functions ath_is_rfkill_set() and ath9k_rfkill_poll_state()
> in gpio.c, this issue causes wiphy_rfkill_set_hw_state() always to be
> passed false when ah->rfkill_polarity==1, breaking rfkill. I don't know how
> common devices with ah->rfkill_polarity==1 are.
>
> I became aware of this issue when rebasing an out-of-tree patch in LEDE
> which uses the WMAC GPIOs to configure some kind of bandpass filter found
> in Ubiquiti hardware. (I hope to find time to get this patch upsteam at
> some point...)
Thanks, this kind of information is good to have in the commit log. That
way maintainers, like me or stable tree folks, can prioritise the fix to
various releases. And I guess Linux distros also like to know this as
well as some of the normal users when trying to solve problems.
I'll mention in the commit log that this doesn't fix any known bug. And
I'll push this to 4.10, I don't think this is important enough for 4.9.
--
Kalle Valo
^ permalink raw reply
* [PATCH] rt2x00: Fix incorrect usage of CONFIG_RT2X00_LIB_USB
From: Vishal Thanki @ 2016-11-16 16:01 UTC (permalink / raw)
To: sgruszka, helmut.schaa, linux-wireless, kvalo; +Cc: Vishal Thanki
In device removal routine, usage of "#ifdef CONFIG_RT2X00_LIB_USB"
will not cover the case when it is configured as module. This will
omit the entire if-block which does cleanup of URBs and cancellation
of pending work. Changing the #ifdef to #if IS_ENABLED() to fix it.
Signed-off-by: Vishal Thanki <vishalthanki@gmail.com>
---
drivers/net/wireless/ralink/rt2x00/rt2x00dev.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/net/wireless/ralink/rt2x00/rt2x00dev.c b/drivers/net/wireless/ralink/rt2x00/rt2x00dev.c
index 17a0c1b..eb7b714 100644
--- a/drivers/net/wireless/ralink/rt2x00/rt2x00dev.c
+++ b/drivers/net/wireless/ralink/rt2x00/rt2x00dev.c
@@ -1441,7 +1441,7 @@ void rt2x00lib_remove_dev(struct rt2x00_dev *rt2x00dev)
cancel_work_sync(&rt2x00dev->intf_work);
cancel_delayed_work_sync(&rt2x00dev->autowakeup_work);
cancel_work_sync(&rt2x00dev->sleep_work);
-#ifdef CONFIG_RT2X00_LIB_USB
+#if IS_ENABLED(CONFIG_RT2X00_LIB_USB)
if (rt2x00_is_usb(rt2x00dev)) {
usb_kill_anchored_urbs(rt2x00dev->anchor);
hrtimer_cancel(&rt2x00dev->txstatus_timer);
--
2.4.11
^ permalink raw reply related
* Re: [PATCH] rt2x00: Fix incorrect usage of CONFIG_RT2X00_LIB_USB
From: Stanislaw Gruszka @ 2016-11-16 16:12 UTC (permalink / raw)
To: Vishal Thanki; +Cc: helmut.schaa, linux-wireless, kvalo
In-Reply-To: <1479312114-9389-1-git-send-email-vishalthanki@gmail.com>
On Wed, Nov 16, 2016 at 05:01:54PM +0100, Vishal Thanki wrote:
> In device removal routine, usage of "#ifdef CONFIG_RT2X00_LIB_USB"
> will not cover the case when it is configured as module. This will
> omit the entire if-block which does cleanup of URBs and cancellation
> of pending work. Changing the #ifdef to #if IS_ENABLED() to fix it.
>
> Signed-off-by: Vishal Thanki <vishalthanki@gmail.com>
Acked-by: Stanislaw Gruszka <sgruszka@redhat.com>
^ permalink raw reply
* Re: [RFC 1/2] ath9k: work around AR_CFG 0xdeadbeef chip hang
From: Valo, Kalle @ 2016-11-16 16:16 UTC (permalink / raw)
To: Sven Eckelmann
Cc: ath9k-devel@lists.ath9k.org, linux-wireless@vger.kernel.org,
ath9k-devel, Simon Wunderlich
In-Reply-To: <20161114144226.15748-1-sven.eckelmann@open-mesh.com>
Sven Eckelmann <sven.eckelmann@open-mesh.com> writes:
> From: Simon Wunderlich <simon.wunderlich@open-mesh.com>
>
> QCA 802.11n chips (especially AR9330/AR9340) sometimes end up in a state =
in
> which a read of AR_CFG always returns 0xdeadbeef. This should not happen
> when when the power_mode of the device is ATH9K_PM_AWAKE.
>
> This problem is not yet detected by any other workaround in ath9k. No way
> is known to reproduce the problem easily.
>
> Signed-off-by: Simon Wunderlich <simon.wunderlich@open-mesh.com>
> [sven.eckelmann@open-mesh.com: port to recent ath9k, add commit message]
> Signed-off-by: Sven Eckelmann <sven.eckelmann@open-mesh.com>
[...]
> +void ath_hw_hang_work(struct work_struct *work)
> +{
> + struct ath_softc *sc =3D container_of(work, struct ath_softc,
> + hw_hang_work.work);
> +
> + if (ath_hw_hang_deadbeef(sc))
> + goto requeue_worker;
> +
> +requeue_worker:
> + ieee80211_queue_delayed_work(sc->hw, &sc->hw_hang_work,
> + msecs_to_jiffies(ATH_HANG_WORK_INTERVAL));
> +}
The goto doesn't make any sense, either me or the function is missing
something :)
--=20
Kalle Valo=
^ permalink raw reply
* Re: [PATCH V3] qtnfmac: announcement of new FullMAC driver for Quantenna chipsets
From: IgorMitsyanko @ 2016-11-16 16:47 UTC (permalink / raw)
To: Johannes Berg, linux-wireless
Cc: btherthala, hwang, smaksimenko, dlebed, Sergey Matyukevich,
Kamlesh Rath, Avinash Patil
In-Reply-To: <1479200773.12007.13.camel@sipsolutions.net>
On 11/15/2016 12:06 PM, Johannes Berg wrote:
> On Wed, 2016-11-09 at 17:00 +0300, igor.mitsyanko.os@quantenna.com
> wrote:
>
>> +static int
>> +qtnf_change_virtual_intf(struct wiphy *wiphy,
>> + struct net_device *dev,
>> + enum nl80211_iftype type, u32 *flags,
>> + struct vif_params *params)
>> +{
>> + struct qtnf_vif *vif;
>> + u8 *mac_addr;
>> +
>> + vif = qtnf_netdev_get_priv(dev);
>> +
>> + if (params)
>> + mac_addr = params->macaddr;
>> + else
>> + mac_addr = NULL;
>> +
>> + if (qtnf_cmd_send_change_intf_type(vif, type, mac_addr)) {
>> + pr_err("failed to change interface type\n");
>> + return -EFAULT;
>> + }
>> +
>> + vif->wdev.iftype = type;
>> + return 0;
>> +}
> Do you really support arbitrary type changes?
We do, only if there are no secondary BSSIDs exist on a physical radio
in AP mode (MBSS is not active).
>
> You might even have to handle ongoing scans, etc.
Point taken, will review how we handle scans and other ongoing
operations. I guess in case of ongoing operation, we can return "busy"
error on attempt to change interface type.
>
>> + /* Clear the vif in mac */
> "mac"? Maybe you mean cfg80211?
It meant to indicate that "vif" is part of "struct qtnf_wmac". I think
we should remove this comment, don't see how it helps.
>
>> + vif->netdev->ieee80211_ptr = NULL;
>> + vif->netdev = NULL;
>> + vif->wdev.iftype = NL80211_IFTYPE_UNSPECIFIED;
>> + eth_zero_addr(vif->mac_addr);
>> +
>> + return 0;
>> +}
> But I'm not sure this makes sense? You're not actually deleting the
> interface here, so why sever all the links/clear all the data?
Setting "iftype = NL80211_IFTYPE_UNSPECIFIED" effectively marks
interface as free to use. Clearing all the data should not be strictly
necessary, but should help catch bugs in case inteface that was "freed"
is used by someone.
>
>
>> +/* concatenate all the beacon IEs into one buffer
>> + * Take IEs from head, tail and beacon_ies fields of
> cfg80211_beacon_data
>> + * and append it to provided buffer.
>> + * Checks total IE buf length to be <= than IEEE80211_MAX_DATA_LEN.
>> + * Checks IE buffers to be valid, so that resulting buffer
>> + * should be a valid IE buffer with length <=
> IEEE80211_MAX_DATA_LEN.
>> + */
> I'm not sure this is right - beacon_ies is head+tail already, I think?
Looking at hostapd sources, looks like you're right. Though it's not
immediately obvious.. Will review the code and handle this correctly.
>
>> +static int
>> +qtnf_dump_station(struct wiphy *wiphy, struct net_device *dev,
>> + int idx, u8 *mac, struct station_info *sinfo)
>> +{
>> + struct qtnf_vif *vif = qtnf_netdev_get_priv(dev);
>> + const struct qtnf_sta_node *sta_node;
>> + int ret;
>> +
>> + sta_node = qtnf_sta_list_lookup_index(&vif->sta_list, idx);
>> +
>> + if (unlikely(!sta_node))
>> + return -ENOENT;
>> +
>> + ether_addr_copy(mac, sta_node->mac_addr);
>> +
>> + ret = qtnf_cmd_get_sta_info(vif, sta_node->mac_addr, sinfo);
>> +
>> + if (unlikely(ret == -ENOENT)) {
>> + sinfo->filled = 0;
>> + ret = 0;
>> + }
> This case seems slightly odd - what does it mean that the station
> existed, but getting the information returned -ENOENT? Is that because
> it's racy, somehow? If so, wouldn't it be better to take this as an
> indication that the station doesn't exist, and skip this entry entirely
> or something?
I see two possibilities here:
- it's possible that STAs will exit on device side while we're
processing dump_station(), and because commands and events processing is
serialized, driver will not know about that yet;
- it's a bug in state synchronization between driver and device
In both cases, I agree that we better to remove STAs from the list here
if returned ENOENT.
>
>> + /* nofity cfg80211 */
> typo :)
Will fix)
>
>> + while (payload_len >= sizeof(struct qlink_tlv_hdr)) {
>> + tlv_type = le16_to_cpu(tlv->type);
>> + tlv_value_len = le16_to_cpu(tlv->len);
>> + tlv_full_len = tlv_value_len + sizeof(struct
> qlink_tlv_hdr);
>> +
>> + if (tlv_full_len > payload_len) {
>> + pr_warn("malformed TLV 0x%.2X; LEN: %u\n",
>> + tlv_type, tlv_value_len);
>> + return -EINVAL;
>> + }
>> +
>> + if (tlv_type == QTN_TLV_ID_IE_SET) {
>> + ies = tlv->val;
>> + ies_len = tlv_value_len;
>> + }
>> +
>> + payload_len -= tlv_full_len;
>> + tlv = (struct qlink_tlv_hdr *)(tlv->val +
> tlv_value_len);
>> + }
>> +
>> + if (payload_len) {
>> + pr_warn("malformed IEs buf; bytes left: %zu\n",
> payload_len);
>> + return -EINVAL;
>> + }
> Don't you mean "malformed TLVs buf"? It's obviously similar, but you
> refer to this encoding as TLV, not IE.
>
> Maybe you should ignore it too, since it's a firmware bug?
Agree, will change to "TLV". As for ignoring or not, there is probably
no harm in returning error if something went the way we did not expect,
since we do not do any parameters values validation we can at least
validate buffer integrity itself.
>
>> + qdev_vif = netdev_priv(dev);
>> + *((unsigned long *)qdev_vif) = (unsigned long)vif;
> This seems very strange - why unsigned long, rather than void? I mean
>
> *(void **)qdev_vif = vif;
Will fix.
>
>> +static int qtnf_pcie_init_shm_ipc(struct qtnf_pcie_bus_priv *priv)
>> +{
>> + struct qtnf_shm_ipc_region __iomem *ipc_tx_reg;
>> + struct qtnf_shm_ipc_region __iomem *ipc_rx_reg;
>> + const struct qtnf_shm_ipc_int ipc_int = {
> qtnf_ipc_gen_ep_int, priv };
>> + const struct qtnf_shm_ipc_rx_callback rx_callback = {
>> + qtnf_pcie_control_rx_callbac
> k, priv };
>
> If those are const, why not also static? In fact, it seems they really
> should be, since they're registered below?
Later qtnf_shm_ipc_init() will make a full copy of rx_callback (which
actually only contains two pointers), it does not expect it to have
static time of live, so no point in statically allocating it.
>
>> +static int alloc_bd_table(struct qtnf_pcie_bus_priv *priv)
>> +{
>> + unsigned long vaddr;
>> + dma_addr_t paddr;
>> + int len;
>> +
>> + len = priv->tx_bd_num * sizeof(struct qtnf_tx_bd) +
>> + priv->rx_bd_num * sizeof(struct qtnf_rx_bd);
>> +
>> + vaddr = (unsigned long)dmam_alloc_coherent(&priv->pdev->dev,
>> + len, &paddr,
> GFP_KERNEL);
>> + if (!vaddr)
>> + return -ENOMEM;
>> +
>> + /* tx bd */
>> +
>> + memset((void *)vaddr, 0, len);
> Those unsigned long/void * casts look strange too. Why not use a "void
> *vaddr" to start with?
Will review these
>
>> + priv->bd_table_vaddr = vaddr;
> Maybe need a cast here, if that variable is needed at all (identical to
> tx_bd_vbase), or make that struct member also void *?
>
>> + priv->bd_table_paddr = paddr;
>> + priv->bd_table_len = len;
>> +
>> + priv->tx_bd_vbase = (struct qtnf_tx_bd *)vaddr;
> Don't even need that cast then.
>
>> + priv->tx_bd_pbase = paddr;
>> +
>> + pr_debug("TX descriptor table: vaddr=0x%p paddr=%pad\n",
>> + (void *)vaddr, &paddr);
>> +
>> + priv->tx_bd_reclaim_start = 0;
>> + priv->tx_bd_index = 0;
>> + priv->tx_queue_len = 0;
>> +
>> + /* rx bd */
>> +
>> + vaddr += priv->tx_bd_num * sizeof(struct qtnf_tx_bd);
> Here you can do something like
>
> vaddr = ((struct qtnf_tx_bd)vaddr) + priv->tx_bd_num;
>
>> + paddr += priv->tx_bd_num * sizeof(struct qtnf_tx_bd);
>> +
>> + priv->rx_bd_vbase = (struct qtnf_rx_bd *)vaddr;
> no need for the cast here then.
Ok, we will review to see how to make this code prettier.
>
>> + priv->rx_bd_pbase = paddr;
>> +
>> + writel(QTN_HOST_LO32(paddr),
>> + PCIE_HDP_TX_HOST_Q_BASE_L(priv->pcie_reg_base));
>> + writel(QTN_HOST_HI32(paddr),
>> + PCIE_HDP_TX_HOST_Q_BASE_H(priv->pcie_reg_base));
>> + writel(priv->rx_bd_num | (sizeof(struct qtnf_rx_bd)) << 16,
>> + PCIE_HDP_TX_HOST_Q_SZ_CTRL(priv->pcie_reg_base));
>> +
>> + priv->hw_txproc_wr_ptr = priv->rx_bd_num -
> rx_bd_reserved_param;
>> +
>> + writel(priv->hw_txproc_wr_ptr,
>> + PCIE_HDP_TX_HOST_Q_WR_PTR(priv->pcie_reg_base));
>> +
>> + pr_debug("RX descriptor table: vaddr=0x%p paddr=%pad\n",
>> + (void *)vaddr, &paddr);
> Nor here.
>
> On the whole, it probably doesn't really matter (I'd let Kalle decide I
> guess). Just looks odd to me.
>
>> + /* sync up all descriptor updates before passing them to EP
> */
>> + wmb();
> I think you need dma_wmb()?
Looks like it, based on documentation. Will confirm and change, if
necessary.
>
>
> So I mostly looked at the cfg80211 bits, obviously - the other comments
> were just in passing.
>
> I also didn't review the flows - some of these things are tricky (e.g.
> are there races between userspace asking to disconnect, and disconnect
> notification, and similar). Maybe it helps anyway :)
The idea we used for flow synchronization is that all commands/events
processing is serialized with RTNL mutex. Not very smart approach, but
simple and in fact can be enough (there are not too much
commands/notifications going on anyway). In the future it is possible to
move to a more fine-grained locking.
Thanks for reviewing this, Johannes!
>
> johannes
^ 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