On 2/1/22 11:50, Carlos Bilbao wrote:
> Hello folks,
>
> It's great to see this patch on the mailing list, this is useful stuff! I
> have some small suggestions to improve things a little more. Besides that
> the driver is in great shape I think.
>
> Feel free to add Reviewed-by: Carlos Bilbao <carlos.bilbao@xxxxxxx>.
>
> On 2/1/2022 7:53 AM, Naveen Krishna Chatradhi wrote:
>> From: Suma Hegde <suma.hegde@xxxxxxx>
>> (...)
>> +
>> +int hsmp_send_message(struct hsmp_message *msg)
>> +{
>> + struct amd_northbridge *nb;
>> + int ret;
>> +
>> + if (!msg)
>> + return -EINVAL;
>> +
>> + nb = node_to_amd_nb(msg->sock_ind);
>> + if (!nb || !nb->root)
>> + return -ENODEV;
>> +
>> + if (msg->msg_id < HSMP_TEST || msg->msg_id >= HSMP_MSG_ID_MAX)
>> + return -EINVAL;
>> +
>> + if (msg->num_args > HSMP_MAX_MSG_LEN || msg->response_sz > HSMP_MAX_MSG_LEN)
>> + return -EINVAL;
>> +
>> + /*
>> + * The time taken by smu operation to complete is between
>> + * 10us to 1ms. Sometime it may take more time.
>> + * In SMP system timeout of 100 millisecs should
>> + * be enough for the previous thread to finish the operation
>> + */
>> + ret = down_timeout(&hsmp_sem[msg->sock_ind],
>> + msecs_to_jiffies(HSMP_MSG_TIMEOUT));
>> + if (ret < 0)
>> + return ret;
>> +
>> + ret = __hsmp_send_message(nb->root, msg);
>> +
>> + up(&hsmp_sem[msg->sock_ind]);
>> +
>> + return ret;
>> +}
>> +EXPORT_SYMBOL_GPL(hsmp_send_message);
>
> In this function it would be nice to check for a msg_id of HSMP_RESERVED
> and return -EINVAL if so. The function hsmp_ioctl() could do the same, but
> it's better here since third-parties can call this directly.
>
>> + (...)
>> +
>> +static int __init hsmp_plt_init(void)
>> +{
>
> Perhaps instead of __init/__exit let's do __devinit/__devexit?
>
>> + int ret = -ENODEV;
>> + u16 num_sockets;
>> + int i;
>> +
>> + /*
>> + * amd_nb_num() returns number of SMN/DF interfaces present in the system
>> + * if we have N SMN/DF interfaces that ideally means N sockets
>> + */
>> + num_sockets = amd_nb_num();
>> + if (num_sockets == 0)
>> + return ret;
>> +
>> + /* Test the hsmp interface on each socket */
>> + for (i = 0; i < num_sockets; i++) {
>> + ret = hsmp_test(i);
>> + if (ret)
>> + return ret;
>> + }
>> +
>> + ret = platform_driver_register(&amd_hsmp_driver);
>> + if (ret)
>> + return ret;
>> +
>> + amd_hsmp_platdev = platform_device_alloc(DRIVER_NAME, -1);
>> + if (!amd_hsmp_platdev) {
>> + ret = -ENOMEM;
>> + goto drv_unregister;
>> + }
>> +
>> + ret = platform_device_add(amd_hsmp_platdev);
>> + if (ret) {
>> + platform_device_put(amd_hsmp_platdev);
>> + goto drv_unregister;
>> + }
>> +
>> + return 0;
>> +
>> +drv_unregister:
>> + platform_driver_unregister(&amd_hsmp_driver);
>> + return ret;
>> +}
>
> I think we really need to do some checking for family and model (>=0x19)
> here.
>
We should only need to do a family check, HSMP is only supported on family 19h.
-Nathan
>> +
>> +static void __exit hsmp_plt_exit(void)
>> +{
>> + platform_device_unregister(amd_hsmp_platdev);
>> + platform_driver_unregister(&amd_hsmp_driver);
>> +}
>> +
>> +device_initcall(hsmp_plt_init);
>> +module_exit(hsmp_plt_exit);
>> +
>> +MODULE_DESCRIPTION("AMD HSMP Platform Interface Driver");
>> +MODULE_VERSION(DRIVER_VERSION);
>> +MODULE_LICENSE("GPL v2");
>
> Thanks,
> Carlos
