Re: [PATCH 1/2] platforms/x86: Add AMD system management interface driver

[Nathan Fontenot <nafonten@amd.com>]

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@amd.com>.
> 
> On 2/1/2022 7:53 AM, Naveen Krishna Chatradhi wrote:
>> From: Suma Hegde <suma.hegde@amd.com>
>> (...)
>> +
>> +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