linux-kernel - Re: [PATCH 2/3] PCI: cadence: Use DT bindings to set PHY latencies

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]

Message-ID: <20221013135903.GA3243887@bhelgaas>
Date:   Thu, 13 Oct 2022 08:59:03 -0500
From:   Bjorn Helgaas <helgaas@...nel.org>
To:     Dominic Rath <dominic.rath@...-augsburg.de>
Cc:     robh+dt@...nel.org, krzysztof.kozlowski+dt@...aro.org,
        tjoseph@...ence.com, bhelgaas@...gle.com, lpieralisi@...nel.org,
        nm@...com, vigneshr@...com, devicetree@...r.kernel.org,
        linux-kernel@...r.kernel.org, linux-pci@...r.kernel.org,
        Alexander Bahle <bahle@...-augsburg.de>,
        Dominic Rath <rath@...-augsburg.de>
Subject: Re: [PATCH 2/3] PCI: cadence: Use DT bindings to set PHY latencies

On Thu, Oct 13, 2022 at 08:26:48AM +0200, Dominic Rath wrote:
> From: Alexander Bahle <bahle@...-augsburg.de>
> 
> Use optional "cdns,tx-phy-latency-ps" and "cdns,rx-phy-latency-ps"
> DeviceTree bindings to set the CDNS_PCIE_LM_PTM_LAT_PARAM(_IDX)
> register(s) during PCIe host and ep setup.
> The properties expect a list of uint32 PHY latencies in picoseconds for
> every supported speed starting at PCIe Gen1, e.g.:

s/ep/endpoint/
s/properties expect a list/properties are lists/

Rewrap into a single paragraph or add a blank line between paragraphs.

>   max-link-speed = <2>;
>   tx-phy-latency-ps = <100000 200000>; /* Gen1: 100ns, Gen2: 200ns */
>   rx-phy-latency-ps = <150000 250000>; /* Gen1: 150ns, Gen2: 250ns */
> 
> There should be a value for every supported speed but it is not enforced or
> necessary. A warning is emitted to let users know that the PTM timestamps
> from this PCIe device may not be precise enough for some applications.

Not sure what "it is not enforced or necessary" means.  Maybe it just
means that if a value is missing, we don't program LAT_PARAM and we
emit a warning?

> +	param_count = of_property_count_u32_elems(np, key);
> +	if (param_count < 0 || param_count < max_link_speed) {
> +		dev_warn(dev,
> +			"no %s set for one or more speeds: %d\n",
> +			key, param_count);
> +	}
> +
> +	/* Don't set param for unsupported speed */
> +	if (param_count > max_link_speed)
> +		param_count = max_link_speed;
> +
> +	for (i = 0; i < param_count; i++) {
> +		if (of_property_read_u32_index(np, key, i,
> +					       &latency) < 0) {
> +			dev_err(dev, "failed to set latency for speed %d. %s\n",
> +				i, key);

Seems like these messages should contain "PTM" somewhere.

If they're truly optional properties, should these be dev_info()
instead of dev_warn/dev_err?

Bjorn