Re: [PATCH 08/10] platform/x86/amd/hsmp: Move read and is_visible to respective files

[Mario Limonciello <mario.limonciello@amd.com>]

On 6/27/2024 00:39, Suma Hegde wrote:
> The .read() and .is_visibile() needs to be handled differently in acpi and

is_visible()

> 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.
> 
> Signed-off-by: Suma Hegde <suma.hegde@amd.com>
> Reviewed-by: Naveen Krishna Chatradhi <naveenkrishna.chatradhi@amd.com>
> ---
>   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 0307f4e7176d..1ea17aa296c7 100644
> --- a/drivers/platform/x86/amd/hsmp/acpi.c
> +++ b/drivers/platform/x86/amd/hsmp/acpi.c
> @@ -12,6 +12,7 @@
>   #include "hsmp.h"
>   
>   #include <linux/acpi.h>
> +#include <asm/amd_hsmp.h>
>   #include <asm/amd_nb.h>
>   #include <linux/platform_device.h>
>   
> @@ -206,6 +207,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)
> @@ -238,6 +241,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 (plat_dev.proto_ver == HSMP_PROTO_VER6)
> +		return battr->attr.mode;
> +	else

Since your only path in the "if" returns this else is redundant.

> +		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 4bf598021f4a..c199a0ff457d 100644
> --- a/drivers/platform/x86/amd/hsmp/hsmp.c
> +++ b/drivers/platform/x86/amd/hsmp/hsmp.c
> @@ -273,34 +273,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 = &plat_dev.sock[sock_ind];
> @@ -334,15 +306,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 (plat_dev.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 = &plat_dev.sock[sock_ind].hsmp_attr;
> diff --git a/drivers/platform/x86/amd/hsmp/plat.c b/drivers/platform/x86/amd/hsmp/plat.c
> index 62423581d839..57aa64b18e0d 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/module.h>
>   #include <linux/pci.h>
> @@ -88,6 +89,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 >= plat_dev.num_sockets)
> +		return -EINVAL;
> +
> +	sock = &plat_dev.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 >= plat_dev.num_sockets)
> +		return SYSFS_GROUP_INVISIBLE;
> +
> +	if (plat_dev.proto_ver == HSMP_PROTO_VER6)
> +		return battr->attr.mode;
> +	else
> +		return 0;

Since your only path in the "if" returns this else is redundant.

> +}
> +
>   static inline bool is_f1a_m0h(void)
>   {
>   	if (boot_cpu_data.x86 == 0x1A && boot_cpu_data.x86_model <= 0x0F)