Re: [PATCH V1] PCI: Hide SBR from reset_methods if masked by CXL

[Dave Jiang]

On 2/25/26 6:13 AM, Vidya Sagar wrote:
> On 23/02/26 21:22, Dave Jiang wrote:
>> External email: Use caution opening links or attachments
>>
>>
>> On 2/23/26 6:11 AM, Vidya Sagar wrote:
>>> On 21/02/26 02:51, Dave Jiang wrote:
>>>> External email: Use caution opening links or attachments
>>>>
>>>>
>>>> On 2/20/26 12:52 PM, Vidya Sagar wrote:
>>>>> The CXL specification (e.g., CXL r3.1 v1.0, sec 8.1.5.2) defines
>>>>> the "Unmask SBR" bit in the Port Control Extensions Register.
>>>>> When this bit is 0 (default), asserting the Secondary Bus Reset (SBR) bit
>>>>> in the Bridge Control register has no effect on the downstream bus.
>>>>>
>>>>> Currently, the Linux PCI core checks this condition in
>>>>> pci_reset_bus_function(). If SBR is masked, it returns -ENOTTY during the
>>>>> execution of the reset. However, during the probe phase (when probe=true),
>>>>> the function currently returns 0. This 0 return value incorrectly signals
>>>>> to the PCI subsystem that SBR is a viable reset method for the device.
>>>>
>>>> The "Unmask SBR" bit is a toggle bit. It does not give indicator whether the device is capable of SBR or not.
>>>
>>> Not sure how is this point relevant here. Can you help me understand?
>>
>> That means it's not a capability bit. It's a r/w toggle bit to mask or unmask SBR for CXL.
>>
>>>
>>> BTW, what do you mean by "whether the device is capable of SBR or not". My understanding is that each downstream port
>>>
>>> must support SBR. The patch I made would ensure that 'bus' entry is not shown under 'reset_methods' if the downstream
>>>
>>> port is a CXL capable port and 'Unmask SBR' is '0'.
>>
>> The purpose of introducing the cxl bus reset method is, "you know what you are doing" because you selected this method and the kernel will set the bit to allow the reset.
> I'm not really questioning the purpose of 'cxl_bus' reset method.
> I'm only wondering why is 'bus' reset method shown if it can't be used.
>> By default the Unmask SBR bit is clear. So if you hide the reset attribute then the user can't reset without going through other means to set the bit first (i.e. via setpci tool). This is intentionally setup to do this through total control of the
>> kernel and not having to jump through hoops to toggle bit via a user tool first. Otherwise you can just toggle the bit with a user tool and use the standard PCI bus reset method.
> This doesn't make much sense to me. Since the 'cxl_bus' method is anyway going to update the 'Unmask SBR' before applying the SBR through Bride Control register, what is the point in showing 'bus' which doesn't work by default and expecting the user to toggle the 'Unmask SBR' through tools like setpci? The user can very well use 'cxl_bus' directly which does the same thing, right?

Ok I think I see where I made a mistake. You are hiding the regular PCI bus reset method and not the cxl_bus reset method. Yes then I agree what you are doing is fine. This does not impact the cxl_bus reset method. Sorry about the noise. 

DJ

> BTW, I found one issue with my current patch where I try to hide 'bus' if 'Unmask SBR' is '0', but the CXL spec also says that for the SBR bit in Bridge Control Register to be ineffective, both 'Unmask SBR' should be '0' and also the link as such must be operating in the CXL mode. I'll push V2 patch to add this check as well.
>>
>> Please see commit log of this commit:
>> 53c49b6e6dd2 ("PCI/CXL: Add 'cxl_bus' reset method for devices below CXL Ports")
>>
>> DJ
>>
>>>
>>>> The original thought was that if the user is issuing CXL SBR, you know what you are doing and the kernel will set that bit and issue the SBR.
>>>>
>>>> DJ
>>>>
>>>>> As a result, 'bus' is listed in the device's
>>>>> /sys/bus/pci/devices/.../reset_methods attribute, even though the hardware
>>>>> is incapable of performing it. If a user attempts to write bus to reset
>>>>> method or triggers a reset that falls back to SBR, the operation fails
>>>>> with: "bash: echo: write error: Inappropriate ioctl for device" error.
>>>>>
>>>>> This patch modifies pci_reset_bus_function() to return -ENOTTY immediately
>>>>> if cxl_sbr_masked() is true, regardless of the probe argument. This
>>>>> ensures that 'bus' is not advertised in reset_methods when the hardware
>>>>> prevents it, improving clarity for users and aligning the sysfs capability
>>>>> report with actual hardware behavior.
>>>>>
>>>>> Signed-off-by: Vidya Sagar <vidyas@xxxxxxxxxx>
>>>>> ---
>>>>>  drivers/pci/pci.c | 6 +-----
>>>>>  1 file changed, 1 insertion(+), 5 deletions(-)
>>>>>
>>>>> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
>>>>> index f3244630bfd0..57e24300d1c7 100644
>>>>> --- a/drivers/pci/pci.c
>>>>> +++ b/drivers/pci/pci.c
>>>>> @@ -4915,12 +4915,8 @@ static int pci_reset_bus_function(struct pci_dev *dev, bool probe)
>>>>>        * If "dev" is below a CXL port that has SBR control masked, SBR
>>>>>        * won't do anything, so return error.
>>>>>        */
>>>>> -     if (bridge && cxl_sbr_masked(bridge)) {
>>>>> -             if (probe)
>>>>> -                     return 0;
>>>>> -
>>>>> +     if (bridge && cxl_sbr_masked(bridge))
>>>>>               return -ENOTTY;
>>>>> -     }
>>>>>
>>>>>       rc = pci_dev_reset_iommu_prepare(dev);
>>>>>       if (rc) {
>>>
>>>
>>
>