Re: [v3 09/11] platform/x86/amd/hsmp: Move read() and is_bin_visible() to respective files

["Ilpo Järvinen" <ilpo.jarvinen@linux.intel.com>]

On Sat, 20 Jul 2024, Suma Hegde wrote:

> The .read() and .is_bin_visibile() need to be handled differently in acpi

ACPI

> and platform drivers, due to the way the sysfs files are created.
> 
> This is in preparation to using .dev_groups instead of dynamic sysfs
> creation. The sysfs at this point is not functional, it will be enabled in
> the next patch.

What does the last sentence mean? If it means that after this commit (w/o 
the next one), sysfs is no longer working, it's not acceptable way to 
construct a patch series. We never intentionally split patches such that 
there's breakage/loss of features in the intermediate state. Thus, 
reconsider how to split things differently so nothing breaks (if nothing 
else helps, just merge the two patches together, this series is already 
much better than the one large move changes was so it shouldn't be a big 
problem).

...If it means something else, you probably should rephrase the sentence.

-- 
 i.

> Signed-off-by: Suma Hegde <suma.hegde@amd.com>
> Reviewed-by: Naveen Krishna Chatradhi <naveenkrishna.chatradhi@amd.com>
> ---
> Changes since v2:
> None
> Changes since v1:
> 1. Change is_visible to is_bin_visible in commit message
> 1. Add parenthesis around read and is_bin_visible in commit message
> 2. Change plat_dev to hsmp_pdev, hsmp_device to mdev
> 3. Remove unnecessary if, else conditions in hsmp_is_sock_attr_visible
> 4. Change un cached to un-cached
> 
>  drivers/platform/x86/amd/hsmp/acpi.c | 41 ++++++++++++++++++++
>  drivers/platform/x86/amd/hsmp/hsmp.c | 37 ------------------
>  drivers/platform/x86/amd/hsmp/plat.c | 57 ++++++++++++++++++++++++++++
>  3 files changed, 98 insertions(+), 37 deletions(-)
> 
> diff --git a/drivers/platform/x86/amd/hsmp/acpi.c b/drivers/platform/x86/amd/hsmp/acpi.c
> index 86100943aadc..7cb38c8dc627 100644
> --- a/drivers/platform/x86/amd/hsmp/acpi.c
> +++ b/drivers/platform/x86/amd/hsmp/acpi.c
> @@ -11,6 +11,7 @@
>  
>  #include "hsmp.h"
>  
> +#include <asm/amd_hsmp.h>
>  #include <asm/amd_nb.h>
>  
>  #include <linux/acpi.h>
> @@ -214,6 +215,8 @@ static int hsmp_parse_acpi_table(struct device *dev, u16 sock_ind)
>  
>  	sema_init(&sock->hsmp_sem, 1);
>  
> +	dev_set_drvdata(dev, sock);
> +
>  	/* Read MP1 base address from CRS method */
>  	ret = hsmp_read_acpi_crs(sock);
>  	if (ret)
> @@ -246,6 +249,44 @@ static int hsmp_create_acpi_sysfs_if(struct device *dev)
>  	return devm_device_add_group(dev, attr_grp);
>  }
>  
> +ssize_t hsmp_metric_tbl_read(struct file *filp, struct kobject *kobj,
> +			     struct bin_attribute *bin_attr, char *buf,
> +			     loff_t off, size_t count)
> +{
> +	struct device *dev = container_of(kobj, struct device, kobj);
> +	struct hsmp_socket *sock = dev_get_drvdata(dev);
> +	struct hsmp_message msg = { 0 };
> +	int ret;
> +
> +	if (!sock)
> +		return -EINVAL;
> +
> +	/* Do not support lseek(), reads entire metric table */
> +	if (count < bin_attr->size) {
> +		dev_err(sock->dev, "Wrong buffer size\n");
> +		return -EINVAL;
> +	}
> +
> +	msg.msg_id      = HSMP_GET_METRIC_TABLE;
> +	msg.sock_ind    = sock->sock_ind;
> +
> +	ret = hsmp_send_message(&msg);
> +	if (ret)
> +		return ret;
> +	memcpy_fromio(buf, sock->metric_tbl_addr, bin_attr->size);
> +
> +	return bin_attr->size;
> +}
> +
> +umode_t hsmp_is_sock_attr_visible(struct kobject *kobj,
> +				  struct bin_attribute *battr, int id)
> +{
> +	if (hsmp_pdev.proto_ver == HSMP_PROTO_VER6)
> +		return battr->attr.mode;
> +
> +	return 0;
> +}
> +
>  static int init_acpi(struct device *dev)
>  {
>  	u16 sock_ind;
> diff --git a/drivers/platform/x86/amd/hsmp/hsmp.c b/drivers/platform/x86/amd/hsmp/hsmp.c
> index 119a1f8895ca..c906723ae2f2 100644
> --- a/drivers/platform/x86/amd/hsmp/hsmp.c
> +++ b/drivers/platform/x86/amd/hsmp/hsmp.c
> @@ -274,34 +274,6 @@ long hsmp_ioctl(struct file *fp, unsigned int cmd, unsigned long arg)
>  	return 0;
>  }
>  
> -ssize_t hsmp_metric_tbl_read(struct file *filp, struct kobject *kobj,
> -			     struct bin_attribute *bin_attr, char *buf,
> -			     loff_t off, size_t count)
> -{
> -	struct hsmp_socket *sock = bin_attr->private;
> -	struct hsmp_message msg = { 0 };
> -	int ret;
> -
> -	if (!sock)
> -		return -EINVAL;
> -
> -	/* Do not support lseek(), reads entire metric table */
> -	if (count < bin_attr->size) {
> -		dev_err(sock->dev, "Wrong buffer size\n");
> -		return -EINVAL;
> -	}
> -
> -	msg.msg_id	= HSMP_GET_METRIC_TABLE;
> -	msg.sock_ind	= sock->sock_ind;
> -
> -	ret = hsmp_send_message(&msg);
> -	if (ret)
> -		return ret;
> -	memcpy_fromio(buf, sock->metric_tbl_addr, bin_attr->size);
> -
> -	return bin_attr->size;
> -}
> -
>  static int hsmp_get_tbl_dram_base(u16 sock_ind)
>  {
>  	struct hsmp_socket *sock = &hsmp_pdev.sock[sock_ind];
> @@ -335,15 +307,6 @@ static int hsmp_get_tbl_dram_base(u16 sock_ind)
>  	return 0;
>  }
>  
> -umode_t hsmp_is_sock_attr_visible(struct kobject *kobj,
> -				  struct bin_attribute *battr, int id)
> -{
> -	if (hsmp_pdev.proto_ver == HSMP_PROTO_VER6)
> -		return battr->attr.mode;
> -	else
> -		return 0;
> -}
> -
>  static int hsmp_init_metric_tbl_bin_attr(struct bin_attribute **hattrs, u16 sock_ind)
>  {
>  	struct bin_attribute *hattr = &hsmp_pdev.sock[sock_ind].hsmp_attr;
> diff --git a/drivers/platform/x86/amd/hsmp/plat.c b/drivers/platform/x86/amd/hsmp/plat.c
> index 3bce2c570f2b..c2b83363713f 100644
> --- a/drivers/platform/x86/amd/hsmp/plat.c
> +++ b/drivers/platform/x86/amd/hsmp/plat.c
> @@ -11,6 +11,7 @@
>  
>  #include "hsmp.h"
>  
> +#include <asm/amd_hsmp.h>
>  #include <asm/amd_nb.h>
>  
>  #include <linux/device.h>
> @@ -90,6 +91,62 @@ static int hsmp_create_non_acpi_sysfs_if(struct device *dev)
>  	return device_add_groups(dev, hsmp_attr_grps);
>  }
>  
> +ssize_t hsmp_metric_tbl_read(struct file *filp, struct kobject *kobj,
> +			     struct bin_attribute *bin_attr, char *buf,
> +			     loff_t off, size_t count)
> +{
> +	struct hsmp_message msg = { 0 };
> +	struct hsmp_socket *sock;
> +	u8 sock_ind;
> +	int ret;
> +
> +	ret = kstrtou8(bin_attr->private, 10, &sock_ind);
> +	if (ret)
> +		return ret;
> +
> +	if (sock_ind >= hsmp_pdev.num_sockets)
> +		return -EINVAL;
> +
> +	sock = &hsmp_pdev.sock[sock_ind];
> +	if (!sock)
> +		return -EINVAL;
> +
> +	/* Do not support lseek(), reads entire metric table */
> +	if (count < bin_attr->size) {
> +		dev_err(sock->dev, "Wrong buffer size\n");
> +		return -EINVAL;
> +	}
> +
> +	msg.msg_id	= HSMP_GET_METRIC_TABLE;
> +	msg.sock_ind	= sock_ind;
> +
> +	ret = hsmp_send_message(&msg);
> +	if (ret)
> +		return ret;
> +	memcpy_fromio(buf, sock->metric_tbl_addr, bin_attr->size);
> +
> +	return bin_attr->size;
> +}
> +
> +umode_t hsmp_is_sock_attr_visible(struct kobject *kobj,
> +				  struct bin_attribute *battr, int id)
> +{
> +	u8 sock_ind;
> +	int ret;
> +
> +	ret = kstrtou8(battr->private, 10, &sock_ind);
> +	if (ret)
> +		return ret;
> +
> +	if (id == 0 && sock_ind >= hsmp_pdev.num_sockets)
> +		return SYSFS_GROUP_INVISIBLE;
> +
> +	if (hsmp_pdev.proto_ver == HSMP_PROTO_VER6)
> +		return battr->attr.mode;
> +
> +	return 0;
> +}
> +
>  static inline bool is_f1a_m0h(void)
>  {
>  	if (boot_cpu_data.x86 == 0x1A && boot_cpu_data.x86_model <= 0x0F)
>