Re: [PATCH v1] Bluetooth: btintel_pcie: Support function level reset

[Bjorn Helgaas <helgaas@kernel.org>]

On Tue, Mar 18, 2025 at 10:55:06AM -0400, Luiz Augusto von Dentz wrote:
> On Fri, Mar 14, 2025 at 3:40 PM Bjorn Helgaas <helgaas@kernel.org> wrote:
> > On Fri, Mar 14, 2025 at 12:16:13PM +0200, Chandrashekar Devegowda wrote:
> > > Support function level reset (flr) on hardware exception to recover
> > > controller. Driver also implements the back-off time of 5 seconds and
> > > the maximum number of retries are limited to 5 before giving up.
> >
> > Sort of weird that the commit log mentions FLR, but it's not mentioned
> > in the patch itself except for BTINTEL_PCIE_FLR_RESET_MAX_RETRY.
> > Apparently the assumption is that DSM_SET_RESET_METHOD_PCIE performs
> > an FLR.
> >
> > Since this is an ACPI _DSM, presumably this mechanism only works for
> > devices built into the platform, not for any potential plug-in devices
> > that would not be described via ACPI.  I guess this driver probably
> > already only works for built-in devices because it also uses
> > DSM_SET_WDISABLE2_DELAY and DSM_SET_RESET_METHOD.
> >
> > There is a generic PCI core way to do FLR (pcie_reset_flr()), so I
> > assume the _DSM exists because the device needs some additional
> > device-specific work around the FLR.
> >
> > > +static void btintel_pcie_removal_work(struct work_struct *wk)
> > > +{
> > > +     struct btintel_pcie_removal *removal =
> > > +             container_of(wk, struct btintel_pcie_removal, work);
> > > +     struct pci_dev *pdev = removal->pdev;
> > > +     struct pci_bus *bus;
> > > +     struct btintel_pcie_data *data;
> > > +
> > > +     data = pci_get_drvdata(pdev);
> > > +
> > > +     pci_lock_rescan_remove();
> > > +
> > > +     bus = pdev->bus;
> > > +     if (!bus)
> > > +             goto out;
> > > +
> > > +     btintel_acpi_reset_method(data->hdev);
> > > +     pci_stop_and_remove_bus_device(pdev);
> > > +     pci_dev_put(pdev);
> > > +
> > > +     if (bus->parent)
> > > +             bus = bus->parent;
> > > +     pci_rescan_bus(bus);
> >
> > This remove and rescan by a driver that's bound to the device subverts
> > the driver model.  pci_stop_and_remove_bus_device() detaches the
> > driver from the device.  After the driver is detached, we should not
> > be running any driver code.
> 
> Yeah, this self removal was sort of bugging me as well, although I'm
> not familiar enough with the pci subsystem, having the driver remove
> and continue running code like pci_rescan_bus seems wrong as we may
> end up with multiple instances of the same driver.
> 
> > There are a couple other drivers that remove their own device (ath9k,
> > iwlwifi, asus_wmi, eeepc-laptop), but I think those are broken and
> > it's a mistake to add this pattern to more drivers.
> >
> > What's the reason for doing the remove and rescan?  The PCI core
> > doesn't reset the device when you do this, so it's not a "bigger
> > hammer reset".
> 
> I guess it was more of the expectation of Chandru to have a sort of
> hard reset, driver remove+probe, instead of a soft reset where the
> driver will just need to reinit itself after performing
> pcie_reset_flr.

If the object is just to reinitialize the driver, I think this hack of
removing and rescanning is a bad way to do it.  If you reset the
device, you now know the state of the device and you can make the
driver state match it.  If necessary you can always reuse part or all
of the .remove() and .probe() methods yourself, without this dance of
calling pci_stop_and_remove_bus_device() and pci_rescan_bus().

Bjorn