[PATCH v4 0/3] Updates to amd-pmc driver

[PATCH v4 0/3] Updates to amd-pmc driver

[Sanket Goswami]

This patch series includes:
- Improvements to error-exits in driver probe
- Store root port information
- Introduce support for AMD Smart Trace Buffer

Sanket Goswami (3):
  platform/x86: amd-pmc: Store the pci_dev instance inside struct
    amd_pmc_dev
  platform/x86: amd-pmc: Simplify error handling path
  platform/x86: amd-pmc: Add support for AMD Smart Trace Buffer

 drivers/platform/x86/amd-pmc.c | 148 ++++++++++++++++++++++++++++++---
 1 file changed, 135 insertions(+), 13 deletions(-)

-- 
2.25.1

[PATCH v4 1/3] platform/x86: amd-pmc: Store the pci_dev instance inside struct amd_pmc_dev

[Sanket Goswami]

Store the root port information in amd_pmc_probe() so that the
information can be used across multiple routines.

Signed-off-by: Sanket Goswami <Sanket.Goswami@amd.com>
---
Changes in v3:
- Address review comments from Hans.

Changes in v2:
- Store the rdev info in amd_pmc_probe() as suggested by Hans.

 drivers/platform/x86/amd-pmc.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/platform/x86/amd-pmc.c b/drivers/platform/x86/amd-pmc.c
index 678bf6874c63..5d88e55e1ce7 100644
--- a/drivers/platform/x86/amd-pmc.c
+++ b/drivers/platform/x86/amd-pmc.c
@@ -121,6 +121,7 @@ struct amd_pmc_dev {
 	u16 minor;
 	u16 rev;
 	struct device *dev;
+	struct pci_dev *rdev;
 	struct mutex lock; /* generic mutex lock */
 #if IS_ENABLED(CONFIG_DEBUG_FS)
 	struct dentry *dbgfs_dir;
@@ -541,6 +542,7 @@ static int amd_pmc_probe(struct platform_device *pdev)
 	}
 
 	dev->cpu_id = rdev->device;
+	dev->rdev = rdev;
 	err = pci_write_config_dword(rdev, AMD_PMC_SMU_INDEX_ADDRESS, AMD_PMC_BASE_ADDR_LO);
 	if (err) {
 		dev_err(dev->dev, "error writing to 0x%x\n", AMD_PMC_SMU_INDEX_ADDRESS);
@@ -570,7 +572,6 @@ static int amd_pmc_probe(struct platform_device *pdev)
 	}
 
 	base_addr_hi = val & AMD_PMC_BASE_ADDR_LO_MASK;
-	pci_dev_put(rdev);
 	base_addr = ((u64)base_addr_hi << 32 | base_addr_lo);
 
 	dev->regbase = devm_ioremap(dev->dev, base_addr + AMD_PMC_BASE_ADDR_OFFSET,
@@ -604,6 +605,7 @@ static int amd_pmc_remove(struct platform_device *pdev)
 	struct amd_pmc_dev *dev = platform_get_drvdata(pdev);
 
 	amd_pmc_dbgfs_unregister(dev);
+	pci_dev_put(dev->rdev);
 	mutex_destroy(&dev->lock);
 	return 0;
 }
-- 
2.25.1

[PATCH v4 2/3] platform/x86: amd-pmc: Simplify error handling path

[Sanket Goswami]

Handle error-exits in the amd_pmc_probe() so that the code duplication
is reduced.

Suggested-by: Hans de Goede <hdegoede@redhat.com>
Signed-off-by: Sanket Goswami <Sanket.Goswami@amd.com>
---
Changes in v4:
- No change.

Changes in v3:
- No change.

Changes in v2:
- No change.

 drivers/platform/x86/amd-pmc.c | 22 ++++++++++------------
 1 file changed, 10 insertions(+), 12 deletions(-)

diff --git a/drivers/platform/x86/amd-pmc.c b/drivers/platform/x86/amd-pmc.c
index 5d88e55e1ce7..50cb65e38d11 100644
--- a/drivers/platform/x86/amd-pmc.c
+++ b/drivers/platform/x86/amd-pmc.c
@@ -546,30 +546,24 @@ static int amd_pmc_probe(struct platform_device *pdev)
 	err = pci_write_config_dword(rdev, AMD_PMC_SMU_INDEX_ADDRESS, AMD_PMC_BASE_ADDR_LO);
 	if (err) {
 		dev_err(dev->dev, "error writing to 0x%x\n", AMD_PMC_SMU_INDEX_ADDRESS);
-		pci_dev_put(rdev);
-		return pcibios_err_to_errno(err);
+		goto err_pci_dev_put;
 	}
 
 	err = pci_read_config_dword(rdev, AMD_PMC_SMU_INDEX_DATA, &val);
-	if (err) {
-		pci_dev_put(rdev);
-		return pcibios_err_to_errno(err);
-	}
+	if (err)
+		goto err_pci_dev_put;
 
 	base_addr_lo = val & AMD_PMC_BASE_ADDR_HI_MASK;
 
 	err = pci_write_config_dword(rdev, AMD_PMC_SMU_INDEX_ADDRESS, AMD_PMC_BASE_ADDR_HI);
 	if (err) {
 		dev_err(dev->dev, "error writing to 0x%x\n", AMD_PMC_SMU_INDEX_ADDRESS);
-		pci_dev_put(rdev);
-		return pcibios_err_to_errno(err);
+		goto err_pci_dev_put;
 	}
 
 	err = pci_read_config_dword(rdev, AMD_PMC_SMU_INDEX_DATA, &val);
-	if (err) {
-		pci_dev_put(rdev);
-		return pcibios_err_to_errno(err);
-	}
+	if (err)
+		goto err_pci_dev_put;
 
 	base_addr_hi = val & AMD_PMC_BASE_ADDR_LO_MASK;
 	base_addr = ((u64)base_addr_hi << 32 | base_addr_lo);
@@ -598,6 +592,10 @@ static int amd_pmc_probe(struct platform_device *pdev)
 	platform_set_drvdata(pdev, dev);
 	amd_pmc_dbgfs_register(dev);
 	return 0;
+
+err_pci_dev_put:
+		pci_dev_put(rdev);
+		return pcibios_err_to_errno(err);
 }
 
 static int amd_pmc_remove(struct platform_device *pdev)
-- 
2.25.1

[PATCH v4 3/3] platform/x86: amd-pmc: Add support for AMD Smart Trace Buffer

[Sanket Goswami]

STB (Smart Trace Buffer), is a debug trace buffer which is used to help
isolate failures by analyzing the last feature that a system was running
before hitting a failure. This nonintrusive way is always running in the
background and trace is stored into the SoC.

This patch provides mechanism to access the STB buffer using the read
and write routines.

Co-developed-by: Shyam Sundar S K <Shyam-sundar.S-k@amd.com>
Signed-off-by: Shyam Sundar S K <Shyam-sundar.S-k@amd.com>
Signed-off-by: Sanket Goswami <Sanket.Goswami@amd.com>
---
Changes in v4:
- Use kzalloc() for memory allocation.

Changes in v3:
- Address review comments from Mark Gross.

Changes in v2:
- Create amd_pmc_stb_debugfs_fops structure to get STB data.
- Address review comments from Hans.

 drivers/platform/x86/amd-pmc.c | 122 +++++++++++++++++++++++++++++++++
 1 file changed, 122 insertions(+)

diff --git a/drivers/platform/x86/amd-pmc.c b/drivers/platform/x86/amd-pmc.c
index 50cb65e38d11..e55c48f675e1 100644
--- a/drivers/platform/x86/amd-pmc.c
+++ b/drivers/platform/x86/amd-pmc.c
@@ -35,6 +35,12 @@
 #define AMD_PMC_SCRATCH_REG_CZN		0x94
 #define AMD_PMC_SCRATCH_REG_YC		0xD14
 
+/* STB Registers */
+#define AMD_PMC_STB_INDEX_ADDRESS	0xF8
+#define AMD_PMC_STB_INDEX_DATA		0xFC
+#define AMD_PMC_STB_PMI_0		0x03E30600
+#define AMD_PMC_STB_PREDEF		0xC6000001
+
 /* Base address of SMU for mapping physical address to virtual address */
 #define AMD_PMC_SMU_INDEX_ADDRESS	0xB8
 #define AMD_PMC_SMU_INDEX_DATA		0xBC
@@ -82,6 +88,7 @@
 #define SOC_SUBSYSTEM_IP_MAX	12
 #define DELAY_MIN_US		2000
 #define DELAY_MAX_US		3000
+#define FIFO_SIZE		4096
 enum amd_pmc_def {
 	MSG_TEST = 0x01,
 	MSG_OS_HINT_PCO,
@@ -128,8 +135,14 @@ struct amd_pmc_dev {
 #endif /* CONFIG_DEBUG_FS */
 };
 
+static bool enable_stb;
+module_param(enable_stb, bool, 0644);
+MODULE_PARM_DESC(enable_stb, "Enable the STB debug mechanism");
+
 static struct amd_pmc_dev pmc;
 static int amd_pmc_send_cmd(struct amd_pmc_dev *dev, u32 arg, u32 *data, u8 msg, bool ret);
+static int amd_pmc_write_stb(struct amd_pmc_dev *dev, u32 data);
+static int amd_pmc_read_stb(struct amd_pmc_dev *dev, u32 *buf);
 
 static inline u32 amd_pmc_reg_read(struct amd_pmc_dev *dev, int reg_offset)
 {
@@ -176,6 +189,53 @@ static int amd_pmc_get_smu_version(struct amd_pmc_dev *dev)
 	return 0;
 }
 
+static int amd_pmc_stb_debugfs_open(struct inode *inode, struct file *filp)
+{
+	struct amd_pmc_dev *dev = filp->f_inode->i_private;
+	u32 size = FIFO_SIZE * sizeof(u32);
+	u32 *buf;
+	int rc;
+
+	buf = kzalloc(size, GFP_KERNEL);
+	if (!buf)
+		return -ENOMEM;
+
+	rc = amd_pmc_read_stb(dev, buf);
+	if (rc)
+		goto out;
+
+	filp->private_data = buf;
+
+out:
+	kfree(buf);
+	return rc;
+}
+
+static ssize_t amd_pmc_stb_debugfs_read(struct file *filp, char __user *buf, size_t size,
+					loff_t *pos)
+{
+	if (!filp->private_data)
+		return -EINVAL;
+
+	return simple_read_from_buffer(buf, size, pos, filp->private_data,
+				       FIFO_SIZE * sizeof(u32));
+}
+
+static int amd_pmc_stb_debugfs_release(struct inode *inode, struct file *filp)
+{
+	kfree(filp->private_data);
+	filp->private_data = NULL;
+
+	return 0;
+}
+
+const struct file_operations amd_pmc_stb_debugfs_fops = {
+	.owner = THIS_MODULE,
+	.open = amd_pmc_stb_debugfs_open,
+	.read = amd_pmc_stb_debugfs_read,
+	.release = amd_pmc_stb_debugfs_release,
+};
+
 static int amd_pmc_idlemask_read(struct amd_pmc_dev *pdev, struct device *dev,
 				 struct seq_file *s)
 {
@@ -289,6 +349,10 @@ static void amd_pmc_dbgfs_register(struct amd_pmc_dev *dev)
 			    &s0ix_stats_fops);
 	debugfs_create_file("amd_pmc_idlemask", 0644, dev->dbgfs_dir, dev,
 			    &amd_pmc_idlemask_fops);
+	/* Enable STB only when the module_param is set */
+	if (enable_stb)
+		debugfs_create_file("stb_read", 0644, dev->dbgfs_dir, dev,
+				    &amd_pmc_stb_debugfs_fops);
 }
 #else
 static inline void amd_pmc_dbgfs_register(struct amd_pmc_dev *dev)
@@ -488,6 +552,9 @@ static int __maybe_unused amd_pmc_suspend(struct device *dev)
 	if (rc)
 		dev_err(pdev->dev, "suspend failed\n");
 
+	if (enable_stb)
+		amd_pmc_write_stb(pdev, AMD_PMC_STB_PREDEF);
+
 	return rc;
 }
 
@@ -508,6 +575,10 @@ static int __maybe_unused amd_pmc_resume(struct device *dev)
 	/* Dump the IdleMask to see the blockers */
 	amd_pmc_idlemask_read(pdev, dev, NULL);
 
+	/* Write data incremented by 1 to distinguish in stb_read */
+	if (enable_stb)
+		amd_pmc_write_stb(pdev, AMD_PMC_STB_PREDEF + 1);
+
 	return 0;
 }
 
@@ -524,6 +595,57 @@ static const struct pci_device_id pmc_pci_ids[] = {
 	{ }
 };
 
+static int amd_pmc_write_stb(struct amd_pmc_dev *dev, u32 data)
+{
+	int rc;
+
+	rc = pci_write_config_dword(dev->rdev, AMD_PMC_STB_INDEX_ADDRESS, AMD_PMC_STB_PMI_0);
+	if (rc) {
+		dev_err(dev->dev, "failed to write addr in stb: 0x%X\n",
+			AMD_PMC_STB_INDEX_ADDRESS);
+		pci_dev_put(dev->rdev);
+		return pcibios_err_to_errno(rc);
+	}
+
+	rc = pci_write_config_dword(dev->rdev, AMD_PMC_STB_INDEX_DATA, data);
+	if (rc) {
+		dev_err(dev->dev, "failed to write data in stb: 0x%X\n",
+			AMD_PMC_STB_INDEX_DATA);
+		pci_dev_put(dev->rdev);
+		return pcibios_err_to_errno(rc);
+	}
+
+	return 0;
+}
+
+static int amd_pmc_read_stb(struct amd_pmc_dev *dev, u32 *buf)
+{
+	int i, err;
+	u32 value;
+
+	err = pci_write_config_dword(dev->rdev, AMD_PMC_STB_INDEX_ADDRESS, AMD_PMC_STB_PMI_0);
+	if (err) {
+		dev_err(dev->dev, "error writing addr to stb: 0x%X\n",
+			AMD_PMC_STB_INDEX_ADDRESS);
+		pci_dev_put(dev->rdev);
+		return pcibios_err_to_errno(err);
+	}
+
+	for (i = 0; i < FIFO_SIZE; i++) {
+		err = pci_read_config_dword(dev->rdev, AMD_PMC_STB_INDEX_DATA, &value);
+		if (err) {
+			dev_err(dev->dev, "error reading data from stb: 0x%X\n",
+				AMD_PMC_STB_INDEX_DATA);
+			pci_dev_put(dev->rdev);
+			return pcibios_err_to_errno(err);
+		}
+
+		*buf++ = value;
+	}
+
+	return 0;
+}
+
 static int amd_pmc_probe(struct platform_device *pdev)
 {
 	struct amd_pmc_dev *dev = &pmc;
-- 
2.25.1

RE: [PATCH v4 3/3] platform/x86: amd-pmc: Add support for AMD Smart Trace Buffer

[Goswami, Sanket]

[AMD Official Use Only]

+Scott

-----Original Message-----
From: Goswami, Sanket <Sanket.Goswami@amd.com> 
Sent: Wednesday, October 27, 2021 0:11
To: S-k, Shyam-sundar <Shyam-sundar.S-k@amd.com>; hdegoede@redhat.com; mgross@linux.intel.com
Cc: platform-driver-x86@vger.kernel.org; Goswami, Sanket <Sanket.Goswami@amd.com>
Subject: [PATCH v4 3/3] platform/x86: amd-pmc: Add support for AMD Smart Trace Buffer

STB (Smart Trace Buffer), is a debug trace buffer which is used to help isolate failures by analyzing the last feature that a system was running before hitting a failure. This nonintrusive way is always running in the background and trace is stored into the SoC.

This patch provides mechanism to access the STB buffer using the read and write routines.

Co-developed-by: Shyam Sundar S K <Shyam-sundar.S-k@amd.com>
Signed-off-by: Shyam Sundar S K <Shyam-sundar.S-k@amd.com>
Signed-off-by: Sanket Goswami <Sanket.Goswami@amd.com>
---
Changes in v4:
- Use kzalloc() for memory allocation.

Changes in v3:
- Address review comments from Mark Gross.

Changes in v2:
- Create amd_pmc_stb_debugfs_fops structure to get STB data.
- Address review comments from Hans.

 drivers/platform/x86/amd-pmc.c | 122 +++++++++++++++++++++++++++++++++
 1 file changed, 122 insertions(+)

diff --git a/drivers/platform/x86/amd-pmc.c b/drivers/platform/x86/amd-pmc.c index 50cb65e38d11..e55c48f675e1 100644
--- a/drivers/platform/x86/amd-pmc.c
+++ b/drivers/platform/x86/amd-pmc.c
@@ -35,6 +35,12 @@
 #define AMD_PMC_SCRATCH_REG_CZN		0x94
 #define AMD_PMC_SCRATCH_REG_YC		0xD14
 
+/* STB Registers */
+#define AMD_PMC_STB_INDEX_ADDRESS	0xF8
+#define AMD_PMC_STB_INDEX_DATA		0xFC
+#define AMD_PMC_STB_PMI_0		0x03E30600
+#define AMD_PMC_STB_PREDEF		0xC6000001
+
 /* Base address of SMU for mapping physical address to virtual address */
 #define AMD_PMC_SMU_INDEX_ADDRESS	0xB8
 #define AMD_PMC_SMU_INDEX_DATA		0xBC
@@ -82,6 +88,7 @@
 #define SOC_SUBSYSTEM_IP_MAX	12
 #define DELAY_MIN_US		2000
 #define DELAY_MAX_US		3000
+#define FIFO_SIZE		4096
 enum amd_pmc_def {
 	MSG_TEST = 0x01,
 	MSG_OS_HINT_PCO,
@@ -128,8 +135,14 @@ struct amd_pmc_dev {  #endif /* CONFIG_DEBUG_FS */  };
 
+static bool enable_stb;
+module_param(enable_stb, bool, 0644);
+MODULE_PARM_DESC(enable_stb, "Enable the STB debug mechanism");
+
 static struct amd_pmc_dev pmc;
 static int amd_pmc_send_cmd(struct amd_pmc_dev *dev, u32 arg, u32 *data, u8 msg, bool ret);
+static int amd_pmc_write_stb(struct amd_pmc_dev *dev, u32 data); static 
+int amd_pmc_read_stb(struct amd_pmc_dev *dev, u32 *buf);
 
 static inline u32 amd_pmc_reg_read(struct amd_pmc_dev *dev, int reg_offset)  { @@ -176,6 +189,53 @@ static int amd_pmc_get_smu_version(struct amd_pmc_dev *dev)
 	return 0;
 }
 
+static int amd_pmc_stb_debugfs_open(struct inode *inode, struct file 
+*filp) {
+	struct amd_pmc_dev *dev = filp->f_inode->i_private;
+	u32 size = FIFO_SIZE * sizeof(u32);
+	u32 *buf;
+	int rc;
+
+	buf = kzalloc(size, GFP_KERNEL);
+	if (!buf)
+		return -ENOMEM;
+
+	rc = amd_pmc_read_stb(dev, buf);
+	if (rc)
+		goto out;
+
+	filp->private_data = buf;
+
+out:
+	kfree(buf);
+	return rc;
+}
+
+static ssize_t amd_pmc_stb_debugfs_read(struct file *filp, char __user *buf, size_t size,
+					loff_t *pos)
+{
+	if (!filp->private_data)
+		return -EINVAL;
+
+	return simple_read_from_buffer(buf, size, pos, filp->private_data,
+				       FIFO_SIZE * sizeof(u32));
+}
+
+static int amd_pmc_stb_debugfs_release(struct inode *inode, struct file 
+*filp) {
+	kfree(filp->private_data);
+	filp->private_data = NULL;
+
+	return 0;
+}
+
+const struct file_operations amd_pmc_stb_debugfs_fops = {
+	.owner = THIS_MODULE,
+	.open = amd_pmc_stb_debugfs_open,
+	.read = amd_pmc_stb_debugfs_read,
+	.release = amd_pmc_stb_debugfs_release, };
+
 static int amd_pmc_idlemask_read(struct amd_pmc_dev *pdev, struct device *dev,
 				 struct seq_file *s)
 {
@@ -289,6 +349,10 @@ static void amd_pmc_dbgfs_register(struct amd_pmc_dev *dev)
 			    &s0ix_stats_fops);
 	debugfs_create_file("amd_pmc_idlemask", 0644, dev->dbgfs_dir, dev,
 			    &amd_pmc_idlemask_fops);
+	/* Enable STB only when the module_param is set */
+	if (enable_stb)
+		debugfs_create_file("stb_read", 0644, dev->dbgfs_dir, dev,
+				    &amd_pmc_stb_debugfs_fops);
 }
 #else
 static inline void amd_pmc_dbgfs_register(struct amd_pmc_dev *dev) @@ -488,6 +552,9 @@ static int __maybe_unused amd_pmc_suspend(struct device *dev)
 	if (rc)
 		dev_err(pdev->dev, "suspend failed\n");
 
+	if (enable_stb)
+		amd_pmc_write_stb(pdev, AMD_PMC_STB_PREDEF);
+
 	return rc;
 }
 
@@ -508,6 +575,10 @@ static int __maybe_unused amd_pmc_resume(struct device *dev)
 	/* Dump the IdleMask to see the blockers */
 	amd_pmc_idlemask_read(pdev, dev, NULL);
 
+	/* Write data incremented by 1 to distinguish in stb_read */
+	if (enable_stb)
+		amd_pmc_write_stb(pdev, AMD_PMC_STB_PREDEF + 1);
+
 	return 0;
 }
 
@@ -524,6 +595,57 @@ static const struct pci_device_id pmc_pci_ids[] = {
 	{ }
 };
 
+static int amd_pmc_write_stb(struct amd_pmc_dev *dev, u32 data) {
+	int rc;
+
+	rc = pci_write_config_dword(dev->rdev, AMD_PMC_STB_INDEX_ADDRESS, AMD_PMC_STB_PMI_0);
+	if (rc) {
+		dev_err(dev->dev, "failed to write addr in stb: 0x%X\n",
+			AMD_PMC_STB_INDEX_ADDRESS);
+		pci_dev_put(dev->rdev);
+		return pcibios_err_to_errno(rc);
+	}
+
+	rc = pci_write_config_dword(dev->rdev, AMD_PMC_STB_INDEX_DATA, data);
+	if (rc) {
+		dev_err(dev->dev, "failed to write data in stb: 0x%X\n",
+			AMD_PMC_STB_INDEX_DATA);
+		pci_dev_put(dev->rdev);
+		return pcibios_err_to_errno(rc);
+	}
+
+	return 0;
+}
+
+static int amd_pmc_read_stb(struct amd_pmc_dev *dev, u32 *buf) {
+	int i, err;
+	u32 value;
+
+	err = pci_write_config_dword(dev->rdev, AMD_PMC_STB_INDEX_ADDRESS, AMD_PMC_STB_PMI_0);
+	if (err) {
+		dev_err(dev->dev, "error writing addr to stb: 0x%X\n",
+			AMD_PMC_STB_INDEX_ADDRESS);
+		pci_dev_put(dev->rdev);
+		return pcibios_err_to_errno(err);
+	}
+
+	for (i = 0; i < FIFO_SIZE; i++) {
+		err = pci_read_config_dword(dev->rdev, AMD_PMC_STB_INDEX_DATA, &value);
+		if (err) {
+			dev_err(dev->dev, "error reading data from stb: 0x%X\n",
+				AMD_PMC_STB_INDEX_DATA);
+			pci_dev_put(dev->rdev);
+			return pcibios_err_to_errno(err);
+		}
+
+		*buf++ = value;
+	}
+
+	return 0;
+}
+
 static int amd_pmc_probe(struct platform_device *pdev)  {
 	struct amd_pmc_dev *dev = &pmc;
--
2.25.1

Re: [PATCH v4 3/3] platform/x86: amd-pmc: Add support for AMD Smart Trace Buffer

[Scott Bruce]

Tested-by: Scott Bruce <smbruce@gmail.com>

On Tue, Oct 26, 2021 at 11:57 AM Goswami, Sanket <Sanket.Goswami@amd.com> wrote:
>
> [AMD Official Use Only]
>
> +Scott
>
> -----Original Message-----
> From: Goswami, Sanket <Sanket.Goswami@amd.com>
> Sent: Wednesday, October 27, 2021 0:11
> To: S-k, Shyam-sundar <Shyam-sundar.S-k@amd.com>; hdegoede@redhat.com; mgross@linux.intel.com
> Cc: platform-driver-x86@vger.kernel.org; Goswami, Sanket <Sanket.Goswami@amd.com>
> Subject: [PATCH v4 3/3] platform/x86: amd-pmc: Add support for AMD Smart Trace Buffer
>
> STB (Smart Trace Buffer), is a debug trace buffer which is used to help isolate failures by analyzing the last feature that a system was running before hitting a failure. This nonintrusive way is always running in the background and trace is stored into the SoC.
>
> This patch provides mechanism to access the STB buffer using the read and write routines.
>
> Co-developed-by: Shyam Sundar S K <Shyam-sundar.S-k@amd.com>
> Signed-off-by: Shyam Sundar S K <Shyam-sundar.S-k@amd.com>
> Signed-off-by: Sanket Goswami <Sanket.Goswami@amd.com>
> ---
> Changes in v4:
> - Use kzalloc() for memory allocation.
>
> Changes in v3:
> - Address review comments from Mark Gross.
>
> Changes in v2:
> - Create amd_pmc_stb_debugfs_fops structure to get STB data.
> - Address review comments from Hans.
>
>  drivers/platform/x86/amd-pmc.c | 122 +++++++++++++++++++++++++++++++++
>  1 file changed, 122 insertions(+)
>
> diff --git a/drivers/platform/x86/amd-pmc.c b/drivers/platform/x86/amd-pmc.c index 50cb65e38d11..e55c48f675e1 100644
> --- a/drivers/platform/x86/amd-pmc.c
> +++ b/drivers/platform/x86/amd-pmc.c
> @@ -35,6 +35,12 @@
>  #define AMD_PMC_SCRATCH_REG_CZN                0x94
>  #define AMD_PMC_SCRATCH_REG_YC         0xD14
>
> +/* STB Registers */
> +#define AMD_PMC_STB_INDEX_ADDRESS      0xF8
> +#define AMD_PMC_STB_INDEX_DATA         0xFC
> +#define AMD_PMC_STB_PMI_0              0x03E30600
> +#define AMD_PMC_STB_PREDEF             0xC6000001
> +
>  /* Base address of SMU for mapping physical address to virtual address */
>  #define AMD_PMC_SMU_INDEX_ADDRESS      0xB8
>  #define AMD_PMC_SMU_INDEX_DATA         0xBC
> @@ -82,6 +88,7 @@
>  #define SOC_SUBSYSTEM_IP_MAX   12
>  #define DELAY_MIN_US           2000
>  #define DELAY_MAX_US           3000
> +#define FIFO_SIZE              4096
>  enum amd_pmc_def {
>         MSG_TEST = 0x01,
>         MSG_OS_HINT_PCO,
> @@ -128,8 +135,14 @@ struct amd_pmc_dev {  #endif /* CONFIG_DEBUG_FS */  };
>
> +static bool enable_stb;
> +module_param(enable_stb, bool, 0644);
> +MODULE_PARM_DESC(enable_stb, "Enable the STB debug mechanism");
> +
>  static struct amd_pmc_dev pmc;
>  static int amd_pmc_send_cmd(struct amd_pmc_dev *dev, u32 arg, u32 *data, u8 msg, bool ret);
> +static int amd_pmc_write_stb(struct amd_pmc_dev *dev, u32 data); static
> +int amd_pmc_read_stb(struct amd_pmc_dev *dev, u32 *buf);
>
>  static inline u32 amd_pmc_reg_read(struct amd_pmc_dev *dev, int reg_offset)  { @@ -176,6 +189,53 @@ static int amd_pmc_get_smu_version(struct amd_pmc_dev *dev)
>         return 0;
>  }
>
> +static int amd_pmc_stb_debugfs_open(struct inode *inode, struct file
> +*filp) {
> +       struct amd_pmc_dev *dev = filp->f_inode->i_private;
> +       u32 size = FIFO_SIZE * sizeof(u32);
> +       u32 *buf;
> +       int rc;
> +
> +       buf = kzalloc(size, GFP_KERNEL);
> +       if (!buf)
> +               return -ENOMEM;
> +
> +       rc = amd_pmc_read_stb(dev, buf);
> +       if (rc)
> +               goto out;
> +
> +       filp->private_data = buf;
> +
> +out:
> +       kfree(buf);
> +       return rc;
> +}
> +
> +static ssize_t amd_pmc_stb_debugfs_read(struct file *filp, char __user *buf, size_t size,
> +                                       loff_t *pos)
> +{
> +       if (!filp->private_data)
> +               return -EINVAL;
> +
> +       return simple_read_from_buffer(buf, size, pos, filp->private_data,
> +                                      FIFO_SIZE * sizeof(u32));
> +}
> +
> +static int amd_pmc_stb_debugfs_release(struct inode *inode, struct file
> +*filp) {
> +       kfree(filp->private_data);
> +       filp->private_data = NULL;
> +
> +       return 0;
> +}
> +
> +const struct file_operations amd_pmc_stb_debugfs_fops = {
> +       .owner = THIS_MODULE,
> +       .open = amd_pmc_stb_debugfs_open,
> +       .read = amd_pmc_stb_debugfs_read,
> +       .release = amd_pmc_stb_debugfs_release, };
> +
>  static int amd_pmc_idlemask_read(struct amd_pmc_dev *pdev, struct device *dev,
>                                  struct seq_file *s)
>  {
> @@ -289,6 +349,10 @@ static void amd_pmc_dbgfs_register(struct amd_pmc_dev *dev)
>                             &s0ix_stats_fops);
>         debugfs_create_file("amd_pmc_idlemask", 0644, dev->dbgfs_dir, dev,
>                             &amd_pmc_idlemask_fops);
> +       /* Enable STB only when the module_param is set */
> +       if (enable_stb)
> +               debugfs_create_file("stb_read", 0644, dev->dbgfs_dir, dev,
> +                                   &amd_pmc_stb_debugfs_fops);
>  }
>  #else
>  static inline void amd_pmc_dbgfs_register(struct amd_pmc_dev *dev) @@ -488,6 +552,9 @@ static int __maybe_unused amd_pmc_suspend(struct device *dev)
>         if (rc)
>                 dev_err(pdev->dev, "suspend failed\n");
>
> +       if (enable_stb)
> +               amd_pmc_write_stb(pdev, AMD_PMC_STB_PREDEF);
> +
>         return rc;
>  }
>
> @@ -508,6 +575,10 @@ static int __maybe_unused amd_pmc_resume(struct device *dev)
>         /* Dump the IdleMask to see the blockers */
>         amd_pmc_idlemask_read(pdev, dev, NULL);
>
> +       /* Write data incremented by 1 to distinguish in stb_read */
> +       if (enable_stb)
> +               amd_pmc_write_stb(pdev, AMD_PMC_STB_PREDEF + 1);
> +
>         return 0;
>  }
>
> @@ -524,6 +595,57 @@ static const struct pci_device_id pmc_pci_ids[] = {
>         { }
>  };
>
> +static int amd_pmc_write_stb(struct amd_pmc_dev *dev, u32 data) {
> +       int rc;
> +
> +       rc = pci_write_config_dword(dev->rdev, AMD_PMC_STB_INDEX_ADDRESS, AMD_PMC_STB_PMI_0);
> +       if (rc) {
> +               dev_err(dev->dev, "failed to write addr in stb: 0x%X\n",
> +                       AMD_PMC_STB_INDEX_ADDRESS);
> +               pci_dev_put(dev->rdev);
> +               return pcibios_err_to_errno(rc);
> +       }
> +
> +       rc = pci_write_config_dword(dev->rdev, AMD_PMC_STB_INDEX_DATA, data);
> +       if (rc) {
> +               dev_err(dev->dev, "failed to write data in stb: 0x%X\n",
> +                       AMD_PMC_STB_INDEX_DATA);
> +               pci_dev_put(dev->rdev);
> +               return pcibios_err_to_errno(rc);
> +       }
> +
> +       return 0;
> +}
> +
> +static int amd_pmc_read_stb(struct amd_pmc_dev *dev, u32 *buf) {
> +       int i, err;
> +       u32 value;
> +
> +       err = pci_write_config_dword(dev->rdev, AMD_PMC_STB_INDEX_ADDRESS, AMD_PMC_STB_PMI_0);
> +       if (err) {
> +               dev_err(dev->dev, "error writing addr to stb: 0x%X\n",
> +                       AMD_PMC_STB_INDEX_ADDRESS);
> +               pci_dev_put(dev->rdev);
> +               return pcibios_err_to_errno(err);
> +       }
> +
> +       for (i = 0; i < FIFO_SIZE; i++) {
> +               err = pci_read_config_dword(dev->rdev, AMD_PMC_STB_INDEX_DATA, &value);
> +               if (err) {
> +                       dev_err(dev->dev, "error reading data from stb: 0x%X\n",
> +                               AMD_PMC_STB_INDEX_DATA);
> +                       pci_dev_put(dev->rdev);
> +                       return pcibios_err_to_errno(err);
> +               }
> +
> +               *buf++ = value;
> +       }
> +
> +       return 0;
> +}
> +
>  static int amd_pmc_probe(struct platform_device *pdev)  {
>         struct amd_pmc_dev *dev = &pmc;
> --
> 2.25.1

Re: [PATCH v4 1/3] platform/x86: amd-pmc: Store the pci_dev instance inside struct amd_pmc_dev

[Hans de Goede]

Hi Sanket,

On 10/26/21 20:40, Sanket Goswami wrote:
> Store the root port information in amd_pmc_probe() so that the
> information can be used across multiple routines.
> 
> Signed-off-by: Sanket Goswami <Sanket.Goswami@amd.com>
> ---
> Changes in v3:
> - Address review comments from Hans.
> 
> Changes in v2:
> - Store the rdev info in amd_pmc_probe() as suggested by Hans.
> 
>  drivers/platform/x86/amd-pmc.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/platform/x86/amd-pmc.c b/drivers/platform/x86/amd-pmc.c
> index 678bf6874c63..5d88e55e1ce7 100644
> --- a/drivers/platform/x86/amd-pmc.c
> +++ b/drivers/platform/x86/amd-pmc.c
> @@ -121,6 +121,7 @@ struct amd_pmc_dev {
>  	u16 minor;
>  	u16 rev;
>  	struct device *dev;
> +	struct pci_dev *rdev;
>  	struct mutex lock; /* generic mutex lock */
>  #if IS_ENABLED(CONFIG_DEBUG_FS)
>  	struct dentry *dbgfs_dir;
> @@ -541,6 +542,7 @@ static int amd_pmc_probe(struct platform_device *pdev)
>  	}
>  
>  	dev->cpu_id = rdev->device;
> +	dev->rdev = rdev;
>  	err = pci_write_config_dword(rdev, AMD_PMC_SMU_INDEX_ADDRESS, AMD_PMC_BASE_ADDR_LO);
>  	if (err) {
>  		dev_err(dev->dev, "error writing to 0x%x\n", AMD_PMC_SMU_INDEX_ADDRESS);

You are still introducing a leak of the rdev on error-exits here,
there are error-exits here:

        dev->regbase = devm_ioremap(dev->dev, base_addr + AMD_PMC_BASE_ADDR_OFFSET,
                                    AMD_PMC_MAPPING_SIZE);
        if (!dev->regbase)
                return -ENOMEM;

and here:

        dev->fch_virt_addr = devm_ioremap(dev->dev, fch_phys_addr, FCH_SSC_MAPPING_SIZE);
        if (!dev->fch_virt_addr)
                return -ENOMEM;

Which now also need a pci_dev_put() call before the "return -ENOMEM". Please make
patch 2/3 the first patch in the series and then use goto err_pci_dev_put here.

Note you will also need to deal with setting err, and with these paths not
using pcibios_err_to_errno() ...

Regards,

Hans




> @@ -570,7 +572,6 @@ static int amd_pmc_probe(struct platform_device *pdev)
>  	}
>  
>  	base_addr_hi = val & AMD_PMC_BASE_ADDR_LO_MASK;
> -	pci_dev_put(rdev);
>  	base_addr = ((u64)base_addr_hi << 32 | base_addr_lo);
>  
>  	dev->regbase = devm_ioremap(dev->dev, base_addr + AMD_PMC_BASE_ADDR_OFFSET,
> @@ -604,6 +605,7 @@ static int amd_pmc_remove(struct platform_device *pdev)
>  	struct amd_pmc_dev *dev = platform_get_drvdata(pdev);
>  
>  	amd_pmc_dbgfs_unregister(dev);
> +	pci_dev_put(dev->rdev);
>  	mutex_destroy(&dev->lock);
>  	return 0;
>  }
> 

Re: [PATCH v4 2/3] platform/x86: amd-pmc: Simplify error handling path

[Hans de Goede]

Hi,

On 10/26/21 20:40, Sanket Goswami wrote:
> Handle error-exits in the amd_pmc_probe() so that the code duplication
> is reduced.
> 
> Suggested-by: Hans de Goede <hdegoede@redhat.com>
> Signed-off-by: Sanket Goswami <Sanket.Goswami@amd.com>
> ---
> Changes in v4:
> - No change.
> 
> Changes in v3:
> - No change.
> 
> Changes in v2:
> - No change.
> 
>  drivers/platform/x86/amd-pmc.c | 22 ++++++++++------------
>  1 file changed, 10 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/platform/x86/amd-pmc.c b/drivers/platform/x86/amd-pmc.c
> index 5d88e55e1ce7..50cb65e38d11 100644
> --- a/drivers/platform/x86/amd-pmc.c
> +++ b/drivers/platform/x86/amd-pmc.c
> @@ -546,30 +546,24 @@ static int amd_pmc_probe(struct platform_device *pdev)
>  	err = pci_write_config_dword(rdev, AMD_PMC_SMU_INDEX_ADDRESS, AMD_PMC_BASE_ADDR_LO);
>  	if (err) {
>  		dev_err(dev->dev, "error writing to 0x%x\n", AMD_PMC_SMU_INDEX_ADDRESS);
> -		pci_dev_put(rdev);
> -		return pcibios_err_to_errno(err);

You probably want to add:

		err = pcibios_err_to_errno(err);

here and in the other similar cases.

> +		goto err_pci_dev_put;
>  	}
>  
>  	err = pci_read_config_dword(rdev, AMD_PMC_SMU_INDEX_DATA, &val);
> -	if (err) {
> -		pci_dev_put(rdev);
> -		return pcibios_err_to_errno(err);
> -	}
> +	if (err)
> +		goto err_pci_dev_put;
>  
>  	base_addr_lo = val & AMD_PMC_BASE_ADDR_HI_MASK;
>  
>  	err = pci_write_config_dword(rdev, AMD_PMC_SMU_INDEX_ADDRESS, AMD_PMC_BASE_ADDR_HI);
>  	if (err) {
>  		dev_err(dev->dev, "error writing to 0x%x\n", AMD_PMC_SMU_INDEX_ADDRESS);
> -		pci_dev_put(rdev);
> -		return pcibios_err_to_errno(err);
> +		goto err_pci_dev_put;
>  	}
>  
>  	err = pci_read_config_dword(rdev, AMD_PMC_SMU_INDEX_DATA, &val);
> -	if (err) {
> -		pci_dev_put(rdev);
> -		return pcibios_err_to_errno(err);
> -	}
> +	if (err)
> +		goto err_pci_dev_put;
>  
>  	base_addr_hi = val & AMD_PMC_BASE_ADDR_LO_MASK;
>  	base_addr = ((u64)base_addr_hi << 32 | base_addr_lo);
> @@ -598,6 +592,10 @@ static int amd_pmc_probe(struct platform_device *pdev)
>  	platform_set_drvdata(pdev, dev);
>  	amd_pmc_dbgfs_register(dev);
>  	return 0;
> +
> +err_pci_dev_put:
> +		pci_dev_put(rdev);
> +		return pcibios_err_to_errno(err);

And change this to just:

		return err;

So that "goto err_pci_dev_put" can also be used for errors which are not
returned by pci_read/write_config_dword() .


>  }
>  
>  static int amd_pmc_remove(struct platform_device *pdev)
> 

Regards,

Hans

Re: [PATCH v4 3/3] platform/x86: amd-pmc: Add support for AMD Smart Trace Buffer

[Hans de Goede]

Hi,

On 10/26/21 20:40, Sanket Goswami wrote:
> STB (Smart Trace Buffer), is a debug trace buffer which is used to help
> isolate failures by analyzing the last feature that a system was running
> before hitting a failure. This nonintrusive way is always running in the
> background and trace is stored into the SoC.
> 
> This patch provides mechanism to access the STB buffer using the read
> and write routines.
> 
> Co-developed-by: Shyam Sundar S K <Shyam-sundar.S-k@amd.com>
> Signed-off-by: Shyam Sundar S K <Shyam-sundar.S-k@amd.com>
> Signed-off-by: Sanket Goswami <Sanket.Goswami@amd.com>
> ---
> Changes in v4:
> - Use kzalloc() for memory allocation.
> 
> Changes in v3:
> - Address review comments from Mark Gross.
> 
> Changes in v2:
> - Create amd_pmc_stb_debugfs_fops structure to get STB data.
> - Address review comments from Hans.
> 
>  drivers/platform/x86/amd-pmc.c | 122 +++++++++++++++++++++++++++++++++
>  1 file changed, 122 insertions(+)
> 
> diff --git a/drivers/platform/x86/amd-pmc.c b/drivers/platform/x86/amd-pmc.c
> index 50cb65e38d11..e55c48f675e1 100644
> --- a/drivers/platform/x86/amd-pmc.c
> +++ b/drivers/platform/x86/amd-pmc.c
> @@ -35,6 +35,12 @@
>  #define AMD_PMC_SCRATCH_REG_CZN		0x94
>  #define AMD_PMC_SCRATCH_REG_YC		0xD14
>  
> +/* STB Registers */
> +#define AMD_PMC_STB_INDEX_ADDRESS	0xF8
> +#define AMD_PMC_STB_INDEX_DATA		0xFC
> +#define AMD_PMC_STB_PMI_0		0x03E30600
> +#define AMD_PMC_STB_PREDEF		0xC6000001
> +
>  /* Base address of SMU for mapping physical address to virtual address */
>  #define AMD_PMC_SMU_INDEX_ADDRESS	0xB8
>  #define AMD_PMC_SMU_INDEX_DATA		0xBC
> @@ -82,6 +88,7 @@
>  #define SOC_SUBSYSTEM_IP_MAX	12
>  #define DELAY_MIN_US		2000
>  #define DELAY_MAX_US		3000
> +#define FIFO_SIZE		4096
>  enum amd_pmc_def {
>  	MSG_TEST = 0x01,
>  	MSG_OS_HINT_PCO,
> @@ -128,8 +135,14 @@ struct amd_pmc_dev {
>  #endif /* CONFIG_DEBUG_FS */
>  };
>  
> +static bool enable_stb;
> +module_param(enable_stb, bool, 0644);
> +MODULE_PARM_DESC(enable_stb, "Enable the STB debug mechanism");
> +
>  static struct amd_pmc_dev pmc;
>  static int amd_pmc_send_cmd(struct amd_pmc_dev *dev, u32 arg, u32 *data, u8 msg, bool ret);
> +static int amd_pmc_write_stb(struct amd_pmc_dev *dev, u32 data);
> +static int amd_pmc_read_stb(struct amd_pmc_dev *dev, u32 *buf);
>  
>  static inline u32 amd_pmc_reg_read(struct amd_pmc_dev *dev, int reg_offset)
>  {
> @@ -176,6 +189,53 @@ static int amd_pmc_get_smu_version(struct amd_pmc_dev *dev)
>  	return 0;
>  }
>  
> +static int amd_pmc_stb_debugfs_open(struct inode *inode, struct file *filp)
> +{
> +	struct amd_pmc_dev *dev = filp->f_inode->i_private;
> +	u32 size = FIFO_SIZE * sizeof(u32);
> +	u32 *buf;
> +	int rc;
> +
> +	buf = kzalloc(size, GFP_KERNEL);
> +	if (!buf)
> +		return -ENOMEM;
> +
> +	rc = amd_pmc_read_stb(dev, buf);
> +	if (rc)
> +		goto out;

goto err? I believe you don't want to kfree the buffer
on success.

> +
> +	filp->private_data = buf;

And add a:

	return 0;

Here so that you don't fall-through to out and
free the buffer.

> +
> +out:
> +	kfree(buf);
> +	return rc;
> +}
> +
> +static ssize_t amd_pmc_stb_debugfs_read(struct file *filp, char __user *buf, size_t size,
> +					loff_t *pos)
> +{
> +	if (!filp->private_data)
> +		return -EINVAL;
> +
> +	return simple_read_from_buffer(buf, size, pos, filp->private_data,
> +				       FIFO_SIZE * sizeof(u32));

With the changes suggested above this refferences already free-ed mem.

> +}
> +
> +static int amd_pmc_stb_debugfs_release(struct inode *inode, struct file *filp)
> +{
> +	kfree(filp->private_data);

And this is a double free !

> +	filp->private_data = NULL;
> +
> +	return 0;
> +}
> +
> +const struct file_operations amd_pmc_stb_debugfs_fops = {
> +	.owner = THIS_MODULE,
> +	.open = amd_pmc_stb_debugfs_open,
> +	.read = amd_pmc_stb_debugfs_read,
> +	.release = amd_pmc_stb_debugfs_release,
> +};
> +
>  static int amd_pmc_idlemask_read(struct amd_pmc_dev *pdev, struct device *dev,
>  				 struct seq_file *s)
>  {
> @@ -289,6 +349,10 @@ static void amd_pmc_dbgfs_register(struct amd_pmc_dev *dev)
>  			    &s0ix_stats_fops);
>  	debugfs_create_file("amd_pmc_idlemask", 0644, dev->dbgfs_dir, dev,
>  			    &amd_pmc_idlemask_fops);
> +	/* Enable STB only when the module_param is set */
> +	if (enable_stb)
> +		debugfs_create_file("stb_read", 0644, dev->dbgfs_dir, dev,
> +				    &amd_pmc_stb_debugfs_fops);
>  }
>  #else
>  static inline void amd_pmc_dbgfs_register(struct amd_pmc_dev *dev)
> @@ -488,6 +552,9 @@ static int __maybe_unused amd_pmc_suspend(struct device *dev)
>  	if (rc)
>  		dev_err(pdev->dev, "suspend failed\n");
>  
> +	if (enable_stb)
> +		amd_pmc_write_stb(pdev, AMD_PMC_STB_PREDEF);
> +
>  	return rc;
>  }
>  
> @@ -508,6 +575,10 @@ static int __maybe_unused amd_pmc_resume(struct device *dev)
>  	/* Dump the IdleMask to see the blockers */
>  	amd_pmc_idlemask_read(pdev, dev, NULL);
>  
> +	/* Write data incremented by 1 to distinguish in stb_read */
> +	if (enable_stb)
> +		amd_pmc_write_stb(pdev, AMD_PMC_STB_PREDEF + 1);
> +
>  	return 0;
>  }
>  
> @@ -524,6 +595,57 @@ static const struct pci_device_id pmc_pci_ids[] = {
>  	{ }
>  };
>  
> +static int amd_pmc_write_stb(struct amd_pmc_dev *dev, u32 data)
> +{
> +	int rc;
> +
> +	rc = pci_write_config_dword(dev->rdev, AMD_PMC_STB_INDEX_ADDRESS, AMD_PMC_STB_PMI_0);
> +	if (rc) {
> +		dev_err(dev->dev, "failed to write addr in stb: 0x%X\n",
> +			AMD_PMC_STB_INDEX_ADDRESS);
> +		pci_dev_put(dev->rdev);
> +		return pcibios_err_to_errno(rc);
> +	}
> +
> +	rc = pci_write_config_dword(dev->rdev, AMD_PMC_STB_INDEX_DATA, data);
> +	if (rc) {
> +		dev_err(dev->dev, "failed to write data in stb: 0x%X\n",
> +			AMD_PMC_STB_INDEX_DATA);
> +		pci_dev_put(dev->rdev);
> +		return pcibios_err_to_errno(rc);
> +	}
> +
> +	return 0;
> +}
> +
> +static int amd_pmc_read_stb(struct amd_pmc_dev *dev, u32 *buf)
> +{
> +	int i, err;
> +	u32 value;
> +
> +	err = pci_write_config_dword(dev->rdev, AMD_PMC_STB_INDEX_ADDRESS, AMD_PMC_STB_PMI_0);
> +	if (err) {
> +		dev_err(dev->dev, "error writing addr to stb: 0x%X\n",
> +			AMD_PMC_STB_INDEX_ADDRESS);
> +		pci_dev_put(dev->rdev);
> +		return pcibios_err_to_errno(err);
> +	}
> +
> +	for (i = 0; i < FIFO_SIZE; i++) {
> +		err = pci_read_config_dword(dev->rdev, AMD_PMC_STB_INDEX_DATA, &value);
> +		if (err) {
> +			dev_err(dev->dev, "error reading data from stb: 0x%X\n",
> +				AMD_PMC_STB_INDEX_DATA);
> +			pci_dev_put(dev->rdev);
> +			return pcibios_err_to_errno(err);
> +		}
> +
> +		*buf++ = value;
> +	}
> +
> +	return 0;
> +}
> +
>  static int amd_pmc_probe(struct platform_device *pdev)
>  {
>  	struct amd_pmc_dev *dev = &pmc;
> 

Regards,

Hans