Hi Everyone,
On 2/2/2022 5:25 PM, Nathan Fontenot wrote:
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
Thank you for the comments, will wait for some more feedback before
submitting the next version addressing these comments along.
+
+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
