diff mbox series

[v3,1/3] platform/x86/amd/hsmp: create plat specific struct

Message ID 20230919092057.2235437-1-suma.hegde@amd.com (mailing list archive)
State Rejected, archived
Headers show
Series [v3,1/3] platform/x86/amd/hsmp: create plat specific struct | expand

Commit Message

Suma Hegde Sept. 19, 2023, 9:20 a.m. UTC 
Having a separate platform device structure helps in future, to
contain platform specific variables and other data.

Signed-off-by: Suma Hegde <suma.hegde@amd.com>
Reviewed-by: Naveen Krishna Chatradhi <nchatrad@amd.com>
---
Changes since v1:
1. defined HSMP_CDEV_NAME and HSMP_DEVNODE_NAME macros
Changes since v2:
1. moved num_sockets variable to plat_dev structure

 drivers/platform/x86/amd/hsmp.c | 61 ++++++++++++++++++++-------------
 1 file changed, 38 insertions(+), 23 deletions(-)

Comments

Ilpo Järvinen Sept. 19, 2023, 1 p.m. UTC | #1 
On Tue, 19 Sep 2023, Suma Hegde wrote:

> Having a separate platform device structure helps in future, to
> contain platform specific variables and other data.
> 
> Signed-off-by: Suma Hegde <suma.hegde@amd.com>
> Reviewed-by: Naveen Krishna Chatradhi <nchatrad@amd.com>
> ---
> Changes since v1:
> 1. defined HSMP_CDEV_NAME and HSMP_DEVNODE_NAME macros
> Changes since v2:
> 1. moved num_sockets variable to plat_dev structure
> 
>  drivers/platform/x86/amd/hsmp.c | 61 ++++++++++++++++++++-------------
>  1 file changed, 38 insertions(+), 23 deletions(-)
> 
> diff --git a/drivers/platform/x86/amd/hsmp.c b/drivers/platform/x86/amd/hsmp.c
> index 31382ef52efb..99727cd705cf 100644
> --- a/drivers/platform/x86/amd/hsmp.c
> +++ b/drivers/platform/x86/amd/hsmp.c
> @@ -47,9 +47,22 @@
>  #define HSMP_INDEX_REG		0xc4
>  #define HSMP_DATA_REG		0xc8
>  
> -static struct semaphore *hsmp_sem;
> +#define HSMP_CDEV_NAME		"hsmp_cdev"
> +#define HSMP_DEVNODE_NAME	"hsmp"
>  
> -static struct miscdevice hsmp_device;
> +struct hsmp_socket {
> +	struct semaphore hsmp_sem;
> +	u16 sock_ind;
> +};
> +
> +struct hsmp_plat_device {
> +	struct miscdevice hsmp_device;
> +	struct hsmp_socket *sock;
> +	struct device *dev;
> +	u16 num_sockets;
> +};
> +
> +static struct hsmp_plat_device plat_dev;
>  
>  static int amd_hsmp_rdwr(struct pci_dev *root, u32 address,
>  			 u32 *value, bool write)
> @@ -188,6 +201,7 @@ static int validate_message(struct hsmp_message *msg)
>  
>  int hsmp_send_message(struct hsmp_message *msg)
>  {
> +	struct hsmp_socket *sock = &plat_dev.sock[msg->sock_ind];
>  	struct amd_northbridge *nb;
>  	int ret;
>  
> @@ -208,14 +222,13 @@ int hsmp_send_message(struct hsmp_message *msg)
>  	 * 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));
> +	ret = down_timeout(&sock->hsmp_sem, msecs_to_jiffies(HSMP_MSG_TIMEOUT));
>  	if (ret < 0)
>  		return ret;
>  
>  	ret = __hsmp_send_message(nb->root, msg);
>  
> -	up(&hsmp_sem[msg->sock_ind]);
> +	up(&sock->hsmp_sem);
>  
>  	return ret;
>  }
> @@ -321,28 +334,31 @@ static int hsmp_pltdrv_probe(struct platform_device *pdev)
>  {
>  	int i;
>  
> -	hsmp_sem = devm_kzalloc(&pdev->dev,
> -				(amd_nb_num() * sizeof(struct semaphore)),
> -				GFP_KERNEL);
> -	if (!hsmp_sem)
> +	plat_dev.sock = devm_kzalloc(&pdev->dev,
> +				     (plat_dev.num_sockets * sizeof(struct hsmp_socket)),
> +				     GFP_KERNEL);
> +	if (!plat_dev.sock)
>  		return -ENOMEM;
> +	plat_dev.dev = &pdev->dev;
>  
> -	for (i = 0; i < amd_nb_num(); i++)
> -		sema_init(&hsmp_sem[i], 1);
> +	for (i = 0; i < plat_dev.num_sockets; i++) {
> +		sema_init(&plat_dev.sock[i].hsmp_sem, 1);
> +		plat_dev.sock[i].sock_ind = i;
> +	}
>  
> -	hsmp_device.name	= "hsmp_cdev";
> -	hsmp_device.minor	= MISC_DYNAMIC_MINOR;
> -	hsmp_device.fops	= &hsmp_fops;
> -	hsmp_device.parent	= &pdev->dev;
> -	hsmp_device.nodename	= "hsmp";
> -	hsmp_device.mode	= 0644;
> +	plat_dev.hsmp_device.name	= HSMP_CDEV_NAME;
> +	plat_dev.hsmp_device.minor	= MISC_DYNAMIC_MINOR;
> +	plat_dev.hsmp_device.fops	= &hsmp_fops;
> +	plat_dev.hsmp_device.parent	= &pdev->dev;
> +	plat_dev.hsmp_device.nodename	= HSMP_DEVNODE_NAME;
> +	plat_dev.hsmp_device.mode	= 0644;
>  
> -	return misc_register(&hsmp_device);
> +	return misc_register(&plat_dev.hsmp_device);
>  }
>  
>  static void hsmp_pltdrv_remove(struct platform_device *pdev)
>  {
> -	misc_deregister(&hsmp_device);
> +	misc_deregister(&plat_dev.hsmp_device);
>  }
>  
>  static struct platform_driver amd_hsmp_driver = {
> @@ -358,7 +374,6 @@ static struct platform_device *amd_hsmp_platdev;
>  static int __init hsmp_plt_init(void)
>  {
>  	int ret = -ENODEV;
> -	u16 num_sockets;
>  	int i;
>  
>  	if (boot_cpu_data.x86_vendor != X86_VENDOR_AMD || boot_cpu_data.x86 < 0x19) {
> @@ -371,12 +386,12 @@ static int __init hsmp_plt_init(void)
>  	 * 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)
> +	plat_dev.num_sockets = amd_nb_num();
> +	if (plat_dev.num_sockets == 0)
>  		return ret;
>  
>  	/* Test the hsmp interface on each socket */
> -	for (i = 0; i < num_sockets; i++) {
> +	for (i = 0; i < plat_dev.num_sockets; i++) {
>  		ret = hsmp_test(i, 0xDEADBEEF);
>  		if (ret) {
>  			pr_err("HSMP is not supported on Fam:%x model:%x\n",
> 

Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
Ilpo Järvinen Sept. 26, 2023, 1:05 p.m. UTC | #2 
On Tue, 19 Sep 2023 09:20:55 +0000, Suma Hegde wrote:

> Having a separate platform device structure helps in future, to
> contain platform specific variables and other data.
> 
> 


Thank you for your contribution, it has been applied to my local
review-ilpo branch. Note it will show up in the public
platform-drivers-x86/review-ilpo branch only once I've pushed my
local branch there, which might take a while.

Once I've run some tests on the review-ilpo branch the patches
there will be added to the platform-drivers-x86/for-next branch
and eventually will be included in the pdx86 pull-request to
Linus for the next merge-window.

The list of commits applied:
[1/3] platform/x86/amd/hsmp: create plat specific struct
      commit: 6448b90731aabbfb0e351b11be37af94c157dda4
[2/3] platform/x86/amd/hsmp: add support for metrics tbl
      commit: 0f21042c11c7fa9053d5af1797f5a9380579f4dd
[3/3] platform/x86/amd/hsmp: improve the error log
      commit: bd2b5ff0df3da9b75e684d99cd5b1ed4e74e24db

--
 i.
Ilpo Järvinen Sept. 27, 2023, 11:18 a.m. UTC | #3 
On Tue, 26 Sep 2023, Ilpo Järvinen wrote:

> On Tue, 19 Sep 2023 09:20:55 +0000, Suma Hegde wrote:
> 
> > Having a separate platform device structure helps in future, to
> > contain platform specific variables and other data.
> > 
> > 
> 
> 
> Thank you for your contribution, it has been applied to my local
> review-ilpo branch. Note it will show up in the public
> platform-drivers-x86/review-ilpo branch only once I've pushed my
> local branch there, which might take a while.
> 
> Once I've run some tests on the review-ilpo branch the patches
> there will be added to the platform-drivers-x86/for-next branch
> and eventually will be included in the pdx86 pull-request to
> Linus for the next merge-window.
> 
> The list of commits applied:
> [1/3] platform/x86/amd/hsmp: create plat specific struct
>       commit: 6448b90731aabbfb0e351b11be37af94c157dda4
> [2/3] platform/x86/amd/hsmp: add support for metrics tbl
>       commit: 0f21042c11c7fa9053d5af1797f5a9380579f4dd

This ended up breaking the build and for some reason lkp had not caught it
(probably didn't build the series at all since the build fails already 
with allyesconfig).

I'll fix this in review-ilpo branch for you but please try to build test 
in the future to add a little bit of redundancy into build testing.

> [3/3] platform/x86/amd/hsmp: improve the error log
>       commit: bd2b5ff0df3da9b75e684d99cd5b1ed4e74e24db
diff mbox series

Patch

diff --git a/drivers/platform/x86/amd/hsmp.c b/drivers/platform/x86/amd/hsmp.c
index 31382ef52efb..99727cd705cf 100644
--- a/drivers/platform/x86/amd/hsmp.c
+++ b/drivers/platform/x86/amd/hsmp.c
@@ -47,9 +47,22 @@ 
 #define HSMP_INDEX_REG		0xc4
 #define HSMP_DATA_REG		0xc8
 
-static struct semaphore *hsmp_sem;
+#define HSMP_CDEV_NAME		"hsmp_cdev"
+#define HSMP_DEVNODE_NAME	"hsmp"
 
-static struct miscdevice hsmp_device;
+struct hsmp_socket {
+	struct semaphore hsmp_sem;
+	u16 sock_ind;
+};
+
+struct hsmp_plat_device {
+	struct miscdevice hsmp_device;
+	struct hsmp_socket *sock;
+	struct device *dev;
+	u16 num_sockets;
+};
+
+static struct hsmp_plat_device plat_dev;
 
 static int amd_hsmp_rdwr(struct pci_dev *root, u32 address,
 			 u32 *value, bool write)
@@ -188,6 +201,7 @@  static int validate_message(struct hsmp_message *msg)
 
 int hsmp_send_message(struct hsmp_message *msg)
 {
+	struct hsmp_socket *sock = &plat_dev.sock[msg->sock_ind];
 	struct amd_northbridge *nb;
 	int ret;
 
@@ -208,14 +222,13 @@  int hsmp_send_message(struct hsmp_message *msg)
 	 * 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));
+	ret = down_timeout(&sock->hsmp_sem, msecs_to_jiffies(HSMP_MSG_TIMEOUT));
 	if (ret < 0)
 		return ret;
 
 	ret = __hsmp_send_message(nb->root, msg);
 
-	up(&hsmp_sem[msg->sock_ind]);
+	up(&sock->hsmp_sem);
 
 	return ret;
 }
@@ -321,28 +334,31 @@  static int hsmp_pltdrv_probe(struct platform_device *pdev)
 {
 	int i;
 
-	hsmp_sem = devm_kzalloc(&pdev->dev,
-				(amd_nb_num() * sizeof(struct semaphore)),
-				GFP_KERNEL);
-	if (!hsmp_sem)
+	plat_dev.sock = devm_kzalloc(&pdev->dev,
+				     (plat_dev.num_sockets * sizeof(struct hsmp_socket)),
+				     GFP_KERNEL);
+	if (!plat_dev.sock)
 		return -ENOMEM;
+	plat_dev.dev = &pdev->dev;
 
-	for (i = 0; i < amd_nb_num(); i++)
-		sema_init(&hsmp_sem[i], 1);
+	for (i = 0; i < plat_dev.num_sockets; i++) {
+		sema_init(&plat_dev.sock[i].hsmp_sem, 1);
+		plat_dev.sock[i].sock_ind = i;
+	}
 
-	hsmp_device.name	= "hsmp_cdev";
-	hsmp_device.minor	= MISC_DYNAMIC_MINOR;
-	hsmp_device.fops	= &hsmp_fops;
-	hsmp_device.parent	= &pdev->dev;
-	hsmp_device.nodename	= "hsmp";
-	hsmp_device.mode	= 0644;
+	plat_dev.hsmp_device.name	= HSMP_CDEV_NAME;
+	plat_dev.hsmp_device.minor	= MISC_DYNAMIC_MINOR;
+	plat_dev.hsmp_device.fops	= &hsmp_fops;
+	plat_dev.hsmp_device.parent	= &pdev->dev;
+	plat_dev.hsmp_device.nodename	= HSMP_DEVNODE_NAME;
+	plat_dev.hsmp_device.mode	= 0644;
 
-	return misc_register(&hsmp_device);
+	return misc_register(&plat_dev.hsmp_device);
 }
 
 static void hsmp_pltdrv_remove(struct platform_device *pdev)
 {
-	misc_deregister(&hsmp_device);
+	misc_deregister(&plat_dev.hsmp_device);
 }
 
 static struct platform_driver amd_hsmp_driver = {
@@ -358,7 +374,6 @@  static struct platform_device *amd_hsmp_platdev;
 static int __init hsmp_plt_init(void)
 {
 	int ret = -ENODEV;
-	u16 num_sockets;
 	int i;
 
 	if (boot_cpu_data.x86_vendor != X86_VENDOR_AMD || boot_cpu_data.x86 < 0x19) {
@@ -371,12 +386,12 @@  static int __init hsmp_plt_init(void)
 	 * 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)
+	plat_dev.num_sockets = amd_nb_num();
+	if (plat_dev.num_sockets == 0)
 		return ret;
 
 	/* Test the hsmp interface on each socket */
-	for (i = 0; i < num_sockets; i++) {
+	for (i = 0; i < plat_dev.num_sockets; i++) {
 		ret = hsmp_test(i, 0xDEADBEEF);
 		if (ret) {
 			pr_err("HSMP is not supported on Fam:%x model:%x\n",