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

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

On Tue, 27 Aug 2024 19:48:32 +0800
"lihuisong (C)" <lihuisong@huawei.com> wrote:

> 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.
Sure, that's fine and why I gave an RB tag even with comments.

Thanks,

Jonathan

> 
> 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)  
> > .