[PATCH v2 0/3] mmc: Fixup HW reset for SDIO cards

[PATCH v2 0/3] mmc: Fixup HW reset for SDIO cards

[Ulf Hansson]

Changes in v2:
	- Add adaptations to the mwifiex driver.
	- Keep existing syncronous reset behaviour if the SDIO card has a single
	func driver.

It has turned out that it's not a good idea to try to power cycle and to
re-initialize the SDIO card, as currently done through mmc_hw_reset(). This
because there may be multiple SDIO funcs attached to the same SDIO card and
some of the others that didn't execute the call to mmc_hw_reset(), may then
simply experience an undefined behaviour.

The following patches in this series attempts to address this problem, by
reworking the mmc_hw_reset() behaviour for SDIO and by adopting the Marvel
mwifiex driver to these changes.

Note that, I don't have the HW at hand so the the code has only compile tested.
Test on HW is greatly appreciated!

Ulf Hansson (3):
  mwifiex: Re-work support for SDIO HW reset
  mmc: core: Drop check for mmc_card_is_removable() in mmc_rescan()
  mmc: core: Re-work HW reset for SDIO cards

 drivers/mmc/core/core.c                     | 12 +++-----
 drivers/mmc/core/core.h                     |  2 ++
 drivers/mmc/core/sdio.c                     | 28 ++++++++++++++++-
 drivers/mmc/core/sdio_bus.c                 |  9 +++++-
 drivers/net/wireless/marvell/mwifiex/main.c |  6 +++-
 drivers/net/wireless/marvell/mwifiex/main.h |  1 +
 drivers/net/wireless/marvell/mwifiex/sdio.c | 33 ++++++++++++++-------
 include/linux/mmc/card.h                    |  1 +
 8 files changed, 70 insertions(+), 22 deletions(-)

-- 
2.17.1

[PATCH v2 1/3] mwifiex: Re-work support for SDIO HW reset

[Ulf Hansson]

The SDIO HW reset procedure in mwifiex_sdio_card_reset_work() is broken,
when the SDIO card is shared with another SDIO func driver. This is the
case when the Bluetooth btmrvl driver is being used in combination with
mwifiex. More precisely, when mwifiex_sdio_card_reset_work() runs to resets
the SDIO card, the btmrvl driver doesn't get notified about it. Beyond that
point, the btmrvl driver will fail to communicate with the SDIO card.

This is a generic problem for SDIO func drivers sharing an SDIO card, which
are about to be addressed in subsequent changes to the mmc core and the
mmc_hw_reset() interface. In principle, these changes means the
mmc_hw_reset() interface starts to return 1 if the are multiple drivers for
the SDIO card, as to indicate to the caller that the reset needed to be
scheduled asynchronously through a hotplug mechanism of the SDIO card.

Let's prepare the mwifiex driver to support the upcoming new behaviour of
mmc_hw_reset(), which means extending the mwifiex_sdio_card_reset_work() to
support the asynchronous SDIO HW reset path. This also means, we need to
allow the ->remove() callback to run, without waiting for the FW to be
loaded. Additionally, during system suspend, mwifiex_sdio_suspend() may be
called when a reset has been scheduled, but waiting to be executed. In this
scenario let's simply return -EBUSY to abort the suspend process, as to
allow the reset to be completed first.

Signed-off-by: Ulf Hansson <ulf.hansson-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
---
 drivers/net/wireless/marvell/mwifiex/main.c |  6 +++-
 drivers/net/wireless/marvell/mwifiex/main.h |  1 +
 drivers/net/wireless/marvell/mwifiex/sdio.c | 33 ++++++++++++++-------
 3 files changed, 28 insertions(+), 12 deletions(-)

diff --git a/drivers/net/wireless/marvell/mwifiex/main.c b/drivers/net/wireless/marvell/mwifiex/main.c
index a9657ae6d782..dbdbdd6769a9 100644
--- a/drivers/net/wireless/marvell/mwifiex/main.c
+++ b/drivers/net/wireless/marvell/mwifiex/main.c
@@ -76,6 +76,7 @@ static int mwifiex_register(void *card, struct device *dev,
 	*padapter = adapter;
 	adapter->dev = dev;
 	adapter->card = card;
+	adapter->is_adapter_up = false;
 
 	/* Save interface specific operations in adapter */
 	memmove(&adapter->if_ops, if_ops, sizeof(struct mwifiex_if_ops));
@@ -631,6 +632,7 @@ static int _mwifiex_fw_dpc(const struct firmware *firmware, void *context)
 
 	mwifiex_drv_get_driver_version(adapter, fmt, sizeof(fmt) - 1);
 	mwifiex_dbg(adapter, MSG, "driver_version = %s\n", fmt);
+	adapter->is_adapter_up = true;
 	goto done;
 
 err_add_intf:
@@ -1469,6 +1471,7 @@ int mwifiex_shutdown_sw(struct mwifiex_adapter *adapter)
 	mwifiex_deauthenticate(priv, NULL);
 
 	mwifiex_uninit_sw(adapter);
+	adapter->is_adapter_up = false;
 
 	if (adapter->if_ops.down_dev)
 		adapter->if_ops.down_dev(adapter);
@@ -1730,7 +1733,8 @@ int mwifiex_remove_card(struct mwifiex_adapter *adapter)
 	if (!adapter)
 		return 0;
 
-	mwifiex_uninit_sw(adapter);
+	if (adapter->is_adapter_up)
+		mwifiex_uninit_sw(adapter);
 
 	if (adapter->irq_wakeup >= 0)
 		device_init_wakeup(adapter->dev, false);
diff --git a/drivers/net/wireless/marvell/mwifiex/main.h b/drivers/net/wireless/marvell/mwifiex/main.h
index 095837fba300..7703d2e5d2e0 100644
--- a/drivers/net/wireless/marvell/mwifiex/main.h
+++ b/drivers/net/wireless/marvell/mwifiex/main.h
@@ -1017,6 +1017,7 @@ struct mwifiex_adapter {
 
 	/* For synchronizing FW initialization with device lifecycle. */
 	struct completion *fw_done;
+	bool is_adapter_up;
 
 	bool ext_scan;
 	u8 fw_api_ver;
diff --git a/drivers/net/wireless/marvell/mwifiex/sdio.c b/drivers/net/wireless/marvell/mwifiex/sdio.c
index 24c041dad9f6..2417c94c29c0 100644
--- a/drivers/net/wireless/marvell/mwifiex/sdio.c
+++ b/drivers/net/wireless/marvell/mwifiex/sdio.c
@@ -444,6 +444,9 @@ static int mwifiex_sdio_suspend(struct device *dev)
 		return 0;
 	}
 
+	if (!adapter->is_adapter_up)
+		return -EBUSY;
+
 	mwifiex_enable_wake(adapter);
 
 	/* Enable the Host Sleep */
@@ -2220,22 +2223,30 @@ static void mwifiex_sdio_card_reset_work(struct mwifiex_adapter *adapter)
 	struct sdio_func *func = card->func;
 	int ret;
 
+	/* Prepare the adapter for the reset. */
 	mwifiex_shutdown_sw(adapter);
+	clear_bit(MWIFIEX_IFACE_WORK_DEVICE_DUMP, &card->work_flags);
+	clear_bit(MWIFIEX_IFACE_WORK_CARD_RESET, &card->work_flags);
 
-	/* power cycle the adapter */
+	/* Run a HW reset of the SDIO interface. */
 	sdio_claim_host(func);
-	mmc_hw_reset(func->card->host);
+	ret = mmc_hw_reset(func->card->host);
 	sdio_release_host(func);
 
-	/* Previous save_adapter won't be valid after this. We will cancel
-	 * pending work requests.
-	 */
-	clear_bit(MWIFIEX_IFACE_WORK_DEVICE_DUMP, &card->work_flags);
-	clear_bit(MWIFIEX_IFACE_WORK_CARD_RESET, &card->work_flags);

[PATCH v2 2/3] mmc: core: Drop check for mmc_card_is_removable() in mmc_rescan()

[Ulf Hansson]

Upfront in mmc_rescan() we use the host->rescan_entered flag, to allow
scanning only once for non-removable cards. Therefore, it's also not
possible that we can have a corresponding card bus attached (host->bus_ops
is NULL), when we are scanning non-removable cards.

For this reason, let' drop the check for mmc_card_is_removable() as it's
redundant.

Reviewed-by: Douglas Anderson <dianders-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
Signed-off-by: Ulf Hansson <ulf.hansson-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
---
 drivers/mmc/core/core.c | 7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)

diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
index 221127324709..6f8342702c73 100644
--- a/drivers/mmc/core/core.c
+++ b/drivers/mmc/core/core.c
@@ -2297,11 +2297,8 @@ void mmc_rescan(struct work_struct *work)
 
 	mmc_bus_get(host);
 
-	/*
-	 * if there is a _removable_ card registered, check whether it is
-	 * still present
-	 */
-	if (host->bus_ops && !host->bus_dead && mmc_card_is_removable(host))
+	/* Verify a registered card to be functional, else remove it. */
+	if (host->bus_ops && !host->bus_dead)
 		host->bus_ops->detect(host);
 
 	host->detect_change = 0;
-- 
2.17.1

[PATCH v2 3/3] mmc: core: Re-work HW reset for SDIO cards

[Ulf Hansson]

It have turned out that it's not a good idea to unconditionally do a power
cycle and then to re-initialize the SDIO card, as currently done through
mmc_hw_reset() -> mmc_sdio_hw_reset(). This because there may be multiple
SDIO func drivers probed, who also shares the same SDIO card.

To address these scenarios, one may be tempted to use a notification
mechanism, as to allow the core to inform each of the probed func drivers,
about an ongoing HW reset. However, supporting such an operation from the
func driver point of view, may not be entirely trivial.

Therefore, let's use a more simplistic approach to solve the problem, by
instead forcing the card to be removed and re-detected, via scheduling a
rescan-work. In this way, we can rely on existing infrastructure, as the
func driver's ->remove() and ->probe() callbacks, becomes invoked to deal
with the cleanup and the re-initialization.

This solution may be considered as rather heavy, especially if a func
driver doesn't share its card with other func drivers. To address this,
let's keep the current immediate HW reset option as well, but run it only
when there is one func driver probed for the card.

Finally, to allow the caller of mmc_hw_reset(), to understand if the reset
is being asynchronously managed from a scheduled work, it returns 1
(propagated from mmc_sdio_hw_reset()). If the HW reset is executed
successfully and synchronously it returns 0, which maintains the existing
behaviour.

Signed-off-by: Ulf Hansson <ulf.hansson-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
---
 drivers/mmc/core/core.c     |  5 ++---
 drivers/mmc/core/core.h     |  2 ++
 drivers/mmc/core/sdio.c     | 28 +++++++++++++++++++++++++++-
 drivers/mmc/core/sdio_bus.c |  9 ++++++++-
 include/linux/mmc/card.h    |  1 +
 5 files changed, 40 insertions(+), 5 deletions(-)

diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
index 6f8342702c73..abf8f5eb0a1c 100644
--- a/drivers/mmc/core/core.c
+++ b/drivers/mmc/core/core.c
@@ -1469,8 +1469,7 @@ void mmc_detach_bus(struct mmc_host *host)
 	mmc_bus_put(host);
 }
 
-static void _mmc_detect_change(struct mmc_host *host, unsigned long delay,
-				bool cd_irq)
+void _mmc_detect_change(struct mmc_host *host, unsigned long delay, bool cd_irq)
 {
 	/*
 	 * If the device is configured as wakeup, we prevent a new sleep for
@@ -2129,7 +2128,7 @@ int mmc_hw_reset(struct mmc_host *host)
 	ret = host->bus_ops->hw_reset(host);
 	mmc_bus_put(host);
 
-	if (ret)
+	if (ret < 0)
 		pr_warn("%s: tried to HW reset card, got error %d\n",
 			mmc_hostname(host), ret);
 
diff --git a/drivers/mmc/core/core.h b/drivers/mmc/core/core.h
index 328c78dbee66..575ac0257af2 100644
--- a/drivers/mmc/core/core.h
+++ b/drivers/mmc/core/core.h
@@ -70,6 +70,8 @@ void mmc_rescan(struct work_struct *work);
 void mmc_start_host(struct mmc_host *host);
 void mmc_stop_host(struct mmc_host *host);
 
+void _mmc_detect_change(struct mmc_host *host, unsigned long delay,
+			bool cd_irq);
 int _mmc_detect_card_removed(struct mmc_host *host);
 int mmc_detect_card_removed(struct mmc_host *host);
 
diff --git a/drivers/mmc/core/sdio.c b/drivers/mmc/core/sdio.c
index 26cabd53ddc5..ebb387aa5158 100644
--- a/drivers/mmc/core/sdio.c
+++ b/drivers/mmc/core/sdio.c
@@ -1048,9 +1048,35 @@ static int mmc_sdio_runtime_resume(struct mmc_host *host)
 	return ret;
 }
 
+/*
+ * SDIO HW reset
+ *
+ * Returns 0 if the HW reset was executed synchronously, returns 1 if the HW
+ * reset was asynchronously scheduled, else a negative error code.
+ */
 static int mmc_sdio_hw_reset(struct mmc_host *host)
 {
-	mmc_power_cycle(host, host->card->ocr);
+	struct mmc_card *card = host->card;
+
+	/*
+	 * In case the card is shared among multiple func drivers, reset the
+	 * card through a rescan work. In this way it will be removed and
+	 * re-detected, thus all func drivers becomes informed about it.
+	 */
+	if (atomic_read(&card->sdio_funcs_probed) > 1) {
+		if (mmc_card_removed(card))
+			return 1;
+		host->rescan_entered = 0;
+		mmc_card_set_removed(card);
+		_mmc_detect_change(host, 0, false);
+		return 1;
+	}
+
+	/*
+	 * A single func driver has been probed, then let's skip the heavy
+	 * hotplug dance above and execute the reset immediately.
+	 */
+	mmc_power_cycle(host, card->ocr);
 	return mmc_sdio_reinit_card(host);
 }
 
diff --git a/drivers/mmc/core/sdio_bus.c b/drivers/mmc/core/sdio_bus.c
index 2963e6542958..3cc928282af7 100644
--- a/drivers/mmc/core/sdio_bus.c
+++ b/drivers/mmc/core/sdio_bus.c
@@ -138,6 +138,8 @@ static int sdio_bus_probe(struct device *dev)
 	if (ret)
 		return ret;
 
+	atomic_inc(&func->card->sdio_funcs_probed);
+
 	/* Unbound SDIO functions are always suspended.
 	 * During probe, the function is set active and the usage count
 	 * is incremented.  If the driver supports runtime PM,
@@ -153,7 +155,10 @@ static int sdio_bus_probe(struct device *dev)
 	/* Set the default block size so the driver is sure it's something
 	 * sensible. */
 	sdio_claim_host(func);
-	ret = sdio_set_block_size(func, 0);
+	if (mmc_card_removed(func->card))
+		ret = -ENOMEDIUM;
+	else
+		ret = sdio_set_block_size(func, 0);
 	sdio_release_host(func);
 	if (ret)
 		goto disable_runtimepm;
@@ -165,6 +170,7 @@ static int sdio_bus_probe(struct device *dev)
 	return 0;
 
 disable_runtimepm:
+	atomic_dec(&func->card->sdio_funcs_probed);
 	if (func->card->host->caps & MMC_CAP_POWER_OFF_CARD)
 		pm_runtime_put_noidle(dev);
 	dev_pm_domain_detach(dev, false);
@@ -181,6 +187,7 @@ static int sdio_bus_remove(struct device *dev)
 		pm_runtime_get_sync(dev);
 
 	drv->remove(func);
+	atomic_dec(&func->card->sdio_funcs_probed);
 
 	if (func->irq_handler) {
 		pr_warn("WARNING: driver %s did not remove its interrupt handler!\n",
diff --git a/include/linux/mmc/card.h b/include/linux/mmc/card.h
index 9b6336ad3266..e459b38ef33c 100644
--- a/include/linux/mmc/card.h
+++ b/include/linux/mmc/card.h
@@ -291,6 +291,7 @@ struct mmc_card {
 	struct sd_switch_caps	sw_caps;	/* switch (CMD6) caps */
 
 	unsigned int		sdio_funcs;	/* number of SDIO functions */
+	atomic_t		sdio_funcs_probed; /* number of probed SDIO funcs */
 	struct sdio_cccr	cccr;		/* common card info */
 	struct sdio_cis		cis;		/* common tuple info */
 	struct sdio_func	*sdio_func[SDIO_MAX_FUNCS]; /* SDIO functions (devices) */
-- 
2.17.1

Re: [PATCH v2 0/3] mmc: Fixup HW reset for SDIO cards

[Tony Lindgren]

Hi,

* Ulf Hansson <ulf.hansson-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> [191109 10:31]:
> Changes in v2:
> 	- Add adaptations to the mwifiex driver.
> 	- Keep existing syncronous reset behaviour if the SDIO card has a single
> 	func driver.
> 
> It has turned out that it's not a good idea to try to power cycle and to
> re-initialize the SDIO card, as currently done through mmc_hw_reset(). This
> because there may be multiple SDIO funcs attached to the same SDIO card and
> some of the others that didn't execute the call to mmc_hw_reset(), may then
> simply experience an undefined behaviour.
> 
> The following patches in this series attempts to address this problem, by
> reworking the mmc_hw_reset() behaviour for SDIO and by adopting the Marvel
> mwifiex driver to these changes.
> 
> Note that, I don't have the HW at hand so the the code has only compile tested.
> Test on HW is greatly appreciated!

Looks good to me. I tried testing this, but looks like at least on duovero
mwifiex_sdio is broken in v5.4-rc7:

# dmesg | grep mwifi
mwifiex_sdio mmc1:0001:1: poll card status failed, tries = 100
mwifiex_sdio mmc1:0001:1: FW download with helper:       poll status timeout @ 0
mwifiex_sdio mmc1:0001:1: prog_fw failed ret=0xffffffff
mwifiex_sdio mmc1:0001:1: info: _mwifiex_fw_dpc: unregister device

No idea when it broke and what might be the issue this time around.
Any ideas?

Regards,

Tony

Re: [PATCH v2 3/3] mmc: core: Re-work HW reset for SDIO cards

[Doug Anderson]

Hi,

On Sat, Nov 9, 2019 at 2:31 AM Ulf Hansson <ulf.hansson-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> wrote:
>
> diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
> index 6f8342702c73..abf8f5eb0a1c 100644
> --- a/drivers/mmc/core/core.c
> +++ b/drivers/mmc/core/core.c
> @@ -1469,8 +1469,7 @@ void mmc_detach_bus(struct mmc_host *host)
>         mmc_bus_put(host);
>  }
>
> -static void _mmc_detect_change(struct mmc_host *host, unsigned long delay,
> -                               bool cd_irq)
> +void _mmc_detect_change(struct mmc_host *host, unsigned long delay, bool cd_irq)
>  {
>         /*
>          * If the device is configured as wakeup, we prevent a new sleep for
> @@ -2129,7 +2128,7 @@ int mmc_hw_reset(struct mmc_host *host)
>         ret = host->bus_ops->hw_reset(host);
>         mmc_bus_put(host);
>
> -       if (ret)
> +       if (ret < 0)
>                 pr_warn("%s: tried to HW reset card, got error %d\n",
>                         mmc_hostname(host), ret);

Other callers besides marvell need to be updated?  In theory only SDIO
should have positive return values so I guess we don't care about the
caller in drivers/mmc/core/block.c, right?  What about:

drivers/net/wireless/ath/ath10k/sdio.c

...I guess I don't know if there is more than one function probed
there.  Maybe there's not and thus we're fine here too?


I didn't spend massive amounts of time looking for potential problems,
but in general seems workable to me.  Thanks!

Reviewed-by: Douglas Anderson <dianders-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>

Re: [PATCH v2 1/3] mwifiex: Re-work support for SDIO HW reset

[Doug Anderson]

Hi,

On Sat, Nov 9, 2019 at 2:31 AM Ulf Hansson <ulf.hansson-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> wrote:
>
> The SDIO HW reset procedure in mwifiex_sdio_card_reset_work() is broken,
> when the SDIO card is shared with another SDIO func driver. This is the
> case when the Bluetooth btmrvl driver is being used in combination with
> mwifiex. More precisely, when mwifiex_sdio_card_reset_work() runs to resets
> the SDIO card, the btmrvl driver doesn't get notified about it. Beyond that
> point, the btmrvl driver will fail to communicate with the SDIO card.
>
> This is a generic problem for SDIO func drivers sharing an SDIO card, which
> are about to be addressed in subsequent changes to the mmc core and the
> mmc_hw_reset() interface. In principle, these changes means the
> mmc_hw_reset() interface starts to return 1 if the are multiple drivers for
> the SDIO card, as to indicate to the caller that the reset needed to be
> scheduled asynchronously through a hotplug mechanism of the SDIO card.
>
> Let's prepare the mwifiex driver to support the upcoming new behaviour of
> mmc_hw_reset(), which means extending the mwifiex_sdio_card_reset_work() to
> support the asynchronous SDIO HW reset path. This also means, we need to
> allow the ->remove() callback to run, without waiting for the FW to be
> loaded. Additionally, during system suspend, mwifiex_sdio_suspend() may be
> called when a reset has been scheduled, but waiting to be executed. In this
> scenario let's simply return -EBUSY to abort the suspend process, as to
> allow the reset to be completed first.
>
> Signed-off-by: Ulf Hansson <ulf.hansson-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
> ---
>  drivers/net/wireless/marvell/mwifiex/main.c |  6 +++-
>  drivers/net/wireless/marvell/mwifiex/main.h |  1 +
>  drivers/net/wireless/marvell/mwifiex/sdio.c | 33 ++++++++++++++-------
>  3 files changed, 28 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/net/wireless/marvell/mwifiex/main.c b/drivers/net/wireless/marvell/mwifiex/main.c
> index a9657ae6d782..dbdbdd6769a9 100644
> --- a/drivers/net/wireless/marvell/mwifiex/main.c
> +++ b/drivers/net/wireless/marvell/mwifiex/main.c
> @@ -76,6 +76,7 @@ static int mwifiex_register(void *card, struct device *dev,
>         *padapter = adapter;
>         adapter->dev = dev;
>         adapter->card = card;
> +       adapter->is_adapter_up = false;

Probably not needed.  The 'adapter' was kzalloc-ed a few lines above
and there's no need to re-init to 0.


> diff --git a/drivers/net/wireless/marvell/mwifiex/sdio.c b/drivers/net/wireless/marvell/mwifiex/sdio.c
> index 24c041dad9f6..2417c94c29c0 100644
> --- a/drivers/net/wireless/marvell/mwifiex/sdio.c
> +++ b/drivers/net/wireless/marvell/mwifiex/sdio.c
> @@ -444,6 +444,9 @@ static int mwifiex_sdio_suspend(struct device *dev)
>                 return 0;
>         }
>
> +       if (!adapter->is_adapter_up)
> +               return -EBUSY;

I'm moderately concerned that there might be cases where firmware
never got loaded but we could suspend/resume OK.  ...and now we never
will?  I'm not familiar enough with the code to know if this is a real
concern, so I guess we can do this and then see...


> @@ -2220,22 +2223,30 @@ static void mwifiex_sdio_card_reset_work(struct mwifiex_adapter *adapter)
>         struct sdio_func *func = card->func;
>         int ret;
>
> +       /* Prepare the adapter for the reset. */
>         mwifiex_shutdown_sw(adapter);
> +       clear_bit(MWIFIEX_IFACE_WORK_DEVICE_DUMP, &card->work_flags);
> +       clear_bit(MWIFIEX_IFACE_WORK_CARD_RESET, &card->work_flags);
>
> -       /* power cycle the adapter */
> +       /* Run a HW reset of the SDIO interface. */
>         sdio_claim_host(func);
> -       mmc_hw_reset(func->card->host);
> +       ret = mmc_hw_reset(func->card->host);
>         sdio_release_host(func);
>
> -       /* Previous save_adapter won't be valid after this. We will cancel
> -        * pending work requests.
> -        */
> -       clear_bit(MWIFIEX_IFACE_WORK_DEVICE_DUMP, &card->work_flags);
> -       clear_bit(MWIFIEX_IFACE_WORK_CARD_RESET, &card->work_flags);

I don't know enough about the clearing of these bits to confirm that
it's OK to move their clearing to be before the mmc_hw_reset().
Possibly +Brian Norris does?


I can't promise that I didn't miss anything, but to the best of my
knowledge this is good now:

Reviewed-by: Douglas Anderson <dianders-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>

Re: [PATCH v2 0/3] mmc: Fixup HW reset for SDIO cards

[Doug Anderson]

Hi,

On Sat, Nov 9, 2019 at 2:31 AM Ulf Hansson <ulf.hansson-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> wrote:
>
> Changes in v2:
>         - Add adaptations to the mwifiex driver.
>         - Keep existing syncronous reset behaviour if the SDIO card has a single
>         func driver.
>
> It has turned out that it's not a good idea to try to power cycle and to
> re-initialize the SDIO card, as currently done through mmc_hw_reset(). This
> because there may be multiple SDIO funcs attached to the same SDIO card and
> some of the others that didn't execute the call to mmc_hw_reset(), may then
> simply experience an undefined behaviour.
>
> The following patches in this series attempts to address this problem, by
> reworking the mmc_hw_reset() behaviour for SDIO and by adopting the Marvel
> mwifiex driver to these changes.
>
> Note that, I don't have the HW at hand so the the code has only compile tested.
> Test on HW is greatly appreciated!
>
> Ulf Hansson (3):
>   mwifiex: Re-work support for SDIO HW reset
>   mmc: core: Drop check for mmc_card_is_removable() in mmc_rescan()
>   mmc: core: Re-work HW reset for SDIO cards
>
>  drivers/mmc/core/core.c                     | 12 +++-----
>  drivers/mmc/core/core.h                     |  2 ++
>  drivers/mmc/core/sdio.c                     | 28 ++++++++++++++++-
>  drivers/mmc/core/sdio_bus.c                 |  9 +++++-
>  drivers/net/wireless/marvell/mwifiex/main.c |  6 +++-
>  drivers/net/wireless/marvell/mwifiex/main.h |  1 +
>  drivers/net/wireless/marvell/mwifiex/sdio.c | 33 ++++++++++++++-------
>  include/linux/mmc/card.h                    |  1 +
>  8 files changed, 70 insertions(+), 22 deletions(-)

I put this on rk3288-veyron-jerry atop v5.4-rc7 and I could run my
test case for a while, AKA I got over 50 cycles of:

---

for i in $(seq 1000); do
  echo "LOOP $i --------"
  echo 1 > /sys/kernel/debug/mwifiex/mlan0/reset

  while true; do
    if ! ping6 -w15 -c1 "${GW}" >/dev/null 2>&1; then
      fail=$(( fail + 1 ))
      echo "Fail WiFi ${fail}"
      if [[ ${fail} == 3 ]]; then
        exit 1
      fi
    else
      fail=0
      break
    fi
  done

  hciconfig hci0 down
  sleep 1
  if ! hciconfig hci0 up; then
    echo "Fail BT"
    exit 1
  fi

done

---

NOTE: with no patches I couldn't even get my test case to pass w/out
the BT bits and I swear that used to work before.  ...but I didn't
debug since the end result (with full card hotplug) is happy-working
for me.  I'll still use it as further argument that (IMO) full unplug
/ plug of the card is better it uses more standard code paths and is
less likely to break.  ;-)

Tested-by: Douglas Anderson <dianders-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>

Re: [PATCH v2 1/3] mwifiex: Re-work support for SDIO HW reset

[Ulf Hansson]

On Tue, 12 Nov 2019 at 01:33, Doug Anderson <dianders-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org> wrote:
>
> Hi,
>
> On Sat, Nov 9, 2019 at 2:31 AM Ulf Hansson <ulf.hansson-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> wrote:
> >
> > The SDIO HW reset procedure in mwifiex_sdio_card_reset_work() is broken,
> > when the SDIO card is shared with another SDIO func driver. This is the
> > case when the Bluetooth btmrvl driver is being used in combination with
> > mwifiex. More precisely, when mwifiex_sdio_card_reset_work() runs to resets
> > the SDIO card, the btmrvl driver doesn't get notified about it. Beyond that
> > point, the btmrvl driver will fail to communicate with the SDIO card.
> >
> > This is a generic problem for SDIO func drivers sharing an SDIO card, which
> > are about to be addressed in subsequent changes to the mmc core and the
> > mmc_hw_reset() interface. In principle, these changes means the
> > mmc_hw_reset() interface starts to return 1 if the are multiple drivers for
> > the SDIO card, as to indicate to the caller that the reset needed to be
> > scheduled asynchronously through a hotplug mechanism of the SDIO card.
> >
> > Let's prepare the mwifiex driver to support the upcoming new behaviour of
> > mmc_hw_reset(), which means extending the mwifiex_sdio_card_reset_work() to
> > support the asynchronous SDIO HW reset path. This also means, we need to
> > allow the ->remove() callback to run, without waiting for the FW to be
> > loaded. Additionally, during system suspend, mwifiex_sdio_suspend() may be
> > called when a reset has been scheduled, but waiting to be executed. In this
> > scenario let's simply return -EBUSY to abort the suspend process, as to
> > allow the reset to be completed first.
> >
> > Signed-off-by: Ulf Hansson <ulf.hansson-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
> > ---
> >  drivers/net/wireless/marvell/mwifiex/main.c |  6 +++-
> >  drivers/net/wireless/marvell/mwifiex/main.h |  1 +
> >  drivers/net/wireless/marvell/mwifiex/sdio.c | 33 ++++++++++++++-------
> >  3 files changed, 28 insertions(+), 12 deletions(-)
> >
> > diff --git a/drivers/net/wireless/marvell/mwifiex/main.c b/drivers/net/wireless/marvell/mwifiex/main.c
> > index a9657ae6d782..dbdbdd6769a9 100644
> > --- a/drivers/net/wireless/marvell/mwifiex/main.c
> > +++ b/drivers/net/wireless/marvell/mwifiex/main.c
> > @@ -76,6 +76,7 @@ static int mwifiex_register(void *card, struct device *dev,
> >         *padapter = adapter;
> >         adapter->dev = dev;
> >         adapter->card = card;
> > +       adapter->is_adapter_up = false;
>
> Probably not needed.  The 'adapter' was kzalloc-ed a few lines above
> and there's no need to re-init to 0.

Right, let me re-spin and drop this.

>
>
> > diff --git a/drivers/net/wireless/marvell/mwifiex/sdio.c b/drivers/net/wireless/marvell/mwifiex/sdio.c
> > index 24c041dad9f6..2417c94c29c0 100644
> > --- a/drivers/net/wireless/marvell/mwifiex/sdio.c
> > +++ b/drivers/net/wireless/marvell/mwifiex/sdio.c
> > @@ -444,6 +444,9 @@ static int mwifiex_sdio_suspend(struct device *dev)
> >                 return 0;
> >         }
> >
> > +       if (!adapter->is_adapter_up)
> > +               return -EBUSY;
>
> I'm moderately concerned that there might be cases where firmware
> never got loaded but we could suspend/resume OK.  ...and now we never
> will?  I'm not familiar enough with the code to know if this is a real
> concern, so I guess we can do this and then see...

There is a completion variable that is used to make sure the firmware
is loaded, before the mwifiex driver runs ->suspend|remove(). This is
needed, because during ->probe() the FW will be loaded asynchronously,
hence both mwifiex_sdio_remove() and mwifiex_sdio_suspend(), may be
called while waiting for the FW to be loaded.

If a HW reset has been scheduled but not completed, which would be the
case if mmc_hw_reset() gets called after mmc_pm_notify() with a
PM_SUSPEND_PREPARE. This is because mmc_pm_notify() then disables the
rescan work, but then re-kicks/enables it at PM_POST_SUSPEND (after
the system has resumed).

Returning -EBUSY, should allow the mmc rescan work to be completed
when the system have resumed.

Of course, one could also considering using pm_wakeup_event(), in case
mmc_hw_reset() needed to schedule the reset, as to prevent the system
for suspending for a small amount of time. As to make sure the rescan
work, gets to run. But I am not sure that's needed here.

>
>
> > @@ -2220,22 +2223,30 @@ static void mwifiex_sdio_card_reset_work(struct mwifiex_adapter *adapter)
> >         struct sdio_func *func = card->func;
> >         int ret;
> >
> > +       /* Prepare the adapter for the reset. */
> >         mwifiex_shutdown_sw(adapter);
> > +       clear_bit(MWIFIEX_IFACE_WORK_DEVICE_DUMP, &card->work_flags);
> > +       clear_bit(MWIFIEX_IFACE_WORK_CARD_RESET, &card->work_flags);
> >
> > -       /* power cycle the adapter */
> > +       /* Run a HW reset of the SDIO interface. */
> >         sdio_claim_host(func);
> > -       mmc_hw_reset(func->card->host);
> > +       ret = mmc_hw_reset(func->card->host);
> >         sdio_release_host(func);
> >
> > -       /* Previous save_adapter won't be valid after this. We will cancel
> > -        * pending work requests.
> > -        */
> > -       clear_bit(MWIFIEX_IFACE_WORK_DEVICE_DUMP, &card->work_flags);
> > -       clear_bit(MWIFIEX_IFACE_WORK_CARD_RESET, &card->work_flags);
>
> I don't know enough about the clearing of these bits to confirm that
> it's OK to move their clearing to be before the mmc_hw_reset().
> Possibly +Brian Norris does?

That shouldn't matter, because we are running in the path of the
mwifiex_sdio_work(), as work from the system_wq. Unless I am mistaken,
only one work of the type mwifiex_sdio_work() can execute at the same
time. By clearing these bits, we want to cancel any potential recently
scheduled work. It should matter if that's done before or after
mmc_hw_reset().

Moreover, this change makes it more consistent with how the pcie
driver clears the bits.

>
>
> I can't promise that I didn't miss anything, but to the best of my
> knowledge this is good now:
>
> Reviewed-by: Douglas Anderson <dianders-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>

Thanks!

Finally, if you want to verify that the above system suspend path
works fine, you could change the call to "_mmc_detect_change(host, 0,
false)" in mmc_sdio_hw_reset(), into "_mmc_detect_change(host,
msecs_to_jiffies(30000), false)", in patch3.

This should leave you a 30s window of where you can try to system
suspend the platform, while also waiting for the scheduled reset to be
completed.

Kind regards
Uffe

Re: [PATCH v2 3/3] mmc: core: Re-work HW reset for SDIO cards

[Ulf Hansson]

On Tue, 12 Nov 2019 at 01:33, Doug Anderson <dianders-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org> wrote:
>
> Hi,
>
> On Sat, Nov 9, 2019 at 2:31 AM Ulf Hansson <ulf.hansson-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> wrote:
> >
> > diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
> > index 6f8342702c73..abf8f5eb0a1c 100644
> > --- a/drivers/mmc/core/core.c
> > +++ b/drivers/mmc/core/core.c
> > @@ -1469,8 +1469,7 @@ void mmc_detach_bus(struct mmc_host *host)
> >         mmc_bus_put(host);
> >  }
> >
> > -static void _mmc_detect_change(struct mmc_host *host, unsigned long delay,
> > -                               bool cd_irq)
> > +void _mmc_detect_change(struct mmc_host *host, unsigned long delay, bool cd_irq)
> >  {
> >         /*
> >          * If the device is configured as wakeup, we prevent a new sleep for
> > @@ -2129,7 +2128,7 @@ int mmc_hw_reset(struct mmc_host *host)
> >         ret = host->bus_ops->hw_reset(host);
> >         mmc_bus_put(host);
> >
> > -       if (ret)
> > +       if (ret < 0)
> >                 pr_warn("%s: tried to HW reset card, got error %d\n",
> >                         mmc_hostname(host), ret);
>
> Other callers besides marvell need to be updated?  In theory only SDIO
> should have positive return values so I guess we don't care about the
> caller in drivers/mmc/core/block.c, right?

Correct, but maybe I should add some more information about that in a
function header of mmc_hw_reset(). Let me consider doing that as a
change on top.

>  What about:
>
> drivers/net/wireless/ath/ath10k/sdio.c
>
> ...I guess I don't know if there is more than one function probed
> there.  Maybe there's not and thus we're fine here too?

Well, honestly I don't know.

In any case, that would mean the driver is broken anyways and needs to
be fixed. At least that's my approach to doing this change.

>
>
> I didn't spend massive amounts of time looking for potential problems,
> but in general seems workable to me.  Thanks!
>
> Reviewed-by: Douglas Anderson <dianders-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>

Thanks!

Kind regards
Uffe

Re: [PATCH v2 0/3] mmc: Fixup HW reset for SDIO cards

[Ulf Hansson]

On Mon, 11 Nov 2019 at 23:08, Tony Lindgren <tony-4v6yS6AI5VpBDgjK7y7TUQ@public.gmane.org> wrote:
>
> Hi,
>
> * Ulf Hansson <ulf.hansson-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> [191109 10:31]:
> > Changes in v2:
> >       - Add adaptations to the mwifiex driver.
> >       - Keep existing syncronous reset behaviour if the SDIO card has a single
> >       func driver.
> >
> > It has turned out that it's not a good idea to try to power cycle and to
> > re-initialize the SDIO card, as currently done through mmc_hw_reset(). This
> > because there may be multiple SDIO funcs attached to the same SDIO card and
> > some of the others that didn't execute the call to mmc_hw_reset(), may then
> > simply experience an undefined behaviour.
> >
> > The following patches in this series attempts to address this problem, by
> > reworking the mmc_hw_reset() behaviour for SDIO and by adopting the Marvel
> > mwifiex driver to these changes.
> >
> > Note that, I don't have the HW at hand so the the code has only compile tested.
> > Test on HW is greatly appreciated!
>
> Looks good to me. I tried testing this, but looks like at least on duovero
> mwifiex_sdio is broken in v5.4-rc7:
>
> # dmesg | grep mwifi
> mwifiex_sdio mmc1:0001:1: poll card status failed, tries = 100
> mwifiex_sdio mmc1:0001:1: FW download with helper:       poll status timeout @ 0
> mwifiex_sdio mmc1:0001:1: prog_fw failed ret=0xffffffff
> mwifiex_sdio mmc1:0001:1: info: _mwifiex_fw_dpc: unregister device
>
> No idea when it broke and what might be the issue this time around.
> Any ideas?

Sorry, no good idea.

Perhaps something on the mmc host level that has changed?

Kind regards
Uffe

Re: [PATCH v2 0/3] mmc: Fixup HW reset for SDIO cards

[Ulf Hansson]

On Tue, 12 Nov 2019 at 01:51, Doug Anderson <dianders-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org> wrote:
>
> Hi,
>
> On Sat, Nov 9, 2019 at 2:31 AM Ulf Hansson <ulf.hansson-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> wrote:
> >
> > Changes in v2:
> >         - Add adaptations to the mwifiex driver.
> >         - Keep existing syncronous reset behaviour if the SDIO card has a single
> >         func driver.
> >
> > It has turned out that it's not a good idea to try to power cycle and to
> > re-initialize the SDIO card, as currently done through mmc_hw_reset(). This
> > because there may be multiple SDIO funcs attached to the same SDIO card and
> > some of the others that didn't execute the call to mmc_hw_reset(), may then
> > simply experience an undefined behaviour.
> >
> > The following patches in this series attempts to address this problem, by
> > reworking the mmc_hw_reset() behaviour for SDIO and by adopting the Marvel
> > mwifiex driver to these changes.
> >
> > Note that, I don't have the HW at hand so the the code has only compile tested.
> > Test on HW is greatly appreciated!
> >
> > Ulf Hansson (3):
> >   mwifiex: Re-work support for SDIO HW reset
> >   mmc: core: Drop check for mmc_card_is_removable() in mmc_rescan()
> >   mmc: core: Re-work HW reset for SDIO cards
> >
> >  drivers/mmc/core/core.c                     | 12 +++-----
> >  drivers/mmc/core/core.h                     |  2 ++
> >  drivers/mmc/core/sdio.c                     | 28 ++++++++++++++++-
> >  drivers/mmc/core/sdio_bus.c                 |  9 +++++-
> >  drivers/net/wireless/marvell/mwifiex/main.c |  6 +++-
> >  drivers/net/wireless/marvell/mwifiex/main.h |  1 +
> >  drivers/net/wireless/marvell/mwifiex/sdio.c | 33 ++++++++++++++-------
> >  include/linux/mmc/card.h                    |  1 +
> >  8 files changed, 70 insertions(+), 22 deletions(-)
>
> I put this on rk3288-veyron-jerry atop v5.4-rc7 and I could run my
> test case for a while, AKA I got over 50 cycles of:
>
> ---
>
> for i in $(seq 1000); do
>   echo "LOOP $i --------"
>   echo 1 > /sys/kernel/debug/mwifiex/mlan0/reset
>
>   while true; do
>     if ! ping6 -w15 -c1 "${GW}" >/dev/null 2>&1; then
>       fail=$(( fail + 1 ))
>       echo "Fail WiFi ${fail}"
>       if [[ ${fail} == 3 ]]; then
>         exit 1
>       fi
>     else
>       fail=0
>       break
>     fi
>   done
>
>   hciconfig hci0 down
>   sleep 1
>   if ! hciconfig hci0 up; then
>     echo "Fail BT"
>     exit 1
>   fi
>
> done
>
> ---
>
> NOTE: with no patches I couldn't even get my test case to pass w/out
> the BT bits and I swear that used to work before.  ...but I didn't
> debug since the end result (with full card hotplug) is happy-working
> for me.  I'll still use it as further argument that (IMO) full unplug
> / plug of the card is better it uses more standard code paths and is
> less likely to break.  ;-)
>
> Tested-by: Douglas Anderson <dianders-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>

Thanks, I add this to the series and make a re-spin.

What do you think about tagging the patches for stable?

I guess there is a risk that we may "break" the other two users of
mmc_hw_reset(). But, as I said, in that case those needs to be fixed
anyways.

Kind regards
Uffe

Re: [PATCH v2 0/3] mmc: Fixup HW reset for SDIO cards

[Doug Anderson]

Hi,

On Tue, Nov 12, 2019 at 4:28 AM Ulf Hansson <ulf.hansson-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> wrote:
>
> On Tue, 12 Nov 2019 at 01:51, Doug Anderson <dianders-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org> wrote:
> >
> > Hi,
> >
> > On Sat, Nov 9, 2019 at 2:31 AM Ulf Hansson <ulf.hansson-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> wrote:
> > >
> > > Changes in v2:
> > >         - Add adaptations to the mwifiex driver.
> > >         - Keep existing syncronous reset behaviour if the SDIO card has a single
> > >         func driver.
> > >
> > > It has turned out that it's not a good idea to try to power cycle and to
> > > re-initialize the SDIO card, as currently done through mmc_hw_reset(). This
> > > because there may be multiple SDIO funcs attached to the same SDIO card and
> > > some of the others that didn't execute the call to mmc_hw_reset(), may then
> > > simply experience an undefined behaviour.
> > >
> > > The following patches in this series attempts to address this problem, by
> > > reworking the mmc_hw_reset() behaviour for SDIO and by adopting the Marvel
> > > mwifiex driver to these changes.
> > >
> > > Note that, I don't have the HW at hand so the the code has only compile tested.
> > > Test on HW is greatly appreciated!
> > >
> > > Ulf Hansson (3):
> > >   mwifiex: Re-work support for SDIO HW reset
> > >   mmc: core: Drop check for mmc_card_is_removable() in mmc_rescan()
> > >   mmc: core: Re-work HW reset for SDIO cards
> > >
> > >  drivers/mmc/core/core.c                     | 12 +++-----
> > >  drivers/mmc/core/core.h                     |  2 ++
> > >  drivers/mmc/core/sdio.c                     | 28 ++++++++++++++++-
> > >  drivers/mmc/core/sdio_bus.c                 |  9 +++++-
> > >  drivers/net/wireless/marvell/mwifiex/main.c |  6 +++-
> > >  drivers/net/wireless/marvell/mwifiex/main.h |  1 +
> > >  drivers/net/wireless/marvell/mwifiex/sdio.c | 33 ++++++++++++++-------
> > >  include/linux/mmc/card.h                    |  1 +
> > >  8 files changed, 70 insertions(+), 22 deletions(-)
> >
> > I put this on rk3288-veyron-jerry atop v5.4-rc7 and I could run my
> > test case for a while, AKA I got over 50 cycles of:
> >
> > ---
> >
> > for i in $(seq 1000); do
> >   echo "LOOP $i --------"
> >   echo 1 > /sys/kernel/debug/mwifiex/mlan0/reset
> >
> >   while true; do
> >     if ! ping6 -w15 -c1 "${GW}" >/dev/null 2>&1; then
> >       fail=$(( fail + 1 ))
> >       echo "Fail WiFi ${fail}"
> >       if [[ ${fail} == 3 ]]; then
> >         exit 1
> >       fi
> >     else
> >       fail=0
> >       break
> >     fi
> >   done
> >
> >   hciconfig hci0 down
> >   sleep 1
> >   if ! hciconfig hci0 up; then
> >     echo "Fail BT"
> >     exit 1
> >   fi
> >
> > done
> >
> > ---
> >
> > NOTE: with no patches I couldn't even get my test case to pass w/out
> > the BT bits and I swear that used to work before.  ...but I didn't
> > debug since the end result (with full card hotplug) is happy-working
> > for me.  I'll still use it as further argument that (IMO) full unplug
> > / plug of the card is better it uses more standard code paths and is
> > less likely to break.  ;-)
> >
> > Tested-by: Douglas Anderson <dianders-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
>
> Thanks, I add this to the series and make a re-spin.
>
> What do you think about tagging the patches for stable?
>
> I guess there is a risk that we may "break" the other two users of
> mmc_hw_reset(). But, as I said, in that case those needs to be fixed
> anyways.

I'm not sure how to make that judgement call.  Certainly it would help
anyone using the Marvell case and the Marvell case was pretty broken
before.

How about this: if you can get a Tested-by from the other users then
I'd be good with a general CC: stable.  Otherwise, I'd be OK with a CC
to stable for 5.4, but I'd be a little hesitant to send it back to
older kernels (even though it certainly applies and fixes problems).
At least in the case of Chrome OS we already have a workable solution
for our 4.19 tree (my previous patches), and I'd guess anyone testing
on real hardware is either not seeing problems or has their own
private patches already.  If things have been sitting stable on 5.4
for a while and no problems were reported, then we could consider
going back further?

-Doug

Re: [PATCH v2 0/3] mmc: Fixup HW reset for SDIO cards

[Tony Lindgren]

* Ulf Hansson <ulf.hansson-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> [191112 12:24]:
> On Mon, 11 Nov 2019 at 23:08, Tony Lindgren <tony-4v6yS6AI5VpBDgjK7y7TUQ@public.gmane.org> wrote:
> > Looks good to me. I tried testing this, but looks like at least on duovero
> > mwifiex_sdio is broken in v5.4-rc7:
> >
> > # dmesg | grep mwifi
> > mwifiex_sdio mmc1:0001:1: poll card status failed, tries = 100
> > mwifiex_sdio mmc1:0001:1: FW download with helper:       poll status timeout @ 0
> > mwifiex_sdio mmc1:0001:1: prog_fw failed ret=0xffffffff
> > mwifiex_sdio mmc1:0001:1: info: _mwifiex_fw_dpc: unregister device
> >
> > No idea when it broke and what might be the issue this time around.
> > Any ideas?
> 
> Sorry, no good idea.
> 
> Perhaps something on the mmc host level that has changed?

No idea, not even v4.19 is not behaving properly. I guess another
victim of eternal ongoing regressions.

Regards,

Tony

Re: [PATCH v2 1/3] mwifiex: Re-work support for SDIO HW reset

[Doug Anderson]

Hi,

On Tue, Nov 12, 2019 at 4:14 AM Ulf Hansson <ulf.hansson-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> wrote:
>
> > > diff --git a/drivers/net/wireless/marvell/mwifiex/sdio.c b/drivers/net/wireless/marvell/mwifiex/sdio.c
> > > index 24c041dad9f6..2417c94c29c0 100644
> > > --- a/drivers/net/wireless/marvell/mwifiex/sdio.c
> > > +++ b/drivers/net/wireless/marvell/mwifiex/sdio.c
> > > @@ -444,6 +444,9 @@ static int mwifiex_sdio_suspend(struct device *dev)
> > >                 return 0;
> > >         }
> > >
> > > +       if (!adapter->is_adapter_up)
> > > +               return -EBUSY;
> >
> > I'm moderately concerned that there might be cases where firmware
> > never got loaded but we could suspend/resume OK.  ...and now we never
> > will?  I'm not familiar enough with the code to know if this is a real
> > concern, so I guess we can do this and then see...
>
> There is a completion variable that is used to make sure the firmware
> is loaded, before the mwifiex driver runs ->suspend|remove(). This is
> needed, because during ->probe() the FW will be loaded asynchronously,
> hence both mwifiex_sdio_remove() and mwifiex_sdio_suspend(), may be
> called while waiting for the FW to be loaded.
>
> If a HW reset has been scheduled but not completed, which would be the
> case if mmc_hw_reset() gets called after mmc_pm_notify() with a
> PM_SUSPEND_PREPARE. This is because mmc_pm_notify() then disables the
> rescan work, but then re-kicks/enables it at PM_POST_SUSPEND (after
> the system has resumed).
>
> Returning -EBUSY, should allow the mmc rescan work to be completed
> when the system have resumed.
>
> Of course, one could also considering using pm_wakeup_event(), in case
> mmc_hw_reset() needed to schedule the reset, as to prevent the system
> for suspending for a small amount of time. As to make sure the rescan
> work, gets to run. But I am not sure that's needed here.

I was more worried that we could get into a state where we'd return
EBUSY forever, but I think I've convinced myself that this isn't
possible.  If we fail to load things then the adapter variable will be
freed anyway.


> Finally, if you want to verify that the above system suspend path
> works fine, you could change the call to "_mmc_detect_change(host, 0,
> false)" in mmc_sdio_hw_reset(), into "_mmc_detect_change(host,
> msecs_to_jiffies(30000), false)", in patch3.
>
> This should leave you a 30s window of where you can try to system
> suspend the platform, while also waiting for the scheduled reset to be
> completed.

It worked.

https://pastebin.com/NdsvAdE8

Re: [PATCH v2 1/3] mwifiex: Re-work support for SDIO HW reset

[Ulf Hansson]

On Tue, 12 Nov 2019 at 19:05, Doug Anderson <dianders-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org> wrote:
>
> Hi,
>
> On Tue, Nov 12, 2019 at 4:14 AM Ulf Hansson <ulf.hansson-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> wrote:
> >
> > > > diff --git a/drivers/net/wireless/marvell/mwifiex/sdio.c b/drivers/net/wireless/marvell/mwifiex/sdio.c
> > > > index 24c041dad9f6..2417c94c29c0 100644
> > > > --- a/drivers/net/wireless/marvell/mwifiex/sdio.c
> > > > +++ b/drivers/net/wireless/marvell/mwifiex/sdio.c
> > > > @@ -444,6 +444,9 @@ static int mwifiex_sdio_suspend(struct device *dev)
> > > >                 return 0;
> > > >         }
> > > >
> > > > +       if (!adapter->is_adapter_up)
> > > > +               return -EBUSY;
> > >
> > > I'm moderately concerned that there might be cases where firmware
> > > never got loaded but we could suspend/resume OK.  ...and now we never
> > > will?  I'm not familiar enough with the code to know if this is a real
> > > concern, so I guess we can do this and then see...
> >
> > There is a completion variable that is used to make sure the firmware
> > is loaded, before the mwifiex driver runs ->suspend|remove(). This is
> > needed, because during ->probe() the FW will be loaded asynchronously,
> > hence both mwifiex_sdio_remove() and mwifiex_sdio_suspend(), may be
> > called while waiting for the FW to be loaded.
> >
> > If a HW reset has been scheduled but not completed, which would be the
> > case if mmc_hw_reset() gets called after mmc_pm_notify() with a
> > PM_SUSPEND_PREPARE. This is because mmc_pm_notify() then disables the
> > rescan work, but then re-kicks/enables it at PM_POST_SUSPEND (after
> > the system has resumed).
> >
> > Returning -EBUSY, should allow the mmc rescan work to be completed
> > when the system have resumed.
> >
> > Of course, one could also considering using pm_wakeup_event(), in case
> > mmc_hw_reset() needed to schedule the reset, as to prevent the system
> > for suspending for a small amount of time. As to make sure the rescan
> > work, gets to run. But I am not sure that's needed here.
>
> I was more worried that we could get into a state where we'd return
> EBUSY forever, but I think I've convinced myself that this isn't
> possible.  If we fail to load things then the adapter variable will be
> freed anyway.
>
>
> > Finally, if you want to verify that the above system suspend path
> > works fine, you could change the call to "_mmc_detect_change(host, 0,
> > false)" in mmc_sdio_hw_reset(), into "_mmc_detect_change(host,
> > msecs_to_jiffies(30000), false)", in patch3.
> >
> > This should leave you a 30s window of where you can try to system
> > suspend the platform, while also waiting for the scheduled reset to be
> > completed.
>
> It worked.
>
> https://pastebin.com/NdsvAdE8

Great, thanks for confirming!

Kind regards
Uffe

Re: [PATCH v2 0/3] mmc: Fixup HW reset for SDIO cards

[Ulf Hansson]

On Tue, 12 Nov 2019 at 18:43, Doug Anderson <dianders-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org> wrote:
>
> Hi,
>
> On Tue, Nov 12, 2019 at 4:28 AM Ulf Hansson <ulf.hansson-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> wrote:
> >
> > On Tue, 12 Nov 2019 at 01:51, Doug Anderson <dianders-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org> wrote:
> > >
> > > Hi,
> > >
> > > On Sat, Nov 9, 2019 at 2:31 AM Ulf Hansson <ulf.hansson-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> wrote:
> > > >
> > > > Changes in v2:
> > > >         - Add adaptations to the mwifiex driver.
> > > >         - Keep existing syncronous reset behaviour if the SDIO card has a single
> > > >         func driver.
> > > >
> > > > It has turned out that it's not a good idea to try to power cycle and to
> > > > re-initialize the SDIO card, as currently done through mmc_hw_reset(). This
> > > > because there may be multiple SDIO funcs attached to the same SDIO card and
> > > > some of the others that didn't execute the call to mmc_hw_reset(), may then
> > > > simply experience an undefined behaviour.
> > > >
> > > > The following patches in this series attempts to address this problem, by
> > > > reworking the mmc_hw_reset() behaviour for SDIO and by adopting the Marvel
> > > > mwifiex driver to these changes.
> > > >
> > > > Note that, I don't have the HW at hand so the the code has only compile tested.
> > > > Test on HW is greatly appreciated!
> > > >
> > > > Ulf Hansson (3):
> > > >   mwifiex: Re-work support for SDIO HW reset
> > > >   mmc: core: Drop check for mmc_card_is_removable() in mmc_rescan()
> > > >   mmc: core: Re-work HW reset for SDIO cards
> > > >
> > > >  drivers/mmc/core/core.c                     | 12 +++-----
> > > >  drivers/mmc/core/core.h                     |  2 ++
> > > >  drivers/mmc/core/sdio.c                     | 28 ++++++++++++++++-
> > > >  drivers/mmc/core/sdio_bus.c                 |  9 +++++-
> > > >  drivers/net/wireless/marvell/mwifiex/main.c |  6 +++-
> > > >  drivers/net/wireless/marvell/mwifiex/main.h |  1 +
> > > >  drivers/net/wireless/marvell/mwifiex/sdio.c | 33 ++++++++++++++-------
> > > >  include/linux/mmc/card.h                    |  1 +
> > > >  8 files changed, 70 insertions(+), 22 deletions(-)
> > >
> > > I put this on rk3288-veyron-jerry atop v5.4-rc7 and I could run my
> > > test case for a while, AKA I got over 50 cycles of:
> > >
> > > ---
> > >
> > > for i in $(seq 1000); do
> > >   echo "LOOP $i --------"
> > >   echo 1 > /sys/kernel/debug/mwifiex/mlan0/reset
> > >
> > >   while true; do
> > >     if ! ping6 -w15 -c1 "${GW}" >/dev/null 2>&1; then
> > >       fail=$(( fail + 1 ))
> > >       echo "Fail WiFi ${fail}"
> > >       if [[ ${fail} == 3 ]]; then
> > >         exit 1
> > >       fi
> > >     else
> > >       fail=0
> > >       break
> > >     fi
> > >   done
> > >
> > >   hciconfig hci0 down
> > >   sleep 1
> > >   if ! hciconfig hci0 up; then
> > >     echo "Fail BT"
> > >     exit 1
> > >   fi
> > >
> > > done
> > >
> > > ---
> > >
> > > NOTE: with no patches I couldn't even get my test case to pass w/out
> > > the BT bits and I swear that used to work before.  ...but I didn't
> > > debug since the end result (with full card hotplug) is happy-working
> > > for me.  I'll still use it as further argument that (IMO) full unplug
> > > / plug of the card is better it uses more standard code paths and is
> > > less likely to break.  ;-)
> > >
> > > Tested-by: Douglas Anderson <dianders-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
> >
> > Thanks, I add this to the series and make a re-spin.
> >
> > What do you think about tagging the patches for stable?
> >
> > I guess there is a risk that we may "break" the other two users of
> > mmc_hw_reset(). But, as I said, in that case those needs to be fixed
> > anyways.
>
> I'm not sure how to make that judgement call.  Certainly it would help
> anyone using the Marvell case and the Marvell case was pretty broken
> before.
>
> How about this: if you can get a Tested-by from the other users then
> I'd be good with a general CC: stable.  Otherwise, I'd be OK with a CC
> to stable for 5.4, but I'd be a little hesitant to send it back to
> older kernels (even though it certainly applies and fixes problems).

This makes sense, let's go with this!

> At least in the case of Chrome OS we already have a workable solution
> for our 4.19 tree (my previous patches), and I'd guess anyone testing
> on real hardware is either not seeing problems or has their own
> private patches already.  If things have been sitting stable on 5.4
> for a while and no problems were reported, then we could consider
> going back further?

Yep!

That said, the v3 series is queued up for next and by adding a stable
tag for 5.4+.

Kind regards
Uffe

Re: [PATCH v2 3/3] mmc: core: Re-work HW reset for SDIO cards

[Kalle Valo]

+ wen, ath10k

Ulf Hansson <ulf.hansson-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> writes:

> On Tue, 12 Nov 2019 at 01:33, Doug Anderson <dianders-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org> wrote:
>>
>> Hi,
>>
>> On Sat, Nov 9, 2019 at 2:31 AM Ulf Hansson <ulf.hansson-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> wrote:
>> >
>> > diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
>> > index 6f8342702c73..abf8f5eb0a1c 100644
>> > --- a/drivers/mmc/core/core.c
>> > +++ b/drivers/mmc/core/core.c
>> > @@ -1469,8 +1469,7 @@ void mmc_detach_bus(struct mmc_host *host)
>> >         mmc_bus_put(host);
>> >  }
>> >
>> > -static void _mmc_detect_change(struct mmc_host *host, unsigned long delay,
>> > -                               bool cd_irq)
>> > +void _mmc_detect_change(struct mmc_host *host, unsigned long delay, bool cd_irq)
>> >  {
>> >         /*
>> >          * If the device is configured as wakeup, we prevent a new sleep for
>> > @@ -2129,7 +2128,7 @@ int mmc_hw_reset(struct mmc_host *host)
>> >         ret = host->bus_ops->hw_reset(host);
>> >         mmc_bus_put(host);
>> >
>> > -       if (ret)
>> > +       if (ret < 0)
>> >                 pr_warn("%s: tried to HW reset card, got error %d\n",
>> >                         mmc_hostname(host), ret);
>>
>> Other callers besides marvell need to be updated?  In theory only SDIO
>> should have positive return values so I guess we don't care about the
>> caller in drivers/mmc/core/block.c, right?
>
> Correct, but maybe I should add some more information about that in a
> function header of mmc_hw_reset(). Let me consider doing that as a
> change on top.
>
>>  What about:
>>
>> drivers/net/wireless/ath/ath10k/sdio.c
>>
>> ...I guess I don't know if there is more than one function probed
>> there.  Maybe there's not and thus we're fine here too?
>
> Well, honestly I don't know.
>
> In any case, that would mean the driver is broken anyways and needs to
> be fixed. At least that's my approach to doing this change.

Wen, does QCA6174 or QCA9377 SDIO devices have other SDIO functions, for
example bluetooth? I'm just wondering how should we handle this in
ath10k.

-- 
Kalle Valo

Re: [PATCH v2 3/3] mmc: core: Re-work HW reset for SDIO cards

[Kalle Valo]

+ wen, ath10k

Ulf Hansson <ulf.hansson@linaro.org> writes:

> On Tue, 12 Nov 2019 at 01:33, Doug Anderson <dianders@chromium.org> wrote:
>>
>> Hi,
>>
>> On Sat, Nov 9, 2019 at 2:31 AM Ulf Hansson <ulf.hansson@linaro.org> wrote:
>> >
>> > diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
>> > index 6f8342702c73..abf8f5eb0a1c 100644
>> > --- a/drivers/mmc/core/core.c
>> > +++ b/drivers/mmc/core/core.c
>> > @@ -1469,8 +1469,7 @@ void mmc_detach_bus(struct mmc_host *host)
>> >         mmc_bus_put(host);
>> >  }
>> >
>> > -static void _mmc_detect_change(struct mmc_host *host, unsigned long delay,
>> > -                               bool cd_irq)
>> > +void _mmc_detect_change(struct mmc_host *host, unsigned long delay, bool cd_irq)
>> >  {
>> >         /*
>> >          * If the device is configured as wakeup, we prevent a new sleep for
>> > @@ -2129,7 +2128,7 @@ int mmc_hw_reset(struct mmc_host *host)
>> >         ret = host->bus_ops->hw_reset(host);
>> >         mmc_bus_put(host);
>> >
>> > -       if (ret)
>> > +       if (ret < 0)
>> >                 pr_warn("%s: tried to HW reset card, got error %d\n",
>> >                         mmc_hostname(host), ret);
>>
>> Other callers besides marvell need to be updated?  In theory only SDIO
>> should have positive return values so I guess we don't care about the
>> caller in drivers/mmc/core/block.c, right?
>
> Correct, but maybe I should add some more information about that in a
> function header of mmc_hw_reset(). Let me consider doing that as a
> change on top.
>
>>  What about:
>>
>> drivers/net/wireless/ath/ath10k/sdio.c
>>
>> ...I guess I don't know if there is more than one function probed
>> there.  Maybe there's not and thus we're fine here too?
>
> Well, honestly I don't know.
>
> In any case, that would mean the driver is broken anyways and needs to
> be fixed. At least that's my approach to doing this change.

Wen, does QCA6174 or QCA9377 SDIO devices have other SDIO functions, for
example bluetooth? I'm just wondering how should we handle this in
ath10k.

-- 
Kalle Valo

Re: [PATCH v2 3/3] mmc: core: Re-work HW reset for SDIO cards

[wgong-sgV2jX0FEOL9JmXXK+q4OQ]

On 2019-11-20 14:28, Kalle Valo wrote:
> + wen, ath10k
> 
> Ulf Hansson <ulf.hansson-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> writes:
> 
>> On Tue, 12 Nov 2019 at 01:33, Doug Anderson <dianders-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org> 
>> wrote:
>>> 
>>> Hi,
>>> 
>>> On Sat, Nov 9, 2019 at 2:31 AM Ulf Hansson <ulf.hansson-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> 
>>> wrote:
>>> >
>>> > diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
>>> > index 6f8342702c73..abf8f5eb0a1c 100644
>>> > --- a/drivers/mmc/core/core.c
>>> > +++ b/drivers/mmc/core/core.c
>>> > @@ -1469,8 +1469,7 @@ void mmc_detach_bus(struct mmc_host *host)
>>> >         mmc_bus_put(host);
>>> >  }
>>> >
>>> > -static void _mmc_detect_change(struct mmc_host *host, unsigned long delay,
>>> > -                               bool cd_irq)
>>> > +void _mmc_detect_change(struct mmc_host *host, unsigned long delay, bool cd_irq)
>>> >  {
>>> >         /*
>>> >          * If the device is configured as wakeup, we prevent a new sleep for
>>> > @@ -2129,7 +2128,7 @@ int mmc_hw_reset(struct mmc_host *host)
>>> >         ret = host->bus_ops->hw_reset(host);
>>> >         mmc_bus_put(host);
>>> >
>>> > -       if (ret)
>>> > +       if (ret < 0)
>>> >                 pr_warn("%s: tried to HW reset card, got error %d\n",
>>> >                         mmc_hostname(host), ret);
>>> 
>>> Other callers besides marvell need to be updated?  In theory only 
>>> SDIO
>>> should have positive return values so I guess we don't care about the
>>> caller in drivers/mmc/core/block.c, right?
>> 
>> Correct, but maybe I should add some more information about that in a
>> function header of mmc_hw_reset(). Let me consider doing that as a
>> change on top.
>> 
>>>  What about:
>>> 
>>> drivers/net/wireless/ath/ath10k/sdio.c
>>> 
>>> ...I guess I don't know if there is more than one function probed
>>> there.  Maybe there's not and thus we're fine here too?
>> 
>> Well, honestly I don't know.
>> 
>> In any case, that would mean the driver is broken anyways and needs to
>> be fixed. At least that's my approach to doing this change.
> 
> Wen, does QCA6174 or QCA9377 SDIO devices have other SDIO functions, 
> for
> example bluetooth? I'm just wondering how should we handle this in
> ath10k.
Hi Kalle,
it does not have other SDIO functions for QCA6174 or QCA9377.

Re: [PATCH v2 3/3] mmc: core: Re-work HW reset for SDIO cards

[wgong]

On 2019-11-20 14:28, Kalle Valo wrote:
> + wen, ath10k
> 
> Ulf Hansson <ulf.hansson@linaro.org> writes:
> 
>> On Tue, 12 Nov 2019 at 01:33, Doug Anderson <dianders@chromium.org> 
>> wrote:
>>> 
>>> Hi,
>>> 
>>> On Sat, Nov 9, 2019 at 2:31 AM Ulf Hansson <ulf.hansson@linaro.org> 
>>> wrote:
>>> >
>>> > diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
>>> > index 6f8342702c73..abf8f5eb0a1c 100644
>>> > --- a/drivers/mmc/core/core.c
>>> > +++ b/drivers/mmc/core/core.c
>>> > @@ -1469,8 +1469,7 @@ void mmc_detach_bus(struct mmc_host *host)
>>> >         mmc_bus_put(host);
>>> >  }
>>> >
>>> > -static void _mmc_detect_change(struct mmc_host *host, unsigned long delay,
>>> > -                               bool cd_irq)
>>> > +void _mmc_detect_change(struct mmc_host *host, unsigned long delay, bool cd_irq)
>>> >  {
>>> >         /*
>>> >          * If the device is configured as wakeup, we prevent a new sleep for
>>> > @@ -2129,7 +2128,7 @@ int mmc_hw_reset(struct mmc_host *host)
>>> >         ret = host->bus_ops->hw_reset(host);
>>> >         mmc_bus_put(host);
>>> >
>>> > -       if (ret)
>>> > +       if (ret < 0)
>>> >                 pr_warn("%s: tried to HW reset card, got error %d\n",
>>> >                         mmc_hostname(host), ret);
>>> 
>>> Other callers besides marvell need to be updated?  In theory only 
>>> SDIO
>>> should have positive return values so I guess we don't care about the
>>> caller in drivers/mmc/core/block.c, right?
>> 
>> Correct, but maybe I should add some more information about that in a
>> function header of mmc_hw_reset(). Let me consider doing that as a
>> change on top.
>> 
>>>  What about:
>>> 
>>> drivers/net/wireless/ath/ath10k/sdio.c
>>> 
>>> ...I guess I don't know if there is more than one function probed
>>> there.  Maybe there's not and thus we're fine here too?
>> 
>> Well, honestly I don't know.
>> 
>> In any case, that would mean the driver is broken anyways and needs to
>> be fixed. At least that's my approach to doing this change.
> 
> Wen, does QCA6174 or QCA9377 SDIO devices have other SDIO functions, 
> for
> example bluetooth? I'm just wondering how should we handle this in
> ath10k.
Hi Kalle,
it does not have other SDIO functions for QCA6174 or QCA9377.

Re: [PATCH v2 3/3] mmc: core: Re-work HW reset for SDIO cards

[Kalle Valo]

wgong-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org writes:

> On 2019-11-20 14:28, Kalle Valo wrote:
>> + wen, ath10k
>>
>> Ulf Hansson <ulf.hansson-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> writes:
>>
>>> On Tue, 12 Nov 2019 at 01:33, Doug Anderson <dianders-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
>>> wrote:
>>>>
>>>> Hi,
>>>>
>>>> On Sat, Nov 9, 2019 at 2:31 AM Ulf Hansson
>>>> <ulf.hansson-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> wrote:
>>>> >
>>>> > diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
>>>> > index 6f8342702c73..abf8f5eb0a1c 100644
>>>> > --- a/drivers/mmc/core/core.c
>>>> > +++ b/drivers/mmc/core/core.c
>>>> > @@ -1469,8 +1469,7 @@ void mmc_detach_bus(struct mmc_host *host)
>>>> >         mmc_bus_put(host);
>>>> >  }
>>>> >
>>>> > -static void _mmc_detect_change(struct mmc_host *host, unsigned long delay,
>>>> > -                               bool cd_irq)
>>>> > +void _mmc_detect_change(struct mmc_host *host, unsigned long delay, bool cd_irq)
>>>> >  {
>>>> >         /*
>>>> >          * If the device is configured as wakeup, we prevent a new sleep for
>>>> > @@ -2129,7 +2128,7 @@ int mmc_hw_reset(struct mmc_host *host)
>>>> >         ret = host->bus_ops->hw_reset(host);
>>>> >         mmc_bus_put(host);
>>>> >
>>>> > -       if (ret)
>>>> > +       if (ret < 0)
>>>> >                 pr_warn("%s: tried to HW reset card, got error %d\n",
>>>> >                         mmc_hostname(host), ret);
>>>>
>>>> Other callers besides marvell need to be updated?  In theory only
>>>> SDIO
>>>> should have positive return values so I guess we don't care about the
>>>> caller in drivers/mmc/core/block.c, right?
>>>
>>> Correct, but maybe I should add some more information about that in a
>>> function header of mmc_hw_reset(). Let me consider doing that as a
>>> change on top.
>>>
>>>>  What about:
>>>>
>>>> drivers/net/wireless/ath/ath10k/sdio.c
>>>>
>>>> ...I guess I don't know if there is more than one function probed
>>>> there.  Maybe there's not and thus we're fine here too?
>>>
>>> Well, honestly I don't know.
>>>
>>> In any case, that would mean the driver is broken anyways and needs to
>>> be fixed. At least that's my approach to doing this change.
>>
>> Wen, does QCA6174 or QCA9377 SDIO devices have other SDIO functions,
>> for
>> example bluetooth? I'm just wondering how should we handle this in
>> ath10k.
>
> it does not have other SDIO functions for QCA6174 or QCA9377.

Thanks, then I don't think we need to change anything in ath10k.

-- 
Kalle Valo

Re: [PATCH v2 3/3] mmc: core: Re-work HW reset for SDIO cards

[Kalle Valo]

wgong@codeaurora.org writes:

> On 2019-11-20 14:28, Kalle Valo wrote:
>> + wen, ath10k
>>
>> Ulf Hansson <ulf.hansson@linaro.org> writes:
>>
>>> On Tue, 12 Nov 2019 at 01:33, Doug Anderson <dianders@chromium.org>
>>> wrote:
>>>>
>>>> Hi,
>>>>
>>>> On Sat, Nov 9, 2019 at 2:31 AM Ulf Hansson
>>>> <ulf.hansson@linaro.org> wrote:
>>>> >
>>>> > diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
>>>> > index 6f8342702c73..abf8f5eb0a1c 100644
>>>> > --- a/drivers/mmc/core/core.c
>>>> > +++ b/drivers/mmc/core/core.c
>>>> > @@ -1469,8 +1469,7 @@ void mmc_detach_bus(struct mmc_host *host)
>>>> >         mmc_bus_put(host);
>>>> >  }
>>>> >
>>>> > -static void _mmc_detect_change(struct mmc_host *host, unsigned long delay,
>>>> > -                               bool cd_irq)
>>>> > +void _mmc_detect_change(struct mmc_host *host, unsigned long delay, bool cd_irq)
>>>> >  {
>>>> >         /*
>>>> >          * If the device is configured as wakeup, we prevent a new sleep for
>>>> > @@ -2129,7 +2128,7 @@ int mmc_hw_reset(struct mmc_host *host)
>>>> >         ret = host->bus_ops->hw_reset(host);
>>>> >         mmc_bus_put(host);
>>>> >
>>>> > -       if (ret)
>>>> > +       if (ret < 0)
>>>> >                 pr_warn("%s: tried to HW reset card, got error %d\n",
>>>> >                         mmc_hostname(host), ret);
>>>>
>>>> Other callers besides marvell need to be updated?  In theory only
>>>> SDIO
>>>> should have positive return values so I guess we don't care about the
>>>> caller in drivers/mmc/core/block.c, right?
>>>
>>> Correct, but maybe I should add some more information about that in a
>>> function header of mmc_hw_reset(). Let me consider doing that as a
>>> change on top.
>>>
>>>>  What about:
>>>>
>>>> drivers/net/wireless/ath/ath10k/sdio.c
>>>>
>>>> ...I guess I don't know if there is more than one function probed
>>>> there.  Maybe there's not and thus we're fine here too?
>>>
>>> Well, honestly I don't know.
>>>
>>> In any case, that would mean the driver is broken anyways and needs to
>>> be fixed. At least that's my approach to doing this change.
>>
>> Wen, does QCA6174 or QCA9377 SDIO devices have other SDIO functions,
>> for
>> example bluetooth? I'm just wondering how should we handle this in
>> ath10k.
>
> it does not have other SDIO functions for QCA6174 or QCA9377.

Thanks, then I don't think we need to change anything in ath10k.

-- 
Kalle Valo

Re: [PATCH v2 3/3] mmc: core: Re-work HW reset for SDIO cards

[Ulf Hansson]

On Wed, 20 Nov 2019 at 08:20, Kalle Valo <kvalo-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org> wrote:
>
> wgong-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org writes:
>
> > On 2019-11-20 14:28, Kalle Valo wrote:
> >> + wen, ath10k
> >>
> >> Ulf Hansson <ulf.hansson-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> writes:
> >>
> >>> On Tue, 12 Nov 2019 at 01:33, Doug Anderson <dianders-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
> >>> wrote:
> >>>>
> >>>> Hi,
> >>>>
> >>>> On Sat, Nov 9, 2019 at 2:31 AM Ulf Hansson
> >>>> <ulf.hansson-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> wrote:
> >>>> >
> >>>> > diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
> >>>> > index 6f8342702c73..abf8f5eb0a1c 100644
> >>>> > --- a/drivers/mmc/core/core.c
> >>>> > +++ b/drivers/mmc/core/core.c
> >>>> > @@ -1469,8 +1469,7 @@ void mmc_detach_bus(struct mmc_host *host)
> >>>> >         mmc_bus_put(host);
> >>>> >  }
> >>>> >
> >>>> > -static void _mmc_detect_change(struct mmc_host *host, unsigned long delay,
> >>>> > -                               bool cd_irq)
> >>>> > +void _mmc_detect_change(struct mmc_host *host, unsigned long delay, bool cd_irq)
> >>>> >  {
> >>>> >         /*
> >>>> >          * If the device is configured as wakeup, we prevent a new sleep for
> >>>> > @@ -2129,7 +2128,7 @@ int mmc_hw_reset(struct mmc_host *host)
> >>>> >         ret = host->bus_ops->hw_reset(host);
> >>>> >         mmc_bus_put(host);
> >>>> >
> >>>> > -       if (ret)
> >>>> > +       if (ret < 0)
> >>>> >                 pr_warn("%s: tried to HW reset card, got error %d\n",
> >>>> >                         mmc_hostname(host), ret);
> >>>>
> >>>> Other callers besides marvell need to be updated?  In theory only
> >>>> SDIO
> >>>> should have positive return values so I guess we don't care about the
> >>>> caller in drivers/mmc/core/block.c, right?
> >>>
> >>> Correct, but maybe I should add some more information about that in a
> >>> function header of mmc_hw_reset(). Let me consider doing that as a
> >>> change on top.
> >>>
> >>>>  What about:
> >>>>
> >>>> drivers/net/wireless/ath/ath10k/sdio.c
> >>>>
> >>>> ...I guess I don't know if there is more than one function probed
> >>>> there.  Maybe there's not and thus we're fine here too?
> >>>
> >>> Well, honestly I don't know.
> >>>
> >>> In any case, that would mean the driver is broken anyways and needs to
> >>> be fixed. At least that's my approach to doing this change.
> >>
> >> Wen, does QCA6174 or QCA9377 SDIO devices have other SDIO functions,
> >> for
> >> example bluetooth? I'm just wondering how should we handle this in
> >> ath10k.
> >
> > it does not have other SDIO functions for QCA6174 or QCA9377.
>
> Thanks, then I don't think we need to change anything in ath10k.
>
> --
> Kalle Valo

Kalle, Wen - thanks for looking into this and for the confirmation.

One thing though, perhaps it's worth to add this as a comment in the
code for ath10k, where mmc_hw_reset() is called. Just to make it
clear.

Kind regards
Uffe

Re: [PATCH v2 3/3] mmc: core: Re-work HW reset for SDIO cards

[Kalle Valo]

Ulf Hansson <ulf.hansson-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> writes:

> On Wed, 20 Nov 2019 at 08:20, Kalle Valo <kvalo-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org> wrote:
>>
>> wgong-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org writes:
>>
>> > On 2019-11-20 14:28, Kalle Valo wrote:
>> >> + wen, ath10k
>> >>
>> >> Ulf Hansson <ulf.hansson-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> writes:
>> >>
>> >>> On Tue, 12 Nov 2019 at 01:33, Doug Anderson <dianders-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
>> >>> wrote:
>> >>>>
>> >>>> Hi,
>> >>>>
>> >>>> On Sat, Nov 9, 2019 at 2:31 AM Ulf Hansson
>> >>>> <ulf.hansson-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> wrote:
>> >>>> >
>> >>>> > diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
>> >>>> > index 6f8342702c73..abf8f5eb0a1c 100644
>> >>>> > --- a/drivers/mmc/core/core.c
>> >>>> > +++ b/drivers/mmc/core/core.c
>> >>>> > @@ -1469,8 +1469,7 @@ void mmc_detach_bus(struct mmc_host *host)
>> >>>> >         mmc_bus_put(host);
>> >>>> >  }
>> >>>> >
>> >>>> > -static void _mmc_detect_change(struct mmc_host *host, unsigned long delay,
>> >>>> > -                               bool cd_irq)
>> >>>> > +void _mmc_detect_change(struct mmc_host *host, unsigned long delay, bool cd_irq)
>> >>>> >  {
>> >>>> >         /*
>> >>>> >          * If the device is configured as wakeup, we prevent a new sleep for
>> >>>> > @@ -2129,7 +2128,7 @@ int mmc_hw_reset(struct mmc_host *host)
>> >>>> >         ret = host->bus_ops->hw_reset(host);
>> >>>> >         mmc_bus_put(host);
>> >>>> >
>> >>>> > -       if (ret)
>> >>>> > +       if (ret < 0)
>> >>>> >                 pr_warn("%s: tried to HW reset card, got error %d\n",
>> >>>> >                         mmc_hostname(host), ret);
>> >>>>
>> >>>> Other callers besides marvell need to be updated?  In theory only
>> >>>> SDIO
>> >>>> should have positive return values so I guess we don't care about the
>> >>>> caller in drivers/mmc/core/block.c, right?
>> >>>
>> >>> Correct, but maybe I should add some more information about that in a
>> >>> function header of mmc_hw_reset(). Let me consider doing that as a
>> >>> change on top.
>> >>>
>> >>>>  What about:
>> >>>>
>> >>>> drivers/net/wireless/ath/ath10k/sdio.c
>> >>>>
>> >>>> ...I guess I don't know if there is more than one function probed
>> >>>> there.  Maybe there's not and thus we're fine here too?
>> >>>
>> >>> Well, honestly I don't know.
>> >>>
>> >>> In any case, that would mean the driver is broken anyways and needs to
>> >>> be fixed. At least that's my approach to doing this change.
>> >>
>> >> Wen, does QCA6174 or QCA9377 SDIO devices have other SDIO functions,
>> >> for
>> >> example bluetooth? I'm just wondering how should we handle this in
>> >> ath10k.
>> >
>> > it does not have other SDIO functions for QCA6174 or QCA9377.
>>
>> Thanks, then I don't think we need to change anything in ath10k.
>>
>> --
>> Kalle Valo
>
> Kalle, Wen - thanks for looking into this and for the confirmation.
>
> One thing though, perhaps it's worth to add this as a comment in the
> code for ath10k, where mmc_hw_reset() is called. Just to make it
> clear.

Good point. Wen, can you send a patch, please?

-- 
https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches

Re: [PATCH v2 3/3] mmc: core: Re-work HW reset for SDIO cards

[Kalle Valo]

Ulf Hansson <ulf.hansson@linaro.org> writes:

> On Wed, 20 Nov 2019 at 08:20, Kalle Valo <kvalo@codeaurora.org> wrote:
>>
>> wgong@codeaurora.org writes:
>>
>> > On 2019-11-20 14:28, Kalle Valo wrote:
>> >> + wen, ath10k
>> >>
>> >> Ulf Hansson <ulf.hansson@linaro.org> writes:
>> >>
>> >>> On Tue, 12 Nov 2019 at 01:33, Doug Anderson <dianders@chromium.org>
>> >>> wrote:
>> >>>>
>> >>>> Hi,
>> >>>>
>> >>>> On Sat, Nov 9, 2019 at 2:31 AM Ulf Hansson
>> >>>> <ulf.hansson@linaro.org> wrote:
>> >>>> >
>> >>>> > diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
>> >>>> > index 6f8342702c73..abf8f5eb0a1c 100644
>> >>>> > --- a/drivers/mmc/core/core.c
>> >>>> > +++ b/drivers/mmc/core/core.c
>> >>>> > @@ -1469,8 +1469,7 @@ void mmc_detach_bus(struct mmc_host *host)
>> >>>> >         mmc_bus_put(host);
>> >>>> >  }
>> >>>> >
>> >>>> > -static void _mmc_detect_change(struct mmc_host *host, unsigned long delay,
>> >>>> > -                               bool cd_irq)
>> >>>> > +void _mmc_detect_change(struct mmc_host *host, unsigned long delay, bool cd_irq)
>> >>>> >  {
>> >>>> >         /*
>> >>>> >          * If the device is configured as wakeup, we prevent a new sleep for
>> >>>> > @@ -2129,7 +2128,7 @@ int mmc_hw_reset(struct mmc_host *host)
>> >>>> >         ret = host->bus_ops->hw_reset(host);
>> >>>> >         mmc_bus_put(host);
>> >>>> >
>> >>>> > -       if (ret)
>> >>>> > +       if (ret < 0)
>> >>>> >                 pr_warn("%s: tried to HW reset card, got error %d\n",
>> >>>> >                         mmc_hostname(host), ret);
>> >>>>
>> >>>> Other callers besides marvell need to be updated?  In theory only
>> >>>> SDIO
>> >>>> should have positive return values so I guess we don't care about the
>> >>>> caller in drivers/mmc/core/block.c, right?
>> >>>
>> >>> Correct, but maybe I should add some more information about that in a
>> >>> function header of mmc_hw_reset(). Let me consider doing that as a
>> >>> change on top.
>> >>>
>> >>>>  What about:
>> >>>>
>> >>>> drivers/net/wireless/ath/ath10k/sdio.c
>> >>>>
>> >>>> ...I guess I don't know if there is more than one function probed
>> >>>> there.  Maybe there's not and thus we're fine here too?
>> >>>
>> >>> Well, honestly I don't know.
>> >>>
>> >>> In any case, that would mean the driver is broken anyways and needs to
>> >>> be fixed. At least that's my approach to doing this change.
>> >>
>> >> Wen, does QCA6174 or QCA9377 SDIO devices have other SDIO functions,
>> >> for
>> >> example bluetooth? I'm just wondering how should we handle this in
>> >> ath10k.
>> >
>> > it does not have other SDIO functions for QCA6174 or QCA9377.
>>
>> Thanks, then I don't think we need to change anything in ath10k.
>>
>> --
>> Kalle Valo
>
> Kalle, Wen - thanks for looking into this and for the confirmation.
>
> One thing though, perhaps it's worth to add this as a comment in the
> code for ath10k, where mmc_hw_reset() is called. Just to make it
> clear.

Good point. Wen, can you send a patch, please?

-- 
https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches

Re: [PATCH v2 3/3] mmc: core: Re-work HW reset for SDIO cards

[wgong-sgV2jX0FEOL9JmXXK+q4OQ]

On 2019-11-21 00:41, Kalle Valo wrote:
> Ulf Hansson <ulf.hansson-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> writes:
> 
>> On Wed, 20 Nov 2019 at 08:20, Kalle Valo <kvalo-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org> wrote:
>>> 
>>> wgong-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org writes:
>>> 
>>> > On 2019-11-20 14:28, Kalle Valo wrote:
>>> >> + wen, ath10k
>>> >>
>>> >> Ulf Hansson <ulf.hansson-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> writes:
>>> >>
>>> >>> On Tue, 12 Nov 2019 at 01:33, Doug Anderson <dianders-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
>>> >>> wrote:
>>> >>>>
>>> >>>> Hi,
>>> >>>>
>>> >>>> On Sat, Nov 9, 2019 at 2:31 AM Ulf Hansson
>>> >>>> <ulf.hansson-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> wrote:
>>> >>>> >
>>> >>>> > diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
>>> >>>> > index 6f8342702c73..abf8f5eb0a1c 100644
>>> >>>> > --- a/drivers/mmc/core/core.c
>>> >>>> > +++ b/drivers/mmc/core/core.c
>>> >>>> > @@ -1469,8 +1469,7 @@ void mmc_detach_bus(struct mmc_host *host)
>>> >>>> >         mmc_bus_put(host);
>>> >>>> >  }
>>> >>>> >
>>> >>>> > -static void _mmc_detect_change(struct mmc_host *host, unsigned long delay,
>>> >>>> > -                               bool cd_irq)
>>> >>>> > +void _mmc_detect_change(struct mmc_host *host, unsigned long delay, bool cd_irq)
>>> >>>> >  {
>>> >>>> >         /*
>>> >>>> >          * If the device is configured as wakeup, we prevent a new sleep for
>>> >>>> > @@ -2129,7 +2128,7 @@ int mmc_hw_reset(struct mmc_host *host)
>>> >>>> >         ret = host->bus_ops->hw_reset(host);
>>> >>>> >         mmc_bus_put(host);
>>> >>>> >
>>> >>>> > -       if (ret)
>>> >>>> > +       if (ret < 0)
>>> >>>> >                 pr_warn("%s: tried to HW reset card, got error %d\n",
>>> >>>> >                         mmc_hostname(host), ret);
>>> >>>>
>>> >>>> Other callers besides marvell need to be updated?  In theory only
>>> >>>> SDIO
>>> >>>> should have positive return values so I guess we don't care about the
>>> >>>> caller in drivers/mmc/core/block.c, right?
>>> >>>
>>> >>> Correct, but maybe I should add some more information about that in a
>>> >>> function header of mmc_hw_reset(). Let me consider doing that as a
>>> >>> change on top.
>>> >>>
>>> >>>>  What about:
>>> >>>>
>>> >>>> drivers/net/wireless/ath/ath10k/sdio.c
>>> >>>>
>>> >>>> ...I guess I don't know if there is more than one function probed
>>> >>>> there.  Maybe there's not and thus we're fine here too?
>>> >>>
>>> >>> Well, honestly I don't know.
>>> >>>
>>> >>> In any case, that would mean the driver is broken anyways and needs to
>>> >>> be fixed. At least that's my approach to doing this change.
>>> >>
>>> >> Wen, does QCA6174 or QCA9377 SDIO devices have other SDIO functions,
>>> >> for
>>> >> example bluetooth? I'm just wondering how should we handle this in
>>> >> ath10k.
>>> >
>>> > it does not have other SDIO functions for QCA6174 or QCA9377.
>>> 
>>> Thanks, then I don't think we need to change anything in ath10k.
>>> 
>>> --
>>> Kalle Valo
>> 
>> Kalle, Wen - thanks for looking into this and for the confirmation.
>> 
>> One thing though, perhaps it's worth to add this as a comment in the
>> code for ath10k, where mmc_hw_reset() is called. Just to make it
>> clear.
> 
> Good point. Wen, can you send a patch, please?
Kalle, sure, I will send the patch.

Re: [PATCH v2 3/3] mmc: core: Re-work HW reset for SDIO cards

[wgong]

On 2019-11-21 00:41, Kalle Valo wrote:
> Ulf Hansson <ulf.hansson@linaro.org> writes:
> 
>> On Wed, 20 Nov 2019 at 08:20, Kalle Valo <kvalo@codeaurora.org> wrote:
>>> 
>>> wgong@codeaurora.org writes:
>>> 
>>> > On 2019-11-20 14:28, Kalle Valo wrote:
>>> >> + wen, ath10k
>>> >>
>>> >> Ulf Hansson <ulf.hansson@linaro.org> writes:
>>> >>
>>> >>> On Tue, 12 Nov 2019 at 01:33, Doug Anderson <dianders@chromium.org>
>>> >>> wrote:
>>> >>>>
>>> >>>> Hi,
>>> >>>>
>>> >>>> On Sat, Nov 9, 2019 at 2:31 AM Ulf Hansson
>>> >>>> <ulf.hansson@linaro.org> wrote:
>>> >>>> >
>>> >>>> > diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
>>> >>>> > index 6f8342702c73..abf8f5eb0a1c 100644
>>> >>>> > --- a/drivers/mmc/core/core.c
>>> >>>> > +++ b/drivers/mmc/core/core.c
>>> >>>> > @@ -1469,8 +1469,7 @@ void mmc_detach_bus(struct mmc_host *host)
>>> >>>> >         mmc_bus_put(host);
>>> >>>> >  }
>>> >>>> >
>>> >>>> > -static void _mmc_detect_change(struct mmc_host *host, unsigned long delay,
>>> >>>> > -                               bool cd_irq)
>>> >>>> > +void _mmc_detect_change(struct mmc_host *host, unsigned long delay, bool cd_irq)
>>> >>>> >  {
>>> >>>> >         /*
>>> >>>> >          * If the device is configured as wakeup, we prevent a new sleep for
>>> >>>> > @@ -2129,7 +2128,7 @@ int mmc_hw_reset(struct mmc_host *host)
>>> >>>> >         ret = host->bus_ops->hw_reset(host);
>>> >>>> >         mmc_bus_put(host);
>>> >>>> >
>>> >>>> > -       if (ret)
>>> >>>> > +       if (ret < 0)
>>> >>>> >                 pr_warn("%s: tried to HW reset card, got error %d\n",
>>> >>>> >                         mmc_hostname(host), ret);
>>> >>>>
>>> >>>> Other callers besides marvell need to be updated?  In theory only
>>> >>>> SDIO
>>> >>>> should have positive return values so I guess we don't care about the
>>> >>>> caller in drivers/mmc/core/block.c, right?
>>> >>>
>>> >>> Correct, but maybe I should add some more information about that in a
>>> >>> function header of mmc_hw_reset(). Let me consider doing that as a
>>> >>> change on top.
>>> >>>
>>> >>>>  What about:
>>> >>>>
>>> >>>> drivers/net/wireless/ath/ath10k/sdio.c
>>> >>>>
>>> >>>> ...I guess I don't know if there is more than one function probed
>>> >>>> there.  Maybe there's not and thus we're fine here too?
>>> >>>
>>> >>> Well, honestly I don't know.
>>> >>>
>>> >>> In any case, that would mean the driver is broken anyways and needs to
>>> >>> be fixed. At least that's my approach to doing this change.
>>> >>
>>> >> Wen, does QCA6174 or QCA9377 SDIO devices have other SDIO functions,
>>> >> for
>>> >> example bluetooth? I'm just wondering how should we handle this in
>>> >> ath10k.
>>> >
>>> > it does not have other SDIO functions for QCA6174 or QCA9377.
>>> 
>>> Thanks, then I don't think we need to change anything in ath10k.
>>> 
>>> --
>>> Kalle Valo
>> 
>> Kalle, Wen - thanks for looking into this and for the confirmation.
>> 
>> One thing though, perhaps it's worth to add this as a comment in the
>> code for ath10k, where mmc_hw_reset() is called. Just to make it
>> clear.
> 
> Good point. Wen, can you send a patch, please?
Kalle, sure, I will send the patch.