Received-SPF: pass (google.com: best guess record for domain of linux-kernel-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> <CAD=FV=XT=NTyPag9wNCotATBzT4v9pg=OOa8X6=xWkMH2AFiLQ@mail.gmail.com>
In-Reply-To: <CAD=FV=XT=NTyPag9wNCotATBzT4v9pg=OOa8X6=xWkMH2AFiLQ@mail.gmail.com>
From:   Abhishek Pandit-Subedi <abhishekpandit@chromium.org>
Date:   Fri, 20 Mar 2020 13:56:23 -0700
Message-ID: <CANFp7mUKRRQT0m9jRB4aeE3GeSW8UM6d-NVJ3CZmHibhSny3+g@mail.gmail.com>
Subject: Re: [PATCH 1/1] Bluetooth: btmrvl: Detect hangs and force a reset of
 the SDIO card
To:     Doug Anderson <dianders@chromium.org>
Cc:     Marcel Holtmann <marcel@holtmann.org>,
        BlueZ <linux-bluetooth@vger.kernel.org>,
        Ulf Hansson <ulf.hansson@linaro.org>,
        ChromeOS Bluetooth Upstreaming 
        <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-kernel-owner@vger.kernel.org
Precedence: bulk

Thanks for the heads up Doug. I'll resend the patch once I handle the
case where the reset is immediate and not a full unplug/replug.

In the meantime, please do not merge this patch.

Thanks
Abhishek

On Fri, Mar 20, 2020 at 1:00 PM Doug Anderson <dianders@chromium.org> wrote:
>
> 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