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