Received-SPF: pass (google.com: best guess record for domain of linux-bluetooth-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
MIME-Version: 1.0
References: <20200320020153.98280-1-abhishekpandit@chromium.org> <20200319190144.1.I40cc9b3d5de04f0631c931d94757fb0f462b24bd@changeid>
In-Reply-To: <20200319190144.1.I40cc9b3d5de04f0631c931d94757fb0f462b24bd@changeid>
From:   Doug Anderson <dianders@chromium.org>
Date:   Fri, 20 Mar 2020 13:00:14 -0700
Message-ID: <CAD=FV=XT=NTyPag9wNCotATBzT4v9pg=OOa8X6=xWkMH2AFiLQ@mail.gmail.com>
Subject: Re: [PATCH 1/1] Bluetooth: btmrvl: Detect hangs and force a reset of
 the SDIO card
To:     Abhishek Pandit-Subedi <abhishekpandit@chromium.org>
Cc:     Marcel Holtmann <marcel@holtmann.org>,
        BlueZ <linux-bluetooth@vger.kernel.org>,
        Ulf Hansson <ulf.hansson@linaro.org>,
        chromeos-bluetooth-upstreaming@chromium.org,
        Matthias Kaehlcke <mka@chromium.org>,
        Johan Hedberg <johan.hedberg@gmail.com>,
        LKML <linux-kernel@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Sender: linux-bluetooth-owner@vger.kernel.org
Precedence: bulk

Hi,

On Thu, Mar 19, 2020 at 7:02 PM Abhishek Pandit-Subedi
<abhishekpandit@chromium.org> wrote:
>
> From: Matthias Kaehlcke <mka@chromium.org>
>
> When scanning for BLE devices for a longer period (e.g. because a
> BLE device is paired, but not connected) the Marvell 8997 often
> ends up in a borked state, which manifests through failures on
> certain SDIO transactions.
>
> When such a SDIO failure is detected force a reset of the SDIO
> card to initialize it from scratch. Since the SDIO bus is shared
> with the WiFi part of the chip this also involves a reset of WiFi.
>
> Signed-off-by: Matthias Kaehlcke <mka@chromium.org>
> Signed-off-by: Abhishek Pandit-Subedi <abhishekpandit@chromium.org>
> ---
>
>  drivers/bluetooth/btmrvl_sdio.c | 24 ++++++++++++++++++++++++
>  drivers/bluetooth/btmrvl_sdio.h |  1 +
>  2 files changed, 25 insertions(+)
>
> diff --git a/drivers/bluetooth/btmrvl_sdio.c b/drivers/bluetooth/btmrvl_sdio.c
> index 0f3a020703ab..69a8b6b3c11c 100644
> --- a/drivers/bluetooth/btmrvl_sdio.c
> +++ b/drivers/bluetooth/btmrvl_sdio.c
> @@ -22,6 +22,8 @@
>  #include <linux/slab.h>
>  #include <linux/suspend.h>
>
> +#include <linux/mmc/core.h>
> +#include <linux/mmc/card.h>
>  #include <linux/mmc/sdio_ids.h>
>  #include <linux/mmc/sdio_func.h>
>  #include <linux/module.h>
> @@ -59,6 +61,23 @@ static const struct of_device_id btmrvl_sdio_of_match_table[] = {
>         { }
>  };
>
> +static void btmrvl_sdio_card_reset_work(struct work_struct *work)
> +{
> +       struct btmrvl_sdio_card *card =
> +               container_of(work, struct btmrvl_sdio_card, reset_work);
> +       struct sdio_func *func = card->func;
> +
> +       sdio_claim_host(func);
> +       mmc_hw_reset(func->card->host);

The fact that you don't check the return value here seems like a
problem.  See specifically how commit cdb2256f795e ("mwifiex: Re-work
support for SDIO HW reset") handles it.

This is a distinct difference between the solution that we landed in
Chrome OS 4.19 and what landed upstream.  In Chrome OS 4.19 we went
the simple approach and said that there was only one way to reset the
card: treat it as a full unplug / replug of the card.  ...but upstream
adopted a different solution.  For upstream if there is only a single
function on the SD card it will not trigger a full unplug/replug and
it's up to the function driver to re-init itself.  This wasn't such a
big deal for the WiFi driver since it already had a way to re-init
itself (mwifiex_reinit_sw).  I don't know how hard it will be for
Bluetooth, but that needs to be part of this patch I think?

-Doug