Message-ID: <20231230184905.GA6104@wunner.de>
Date: Sat, 30 Dec 2023 19:49:05 +0100
From: Lukas Wunner <lukas@...ner.de>
To: Ilpo Järvinen <ilpo.jarvinen@...ux.intel.com>
Cc: linux-pci@...r.kernel.org, Bjorn Helgaas <helgaas@...nel.org>,
	Lorenzo Pieralisi <lorenzo.pieralisi@....com>,
	Rob Herring <robh@...nel.org>, Krzysztof Wilczy??ski <kw@...ux.com>,
	Alexandru Gagniuc <mr.nuke.me@...il.com>,
	Krishna chaitanya chundru <quic_krichai@...cinc.com>,
	Srinivas Pandruvada <srinivas.pandruvada@...ux.intel.com>,
	"Rafael J . Wysocki" <rafael@...nel.org>, linux-pm@...r.kernel.org,
	Bjorn Helgaas <bhelgaas@...gle.com>, linux-kernel@...r.kernel.org,
	Alex Deucher <alexdeucher@...il.com>,
	Daniel Lezcano <daniel.lezcano@...aro.org>,
	Amit Kucheria <amitk@...nel.org>, Zhang Rui <rui.zhang@...el.com>
Subject: Re: [PATCH v3 08/10] PCI/bwctrl: Add "controller" part into PCIe
 bwctrl

On Fri, Sep 29, 2023 at 02:57:21PM +0300, Ilpo Järvinen wrote:
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -16569,6 +16569,13 @@ F:	include/linux/pci*
>  F:	include/uapi/linux/pci*
>  F:	lib/pci*
>  
> +PCIE BANDWIDTH CONTROLLER
> +M:	Ilpo Järvinen <ilpo.jarvinen@...ux.intel.com>
> +L:	linux-pci@...r.kernel.org
> +S:	Supported
> +F:	drivers/pci/pcie/bwctrl.c
> +F:	include/linux/pci-bwctrl.h
> +

Maybe add this already in the preceding patch.


> --- a/drivers/pci/pcie/Kconfig
> +++ b/drivers/pci/pcie/Kconfig
> @@ -138,12 +138,13 @@ config PCIE_PTM
>  	  is safe to enable even if you don't.
>  
>  config PCIE_BW
> -	bool "PCI Express Bandwidth Change Notification"
> +	bool "PCI Express Bandwidth Controller"

I'd fold this change into the preceding patch.
Deleting a line introduced by the preceding patch isn't great.

Never mind that the patch isn't a clean revert that way.
Your approach of making changes to the original version
of the bandwith monitoring driver and calling out those
changes in the commit message seems fine to me.


>  	depends on PCIEPORTBUS
>  	help
> -	  This enables PCI Express Bandwidth Change Notification.  If
> -	  you know link width or rate changes occur to correct unreliable
> -	  links, you may answer Y.
> +	  This enables PCI Express Bandwidth Controller. The Bandwidth
> +	  Controller allows controlling PCIe link speed and listens for link
> +	  peed Change Notifications. If you know link width or rate changes
> +	  occur to correct unreliable links, you may answer Y.

It would be neater if you could avoid deleting lines introduced by
the preceding patch.  Ideally you'd add a new paragraph, thus only
add new lines but not delete any existing ones.


> --- a/drivers/pci/pcie/bwctrl.c
> +++ b/drivers/pci/pcie/bwctrl.c
> @@ -1,14 +1,16 @@
>  // SPDX-License-Identifier: GPL-2.0+
>  /*
> - * PCI Express Link Bandwidth Notification services driver
> + * PCIe bandwidth controller
> + *

Again, fold this change into the preceding patch please.

>   * Author: Alexandru Gagniuc <mr.nuke.me@...il.com>
>   *
>   * Copyright (C) 2019, Dell Inc
> + * Copyright (C) 2023 Intel Corporation.

For extra neatness, drop the comma in the preceding patch and
the period in this patch.


> - * The PCIe Link Bandwidth Notification provides a way to notify the
> - * operating system when the link width or data rate changes.  This
> - * capability is required for all root ports and downstream ports
> - * supporting links wider than x1 and/or multiple link speeds.
> + * The PCIe Bandwidth Controller provides a way to alter PCIe link speeds
> + * and notify the operating system when the link width or data rate changes.
> + * The notification capability is required for all Root Ports and Downstream
> + * Ports supporting links wider than x1 and/or multiple link speeds.

Again, adding a new paragraph and not deleting lines introduced by
the preceding patch would be much nicer.


> +#include <linux/bitops.h>
> +#include <linux/errno.h>
> +#include <linux/mutex.h>
> +#include <linux/pci.h>
> +#include <linux/pci-bwctrl.h>
> +#include <linux/types.h>
> +
> +#include <asm/rwonce.h>
> +

Which of these are necessary for functionality introduced by this
patch and which are necessary anyway and should be moved to the
preceding patch?


> +/**
> + * struct bwctrl_service_data - PCIe Port Bandwidth Controller

Code comment at the top of this file says "PCIe bandwidth controller",
use here as well for neatness.


> +static u16 speed2lnkctl2(enum pci_bus_speed speed)
> +{
> +	static const u16 speed_conv[] = {
> +		[PCIE_SPEED_2_5GT] = PCI_EXP_LNKCTL2_TLS_2_5GT,
> +		[PCIE_SPEED_5_0GT] = PCI_EXP_LNKCTL2_TLS_5_0GT,
> +		[PCIE_SPEED_8_0GT] = PCI_EXP_LNKCTL2_TLS_8_0GT,
> +		[PCIE_SPEED_16_0GT] = PCI_EXP_LNKCTL2_TLS_16_0GT,
> +		[PCIE_SPEED_32_0GT] = PCI_EXP_LNKCTL2_TLS_32_0GT,
> +		[PCIE_SPEED_64_0GT] = PCI_EXP_LNKCTL2_TLS_64_0GT,
> +	};

Looks like this could be a u8 array to save a little bit of memory.

Also, this seems to duplicate pcie_link_speed[] defined in probe.c.


> +static int bwctrl_select_speed(struct pcie_device *srv, enum pci_bus_speed *speed)
> +{
> +	struct pci_bus *bus = srv->port->subordinate;
> +	u8 speeds, dev_speeds;
> +	int i;
> +
> +	if (*speed > PCIE_LNKCAP2_SLS2SPEED(bus->pcie_bus_speeds))
> +		return -EINVAL;
> +
> +	dev_speeds = READ_ONCE(bus->pcie_dev_speeds);

Hm, why is the compiler barrier needed?


> +	/* Only the lowest speed can be set when there are no devices */
> +	if (!dev_speeds)
> +		dev_speeds = PCI_EXP_LNKCAP2_SLS_2_5GB;

Maybe move this to patch [06/10], i.e. set dev->bus->pcie_dev_speeds to
PCI_EXP_LNKCAP2_SLS_2_5GB on removal (instead of 0).


> +	speeds = bus->pcie_bus_speeds & dev_speeds;
> +	i = BIT(fls(speeds));
> +	while (i >= PCI_EXP_LNKCAP2_SLS_2_5GB) {
> +		enum pci_bus_speed candidate;
> +
> +		if (speeds & i) {
> +			candidate = PCIE_LNKCAP2_SLS2SPEED(i);
> +			if (candidate <= *speed) {
> +				*speed = candidate;
> +				return 0;
> +			}
> +		}
> +		i >>= 1;
> +	}

Can't we just do something like

	supported_speeds = bus->pcie_bus_speeds & dev_speeds;
	desired_speeds = GEN_MASK(pcie_link_speed[*speed], 1);
	*speed = BIT(fls(supported_speeds & desired_speeds));

and thus avoid the loop altogether?


> +/**
> + * bwctrl_set_current_speed - Set downstream link speed for PCIe port
> + * @srv: PCIe port
> + * @speed: PCIe bus speed to set
> + *
> + * Attempts to set PCIe port link speed to @speed. As long as @speed is less
> + * than the maximum of what is supported by @srv, the speed is adjusted
> + * downwards to the best speed supported by both the port and device
> + * underneath it.
> + *
> + * Return:
> + * * 0 - on success
> + * * -EINVAL - @speed is higher than the maximum @srv supports
> + * * -ETIMEDOUT - changing link speed took too long
> + * * -EAGAIN - link speed was changed but @speed was not achieved
> + */

So passing a higher speed than what's supported by the Endpoint is fine
but passing a higher speed than what's supported by the Downstream Port
is not?  Why the distinction?  Maybe it would be more logical to just
pick the maximum of what either supports and use that in lieu of a
too-high speed?


> +int bwctrl_set_current_speed(struct pcie_device *srv, enum pci_bus_speed speed)
> +{
> +	struct bwctrl_service_data *data = get_service_data(srv);
> +	struct pci_dev *port = srv->port;
> +	u16 link_status;
> +	int ret;
> +
> +	if (WARN_ON_ONCE(!bwctrl_valid_pcie_speed(speed)))
> +		return -EINVAL;
> +
> +	ret = bwctrl_select_speed(srv, &speed);
> +	if (ret < 0)
> +		return ret;

How about checking here if the selected speed is equivalent to what's
being used right now and bailing out if so?  No need to retrain if
we're already at the desired speed.


> @@ -91,16 +242,32 @@ static int pcie_bandwidth_notification_probe(struct pcie_device *srv)
>  	if (ret)
>  		return ret;
>  
> +	data = kzalloc(sizeof(*data), GFP_KERNEL);
> +	if (!data) {
> +		ret = -ENOMEM;
> +		goto free_irq;
> +	}
> +	mutex_init(&data->set_speed_mutex);
> +	set_service_data(srv, data);
> +

I think you should move that further up so that you allocate and populate
the data struct before requesting the IRQ.  Otherwise if BIOS has already
enabled link bandwith notification for some reason, the IRQ handler might
be invoked without the data struct being allocated.


> --- /dev/null
> +++ b/include/linux/pci-bwctrl.h
> @@ -0,0 +1,17 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +/*
> + * PCIe bandwidth controller
> + *
> + * Copyright (C) 2023 Intel Corporation.

Another trailing period I'd remove.

Thanks,

Lukas

Powered by blists - more mailing lists