Re: [PATCH] PCI: PTM preliminary implementation

[Bjorn Helgaas <helgaas@kernel.org>]

On Fri, Apr 29, 2016 at 11:20:26AM -0500, Bjorn Helgaas wrote:
> On Tue, Apr 19, 2016 at 06:29:18AM +0000, Yong, Jonathan wrote:
> > ...
> > +static ssize_t ptm_status_show(struct device *dev,
> > +	struct device_attribute *attr, char *buf)
> > +{
> > +	struct pci_dev *pdev = to_pci_dev(dev);
> > +	u16 word;
> > +	int pos;
> > +
> > +	pos = pci_find_ext_capability(pdev, PCI_EXT_CAP_ID_PTM);
> > +	if (!pos)
> > +		return -ENXIO;
> > +
> > +	pci_read_config_word(pdev, pos + PCI_PTM_CONTROL_REG_OFFSET, &word);
> 
> This is a 32-bit register; read it as a dword, even though the upper
> bits are currently reserved.
> 
> > +	return sprintf(buf, "%u\n", word & PCI_PTM_CTRL_ENABLE ? 1 : 0);
> > +}
> > +
> > +static ssize_t ptm_status_store(struct device *dev,
> > +	struct device_attribute *attr, const char *buf, size_t count)
> > +{
> > +	struct pci_dev *pdev = to_pci_dev(dev);
> > +	unsigned long val;
> > +	ssize_t ret;
> > +
> > +	ret = kstrtoul(buf, 0, &val);
> > +	if (ret)
> > +		return ret;
> > +	if (val)
> > +		return pci_enable_ptm(pdev);
> > +	pci_disable_ptm(pdev);
> > +	return 0;
> > +}
> > +
> > +static DEVICE_ATTR_RW(ptm_status);

Thinking about this sysfs interface some more...  Do you think we
really need a knob in sysfs?  What's the use case for it?  I suppose
there's potentially a performance impact because we'll send more
messages across the link, but I'd be surprised if it were measurable.
If it's basically free, there shouldn't be a need to enable/disable it
individually, except maybe to work around hardware bugs.  A kernel
boot parameter might be enough for that.  Even a kernel boot parameter
should be regarded as a debug/testing tool, not a solution.  If we
find broken devices, we should add quirks for them so users don't need
to use a boot parameter.

Bjorn