Re: [PATCH v2 6/6] soc: hisilicon: kunpeng_hccs: Support low power feature for the specified HCCS type

Jonathan Cameron
2024-08-23T08:58:55Z

On Fri, 23 Aug 2024 11:10:59 +0800
Huisong Li <lihuisong@huawei.com> wrote:

> Add the low power feature for the specified HCCS type by increasing
> and decreasing the used lane number of these HCCS ports on platform.
>
> Signed-off-by: Huisong Li <lihuisong@huawei.com>

Hi Huisong,

A few comments inline, but all minor things.

With at least the "none" string print dropped as it's in an error path
that shouldn't be hit you can add
Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>

The early return comment and whitespace suggestion are things you
can act on if you liek for v2.

Jonathan

> ---
> .../sysfs-devices-platform-kunpeng_hccs | 37 ++
> drivers/soc/hisilicon/Kconfig | 7 +-
> drivers/soc/hisilicon/kunpeng_hccs.c | 378 +++++++++++++++++-
> drivers/soc/hisilicon/kunpeng_hccs.h | 14 +
> 4 files changed, 433 insertions(+), 3 deletions(-)
>
> diff --git a/Documentation/ABI/testing/sysfs-devices-platform-kunpeng_hccs b/Documentation/ABI/testing/sysfs-devices-platform-kunpeng_hccs
> index d4c355e0e0bb..d1b3a95a5518 100644
> --- a/Documentation/ABI/testing/sysfs-devices-platform-kunpeng_hccs
> +++ b/Documentation/ABI/testing/sysfs-devices-platform-kunpeng_hccs
> @@ -87,3 +87,40 @@ Contact: Huisong Li <lihuisong@huawei.com>
> Description:
> This interface is used to show all HCCS types used on the
> platform, like, HCCS-v1, HCCS-v2 and so on.
> +
> +What: /sys/devices/platform/HISI04Bx:00/available_inc_dec_lane_types
> +What: /sys/devices/platform/HISI04Bx:00/dec_lane_of_type
> +What: /sys/devices/platform/HISI04Bx:00/inc_lane_of_type
> +Date: August 2024
> +KernelVersion: 6.12
> +Contact: Huisong Li <lihuisong@huawei.com>
> +Description:
> + These interfaces under /sys/devices/platform/HISI04Bx/ are
> + used to support the low power consumption feature of some
> + HCCS types by changing the number of lanes used. The interfaces
> + changing the number of lanes used are 'dec_lane_of_type' and
> + 'inc_lane_of_type' which require root privileges. These
> + interfaces aren't exposed if no HCCS type on platform support
> + this feature. Please note that decreasing lane number is only
> + allowed if all the specified HCCS ports are not busy.
> +
> + The low power consumption interfaces are as follows:
> +
> + ============================= ==== ================================
> + available_inc_dec_lane_types: (RO) available HCCS types (string) to
> + increase and decrease the number
> + of lane used, e.g. HCCS-v2.

See below. There is an apparent value of 'none' available, but I think in reality the
interface doesn't exist if that is present. So drop it as it will just cause confusion.

> + dec_lane_of_type: (WO) input HCCS type supported
> + decreasing lane to decrease the
> + used lane number of all specified
> + HCCS type ports on platform to
> + the minimum.
> + You can query the 'cur_lane_num'
> + to get the minimum lane number
> + after executing successfully.
> + inc_lane_of_type: (WO) input HCCS type supported
> + increasing lane to increase the
> + used lane number of all specified
> + HCCS type ports on platform to
> + the full lane state.
> + ============================= ==== ================================

> +static int hccs_wait_serdes_adapt_completed(struct hccs_dev *hdev, u8 type)
> +{
> +#define HCCS_MAX_WAIT_CNT_FOR_ADAPT 10
> +#define HCCS_QUERY_ADAPT_RES_DELAY_MS 100
> +#define HCCS_SERDES_ADAPT_OK 0
> +
> + struct hccs_inc_lane_req_param *req_param;
> + u8 wait_cnt = HCCS_MAX_WAIT_CNT_FOR_ADAPT;
> + struct hccs_desc desc;
> + u8 adapt_res;
> + int ret;
> +
> + do {
> + hccs_init_req_desc(&desc);
> + req_param = (struct hccs_inc_lane_req_param *)desc.req.data;
> + req_param->port_type = type;
> + req_param->opt_type = HCCS_GET_ADAPT_RES;
> + ret = hccs_pcc_cmd_send(hdev, HCCS_PM_INC_LANE, &desc);
> + if (ret) {
> + dev_err(hdev->dev, "query adapting result failed, ret = %d.\n",
> + ret);
> + return ret;
> + }
> + adapt_res = *((u8 *)&desc.rsp.data);
> + if (adapt_res == HCCS_SERDES_ADAPT_OK)
> + break;

return 0; here perhaps?

> +
> + msleep(HCCS_QUERY_ADAPT_RES_DELAY_MS);
> + } while (--wait_cnt);
> +
> + if (adapt_res != HCCS_SERDES_ADAPT_OK) {

With above early exit in good path, this can be unconditional perhaps?

> + dev_err(hdev->dev, "wait for adapting completed timeout.\n");
> + return -ETIMEDOUT;
> + }
> +
> + return ret;
> +}

> +static ssize_t inc_lane_of_type_store(struct kobject *kobj, struct kobj_attribute *attr,
> + const char *buf, size_t count)
> +{
> + struct hccs_dev *hdev = device_kobj_to_hccs_dev(kobj);
> + bool full_lane;
> + u8 port_type;
> + int ret;
> +
> + ret = hccs_parse_pm_port_type(hdev, buf, &port_type);
> + if (ret)
> + return ret;
> +
> + mutex_lock(&hdev->lock);

Another comment for a future patch series perhaps.

guard(mutex)(&hdev->lock); in all these will make the code quite a bit cleaner.

> + ret = hccs_get_all_spec_port_full_lane_sta(hdev, port_type, &full_lane);
> + if (ret || full_lane)
> + goto out;
> +
> + ret = hccs_start_inc_lane(hdev, port_type);
> +out:
> + mutex_unlock(&hdev->lock);
> + return ret == 0 ? count : ret;
> +}
> +static struct kobj_attribute inc_lane_of_type_attr =
> + __ATTR(inc_lane_of_type, 0200, NULL, inc_lane_of_type_store);
> +
> +static ssize_t available_inc_dec_lane_types_show(struct kobject *kobj,
> + struct kobj_attribute *attr,
> + char *buf)
> +{
> + struct hccs_dev *hdev = device_kobj_to_hccs_dev(kobj);
> +
> + if (hdev->caps & HCCS_CAPS_HCCS_V2_PM)
> + return sysfs_emit(buf, "%s\n",
> + hccs_port_type_to_name(hdev, HCCS_V2));
> +
> + return sysfs_emit(buf, "%s\n", "none");

Can we get here? I thought this was only registered if the condition
above is true?

Maybe worth keeping a fallback here as a code hardening measure, but
perhaps return -EINVAL; is fine?

> +}
> +static struct kobj_attribute available_inc_dec_lane_types_attr =
> + __ATTR(available_inc_dec_lane_types, 0444,
> + available_inc_dec_lane_types_show, NULL);
>
> static ssize_t used_types_show(struct kobject *kobj,
> struct kobj_attribute *attr, char *buf)
> @@ -1215,11 +1553,49 @@ static struct kobj_attribute used_types_attr =
> static void hccs_remove_misc_sysfs(struct hccs_dev *hdev)
> {
> sysfs_remove_file(&hdev->dev->kobj, &used_types_attr.attr);
> +
> + if (!(hdev->caps & HCCS_CAPS_HCCS_V2_PM))
> + return;
> +
> + sysfs_remove_file(&hdev->dev->kobj,
> + &available_inc_dec_lane_types_attr.attr);
> + sysfs_remove_file(&hdev->dev->kobj, &dec_lane_of_type_attr.attr);
> + sysfs_remove_file(&hdev->dev->kobj, &inc_lane_of_type_attr.attr);
> }
>
> static int hccs_add_misc_sysfs(struct hccs_dev *hdev)
> {
> - return sysfs_create_file(&hdev->dev->kobj, &used_types_attr.attr);
> + int ret;
> +
> + ret = sysfs_create_file(&hdev->dev->kobj, &used_types_attr.attr);
> + if (ret)
> + return ret;
> +
> + if (!(hdev->caps & HCCS_CAPS_HCCS_V2_PM))
> + return 0;
> +
> + ret = sysfs_create_file(&hdev->dev->kobj,
> + &available_inc_dec_lane_types_attr.attr);
> + if (ret)
> + goto used_types_remove;
> +
> + ret = sysfs_create_file(&hdev->dev->kobj, &dec_lane_of_type_attr.attr);
> + if (ret)
> + goto inc_dec_lane_types_remove;

I can sort of see why no line break makes some sense here given these
two files are closely related, but I'd still add one here as I think
visual consistency is more important for readability reasons.

> + ret = sysfs_create_file(&hdev->dev->kobj, &inc_lane_of_type_attr.attr);
> + if (ret)
> + goto dec_lane_of_type_remove;
> +
> + return 0;
> +
> +dec_lane_of_type_remove:
> + sysfs_remove_file(&hdev->dev->kobj, &dec_lane_of_type_attr.attr);
> +inc_dec_lane_types_remove:
> + sysfs_remove_file(&hdev->dev->kobj,
> + &available_inc_dec_lane_types_attr.attr);
> +used_types_remove:
> + sysfs_remove_file(&hdev->dev->kobj, &used_types_attr.attr);
> + return ret;
> }
>
> static void hccs_remove_die_dir(struct hccs_die_info *die)