Re: [PATCH 3/3] k10temp: Add power sensor for family 19h

[Guenter Roeck <linux@roeck-us.net>]

On Thu, Sep 02, 2021 at 11:11:55PM +0530, Naveen Krishna Chatradhi wrote:
> On newer Fam19h server line of AMD processors a new channel
> is created to report and manage socket power limits.
> 
> This patch creates hwmon_power sensor to report current power,
> power cap and max power cap of the socket.
> While at it, update the k10temp documentation
> 
> Tested-by: suma hegde <suma.hegde@amd.com>
> Signed-off-by: Naveen Krishna Chatradhi <nchatrad@amd.com>
> Cc: Guenter Roeck <linux@roeck-us.net>

Would it make sense to add this to fam15h_power instead ?
Just wondering, no strong opinion. That driver does seem to be
a bit messy, so it might be best to leave it alone.

One other question, though: We had to drop the amd_energy driver
because it opened up an attack surface. Is the power consumption
reported by this patch vague enough that this won't be a problem ?

Thanks,
Guenter

> ---
>  Documentation/hwmon/k10temp.rst |  18 ++++++
>  drivers/hwmon/k10temp.c         | 106 ++++++++++++++++++++++++++++++++
>  2 files changed, 124 insertions(+)
> 
> diff --git a/Documentation/hwmon/k10temp.rst b/Documentation/hwmon/k10temp.rst
> index 91b99adc6c48..d5a78a7b6ca5 100644
> --- a/Documentation/hwmon/k10temp.rst
> +++ b/Documentation/hwmon/k10temp.rst
> @@ -132,3 +132,21 @@ On Family 17h and Family 18h CPUs, additional temperature sensors may report
>  Core Complex Die (CCD) temperatures. Up to 8 such temperatures are reported
>  as temp{3..10}_input, labeled Tccd{1..8}. Actual support depends on the CPU
>  variant.
> +
> +On Family 19h server line of CPUs, additionally driver may report socket
> +current power consumption with power cap and power cap max. This requires the
> +HSMP support exported in the amd_nb module.
> +
> +The power1_cap can be set to any value, SMU FW will limit the maximum cap to
> +the value reported by power1_cap_max entry. The SMU FW may not take any action
> +if the power1_cap is set to a value lesser than the minimum socket consumption.
> +
> +The following attributes may be reported.
> +
> +================ ===== ========================================================
> +Name             Perm  Description
> +================ ===== ========================================================
> +power1_input     RO    Socket current Power consumed
> +power1_cap       RW    Socket Power limit can be set between 0 and power1_cap_max
> +power1_cap_max   RO    Maximum powerlimit calculated and reported by the SMU FW
> +================ ===== ========================================================
> diff --git a/drivers/hwmon/k10temp.c b/drivers/hwmon/k10temp.c
> index 3618a924e78e..b993fdd94979 100644
> --- a/drivers/hwmon/k10temp.c
> +++ b/drivers/hwmon/k10temp.c
> @@ -105,6 +105,9 @@ struct k10temp_data {
>  	u32 show_temp;
>  	bool is_zen;
>  	u32 ccd_offset;
> +	bool show_power;
> +	char pwr_label[20];
> +	u32 power_cap_max;
>  };
>  
>  #define TCTL_BIT	0
> @@ -197,10 +200,14 @@ static int k10temp_read_labels(struct device *dev,
>  			       enum hwmon_sensor_types type,
>  			       u32 attr, int channel, const char **str)
>  {
> +	struct k10temp_data *data = dev_get_drvdata(dev);
>  	switch (type) {
>  	case hwmon_temp:
>  		*str = k10temp_temp_label[channel];
>  		break;
> +	case hwmon_power:
> +		*str = data->pwr_label;
> +		break;
>  	default:
>  		return -EOPNOTSUPP;
>  	}
> @@ -254,12 +261,43 @@ static int k10temp_read_temp(struct device *dev, u32 attr, int channel,
>  	return 0;
>  }
>  
> +static int k10temp_read_power(struct device *dev, u32 attr, int channel, long *val)
> +{
> +	struct k10temp_data *data = dev_get_drvdata(dev);
> +	struct hsmp_message msg = { 0 };
> +	int err;
> +
> +	switch (attr) {
> +	case hwmon_power_input:
> +		msg.msg_id = HSMP_GET_SOCKET_POWER;
> +		break;
> +	case hwmon_power_cap:
> +		msg.msg_id = HSMP_GET_SOCKET_POWER_LIMIT;
> +		break;
> +	case hwmon_power_cap_max:
> +		/* power_cap_max does not change dynamically, hence return the cached value */
> +		*val = data->power_cap_max * 1000;
> +		return 0;
> +	default:
> +		return -EOPNOTSUPP;
> +	}
> +	msg.response_sz = 1;
> +	err = hsmp_send_message(amd_pci_dev_to_node_id(data->pdev), &msg);
> +	if (!err)
> +		/* power metric is reported in micro watts. hence multiply by 1000 */
> +		*val = msg.response[0] * 1000;
> +
> +	return err;
> +}
> +
>  static int k10temp_read(struct device *dev, enum hwmon_sensor_types type,
>  			u32 attr, int channel, long *val)
>  {
>  	switch (type) {
>  	case hwmon_temp:
>  		return k10temp_read_temp(dev, attr, channel, val);
> +	case hwmon_power:
> +		return k10temp_read_power(dev, attr, channel, val);
>  	default:
>  		return -EOPNOTSUPP;
>  	}
> @@ -308,12 +346,45 @@ static umode_t k10temp_is_visible(const void *_data,
>  			return 0;
>  		}
>  		break;
> +	case hwmon_power:
> +		switch (attr) {
> +		case hwmon_power_input:
> +		case hwmon_power_cap_max:
> +		case hwmon_power_label:
> +			/* Show power attributes only if show_power is available */
> +			if (data->show_power)
> +				break;
> +			return 0;
> +		case hwmon_power_cap:
> +			if (data->show_power)
> +				return 0644;
> +			return 0;
> +		default:
> +			return -EOPNOTSUPP;
> +		}
> +		break;
>  	default:
>  		return 0;
>  	}
>  	return 0444;
>  }
>  
> +static int k10temp_write(struct device *dev, enum hwmon_sensor_types type,
> +			 u32 attr, int channel, long val)
> +{
> +	struct hsmp_message msg = { 0 };
> +	struct k10temp_data *data = dev_get_drvdata(dev);
> +
> +	if (type == hwmon_power && attr == hwmon_power_cap) {
> +		msg.response_sz = 1;
> +		msg.num_args	= 1;
> +		msg.msg_id	= HSMP_SET_SOCKET_POWER_LIMIT;
> +		msg.args[0]	= val / 1000;
> +		return hsmp_send_message(amd_pci_dev_to_node_id(data->pdev), &msg);
> +	}
> +	return -EOPNOTSUPP;
> +}
> +
>  static bool has_erratum_319(struct pci_dev *pdev)
>  {
>  	u32 pkg_type, reg_dram_cfg;
> @@ -362,6 +433,9 @@ static const struct hwmon_channel_info *k10temp_info[] = {
>  			   HWMON_T_INPUT | HWMON_T_LABEL,
>  			   HWMON_T_INPUT | HWMON_T_LABEL,
>  			   HWMON_T_INPUT | HWMON_T_LABEL),
> +	HWMON_CHANNEL_INFO(power,
> +			   HWMON_P_INPUT | HWMON_P_LABEL |
> +			   HWMON_P_CAP | HWMON_P_CAP_MAX),
>  	NULL
>  };
>  
> @@ -369,6 +443,7 @@ static const struct hwmon_ops k10temp_hwmon_ops = {
>  	.is_visible = k10temp_is_visible,
>  	.read = k10temp_read,
>  	.read_string = k10temp_read_labels,
> +	.write = k10temp_write,
>  };
>  
>  static const struct hwmon_chip_info k10temp_chip_info = {
> @@ -390,6 +465,32 @@ static void k10temp_get_ccd_support(struct pci_dev *pdev,
>  	}
>  }
>  
> +static int k10temp_get_max_power(struct k10temp_data *data)
> +{
> +	int err;
> +	struct hsmp_message msg = { 0 };
> +
> +	msg.msg_id = HSMP_GET_SOCKET_POWER_LIMIT_MAX;
> +	msg.response_sz = 1;
> +	err = hsmp_send_message(amd_pci_dev_to_node_id(data->pdev), &msg);
> +	if (!err)
> +		data->power_cap_max = msg.response[0];
> +	return err;
> +}
> +
> +static void check_power_support(struct k10temp_data *data)
> +{
> +	/* HSMP support is required to obtain power metrics */
> +	if (!amd_nb_has_feature(AMD_NB_HSMP))
> +		return;
> +
> +	if (k10temp_get_max_power(data))
> +		return;
> +
> +	sprintf(data->pwr_label, "socket%d_pwr", amd_pci_dev_to_node_id(data->pdev));
> +	data->show_power = true;
> +}
> +
>  static int k10temp_probe(struct pci_dev *pdev, const struct pci_device_id *id)
>  {
>  	int unreliable = has_erratum_319(pdev);
> @@ -448,6 +549,11 @@ static int k10temp_probe(struct pci_dev *pdev, const struct pci_device_id *id)
>  
>  		switch (boot_cpu_data.x86_model) {
>  		case 0x0 ... 0x1:	/* Zen3 SP3/TR */
> +		case 0x30 ... 0x31:
> +			check_power_support(data);
> +			data->ccd_offset = 0x154;
> +			k10temp_get_ccd_support(pdev, data, 8);
> +			break;
>  		case 0x21:		/* Zen3 Ryzen Desktop */
>  		case 0x50 ... 0x5f:	/* Green Sardine */
>  			data->ccd_offset = 0x154;