Re: [UPDATE][PATCH] ACPI / fan: Display fan performance state information

[Rafael J. Wysocki]

On Thursday, December 5, 2019 2:23:09 AM CET Srinivas Pandruvada wrote:
> When _FPS object indicates support of variable speed fan, thermal cooling
> devices for fan shows max performance state count using attribute
> "max_state" greater than or equal to 1.
> 
> But the thermal cooling device doesn't display properties of each
> performance state. This is not enough for smart fan control user space
> software, which also considers speed, power and noise level.
> 
> This change presents fan performance states attributes under acpi
> device for the fan. This will be under:
> /sys/bus/acpi/devices/devices/INT3404:00
> or
> /sys/bus/platform/devices/PNP0C0B:00.
> 
> For more information refer to:
> Documentation/acpi/fan_performance_states.txt
> 
> Signed-off-by: Srinivas Pandruvada <srinivas.pandruvada@xxxxxxxxxxxxxxx>
> Tested-by: Sumeet Pawnikar <sumeet.r.pawnikar@xxxxxxxxx>
> ---
> updated version:
> 	Removed one unnecessary platoform_device* conversion from
> 	acpi_device* in the function argument.
> 
>  Documentation/acpi/fan_performance_states.txt |  39 +++++++

Documentation/acpi/ is not there any more.

It looks like this could go into admin-guide/acpi/ in the RST format.

>  drivers/acpi/fan.c                            | 106 ++++++++++++++++--
>  2 files changed, 138 insertions(+), 7 deletions(-)
>  create mode 100644 Documentation/acpi/fan_performance_states.txt
> 
> diff --git a/Documentation/acpi/fan_performance_states.txt b/Documentation/acpi/fan_performance_states.txt
> new file mode 100644
> index 000000000000..06fd8eb002f2
> --- /dev/null
> +++ b/Documentation/acpi/fan_performance_states.txt
> @@ -0,0 +1,39 @@
> +When the optional object _FPS is present for ACPI devices PNP0C0B or INT3404,
> +additional attributes are displayed under acpi device object. These attributes
> +display information about each performance state.
> +
> +For example
> +$ ls /sys/bus/acpi/devices/INT3404\:00
> +description        fan_perf_state_11  fan_perf_state_5  fan_perf_state_9  physical_node  uevent
> +fan_perf_state_0   fan_perf_state_2   fan_perf_state_6  hid               power          uid
> +fan_perf_state_1   fan_perf_state_3   fan_perf_state_7  modalias          status         wakeup
> +fan_perf_state_10  fan_perf_state_4   fan_perf_state_8  path              subsystem

This looks kind of messy IMO.

I would drop the "fan_perf_" prefix and the underline between "state" and the
number, so they become "state0" through "state11".

There shouldn't be any confusion related to that.

> +
> +Each fan_perf_state_* contains the information about the fields for each state as
> +defined by the ACPI specification.
> +
> +For example
> +$ grep . /sys/bus/acpi/devices/INT3404\:00/fan_perf_state_1/*
> +/sys/bus/acpi/devices/INT3404:00/fan_perf_state_1/control_percent:25
> +/sys/bus/acpi/devices/INT3404:00/fan_perf_state_1/noise_level_mdb:12500
> +/sys/bus/acpi/devices/INT3404:00/fan_perf_state_1/power_mw:1250
> +/sys/bus/acpi/devices/INT3404:00/fan_perf_state_1/speed_rpm:3200
> +/sys/bus/acpi/devices/INT3404:00/fan_perf_state_1/trip_point_index:invalid

Alternatively, the states could be represented as individual files each
in the

control_percent:noise_level_mdb:power_mw:speed_rpm:trip_point_index

format (ie. colon-separated list of values).  Then you'd not need to
use the new ktype etc.

> +
> +Fields
> +control_percent: Indicates the value to be used to set the fan speed to a
> +specific level using the _FSL object. The value here is from 0-100 percent.
> +
> +noise_level_mdb: Indicates the audible noise emitted by the fan. By the
> +specification the value represents the noise in 10ths of decibels. Here
> +it is multiplied with 100 to present in milli-db, to avoid loss of
> +precision. When not populated, "not-defined" is displayed.
> +
> +speed_rpm: Indicates the speed of the fan in revolutions per minute.
> +
> +power_mw: Indicates the power consumption in milliwatts. When not populated,
> +"not-defined" is displayed.
> +
> +trip_point_index: The active cooling trip point number that corresponds to this
> +performance state. The range is from 0-9. For any other values, "invalid" is
> +be displayed.
> diff --git a/drivers/acpi/fan.c b/drivers/acpi/fan.c
> index 816b0803f7fb..f5e9f67e6a5f 100644
> --- a/drivers/acpi/fan.c
> +++ b/drivers/acpi/fan.c
> @@ -50,6 +50,7 @@ struct acpi_fan_fps {
>  	u64 speed;
>  	u64 noise_level;
>  	u64 power;
> +	struct kobject kobj;
>  };
>  
>  struct acpi_fan_fif {
> @@ -265,6 +266,64 @@ static int acpi_fan_speed_cmp(const void *a, const void *b)
>  	return fps1->speed - fps2->speed;
>  }
>  
> +#define to_fps_state(k) container_of(k, struct acpi_fan_fps, kobj)
> +
> +#define DEFINE_ONE_FPS_ATTR_RO(_name)\
> +	static struct kobj_attribute _name =\
> +		__ATTR(_name, 0444, show_##_name, NULL)
> +
> +#define FPS_INFO_SHOW(name, object, multiplier)\
> +static ssize_t show_##name(struct kobject *kobj,\
> +			   struct kobj_attribute *attr,\
> +			   char *buf)\
> +{\
> +	struct acpi_fan_fps *fps = to_fps_state(kobj);\
> +\
> +	if (fps->object == 0xFFFFFFFF)\
> +		return sprintf(buf, "not-defined\n");\
> +\
> +	return sprintf(buf, "%llu\n", multiplier * fps->object);\
> +}
> +
> +FPS_INFO_SHOW(control_percent, control, 1)
> +FPS_INFO_SHOW(speed_rpm, speed, 1)
> +FPS_INFO_SHOW(power_mw, power, 1)
> +FPS_INFO_SHOW(noise_level_mdb, noise_level, 100)
> +
> +static ssize_t show_trip_point_index(struct kobject *kobj,
> +				     struct kobj_attribute *attr,
> +				     char *buf)
> +{
> +	struct acpi_fan_fps *fps = to_fps_state(kobj);
> +
> +	if (fps->trip_point > 9)
> +		return sprintf(buf, "invalid\n");
> +
> +	return sprintf(buf, "%llu\n", fps->trip_point);
> +}
> +
> +DEFINE_ONE_FPS_ATTR_RO(control_percent);
> +DEFINE_ONE_FPS_ATTR_RO(trip_point_index);
> +DEFINE_ONE_FPS_ATTR_RO(speed_rpm);
> +DEFINE_ONE_FPS_ATTR_RO(noise_level_mdb);
> +DEFINE_ONE_FPS_ATTR_RO(power_mw);
> +
> +static struct attribute *fps_attrs[] = {
> +	&control_percent.attr,
> +	&trip_point_index.attr,
> +	&speed_rpm.attr,
> +	&noise_level_mdb.attr,
> +	&power_mw.attr,
> +	NULL
> +};
> +
> +static struct kobj_type fps_ktype = {
> +	.sysfs_ops = &kobj_sysfs_ops,
> +	.default_attrs = fps_attrs,
> +};
> +
> +#define ACPI_FPS_NAME_LEN	20
> +
>  static int acpi_fan_get_fps(struct acpi_device *device)
>  {
>  	struct acpi_fan *fan = acpi_driver_data(device);
> @@ -295,12 +354,13 @@ static int acpi_fan_get_fps(struct acpi_device *device)
>  	}
>  	for (i = 0; i < fan->fps_count; i++) {
>  		struct acpi_buffer format = { sizeof("NNNNN"), "NNNNN" };
> -		struct acpi_buffer fps = { sizeof(fan->fps[i]), &fan->fps[i] };
> +		struct acpi_buffer fps = { offsetof(struct acpi_fan_fps, kobj),
> +						&fan->fps[i] };
>  		status = acpi_extract_package(&obj->package.elements[i + 1],
>  					      &format, &fps);
>  		if (ACPI_FAILURE(status)) {
>  			dev_err(&device->dev, "Invalid _FPS element\n");
> -			break;
> +			goto err;
>  		}
>  	}
>  
> @@ -308,6 +368,21 @@ static int acpi_fan_get_fps(struct acpi_device *device)
>  	sort(fan->fps, fan->fps_count, sizeof(*fan->fps),
>  	     acpi_fan_speed_cmp, NULL);
>  
> +	for (i = 0; i < fan->fps_count; ++i) {
> +		char name[ACPI_FPS_NAME_LEN];
> +
> +		snprintf(name, ACPI_FPS_NAME_LEN, "fan_perf_state_%d", i);
> +		status = kobject_init_and_add(&fan->fps[i].kobj, &fps_ktype,
> +					      &device->dev.kobj, name);
> +		if (status) {
> +			int j;
> +
> +			for (j = 0; j < i; ++j)
> +				kobject_put(&fan->fps[j].kobj);
> +			break;
> +		}
> +	}
> +
>  err:
>  	kfree(obj);
>  	return status;
> @@ -331,13 +406,13 @@ static int acpi_fan_probe(struct platform_device *pdev)
>  
>  	if (acpi_fan_is_acpi4(device)) {
>  		if (acpi_fan_get_fif(device) || acpi_fan_get_fps(device))
> -			goto end;
> +			goto err_end;
>  		fan->acpi4 = true;
>  	} else {
>  		result = acpi_device_update_power(device, NULL);
>  		if (result) {
>  			dev_err(&device->dev, "Failed to set initial power state\n");
> -			goto end;
> +			goto err_end;
>  		}
>  	}
>  
> @@ -350,7 +425,7 @@ static int acpi_fan_probe(struct platform_device *pdev)
>  						&fan_cooling_ops);
>  	if (IS_ERR(cdev)) {
>  		result = PTR_ERR(cdev);
> -		goto end;
> +		goto err_end;
>  	}
>  
>  	dev_dbg(&pdev->dev, "registered as cooling_device%d\n", cdev->id);
> @@ -365,10 +440,21 @@ static int acpi_fan_probe(struct platform_device *pdev)
>  	result = sysfs_create_link(&cdev->device.kobj,
>  				   &pdev->dev.kobj,
>  				   "device");
> -	if (result)
> +	if (result) {
>  		dev_err(&pdev->dev, "Failed to create sysfs link 'device'\n");
> +		goto err_end;
> +	}
> +
> +	return 0;
> +
> +err_end:
> +	if (fan->acpi4) {
> +		int i;
> +
> +		for (i = 0; i < fan->fps_count; ++i)
> +			kobject_put(&fan->fps[i].kobj);
> +	}
>  
> -end:
>  	return result;
>  }
>  
> @@ -376,6 +462,12 @@ static int acpi_fan_remove(struct platform_device *pdev)
>  {
>  	struct acpi_fan *fan = platform_get_drvdata(pdev);
>  
> +	if (fan->acpi4) {
> +		int i;
> +
> +		for (i = 0; i < fan->fps_count; ++i)
> +			kobject_put(&fan->fps[i].kobj);
> +	}
>  	sysfs_remove_link(&pdev->dev.kobj, "thermal_cooling");
>  	sysfs_remove_link(&fan->cdev->device.kobj, "device");
>  	thermal_cooling_device_unregister(fan->cdev);
>