linux-kernel - Re: [PATCH v4 3/3] pci: intel: Add sysfs attributes to configure pcie link

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Mon, 21 Oct 2019 12:18:32 -0500
From:   Bjorn Helgaas <helgaas@...nel.org>
To:     Andrew Murray <andrew.murray@....com>
Cc:     Dilip Kota <eswara.kota@...ux.intel.com>, jingoohan1@...il.com,
        gustavo.pimentel@...opsys.com, lorenzo.pieralisi@....com,
        robh@...nel.org, martin.blumenstingl@...glemail.com,
        linux-pci@...r.kernel.org, hch@...radead.org,
        devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
        andriy.shevchenko@...el.com, cheol.yong.kim@...el.com,
        chuanhua.lei@...ux.intel.com, qi-ming.wu@...el.com
Subject: Re: [PATCH v4 3/3] pci: intel: Add sysfs attributes to configure
 pcie link

On Mon, Oct 21, 2019 at 02:38:50PM +0100, Andrew Murray wrote:
> On Mon, Oct 21, 2019 at 02:39:20PM +0800, Dilip Kota wrote:
> > PCIe RC driver on Intel Gateway SoCs have a requirement
> > of changing link width and speed on the fly.

Please add more details about why this is needed.  Since you're adding
sysfs files, it sounds like it's not actually the *driver* that needs
this; it's something in userspace?

The normal scenario is that the hardware negotiates link widths and
speeds without any software involvement (PCIe r5.0, sec 1.2).

If this is to work around hardware defects, we should try to do that
inside the kernel because we can't expect userspace to do it reliably.

As Andrew points out below, this all sounds like it should be generic
rather than Intel-specific.

> > So add the sysfs attributes to show and store the link
> > properties.
> > Add the respective link resize function in pcie DesignWare
> > framework so that Intel PCIe driver can use during link
> > width configuration on the fly.
> > ...

> > +static ssize_t pcie_link_status_show(struct device *dev,
> > +				     struct device_attribute *attr, char *buf)
> > +{
> > +	struct intel_pcie_port *lpp = dev_get_drvdata(dev);
> > +	u32 reg, width, gen;
> > +
> > +	reg = pcie_rc_cfg_rd(lpp, PCIE_CAP_OFST + PCI_EXP_LNKCTL);
> > +	width = FIELD_GET(PCI_EXP_LNKSTA_NLW, reg >> 16);
> > +	gen = FIELD_GET(PCI_EXP_LNKSTA_CLS, reg >> 16);
> > +
> > +	if (gen > lpp->max_speed)
> > +		return -EINVAL;
> > +
> > +	return sprintf(buf, "Port %2u Width x%u Speed %s GT/s\n", lpp->id,
> > +		       width, pcie_link_gen_to_str(gen));
> > +}
> > +static DEVICE_ATTR_RO(pcie_link_status);

We already have generic current_link_speed and current_link_width
files.

> > +static ssize_t pcie_speed_store(struct device *dev,
> > +				struct device_attribute *attr,
> > +				const char *buf, size_t len)
> > +{
> > +	struct intel_pcie_port *lpp = dev_get_drvdata(dev);
> > +	unsigned long val;
> > +	int ret;
> > +
> > +	ret = kstrtoul(buf, 10, &val);
> > +	if (ret)
> > +		return ret;
> > +
> > +	if (val > lpp->max_speed)
> > +		return -EINVAL;
> > +
> > +	lpp->link_gen = val;
> > +	intel_pcie_max_speed_setup(lpp);
> > +	dw_pcie_link_speed_change(&lpp->pci, false);
> > +	dw_pcie_link_speed_change(&lpp->pci, true);
> > +
> > +	return len;
> > +}
> > +static DEVICE_ATTR_WO(pcie_speed);
> > +
> > +/*
> > + * Link width change on the fly is not always successful.
> > + * It also depends on the partner.
> > + */
> > +static ssize_t pcie_width_store(struct device *dev,
> > +				struct device_attribute *attr,
> > +				const char *buf, size_t len)
> > +{
> > +	struct intel_pcie_port *lpp = dev_get_drvdata(dev);
> > +	unsigned long val;
> > +	int ret;
> > +
> > +	lpp = dev_get_drvdata(dev);
> > +
> > +	ret = kstrtoul(buf, 10, &val);
> > +	if (ret)
> > +		return ret;
> > +
> > +	if (val > lpp->max_width)
> > +		return -EINVAL;
> > +
> > +	/* HW auto bandwidth negotiation must be enabled */
> > +	pcie_rc_cfg_wr_mask(lpp, PCI_EXP_LNKCTL_HAWD, 0,
> > +			    PCIE_CAP_OFST + PCI_EXP_LNKCTL);
> > +	dw_pcie_link_width_resize(&lpp->pci, val);
> > +
> > +	return len;
> > +}
> > +static DEVICE_ATTR_WO(pcie_width);
> > +
> > +static struct attribute *pcie_cfg_attrs[] = {
> > +	&dev_attr_pcie_link_status.attr,
> > +	&dev_attr_pcie_speed.attr,
> > +	&dev_attr_pcie_width.attr,
> > +	NULL,
> > +};
> 
> Is there a reason that these are limited only to the Intel driver and
> not the wider set of DWC drivers?
> 
> Is there anything specific here about the Intel GW driver?