Re: [PATCH v18 3/3] PCI: Fail FLR when config space is inaccessible

[Farhan Ali <alifm@linux.ibm.com>]

On 6/10/2026 1:02 PM, Bjorn Helgaas wrote:
> [+cc Alex]
>
> On Wed, Jun 10, 2026 at 09:51:55AM -0700, Farhan Ali wrote:
>> On 6/9/2026 3:33 PM, Bjorn Helgaas wrote:
>>> On Wed, Jun 03, 2026 at 06:57:36PM +0000, sashiko-bot@kernel.org wrote:
>>>> Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
>>>>
>>>> Pre-existing issues:
>> ...
>>>>> +++ b/drivers/pci/pci.c
>>>>> @@ -4396,6 +4396,9 @@ int pcie_reset_flr(struct pci_dev *dev, bool probe)
>>>>>    	if (!(dev->devcap & PCI_EXP_DEVCAP_FLR))
>>>>>    		return -ENOTTY;
>>>>> +	if (!pci_dev_config_accessible(dev, "FLR"))
>>>>> +		return -ENOTTY;
>>>>> +
>>>> [Severity: High]
>>>> This isn't a bug introduced by this patch, but does placing this
>>>> check in pcie_reset_flr() leave direct callers of pcie_flr()
>>>> exposed to 60-second hangs?
>>>>
>>>> Direct callers of pcie_flr() (like network and storage drivers
>>>> during error recovery) bypass this wrapper. They will still
>>>> experience a stall in pci_dev_wait() when attempting to reset a
>>>> device with an inaccessible config space. Should this
>>>> accessibility check be moved into pcie_flr() instead to protect
>>>> all callers?
>>> I can't remember why we have both pcie_reset_flr() and pcie_flr(),
>>> other than the fact that callers don't need to supply a "probe"
>>> argument to pcie_flr().
>>>
>>> Is there a reason to call pci_dev_config_accessible() here rather
>>> than in pcie_flr()?
>> The reason we wanted the check in pcie_reset_flr() was so to be able
>> to escalate to bus reset method if we can't do an FLR. I think the
>> check could be moved to pcie_flr(). Is that more preferable?
> What if we added a preliminary patch that folds the body of pcie_flr()
> into pcie_reset_flr() and adds:
>
>    #define pcie_flr(dev)	pcie_reset_flr(dev, PCI_RESET_DO_RESET)
>
> That would mean pcie_flr() users would start paying attention to
> PCI_DEV_FLAGS_NO_FLR_RESET and PCI_EXP_DEVCAP_FLR, which seems like a
> good thing.

Looking at the history, it seems commit 56f107d7813f ("PCI: Add 
pcie_reset_flr() with 'probe' argument") added pcie_reset_flr() to add 
the probe and remove pcie_has_flr(). It also notes that callers of 
pcie_flr() that didn't call pcie_has_flr() remain. I guess it didn't 
want to change the drivers that called pcie_flr() unconditionally?

There is also reset_intel_82599_sfp_virtfn() that mentions the need to 
FLR without checking for FLR support due to some quirk. So I wonder if 
there were genuine reasons to avoid doing the check for 
PCI_DEV_FLAGS_NO_FLR_RESET and PCI_EXP_DEVCAP_FLR?

Thanks

Farhan