Received-SPF: pass (google.com: domain of linux-bluetooth-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <20201222124651.101063-1-marcel@holtmann.org> <CANFp7mV8RpF8gkrEbyA8ZOP1YH5CH0xkJeyPcaetROsqHiZWWg@mail.gmail.com>
 <CABBYNZJ7ednKQfBG+Ky+39+d2qO3YvXGn7TTvm-3SZ-uSNcsfA@mail.gmail.com> <CANFp7mVYx3oBLnq=oj2gODcK6Wm_bq8Ykv2MD6DnqvS4gdOu9Q@mail.gmail.com>
In-Reply-To: <CANFp7mVYx3oBLnq=oj2gODcK6Wm_bq8Ykv2MD6DnqvS4gdOu9Q@mail.gmail.com>
From:   Luiz Augusto von Dentz <luiz.dentz@gmail.com>
Date:   Tue, 12 Jan 2021 11:37:40 -0800
Message-ID: <CABBYNZLVN7GV5fMb05pgosAEfHPur-EPrvggffUikaKd4hOO5Q@mail.gmail.com>
Subject: Re: [PATCH] doc/mgmt-api.txt: Introduce Set Runtime Firmware command
To:     Abhishek Pandit-Subedi <abhishekpandit@chromium.org>
Cc:     Marcel Holtmann <marcel@holtmann.org>,
        Bluez mailing list <linux-bluetooth@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

Hi Abhishek,

On Tue, Jan 12, 2021 at 10:03 AM Abhishek Pandit-Subedi
<abhishekpandit@chromium.org> wrote:
>
> Hi Luiz,
>
> On Mon, Jan 11, 2021 at 1:55 PM Luiz Augusto von Dentz
> <luiz.dentz@gmail.com> wrote:
> >
> > Hi Abhishek,
> >
> > On Mon, Jan 11, 2021 at 11:38 AM Abhishek Pandit-Subedi
> > <abhishekpandit@chromium.org> wrote:
> > >
> > > Hi Marcel,
> > >
> > > I don't think this solves the original problem we were talking about:
> > > the driver should replace the runtime firmware on reload if it doesn't
> > > match what's on disk.
> > >
> > > Some background for the mailing list:
> > > - On a ChromeOS laptop, we discovered that the Bluetooth controller
> > > wasn't being fully powered down in some reboots. As a result, a new
> > > firmware wasn't being applied after an update.
> > > - The kernel driver was checking if the bluetooth controller had
> > > loaded some firmware already. If it was in bootloader mode, it would
> > > download new firmware. If it was not, it would skip downloading new
> > > firmware.
> > >
> > > The useful part of this mgmt command is to force the driver to reset
> > > to bootloader (Action = 0 in Set Runtime Firmware). However, without
> > > being able to compare the firmware version loaded on the controller,
> > > there's no clear signal for when this should be called. Loading the
> > > firmware through mgmt may be useful for debugging but you could also
> > > just replace the firmware on disk and "reset to bootloader" to achieve
> > > the same effect. I would actually expect unloading and reloading the
> > > module should do that.
> > >
> > > Also, moving the firmware loading from the driver to the userspace
> > > seems odd to me. Since the comparison is between the controller
> > > firmware and disk firmware, there's not much extra that the userspace
> > > knows that the kernel does not.
> >
> > My last suggestion was just to have a MGMT command suggesting the
> > kernel to load the firmware from a different location, this could be
> > useful for testing purpose so one can set for example an old/beta
> > firmware to compare for regressions or test new features that
> > otherwise would not be available. That said perhaps we don't actually
> > need a new MGMT command for doing this and just by replacing the
> > current file would trigger a reload but that may get tricky when if
> > the location does get unmonted/remounted etc.
> >
> > >
> > > ----
> > >
> > > Coming back to the original problem of when to reload runtime
> > > firmware, here are the conditions under which we do and don't want a
> > > reload.
> > >
> > > Do want a reload:
> > > - Reboot
> > > - Module is unloaded and reloaded
> > >
> > > Don't want a reload:
> > > - Transport disconnection (i.e. usb disconnect; some laptops will
> > > power down USB during suspend to save additional power but BT will
> > > stay powered up)
> >
> > Well if the device disappears from the host I'm not really sure how
> > you will be able to detect that the firmware was retained, that said
> > when the adapter is power up again it should be possible to query it
> > what firmware it is currently running and then compare with the one
> > from file before attempting to load it, this should also work
> > regardless of the underlying transport/bus so it would work regardless
> > of the driver in use.
>
> Generally the device drivers detect whether it's in bootloader mode or
> runtime firmware is uploaded. With the runtime firmware, it can also
> query what version is running. However, there's currently no way to
> read the version on disk because the firmware version isn't stored at
> some known location in the firmware blob. That's the crux of the
> problem here.

Hmm I believe we can extract the build number of the firmware, I mean
it is obviously in there and we could possibly start encoding its
version/build as part of the filename as well not just the model.

https://git.kernel.org/pub/scm/linux/kernel/git/bluetooth/bluetooth-next.git/tree/drivers/bluetooth/btintel.c#n226
https://git.kernel.org/pub/scm/linux/kernel/git/bluetooth/bluetooth-next.git/tree/drivers/bluetooth/btusb.c#n2723

So we just have to figure out where the build number is in the
firmware file, so store it somewhere else perhaps using some metadata
perhaps.