Re: [BUG?] platform/x86/amd/hsmp: Create separate ACPI, plat and common drivers

[Hans de Goede <hdegoede@redhat.com>]

Hi Gregory,

On 19-Apr-25 1:01 AM, Gregory Price wrote:
> On Mon, Oct 21, 2024 at 11:14:25AM +0000, Suma Hegde wrote:
>> diff --git a/drivers/platform/x86/amd/hsmp/acpi.c b/drivers/platform/x86/amd/hsmp/acpi.c
> ... snip ...
>> +#define DRIVER_NAME		"amd_hsmp"
> ... snip ...
>> diff --git a/drivers/platform/x86/amd/hsmp/plat.c b/drivers/platform/x86/amd/hsmp/plat.c
>> +#define DRIVER_NAME		"amd_hsmp"
> 
> Hi,
> 
> From looking around this patch, the code seems to suggest that these
> drivers should not be mutually exclusive, but we are seeing an error
> when attempting to load them both - apparently because they both use the
> same driver name.
> 
> Was this intended, or should the DRIVER_NAME="hsmp_acpi" for acpi.c?

These drivers are mutually exclusive in the sense that only one of
them should actually bind to the hw.

Looking at the code this seems not to be guaranteed though ...

drivers/platform/x86/amd/hsmp/plat.c has:

static int __init hsmp_plt_init(void)
{
        int ret = -ENODEV;

        if (!legacy_hsmp_support()) {
                pr_info("HSMP is not supported on Family:%x model:%x\n",
                        boot_cpu_data.x86, boot_cpu_data.x86_model);
                return ret;
        }

	...

So loading that module should fail on AMD CPUs which fail the
legacy_hsmp_support() check. Which checks CPU family and model.

Since you are seeing the error you are reporting the system you
are testing on does pass that test.

Question, did you manually load hsmp_acpi, or did that auto-load?

Or a totally different way to ask the same thing, do you have
any AMDI0097:0? devices under /sys/bus/platform/devices ?

ATM it seems that the plat.c code assumes that on hw where
legacy_hsmp_support() returns true there will never be an
AMDI0097:0? device which I'm not sure is a safe assumption
to make.

IMHO besides the "if (!legacy_hsmp_support())" check,
hsmp_plt_init() should also do:

        if (acpi_dev_present("AMDI0097", NULL, -1))
                return -ENODEV;

leaving the HSMP handling up to the hsmp_acpi driver
when an AMDI0097:0? device is present.

Note that the identical driver-name is still an issue even
with this check, because if both checks pass a user could
still manually load hsmp_acpi and get the driver name conflict
error.

Another issue with the driver seems to be that hsmp_acpi_probe()
relies on hsmp_pdev->is_probed to only do some initialization
once, but that is_probed flag is not protected by a mutex,
so when async probing is used 2 hsmp_acpi_probe() calls can
race and make a mess of things.

Regards,

Hans




> ---
> 
> diff --git a/drivers/platform/x86/amd/hsmp/acpi.c b/drivers/platform/x86/amd/hsmp/acpi.c
> index e981d45e1c12..871724995897 100644
> --- a/drivers/platform/x86/amd/hsmp/acpi.c
> +++ b/drivers/platform/x86/amd/hsmp/acpi.c
> @@ -26,7 +26,7 @@
> 
>  #include "hsmp.h"
> 
> -#define DRIVER_NAME            "amd_hsmp"
> +#define DRIVER_NAME            "hsmp_acpi"
>  #define DRIVER_VERSION         "2.3"
>  #define ACPI_HSMP_DEVICE_HID   "AMDI0097"
>