Re: [PATCH 1/2] platform/x86/amd: amd_3d_vcache: Add AMD 3D V-Cache optimizer driver

[Basavaraj Natikar <bnatikar@xxxxxxx>]

On 10/11/2024 4:51 PM, Armin Wolf wrote:
> Am 10.10.24 um 11:42 schrieb Basavaraj Natikar:
>
> > AMD X3D processors, also known as AMD 3D V-Cache, feature dual Core
> > Complex Dies (CCDs) and enlarged L3 cache, enabling dynamic mode
> > switching between Frequency and Cache modes. To optimize performance,
> > implement the AMD 3D V-Cache Optimizer, which allows selecting either:
> >
> > Frequency mode: cores within the faster CCD are prioritized before
> > those in the slower CCD.
> >
> > Cache mode: cores within the larger L3 CCD are prioritized before
> > those in the smaller L3 CCD.
> >
> > Co-developed-by: Perry Yuan <perry.yuan@xxxxxxx>
> > Signed-off-by: Perry Yuan <perry.yuan@xxxxxxx>
> > Co-developed-by: Mario Limonciello <mario.limonciello@xxxxxxx>
> > Signed-off-by: Mario Limonciello <mario.limonciello@xxxxxxx>
> > Reviewed-by: Shyam Sundar S K <Shyam-sundar.S-k@xxxxxxx>
> > Signed-off-by: Basavaraj Natikar <Basavaraj.Natikar@xxxxxxx>
> > ---
> >   MAINTAINERS                           |   7 +
> >   drivers/platform/x86/amd/Kconfig      |  12 ++
> >   drivers/platform/x86/amd/Makefile     |   2 +
> >   drivers/platform/x86/amd/x3d_vcache.c | 193 ++++++++++++++++++++++++++
> >   4 files changed, 214 insertions(+)
> >   create mode 100644 drivers/platform/x86/amd/x3d_vcache.c
> >
> > diff --git a/MAINTAINERS b/MAINTAINERS
> > index a097afd76ded..61cb6a294f4c 100644
> > --- a/MAINTAINERS
> > +++ b/MAINTAINERS
> > @@ -972,6 +972,13 @@ Q: 
> > https://patchwork.kernel.org/project/linux-rdma/list/
> >   F:    drivers/infiniband/hw/efa/
> >   F:    include/uapi/rdma/efa-abi.h
> >
> > +AMD 3D V-CACHE PERFORMANCE OPTIMIZER DRIVER
> > +M:    Basavaraj Natikar <Basavaraj.Natikar@xxxxxxx>
> > +R:    Mario Limonciello <mario.limonciello@xxxxxxx>
> > +L:    platform-driver-x86@xxxxxxxxxxxxxxx
> > +S:    Supported
> > +F:    drivers/platform/x86/amd/x3d_vcache.c
> > +
> >   AMD ADDRESS TRANSLATION LIBRARY (ATL)
> >   M:    Yazen Ghannam <Yazen.Ghannam@xxxxxxx>
> >   L:    linux-edac@xxxxxxxxxxxxxxx
> > diff --git a/drivers/platform/x86/amd/Kconfig 
> > b/drivers/platform/x86/amd/Kconfig
> > index f88682d36447..d73f691020d0 100644
> > --- a/drivers/platform/x86/amd/Kconfig
> > +++ b/drivers/platform/x86/amd/Kconfig
> > @@ -6,6 +6,18 @@
> >   source "drivers/platform/x86/amd/pmf/Kconfig"
> >   source "drivers/platform/x86/amd/pmc/Kconfig"
> >
> > +config AMD_3D_VCACHE
> > +    tristate "AMD 3D V-Cache Performance Optimizer Driver"
> > +    depends on X86_64 && ACPI
> > +    help
> > +      The driver provides a sysfs interface, enabling the setting of 
> > a bias
> > +      that alters CPU core reordering. This bias prefers cores with 
> > higher
> > +      frequencies or larger L3 caches on processors supporting AMD 
> > 3D V-Cache
> > +      technology.
> > +
> > +      If you choose to compile this driver as a module the module 
> > will be
> > +      called amd_3d_vcache.
> > +
> >   config AMD_HSMP
> >       tristate "AMD HSMP Driver"
> >       depends on AMD_NB && X86_64 && ACPI
> > diff --git a/drivers/platform/x86/amd/Makefile 
> > b/drivers/platform/x86/amd/Makefile
> > index dcec0a46f8af..16e4cce02242 100644
> > --- a/drivers/platform/x86/amd/Makefile
> > +++ b/drivers/platform/x86/amd/Makefile
> > @@ -4,6 +4,8 @@
> >   # AMD x86 Platform-Specific Drivers
> >   #
> >
> > +obj-$(CONFIG_AMD_3D_VCACHE)     += amd_3d_vcache.o
> > +amd_3d_vcache-objs              := x3d_vcache.o
> >   obj-$(CONFIG_AMD_PMC)        += pmc/
> >   amd_hsmp-y            := hsmp.o
> >   obj-$(CONFIG_AMD_HSMP)        += amd_hsmp.o
> > diff --git a/drivers/platform/x86/amd/x3d_vcache.c 
> > b/drivers/platform/x86/amd/x3d_vcache.c
> > new file mode 100644
> > index 000000000000..679613d02b9a
> > --- /dev/null
> > +++ b/drivers/platform/x86/amd/x3d_vcache.c
> > @@ -0,0 +1,193 @@
> > +// SPDX-License-Identifier: GPL-2.0-or-later
> > +/*
> > + * AMD 3D V-Cache Performance Optimizer Driver
> > + *
> > + * Copyright (c) 2024, Advanced Micro Devices, Inc.
> > + * All Rights Reserved.
> > + *
> > + * Authors: Basavaraj Natikar <Basavaraj.Natikar@xxxxxxx>
> > + *          Perry Yuan <perry.yuan@xxxxxxx>
> > + *          Mario Limonciello <mario.limonciello@xxxxxxx>
> > + *
> > + */
> > +
> > +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
> > +
> > +#include <linux/acpi.h>
> > +#include <linux/device.h>
> > +#include <linux/errno.h>
> > +#include <linux/module.h>
> > +#include <linux/mutex.h>
> > +#include <linux/platform_device.h>
> > +
> > +static char *x3d_mode = "frequency";
> > +module_param(x3d_mode, charp, 0444);
> > +MODULE_PARM_DESC(x3d_mode, "Initial 3D-VCache mode; 'frequency' 
> > (default) or 'cache'");
> > +
> > +#define DSM_REVISION_ID            0
> > +#define DSM_GET_FUNCS_SUPPORTED        0
> > +#define DSM_SET_X3D_MODE        1
> > +
> > +static guid_t x3d_guid = GUID_INIT(0xdff8e55f, 0xbcfd, 0x46fb, 0xba, 
> > 0x0a,
> > +                   0xef, 0xd0, 0x45, 0x0f, 0x34, 0xee);
> > +
> > +enum amd_x3d_mode_type {
> > +    MODE_INDEX_FREQ,
> > +    MODE_INDEX_CACHE,
> > +};
> > +
> > +static const char * const amd_x3d_mode_strings[] = {
> > +    [MODE_INDEX_FREQ] = "frequency",
> > +    [MODE_INDEX_CACHE] = "cache",
> > +};
> > +
> > +struct amd_x3d_dev {
> > +    struct device *dev;
> > +    acpi_handle ahandle;
> > +    /* To protect x3d mode setting */
> > +    struct mutex lock;
> > +    enum amd_x3d_mode_type curr_mode;
> > +};
> > +
> > +static int amd_x3d_mode_switch(struct amd_x3d_dev *data, int new_state)
> > +{
> > +    union acpi_object *out, argv;
> > +
> > +    guard(mutex)(&data->lock);
> > +    argv.type = ACPI_TYPE_INTEGER;
> > +    argv.integer.value = new_state;
> > +
> > +    out = acpi_evaluate_dsm(data->ahandle, &x3d_guid, 
> > DSM_REVISION_ID, DSM_SET_X3D_MODE,
> > +                &argv);
> > +    if (!out) {
> > +        dev_err(data->dev, "failed to evaluate _DSM\n");
> > +        return -EINVAL;
> > +    }
> > +
> > +    data->curr_mode = new_state;
> > +
> > +    ACPI_FREE(out);
>
> Hi,
>
> please use kfree() instead of ACPI_FREE().
>
> > +
> > +    return 0;
> > +}
> > +
> > +static ssize_t amd_x3d_mode_store(struct device *dev, struct 
> > device_attribute *attr,
> > +                  const char *buf, size_t count)
> > +{
> > +    struct amd_x3d_dev *data = dev_get_drvdata(dev);
> > +    int ret;
> > +
> > +    ret = sysfs_match_string(amd_x3d_mode_strings, buf);
> > +    if (ret < 0) {
> > +        dev_err(dev, "no matching mode to set %s\n", buf);
> > +        return ret;
> > +    }
> > +
> > +    ret = amd_x3d_mode_switch(data, ret);
> > +
> > +    return ret ? ret : count;
>
> ret = amd_x3d_mode_switch(data, ret);
> if (ret < 0)
>     return ret;
>
> return count;
>
> > +}
> > +
> > +static ssize_t amd_x3d_mode_show(struct device *dev, struct 
> > device_attribute *attr, char *buf)
> > +{
> > +    struct amd_x3d_dev *data = dev_get_drvdata(dev);
> > +
> > +    if (data->curr_mode > MODE_INDEX_CACHE || data->curr_mode < 
> > MODE_INDEX_FREQ)
> > +        return -EINVAL;
>
> Can curr_mode ever leave this range of values?

yes it's either MODE_INDEX_CACHE or MODE_INDEX_FREQ

Thanks,
--
Basavaraj

> > +
> > +    return sysfs_emit(buf, "%s\n", 
> > amd_x3d_mode_strings[data->curr_mode]);
> > +}
> > +static DEVICE_ATTR_RW(amd_x3d_mode);
> > +
> > +static struct attribute *amd_x3d_attrs[] = {
> > +    &dev_attr_amd_x3d_mode.attr,
> > +    NULL
> > +};
> > +ATTRIBUTE_GROUPS(amd_x3d);
> > +
> > +static int amd_x3d_supported(struct amd_x3d_dev *data)
> > +{
> > +    union acpi_object *out;
> > +
> > +    out = acpi_evaluate_dsm(data->ahandle, &x3d_guid, DSM_REVISION_ID,
> > +                DSM_GET_FUNCS_SUPPORTED, NULL);
>
> Please use acpi_check_dsm().
>
> > +    if (!out) {
> > +        dev_err(data->dev, "failed to evaluate _DSM\n");
> > +        return -ENODEV;
> > +    }
> > +
> > +    if (out->type != ACPI_TYPE_BUFFER) {
> > +        dev_err(data->dev, "invalid type %d\n", out->type);
> > +        ACPI_FREE(out);
> > +        return -EINVAL;
> > +    }
> > +
> > +    ACPI_FREE(out);
> > +    return 0;
> > +}
> > +
> > +static const struct acpi_device_id amd_x3d_acpi_ids[] = {
> > +    {"AMDI0101"},
> > +    { },
> > +};
> > +MODULE_DEVICE_TABLE(acpi, amd_x3d_acpi_ids);
> > +
> > +static void amd_x3d_remove(void *context)
> > +{
> > +    struct amd_x3d_dev *data = context;
> > +
> > +    mutex_destroy(&data->lock);
>
> Please use devm_mutex_init().
>
> > +}
> > +
> > +static int amd_x3d_probe(struct platform_device *pdev)
> > +{
> > +    const struct acpi_device_id *id;
> > +    struct amd_x3d_dev *data;
> > +    acpi_handle handle;
> > +    int ret;
> > +
> > +    handle = ACPI_HANDLE(&pdev->dev);
> > +    if (!handle)
> > +        return -ENODEV;
> > +
> > +    id = acpi_match_device(amd_x3d_acpi_ids, &pdev->dev);
> > +    if (!id)
> > +        dev_err_probe(&pdev->dev, -ENODEV, "unable to match ACPI ID 
> > and data\n");
>
> The driver core already takes care of that, please remove.
>
> > +
> > +    data = devm_kzalloc(&pdev->dev, sizeof(*data), GFP_KERNEL);
> > +    if (!data)
> > +        return -ENOMEM;
> > +
> > +    data->dev = &pdev->dev;
> > +    data->ahandle = handle;
> > +    platform_set_drvdata(pdev, data);
> > +
> > +    ret = amd_x3d_supported(data);
> > +    if (ret)
> > +        dev_err_probe(&pdev->dev, ret, "not supported on this 
> > platform\n");
> > +
> > +    ret = match_string(amd_x3d_mode_strings, 
> > ARRAY_SIZE(amd_x3d_mode_strings), x3d_mode);
> > +    if (ret < 0)
> > +        return dev_err_probe(&pdev->dev, -EINVAL, "invalid mode 
> > %s\n", x3d_mode);
> > +
> > +    mutex_init(&data->lock);
> > +
> > +    ret = amd_x3d_mode_switch(data, ret);
> > +    if (ret < 0)
> > +        return ret;
>
> You forgot to call mutex_destroy() here in case of an error. Using 
> devm_mutex_init() would solve that.
>
> Thanks,
> Armin Wolf
>
> > +
> > +    return devm_add_action_or_reset(&pdev->dev, amd_x3d_remove, data);
> > +}
> > +
> > +static struct platform_driver amd_3d_vcache_driver = {
> > +    .driver = {
> > +        .name = "amd_x3d_vcache",
> > +        .dev_groups = amd_x3d_groups,
> > +        .acpi_match_table = amd_x3d_acpi_ids,
> > +    },
> > +    .probe = amd_x3d_probe,
> > +};
> > +module_platform_driver(amd_3d_vcache_driver);
> > +
> > +MODULE_DESCRIPTION("AMD 3D V-Cache Performance Optimizer Driver");
> > +MODULE_LICENSE("GPL");