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

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

Hi Hans,


On 4/21/2025 9:10 PM, Hans de Goede wrote:
> Caution: This message originated from an External Source. Use proper caution when opening attachments, clicking links, or responding.
>
>
> Hi,
>
> On 21-Apr-25 12:34 PM, Suma Hegde wrote:
>> 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,
> That will not work though for legacy devices if e.g. both
> are builtin and the acpi.c code gets to register its
> driver first. In that case the legacy code will fail to
> register its driver, even though it is the one which should
> be used.
>
> Same happens if the acpi.c and plat.c code both are modules
> but acpi.c is (manually) loaded first on a legacy platform.
>
>> same name for character device and device file.
> Those are fine.
>
>> 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;
> Ack, the combination of rename of the driver + the above check
> should fix things.
>
>> 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.
> Great, thank you. Note you need to keep the mutex locked
> for almost the entire code flow in hsmp_acpi_probe() (until
> is_probed = true is done).
>
> Please consider using:
>
>          guard(mutex)(&hsmp_pdev->mutex);
>
> in hsmp_acpi_probe() directly after getting hsmp_dev, this
> this avoids the need to manual unlock the mutex.
>
> Note the mutex will be a new addition to struct hsmp_plat_device,
> currently this has no mutex.
>
> And this will need to use a static mutex initializer.
>
> Regards,
>
> Hans

Thanks for these inputs. I will take all these into account and send a 
patch.


Regards,

Suma

>
>
>