Re: [PATCH 1/2] mmc: core: Add sdio_trigger_replug() API

[Matthias Kaehlcke <mka@chromium.org>]

On Tue, Jul 16, 2019 at 09:42:08AM -0700, Douglas Anderson wrote:
> When using Marvell WiFi SDIO cards, it is not uncommon for Linux WiFi
> driver to fully lose the communication channel to the firmware running
> on the card.  Presumably the firmware on the card has a bug or two in
> it and occasionally crashes.
> 
> The Marvell WiFi driver attempts to recover from this problem.
> Specifically the driver has the function mwifiex_sdio_card_reset()
> which is called when communcation problems are found.  That function
> attempts to reset the state of things by utilizing the mmc_hw_reset()
> function.
> 
> The current solution is a bit complex because the Marvell WiFi driver
> needs to manually deinit and reinit the WiFi driver around the reset
> call.  This means it's going through a bunch of code paths that aren't
> normally tested.  However, complexity isn't our only problem.  The
> other (bigger) problem is that Marvell WiFi cards are often combo
> WiFi/Bluetooth cards and Bluetooth runs on a second SDIO func.  While
> the WiFi driver knows that it should re-init its own state around the
> mmc_hw_reset() call there is no good way to inform the Bluetooth
> driver.  That means that in Linux today when you reset the Marvell
> WiFi driver you lose all Bluetooth communication.  Doh!
> 
> One way to fix the above problems is to leverage a more standard way
> to reset the Marvell WiFi card where we go through the same code paths
> as card unplug and the card plug.  In this patch we introduce a new
> API call for doing just that: sdio_trigger_replug().  This API call
> will trigger an unplug of the SDIO card followed by a plug of the
> card.  As part of this the card will be nicely reset.
> 
> Signed-off-by: Douglas Anderson <dianders@chromium.org>
> ---
> 
>  drivers/mmc/core/core.c       | 28 ++++++++++++++++++++++++++--
>  drivers/mmc/core/sdio_io.c    | 20 ++++++++++++++++++++
>  include/linux/mmc/host.h      | 15 ++++++++++++++-
>  include/linux/mmc/sdio_func.h |  2 ++
>  4 files changed, 62 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
> index 9020cb2490f7..48a7d23aed26 100644
> --- a/drivers/mmc/core/core.c
> +++ b/drivers/mmc/core/core.c
> @@ -2164,6 +2164,12 @@ int mmc_sw_reset(struct mmc_host *host)
>  }
>  EXPORT_SYMBOL(mmc_sw_reset);
>  
> +void mmc_trigger_replug(struct mmc_host *host)
> +{
> +	host->trigger_replug_state = MMC_REPLUG_STATE_UNPLUG;
> +	_mmc_detect_change(host, 0, false);
> +}
> +
>  static int mmc_rescan_try_freq(struct mmc_host *host, unsigned freq)
>  {
>  	host->f_init = freq;
> @@ -2217,6 +2223,11 @@ int _mmc_detect_card_removed(struct mmc_host *host)
>  	if (!host->card || mmc_card_removed(host->card))
>  		return 1;
>  
> +	if (host->trigger_replug_state == MMC_REPLUG_STATE_UNPLUG) {
> +		mmc_card_set_removed(host->card);
> +		return 1;
> +	}
> +
>  	ret = host->bus_ops->alive(host);
>  
>  	/*
> @@ -2329,8 +2340,21 @@ void mmc_rescan(struct work_struct *work)
>  	mmc_bus_put(host);
>  
>  	mmc_claim_host(host);
> -	if (mmc_card_is_removable(host) && host->ops->get_cd &&
> -			host->ops->get_cd(host) == 0) {
> +
> +	/*
> +	 * Move through the state machine if we're triggering an unplug
> +	 * followed by a re-plug.
> +	 */
> +	if (host->trigger_replug_state == MMC_REPLUG_STATE_UNPLUG) {
> +		host->trigger_replug_state = MMC_REPLUG_STATE_PLUG;
> +		_mmc_detect_change(host, 0, false);
> +	} else if (host->trigger_replug_state == MMC_REPLUG_STATE_PLUG) {
> +		host->trigger_replug_state = MMC_REPLUG_STATE_NONE;
> +	}
> +
> +	if (host->trigger_replug_state == MMC_REPLUG_STATE_PLUG ||
> +	    (mmc_card_is_removable(host) && host->ops->get_cd &&
> +			host->ops->get_cd(host) == 0)) {

at first I was concerned there could be race conditions with the
different invocations of mmc_rescan(), but IIUC all calls are through
the host->detect work, so only one instance should be running at any
time.

>  		mmc_power_off(host);
>  		mmc_release_host(host);
>  		goto out;
> diff --git a/drivers/mmc/core/sdio_io.c b/drivers/mmc/core/sdio_io.c
> index 2ba00acf64e6..1c5c2a3ebe5e 100644
> --- a/drivers/mmc/core/sdio_io.c
> +++ b/drivers/mmc/core/sdio_io.c
> @@ -811,3 +811,23 @@ void sdio_retune_release(struct sdio_func *func)
>  	mmc_retune_release(func->card->host);
>  }
>  EXPORT_SYMBOL_GPL(sdio_retune_release);
> +
> +/**
> + *	sdio_trigger_replug - trigger an "unplug" + "plug" of the card
> + *	@func: SDIO function attached to host
> + *
> + *	When you call this function we will schedule events that will
> + *	make it look like the card contining the given SDIO func was

nit: containing

> + *	unplugged and then re-plugged-in.  This is as close as possible
> + *	to a full reset of the card that can be achieved.
> + *
> + *	NOTE: routnine will temporarily make the card look as if it is

nit: routine

Other than the typos this looks sane to me, I don't claim to have a
deep understanding of the MMC codebase though.

Reviewed-by: Matthias Kaehlcke <mka@chromium.org>