* Re: [PATCH v2 3/3] mwifiex: Enable WoWLAN for both sdio and pcie
From: Brian Norris @ 2016-11-11 22:05 UTC (permalink / raw)
To: Amitkumar Karwar
Cc: linux-wireless, Cathy Luo, Nishant Sarmukadam, rajatja,
dmitry.torokhov
In-Reply-To: <20161111204235.GB111624@google.com>
Hi,
One correction to my review:
On Fri, Nov 11, 2016 at 12:42:36PM -0800, Brian Norris wrote:
> On Fri, Nov 11, 2016 at 04:45:11PM +0530, Amitkumar Karwar wrote:
> > --- a/drivers/net/wireless/marvell/mwifiex/main.c
> > +++ b/drivers/net/wireless/marvell/mwifiex/main.c
> > @@ -1552,14 +1552,55 @@ void mwifiex_do_flr(struct mwifiex_adapter *adapter, bool prepare)
> > }
> > EXPORT_SYMBOL_GPL(mwifiex_do_flr);
> >
> > +static irqreturn_t mwifiex_irq_wakeup_handler(int irq, void *priv)
> > +{
> > + struct mwifiex_adapter *adapter = priv;
> > +
> > + if (adapter->irq_wakeup >= 0) {
> > + dev_dbg(adapter->dev, "%s: wake by wifi", __func__);
> > + adapter->wake_by_wifi = true;
>
> This flag is unecessary and buggy. IIUC, you're trying to avoid calling
> disable_irq() in resume(), if you've called it here?
>
> > + disable_irq_nosync(irq);
>
> ...but this is unnecessary, I think, unless you're trying to make up for
> buggy wakeup interrupts that keep firing? See my suggestion below.
I think I figured out some of the logic here, and my suggestion was
somewhat wrong. This 'wake_by_wifi' flag seems to be used here to assist
with the fact that we're requesting a level-triggered interrupt, but
the card doesn't deassert the interrupt until much later -- when we
finish the wakeup handshake.
So I guess it is necessary (or at least, expedient) to disable the
interrupt here.
> > + }
> > +
> > + /* Notify PM core we are wakeup source */
> > + pm_wakeup_event(adapter->dev, 0);
> > +
> > + return IRQ_HANDLED;
> > +}
> > +
> > static void mwifiex_probe_of(struct mwifiex_adapter *adapter)
> > {
> > + int ret;
> > struct device *dev = adapter->dev;
> >
> > if (!dev->of_node)
> > return;
> >
> > adapter->dt_node = dev->of_node;
> > + adapter->irq_wakeup = irq_of_parse_and_map(adapter->dt_node, 0);
> > + if (!adapter->irq_wakeup) {
> > + dev_info(dev, "fail to parse irq_wakeup from device tree\n");
> > + return;
> > + }
> > +
> > + ret = devm_request_irq(dev, adapter->irq_wakeup,
> > + mwifiex_irq_wakeup_handler, IRQF_TRIGGER_LOW,
> > + "wifi_wake", adapter);
> > + if (ret) {
> > + dev_err(dev, "Failed to request irq_wakeup %d (%d)\n",
> > + adapter->irq_wakeup, ret);
> > + goto err_exit;
> > + }
> > +
> > + disable_irq(adapter->irq_wakeup);
> > + if (device_init_wakeup(dev, true)) {
> > + dev_err(dev, "fail to init wakeup for mwifiex\n");
> > + goto err_exit;
> > + }
> > + return;
> > +
> > +err_exit:
> > + adapter->irq_wakeup = 0;
> > }
> >
> > /*
> > diff --git a/drivers/net/wireless/marvell/mwifiex/main.h b/drivers/net/wireless/marvell/mwifiex/main.h
> > index 0c07434..11abc49 100644
> > --- a/drivers/net/wireless/marvell/mwifiex/main.h
> > +++ b/drivers/net/wireless/marvell/mwifiex/main.h
> > @@ -1011,6 +1011,10 @@ struct mwifiex_adapter {
> > bool usb_mc_setup;
> > struct cfg80211_wowlan_nd_info *nd_info;
> > struct ieee80211_regdomain *regd;
> > +
> > + /* Wake-on-WLAN (WoWLAN) */
> > + int irq_wakeup;
> > + bool wake_by_wifi;
> > };
> >
> > void mwifiex_process_tx_queue(struct mwifiex_adapter *adapter);
> > @@ -1410,6 +1414,27 @@ static inline u8 mwifiex_is_tdls_link_setup(u8 status)
> > return false;
> > }
> >
> > +/* Disable platform specific wakeup interrupt */
> > +static inline void mwifiex_disable_wake(struct mwifiex_adapter *adapter)
> > +{
> > + if (adapter->irq_wakeup >= 0) {
> > + disable_irq_wake(adapter->irq_wakeup);
> > + if (!adapter->wake_by_wifi)
>
> You're depending on the wake IRQ handler to set this flag, so you don't
> disable the IRQ twice (the calls nest). But what if the IRQ is being
> serviced concurrently with this? Then you'll double-disable the IRQ.
>
> I believe you can just remove the disable_irq_nosync() from the handler,
> kill the above flag, and just unconditionally disable the IRQ.
So my suggestion here was wrong; we shouldn't completely kill the check
for ->wake_by_wifi I don't think, but we *should* wait for the interrupt
handler to complete before checking the flag. i.e., synchronize_irq():
diff --git a/drivers/net/wireless/marvell/mwifiex/util.c b/drivers/net/wireless/marvell/mwifiex/util.c
index 4063086ab5b8..e9446eeafb9d 100644
--- a/drivers/net/wireless/marvell/mwifiex/util.c
+++ b/drivers/net/wireless/marvell/mwifiex/util.c
@@ -841,6 +841,11 @@ void mwifiex_disable_wake(struct mwifiex_plt_wake_cfg *wake_cfg)
{
if (wake_cfg && wake_cfg->irq_wifi >= 0) {
disable_irq_wake(wake_cfg->irq_wifi);
+ /*
+ * Disable the wake IRQ only if it didn't already fire (and
+ * disable itself).
+ */
+ synchronize_irq(wake_cfg->irq_wifi);
if (!wake_cfg->wake_by_wifi)
disable_irq(wake_cfg->irq_wifi);
}
I'd appreciate if you bugfix this, either before or after this patch.
Brian
> > + disable_irq(adapter->irq_wakeup);
> > + }
> > +}
> > +
> > +/* Enable platform specific wakeup interrupt */
> > +static inline void mwifiex_enable_wake(struct mwifiex_adapter *adapter)
> > +{
> > + /* Enable platform specific wakeup interrupt */
> > + if (adapter->irq_wakeup >= 0) {
> > + adapter->wake_by_wifi = false;
> > + enable_irq(adapter->irq_wakeup);
> > + enable_irq_wake(adapter->irq_wakeup);
> > + }
> > +}
> > +
> > int mwifiex_init_shutdown_fw(struct mwifiex_private *priv,
> > u32 func_init_shutdown);
> > int mwifiex_add_card(void *, struct semaphore *, struct mwifiex_if_ops *, u8,
^ permalink raw reply related
* Re: [PATCH v2 2/3] mwifiex: Introduce mwifiex_probe_of() to parse common properties
From: Brian Norris @ 2016-11-11 20:49 UTC (permalink / raw)
To: Amitkumar Karwar
Cc: linux-wireless, Cathy Luo, Nishant Sarmukadam, rajatja,
dmitry.torokhov
In-Reply-To: <1478862911-15498-2-git-send-email-akarwar@marvell.com>
On Fri, Nov 11, 2016 at 04:45:10PM +0530, Amitkumar Karwar wrote:
> From: Rajat Jain <rajatja@google.com>
>
> Introduce function mwifiex_probe_of() to parse common properties.
> Since the interface drivers get to decide whether or not the device
> tree node was a valid one (depending on the compatible property),
> let the interface drivers pass a flag to indicate whether the device
> tree node was a valid one.
Wait, what? I don't understand why this is needed. The current of_node
user (SDIO) always checks dev->of_node (if !NULL), and if it's not
matching, it rejects the device and doesn't even try to register the
card at all. That's a common pattern for DT-based drivers, and I don't
see why we need to do differently for any other driver (e.g., PCIe).
So...isn't 'dev->of_node != NULL' an equivalent test to 'of_node_valid'?
Or put another way, mwifiex_add_card() should never see a 'struct
device' whose of_node is not compatible. Do you agree?
> The function mwifiex_probe_of()nodetself is currently only a place
> holder with the next patch adding content to it.
>
> Signed-off-by: Rajat Jain <rajatja@google.com>
> ---
> v2: Same as v1
> ---
> drivers/net/wireless/marvell/mwifiex/main.c | 15 ++++++++++++++-
> drivers/net/wireless/marvell/mwifiex/main.h | 2 +-
> drivers/net/wireless/marvell/mwifiex/pcie.c | 4 +++-
> drivers/net/wireless/marvell/mwifiex/sdio.c | 4 +++-
> drivers/net/wireless/marvell/mwifiex/sta_cmd.c | 5 +----
> drivers/net/wireless/marvell/mwifiex/usb.c | 2 +-
> 6 files changed, 23 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/net/wireless/marvell/mwifiex/main.c b/drivers/net/wireless/marvell/mwifiex/main.c
> index dcceab2..b2f3d96 100644
> --- a/drivers/net/wireless/marvell/mwifiex/main.c
> +++ b/drivers/net/wireless/marvell/mwifiex/main.c
> @@ -1552,6 +1552,16 @@ void mwifiex_do_flr(struct mwifiex_adapter *adapter, bool prepare)
> }
> EXPORT_SYMBOL_GPL(mwifiex_do_flr);
>
> +static void mwifiex_probe_of(struct mwifiex_adapter *adapter)
> +{
> + struct device *dev = adapter->dev;
> +
> + if (!dev->of_node)
> + return;
> +
> + adapter->dt_node = dev->of_node;
> +}
> +
> /*
> * This function adds the card.
> *
> @@ -1568,7 +1578,7 @@ void mwifiex_do_flr(struct mwifiex_adapter *adapter, bool prepare)
> int
> mwifiex_add_card(void *card, struct semaphore *sem,
> struct mwifiex_if_ops *if_ops, u8 iface_type,
> - struct device *dev)
> + struct device *dev, bool of_node_valid)
> {
> struct mwifiex_adapter *adapter;
>
> @@ -1581,6 +1591,9 @@ void mwifiex_do_flr(struct mwifiex_adapter *adapter, bool prepare)
> }
>
> adapter->dev = dev;
> + if (of_node_valid)
> + mwifiex_probe_of(adapter);
> +
> adapter->iface_type = iface_type;
> adapter->card_sem = sem;
>
> diff --git a/drivers/net/wireless/marvell/mwifiex/main.h b/drivers/net/wireless/marvell/mwifiex/main.h
> index 2664513..0c07434 100644
> --- a/drivers/net/wireless/marvell/mwifiex/main.h
> +++ b/drivers/net/wireless/marvell/mwifiex/main.h
> @@ -1413,7 +1413,7 @@ static inline u8 mwifiex_is_tdls_link_setup(u8 status)
> int mwifiex_init_shutdown_fw(struct mwifiex_private *priv,
> u32 func_init_shutdown);
> int mwifiex_add_card(void *, struct semaphore *, struct mwifiex_if_ops *, u8,
> - struct device *);
> + struct device *, bool);
IOW, I think this extra bool flag is a bad idea.
Brian
> int mwifiex_remove_card(struct mwifiex_adapter *, struct semaphore *);
>
> void mwifiex_get_version(struct mwifiex_adapter *adapter, char *version,
> diff --git a/drivers/net/wireless/marvell/mwifiex/pcie.c b/drivers/net/wireless/marvell/mwifiex/pcie.c
> index de6939c..fe7f3b0 100644
> --- a/drivers/net/wireless/marvell/mwifiex/pcie.c
> +++ b/drivers/net/wireless/marvell/mwifiex/pcie.c
> @@ -201,6 +201,7 @@ static int mwifiex_pcie_probe(struct pci_dev *pdev,
> const struct pci_device_id *ent)
> {
> struct pcie_service_card *card;
> + bool valid_of_node = false;
> int ret;
>
> pr_debug("info: vendor=0x%4.04X device=0x%4.04X rev=%d\n",
> @@ -228,10 +229,11 @@ static int mwifiex_pcie_probe(struct pci_dev *pdev,
> ret = mwifiex_pcie_probe_of(&pdev->dev);
> if (ret)
> goto err_free;
> + valid_of_node = true;
> }
>
> ret = mwifiex_add_card(card, &add_remove_card_sem, &pcie_ops,
> - MWIFIEX_PCIE, &pdev->dev);
> + MWIFIEX_PCIE, &pdev->dev, valid_of_node);
> if (ret) {
> pr_err("%s failed\n", __func__);
> goto err_free;
> diff --git a/drivers/net/wireless/marvell/mwifiex/sdio.c b/drivers/net/wireless/marvell/mwifiex/sdio.c
> index c95f41f..558743a 100644
> --- a/drivers/net/wireless/marvell/mwifiex/sdio.c
> +++ b/drivers/net/wireless/marvell/mwifiex/sdio.c
> @@ -156,6 +156,7 @@ static int mwifiex_sdio_probe_of(struct device *dev, struct sdio_mmc_card *card)
> {
> int ret;
> struct sdio_mmc_card *card = NULL;
> + bool valid_of_node = false;
>
> pr_debug("info: vendor=0x%4.04X device=0x%4.04X class=%d function=%d\n",
> func->vendor, func->device, func->class, func->num);
> @@ -203,10 +204,11 @@ static int mwifiex_sdio_probe_of(struct device *dev, struct sdio_mmc_card *card)
> dev_err(&func->dev, "SDIO dt node parse failed\n");
> goto err_disable;
> }
> + valid_of_node = true;
> }
>
> ret = mwifiex_add_card(card, &add_remove_card_sem, &sdio_ops,
> - MWIFIEX_SDIO, &func->dev);
> + MWIFIEX_SDIO, &func->dev, valid_of_node);
> if (ret) {
> dev_err(&func->dev, "add card failed\n");
> goto err_disable;
> diff --git a/drivers/net/wireless/marvell/mwifiex/sta_cmd.c b/drivers/net/wireless/marvell/mwifiex/sta_cmd.c
> index b697b61..bcd6408 100644
> --- a/drivers/net/wireless/marvell/mwifiex/sta_cmd.c
> +++ b/drivers/net/wireless/marvell/mwifiex/sta_cmd.c
> @@ -2235,10 +2235,7 @@ int mwifiex_sta_init_cmd(struct mwifiex_private *priv, u8 first_sta, bool init)
> * The cal-data can be read from device tree and/or
> * a configuration file and downloaded to firmware.
> */
> - if ((priv->adapter->iface_type == MWIFIEX_SDIO ||
> - priv->adapter->iface_type == MWIFIEX_PCIE) &&
> - adapter->dev->of_node) {
> - adapter->dt_node = adapter->dev->of_node;
> + if (adapter->dt_node) {
> if (of_property_read_u32(adapter->dt_node,
> "marvell,wakeup-pin",
> &data) == 0) {
> diff --git a/drivers/net/wireless/marvell/mwifiex/usb.c b/drivers/net/wireless/marvell/mwifiex/usb.c
> index f847fff..11c9629 100644
> --- a/drivers/net/wireless/marvell/mwifiex/usb.c
> +++ b/drivers/net/wireless/marvell/mwifiex/usb.c
> @@ -476,7 +476,7 @@ static int mwifiex_usb_probe(struct usb_interface *intf,
> usb_set_intfdata(intf, card);
>
> ret = mwifiex_add_card(card, &add_remove_card_sem, &usb_ops,
> - MWIFIEX_USB, &card->udev->dev);
> + MWIFIEX_USB, &card->udev->dev, false);
> if (ret) {
> pr_err("%s: mwifiex_add_card failed: %d\n", __func__, ret);
> usb_reset_device(udev);
> --
> 1.9.1
>
^ permalink raw reply
* Re: [PATCH v2 3/3] mwifiex: Enable WoWLAN for both sdio and pcie
From: Brian Norris @ 2016-11-11 20:42 UTC (permalink / raw)
To: Amitkumar Karwar
Cc: linux-wireless, Cathy Luo, Nishant Sarmukadam, rajatja,
dmitry.torokhov
In-Reply-To: <1478862911-15498-3-git-send-email-akarwar@marvell.com>
On Fri, Nov 11, 2016 at 04:45:11PM +0530, Amitkumar Karwar wrote:
> From: Rajat Jain <rajatja@google.com>
>
> Commit ce4f6f0c353b ("mwifiex: add platform specific wakeup interrupt
> support") added WoWLAN feature only for sdio. This patch moves that
> code to the common module so that all the interface drivers can use
> it for free. It enables pcie and sdio for its use currently.
>
> Signed-off-by: Rajat Jain <rajatja@google.com>
> ---
> v2: v1 doesn't apply smoothly on latest code due to recently merged
> patch "mwifiex: report error to PCIe for suspend failure". Minor
> conflict is resolved in v2
> ---
In case you haven't noticed my comments elsewhere, I'll repeat them
here: you're copy-and-pasting buggy code. That can be OK I suppose, but
please fix these issues, possibly in a follow-up patch.
> drivers/net/wireless/marvell/mwifiex/main.c | 41 ++++++++++++++++
> drivers/net/wireless/marvell/mwifiex/main.h | 25 ++++++++++
> drivers/net/wireless/marvell/mwifiex/pcie.c | 2 +
> drivers/net/wireless/marvell/mwifiex/sdio.c | 72 ++---------------------------
> drivers/net/wireless/marvell/mwifiex/sdio.h | 8 ----
> 5 files changed, 73 insertions(+), 75 deletions(-)
>
> diff --git a/drivers/net/wireless/marvell/mwifiex/main.c b/drivers/net/wireless/marvell/mwifiex/main.c
> index b2f3d96..20c9b77 100644
> --- a/drivers/net/wireless/marvell/mwifiex/main.c
> +++ b/drivers/net/wireless/marvell/mwifiex/main.c
> @@ -1552,14 +1552,55 @@ void mwifiex_do_flr(struct mwifiex_adapter *adapter, bool prepare)
> }
> EXPORT_SYMBOL_GPL(mwifiex_do_flr);
>
> +static irqreturn_t mwifiex_irq_wakeup_handler(int irq, void *priv)
> +{
> + struct mwifiex_adapter *adapter = priv;
> +
> + if (adapter->irq_wakeup >= 0) {
> + dev_dbg(adapter->dev, "%s: wake by wifi", __func__);
> + adapter->wake_by_wifi = true;
This flag is unecessary and buggy. IIUC, you're trying to avoid calling
disable_irq() in resume(), if you've called it here?
> + disable_irq_nosync(irq);
...but this is unnecessary, I think, unless you're trying to make up for
buggy wakeup interrupts that keep firing? See my suggestion below.
> + }
> +
> + /* Notify PM core we are wakeup source */
> + pm_wakeup_event(adapter->dev, 0);
> +
> + return IRQ_HANDLED;
> +}
> +
> static void mwifiex_probe_of(struct mwifiex_adapter *adapter)
> {
> + int ret;
> struct device *dev = adapter->dev;
>
> if (!dev->of_node)
> return;
>
> adapter->dt_node = dev->of_node;
> + adapter->irq_wakeup = irq_of_parse_and_map(adapter->dt_node, 0);
> + if (!adapter->irq_wakeup) {
> + dev_info(dev, "fail to parse irq_wakeup from device tree\n");
> + return;
> + }
> +
> + ret = devm_request_irq(dev, adapter->irq_wakeup,
> + mwifiex_irq_wakeup_handler, IRQF_TRIGGER_LOW,
> + "wifi_wake", adapter);
> + if (ret) {
> + dev_err(dev, "Failed to request irq_wakeup %d (%d)\n",
> + adapter->irq_wakeup, ret);
> + goto err_exit;
> + }
> +
> + disable_irq(adapter->irq_wakeup);
> + if (device_init_wakeup(dev, true)) {
> + dev_err(dev, "fail to init wakeup for mwifiex\n");
> + goto err_exit;
> + }
> + return;
> +
> +err_exit:
> + adapter->irq_wakeup = 0;
> }
>
> /*
> diff --git a/drivers/net/wireless/marvell/mwifiex/main.h b/drivers/net/wireless/marvell/mwifiex/main.h
> index 0c07434..11abc49 100644
> --- a/drivers/net/wireless/marvell/mwifiex/main.h
> +++ b/drivers/net/wireless/marvell/mwifiex/main.h
> @@ -1011,6 +1011,10 @@ struct mwifiex_adapter {
> bool usb_mc_setup;
> struct cfg80211_wowlan_nd_info *nd_info;
> struct ieee80211_regdomain *regd;
> +
> + /* Wake-on-WLAN (WoWLAN) */
> + int irq_wakeup;
> + bool wake_by_wifi;
> };
>
> void mwifiex_process_tx_queue(struct mwifiex_adapter *adapter);
> @@ -1410,6 +1414,27 @@ static inline u8 mwifiex_is_tdls_link_setup(u8 status)
> return false;
> }
>
> +/* Disable platform specific wakeup interrupt */
> +static inline void mwifiex_disable_wake(struct mwifiex_adapter *adapter)
> +{
> + if (adapter->irq_wakeup >= 0) {
> + disable_irq_wake(adapter->irq_wakeup);
> + if (!adapter->wake_by_wifi)
You're depending on the wake IRQ handler to set this flag, so you don't
disable the IRQ twice (the calls nest). But what if the IRQ is being
serviced concurrently with this? Then you'll double-disable the IRQ.
I believe you can just remove the disable_irq_nosync() from the handler,
kill the above flag, and just unconditionally disable the IRQ.
> + disable_irq(adapter->irq_wakeup);
> + }
> +}
> +
> +/* Enable platform specific wakeup interrupt */
> +static inline void mwifiex_enable_wake(struct mwifiex_adapter *adapter)
> +{
> + /* Enable platform specific wakeup interrupt */
> + if (adapter->irq_wakeup >= 0) {
> + adapter->wake_by_wifi = false;
> + enable_irq(adapter->irq_wakeup);
> + enable_irq_wake(adapter->irq_wakeup);
> + }
> +}
> +
> int mwifiex_init_shutdown_fw(struct mwifiex_private *priv,
> u32 func_init_shutdown);
> int mwifiex_add_card(void *, struct semaphore *, struct mwifiex_if_ops *, u8,
> diff --git a/drivers/net/wireless/marvell/mwifiex/pcie.c b/drivers/net/wireless/marvell/mwifiex/pcie.c
> index fe7f3b0..a38994e 100644
> --- a/drivers/net/wireless/marvell/mwifiex/pcie.c
> +++ b/drivers/net/wireless/marvell/mwifiex/pcie.c
> @@ -131,6 +131,7 @@ static int mwifiex_pcie_suspend(struct device *dev)
> }
>
> adapter = card->adapter;
> + mwifiex_enable_wake(adapter);
>
> /* Enable the Host Sleep */
> if (!mwifiex_enable_hs(adapter)) {
^^^ What if this fails? Then you'll leave the wake IRQ enabled.
(You've propagated this error from the SDIO driver.)
> @@ -186,6 +187,7 @@ static int mwifiex_pcie_resume(struct device *dev)
>
> mwifiex_cancel_hs(mwifiex_get_priv(adapter, MWIFIEX_BSS_ROLE_STA),
> MWIFIEX_ASYNC_CMD);
> + mwifiex_disable_wake(adapter);
>
> return 0;
> }
> diff --git a/drivers/net/wireless/marvell/mwifiex/sdio.c b/drivers/net/wireless/marvell/mwifiex/sdio.c
> index 558743a..ff5bb45 100644
> --- a/drivers/net/wireless/marvell/mwifiex/sdio.c
> +++ b/drivers/net/wireless/marvell/mwifiex/sdio.c
> @@ -79,67 +79,18 @@
> { }
> };
>
> -static irqreturn_t mwifiex_wake_irq_wifi(int irq, void *priv)
> -{
> - struct mwifiex_plt_wake_cfg *cfg = priv;
> -
> - if (cfg->irq_wifi >= 0) {
> - pr_info("%s: wake by wifi", __func__);
> - cfg->wake_by_wifi = true;
> - disable_irq_nosync(irq);
> - }
> -
> - /* Notify PM core we are wakeup source */
> - pm_wakeup_event(cfg->dev, 0);
> -
> - return IRQ_HANDLED;
> -}
> -
> /* This function parse device tree node using mmc subnode devicetree API.
> * The device node is saved in card->plt_of_node.
> * if the device tree node exist and include interrupts attributes, this
> * function will also request platform specific wakeup interrupt.
> */
> -static int mwifiex_sdio_probe_of(struct device *dev, struct sdio_mmc_card *card)
> +static int mwifiex_sdio_probe_of(struct device *dev)
> {
> - struct mwifiex_plt_wake_cfg *cfg;
> - int ret;
> -
> if (!of_match_node(mwifiex_sdio_of_match_table, dev->of_node)) {
> dev_err(dev, "required compatible string missing\n");
> return -EINVAL;
> }
>
> - card->plt_of_node = dev->of_node;
> - card->plt_wake_cfg = devm_kzalloc(dev, sizeof(*card->plt_wake_cfg),
> - GFP_KERNEL);
> - cfg = card->plt_wake_cfg;
> - if (cfg && card->plt_of_node) {
> - cfg->dev = dev;
> - cfg->irq_wifi = irq_of_parse_and_map(card->plt_of_node, 0);
> - if (!cfg->irq_wifi) {
> - dev_dbg(dev,
> - "fail to parse irq_wifi from device tree\n");
> - } else {
> - ret = devm_request_irq(dev, cfg->irq_wifi,
> - mwifiex_wake_irq_wifi,
> - IRQF_TRIGGER_LOW,
> - "wifi_wake", cfg);
> - if (ret) {
> - dev_dbg(dev,
> - "Failed to request irq_wifi %d (%d)\n",
> - cfg->irq_wifi, ret);
> - card->plt_wake_cfg = NULL;
> - return 0;
> - }
> - disable_irq(cfg->irq_wifi);
> - }
> - }
> -
> - ret = device_init_wakeup(dev, true);
> - if (ret)
> - dev_err(dev, "fail to init wakeup for mwifiex");
> -
> return 0;
> }
>
> @@ -199,11 +150,9 @@ static int mwifiex_sdio_probe_of(struct device *dev, struct sdio_mmc_card *card)
>
> /* device tree node parsing and platform specific configuration*/
> if (func->dev.of_node) {
> - ret = mwifiex_sdio_probe_of(&func->dev, card);
> - if (ret) {
> - dev_err(&func->dev, "SDIO dt node parse failed\n");
> + ret = mwifiex_sdio_probe_of(&func->dev);
> + if (ret)
> goto err_disable;
> - }
> valid_of_node = true;
> }
>
> @@ -269,12 +218,7 @@ static int mwifiex_sdio_resume(struct device *dev)
> mwifiex_cancel_hs(mwifiex_get_priv(adapter, MWIFIEX_BSS_ROLE_STA),
> MWIFIEX_SYNC_CMD);
>
> - /* Disable platform specific wakeup interrupt */
> - if (card->plt_wake_cfg && card->plt_wake_cfg->irq_wifi >= 0) {
> - disable_irq_wake(card->plt_wake_cfg->irq_wifi);
> - if (!card->plt_wake_cfg->wake_by_wifi)
> - disable_irq(card->plt_wake_cfg->irq_wifi);
> - }
> + mwifiex_disable_wake(adapter);
>
> return 0;
> }
> @@ -354,13 +298,7 @@ static int mwifiex_sdio_suspend(struct device *dev)
> }
>
> adapter = card->adapter;
> -
> - /* Enable platform specific wakeup interrupt */
> - if (card->plt_wake_cfg && card->plt_wake_cfg->irq_wifi >= 0) {
> - card->plt_wake_cfg->wake_by_wifi = false;
> - enable_irq(card->plt_wake_cfg->irq_wifi);
> - enable_irq_wake(card->plt_wake_cfg->irq_wifi);
> - }
> + mwifiex_enable_wake(adapter);
>
> /* Enable the Host Sleep */
> if (!mwifiex_enable_hs(adapter)) {
Same problem here. Existing issue.
Brian
> diff --git a/drivers/net/wireless/marvell/mwifiex/sdio.h b/drivers/net/wireless/marvell/mwifiex/sdio.h
> index 07cdd23..b9fbc5c 100644
> --- a/drivers/net/wireless/marvell/mwifiex/sdio.h
> +++ b/drivers/net/wireless/marvell/mwifiex/sdio.h
> @@ -154,12 +154,6 @@
> a->mpa_rx.start_port = 0; \
> } while (0)
>
> -struct mwifiex_plt_wake_cfg {
> - struct device *dev;
> - int irq_wifi;
> - bool wake_by_wifi;
> -};
> -
> /* data structure for SDIO MPA TX */
> struct mwifiex_sdio_mpa_tx {
> /* multiport tx aggregation buffer pointer */
> @@ -243,8 +237,6 @@ struct mwifiex_sdio_card_reg {
> struct sdio_mmc_card {
> struct sdio_func *func;
> struct mwifiex_adapter *adapter;
> - struct device_node *plt_of_node;
> - struct mwifiex_plt_wake_cfg *plt_wake_cfg;
>
> const char *firmware;
> const struct mwifiex_sdio_card_reg *reg;
> --
> 1.9.1
>
^ permalink raw reply
* Re: [PATCH] iwlwifi: fix undefined 6000G2B firmware MAX API version
From: Luca Coelho @ 2016-11-11 19:01 UTC (permalink / raw)
To: Tj, open list:INTEL WIRELESS WIFI LINK (iwlwifi)
Cc: Intel Linux Wireless (supporter:INTEL WIRELESS WIFI LINK (iwlwifi))
In-Reply-To: <87adaa7e-3e64-51eb-f250-bf4f1c025b66@iam.tj>
On Fri, 2016-11-11 at 17:47 +0000, Tj wrote:
> On 11/11/16 08:35, Luca Coelho wrote:
> > Hi,
> > On Thu, 2016-11-10 at 22:04 +0000, Tj wrote:
> > > $ modinfo -F firmware iwlwifi | grep API
> > > iwlwifi-6000g2b-IWL6000G2B_UCODE_API_MAX.ucode
> > > $ modinfo -F vermagic iwlwifi
> > > 4.9.0-040900rc4-lowlatency SMP preempt mod_unload modversions
> > >
> > > Change-Id: Ie21a4be0b12b520844c1da4a8bef9e8a0097d919
> > > Signed-off-by: TJ <linux@iam.tj>
> > > ---
> > > drivers/net/wireless/intel/iwlwifi/iwl-6000.c | 1 +
> > > 1 file changed, 1 insertion(+)
> > >
> > > diff --git a/drivers/net/wireless/intel/iwlwifi/iwl-6000.c
> > > b/drivers/net/wireless/intel/iwlwifi/iwl-6000.c
> > > index 0b9f6a7..19b85e8 100644
> > > --- a/drivers/net/wireless/intel/iwlwifi/iwl-6000.c
> > > +++ b/drivers/net/wireless/intel/iwlwifi/iwl-6000.c
> > > @@ -34,6 +34,7 @@
> > > #define IWL6000_UCODE_API_MAX 6
> > > #define IWL6050_UCODE_API_MAX 5
> > > #define IWL6000G2_UCODE_API_MAX 6
> > > +#define IWL6000G2B_UCODE_API_MAX 6
> > > #define IWL6035_UCODE_API_MAX 6
> > > /* Lowest firmware API version supported */
> >
> > Thanks for your patch! But the correct thing to do would be to change
> > the MODULE_FIRMWARE line instead of adding this define here. The 6030
> > NIC uses IWL6000G2_UCODE_API_MAX in the config structure, so the
> > MODULE_FIRMWARE macro should use that too.
> >
> > diff --git a/drivers/net/wireless/intel/iwlwifi/iwl-6000.c
> > b/drivers/net/wireless/intel/iwlwifi/iwl-6000.c
> > index 0b9f6a7..39335b7 100644
> > --- a/drivers/net/wireless/intel/iwlwifi/iwl-6000.c
> > +++ b/drivers/net/wireless/intel/iwlwifi/iwl-6000.c
> > @@ -371,4 +371,4 @@ const struct iwl_cfg iwl6000_3agn_cfg = {
> > MODULE_FIRMWARE(IWL6000_MODULE_FIRMWARE(IWL6000_UCODE_API_MAX));
> > MODULE_FIRMWARE(IWL6050_MODULE_FIRMWARE(IWL6050_UCODE_API_MAX));
> > MODULE_FIRMWARE(IWL6005_MODULE_FIRMWARE(IWL6000G2_UCODE_API_MAX));
> > -MODULE_FIRMWARE(IWL6030_MODULE_FIRMWARE(IWL6000G2B_UCODE_API_MAX));
> > +MODULE_FIRMWARE(IWL6030_MODULE_FIRMWARE(IWL6000G2_UCODE_API_MAX));
>
> I don't think this is correct. There are different firmware files for
> the 'g2a' (6005) and 'g2b' (6030) and as such the revisions for each
> could be different, so setting the 6030 to use the same API_MAX as the
> 6005 looks wrong to me.
>
> $ ls -1 /lib/firmware/iwlwifi-*6000g*
> /lib/firmware/iwlwifi-6000g2a-5.ucode
> /lib/firmware/iwlwifi-6000g2a-6.ucode
> /lib/firmware/iwlwifi-6000g2b-6.ucode
Maybe you're right, but still, just changing the firmware name is not
enough. If that is really supposed to be the case (and TBH I really
don't know these old NICs), then the config structs should be changed
as well.
In any case, I've applied (internally, for now) the patches that Jürg
sent a while ago. One of them does pretty much the same thing I
suggested her.
--
Cheers,
Luca.
^ permalink raw reply
* Re: [PATCH] iwlwifi: fix undefined 6000G2B firmware MAX API version
From: Tj @ 2016-11-11 17:47 UTC (permalink / raw)
To: Luca Coelho, open list:INTEL WIRELESS WIFI LINK (iwlwifi)
Cc: Intel Linux Wireless (supporter:INTEL WIRELESS WIFI LINK (iwlwifi))
In-Reply-To: <1478853340.15030.21.camel@coelho.fi>
On 11/11/16 08:35, Luca Coelho wrote:
> Hi,
> On Thu, 2016-11-10 at 22:04 +0000, Tj wrote:
>> $ modinfo -F firmware iwlwifi | grep API
>> iwlwifi-6000g2b-IWL6000G2B_UCODE_API_MAX.ucode
>> $ modinfo -F vermagic iwlwifi
>> 4.9.0-040900rc4-lowlatency SMP preempt mod_unload modversions
>>
>> Change-Id: Ie21a4be0b12b520844c1da4a8bef9e8a0097d919
>> Signed-off-by: TJ <linux@iam.tj>
>> ---
>> drivers/net/wireless/intel/iwlwifi/iwl-6000.c | 1 +
>> 1 file changed, 1 insertion(+)
>>
>> diff --git a/drivers/net/wireless/intel/iwlwifi/iwl-6000.c
>> b/drivers/net/wireless/intel/iwlwifi/iwl-6000.c
>> index 0b9f6a7..19b85e8 100644
>> --- a/drivers/net/wireless/intel/iwlwifi/iwl-6000.c
>> +++ b/drivers/net/wireless/intel/iwlwifi/iwl-6000.c
>> @@ -34,6 +34,7 @@
>> #define IWL6000_UCODE_API_MAX 6
>> #define IWL6050_UCODE_API_MAX 5
>> #define IWL6000G2_UCODE_API_MAX 6
>> +#define IWL6000G2B_UCODE_API_MAX 6
>> #define IWL6035_UCODE_API_MAX 6
>> /* Lowest firmware API version supported */
> Thanks for your patch! But the correct thing to do would be to change
> the MODULE_FIRMWARE line instead of adding this define here. The 6030
> NIC uses IWL6000G2_UCODE_API_MAX in the config structure, so the
> MODULE_FIRMWARE macro should use that too.
>
> diff --git a/drivers/net/wireless/intel/iwlwifi/iwl-6000.c
> b/drivers/net/wireless/intel/iwlwifi/iwl-6000.c
> index 0b9f6a7..39335b7 100644
> --- a/drivers/net/wireless/intel/iwlwifi/iwl-6000.c
> +++ b/drivers/net/wireless/intel/iwlwifi/iwl-6000.c
> @@ -371,4 +371,4 @@ const struct iwl_cfg iwl6000_3agn_cfg = {
> MODULE_FIRMWARE(IWL6000_MODULE_FIRMWARE(IWL6000_UCODE_API_MAX));
> MODULE_FIRMWARE(IWL6050_MODULE_FIRMWARE(IWL6050_UCODE_API_MAX));
> MODULE_FIRMWARE(IWL6005_MODULE_FIRMWARE(IWL6000G2_UCODE_API_MAX));
> -MODULE_FIRMWARE(IWL6030_MODULE_FIRMWARE(IWL6000G2B_UCODE_API_MAX));
> +MODULE_FIRMWARE(IWL6030_MODULE_FIRMWARE(IWL6000G2_UCODE_API_MAX));
I don't think this is correct. There are different firmware files for
the 'g2a' (6005) and 'g2b' (6030) and as such the revisions for each
could be different, so setting the 6030 to use the same API_MAX as the
6005 looks wrong to me.
$ ls -1 /lib/firmware/iwlwifi-*6000g*
/lib/firmware/iwlwifi-6000g2a-5.ucode
/lib/firmware/iwlwifi-6000g2a-6.ucode
/lib/firmware/iwlwifi-6000g2b-6.ucode
^ permalink raw reply
* [RFC] change mac80211_hwsim tx_rates to ieee80211_tx_rate
From: Benjamin Beichler @ 2016-11-11 17:22 UTC (permalink / raw)
To: linux-wireless
Hi,
we are working on a sophisticated Wifi simulation framework based on
mac80211_hwsim. This includes the simulation of frame transmissions with
realistic channel access and channel conditions. Therefore we need
several information (e.g. long/short gi, usage of RTS/CTS, ...), which
are available on a per frame/rate basis from struct ieee80211_tx_rate.
But this information is thrown away by the conversation to struct
hwsim_tx_rate (eg. in mac80211_hwsim_tx_frame_nl)
Firstly I think this information is valuable and I don't believe, the 4
Byte per frame greatly influences speed. Moreover the explicit double
copy (firstly from the info->status.rates to tx_attempts, secondly by
nla_put), takes longer than simply put the whole info->status.rates into
the netlink message.
So I would propose to put the whole struct into the netlink messages,
but I think that will break up the communication to e.g. bob copelands
wmediumd and similar simulations. I would like to have our
Implementation working with mainline kernels and therefore ask how we
could achieve this easily.
Obviously, we could define another field in the hwsim messages, but as
bob copeland already stated, significantly more information within the
netlink messages could intensify the timing overhead of hwsim.
Any suggestions?
^ permalink raw reply
* wl1251 & mac address & calibration data
From: Pali Rohár @ 2016-11-11 17:20 UTC (permalink / raw)
To: Kalle Valo, Pavel Machek, Ivaylo Dimitrov, Sebastian Reichel,
Aaro Koskinen, Tony Lindgren
Cc: linux-wireless, netdev, linux-kernel
[-- Attachment #1: Type: Text/Plain, Size: 1626 bytes --]
Hi! I will open discussion about mac address and calibration data for
wl1251 wireless chip again...
Problem: Mac address & calibration data for wl1251 chip on Nokia N900
are stored on second nand partition (mtd1) in special proprietary format
which is used only for Nokia N900 (probably on N8x0 and N9 too).
Wireless driver wl1251.ko cannot work without mac address and
calibration data.
Absence of mac address cause that driver generates random mac address at
every kernel boot which has couple of problems (unstable identifier of
wireless device due to udev permanent storage rules; unpredictable
behaviour for dhcp mac address assignment, mac address filtering, ...).
Currently there is no way to set (permanent) mac address for network
interface from userspace. And it does not make sense to implement in
linux kernel large parser for proprietary format of second nand
partition where is mac address stored only for one device -- Nokia N900.
Driver wl1251.ko loads calibration data via request_firmware() for file
wl1251-nvs.bin. There are some "example" calibration file in linux-
firmware repository, but it is not suitable for normal usage as real
calibration data are per-device specific.
So questions are:
1) How to set mac address from userspace for that wl1251 interface? In
userspace I can write parser for that proprietary format of nand
partition and extract mac address from it
2) How to send calibration data to wl1251 driver? Those are again stored
in proprietary format and I can write userspace parser for it.
--
Pali Rohár
pali.rohar@gmail.com
[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 198 bytes --]
^ permalink raw reply
* [PATCH] fixed hwsim beacon delta calculation
From: Benjamin Beichler @ 2016-11-11 16:37 UTC (permalink / raw)
To: linux-wireless, johannes; +Cc: Benjamin Beichler
Due to the cast from uint32_t to int64_t, a wrong next beacon timing is
calculated and effectively the beacon timer stops to work. This is
especially bad for 802.11s mesh networks, because discovery breaks
without beacons.
Signed-off-by: Benjamin Beichler <benjamin.beichler@uni-rostock.de>
---
drivers/net/wireless/mac80211_hwsim.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/net/wireless/mac80211_hwsim.c b/drivers/net/wireless/mac80211_hwsim.c
index 8f366cc..8d7b0c6 100644
--- a/drivers/net/wireless/mac80211_hwsim.c
+++ b/drivers/net/wireless/mac80211_hwsim.c
@@ -819,7 +819,7 @@ static void mac80211_hwsim_set_tsf(struct ieee80211_hw *hw,
data->bcn_delta = do_div(delta, bcn_int);
} else {
data->tsf_offset -= delta;
- data->bcn_delta = -do_div(delta, bcn_int);
+ data->bcn_delta = -(s64)(do_div(delta, bcn_int));
}
}
--
1.9.1
^ permalink raw reply related
* Re: [linuxwifi] [PATCH 1/2] iwlwifi: fix MODULE_FIRMWARE for 6030
From: Luca Coelho @ 2016-11-11 13:56 UTC (permalink / raw)
To: Jürg Billeter, Emmanuel Grumbach, Sara Sharon
Cc: linuxwifi, Kalle Valo, linux-kernel, linux-wireless
In-Reply-To: <20161010163001.22865-1-j@bitron.ch>
Hi Jürg,
On Mon, 2016-10-10 at 18:30 +0200, Jürg Billeter wrote:
> IWL6000G2B_UCODE_API_MAX is not defined. ucode_api_max of
> IWL_DEVICE_6030 uses IWL6000G2_UCODE_API_MAX. Use this also for
> MODULE_FIRMWARE.
>
> Fixes: 9d9b21d1b616 ("iwlwifi: remove IWL_*_UCODE_API_OK")
> Signed-off-by: Jürg Billeter <j@bitron.ch>
> ---
> drivers/net/wireless/intel/iwlwifi/iwl-6000.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/net/wireless/intel/iwlwifi/iwl-6000.c b/drivers/net/wireless/intel/iwlwifi/iwl-6000.c
> index 0b9f6a7..39335b7 100644
> --- a/drivers/net/wireless/intel/iwlwifi/iwl-6000.c
> +++ b/drivers/net/wireless/intel/iwlwifi/iwl-6000.c
> @@ -371,4 +371,4 @@ const struct iwl_cfg iwl6000_3agn_cfg = {
> MODULE_FIRMWARE(IWL6000_MODULE_FIRMWARE(IWL6000_UCODE_API_MAX));
> MODULE_FIRMWARE(IWL6050_MODULE_FIRMWARE(IWL6050_UCODE_API_MAX));
> MODULE_FIRMWARE(IWL6005_MODULE_FIRMWARE(IWL6000G2_UCODE_API_MAX));
> -MODULE_FIRMWARE(IWL6030_MODULE_FIRMWARE(IWL6000G2B_UCODE_API_MAX));
> +MODULE_FIRMWARE(IWL6030_MODULE_FIRMWARE(IWL6000G2_UCODE_API_MAX));
Sorry I had missed these patches because linux-wireless@vger was not
CCed, so it didn't get to patchwork. I have applied both in our
internal tree and they'll reach the mainline in my next pull-request.
Thanks!
--
Cheers,
Luca.
^ permalink raw reply
* [PATCH v3 11/11] mwifiex: pcie: stop checking for NULL adapter->card
From: Amitkumar Karwar @ 2016-11-11 13:10 UTC (permalink / raw)
To: linux-wireless
Cc: Cathy Luo, Nishant Sarmukadam, rajatja, briannorris,
dmitry.torokhov, Brian Norris
In-Reply-To: <1478869818-4340-1-git-send-email-akarwar@marvell.com>
From: Brian Norris <briannorris@chromium.org>
It should never be NULL here, and to think otherwise makes things
confusing.
Signed-off-by: Brian Norris <briannorris@chromium.org>
---
v2: Same as v1
v3: Below checkpatch warnings are resolved
WARNING: please, no spaces at the start of a line
#50: FILE: drivers/net/wireless/marvell/mwifiex/pcie.c:3037:
+ } else {$
WARNING: please, no spaces at the start of a line
#62: FILE: drivers/net/wireless/marvell/mwifiex/pcie.c:3044:
+ }$
---
drivers/net/wireless/marvell/mwifiex/pcie.c | 53 +++++++++++++----------------
1 file changed, 23 insertions(+), 30 deletions(-)
diff --git a/drivers/net/wireless/marvell/mwifiex/pcie.c b/drivers/net/wireless/marvell/mwifiex/pcie.c
index 3b6d908..fb2b22d 100644
--- a/drivers/net/wireless/marvell/mwifiex/pcie.c
+++ b/drivers/net/wireless/marvell/mwifiex/pcie.c
@@ -3021,31 +3021,28 @@ static int mwifiex_register_dev(struct mwifiex_adapter *adapter)
static void mwifiex_unregister_dev(struct mwifiex_adapter *adapter)
{
struct pcie_service_card *card = adapter->card;
- struct pci_dev *pdev;
+ struct pci_dev *pdev = card->dev;
int i;
- if (card) {
- pdev = card->dev;
- if (card->msix_enable) {
- for (i = 0; i < MWIFIEX_NUM_MSIX_VECTORS; i++)
- synchronize_irq(card->msix_entries[i].vector);
+ if (card->msix_enable) {
+ for (i = 0; i < MWIFIEX_NUM_MSIX_VECTORS; i++)
+ synchronize_irq(card->msix_entries[i].vector);
- for (i = 0; i < MWIFIEX_NUM_MSIX_VECTORS; i++)
- free_irq(card->msix_entries[i].vector,
- &card->msix_ctx[i]);
+ for (i = 0; i < MWIFIEX_NUM_MSIX_VECTORS; i++)
+ free_irq(card->msix_entries[i].vector,
+ &card->msix_ctx[i]);
- card->msix_enable = 0;
- pci_disable_msix(pdev);
- } else {
- mwifiex_dbg(adapter, INFO,
- "%s(): calling free_irq()\n", __func__);
- free_irq(card->dev->irq, &card->share_irq_ctx);
+ card->msix_enable = 0;
+ pci_disable_msix(pdev);
+ } else {
+ mwifiex_dbg(adapter, INFO,
+ "%s(): calling free_irq()\n", __func__);
+ free_irq(card->dev->irq, &card->share_irq_ctx);
- if (card->msi_enable)
- pci_disable_msi(pdev);
- }
- card->adapter = NULL;
+ if (card->msi_enable)
+ pci_disable_msi(pdev);
}
+ card->adapter = NULL;
}
/* This function initializes the PCI-E host memory space, WCB rings, etc.
@@ -3128,18 +3125,14 @@ static void mwifiex_pcie_down_dev(struct mwifiex_adapter *adapter)
adapter->seq_num = 0;
adapter->tx_buf_size = MWIFIEX_TX_DATA_BUF_SIZE_4K;
- if (card) {
- if (reg->sleep_cookie)
- mwifiex_pcie_delete_sleep_cookie_buf(adapter);
-
- mwifiex_pcie_delete_cmdrsp_buf(adapter);
- mwifiex_pcie_delete_evtbd_ring(adapter);
- mwifiex_pcie_delete_rxbd_ring(adapter);
- mwifiex_pcie_delete_txbd_ring(adapter);
- card->cmdrsp_buf = NULL;
- }
+ if (reg->sleep_cookie)
+ mwifiex_pcie_delete_sleep_cookie_buf(adapter);
- return;
+ mwifiex_pcie_delete_cmdrsp_buf(adapter);
+ mwifiex_pcie_delete_evtbd_ring(adapter);
+ mwifiex_pcie_delete_rxbd_ring(adapter);
+ mwifiex_pcie_delete_txbd_ring(adapter);
+ card->cmdrsp_buf = NULL;
}
static struct mwifiex_if_ops pcie_ops = {
--
1.9.1
^ permalink raw reply related
* [PATCH v3 09/11] mwifiex: sdio: don't check for NULL sdio_func
From: Amitkumar Karwar @ 2016-11-11 13:10 UTC (permalink / raw)
To: linux-wireless
Cc: Cathy Luo, Nishant Sarmukadam, rajatja, briannorris,
dmitry.torokhov, Brian Norris, Amitkumar Karwar
In-Reply-To: <1478869818-4340-1-git-send-email-akarwar@marvell.com>
From: Brian Norris <briannorris@chromium.org>
sdio_func is retrieved via container_of() and should never be NULL.
Checking for NULL just makes the logic more confusing than necessary.
Stop doing that.
Signed-off-by: Brian Norris <briannorris@chromium.org>
Tested-by: Xinming Hu <huxm@marvell.com>
Signed-off-by: Amitkumar Karwar <akarwar@marvell.com>
---
v3: Same as v1 and v2
---
drivers/net/wireless/marvell/mwifiex/sdio.c | 40 +++++++++++------------------
1 file changed, 15 insertions(+), 25 deletions(-)
diff --git a/drivers/net/wireless/marvell/mwifiex/sdio.c b/drivers/net/wireless/marvell/mwifiex/sdio.c
index 4c4b012..bee7326 100644
--- a/drivers/net/wireless/marvell/mwifiex/sdio.c
+++ b/drivers/net/wireless/marvell/mwifiex/sdio.c
@@ -190,15 +190,10 @@ static int mwifiex_sdio_resume(struct device *dev)
struct mwifiex_adapter *adapter;
mmc_pm_flag_t pm_flag = 0;
- if (func) {
- pm_flag = sdio_get_host_pm_caps(func);
- card = sdio_get_drvdata(func);
- if (!card || !card->adapter) {
- pr_err("resume: invalid card or adapter\n");
- return 0;
- }
- } else {
- pr_err("resume: sdio_func is not specified\n");
+ pm_flag = sdio_get_host_pm_caps(func);
+ card = sdio_get_drvdata(func);
+ if (!card || !card->adapter) {
+ dev_err(dev, "resume: invalid card or adapter\n");
return 0;
}
@@ -274,23 +269,18 @@ static int mwifiex_sdio_suspend(struct device *dev)
mmc_pm_flag_t pm_flag = 0;
int ret = 0;
- if (func) {
- pm_flag = sdio_get_host_pm_caps(func);
- pr_debug("cmd: %s: suspend: PM flag = 0x%x\n",
- sdio_func_id(func), pm_flag);
- if (!(pm_flag & MMC_PM_KEEP_POWER)) {
- pr_err("%s: cannot remain alive while host is"
- " suspended\n", sdio_func_id(func));
- return -ENOSYS;
- }
+ pm_flag = sdio_get_host_pm_caps(func);
+ pr_debug("cmd: %s: suspend: PM flag = 0x%x\n",
+ sdio_func_id(func), pm_flag);
+ if (!(pm_flag & MMC_PM_KEEP_POWER)) {
+ dev_err(dev, "%s: cannot remain alive while host is"
+ " suspended\n", sdio_func_id(func));
+ return -ENOSYS;
+ }
- card = sdio_get_drvdata(func);
- if (!card) {
- dev_err(dev, "suspend: invalid card\n");
- return 0;
- }
- } else {
- pr_err("suspend: sdio_func is not specified\n");
+ card = sdio_get_drvdata(func);
+ if (!card) {
+ dev_err(dev, "suspend: invalid card\n");
return 0;
}
--
1.9.1
^ permalink raw reply related
* [PATCH v3 10/11] mwifiex: stop checking for NULL drvata/intfdata
From: Amitkumar Karwar @ 2016-11-11 13:10 UTC (permalink / raw)
To: linux-wireless
Cc: Cathy Luo, Nishant Sarmukadam, rajatja, briannorris,
dmitry.torokhov, Brian Norris, Amitkumar Karwar
In-Reply-To: <1478869818-4340-1-git-send-email-akarwar@marvell.com>
From: Brian Norris <briannorris@chromium.org>
These are never NULL, so stop making people think they might be.
I don't change this for SDIO because SDIO has a racy card-reset handler
that reallocates this struct. I'd rather not touch that mess right now.
Signed-off-by: Brian Norris <briannorris@chromium.org>
Tested-by: Xinming Hu <huxm@marvell.com>
Signed-off-by: Amitkumar Karwar <akarwar@marvell.com>
---
v3: Same as v1 and v2
---
drivers/net/wireless/marvell/mwifiex/pcie.c | 14 +++++---------
drivers/net/wireless/marvell/mwifiex/usb.c | 15 +++------------
2 files changed, 8 insertions(+), 21 deletions(-)
diff --git a/drivers/net/wireless/marvell/mwifiex/pcie.c b/drivers/net/wireless/marvell/mwifiex/pcie.c
index 1ab366c..3b6d908 100644
--- a/drivers/net/wireless/marvell/mwifiex/pcie.c
+++ b/drivers/net/wireless/marvell/mwifiex/pcie.c
@@ -118,10 +118,6 @@ static int mwifiex_pcie_suspend(struct device *dev)
struct pci_dev *pdev = to_pci_dev(dev);
card = pci_get_drvdata(pdev);
- if (!card) {
- dev_err(dev, "card structure is not valid\n");
- return 0;
- }
/* Might still be loading firmware */
wait_for_completion(&card->fw_done);
@@ -166,8 +162,9 @@ static int mwifiex_pcie_resume(struct device *dev)
struct pci_dev *pdev = to_pci_dev(dev);
card = pci_get_drvdata(pdev);
- if (!card || !card->adapter) {
- dev_err(dev, "Card or adapter structure is not valid\n");
+
+ if (!card->adapter) {
+ dev_err(dev, "adapter structure is not valid\n");
return 0;
}
@@ -255,8 +252,6 @@ static void mwifiex_pcie_remove(struct pci_dev *pdev)
struct mwifiex_private *priv;
card = pci_get_drvdata(pdev);
- if (!card)
- return;
wait_for_completion(&card->fw_done);
@@ -2249,7 +2244,8 @@ static irqreturn_t mwifiex_pcie_interrupt(int irq, void *context)
}
card = pci_get_drvdata(pdev);
- if (!card || !card->adapter) {
+
+ if (!card->adapter) {
pr_err("info: %s: card=%p adapter=%p\n", __func__, card,
card ? card->adapter : NULL);
goto exit;
diff --git a/drivers/net/wireless/marvell/mwifiex/usb.c b/drivers/net/wireless/marvell/mwifiex/usb.c
index 8bcfd92..806ded6 100644
--- a/drivers/net/wireless/marvell/mwifiex/usb.c
+++ b/drivers/net/wireless/marvell/mwifiex/usb.c
@@ -503,11 +503,6 @@ static int mwifiex_usb_suspend(struct usb_interface *intf, pm_message_t message)
struct usb_tx_data_port *port;
int i, j;
- if (!card) {
- dev_err(&intf->dev, "%s: card is NULL\n", __func__);
- return 0;
- }
-
/* Might still be loading firmware */
wait_for_completion(&card->fw_done);
@@ -574,8 +569,9 @@ static int mwifiex_usb_resume(struct usb_interface *intf)
struct mwifiex_adapter *adapter;
int i;
- if (!card || !card->adapter) {
- pr_err("%s: card or card->adapter is NULL\n", __func__);
+ if (!card->adapter) {
+ dev_err(&intf->dev, "%s: card->adapter is NULL\n",
+ __func__);
return 0;
}
adapter = card->adapter;
@@ -617,11 +613,6 @@ static void mwifiex_usb_disconnect(struct usb_interface *intf)
struct usb_card_rec *card = usb_get_intfdata(intf);
struct mwifiex_adapter *adapter;
- if (!card) {
- dev_err(&intf->dev, "%s: card is NULL\n", __func__);
- return;
- }
-
wait_for_completion(&card->fw_done);
adapter = card->adapter;
--
1.9.1
^ permalink raw reply related
* [PATCH v3 08/11] mwifiex: usb: handle HS failures
From: Amitkumar Karwar @ 2016-11-11 13:10 UTC (permalink / raw)
To: linux-wireless
Cc: Cathy Luo, Nishant Sarmukadam, rajatja, briannorris,
dmitry.torokhov, Brian Norris, Amitkumar Karwar
In-Reply-To: <1478869818-4340-1-git-send-email-akarwar@marvell.com>
From: Brian Norris <briannorris@chromium.org>
SDIO and PCIe drivers handle this. Let's imitate it.
Signed-off-by: Brian Norris <briannorris@chromium.org>
Tested-by: Xinming Hu <huxm@marvell.com>
Signed-off-by: Amitkumar Karwar <akarwar@marvell.com>
---
v3: Same as v1 and v2
---
drivers/net/wireless/marvell/mwifiex/usb.c | 9 ++++++++-
1 file changed, 8 insertions(+), 1 deletion(-)
diff --git a/drivers/net/wireless/marvell/mwifiex/usb.c b/drivers/net/wireless/marvell/mwifiex/usb.c
index 671702c..8bcfd92 100644
--- a/drivers/net/wireless/marvell/mwifiex/usb.c
+++ b/drivers/net/wireless/marvell/mwifiex/usb.c
@@ -521,7 +521,14 @@ static int mwifiex_usb_suspend(struct usb_interface *intf, pm_message_t message)
mwifiex_dbg(adapter, WARN,
"Device already suspended\n");
- mwifiex_enable_hs(adapter);
+ /* Enable the Host Sleep */
+ if (!mwifiex_enable_hs(adapter)) {
+ mwifiex_dbg(adapter, ERROR,
+ "cmd: failed to suspend\n");
+ adapter->hs_enabling = false;
+ return -EFAULT;
+ }
+
/* 'is_suspended' flag indicates device is suspended.
* It must be set here before the usb_kill_urb() calls. Reason
--
1.9.1
^ permalink raw reply related
* [PATCH v3 07/11] mwifiex: reset card->adapter during device unregister
From: Amitkumar Karwar @ 2016-11-11 13:10 UTC (permalink / raw)
To: linux-wireless
Cc: Cathy Luo, Nishant Sarmukadam, rajatja, briannorris,
dmitry.torokhov, Xinming Hu, Amitkumar Karwar, Brian Norris
In-Reply-To: <1478869818-4340-1-git-send-email-akarwar@marvell.com>
From: Xinming Hu <huxm@marvell.com>
card->adapter gets initialized in mwifiex_register_dev(). As it's not
cleared in mwifiex_unregister_dev(), we may end up accessing the memory
which is already free in below scenario.
Scenario: Driver initialization is failed due to incorrect firmware or
some other reason. Meanwhile device reboot/unload occurs.
This is safe, now that we've properly synchronized suspend() and
remove() with the FW initialization thread; now that code can simply
check for 'card->adapter == NULL' and exit safely.
Signed-off-by: Xinming Hu <huxm@marvell.com>
Tested-by: Xinming Hu <huxm@marvell.com>
Signed-off-by: Amitkumar Karwar <akarwar@marvell.com>
Signed-off-by: Brian Norris <briannorris@chromium.org>
---
v3: Same as v1 and v2
---
drivers/net/wireless/marvell/mwifiex/pcie.c | 1 +
drivers/net/wireless/marvell/mwifiex/sdio.c | 1 +
2 files changed, 2 insertions(+)
diff --git a/drivers/net/wireless/marvell/mwifiex/pcie.c b/drivers/net/wireless/marvell/mwifiex/pcie.c
index 4d96683..1ab366c 100644
--- a/drivers/net/wireless/marvell/mwifiex/pcie.c
+++ b/drivers/net/wireless/marvell/mwifiex/pcie.c
@@ -3048,6 +3048,7 @@ static void mwifiex_unregister_dev(struct mwifiex_adapter *adapter)
if (card->msi_enable)
pci_disable_msi(pdev);
}
+ card->adapter = NULL;
}
}
diff --git a/drivers/net/wireless/marvell/mwifiex/sdio.c b/drivers/net/wireless/marvell/mwifiex/sdio.c
index 39ffe7d..4c4b012 100644
--- a/drivers/net/wireless/marvell/mwifiex/sdio.c
+++ b/drivers/net/wireless/marvell/mwifiex/sdio.c
@@ -2019,6 +2019,7 @@ static int mwifiex_alloc_sdio_mpa_buffers(struct mwifiex_adapter *adapter,
struct sdio_mmc_card *card = adapter->card;
if (adapter->card) {
+ card->adapter = NULL;
sdio_claim_host(card->func);
sdio_disable_func(card->func);
sdio_release_host(card->func);
--
1.9.1
^ permalink raw reply related
* [PATCH v3 06/11] mwifiex: resolve suspend() race with async FW init failure
From: Amitkumar Karwar @ 2016-11-11 13:10 UTC (permalink / raw)
To: linux-wireless
Cc: Cathy Luo, Nishant Sarmukadam, rajatja, briannorris,
dmitry.torokhov, Brian Norris, Amitkumar Karwar
In-Reply-To: <1478869818-4340-1-git-send-email-akarwar@marvell.com>
From: Brian Norris <briannorris@chromium.org>
Signed-off-by: Brian Norris <briannorris@chromium.org>
Tested-by: Xinming Hu <huxm@marvell.com>
Signed-off-by: Amitkumar Karwar <akarwar@marvell.com>
---
v3: Same as v1 and v2
---
drivers/net/wireless/marvell/mwifiex/pcie.c | 12 ++++++++++--
drivers/net/wireless/marvell/mwifiex/sdio.c | 12 ++++++++++--
drivers/net/wireless/marvell/mwifiex/usb.c | 12 ++++++++++--
3 files changed, 30 insertions(+), 6 deletions(-)
diff --git a/drivers/net/wireless/marvell/mwifiex/pcie.c b/drivers/net/wireless/marvell/mwifiex/pcie.c
index a2353f9..4d96683 100644
--- a/drivers/net/wireless/marvell/mwifiex/pcie.c
+++ b/drivers/net/wireless/marvell/mwifiex/pcie.c
@@ -118,12 +118,20 @@ static int mwifiex_pcie_suspend(struct device *dev)
struct pci_dev *pdev = to_pci_dev(dev);
card = pci_get_drvdata(pdev);
- if (!card || !card->adapter) {
- pr_err("Card or adapter structure is not valid\n");
+ if (!card) {
+ dev_err(dev, "card structure is not valid\n");
return 0;
}
+ /* Might still be loading firmware */
+ wait_for_completion(&card->fw_done);
+
adapter = card->adapter;
+ if (!adapter) {
+ dev_err(dev, "adapter is not valid\n");
+ return 0;
+ }
+
mwifiex_enable_wake(adapter);
/* Enable the Host Sleep */
diff --git a/drivers/net/wireless/marvell/mwifiex/sdio.c b/drivers/net/wireless/marvell/mwifiex/sdio.c
index a2257a4..39ffe7d 100644
--- a/drivers/net/wireless/marvell/mwifiex/sdio.c
+++ b/drivers/net/wireless/marvell/mwifiex/sdio.c
@@ -285,8 +285,8 @@ static int mwifiex_sdio_suspend(struct device *dev)
}
card = sdio_get_drvdata(func);
- if (!card || !card->adapter) {
- pr_err("suspend: invalid card or adapter\n");
+ if (!card) {
+ dev_err(dev, "suspend: invalid card\n");
return 0;
}
} else {
@@ -294,7 +294,15 @@ static int mwifiex_sdio_suspend(struct device *dev)
return 0;
}
+ /* Might still be loading firmware */
+ wait_for_completion(&card->fw_done);
+
adapter = card->adapter;
+ if (!adapter) {
+ dev_err(dev, "adapter is not valid\n");
+ return 0;
+ }
+
mwifiex_enable_wake(adapter);
/* Enable the Host Sleep */
diff --git a/drivers/net/wireless/marvell/mwifiex/usb.c b/drivers/net/wireless/marvell/mwifiex/usb.c
index 558a7f1..671702c 100644
--- a/drivers/net/wireless/marvell/mwifiex/usb.c
+++ b/drivers/net/wireless/marvell/mwifiex/usb.c
@@ -503,11 +503,19 @@ static int mwifiex_usb_suspend(struct usb_interface *intf, pm_message_t message)
struct usb_tx_data_port *port;
int i, j;
- if (!card || !card->adapter) {
- pr_err("%s: card or card->adapter is NULL\n", __func__);
+ if (!card) {
+ dev_err(&intf->dev, "%s: card is NULL\n", __func__);
return 0;
}
+
+ /* Might still be loading firmware */
+ wait_for_completion(&card->fw_done);
+
adapter = card->adapter;
+ if (!adapter) {
+ dev_err(&intf->dev, "card is not valid\n");
+ return 0;
+ }
if (unlikely(adapter->is_suspended))
mwifiex_dbg(adapter, WARN,
--
1.9.1
^ permalink raw reply related
* [PATCH v3 05/11] mwifiex: don't pretend to resume while remove()'ing
From: Amitkumar Karwar @ 2016-11-11 13:10 UTC (permalink / raw)
To: linux-wireless
Cc: Cathy Luo, Nishant Sarmukadam, rajatja, briannorris,
dmitry.torokhov, Brian Norris, Amitkumar Karwar
In-Reply-To: <1478869818-4340-1-git-send-email-akarwar@marvell.com>
From: Brian Norris <briannorris@chromium.org>
The device core will not allow suspend() to race with remove().
Signed-off-by: Brian Norris <briannorris@chromium.org>
Tested-by: Xinming Hu <huxm@marvell.com>
Signed-off-by: Amitkumar Karwar <akarwar@marvell.com>
---
v3: Same as v1 and v2
---
drivers/net/wireless/marvell/mwifiex/pcie.c | 5 -----
drivers/net/wireless/marvell/mwifiex/sdio.c | 3 ---
drivers/net/wireless/marvell/mwifiex/usb.c | 5 -----
3 files changed, 13 deletions(-)
diff --git a/drivers/net/wireless/marvell/mwifiex/pcie.c b/drivers/net/wireless/marvell/mwifiex/pcie.c
index 6152f08..a2353f9 100644
--- a/drivers/net/wireless/marvell/mwifiex/pcie.c
+++ b/drivers/net/wireless/marvell/mwifiex/pcie.c
@@ -257,11 +257,6 @@ static void mwifiex_pcie_remove(struct pci_dev *pdev)
return;
if (user_rmmod && !adapter->mfg_mode) {
-#ifdef CONFIG_PM_SLEEP
- if (adapter->is_suspended)
- mwifiex_pcie_resume(&pdev->dev);
-#endif
-
mwifiex_deauthenticate_all(adapter);
priv = mwifiex_get_priv(adapter, MWIFIEX_BSS_ROLE_ANY);
diff --git a/drivers/net/wireless/marvell/mwifiex/sdio.c b/drivers/net/wireless/marvell/mwifiex/sdio.c
index 90f45d9..a2257a4 100644
--- a/drivers/net/wireless/marvell/mwifiex/sdio.c
+++ b/drivers/net/wireless/marvell/mwifiex/sdio.c
@@ -246,9 +246,6 @@ static int mwifiex_sdio_resume(struct device *dev)
mwifiex_dbg(adapter, INFO, "info: SDIO func num=%d\n", func->num);
if (user_rmmod && !adapter->mfg_mode) {
- if (adapter->is_suspended)
- mwifiex_sdio_resume(adapter->dev);
-
mwifiex_deauthenticate_all(adapter);
priv = mwifiex_get_priv(adapter, MWIFIEX_BSS_ROLE_ANY);
diff --git a/drivers/net/wireless/marvell/mwifiex/usb.c b/drivers/net/wireless/marvell/mwifiex/usb.c
index 14f89fe..558a7f1 100644
--- a/drivers/net/wireless/marvell/mwifiex/usb.c
+++ b/drivers/net/wireless/marvell/mwifiex/usb.c
@@ -614,11 +614,6 @@ static void mwifiex_usb_disconnect(struct usb_interface *intf)
return;
if (user_rmmod && !adapter->mfg_mode) {
-#ifdef CONFIG_PM
- if (adapter->is_suspended)
- mwifiex_usb_resume(intf);
-#endif
-
mwifiex_deauthenticate_all(adapter);
mwifiex_init_shutdown_fw(mwifiex_get_priv(adapter,
--
1.9.1
^ permalink raw reply related
* [PATCH v3 04/11] mwifiex: remove redundant pdev check in suspend/resume handlers
From: Amitkumar Karwar @ 2016-11-11 13:10 UTC (permalink / raw)
To: linux-wireless
Cc: Cathy Luo, Nishant Sarmukadam, rajatja, briannorris,
dmitry.torokhov, Amitkumar Karwar, Brian Norris
In-Reply-To: <1478869818-4340-1-git-send-email-akarwar@marvell.com>
to_pci_dev() would just do struct offset arithmetic on struct
device to get 'pdev' pointer. We never get NULL pdev pointer.
Signed-off-by: Amitkumar Karwar <akarwar@marvell.com>
Tested-by: Xinming Hu <huxm@marvell.com>
Signed-off-by: Brian Norris <briannorris@chromium.org>
---
v3: Same as v1 and v2
---
drivers/net/wireless/marvell/mwifiex/pcie.c | 22 ++++++----------------
1 file changed, 6 insertions(+), 16 deletions(-)
diff --git a/drivers/net/wireless/marvell/mwifiex/pcie.c b/drivers/net/wireless/marvell/mwifiex/pcie.c
index c4bd64b..6152f08 100644
--- a/drivers/net/wireless/marvell/mwifiex/pcie.c
+++ b/drivers/net/wireless/marvell/mwifiex/pcie.c
@@ -117,14 +117,9 @@ static int mwifiex_pcie_suspend(struct device *dev)
struct pcie_service_card *card;
struct pci_dev *pdev = to_pci_dev(dev);
- if (pdev) {
- card = pci_get_drvdata(pdev);
- if (!card || !card->adapter) {
- pr_err("Card or adapter structure is not valid\n");
- return 0;
- }
- } else {
- pr_err("PCIE device is not specified\n");
+ card = pci_get_drvdata(pdev);
+ if (!card || !card->adapter) {
+ pr_err("Card or adapter structure is not valid\n");
return 0;
}
@@ -162,14 +157,9 @@ static int mwifiex_pcie_resume(struct device *dev)
struct pcie_service_card *card;
struct pci_dev *pdev = to_pci_dev(dev);
- if (pdev) {
- card = pci_get_drvdata(pdev);
- if (!card || !card->adapter) {
- pr_err("Card or adapter structure is not valid\n");
- return 0;
- }
- } else {
- pr_err("PCIE device is not specified\n");
+ card = pci_get_drvdata(pdev);
+ if (!card || !card->adapter) {
+ dev_err(dev, "Card or adapter structure is not valid\n");
return 0;
}
--
1.9.1
^ permalink raw reply related
* [PATCH v3 03/11] mwifiex: resolve races between async FW init (failure) and device removal
From: Amitkumar Karwar @ 2016-11-11 13:10 UTC (permalink / raw)
To: linux-wireless
Cc: Cathy Luo, Nishant Sarmukadam, rajatja, briannorris,
dmitry.torokhov, Brian Norris, Amitkumar Karwar
In-Reply-To: <1478869818-4340-1-git-send-email-akarwar@marvell.com>
From: Brian Norris <briannorris@chromium.org>
It's possible for the FW init sequence to fail, which will trigger a
device cleanup sequence in mwifiex_fw_dpc(). This sequence can race with
device suspend() or remove() (e.g., reboot or unbind), and can trigger
use-after-free issues. Currently, this driver attempts (poorly) to
synchronize remove() using a semaphore, but it doesn't protect some of
the critical sections properly. Particularly, we grab a pointer to the
adapter struct (card->adapter) without checking if it's being freed or
not. We later do a NULL check on the adapter, but that doesn't work if
the adapter was freed.
Also note that the PCIe interface driver doesn't ever set card->adapter
to NULL, so even if we get the synchronization right, we still might try
to redo the cleanup in ->remove(), even if the FW init failure sequence
already did it.
This patch replaces the static semaphore with a per-device completion
struct, and uses that completion to synchronize the remove() thread with
the mwifiex_fw_dpc(). A future patch will utilize this completion to
synchronize the suspend() thread as well.
Signed-off-by: Brian Norris <briannorris@chromium.org>
Tested-by: Xinming Hu <huxm@marvell.com>
Signed-off-by: Amitkumar Karwar <akarwar@marvell.com>
---
v2: Same as v1
v3: Included Brian's suggested change which fixes new use-after-free
introduced by this patch.
---
drivers/net/wireless/marvell/mwifiex/main.c | 47 +++++++++++------------------
drivers/net/wireless/marvell/mwifiex/main.h | 13 +++++---
drivers/net/wireless/marvell/mwifiex/pcie.c | 18 +++++------
drivers/net/wireless/marvell/mwifiex/pcie.h | 2 ++
drivers/net/wireless/marvell/mwifiex/sdio.c | 18 +++++------
drivers/net/wireless/marvell/mwifiex/sdio.h | 2 ++
drivers/net/wireless/marvell/mwifiex/usb.c | 23 ++++++--------
drivers/net/wireless/marvell/mwifiex/usb.h | 2 ++
8 files changed, 56 insertions(+), 69 deletions(-)
diff --git a/drivers/net/wireless/marvell/mwifiex/main.c b/drivers/net/wireless/marvell/mwifiex/main.c
index c710d5e..1c81ca0 100644
--- a/drivers/net/wireless/marvell/mwifiex/main.c
+++ b/drivers/net/wireless/marvell/mwifiex/main.c
@@ -521,9 +521,9 @@ static void mwifiex_fw_dpc(const struct firmware *firmware, void *context)
struct mwifiex_private *priv;
struct mwifiex_adapter *adapter = context;
struct mwifiex_fw_image fw;
- struct semaphore *sem = adapter->card_sem;
bool init_failed = false;
struct wireless_dev *wdev;
+ struct completion *fw_done = adapter->fw_done;
if (!firmware) {
mwifiex_dbg(adapter, ERROR,
@@ -670,7 +670,8 @@ static void mwifiex_fw_dpc(const struct firmware *firmware, void *context)
}
if (init_failed)
mwifiex_free_adapter(adapter);
- up(sem);
+ /* Tell all current and future waiters we're finished */
+ complete_all(fw_done);
return;
}
@@ -1365,7 +1366,7 @@ static void mwifiex_main_work_queue(struct work_struct *work)
* code is extracted from mwifiex_remove_card()
*/
static int
-mwifiex_shutdown_sw(struct mwifiex_adapter *adapter, struct semaphore *sem)
+mwifiex_shutdown_sw(struct mwifiex_adapter *adapter)
{
struct mwifiex_private *priv;
int i;
@@ -1373,8 +1374,9 @@ static void mwifiex_main_work_queue(struct work_struct *work)
if (!adapter)
goto exit_return;
- if (down_interruptible(sem))
- goto exit_sem_err;
+ wait_for_completion(adapter->fw_done);
+ /* Caller should ensure we aren't suspending while this happens */
+ reinit_completion(adapter->fw_done);
priv = mwifiex_get_priv(adapter, MWIFIEX_BSS_ROLE_ANY);
mwifiex_deauthenticate(priv, NULL);
@@ -1431,8 +1433,6 @@ static void mwifiex_main_work_queue(struct work_struct *work)
rtnl_unlock();
}
- up(sem);
-exit_sem_err:
mwifiex_dbg(adapter, INFO, "%s, successful\n", __func__);
exit_return:
return 0;
@@ -1442,21 +1442,18 @@ static void mwifiex_main_work_queue(struct work_struct *work)
* code is extracted from mwifiex_add_card()
*/
static int
-mwifiex_reinit_sw(struct mwifiex_adapter *adapter, struct semaphore *sem,
+mwifiex_reinit_sw(struct mwifiex_adapter *adapter, struct completion *fw_done,
struct mwifiex_if_ops *if_ops, u8 iface_type)
{
char fw_name[32];
struct pcie_service_card *card = adapter->card;
- if (down_interruptible(sem))
- goto exit_sem_err;
-
mwifiex_init_lock_list(adapter);
if (adapter->if_ops.up_dev)
adapter->if_ops.up_dev(adapter);
adapter->iface_type = iface_type;
- adapter->card_sem = sem;
+ adapter->fw_done = fw_done;
adapter->hw_status = MWIFIEX_HW_STATUS_INITIALIZING;
adapter->surprise_removed = false;
@@ -1507,7 +1504,8 @@ static void mwifiex_main_work_queue(struct work_struct *work)
}
strcpy(adapter->fw_name, fw_name);
mwifiex_dbg(adapter, INFO, "%s, successful\n", __func__);
- up(sem);
+
+ complete_all(adapter->fw_done);
return 0;
err_init_fw:
@@ -1527,8 +1525,7 @@ static void mwifiex_main_work_queue(struct work_struct *work)
err_kmalloc:
mwifiex_terminate_workqueue(adapter);
adapter->surprise_removed = true;
- up(sem);
-exit_sem_err:
+ complete_all(adapter->fw_done);
mwifiex_dbg(adapter, INFO, "%s, error\n", __func__);
return -1;
@@ -1543,12 +1540,12 @@ void mwifiex_do_flr(struct mwifiex_adapter *adapter, bool prepare)
struct mwifiex_if_ops if_ops;
if (!prepare) {
- mwifiex_reinit_sw(adapter, adapter->card_sem, &if_ops,
+ mwifiex_reinit_sw(adapter, adapter->fw_done, &if_ops,
adapter->iface_type);
} else {
memcpy(&if_ops, &adapter->if_ops,
sizeof(struct mwifiex_if_ops));
- mwifiex_shutdown_sw(adapter, adapter->card_sem);
+ mwifiex_shutdown_sw(adapter);
}
}
EXPORT_SYMBOL_GPL(mwifiex_do_flr);
@@ -1618,15 +1615,12 @@ static void mwifiex_probe_of(struct mwifiex_adapter *adapter)
* - Add logical interfaces
*/
int
-mwifiex_add_card(void *card, struct semaphore *sem,
+mwifiex_add_card(void *card, struct completion *fw_done,
struct mwifiex_if_ops *if_ops, u8 iface_type,
struct device *dev, bool of_node_valid)
{
struct mwifiex_adapter *adapter;
- if (down_interruptible(sem))
- goto exit_sem_err;
-
if (mwifiex_register(card, if_ops, (void **)&adapter)) {
pr_err("%s: software init failed\n", __func__);
goto err_init_sw;
@@ -1637,7 +1631,7 @@ static void mwifiex_probe_of(struct mwifiex_adapter *adapter)
mwifiex_probe_of(adapter);
adapter->iface_type = iface_type;
- adapter->card_sem = sem;
+ adapter->fw_done = fw_done;
adapter->hw_status = MWIFIEX_HW_STATUS_INITIALIZING;
adapter->surprise_removed = false;
@@ -1706,9 +1700,7 @@ static void mwifiex_probe_of(struct mwifiex_adapter *adapter)
mwifiex_free_adapter(adapter);
err_init_sw:
- up(sem);
-exit_sem_err:
return -1;
}
EXPORT_SYMBOL_GPL(mwifiex_add_card);
@@ -1724,14 +1716,11 @@ static void mwifiex_probe_of(struct mwifiex_adapter *adapter)
* - Unregister the device
* - Free the adapter structure
*/
-int mwifiex_remove_card(struct mwifiex_adapter *adapter, struct semaphore *sem)
+int mwifiex_remove_card(struct mwifiex_adapter *adapter)
{
struct mwifiex_private *priv = NULL;
int i;
- if (down_trylock(sem))
- goto exit_sem_err;
-
if (!adapter)
goto exit_remove;
@@ -1801,8 +1790,6 @@ int mwifiex_remove_card(struct mwifiex_adapter *adapter, struct semaphore *sem)
mwifiex_free_adapter(adapter);
exit_remove:
- up(sem);
-exit_sem_err:
return 0;
}
EXPORT_SYMBOL_GPL(mwifiex_remove_card);
diff --git a/drivers/net/wireless/marvell/mwifiex/main.h b/drivers/net/wireless/marvell/mwifiex/main.h
index b0c501d..d0fbb2f 100644
--- a/drivers/net/wireless/marvell/mwifiex/main.h
+++ b/drivers/net/wireless/marvell/mwifiex/main.h
@@ -20,6 +20,7 @@
#ifndef _MWIFIEX_MAIN_H_
#define _MWIFIEX_MAIN_H_
+#include <linux/completion.h>
#include <linux/kernel.h>
#include <linux/module.h>
#include <linux/sched.h>
@@ -985,7 +986,10 @@ struct mwifiex_adapter {
u32 usr_dot_11ac_mcs_support;
atomic_t pending_bridged_pkts;
- struct semaphore *card_sem;
+
+ /* For synchronizing FW initialization with device lifecycle. */
+ struct completion *fw_done;
+
bool ext_scan;
u8 fw_api_ver;
u8 key_api_major_ver, key_api_minor_ver;
@@ -1438,9 +1442,10 @@ static inline void mwifiex_enable_wake(struct mwifiex_adapter *adapter)
int mwifiex_init_shutdown_fw(struct mwifiex_private *priv,
u32 func_init_shutdown);
-int mwifiex_add_card(void *, struct semaphore *, struct mwifiex_if_ops *, u8,
- struct device *, bool);
-int mwifiex_remove_card(struct mwifiex_adapter *, struct semaphore *);
+int mwifiex_add_card(void *card, struct completion *fw_done,
+ struct mwifiex_if_ops *if_ops, u8 iface_type,
+ struct device *dev, bool of_node_valid);
+int mwifiex_remove_card(struct mwifiex_adapter *adapter);
void mwifiex_get_version(struct mwifiex_adapter *adapter, char *version,
int maxlen);
diff --git a/drivers/net/wireless/marvell/mwifiex/pcie.c b/drivers/net/wireless/marvell/mwifiex/pcie.c
index 509c156..c4bd64b 100644
--- a/drivers/net/wireless/marvell/mwifiex/pcie.c
+++ b/drivers/net/wireless/marvell/mwifiex/pcie.c
@@ -35,8 +35,6 @@
static struct mwifiex_if_ops pcie_ops;
-static struct semaphore add_remove_card_sem;
-
static const struct of_device_id mwifiex_pcie_of_match_table[] = {
{ .compatible = "pci11ab,2b42" },
{ .compatible = "pci1b4b,2b42" },
@@ -213,6 +211,8 @@ static int mwifiex_pcie_probe(struct pci_dev *pdev,
if (!card)
return -ENOMEM;
+ init_completion(&card->fw_done);
+
card->dev = pdev;
if (ent->driver_data) {
@@ -234,7 +234,7 @@ static int mwifiex_pcie_probe(struct pci_dev *pdev,
valid_of_node = true;
}
- ret = mwifiex_add_card(card, &add_remove_card_sem, &pcie_ops,
+ ret = mwifiex_add_card(card, &card->fw_done, &pcie_ops,
MWIFIEX_PCIE, &pdev->dev, valid_of_node);
if (ret) {
pr_err("%s failed\n", __func__);
@@ -260,6 +260,8 @@ static void mwifiex_pcie_remove(struct pci_dev *pdev)
if (!card)
return;
+ wait_for_completion(&card->fw_done);
+
adapter = card->adapter;
if (!adapter || !adapter->priv_num)
return;
@@ -279,7 +281,7 @@ static void mwifiex_pcie_remove(struct pci_dev *pdev)
mwifiex_init_shutdown_fw(priv, MWIFIEX_FUNC_SHUTDOWN);
}
- mwifiex_remove_card(card->adapter, &add_remove_card_sem);
+ mwifiex_remove_card(adapter);
}
static void mwifiex_pcie_shutdown(struct pci_dev *pdev)
@@ -3181,8 +3183,7 @@ static void mwifiex_pcie_down_dev(struct mwifiex_adapter *adapter)
/*
* This function initializes the PCIE driver module.
*
- * This initiates the semaphore and registers the device with
- * PCIE bus.
+ * This registers the device with PCIE bus.
*/
static int mwifiex_pcie_init_module(void)
{
@@ -3190,8 +3191,6 @@ static int mwifiex_pcie_init_module(void)
pr_debug("Marvell PCIe Driver\n");
- sema_init(&add_remove_card_sem, 1);
-
/* Clear the flag in case user removes the card. */
user_rmmod = 0;
@@ -3215,9 +3214,6 @@ static int mwifiex_pcie_init_module(void)
*/
static void mwifiex_pcie_cleanup_module(void)
{
- if (!down_interruptible(&add_remove_card_sem))
- up(&add_remove_card_sem);
-
/* Set the flag as user is removing this module. */
user_rmmod = 1;
diff --git a/drivers/net/wireless/marvell/mwifiex/pcie.h b/drivers/net/wireless/marvell/mwifiex/pcie.h
index 46f99ca..ae3365d 100644
--- a/drivers/net/wireless/marvell/mwifiex/pcie.h
+++ b/drivers/net/wireless/marvell/mwifiex/pcie.h
@@ -22,6 +22,7 @@
#ifndef _MWIFIEX_PCIE_H
#define _MWIFIEX_PCIE_H
+#include <linux/completion.h>
#include <linux/pci.h>
#include <linux/interrupt.h>
@@ -345,6 +346,7 @@ struct pcie_service_card {
struct pci_dev *dev;
struct mwifiex_adapter *adapter;
struct mwifiex_pcie_device pcie;
+ struct completion fw_done;
u8 txbd_flush;
u32 txbd_wrptr;
diff --git a/drivers/net/wireless/marvell/mwifiex/sdio.c b/drivers/net/wireless/marvell/mwifiex/sdio.c
index ca5b0aa..90f45d9 100644
--- a/drivers/net/wireless/marvell/mwifiex/sdio.c
+++ b/drivers/net/wireless/marvell/mwifiex/sdio.c
@@ -49,8 +49,6 @@
static struct mwifiex_if_ops sdio_ops;
static unsigned long iface_work_flags;
-static struct semaphore add_remove_card_sem;
-
static struct memory_type_mapping generic_mem_type_map[] = {
{"DUMP", NULL, 0, 0xDD},
};
@@ -116,6 +114,8 @@ static int mwifiex_sdio_probe_of(struct device *dev)
if (!card)
return -ENOMEM;
+ init_completion(&card->fw_done);
+
card->func = func;
card->device_id = id;
@@ -156,7 +156,7 @@ static int mwifiex_sdio_probe_of(struct device *dev)
valid_of_node = true;
}
- ret = mwifiex_add_card(card, &add_remove_card_sem, &sdio_ops,
+ ret = mwifiex_add_card(card, &card->fw_done, &sdio_ops,
MWIFIEX_SDIO, &func->dev, valid_of_node);
if (ret) {
dev_err(&func->dev, "add card failed\n");
@@ -237,6 +237,8 @@ static int mwifiex_sdio_resume(struct device *dev)
if (!card)
return;
+ wait_for_completion(&card->fw_done);
+
adapter = card->adapter;
if (!adapter || !adapter->priv_num)
return;
@@ -254,7 +256,7 @@ static int mwifiex_sdio_resume(struct device *dev)
mwifiex_init_shutdown_fw(priv, MWIFIEX_FUNC_SHUTDOWN);
}
- mwifiex_remove_card(card->adapter, &add_remove_card_sem);
+ mwifiex_remove_card(adapter);
}
/*
@@ -2716,14 +2718,11 @@ static void mwifiex_sdio_device_dump(struct mwifiex_adapter *adapter)
/*
* This function initializes the SDIO driver.
*
- * This initiates the semaphore and registers the device with
- * SDIO bus.
+ * This registers the device with SDIO bus.
*/
static int
mwifiex_sdio_init_module(void)
{
- sema_init(&add_remove_card_sem, 1);
-
/* Clear the flag in case user removes the card. */
user_rmmod = 0;
@@ -2742,9 +2741,6 @@ static void mwifiex_sdio_device_dump(struct mwifiex_adapter *adapter)
static void
mwifiex_sdio_cleanup_module(void)
{
- if (!down_interruptible(&add_remove_card_sem))
- up(&add_remove_card_sem);
-
/* Set the flag as user is removing this module. */
user_rmmod = 1;
cancel_work_sync(&sdio_work);
diff --git a/drivers/net/wireless/marvell/mwifiex/sdio.h b/drivers/net/wireless/marvell/mwifiex/sdio.h
index b9fbc5c..cdbf3a3a 100644
--- a/drivers/net/wireless/marvell/mwifiex/sdio.h
+++ b/drivers/net/wireless/marvell/mwifiex/sdio.h
@@ -21,6 +21,7 @@
#define _MWIFIEX_SDIO_H
+#include <linux/completion.h>
#include <linux/mmc/sdio.h>
#include <linux/mmc/sdio_ids.h>
#include <linux/mmc/sdio_func.h>
@@ -238,6 +239,7 @@ struct sdio_mmc_card {
struct sdio_func *func;
struct mwifiex_adapter *adapter;
+ struct completion fw_done;
const char *firmware;
const struct mwifiex_sdio_card_reg *reg;
u8 max_ports;
diff --git a/drivers/net/wireless/marvell/mwifiex/usb.c b/drivers/net/wireless/marvell/mwifiex/usb.c
index 889203d..14f89fe 100644
--- a/drivers/net/wireless/marvell/mwifiex/usb.c
+++ b/drivers/net/wireless/marvell/mwifiex/usb.c
@@ -24,7 +24,6 @@
static u8 user_rmmod;
static struct mwifiex_if_ops usb_ops;
-static struct semaphore add_remove_card_sem;
static struct usb_device_id mwifiex_usb_table[] = {
/* 8766 */
@@ -386,6 +385,8 @@ static int mwifiex_usb_probe(struct usb_interface *intf,
if (!card)
return -ENOMEM;
+ init_completion(&card->fw_done);
+
id_vendor = le16_to_cpu(udev->descriptor.idVendor);
id_product = le16_to_cpu(udev->descriptor.idProduct);
bcd_device = le16_to_cpu(udev->descriptor.bcdDevice);
@@ -475,7 +476,7 @@ static int mwifiex_usb_probe(struct usb_interface *intf,
usb_set_intfdata(intf, card);
- ret = mwifiex_add_card(card, &add_remove_card_sem, &usb_ops,
+ ret = mwifiex_add_card(card, &card->fw_done, &usb_ops,
MWIFIEX_USB, &card->udev->dev, false);
if (ret) {
pr_err("%s: mwifiex_add_card failed: %d\n", __func__, ret);
@@ -601,13 +602,15 @@ static void mwifiex_usb_disconnect(struct usb_interface *intf)
struct usb_card_rec *card = usb_get_intfdata(intf);
struct mwifiex_adapter *adapter;
- if (!card || !card->adapter) {
- pr_err("%s: card or card->adapter is NULL\n", __func__);
+ if (!card) {
+ dev_err(&intf->dev, "%s: card is NULL\n", __func__);
return;
}
+ wait_for_completion(&card->fw_done);
+
adapter = card->adapter;
- if (!adapter->priv_num)
+ if (!adapter || !adapter->priv_num)
return;
if (user_rmmod && !adapter->mfg_mode) {
@@ -627,7 +630,7 @@ static void mwifiex_usb_disconnect(struct usb_interface *intf)
mwifiex_dbg(adapter, FATAL,
"%s: removing card\n", __func__);
- mwifiex_remove_card(adapter, &add_remove_card_sem);
+ mwifiex_remove_card(adapter);
usb_put_dev(interface_to_usbdev(intf));
}
@@ -1200,8 +1203,7 @@ static void mwifiex_usb_submit_rem_rx_urbs(struct mwifiex_adapter *adapter)
/* This function initializes the USB driver module.
*
- * This initiates the semaphore and registers the device with
- * USB bus.
+ * This registers the device with USB bus.
*/
static int mwifiex_usb_init_module(void)
{
@@ -1209,8 +1211,6 @@ static int mwifiex_usb_init_module(void)
pr_debug("Marvell USB8797 Driver\n");
- sema_init(&add_remove_card_sem, 1);
-
ret = usb_register(&mwifiex_usb_driver);
if (ret)
pr_err("Driver register failed!\n");
@@ -1230,9 +1230,6 @@ static int mwifiex_usb_init_module(void)
*/
static void mwifiex_usb_cleanup_module(void)
{
- if (!down_interruptible(&add_remove_card_sem))
- up(&add_remove_card_sem);
-
/* set the flag as user is removing this module */
user_rmmod = 1;
diff --git a/drivers/net/wireless/marvell/mwifiex/usb.h b/drivers/net/wireless/marvell/mwifiex/usb.h
index 30e8eb8..e5f204e 100644
--- a/drivers/net/wireless/marvell/mwifiex/usb.h
+++ b/drivers/net/wireless/marvell/mwifiex/usb.h
@@ -20,6 +20,7 @@
#ifndef _MWIFIEX_USB_H
#define _MWIFIEX_USB_H
+#include <linux/completion.h>
#include <linux/usb.h>
#define USB8XXX_VID 0x1286
@@ -75,6 +76,7 @@ struct usb_card_rec {
struct mwifiex_adapter *adapter;
struct usb_device *udev;
struct usb_interface *intf;
+ struct completion fw_done;
u8 rx_cmd_ep;
struct urb_context rx_cmd;
atomic_t rx_cmd_urb_pending;
--
1.9.1
^ permalink raw reply related
* [PATCH v3 02/11] mwifiex: complete blocked power save handshake in main process
From: Amitkumar Karwar @ 2016-11-11 13:10 UTC (permalink / raw)
To: linux-wireless
Cc: Cathy Luo, Nishant Sarmukadam, rajatja, briannorris,
dmitry.torokhov, Shengzhen Li, Amitkumar Karwar
In-Reply-To: <1478869818-4340-1-git-send-email-akarwar@marvell.com>
From: Shengzhen Li <szli@marvell.com>
Power save handshake with firmware might be blocked by on-going
data transfer.
this patch check the PS status in main process and complete
previous blocked PS handshake.
this patch also remove redudant check before call
mwifiex_check_ps_cond function.
Signed-off-by: Cathy Luo <cluo@marvell.com>
Signed-off-by: Shengzhen Li <szli@marvell.com>
Tested-by: Xinming Hu <huxm@marvell.com>
Signed-off-by: Amitkumar Karwar <akarwar@marvell.com>
---
v2: 1. remove redudant check(Brain)
2. reorgnized to follow tx_hw_pending patch
v3: Same as v2
---
drivers/net/wireless/marvell/mwifiex/main.c | 9 +++++----
1 file changed, 5 insertions(+), 4 deletions(-)
diff --git a/drivers/net/wireless/marvell/mwifiex/main.c b/drivers/net/wireless/marvell/mwifiex/main.c
index 20c9b77..c710d5e 100644
--- a/drivers/net/wireless/marvell/mwifiex/main.c
+++ b/drivers/net/wireless/marvell/mwifiex/main.c
@@ -308,6 +308,9 @@ int mwifiex_main_process(struct mwifiex_adapter *adapter)
/* We have tried to wakeup the card already */
if (adapter->pm_wakeup_fw_try)
break;
+ if (adapter->ps_state == PS_STATE_PRE_SLEEP)
+ mwifiex_check_ps_cond(adapter);
+
if (adapter->ps_state != PS_STATE_AWAKE)
break;
if (adapter->tx_lock_flag) {
@@ -355,10 +358,8 @@ int mwifiex_main_process(struct mwifiex_adapter *adapter)
/* Check if we need to confirm Sleep Request
received previously */
- if (adapter->ps_state == PS_STATE_PRE_SLEEP) {
- if (!adapter->cmd_sent && !adapter->curr_cmd)
- mwifiex_check_ps_cond(adapter);
- }
+ if (adapter->ps_state == PS_STATE_PRE_SLEEP)
+ mwifiex_check_ps_cond(adapter);
/* * The ps_state may have been changed during processing of
* Sleep Request event.
--
1.9.1
^ permalink raw reply related
* [PATCH v3 01/11] mwifiex: check tx_hw_pending before downloading sleep confirm
From: Amitkumar Karwar @ 2016-11-11 13:10 UTC (permalink / raw)
To: linux-wireless
Cc: Cathy Luo, Nishant Sarmukadam, rajatja, briannorris,
dmitry.torokhov, Shengzhen Li, Amitkumar Karwar
From: Shengzhen Li <szli@marvell.com>
We may get SLEEP event from firmware even if TXDone interrupt
for last Tx packet is still pending. In this case, we may
end up accessing PCIe memory for handling TXDone after power
save handshake is completed. This causes kernel crash with
external abort.
This patch will only allow downloading sleep confirm
when no tx done interrupt is pending in the hardware.
Signed-off-by: Cathy Luo <cluo@marvell.com>
Signed-off-by: Shengzhen Li <szli@marvell.com>
Tested-by: Xinming Hu <huxm@marvell.com>
Signed-off-by: Amitkumar Karwar <akarwar@marvell.com>
---
v2: address format issues(Brain)
RESEND v2(Applicable for complete patch series):
1) Fixed syntax issue "changelog not placed after the Sign-offs"
pointed by Brian.
2) Dropped "[v2,03/12] mwifiex: don't do unbalanced free()'ing in
cleanup_if()" patch in this series. It was already sent by Brian
separately.
v3: Same as RESEND v2.
Hi Kalle,
There are multiple mwifiex patches under review. I want you consider them
in following sequence(first being oldest) to avoid conflicts
[v3] mwifiex: report wakeup for wowlan
mwifiex: add power save parameters in hs_cfg cmd
[2/2] mwifiex: ignore calibration data failure (Note: 1/2 has dropped)
[v6] mwifiex: parse device tree node for PCIe
[v2,1/3] mwifiex: Allow mwifiex early access to device structure
[v2,2/3] mwifiex: Introduce mwifiex_probe_of() to parse common properties
[v2,3/3] mwifiex: Enable WoWLAN for both sdio and pcie
mwifiex: don't do unbalanced free()'ing in cleanup_if()
mwifiex: printk() overflow with 32-byte SSIDs
mwifiex: fix memory leak in mwifiex_save_hidden_ssid_channels()
[v3,01/11] mwifiex: check tx_hw_pending before downloading sleep confirm
[v3,02/11] mwifiex: complete blocked power save handshake in main process
[v3,03/11] mwifiex: resolve races between async FW init (failure) and device removal
[v3,04/11] mwifiex: remove redundant pdev check in suspend/resume handlers
[v3,05/11] mwifiex: don't pretend to resume while remove()'ing
[v3,06/11] mwifiex: resolve suspend() race with async FW init failure
[v3,07/11] mwifiex: reset card->adapter during device unregister
[v3,08/11] mwifiex: usb: handle HS failures
[v3,09/11] mwifiex: sdio: don't check for NULL sdio_func
[v3,10/11] mwifiex: stop checking for NULL drvata/intfdata
[v3,11/11] mwifiex: pcie: stop checking for NULL adapter->card
---
drivers/net/wireless/marvell/mwifiex/cmdevt.c | 5 +++--
drivers/net/wireless/marvell/mwifiex/init.c | 1 +
drivers/net/wireless/marvell/mwifiex/main.h | 1 +
drivers/net/wireless/marvell/mwifiex/pcie.c | 5 +++++
4 files changed, 10 insertions(+), 2 deletions(-)
diff --git a/drivers/net/wireless/marvell/mwifiex/cmdevt.c b/drivers/net/wireless/marvell/mwifiex/cmdevt.c
index 5347728..25a7475 100644
--- a/drivers/net/wireless/marvell/mwifiex/cmdevt.c
+++ b/drivers/net/wireless/marvell/mwifiex/cmdevt.c
@@ -1118,13 +1118,14 @@ int mwifiex_process_cmdresp(struct mwifiex_adapter *adapter)
void
mwifiex_check_ps_cond(struct mwifiex_adapter *adapter)
{
- if (!adapter->cmd_sent &&
+ if (!adapter->cmd_sent && !atomic_read(&adapter->tx_hw_pending) &&
!adapter->curr_cmd && !IS_CARD_RX_RCVD(adapter))
mwifiex_dnld_sleep_confirm_cmd(adapter);
else
mwifiex_dbg(adapter, CMD,
- "cmd: Delay Sleep Confirm (%s%s%s)\n",
+ "cmd: Delay Sleep Confirm (%s%s%s%s)\n",
(adapter->cmd_sent) ? "D" : "",
+ atomic_read(&adapter->tx_hw_pending) ? "T" : "",
(adapter->curr_cmd) ? "C" : "",
(IS_CARD_RX_RCVD(adapter)) ? "R" : "");
}
diff --git a/drivers/net/wireless/marvell/mwifiex/init.c b/drivers/net/wireless/marvell/mwifiex/init.c
index 82839d9..b36cb3f 100644
--- a/drivers/net/wireless/marvell/mwifiex/init.c
+++ b/drivers/net/wireless/marvell/mwifiex/init.c
@@ -270,6 +270,7 @@ static void mwifiex_init_adapter(struct mwifiex_adapter *adapter)
adapter->adhoc_11n_enabled = false;
mwifiex_wmm_init(adapter);
+ atomic_set(&adapter->tx_hw_pending, 0);
sleep_cfm_buf = (struct mwifiex_opt_sleep_confirm *)
adapter->sleep_cfm->data;
diff --git a/drivers/net/wireless/marvell/mwifiex/main.h b/drivers/net/wireless/marvell/mwifiex/main.h
index 11abc49..b0c501d 100644
--- a/drivers/net/wireless/marvell/mwifiex/main.h
+++ b/drivers/net/wireless/marvell/mwifiex/main.h
@@ -857,6 +857,7 @@ struct mwifiex_adapter {
atomic_t rx_pending;
atomic_t tx_pending;
atomic_t cmd_pending;
+ atomic_t tx_hw_pending;
struct workqueue_struct *workqueue;
struct work_struct main_work;
struct workqueue_struct *rx_workqueue;
diff --git a/drivers/net/wireless/marvell/mwifiex/pcie.c b/drivers/net/wireless/marvell/mwifiex/pcie.c
index ba1fa17..509c156 100644
--- a/drivers/net/wireless/marvell/mwifiex/pcie.c
+++ b/drivers/net/wireless/marvell/mwifiex/pcie.c
@@ -522,6 +522,7 @@ static int mwifiex_pcie_disable_host_int(struct mwifiex_adapter *adapter)
}
}
+ atomic_set(&adapter->tx_hw_pending, 0);
return 0;
}
@@ -721,6 +722,7 @@ static void mwifiex_cleanup_txq_ring(struct mwifiex_adapter *adapter)
card->tx_buf_list[i] = NULL;
}
+ atomic_set(&adapter->tx_hw_pending, 0);
return;
}
@@ -1158,6 +1160,7 @@ static int mwifiex_pcie_send_data_complete(struct mwifiex_adapter *adapter)
-1);
else
mwifiex_write_data_complete(adapter, skb, 0, 0);
+ atomic_dec(&adapter->tx_hw_pending);
}
card->tx_buf_list[wrdoneidx] = NULL;
@@ -1250,6 +1253,7 @@ static int mwifiex_pcie_send_data_complete(struct mwifiex_adapter *adapter)
wrindx = (card->txbd_wrptr & reg->tx_mask) >> reg->tx_start_ptr;
buf_pa = MWIFIEX_SKB_DMA_ADDR(skb);
card->tx_buf_list[wrindx] = skb;
+ atomic_inc(&adapter->tx_hw_pending);
if (reg->pfu_enabled) {
desc2 = card->txbd_ring[wrindx];
@@ -1327,6 +1331,7 @@ static int mwifiex_pcie_send_data_complete(struct mwifiex_adapter *adapter)
done_unmap:
mwifiex_unmap_pci_memory(adapter, skb, PCI_DMA_TODEVICE);
card->tx_buf_list[wrindx] = NULL;
+ atomic_dec(&adapter->tx_hw_pending);
if (reg->pfu_enabled)
memset(desc2, 0, sizeof(*desc2));
else
--
1.9.1
^ permalink raw reply related
* Re: Problems getting mwifiex with sd8887 to work
From: Wolfram Sang @ 2016-11-11 13:10 UTC (permalink / raw)
To: Amitkumar Karwar
Cc: linux-wireless@vger.kernel.org, Nishant Sarmukadam, Kalle Valo
In-Reply-To: <507fff29bd2048c2b2295beb451943b5@SC-EXCH04.marvell.com>
[-- Attachment #1: Type: text/plain, Size: 398 bytes --]
> 0x242 command failure is expected for sd8887, as It's not supported.
> Driver shouldn't abort initialization in this case. You can try below
> change. I will prepare a patch to send this command based on firmware
> version/capability.
Thanks for the heads up. If you CC me on the patch, I'll happily ack it.
Meanwhile, I nailed my problem down to some SDIO IRQ delivery issue.
Thanks again.
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply
* RE: [PATCH RESEND v2 01/11] mwifiex: check tx_hw_pending before downloading sleep confirm
From: Amitkumar Karwar @ 2016-11-11 13:09 UTC (permalink / raw)
To: Kalle Valo, Xinming Hu
Cc: Linux Wireless, Brian Norris, Dmitry Torokhov, Cathy Luo,
Shengzhen Li, Xinming Hu
In-Reply-To: <87y40rpgz0.fsf@purkki.adurom.net>
Hi Kalle,
> From: linux-wireless-owner@vger.kernel.org [mailto:linux-wireless-
> owner@vger.kernel.org] On Behalf Of Kalle Valo
> Sent: Thursday, November 10, 2016 7:36 PM
> To: Xinming Hu
> Cc: Linux Wireless; Brian Norris; Dmitry Torokhov; Amitkumar Karwar;
> Cathy Luo; Shengzhen Li; Xinming Hu
> Subject: Re: [PATCH RESEND v2 01/11] mwifiex: check tx_hw_pending
> before downloading sleep confirm
>
> Xinming Hu <huxinming820@gmail.com> writes:
>
> > From: Shengzhen Li <szli@marvell.com>
> >
> > We may get SLEEP event from firmware even if TXDone interrupt for
> last
> > Tx packet is still pending. In this case, we may end up accessing
> PCIe
> > memory for handling TXDone after power save handshake is completed.
> > This causes kernel crash with external abort.
> >
> > This patch will only allow downloading sleep confirm when no tx done
> > interrupt is pending in the hardware.
> >
> > Signed-off-by: Cathy Luo <cluo@marvell.com>
> > Signed-off-by: Shengzhen Li <szli@marvell.com>
> > Signed-off-by: Xinming Hu <huxm@marvell.com>
> > Signed-off-by: Amitkumar Karwar <akarwar@marvell.com>
> > ---
> > v2: address format issues(Brain)
> > RESEND v2(Applicable for complete patch series):
> > 1) Fixed syntax issue "changelog not placed after the Sign-offs"
> > pointed by Brian.
> > 2) Dropped "[v2,03/12] mwifiex: don't do unbalanced free()'ing in
> > cleanup_if()" patch in this series. It was already sent by
> Brian
> > separately.
>
> I do not know where this "v2 RESEND" practise is coming from but I very
> much prefer that people would not use it. If you need to resend
> something keep things simple, just increase the version number and
> document in the changelog that nothing changed. I don't care what the
> patchset version number is, I always look at the latest version and
> discard the older ones.
>
> This is just for the future, no need resend anything because of this.
Got it. We will avoid "RESEND" and increase version number for our future submissions.
There is an improvement pointed out by Brian for "RESEND v2 03/11" patch. I will include it in v3.
Regards,
Amitkumar
^ permalink raw reply
* RE: [PATCH RESEND v2 03/11] mwifiex: resolve races between async FW init (failure) and device removal
From: Amitkumar Karwar @ 2016-11-11 13:06 UTC (permalink / raw)
To: Brian Norris, Xinming Hu
Cc: Linux Wireless, Kalle Valo, Dmitry Torokhov, Cathy Luo
In-Reply-To: <20161110183723.GA134624@google.com>
> From: Brian Norris [mailto:briannorris@chromium.org]
> Sent: Friday, November 11, 2016 12:07 AM
> To: Xinming Hu
> Cc: Linux Wireless; Kalle Valo; Dmitry Torokhov; Amitkumar Karwar;
> Cathy Luo
> Subject: Re: [PATCH RESEND v2 03/11] mwifiex: resolve races between
> async FW init (failure) and device removal
>
> Hi,
>
> On Thu, Nov 10, 2016 at 07:57:04PM +0800, Xinming Hu wrote:
> > From: Brian Norris <briannorris@chromium.org>
> >
> > It's possible for the FW init sequence to fail, which will trigger a
> > device cleanup sequence in mwifiex_fw_dpc(). This sequence can race
> > with device suspend() or remove() (e.g., reboot or unbind), and can
> > trigger use-after-free issues. Currently, this driver attempts
> > (poorly) to synchronize remove() using a semaphore, but it doesn't
> > protect some of the critical sections properly. Particularly, we grab
> > a pointer to the adapter struct (card->adapter) without checking if
> > it's being freed or not. We later do a NULL check on the adapter, but
> > that doesn't work if the adapter was freed.
> >
> > Also note that the PCIe interface driver doesn't ever set
> > card->adapter to NULL, so even if we get the synchronization right,
> we
> > still might try to redo the cleanup in ->remove(), even if the FW
> init
> > failure sequence already did it.
> >
> > This patch replaces the static semaphore with a per-device completion
> > struct, and uses that completion to synchronize the remove() thread
> > with the mwifiex_fw_dpc(). A future patch will utilize this
> completion
> > to synchronize the suspend() thread as well.
> >
> > Signed-off-by: Brian Norris <briannorris@chromium.org>
> > ---
> > v2: Same as v1
> > ---
> > drivers/net/wireless/marvell/mwifiex/main.c | 46
> > ++++++++++-------------------
> > drivers/net/wireless/marvell/mwifiex/main.h | 13 +++++---
> > drivers/net/wireless/marvell/mwifiex/pcie.c | 18 +++++------
> > drivers/net/wireless/marvell/mwifiex/pcie.h | 2 ++
> > drivers/net/wireless/marvell/mwifiex/sdio.c | 18 +++++------
> > drivers/net/wireless/marvell/mwifiex/sdio.h | 2 ++
> > drivers/net/wireless/marvell/mwifiex/usb.c | 23 +++++++--------
> > drivers/net/wireless/marvell/mwifiex/usb.h | 2 ++
> > 8 files changed, 55 insertions(+), 69 deletions(-)
> >
> > diff --git a/drivers/net/wireless/marvell/mwifiex/main.c
> > b/drivers/net/wireless/marvell/mwifiex/main.c
> > index c710d5e..09d46d6 100644
> > --- a/drivers/net/wireless/marvell/mwifiex/main.c
> > +++ b/drivers/net/wireless/marvell/mwifiex/main.c
> > @@ -521,7 +521,6 @@ static void mwifiex_fw_dpc(const struct firmware
> *firmware, void *context)
> > struct mwifiex_private *priv;
> > struct mwifiex_adapter *adapter = context;
> > struct mwifiex_fw_image fw;
> > - struct semaphore *sem = adapter->card_sem;
> > bool init_failed = false;
> > struct wireless_dev *wdev;
> >
> > @@ -670,7 +669,8 @@ static void mwifiex_fw_dpc(const struct firmware
> *firmware, void *context)
> > }
> > if (init_failed)
> > mwifiex_free_adapter(adapter);
> > - up(sem);
> > + /* Tell all current and future waiters we're finished */
> > + complete_all(adapter->fw_done);
>
> This part introduces a new use-after-free. We need to dereference
> adapter->fw_done *before* we free the adapter in
> mwifiex_free_adapter().
> So you need a diff that looks something like this:
>
> --- a/drivers/net/wireless/marvell/mwifiex/main.c
> +++ b/drivers/net/wireless/marvell/mwifiex/main.c
> @@ -514,6 +514,7 @@ static void mwifiex_fw_dpc(const struct firmware
> *firmware, void *context)
> struct mwifiex_fw_image fw;
> bool init_failed = false;
> struct wireless_dev *wdev;
> + struct completion *fw_done = adapter->fw_done;
>
> if (!firmware) {
> mwifiex_dbg(adapter, ERROR,
> @@ -654,7 +655,7 @@ done:
> if (init_failed)
> mwifiex_free_adapter(adapter);
> /* Tell all current and future waiters we're finished */
> - complete_all(adapter->fw_done);
> + complete_all(fw_done);
> return;
> }
>
Thanks. I will include this change in v3.
Regards,
Amitkumar
^ permalink raw reply
* Re: [RFC] qtn: add FullMAC firmware for Quantenna QSR10G wifi device
From: Johannes Berg @ 2016-11-11 11:35 UTC (permalink / raw)
To: IgorMitsyanko
Cc: linux-wireless, btherthala, hwang, smaksimenko, dlebed,
Igor Mitsyanko, Kamlesh Rath, Sergey Matyukevich, Avinash Patil,
Ben Hutchings, Kyle McMartin
In-Reply-To: <d1cfaf8a-55a6-3a7c-9e9d-ec810fe5b80d@quantenna.com>
Adding linux-firmware people to Cc, since presumably they don't
necessarily read linux-wireless...
> Johannes, from that perspective, who are the "redistributors"?
> Specifically, is linux-firmware git repository considered a
> redistributor or its just hosting files? I mean, at what moment
> someone else other then Quantenna will start to be legally obliged to
> make GPL code used in firmware available for others?
Look, I don't know. I'd assume people who ship it, like any regular
distro, would be (re)distributors thereof. "Normal" (non-GPL) firmware
images come with a redistribution license, but that obviously can't
work here.
There's some info from Ben here regarding the carl9170 case:
http://lkml.iu.edu/hypermail/linux/kernel/1605.3/01176.html
> Personally I still hope that linux-firmware itself is not legally
> concerned with what is the content of firmware its hosting, but looks
> like there already was a precedent case with carl9170 driver and
> we have to somehow deal with it.
That's really all I wanted to bring up. I'm not involved with the
linux-firmware git tree.
> There still may be a difference though: Quantenna is semiconductor
> company only, software
> used on actual products based on Quantenna chipsets is released by
> other
> companies.
> I just want to present our legal team with a clear case (and position
> of
> Linux maintainers) so that they can
> work with it and make decision on how to proceed.
>
> From technical perspective, as I mentioned, SDK is quite huge and
> include a lot of opensource
> components including full Linux, I don't think its reasonable to have
> it
> inside linux-firmware tree.
> What are the options to share it other then providing it on request
> basis:
> - git repository
> - store tarball somewhere on official website
Clearly that wasn't deemed appropriate for carl9170, so I don't see why
it'd be different here.
johannes
^ permalink raw reply
* [PATCH v2 2/3] mwifiex: Introduce mwifiex_probe_of() to parse common properties
From: Amitkumar Karwar @ 2016-11-11 11:15 UTC (permalink / raw)
To: linux-wireless
Cc: Cathy Luo, Nishant Sarmukadam, rajatja, briannorris,
dmitry.torokhov
In-Reply-To: <1478862911-15498-1-git-send-email-akarwar@marvell.com>
From: Rajat Jain <rajatja@google.com>
Introduce function mwifiex_probe_of() to parse common properties.
Since the interface drivers get to decide whether or not the device
tree node was a valid one (depending on the compatible property),
let the interface drivers pass a flag to indicate whether the device
tree node was a valid one.
The function mwifiex_probe_of()nodetself is currently only a place
holder with the next patch adding content to it.
Signed-off-by: Rajat Jain <rajatja@google.com>
---
v2: Same as v1
---
drivers/net/wireless/marvell/mwifiex/main.c | 15 ++++++++++++++-
drivers/net/wireless/marvell/mwifiex/main.h | 2 +-
drivers/net/wireless/marvell/mwifiex/pcie.c | 4 +++-
drivers/net/wireless/marvell/mwifiex/sdio.c | 4 +++-
drivers/net/wireless/marvell/mwifiex/sta_cmd.c | 5 +----
drivers/net/wireless/marvell/mwifiex/usb.c | 2 +-
6 files changed, 23 insertions(+), 9 deletions(-)
diff --git a/drivers/net/wireless/marvell/mwifiex/main.c b/drivers/net/wireless/marvell/mwifiex/main.c
index dcceab2..b2f3d96 100644
--- a/drivers/net/wireless/marvell/mwifiex/main.c
+++ b/drivers/net/wireless/marvell/mwifiex/main.c
@@ -1552,6 +1552,16 @@ void mwifiex_do_flr(struct mwifiex_adapter *adapter, bool prepare)
}
EXPORT_SYMBOL_GPL(mwifiex_do_flr);
+static void mwifiex_probe_of(struct mwifiex_adapter *adapter)
+{
+ struct device *dev = adapter->dev;
+
+ if (!dev->of_node)
+ return;
+
+ adapter->dt_node = dev->of_node;
+}
+
/*
* This function adds the card.
*
@@ -1568,7 +1578,7 @@ void mwifiex_do_flr(struct mwifiex_adapter *adapter, bool prepare)
int
mwifiex_add_card(void *card, struct semaphore *sem,
struct mwifiex_if_ops *if_ops, u8 iface_type,
- struct device *dev)
+ struct device *dev, bool of_node_valid)
{
struct mwifiex_adapter *adapter;
@@ -1581,6 +1591,9 @@ void mwifiex_do_flr(struct mwifiex_adapter *adapter, bool prepare)
}
adapter->dev = dev;
+ if (of_node_valid)
+ mwifiex_probe_of(adapter);
+
adapter->iface_type = iface_type;
adapter->card_sem = sem;
diff --git a/drivers/net/wireless/marvell/mwifiex/main.h b/drivers/net/wireless/marvell/mwifiex/main.h
index 2664513..0c07434 100644
--- a/drivers/net/wireless/marvell/mwifiex/main.h
+++ b/drivers/net/wireless/marvell/mwifiex/main.h
@@ -1413,7 +1413,7 @@ static inline u8 mwifiex_is_tdls_link_setup(u8 status)
int mwifiex_init_shutdown_fw(struct mwifiex_private *priv,
u32 func_init_shutdown);
int mwifiex_add_card(void *, struct semaphore *, struct mwifiex_if_ops *, u8,
- struct device *);
+ struct device *, bool);
int mwifiex_remove_card(struct mwifiex_adapter *, struct semaphore *);
void mwifiex_get_version(struct mwifiex_adapter *adapter, char *version,
diff --git a/drivers/net/wireless/marvell/mwifiex/pcie.c b/drivers/net/wireless/marvell/mwifiex/pcie.c
index de6939c..fe7f3b0 100644
--- a/drivers/net/wireless/marvell/mwifiex/pcie.c
+++ b/drivers/net/wireless/marvell/mwifiex/pcie.c
@@ -201,6 +201,7 @@ static int mwifiex_pcie_probe(struct pci_dev *pdev,
const struct pci_device_id *ent)
{
struct pcie_service_card *card;
+ bool valid_of_node = false;
int ret;
pr_debug("info: vendor=0x%4.04X device=0x%4.04X rev=%d\n",
@@ -228,10 +229,11 @@ static int mwifiex_pcie_probe(struct pci_dev *pdev,
ret = mwifiex_pcie_probe_of(&pdev->dev);
if (ret)
goto err_free;
+ valid_of_node = true;
}
ret = mwifiex_add_card(card, &add_remove_card_sem, &pcie_ops,
- MWIFIEX_PCIE, &pdev->dev);
+ MWIFIEX_PCIE, &pdev->dev, valid_of_node);
if (ret) {
pr_err("%s failed\n", __func__);
goto err_free;
diff --git a/drivers/net/wireless/marvell/mwifiex/sdio.c b/drivers/net/wireless/marvell/mwifiex/sdio.c
index c95f41f..558743a 100644
--- a/drivers/net/wireless/marvell/mwifiex/sdio.c
+++ b/drivers/net/wireless/marvell/mwifiex/sdio.c
@@ -156,6 +156,7 @@ static int mwifiex_sdio_probe_of(struct device *dev, struct sdio_mmc_card *card)
{
int ret;
struct sdio_mmc_card *card = NULL;
+ bool valid_of_node = false;
pr_debug("info: vendor=0x%4.04X device=0x%4.04X class=%d function=%d\n",
func->vendor, func->device, func->class, func->num);
@@ -203,10 +204,11 @@ static int mwifiex_sdio_probe_of(struct device *dev, struct sdio_mmc_card *card)
dev_err(&func->dev, "SDIO dt node parse failed\n");
goto err_disable;
}
+ valid_of_node = true;
}
ret = mwifiex_add_card(card, &add_remove_card_sem, &sdio_ops,
- MWIFIEX_SDIO, &func->dev);
+ MWIFIEX_SDIO, &func->dev, valid_of_node);
if (ret) {
dev_err(&func->dev, "add card failed\n");
goto err_disable;
diff --git a/drivers/net/wireless/marvell/mwifiex/sta_cmd.c b/drivers/net/wireless/marvell/mwifiex/sta_cmd.c
index b697b61..bcd6408 100644
--- a/drivers/net/wireless/marvell/mwifiex/sta_cmd.c
+++ b/drivers/net/wireless/marvell/mwifiex/sta_cmd.c
@@ -2235,10 +2235,7 @@ int mwifiex_sta_init_cmd(struct mwifiex_private *priv, u8 first_sta, bool init)
* The cal-data can be read from device tree and/or
* a configuration file and downloaded to firmware.
*/
- if ((priv->adapter->iface_type == MWIFIEX_SDIO ||
- priv->adapter->iface_type == MWIFIEX_PCIE) &&
- adapter->dev->of_node) {
- adapter->dt_node = adapter->dev->of_node;
+ if (adapter->dt_node) {
if (of_property_read_u32(adapter->dt_node,
"marvell,wakeup-pin",
&data) == 0) {
diff --git a/drivers/net/wireless/marvell/mwifiex/usb.c b/drivers/net/wireless/marvell/mwifiex/usb.c
index f847fff..11c9629 100644
--- a/drivers/net/wireless/marvell/mwifiex/usb.c
+++ b/drivers/net/wireless/marvell/mwifiex/usb.c
@@ -476,7 +476,7 @@ static int mwifiex_usb_probe(struct usb_interface *intf,
usb_set_intfdata(intf, card);
ret = mwifiex_add_card(card, &add_remove_card_sem, &usb_ops,
- MWIFIEX_USB, &card->udev->dev);
+ MWIFIEX_USB, &card->udev->dev, false);
if (ret) {
pr_err("%s: mwifiex_add_card failed: %d\n", __func__, ret);
usb_reset_device(udev);
--
1.9.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