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

[Bjorn Helgaas <helgaas@kernel.org>]

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.

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".

> +out:
> +	pci_unlock_rescan_remove();
> +	kfree(removal);
> +}