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

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

Hi Hans and Gregory,


On 4/19/2025 2:29 PM, Hans de Goede wrote:
> Caution: This message originated from an External Source. Use proper caution when opening attachments, clicking links, or responding.
>
>
> 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 ...

These drivers are supposed to be mutually exclusive drivers.

We don't expect them to co-exist, Thats the reason we used the same name 
for the driver, same name for character device and device file.

amd_hsmp is only for legacy platforms. All the new platforms will be 
having ACPI device.

>
> 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.

To fix the "Driver 'amd_hsmp' is already registered, aborting..." error 
when both are loaded, the driver name has to be changed, but while 
changing the driver name

adding the below lines which is suggested above by Hans is important.

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

Otherwise on Family 0x1A Model 0x0-0xF platform where both are supported (this is the transitional platform where both are supported)

it will register the driver and complain for creating duplicate entries 
of character device if user tries to load both.

We will add this check and send out the updated patch.

> 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.

For the asynchronous probing, we will guard hsmp_pdev->is_probed code with mutex.

> Regards,
>
> Hans
>

Thanks and Regards,
Suma

>
>
>> ---
>>
>> 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"
>>