* [PATCH v5 0/3] Updates to amd-pmc driver
@ 2021-10-29 17:23 Sanket Goswami
2021-10-29 17:23 ` [PATCH v5 1/3] platform/x86: amd-pmc: Simplify error handling path Sanket Goswami
` (2 more replies)
0 siblings, 3 replies; 10+ messages in thread
From: Sanket Goswami @ 2021-10-29 17:23 UTC (permalink / raw)
To: Shyam-sundar.S-k, hdegoede, mgross; +Cc: platform-driver-x86, 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: Simplify error handling path
platform/x86: amd-pmc: Store the pci_dev instance inside struct
amd_pmc_dev
platform/x86: amd-pmc: Add support for AMD Smart Trace Buffer
drivers/platform/x86/amd-pmc.c | 153 ++++++++++++++++++++++++++++++---
1 file changed, 143 insertions(+), 10 deletions(-)
--
2.25.1
^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH v5 1/3] platform/x86: amd-pmc: Simplify error handling path
2021-10-29 17:23 [PATCH v5 0/3] Updates to amd-pmc driver Sanket Goswami
@ 2021-10-29 17:23 ` Sanket Goswami
2021-10-29 23:27 ` Scott Bruce
2021-10-29 17:23 ` [PATCH v5 2/3] platform/x86: amd-pmc: Store the pci_dev instance inside struct amd_pmc_dev Sanket Goswami
2021-10-29 17:23 ` [PATCH v5 3/3] platform/x86: amd-pmc: Add support for AMD Smart Trace Buffer Sanket Goswami
2 siblings, 1 reply; 10+ messages in thread
From: Sanket Goswami @ 2021-10-29 17:23 UTC (permalink / raw)
To: Shyam-sundar.S-k, hdegoede, mgross; +Cc: platform-driver-x86, 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 v5:
- Use goto label incase of error-exit path.
Changes in v4:
- No change.
Changes in v3:
- No change.
Changes in v2:
- No change.
drivers/platform/x86/amd-pmc.c | 24 ++++++++++++++----------
1 file changed, 14 insertions(+), 10 deletions(-)
diff --git a/drivers/platform/x86/amd-pmc.c b/drivers/platform/x86/amd-pmc.c
index b7e50ed050a8..9af02860ed59 100644
--- a/drivers/platform/x86/amd-pmc.c
+++ b/drivers/platform/x86/amd-pmc.c
@@ -533,22 +533,22 @@ static int amd_pmc_probe(struct platform_device *pdev)
rdev = pci_get_domain_bus_and_slot(0, 0, PCI_DEVFN(0, 0));
if (!rdev || !pci_match_id(pmc_pci_ids, rdev)) {
- pci_dev_put(rdev);
- return -ENODEV;
+ err = -ENODEV;
+ goto err_pci_dev_put;
}
dev->cpu_id = rdev->device;
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);
+ err = 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);
+ err = pcibios_err_to_errno(err);
+ goto err_pci_dev_put;
}
base_addr_lo = val & AMD_PMC_BASE_ADDR_HI_MASK;
@@ -556,14 +556,14 @@ static int amd_pmc_probe(struct platform_device *pdev)
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);
+ err = 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);
+ err = pcibios_err_to_errno(err);
+ goto err_pci_dev_put;
}
base_addr_hi = val & AMD_PMC_BASE_ADDR_LO_MASK;
@@ -594,6 +594,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 err;
}
static int amd_pmc_remove(struct platform_device *pdev)
--
2.25.1
^ permalink raw reply related [flat|nested] 10+ messages in thread
* [PATCH v5 2/3] platform/x86: amd-pmc: Store the pci_dev instance inside struct amd_pmc_dev
2021-10-29 17:23 [PATCH v5 0/3] Updates to amd-pmc driver Sanket Goswami
2021-10-29 17:23 ` [PATCH v5 1/3] platform/x86: amd-pmc: Simplify error handling path Sanket Goswami
@ 2021-10-29 17:23 ` Sanket Goswami
2021-10-29 23:27 ` Scott Bruce
2021-11-16 13:31 ` Hans de Goede
2021-10-29 17:23 ` [PATCH v5 3/3] platform/x86: amd-pmc: Add support for AMD Smart Trace Buffer Sanket Goswami
2 siblings, 2 replies; 10+ messages in thread
From: Sanket Goswami @ 2021-10-29 17:23 UTC (permalink / raw)
To: Shyam-sundar.S-k, hdegoede, mgross; +Cc: platform-driver-x86, 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 v5:
- Remove pci_dev_put() from amd_pmc_remove() as its no longer required.
Changes in v4:
- No change.
Changes in v3:
- Add pci_dev_put() in amd_pmc_remove().
Changes in v2:
- Store the rdev info in amd_pmc_probe() as suggested by Hans.
drivers/platform/x86/amd-pmc.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/drivers/platform/x86/amd-pmc.c b/drivers/platform/x86/amd-pmc.c
index 9af02860ed59..ea099f7759f2 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;
@@ -538,6 +539,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);
--
2.25.1
^ permalink raw reply related [flat|nested] 10+ messages in thread
* [PATCH v5 3/3] platform/x86: amd-pmc: Add support for AMD Smart Trace Buffer
2021-10-29 17:23 [PATCH v5 0/3] Updates to amd-pmc driver Sanket Goswami
2021-10-29 17:23 ` [PATCH v5 1/3] platform/x86: amd-pmc: Simplify error handling path Sanket Goswami
2021-10-29 17:23 ` [PATCH v5 2/3] platform/x86: amd-pmc: Store the pci_dev instance inside struct amd_pmc_dev Sanket Goswami
@ 2021-10-29 17:23 ` Sanket Goswami
2021-10-29 23:26 ` Scott Bruce
2021-11-03 18:39 ` Limonciello, Mario
2 siblings, 2 replies; 10+ messages in thread
From: Sanket Goswami @ 2021-10-29 17:23 UTC (permalink / raw)
To: Shyam-sundar.S-k, hdegoede, mgross; +Cc: platform-driver-x86, 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 v5:
- Use kfree() only once in .open as suggested by Hans.
Changes in v4:
- Use kzalloc() for memory allocation.
Changes in v3:
- Use sizeof(u32) with multiplier as suggested by 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 | 127 +++++++++++++++++++++++++++++++++
1 file changed, 127 insertions(+)
diff --git a/drivers/platform/x86/amd-pmc.c b/drivers/platform/x86/amd-pmc.c
index ea099f7759f2..a39354ea96c4 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,50 @@ 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) {
+ kfree(buf);
+ return rc;
+ }
+
+ filp->private_data = 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);
+ 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 +346,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)
@@ -485,6 +546,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;
}
@@ -505,6 +569,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;
}
@@ -521,6 +589,65 @@ static const struct pci_device_id pmc_pci_ids[] = {
{ }
};
+static int amd_pmc_write_stb(struct amd_pmc_dev *dev, u32 data)
+{
+ int err;
+
+ err = pci_write_config_dword(dev->rdev, AMD_PMC_STB_INDEX_ADDRESS, AMD_PMC_STB_PMI_0);
+ if (err) {
+ dev_err(dev->dev, "failed to write addr in stb: 0x%X\n",
+ AMD_PMC_STB_INDEX_ADDRESS);
+ err = pcibios_err_to_errno(err);
+ goto err_pci_dev_put;
+ }
+
+ err = pci_write_config_dword(dev->rdev, AMD_PMC_STB_INDEX_DATA, data);
+ if (err) {
+ dev_err(dev->dev, "failed to write data in stb: 0x%X\n",
+ AMD_PMC_STB_INDEX_DATA);
+ err = pcibios_err_to_errno(err);
+ goto err_pci_dev_put;
+ }
+
+ return 0;
+
+err_pci_dev_put:
+ pci_dev_put(dev->rdev);
+ return err;
+}
+
+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);
+ err = pcibios_err_to_errno(err);
+ goto err_pci_dev_put;
+ }
+
+ 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);
+ err = pcibios_err_to_errno(err);
+ goto err_pci_dev_put;
+ }
+
+ *buf++ = value;
+ }
+
+ return 0;
+
+err_pci_dev_put:
+ pci_dev_put(dev->rdev);
+ return err;
+}
+
static int amd_pmc_probe(struct platform_device *pdev)
{
struct amd_pmc_dev *dev = &pmc;
--
2.25.1
^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH v5 3/3] platform/x86: amd-pmc: Add support for AMD Smart Trace Buffer
2021-10-29 17:23 ` [PATCH v5 3/3] platform/x86: amd-pmc: Add support for AMD Smart Trace Buffer Sanket Goswami
@ 2021-10-29 23:26 ` Scott Bruce
2021-11-03 18:39 ` Limonciello, Mario
1 sibling, 0 replies; 10+ messages in thread
From: Scott Bruce @ 2021-10-29 23:26 UTC (permalink / raw)
To: Sanket Goswami; +Cc: Shyam Sundar S K, hdegoede, mgross, platform-driver-x86
Tested-by: Scott Bruce <smbruce@gmail.com>
On Fri, Oct 29, 2021 at 10:25 AM Sanket Goswami <Sanket.Goswami@amd.com> 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 v5:
> - Use kfree() only once in .open as suggested by Hans.
>
> Changes in v4:
> - Use kzalloc() for memory allocation.
>
> Changes in v3:
> - Use sizeof(u32) with multiplier as suggested by 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 | 127 +++++++++++++++++++++++++++++++++
> 1 file changed, 127 insertions(+)
>
> diff --git a/drivers/platform/x86/amd-pmc.c b/drivers/platform/x86/amd-pmc.c
> index ea099f7759f2..a39354ea96c4 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,50 @@ 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) {
> + kfree(buf);
> + return rc;
> + }
> +
> + filp->private_data = 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);
> + 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 +346,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)
> @@ -485,6 +546,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;
> }
>
> @@ -505,6 +569,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;
> }
>
> @@ -521,6 +589,65 @@ static const struct pci_device_id pmc_pci_ids[] = {
> { }
> };
>
> +static int amd_pmc_write_stb(struct amd_pmc_dev *dev, u32 data)
> +{
> + int err;
> +
> + err = pci_write_config_dword(dev->rdev, AMD_PMC_STB_INDEX_ADDRESS, AMD_PMC_STB_PMI_0);
> + if (err) {
> + dev_err(dev->dev, "failed to write addr in stb: 0x%X\n",
> + AMD_PMC_STB_INDEX_ADDRESS);
> + err = pcibios_err_to_errno(err);
> + goto err_pci_dev_put;
> + }
> +
> + err = pci_write_config_dword(dev->rdev, AMD_PMC_STB_INDEX_DATA, data);
> + if (err) {
> + dev_err(dev->dev, "failed to write data in stb: 0x%X\n",
> + AMD_PMC_STB_INDEX_DATA);
> + err = pcibios_err_to_errno(err);
> + goto err_pci_dev_put;
> + }
> +
> + return 0;
> +
> +err_pci_dev_put:
> + pci_dev_put(dev->rdev);
> + return err;
> +}
> +
> +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);
> + err = pcibios_err_to_errno(err);
> + goto err_pci_dev_put;
> + }
> +
> + 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);
> + err = pcibios_err_to_errno(err);
> + goto err_pci_dev_put;
> + }
> +
> + *buf++ = value;
> + }
> +
> + return 0;
> +
> +err_pci_dev_put:
> + pci_dev_put(dev->rdev);
> + return err;
> +}
> +
> static int amd_pmc_probe(struct platform_device *pdev)
> {
> struct amd_pmc_dev *dev = &pmc;
> --
> 2.25.1
>
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v5 1/3] platform/x86: amd-pmc: Simplify error handling path
2021-10-29 17:23 ` [PATCH v5 1/3] platform/x86: amd-pmc: Simplify error handling path Sanket Goswami
@ 2021-10-29 23:27 ` Scott Bruce
0 siblings, 0 replies; 10+ messages in thread
From: Scott Bruce @ 2021-10-29 23:27 UTC (permalink / raw)
To: Sanket Goswami; +Cc: Shyam Sundar S K, hdegoede, mgross, platform-driver-x86
Tested-by: Scott Bruce <smbruce@gmail.com>
On Fri, Oct 29, 2021 at 10:25 AM Sanket Goswami <Sanket.Goswami@amd.com> 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 v5:
> - Use goto label incase of error-exit path.
>
> Changes in v4:
> - No change.
>
> Changes in v3:
> - No change.
>
> Changes in v2:
> - No change.
>
> drivers/platform/x86/amd-pmc.c | 24 ++++++++++++++----------
> 1 file changed, 14 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/platform/x86/amd-pmc.c b/drivers/platform/x86/amd-pmc.c
> index b7e50ed050a8..9af02860ed59 100644
> --- a/drivers/platform/x86/amd-pmc.c
> +++ b/drivers/platform/x86/amd-pmc.c
> @@ -533,22 +533,22 @@ static int amd_pmc_probe(struct platform_device *pdev)
>
> rdev = pci_get_domain_bus_and_slot(0, 0, PCI_DEVFN(0, 0));
> if (!rdev || !pci_match_id(pmc_pci_ids, rdev)) {
> - pci_dev_put(rdev);
> - return -ENODEV;
> + err = -ENODEV;
> + goto err_pci_dev_put;
> }
>
> dev->cpu_id = rdev->device;
> 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);
> + err = 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);
> + err = pcibios_err_to_errno(err);
> + goto err_pci_dev_put;
> }
>
> base_addr_lo = val & AMD_PMC_BASE_ADDR_HI_MASK;
> @@ -556,14 +556,14 @@ static int amd_pmc_probe(struct platform_device *pdev)
> 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);
> + err = 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);
> + err = pcibios_err_to_errno(err);
> + goto err_pci_dev_put;
> }
>
> base_addr_hi = val & AMD_PMC_BASE_ADDR_LO_MASK;
> @@ -594,6 +594,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 err;
> }
>
> static int amd_pmc_remove(struct platform_device *pdev)
> --
> 2.25.1
>
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v5 2/3] platform/x86: amd-pmc: Store the pci_dev instance inside struct amd_pmc_dev
2021-10-29 17:23 ` [PATCH v5 2/3] platform/x86: amd-pmc: Store the pci_dev instance inside struct amd_pmc_dev Sanket Goswami
@ 2021-10-29 23:27 ` Scott Bruce
2021-11-16 13:31 ` Hans de Goede
1 sibling, 0 replies; 10+ messages in thread
From: Scott Bruce @ 2021-10-29 23:27 UTC (permalink / raw)
To: Sanket Goswami; +Cc: Shyam Sundar S K, hdegoede, mgross, platform-driver-x86
Tested-by: Scott Bruce <smbruce@gmail.com>
On Fri, Oct 29, 2021 at 10:25 AM Sanket Goswami <Sanket.Goswami@amd.com> 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 v5:
> - Remove pci_dev_put() from amd_pmc_remove() as its no longer required.
>
> Changes in v4:
> - No change.
>
> Changes in v3:
> - Add pci_dev_put() in amd_pmc_remove().
>
> Changes in v2:
> - Store the rdev info in amd_pmc_probe() as suggested by Hans.
>
> drivers/platform/x86/amd-pmc.c | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/drivers/platform/x86/amd-pmc.c b/drivers/platform/x86/amd-pmc.c
> index 9af02860ed59..ea099f7759f2 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;
> @@ -538,6 +539,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);
> --
> 2.25.1
>
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v5 3/3] platform/x86: amd-pmc: Add support for AMD Smart Trace Buffer
2021-10-29 17:23 ` [PATCH v5 3/3] platform/x86: amd-pmc: Add support for AMD Smart Trace Buffer Sanket Goswami
2021-10-29 23:26 ` Scott Bruce
@ 2021-11-03 18:39 ` Limonciello, Mario
1 sibling, 0 replies; 10+ messages in thread
From: Limonciello, Mario @ 2021-11-03 18:39 UTC (permalink / raw)
To: Sanket Goswami, Shyam-sundar.S-k, hdegoede, mgross; +Cc: platform-driver-x86
On 10/29/2021 12:23, 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>
> Tested-by: Scott Bruce <smbruce@gmail.com>
> ---
> Changes in v5:
> - Use kfree() only once in .open as suggested by Hans.
>
> Changes in v4:
> - Use kzalloc() for memory allocation.
>
> Changes in v3:
> - Use sizeof(u32) with multiplier as suggested by 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 | 127 +++++++++++++++++++++++++++++++++
> 1 file changed, 127 insertions(+)
>
> diff --git a/drivers/platform/x86/amd-pmc.c b/drivers/platform/x86/amd-pmc.c
> index ea099f7759f2..a39354ea96c4 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,50 @@ 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) {
> + kfree(buf);
> + return rc;
> + }
> +
> + filp->private_data = 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);
> + 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 +346,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);
Have you considered using debugfs_create_blob instead? Wouldn't this
make it match the data type more closely?
> }
> #else
> static inline void amd_pmc_dbgfs_register(struct amd_pmc_dev *dev)
> @@ -485,6 +546,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;
> }
>
> @@ -505,6 +569,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;
> }
>
> @@ -521,6 +589,65 @@ static const struct pci_device_id pmc_pci_ids[] = {
> { }
> };
>
> +static int amd_pmc_write_stb(struct amd_pmc_dev *dev, u32 data)
> +{
> + int err;
> +
> + err = pci_write_config_dword(dev->rdev, AMD_PMC_STB_INDEX_ADDRESS, AMD_PMC_STB_PMI_0);
> + if (err) {
> + dev_err(dev->dev, "failed to write addr in stb: 0x%X\n",
> + AMD_PMC_STB_INDEX_ADDRESS);
> + err = pcibios_err_to_errno(err);
> + goto err_pci_dev_put;
> + }
> +
> + err = pci_write_config_dword(dev->rdev, AMD_PMC_STB_INDEX_DATA, data);
> + if (err) {
> + dev_err(dev->dev, "failed to write data in stb: 0x%X\n",
> + AMD_PMC_STB_INDEX_DATA);
> + err = pcibios_err_to_errno(err);
> + goto err_pci_dev_put;
> + }
> +
> + return 0;
> +
> +err_pci_dev_put:
> + pci_dev_put(dev->rdev);
> + return err;
> +}
> +
> +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);
> + err = pcibios_err_to_errno(err);
> + goto err_pci_dev_put;
> + }
> +
> + 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);
> + err = pcibios_err_to_errno(err);
> + goto err_pci_dev_put;
> + }
> +
> + *buf++ = value;
> + }
> +
> + return 0;
> +
> +err_pci_dev_put:
> + pci_dev_put(dev->rdev);
> + return err;
> +}
> +
> static int amd_pmc_probe(struct platform_device *pdev)
> {
> struct amd_pmc_dev *dev = &pmc;
>
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v5 2/3] platform/x86: amd-pmc: Store the pci_dev instance inside struct amd_pmc_dev
2021-10-29 17:23 ` [PATCH v5 2/3] platform/x86: amd-pmc: Store the pci_dev instance inside struct amd_pmc_dev Sanket Goswami
2021-10-29 23:27 ` Scott Bruce
@ 2021-11-16 13:31 ` Hans de Goede
2021-11-30 11:25 ` Goswami, Sanket
1 sibling, 1 reply; 10+ messages in thread
From: Hans de Goede @ 2021-11-16 13:31 UTC (permalink / raw)
To: Sanket Goswami, Shyam-sundar.S-k, mgross, Limonciello, Mario
Cc: platform-driver-x86
Hi,
On 10/29/21 19:23, 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 v5:
> - Remove pci_dev_put() from amd_pmc_remove() as its no longer required.
>
> Changes in v4:
> - No change.
>
> Changes in v3:
> - Add pci_dev_put() in amd_pmc_remove().
>
> Changes in v2:
> - Store the rdev info in amd_pmc_probe() as suggested by Hans.
>
> drivers/platform/x86/amd-pmc.c | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/drivers/platform/x86/amd-pmc.c b/drivers/platform/x86/amd-pmc.c
> index 9af02860ed59..ea099f7759f2 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;
> @@ -538,6 +539,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);
>
I'm afraid this is still not correct:
1. The pci_dev_put() at line 570 is still there, so after that line
you no longer have a reference to the pci_dev and the pointer may
end up pointing to free-ed memory
2. Once you drop the pci_dev_put() at line 570, all the error-exit
paths from probe after that, like this one :
dev->regbase = devm_ioremap(dev->dev, base_addr + AMD_PMC_BASE_ADDR_OFFSET,
AMD_PMC_MAPPING_SIZE);
if (!dev->regbase)
return -ENOMEM;
need to be changed to now, put the rdev (since that is now no longer
done on line 570), so this needs to be changed to:
dev->regbase = devm_ioremap(dev->dev, base_addr + AMD_PMC_BASE_ADDR_OFFSET,
AMD_PMC_MAPPING_SIZE);
if (!dev->regbase) {
err = -ENOMEM;
goto err_pci_dev_put;
}
and the same for:
dev->fch_virt_addr = devm_ioremap(dev->dev, fch_phys_addr, FCH_SSC_MAPPING_SIZE);
if (!dev->fch_virt_addr)
return -ENOMEM;
3. Since you now keep the reference on a succesfull probe, you need to add a
pci_dev_put(dev->rdev);
call to amd_pmc_remove()
Note that the changelog says you already did 3. in v3 but for some reason you
have completely dropped that and related changes now again :|
I've already asked for these changes and explained what you needed to do
several times now; and to be honest this is growing quite tiresome.
You should have noticed that somehow the changes from v3 disappeared here
yourself.
Please be more thorough and check your own work before posting for v6.
Regards,
Hans
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v5 2/3] platform/x86: amd-pmc: Store the pci_dev instance inside struct amd_pmc_dev
2021-11-16 13:31 ` Hans de Goede
@ 2021-11-30 11:25 ` Goswami, Sanket
0 siblings, 0 replies; 10+ messages in thread
From: Goswami, Sanket @ 2021-11-30 11:25 UTC (permalink / raw)
To: Hans de Goede, S-k, Shyam-sundar, mgross, Limonciello, Mario
Cc: platform-driver-x86
Hi Hans,
On 16-Nov-21 19:01, Hans de Goede wrote:
> [CAUTION: External Email]
>
> Hi,
>
> On 10/29/21 19:23, 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 v5:
>> - Remove pci_dev_put() from amd_pmc_remove() as its no longer required.
>>
>> Changes in v4:
>> - No change.
>>
>> Changes in v3:
>> - Add pci_dev_put() in amd_pmc_remove().
>>
>> Changes in v2:
>> - Store the rdev info in amd_pmc_probe() as suggested by Hans.
>>
>> drivers/platform/x86/amd-pmc.c | 2 ++
>> 1 file changed, 2 insertions(+)
>>
>> diff --git a/drivers/platform/x86/amd-pmc.c b/drivers/platform/x86/amd-pmc.c
>> index 9af02860ed59..ea099f7759f2 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;
>> @@ -538,6 +539,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);
>>
>
>
> I'm afraid this is still not correct:
>
> 1. The pci_dev_put() at line 570 is still there, so after that line
> you no longer have a reference to the pci_dev and the pointer may
> end up pointing to free-ed memory
>
> 2. Once you drop the pci_dev_put() at line 570, all the error-exit
> paths from probe after that, like this one :
>
> dev->regbase = devm_ioremap(dev->dev, base_addr + AMD_PMC_BASE_ADDR_OFFSET,
> AMD_PMC_MAPPING_SIZE);
> if (!dev->regbase)
> return -ENOMEM;
>
> need to be changed to now, put the rdev (since that is now no longer
> done on line 570), so this needs to be changed to:
>
> dev->regbase = devm_ioremap(dev->dev, base_addr + AMD_PMC_BASE_ADDR_OFFSET,
> AMD_PMC_MAPPING_SIZE);
> if (!dev->regbase) {
> err = -ENOMEM;
> goto err_pci_dev_put;
> }
>
> and the same for:
>
> dev->fch_virt_addr = devm_ioremap(dev->dev, fch_phys_addr, FCH_SSC_MAPPING_SIZE);
> if (!dev->fch_virt_addr)
> return -ENOMEM;
>
> 3. Since you now keep the reference on a succesfull probe, you need to add a
>
> pci_dev_put(dev->rdev);
>
> call to amd_pmc_remove()
>
>
> Note that the changelog says you already did 3. in v3 but for some reason you
> have completely dropped that and related changes now again :|
>
> I've already asked for these changes and explained what you needed to do
> several times now; and to be honest this is growing quite tiresome.
Apologies for messing it up. I just sent out v6 for your review. Hope it would address the issues
Thanks,
Sanket
^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2021-11-30 11:26 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-10-29 17:23 [PATCH v5 0/3] Updates to amd-pmc driver Sanket Goswami
2021-10-29 17:23 ` [PATCH v5 1/3] platform/x86: amd-pmc: Simplify error handling path Sanket Goswami
2021-10-29 23:27 ` Scott Bruce
2021-10-29 17:23 ` [PATCH v5 2/3] platform/x86: amd-pmc: Store the pci_dev instance inside struct amd_pmc_dev Sanket Goswami
2021-10-29 23:27 ` Scott Bruce
2021-11-16 13:31 ` Hans de Goede
2021-11-30 11:25 ` Goswami, Sanket
2021-10-29 17:23 ` [PATCH v5 3/3] platform/x86: amd-pmc: Add support for AMD Smart Trace Buffer Sanket Goswami
2021-10-29 23:26 ` Scott Bruce
2021-11-03 18:39 ` Limonciello, Mario
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.