Re: [PATCH v2 1/3] PCI: qcom: Refactor common code

[Johan Hovold]

On Wed, Mar 20, 2024 at 12:14:45AM -0700, Shashank Babu Chinta Venkata wrote:
> Refactor common code from RC(Root Complex) and EP(End Point)

Please add a space before the open parentheses above, these are not
function calls.

> drivers and move them to a common repository. This acts as placeholder
> for common source code for both drivers avoiding duplication.
> 
> Signed-off-by: Shashank Babu Chinta Venkata <quic_schintav@xxxxxxxxxxx>

> --- /dev/null
> +++ b/drivers/pci/controller/dwc/pcie-qcom-cmn.c
> @@ -0,0 +1,81 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (c) 2014-2015, 2020 The Linux Foundation. All rights reserved.
> + * Copyright 2015, 2021 Linaro Limited.
> + * Copyright (c) 2024 Qualcomm Innovation Center, Inc. All rights reserved.
> + *
> + */
> +
> +#include <linux/debugfs.h>

Not needed.

> +#include <linux/pci.h>
> +#include <linux/interconnect.h>
> +
> +#include "../../pci.h"
> +#include "pcie-designware.h"
> +#include "pcie-qcom-cmn.h"
> +
> +#define QCOM_PCIE_LINK_SPEED_TO_BW(speed) \
> +		Mbps_to_icc(PCIE_SPEED2MBS_ENC(pcie_link_speed[speed]))
> +
> +int qcom_pcie_cmn_icc_get_resource(struct dw_pcie *pci, struct icc_path *icc_mem)
> +{
> +	if (IS_ERR(pci))
> +		return PTR_ERR(pci);

Not needed.

> +
> +	icc_mem = devm_of_icc_get(pci->dev, "pcie-mem");
> +	if (IS_ERR(icc_mem))
> +		return PTR_ERR(icc_mem);
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(qcom_pcie_cmn_icc_get_resource);

So this series was clearly never tested properly as the above function
will leave the driver's icc_mem path uninitialised. You're passing in a
NULL pointer by value and then update your local variable, which
obviously has no effect for the caller.

This means that all later icc operation become no-ops, which crashes
machine like the Lenovo ThinkPad X13s and the x1e80100 CRD that depends
on having a non-zero vote before enabling clocks at probe.

How did this go unnoticed? I can only assume you did not test this
series (in isolation) before posting?

> +int qcom_pcie_cmn_icc_init(struct dw_pcie *pci, struct icc_path *icc_mem)
> +{
> +	int ret;
> +
> +	if (IS_ERR(pci))
> +		return PTR_ERR(pci);
> +
> +	if (IS_ERR(icc_mem))
> +		return PTR_ERR(icc_mem);

Neither is needed.

> +
> +	/*
> +	 * Some Qualcomm platforms require interconnect bandwidth constraints
> +	 * to be set before enabling interconnect clocks.
> +	 *
> +	 * Set an initial peak bandwidth corresponding to single-lane Gen 1
> +	 * for the pcie-mem path.
> +	 */

I'm not sure about hiding this away in a separate compilation unit. The
above comments makes sense in the driver, where it's easy to see that
the icc is initialised and the vote added before enabling clocks.

Also these helpers are so small it may not even be worth trying to
refactor them (all).

> +	ret = icc_set_bw(icc_mem, 0, QCOM_PCIE_LINK_SPEED_TO_BW(1));
> +	if (ret) {
> +		dev_err(pci->dev, "failed to set interconnect bandwidth: %d\n",
> +			ret);
> +		return ret;
> +	}
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(qcom_pcie_cmn_icc_init);

> --- /dev/null
> +++ b/drivers/pci/controller/dwc/pcie-qcom-cmn.h
> @@ -0,0 +1,14 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + * Copyright (c) 2014-2015, 2020 The Linux Foundation. All rights reserved.
> + * Copyright 2015, 2021 Linaro Limited.
> + * Copyright (c) 2024 Qualcomm Innovation Center, Inc. All rights reserved.
> + */
> +
> +#include <linux/pci.h>
> +#include "../../pci.h"
> +#include "pcie-designware.h"
> +

Compile guards missing.

> +int qcom_pcie_cmn_icc_get_resource(struct dw_pcie *pci, struct icc_path *icc_mem);
> +int qcom_pcie_cmn_icc_init(struct dw_pcie *pci, struct icc_path *icc_mem);
> +void qcom_pcie_cmn_icc_update(struct dw_pcie *pci, struct icc_path *icc_mem);

Johan