Re: [PATCH v5 2/2] PM / devfreq: Add HiSilicon uncore frequency scaling driver

[Jonathan Cameron <Jonathan.Cameron@huawei.com>]

On Thu, 19 Jun 2025 23:14:56 +0800
Jie Zhan <zhanjie9@hisilicon.com> wrote:

> Add the HiSilicon uncore frequency scaling driver for Kunpeng SoCs based on
> the devfreq framework.  The uncore domain contains shared computing
> resources, including system interconnects and L3 cache.  The uncore
> frequency significantly impacts the system-wide performance as well as
> power consumption.  This driver adds support for runtime management of
> uncore frequency from kernel and userspace.  The main function includes
> setting and getting frequencies, changing frequency scaling policies, and
> querying the list of CPUs whose performance is significantly related to
> this uncore frequency domain, etc.  The driver communicates with a platform
> controller through an ACPI PCC mailbox to take the actual actions of
> frequency scaling.
> 
> Co-developed-by: Lifeng Zheng <zhenglifeng1@huawei.com>
> Signed-off-by: Lifeng Zheng <zhenglifeng1@huawei.com>
> Signed-off-by: Jie Zhan <zhanjie9@hisilicon.com>

Hi zhanjie,

A few more things inline that I missed in earlier review.

The devm_mutex one is a definite thing to fix as having it
where it is will make it far to easy to add bugs.

The other stuff is a nice to have.

So with at least the devm_mutex() call moved

Reviewed-by: Jonathan Cameron <jonathan.cameron@huawei.com>


> diff --git a/drivers/devfreq/hisi_uncore_freq.c b/drivers/devfreq/hisi_uncore_freq.c
> new file mode 100644
> index 000000000000..e19678692c16
> --- /dev/null
> +++ b/drivers/devfreq/hisi_uncore_freq.c
> @@ -0,0 +1,664 @@

> +static int hisi_uncore_request_pcc_chan(struct hisi_uncore_freq *uncore)
> +{
> +	struct device *dev = uncore->dev;
> +	struct pcc_mbox_chan *pcc_chan;
> +	int rc;
> +
> +	uncore->cl = (struct mbox_client) {
> +		.dev = dev,
> +		.tx_block = false,
> +		.knows_txdone = true,
> +	};
> +
> +	pcc_chan = pcc_mbox_request_channel(&uncore->cl, uncore->chan_id);

I'm not particularly keen on the repeats of pcc_mbox_free_channel() in each of
the error paths.  Either use a goto and clean it up at the end or
DEFINE_FREE(pcc_mbox_chan, struct pcc_mbox_chan *, if (_T) pcc_mbox_free_channel(_T));

then here
	struct pcc_mbox_chan __free(pcc_mbox_chan) *pcc_chan =
		pcc_mbox_request_channel(&uncore->cl, uncore->chan_id);

remembering to do 
	uncor->chan = no_free_ptr(pcc_chan);
where you currently set it below.

The DEFINE_FREE() might prove useful in other drivers.  kunpeng_hccs
for instance could be simplified with this.  Various other candidates
though not sure how keen on cleanup.h magic those areas of the kernel are.


> +	if (IS_ERR(pcc_chan))
> +		return dev_err_probe(dev, PTR_ERR(pcc_chan),
> +			"Failed to request PCC channel %u\n", uncore->chan_id);
> +
> +	if (!pcc_chan->shmem_base_addr) {
> +		pcc_mbox_free_channel(pcc_chan);
> +		return dev_err_probe(dev, -EINVAL,
> +			"Invalid PCC shared memory address\n");
> +	}
> +
> +	if (pcc_chan->shmem_size < sizeof(struct hisi_uncore_pcc_shmem)) {
> +		pcc_mbox_free_channel(pcc_chan);
> +		return dev_err_probe(dev, -EINVAL,
> +			"Invalid PCC shared memory size (%lluB)\n",
> +			pcc_chan->shmem_size);
> +	}
> +
> +	rc = devm_mutex_init(dev, &uncore->pcc_lock);

This is oddly placed.  Drop this out of this function and do it at least one layer
up.

Having it here leaves the risk of future error cases being added after this
where the devm cleanup will happen out of order as a result.

> +	if (rc) {
> +		pcc_mbox_free_channel(pcc_chan);
> +		return rc;
> +	}
> +
> +	uncore->pchan = pcc_chan;
> +
> +	return 0;
> +}