Re: [PATCH 1/3] PCI: Add check for CXL Secondary Bus Reset

[Dave Jiang <dave.jiang@intel.com>]

On 3/12/24 12:30 AM, Lukas Wunner wrote:
> On Mon, Mar 11, 2024 at 01:39:53PM -0700, Dave Jiang wrote:
>> +static bool is_cxl_device(struct pci_dev *dev)
>> +{
>> +	return pci_find_dvsec_capability(dev, PCI_DVSEC_VENDOR_ID_CXL,
>> +					 CXL_DVSEC_PCIE_DEVICE);
>> +}
> 
> If this was my bikeshed, I'd call it pci_is_cxl() to match pci_is_pcie().

Ok will change.
> 
> 
>> +static bool is_cxl_port_sbr_masked(struct pci_dev *dev)
>> +{
>> +	int dvsec;
>> +	int rc;
>> +	u16 reg;
> 
> Nit: Inverse Christmas tree?

Will change.
> 
> 
>>  static int pci_reset_bus_function(struct pci_dev *dev, bool probe)
>>  {
>>  	int rc;
>>  
>> +	/* If it's a CXL port and the SBR control is masked, fail the SBR */
>> +	if (is_cxl_device(dev) && dev->bus->self &&
>> +	    is_cxl_port_sbr_masked(dev->bus->self)) {
>> +		if (probe)
>> +			return 0;
>> +
>> +		return -EPERM;
>> +	}
>> +
> 
> Is this also necessary if CONFIG_CXL_PCI=n?

Yes. As the kernel only loads type3 mem class CXL device driver. This is attempt to cover all CXL devices and not dependent on a loaded driver.

> 
> Return code on non-availability of a reset method is generally -ENOTTY.

Ok I can change it to that.

> Or is the choice deliberate to expose this reset method despite the bit
> being set and thus allow user space to unmask it in the first place?

Yes the idea is if user intentionally unmasks the bit via a user tool then reset can go through.

> 
> Also, we mostly use pci_upstream_bridge(dev) in lieu of dev->bus->self.
> Is the choice to use the latter deliberate because maybe is_virtfn is
> never set and the device can never be on the root bus?  (What about
> RCiEP CXL devices?)

I'll change to pci_upstream_bridge() call. I didn't realize that existed.

> 
> Thanks,
> 
> Lukas