| 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
| ||
|
Message-ID: <20231128154448.00006b8f@Huawei.com>
Date: Tue, 28 Nov 2023 15:44:48 +0000
From: Jonathan Cameron <Jonathan.Cameron@...wei.com>
To: Huisong Li <lihuisong@...wei.com>
CC: <xuwei5@...ilicon.com>, <linux-kernel@...r.kernel.org>,
<soc@...nel.org>, <linux-arm-kernel@...ts.infradead.org>,
<arnd@...db.de>, <krzk@...nel.org>, <sudeep.holla@....com>,
<liuyonglong@...wei.com>
Subject: Re: [PATCH v1 3/3] soc: hisilicon: kunpeng_hccs: Support the
platform with PCC type3 and interrupt ack
On Thu, 9 Nov 2023 13:45:26 +0800
Huisong Li <lihuisong@...wei.com> wrote:
> Support the platform with PCC type3 and interrupt ack.
Probably mention this is version 2 as that's what you call it
in the code.
>
> Signed-off-by: Huisong Li <lihuisong@...wei.com>
Hi.
Main comment in here is that it is almost always better to use
a version specific structure with callbacks / data etc rather than
have instances of if (version1) do_x; else if (version2) do_y;
It ends up pulling all the differences into one place + allows a
great deal more flexibility. See inline for details.
Otherwise looks fine to me
Jonathan
> ---
> drivers/soc/hisilicon/kunpeng_hccs.c | 137 +++++++++++++++++++++------
> drivers/soc/hisilicon/kunpeng_hccs.h | 2 +
> 2 files changed, 110 insertions(+), 29 deletions(-)
>
> diff --git a/drivers/soc/hisilicon/kunpeng_hccs.c b/drivers/soc/hisilicon/kunpeng_hccs.c
> index fd3ca0eb8175..96cdac7be244 100644
> --- a/drivers/soc/hisilicon/kunpeng_hccs.c
> +++ b/drivers/soc/hisilicon/kunpeng_hccs.c
> @@ -38,6 +38,11 @@
...
>
> +static int hccs_get_device_version(struct hccs_dev *hdev)
> +{
> + const struct acpi_device_id *acpi_id;
> +
> + acpi_id = acpi_match_device(hdev->dev->driver->acpi_match_table,
> + hdev->dev);
Why not just have
hdev->dev_ver = (u8)acpi_device_get_match_data(&hdev->dev);
inline where this is called?
You probably don't even need the error check as you can't get here
without an appropriate match as the driver would never be matched.
> + if (!acpi_id) {
> + dev_err(hdev->dev, "get device version failed.");
> + return -EINVAL;
> + }
> +
> + hdev->dev_ver = (u8)acpi_id->driver_data;
> + return 0;
> +}
> +
...
> static void hccs_unregister_pcc_channel(struct hccs_dev *hdev)
> {
> struct hccs_mbox_client_info *cl_info = &hdev->cl_info;
> @@ -131,6 +159,11 @@ static int hccs_register_pcc_channel(struct hccs_dev *hdev)
> cl->tx_block = false;
> cl->knows_txdone = true;
> cl->tx_done = hccs_chan_tx_done;
> + if (hdev->dev_ver == HCCS_HW_DEVICE_V2) {
I'd prefer to see this done as data. That is, instead of having an enum
used in the ACPI match data, have a pointer to a struct with the callback.
Then this will become something like
hcc_info = acpi_device_get_match_data(hdev);
cl->rx_callback = hcc_info->rx_callback;
init_completion(&cl_info->done);
Initializing the completion is harmless if it's not used, so just do it
unconditionally.
> + cl->rx_callback = hccs_pcc_rx_callback;
> + init_completion(&cl_info->done);
> + }
> +
> pcc_chan = pcc_mbox_request_channel(cl, hdev->chan_id);
> if (IS_ERR(pcc_chan)) {
> dev_err(dev, "PPC channel request failed.\n");
> @@ -147,10 +180,16 @@ static int hccs_register_pcc_channel(struct hccs_dev *hdev)
> */
> cl_info->deadline_us =
> HCCS_PCC_CMD_WAIT_RETRIES_NUM * pcc_chan->latency;
> - if (cl_info->mbox_chan->mbox->txdone_irq) {
> + if (hdev->dev_ver == HCCS_HW_DEVICE_V1 &&
> + cl_info->mbox_chan->mbox->txdone_irq) {
Also data in hcc_info would be better than version number based
code flow.
if (hcc_info->has_txdone_irq &&
cl_info->mbox_chan->mbox->rx_done_irq) {
....
} else if (!hcc_info->has_txdone_irq &&
!cl_info->mbox_chan->mbox->tx_done_irq) {
...
}
> dev_err(dev, "PCC IRQ in PCCT is enabled.\n");
> rc = -EINVAL;
> goto err_mbx_channel_free;
> + } else if (hdev->dev_ver == HCCS_HW_DEVICE_V2 &&
> + !cl_info->mbox_chan->mbox->txdone_irq) {
> + dev_err(dev, "PCC IRQ in PCCT isn't supported.\n");
> + rc = -EINVAL;
> + goto err_mbx_channel_free;
> }
>
> if (pcc_chan->shmem_base_addr) {
> @@ -175,49 +214,81 @@ static int hccs_register_pcc_channel(struct hccs_dev *hdev)
> static int hccs_check_chan_cmd_complete(struct hccs_dev *hdev)
> {
> struct hccs_mbox_client_info *cl_info = &hdev->cl_info;
> - struct acpi_pcct_shared_memory __iomem *comm_base =
> - cl_info->pcc_comm_addr;
> + struct acpi_pcct_shared_memory __iomem *comm_base;
> + int ret = 0;
> u16 status;
> - int ret;
>
> /*
> * Poll PCC status register every 3us(delay_us) for maximum of
> * deadline_us(timeout_us) until PCC command complete bit is set(cond)
> */
> - ret = readw_poll_timeout(&comm_base->status, status,
> - status & PCC_STATUS_CMD_COMPLETE,
> - HCCS_POLL_STATUS_TIME_INTERVAL_US,
> - cl_info->deadline_us);
> - if (unlikely(ret))
> - dev_err(hdev->dev, "poll PCC status failed, ret = %d.\n", ret);
> + if (hdev->dev_ver == HCCS_HW_DEVICE_V1) {
As above. I'd prefer to see this as a call back in an info structure rather
than code here.
> + comm_base = cl_info->pcc_comm_addr;
> + ret = readw_poll_timeout(&comm_base->status, status,
> + status & PCC_STATUS_CMD_COMPLETE,
> + HCCS_POLL_STATUS_TIME_INTERVAL_US,
> + cl_info->deadline_us);
> + if (unlikely(ret))
> + dev_err(hdev->dev, "poll PCC status failed, ret = %d.\n", ret);
> + } else {
> + if (!wait_for_completion_timeout(&cl_info->done,
> + usecs_to_jiffies(cl_info->deadline_us))) {
> + dev_err(hdev->dev, "PCC command executed timeout!\n");
> + ret = -ETIMEDOUT;
> + }
> + }
>
> return ret;
> }
>
> +static void hccs_fill_pcc_shared_mem_region(struct hccs_dev *hdev, u8 cmd,
> + struct hccs_desc *desc,
> + void __iomem *comm_space,
> + u16 space_size)
> +{
> + struct hccs_mbox_client_info *cl_info = &hdev->cl_info;
> + struct acpi_pcct_ext_pcc_shared_memory tmp1 = {0};
> + struct acpi_pcct_shared_memory tmp2 = {0};
> +
> + if (hdev->dev_ver == HCCS_HW_DEVICE_V1) {
1) tmp1 and temp2 are local to the two forks of this statement
so pull them down here.
2) Use c99 style struct init to make this cleaner.
struct acpi_pcct_shared_memory = {
.signature = x,
.command = cmd,
};
I'd also like this to be a callback in the version specific info
structure rather than done as an if / else here that really doesn't
extend well if we get a lot more versions doing it differently.
> + tmp2.signature = PCC_SIGNATURE | hdev->chan_id;
> + tmp2.command = cmd;
> + tmp2.status = 0;
> + memcpy_toio(cl_info->pcc_comm_addr, (void *)&tmp2,
> + sizeof(struct acpi_pcct_shared_memory));
> + } else {
> + tmp1.signature = PCC_SIGNATURE | hdev->chan_id;
> + tmp1.command = cmd;
> + tmp1.flags = PCC_CMD_COMPLETION_NOTIFY;
> + tmp1.length = HCCS_PCC_SHARE_MEM_BYTES;
> + memcpy_toio(cl_info->pcc_comm_addr, (void *)&tmp1,
> + sizeof(struct acpi_pcct_ext_pcc_shared_memory));
> + }
> +
> + /* Copy the message to the PCC comm space */
> + memcpy_toio(comm_space, (void *)desc, space_size);
> +}
...
> @@ -1214,6 +1288,10 @@ static int hccs_probe(struct platform_device *pdev)
> hdev->dev = &pdev->dev;
> platform_set_drvdata(pdev, hdev);
>
> + rc = hccs_get_device_version(hdev);
> + if (rc)
> + return rc;
> +
> mutex_init(&hdev->lock);
> rc = hccs_get_pcc_chan_id(hdev);
> if (rc)
> @@ -1251,7 +1329,8 @@ static void hccs_remove(struct platform_device *pdev)
> }
>
> static const struct acpi_device_id hccs_acpi_match[] = {
> - { "HISI04B1"},
> + { "HISI04B1", HCCS_HW_DEVICE_V1},
> + { "HISI04B2", HCCS_HW_DEVICE_V2},
> { ""},
Side comment, but
{}
So no content (rely on c initializing it to be zero filled anyway and no comma
as we don't want anything after this point.
> };
> MODULE_DEVICE_TABLE(acpi, hccs_acpi_match);
> diff --git a/drivers/soc/hisilicon/kunpeng_hccs.h b/drivers/soc/hisilicon/kunpeng_hccs.h
> index 6012d2776028..bbb1aada0c6c 100644
> --- a/drivers/soc/hisilicon/kunpeng_hccs.h
> +++ b/drivers/soc/hisilicon/kunpeng_hccs.h
> @@ -51,12 +51,14 @@ struct hccs_mbox_client_info {
> struct pcc_mbox_chan *pcc_chan;
> u64 deadline_us;
> void __iomem *pcc_comm_addr;
> + struct completion done;
> };
>
> struct hccs_dev {
> struct device *dev;
> struct acpi_device *acpi_dev;
> u64 caps;
> + u8 dev_ver;
See above, but I'd rather see
const struct hcc_verspecific_info *info;
here that encodes the differences as callbacks and data and rather than code.
> u8 chip_num;
> struct hccs_chip_info *chips;
> u8 chan_id;
Powered by blists - more mailing lists