Hi Bjorn, Luiz,
Thanks for the comments. I have published v2 version of the patch addressing the comments.
>Subject: Re: [PATCH v1] Bluetooth: btintel_pcie: Support function level reset
>
>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@xxxxxxxxxx> 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
Thanks,
Kiran
