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

["lihuisong (C)" <lihuisong@huawei.com>]

Hi Jonathan,

Thanks for your review again.
Your proposal are good and are also more worth to enhance code.
How about use guard() for all sysfs interface in furture patch?
I want to support this feature first.

Huisong


在 2024/8/23 16:58, Jonathan Cameron 写道:
> 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
You are correct.
> 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.
Ack
>
>> +		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?

It's ok. And then we can directly return failure if timeout.

>
>> +
>> +		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?
Yes
>
>> +		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.
This is a good way. very nice and simple.
But many sysfs interfaces in this driver have used mutex_lock/mutex_unlock.
So is it better for us to keep the same mutex lock way in this patch and 
use guard() for all sysfs interface in furture patch?
>> +	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?
Ack
>
>
>> +}
>> +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.
Ack
>
>> +	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)
> .