Re: [PATCH v3] cxl/pci: Support Global Persistent Flush (GPF)

[Dan Williams <dan.j.williams@intel.com>]

Davidlohr Bueso wrote:
[..]
> >> +	if (!dvsec) {
> >> +		dev_warn(&pdev->dev, "Port GPF DVSEC not present\n");
> >
> >Could be pci_warn() to use @pdev directly.
> 
> Sure, but afaict the pci_ flavors aren't used throughout the cxl driver.

Fair enough, yes, match local style.

> 
> ...
> 
> >> @@ -1038,6 +1029,18 @@ static int cxl_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id)
> >>  	if (cxl_pci_ras_unmask(pdev))
> >>  		dev_dbg(&pdev->dev, "No RAS reporting unmasked\n");
> >>
> >> +	/*
> >> +	 * Set dirty shutdown now, with the expectation that the device
> >> +	 * clear it upon a successful GPF flow. The exception to this
> >> +	 * is upon Viral detection, per CXL 3.2 section 12.4.2.
> >> +	 */
> >> +	if (resource_size(&cxlds->pmem_res)) {
> >> +		rc = cxl_dirty_shutdown_state(mds);
> >> +		if (rc)
> >> +			dev_warn(&pdev->dev,
> >> +				 "GPF: could not dirty shutdown state\n");
> >
> >I think the proper place for this call is in cxl_nvdimm_probe(). That
> >happens after the device has succeeded cxl_mem_probe() and successfully
> >enumerated pmem capacity, but before the kernel commits to using pmem
> >through the nvdimm subsystem.
> >
> >Had shutdown_state been part of cxl_nvdimm_probe() since day one, the
> >policy could have been: "if cxl_dirty_shutdown_state() fails,
> >cxl_nvdimm_probe() fails". However, I know that would regress cxl-test
> >and qemu if not devices in the wild that do not meet that spec mandatory
> >requirement where Linux has been lenient for a long while now.
> 
> So qemu has patches out there and I will also send a patch for the mock device.
> 
> But yes, I agree with your approach to failing the dirty shutdown.
> 
> There is a similar consideration to have with failing to set the port
> dvsec (ie: not there); should we fail the enumeration altogether? So far
> I have been lenient, and considering that you did not mention this, I *think*
> you agree. For users that know they will never need pmem that doesn't matter,
> but overall the platform would be considered to be exposed to data corruption
> (and afaict gpf dvsecs are mandatory). Perhaps another potential sysfs
> file that would alert users of quirky hardware?

gpf dvsecs are mandatory for pmem devices, but given older kernels
happily allow operation to continue it feels late to start breaking
setups on new kernel upgrades. Given that the administrator needs to
decide about their dirty-shutdown count policy anyway I think it is ok
to punt that whole policy problem to userspace.

I recall that the acpi_nfit driver exports an Optane dirty shutdown
count attribue in the nvdimm sysfs hierarchy, and there was never a
kernel policy to fail driver load based on dirty-shutdown. So we both
have precedent to not fail here and can eventually copy the same sysfs
precedent for attaching a $provider/dirty_shutdown attribute via the
cxl_dimm_attribute_group.

The rule I am thinking of is:

---
If you do not see a:

/sys/bus/cxl/devices/nvdimm-bridge0/ndbusX/nmemY/cxl/dirty_shutdown

...entry for your device it means dirty-shutdown tracking is disabled
due to:

- lack of kernel support
- lack of device gpf dvsec support
- lack of device Set Shutdown State support
- failure of Set Shutdown State to dirty the device
- failure of Get Health Info to retrieve the count
---