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

[Suma Hegde <Suma.Hegde@amd.com>]

Hi Mario,

Thank you for your review. I will address these review comments in v2.

On 6/28/2024 1:18 AM, Mario Limonciello wrote:
> 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)

Thanks and Regards,

Suma