LinuxLists.cc - [PATCH v3 0/8] Enhance AMD SMN Error Checking

[permalink] [raw]

Subject: [PATCH v3 2/8] EDAC/amd64: Check return value of amd_smn_read()

Check the return value of amd_smn_read() before saving a value. This
ensures invalid values aren't saved. The struct umc instance is
initialized to 0 during memory allocation. Therefore, a bad read will
keep the value as 0 providing the expected Read-as-Zero behavior.

Furthermore, the __must_check attribute will be added to amd_smn_read().
Therefore, this change is required to avoid compile-time warnings.

Signed-off-by: Yazen Ghannam <[email protected]>
Reviewed-by: Mario Limonciello <[email protected]>
Cc: [email protected]
---
drivers/edac/amd64_edac.c | 51 ++++++++++++++++++++++++++++++++++-------------
1 file changed, 37 insertions(+), 14 deletions(-)

diff --git a/drivers/edac/amd64_edac.c b/drivers/edac/amd64_edac.c
index 4cedfb3b4cb6..e958ade6ff24 100644
--- a/drivers/edac/amd64_edac.c
+++ b/drivers/edac/amd64_edac.c
@@ -1436,6 +1436,7 @@ static void umc_read_base_mask(struct amd64_pvt *pvt)
u32 *base, *base_sec;
u32 *mask, *mask_sec;
int cs, umc;
+ u32 tmp;

for_each_umc(umc) {
umc_base_reg = get_umc_base(umc) + UMCCH_BASE_ADDR;
@@ -1448,13 +1449,17 @@ static void umc_read_base_mask(struct amd64_pvt *pvt)
base_reg = umc_base_reg + (cs * 4);
base_reg_sec = umc_base_reg_sec + (cs * 4);

- if (!amd_smn_read(pvt->mc_node_id, base_reg, base))
+ if (!amd_smn_read(pvt->mc_node_id, base_reg, &tmp)) {
+ *base = tmp;
edac_dbg(0, " DCSB%d[%d]=0x%08x reg: 0x%x\n",
umc, cs, *base, base_reg);
+ }

- if (!amd_smn_read(pvt->mc_node_id, base_reg_sec, base_sec))
+ if (!amd_smn_read(pvt->mc_node_id, base_reg_sec, &tmp)) {
+ *base_sec = tmp;
edac_dbg(0, " DCSB_SEC%d[%d]=0x%08x reg: 0x%x\n",
umc, cs, *base_sec, base_reg_sec);
+ }
}

umc_mask_reg = get_umc_base(umc) + UMCCH_ADDR_MASK;
@@ -1467,13 +1472,17 @@ static void umc_read_base_mask(struct amd64_pvt *pvt)
mask_reg = umc_mask_reg + (cs * 4);
mask_reg_sec = umc_mask_reg_sec + (cs * 4);

- if (!amd_smn_read(pvt->mc_node_id, mask_reg, mask))
+ if (!amd_smn_read(pvt->mc_node_id, mask_reg, &tmp)) {
+ *mask = tmp;
edac_dbg(0, " DCSM%d[%d]=0x%08x reg: 0x%x\n",
umc, cs, *mask, mask_reg);
+ }

- if (!amd_smn_read(pvt->mc_node_id, mask_reg_sec, mask_sec))
+ if (!amd_smn_read(pvt->mc_node_id, mask_reg_sec, &tmp)) {
+ *mask_sec = tmp;
edac_dbg(0, " DCSM_SEC%d[%d]=0x%08x reg: 0x%x\n",
umc, cs, *mask_sec, mask_reg_sec);
+ }
}
}
}
@@ -2892,7 +2901,7 @@ static void umc_read_mc_regs(struct amd64_pvt *pvt)
{
u8 nid = pvt->mc_node_id;
struct amd64_umc *umc;
- u32 i, umc_base;
+ u32 i, tmp, umc_base;

/* Read registers from each UMC */
for_each_umc(i) {
@@ -2900,11 +2909,20 @@ static void umc_read_mc_regs(struct amd64_pvt *pvt)
umc_base = get_umc_base(i);
umc = &pvt->umc[i];

- amd_smn_read(nid, umc_base + get_umc_reg(pvt, UMCCH_DIMM_CFG), &umc->dimm_cfg);
- amd_smn_read(nid, umc_base + UMCCH_UMC_CFG, &umc->umc_cfg);
- amd_smn_read(nid, umc_base + UMCCH_SDP_CTRL, &umc->sdp_ctrl);
- amd_smn_read(nid, umc_base + UMCCH_ECC_CTRL, &umc->ecc_ctrl);
- amd_smn_read(nid, umc_base + UMCCH_UMC_CAP_HI, &umc->umc_cap_hi);
+ if (!amd_smn_read(nid, umc_base + get_umc_reg(pvt, UMCCH_DIMM_CFG), &tmp))
+ umc->dimm_cfg = tmp;
+
+ if (!amd_smn_read(nid, umc_base + UMCCH_UMC_CFG, &tmp))
+ umc->umc_cfg = tmp;
+
+ if (!amd_smn_read(nid, umc_base + UMCCH_SDP_CTRL, &tmp))
+ umc->sdp_ctrl = tmp;
+
+ if (!amd_smn_read(nid, umc_base + UMCCH_ECC_CTRL, &tmp))
+ umc->ecc_ctrl = tmp;
+
+ if (!amd_smn_read(nid, umc_base + UMCCH_UMC_CAP_HI, &tmp))
+ umc->umc_cap_hi = tmp;
}
}

@@ -3633,16 +3651,21 @@ static void gpu_read_mc_regs(struct amd64_pvt *pvt)
{
u8 nid = pvt->mc_node_id;
struct amd64_umc *umc;
- u32 i, umc_base;
+ u32 i, tmp, umc_base;

/* Read registers from each UMC */
for_each_umc(i) {
umc_base = gpu_get_umc_base(pvt, i, 0);
umc = &pvt->umc[i];

- amd_smn_read(nid, umc_base + UMCCH_UMC_CFG, &umc->umc_cfg);
- amd_smn_read(nid, umc_base + UMCCH_SDP_CTRL, &umc->sdp_ctrl);
- amd_smn_read(nid, umc_base + UMCCH_ECC_CTRL, &umc->ecc_ctrl);
+ if (!amd_smn_read(nid, umc_base + UMCCH_UMC_CFG, &tmp))
+ umc->umc_cfg = tmp;
+
+ if (!amd_smn_read(nid, umc_base + UMCCH_SDP_CTRL, &tmp))
+ umc->sdp_ctrl = tmp;
+
+ if (!amd_smn_read(nid, umc_base + UMCCH_ECC_CTRL, &tmp))
+ umc->ecc_ctrl = tmp;
}
}

--
2.34.1

2024-05-23 18:28:10

[permalink] [raw]

Subject: [PATCH v3 7/8] hwmon: (k10temp) Remove unused HAVE_TDIE() macro

..to address the following warning:

drivers/hwmon/k10temp.c:104:9:
warning: macro is not used [-Wunused-macros]

No functional change is intended.

Signed-off-by: Yazen Ghannam <[email protected]>
---
drivers/hwmon/k10temp.c | 1 -
1 file changed, 1 deletion(-)

diff --git a/drivers/hwmon/k10temp.c b/drivers/hwmon/k10temp.c
index 6deb272c7cef..a2d203304533 100644
--- a/drivers/hwmon/k10temp.c
+++ b/drivers/hwmon/k10temp.c
@@ -101,7 +101,6 @@ struct k10temp_data {
#define TCCD_BIT(x) ((x) + 2)

#define HAVE_TEMP(d, channel) ((d)->show_temp & BIT(channel))
-#define HAVE_TDIE(d) HAVE_TEMP(d, TDIE_BIT)

struct tctl_offset {
u8 model;

--
2.34.1

2024-05-23 18:28:11

[permalink] [raw]

Subject: [PATCH v3 3/8] hwmon: (k10temp) Check return value of amd_smn_read()

Check the return value of amd_smn_read() before saving a value. This
ensures invalid values aren't saved or used.

There are three cases here with slightly different behavior.

1) read_tempreg_nb_zen():
This is a function pointer which does not include a return code.
In this case, set the register value to 0 on failure. This
enforces Read-as-Zero behavior.

2) k10temp_read_temp():
This function does have return codes, so return the error code
from the failed register read. Continued operation is not
necessary, since there is no valid data from the register.
Furthermore, if the register value was set to 0, then the
following operation would underflow.

3) k10temp_get_ccd_support():
This function reads the same register from multiple CCD
instances in a loop. And a bitmask is formed if a specific bit
is set in each register instance. The loop should continue on a
failed register read, skipping the bit check.

Furthermore, the __must_check attribute will be added to amd_smn_read().
Therefore, this change is required to avoid compile-time warnings.

Signed-off-by: Yazen Ghannam <[email protected]>
Reviewed-by: Mario Limonciello <[email protected]>
Acked-by: Guenter Roeck <[email protected]>
Cc: [email protected]
---
drivers/hwmon/k10temp.c | 36 +++++++++++++++++++++++++++---------
1 file changed, 27 insertions(+), 9 deletions(-)

diff --git a/drivers/hwmon/k10temp.c b/drivers/hwmon/k10temp.c
index 8092312c0a87..6cad35e7f182 100644
--- a/drivers/hwmon/k10temp.c
+++ b/drivers/hwmon/k10temp.c
@@ -153,8 +153,9 @@ static void read_tempreg_nb_f15(struct pci_dev *pdev, u32 *regval)

static void read_tempreg_nb_zen(struct pci_dev *pdev, u32 *regval)
{
- amd_smn_read(amd_pci_dev_to_node_id(pdev),
- ZEN_REPORTED_TEMP_CTRL_BASE, regval);
+ if (amd_smn_read(amd_pci_dev_to_node_id(pdev),
+ ZEN_REPORTED_TEMP_CTRL_BASE, regval))
+ *regval = 0;
}

static long get_raw_temp(struct k10temp_data *data)
@@ -205,6 +206,7 @@ static int k10temp_read_temp(struct device *dev, u32 attr, int channel,
long *val)
{
struct k10temp_data *data = dev_get_drvdata(dev);
+ int ret = -EOPNOTSUPP;
u32 regval;

switch (attr) {
@@ -221,13 +223,17 @@ static int k10temp_read_temp(struct device *dev, u32 attr, int channel,
*val = 0;
break;
case 2 ... 13: /* Tccd{1-12} */
- amd_smn_read(amd_pci_dev_to_node_id(data->pdev),
- ZEN_CCD_TEMP(data->ccd_offset, channel - 2),
- &regval);
+ ret = amd_smn_read(amd_pci_dev_to_node_id(data->pdev),
+ ZEN_CCD_TEMP(data->ccd_offset, channel - 2),
+ &regval);
+
+ if (ret)
+ return ret;
+
*val = (regval & ZEN_CCD_TEMP_MASK) * 125 - 49000;
break;
default:
- return -EOPNOTSUPP;
+ return ret;
}
break;
case hwmon_temp_max:
@@ -243,7 +249,7 @@ static int k10temp_read_temp(struct device *dev, u32 attr, int channel,
- ((regval >> 24) & 0xf)) * 500 + 52000;
break;
default:
- return -EOPNOTSUPP;
+ return ret;
}
return 0;
}
@@ -381,8 +387,20 @@ static void k10temp_get_ccd_support(struct pci_dev *pdev,
int i;

for (i = 0; i < limit; i++) {
- amd_smn_read(amd_pci_dev_to_node_id(pdev),
- ZEN_CCD_TEMP(data->ccd_offset, i), &regval);
+ /*
+ * Ignore inaccessible CCDs.
+ *
+ * Some systems will return a register value of 0, and the TEMP_VALID
+ * bit check below will naturally fail.
+ *
+ * Other systems will return a PCI_ERROR_RESPONSE (0xFFFFFFFF) for
+ * the register value. And this will incorrectly pass the TEMP_VALID
+ * bit check.
+ */
+ if (amd_smn_read(amd_pci_dev_to_node_id(pdev),
+ ZEN_CCD_TEMP(data->ccd_offset, i), &regval))
+ continue;
+
if (regval & ZEN_CCD_TEMP_VALID)
data->show_temp |= BIT(TCCD_BIT(i));
}

--
2.34.1

2024-05-23 18:28:11

[permalink] [raw]

Subject: [PATCH v3 6/8] hwmon: (k10temp) Reduce k10temp_get_ccd_support() parameters

Currently, k10temp_get_ccd_support() takes as input "pdev" and "data".
However, "pdev" is already included in "data". Furthermore, the "pdev"
parameter is no longer used in k10temp_get_ccd_support(), since its use
was moved into read_ccd_temp_reg().

Drop the "pdev" input parameter as it is no longer needed.

No functional change is intended.

Signed-off-by: Yazen Ghannam <[email protected]>
Reviewed-by: Mario Limonciello <[email protected]>
Acked-by: Guenter Roeck <[email protected]>
---
drivers/hwmon/k10temp.c | 17 ++++++++---------
1 file changed, 8 insertions(+), 9 deletions(-)

diff --git a/drivers/hwmon/k10temp.c b/drivers/hwmon/k10temp.c
index 315c52de6e54..6deb272c7cef 100644
--- a/drivers/hwmon/k10temp.c
+++ b/drivers/hwmon/k10temp.c
@@ -385,8 +385,7 @@ static const struct hwmon_chip_info k10temp_chip_info = {
.info = k10temp_info,
};

-static void k10temp_get_ccd_support(struct pci_dev *pdev,
- struct k10temp_data *data, int limit)
+static void k10temp_get_ccd_support(struct k10temp_data *data, int limit)
{
u32 regval;
int i;
@@ -456,18 +455,18 @@ static int k10temp_probe(struct pci_dev *pdev, const struct pci_device_id *id)
case 0x11: /* Zen APU */
case 0x18: /* Zen+ APU */
data->ccd_offset = 0x154;
- k10temp_get_ccd_support(pdev, data, 4);
+ k10temp_get_ccd_support(data, 4);
break;
case 0x31: /* Zen2 Threadripper */
case 0x60: /* Renoir */
case 0x68: /* Lucienne */
case 0x71: /* Zen2 */
data->ccd_offset = 0x154;
- k10temp_get_ccd_support(pdev, data, 8);
+ k10temp_get_ccd_support(data, 8);
break;
case 0xa0 ... 0xaf:
data->ccd_offset = 0x300;
- k10temp_get_ccd_support(pdev, data, 8);
+ k10temp_get_ccd_support(data, 8);
break;
}
} else if (boot_cpu_data.x86 == 0x19) {
@@ -481,21 +480,21 @@ static int k10temp_probe(struct pci_dev *pdev, const struct pci_device_id *id)
case 0x21: /* Zen3 Ryzen Desktop */
case 0x50 ... 0x5f: /* Green Sardine */
data->ccd_offset = 0x154;
- k10temp_get_ccd_support(pdev, data, 8);
+ k10temp_get_ccd_support(data, 8);
break;
case 0x40 ... 0x4f: /* Yellow Carp */
data->ccd_offset = 0x300;
- k10temp_get_ccd_support(pdev, data, 8);
+ k10temp_get_ccd_support(data, 8);
break;
case 0x60 ... 0x6f:
case 0x70 ... 0x7f:
data->ccd_offset = 0x308;
- k10temp_get_ccd_support(pdev, data, 8);
+ k10temp_get_ccd_support(data, 8);
break;
case 0x10 ... 0x1f:
case 0xa0 ... 0xaf:
data->ccd_offset = 0x300;
- k10temp_get_ccd_support(pdev, data, 12);
+ k10temp_get_ccd_support(data, 12);
break;
}
} else if (boot_cpu_data.x86 == 0x1a) {

--
2.34.1

2024-05-23 18:28:30

[permalink] [raw]

Subject: [PATCH v3 5/8] hwmon: (k10temp) Define helper function to read CCD temp

The CCD temperature register is read in two places. These reads are done
using an AMD SMN access, and a number of parameters are needed for the
operation.

Move the SMN access and parameter gathering into a helper function in
order to simply the code flow. This also has a benefit of centralizing
the hardware register access in a single place in case fixes or special
decoding is required.

Signed-off-by: Yazen Ghannam <[email protected]>
Reviewed-by: Mario Limonciello <[email protected]>
Acked-by: Guenter Roeck <[email protected]>
---
drivers/hwmon/k10temp.c | 14 +++++++++-----
1 file changed, 9 insertions(+), 5 deletions(-)

diff --git a/drivers/hwmon/k10temp.c b/drivers/hwmon/k10temp.c
index 6cad35e7f182..315c52de6e54 100644
--- a/drivers/hwmon/k10temp.c
+++ b/drivers/hwmon/k10temp.c
@@ -158,6 +158,13 @@ static void read_tempreg_nb_zen(struct pci_dev *pdev, u32 *regval)
*regval = 0;
}

+static int read_ccd_temp_reg(struct k10temp_data *data, int ccd, u32 *regval)
+{
+ u16 node_id = amd_pci_dev_to_node_id(data->pdev);
+
+ return amd_smn_read(node_id, ZEN_CCD_TEMP(data->ccd_offset, ccd), regval);
+}
+
static long get_raw_temp(struct k10temp_data *data)
{
u32 regval;
@@ -223,9 +230,7 @@ static int k10temp_read_temp(struct device *dev, u32 attr, int channel,
*val = 0;
break;
case 2 ... 13: /* Tccd{1-12} */
- ret = amd_smn_read(amd_pci_dev_to_node_id(data->pdev),
- ZEN_CCD_TEMP(data->ccd_offset, channel - 2),
- &regval);
+ ret = read_ccd_temp_reg(data, channel - 2, &regval);

if (ret)
return ret;
@@ -397,8 +402,7 @@ static void k10temp_get_ccd_support(struct pci_dev *pdev,
* the register value. And this will incorrectly pass the TEMP_VALID
* bit check.
*/
- if (amd_smn_read(amd_pci_dev_to_node_id(pdev),
- ZEN_CCD_TEMP(data->ccd_offset, i), &regval))
+ if (read_ccd_temp_reg(data, i, &regval))
continue;

if (regval & ZEN_CCD_TEMP_VALID)

--
2.34.1

2024-05-23 18:28:52

[permalink] [raw]

Subject: [PATCH v3 8/8] hwmon: (k10temp) Rename _data variable

..to address the following warning:

drivers/hwmon/k10temp.c:273:47:
warning: declaration shadows a variable in the global scope [-Wshadow]
static umode_t k10temp_is_visible(const void *_data,
^
include/asm-generic/sections.h:36:13:
note: previous declaration is here
extern char _data[], _sdata[], _edata[];

No functional change is intended.

Signed-off-by: Yazen Ghannam <[email protected]>
---
drivers/hwmon/k10temp.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/hwmon/k10temp.c b/drivers/hwmon/k10temp.c
index a2d203304533..543526bac042 100644
--- a/drivers/hwmon/k10temp.c
+++ b/drivers/hwmon/k10temp.c
@@ -269,11 +269,11 @@ static int k10temp_read(struct device *dev, enum hwmon_sensor_types type,
}
}

-static umode_t k10temp_is_visible(const void *_data,
+static umode_t k10temp_is_visible(const void *drvdata,
enum hwmon_sensor_types type,
u32 attr, int channel)
{
- const struct k10temp_data *data = _data;
+ const struct k10temp_data *data = drvdata;
struct pci_dev *pdev = data->pdev;
u32 reg;

--
2.34.1

2024-05-23 18:29:13

[permalink] [raw]

Subject: [PATCH v3 4/8] x86/amd_nb: Enhance SMN access error checking

AMD Zen-based systems use a System Management Network (SMN) that
provides access to implementation-specific registers.

SMN accesses are done indirectly through an index/data pair in PCI
config space. The PCI config access may fail and return an error code.
This would prevent the "read" value from being updated, and it would
give an indication to the caller that the read or write operation failed.

However for reads, the PCI config access may succeed, but the return
value may be invalid. This is in similar fashion to PCI bad reads, i.e.
return all bits set.

Most systems will return 0 for SMN addresses that are not accessible.
This is in line with AMD convention that unavailable registers are
Read-as-Zero/Writes-Ignored.

However, some systems will return a "PCI Error Response" instead. This
value, along with an error code of 0 from the PCI config access, will
confuse callers of the amd_smn_read() function.

Check for this condition and set a proper error code for SMN reads.

Furthermore, require error checking for callers of amd_smn_read() and
amd_smn_write(). This is needed because many error conditions cannot
be checked by these functions.

Also, drop the extern keyword as it's not needed. And remove a warning
that will not be trigger in many cases.

Fixes: ddfe43cdc0da ("x86/amd_nb: Add SMN and Indirect Data Fabric access for AMD Fam17h")
Signed-off-by: Yazen Ghannam <[email protected]>
Reviewed-by: Mario Limonciello <[email protected]>
Cc: [email protected]
---
arch/x86/include/asm/amd_nb.h | 4 ++--
arch/x86/kernel/amd_nb.c | 46 +++++++++++++++++++++++++++++++++++++------
2 files changed, 42 insertions(+), 8 deletions(-)

diff --git a/arch/x86/include/asm/amd_nb.h b/arch/x86/include/asm/amd_nb.h
index 5c37944c8a5e..6f3b6aef47ba 100644
--- a/arch/x86/include/asm/amd_nb.h
+++ b/arch/x86/include/asm/amd_nb.h
@@ -21,8 +21,8 @@ extern int amd_numa_init(void);
extern int amd_get_subcaches(int);
extern int amd_set_subcaches(int, unsigned long);

-extern int amd_smn_read(u16 node, u32 address, u32 *value);
-extern int amd_smn_write(u16 node, u32 address, u32 value);
+int __must_check amd_smn_read(u16 node, u32 address, u32 *value);
+int __must_check amd_smn_write(u16 node, u32 address, u32 value);

struct amd_l3_cache {
unsigned indices;
diff --git a/arch/x86/kernel/amd_nb.c b/arch/x86/kernel/amd_nb.c
index 3cf156f70859..d00c568e45b8 100644
--- a/arch/x86/kernel/amd_nb.c
+++ b/arch/x86/kernel/amd_nb.c
@@ -180,6 +180,38 @@ static struct pci_dev *next_northbridge(struct pci_dev *dev,
return dev;
}

+/*
+ * SMN accesses may fail in ways that are difficult to detect here in the called
+ * functions smn_read() and smn_write(). Therefore, callers of these functions
+ * must do their own checking based on what behavior they expect.
+ *
+ * For SMN reads, the returned SMN value may be zero if the register is
+ * Read-as-Zero . Or it may be a "PCI Error Response", e.g. all 0xFFs. The "PCI
+ * Error Response" can be checked here, and a proper error code can be returned.
+ * But the Read-as-Zero response cannot be verified here. A value of 0 may be
+ * correct in some cases, so callers must check that this correct is for the
+ * register/fields they need.
+ *
+ * For SMN writes, success can be determined through a "write and read back"
+ * procedure. However, this is not robust when done here.
+ *
+ * Possible issues:
+ * 1) Bits that are "Write-1-to-Clear". In this case, the read value should
+ * *not* match the write value.
+ * 2) Bits that are "Read-as-Zero"/"Writes-Ignored". This information cannot be
+ * known here.
+ * 3) Bits that are "Reserved / Set to 1". Ditto above.
+ *
+ * Callers of amd_smn_write() should do the "write and read back" check themselves,
+ * if needed.
+ *
+ * For #1, they can see if their target bits got cleared.
+ *
+ * For #2 and #3, they can check if their target bits got set as intended.
+ *
+ * This matches what is done for rdmsr/wrmsr. As long as there's no #GP, then
+ * the operation is considered a success, and the caller does their own checking.
+ */
static int __amd_smn_rw(u16 node, u32 address, u32 *value, bool write)
{
struct pci_dev *root;
@@ -202,9 +234,6 @@ static int __amd_smn_rw(u16 node, u32 address, u32 *value, bool write)

err = (write ? pci_write_config_dword(root, 0x64, *value)
: pci_read_config_dword(root, 0x64, value));
- if (err)
- pr_warn("Error %s SMN address 0x%x.\n",
- (write ? "writing to" : "reading from"), address);

out_unlock:
mutex_unlock(&smn_mutex);
@@ -213,13 +242,18 @@ static int __amd_smn_rw(u16 node, u32 address, u32 *value, bool write)
return err;
}

-int amd_smn_read(u16 node, u32 address, u32 *value)
+int __must_check amd_smn_read(u16 node, u32 address, u32 *value)
{
- return __amd_smn_rw(node, address, value, false);
+ int err = __amd_smn_rw(node, address, value, false);
+
+ if (PCI_POSSIBLE_ERROR(*value))
+ err = -ENODEV;
+
+ return err;
}
EXPORT_SYMBOL_GPL(amd_smn_read);

-int amd_smn_write(u16 node, u32 address, u32 value)
+int __must_check amd_smn_write(u16 node, u32 address, u32 value)
{
return __amd_smn_rw(node, address, &value, true);
}

--
2.34.1

2024-05-23 19:32:48

by Guenter Roeck

[permalink] [raw]

Subject: Re: [PATCH v3 7/8] hwmon: (k10temp) Remove unused HAVE_TDIE() macro

On 5/23/24 11:26, Yazen Ghannam wrote:
> ...to address the following warning:
>
> drivers/hwmon/k10temp.c:104:9:
> warning: macro is not used [-Wunused-macros]
>
> No functional change is intended.
>
> Signed-off-by: Yazen Ghannam <[email protected]>

Acked-by: Guenter Roeck <[email protected]>

> ---
> drivers/hwmon/k10temp.c | 1 -
> 1 file changed, 1 deletion(-)
>
> diff --git a/drivers/hwmon/k10temp.c b/drivers/hwmon/k10temp.c
> index 6deb272c7cef..a2d203304533 100644
> --- a/drivers/hwmon/k10temp.c
> +++ b/drivers/hwmon/k10temp.c
> @@ -101,7 +101,6 @@ struct k10temp_data {
> #define TCCD_BIT(x) ((x) + 2)
>
> #define HAVE_TEMP(d, channel) ((d)->show_temp & BIT(channel))
> -#define HAVE_TDIE(d) HAVE_TEMP(d, TDIE_BIT)
>
> struct tctl_offset {
> u8 model;
>

2024-05-23 19:33:49

by Guenter Roeck

[permalink] [raw]

Subject: Re: [PATCH v3 8/8] hwmon: (k10temp) Rename _data variable

On 5/23/24 11:26, Yazen Ghannam wrote:
> ...to address the following warning:
>
> drivers/hwmon/k10temp.c:273:47:
> warning: declaration shadows a variable in the global scope [-Wshadow]
> static umode_t k10temp_is_visible(const void *_data,
> ^
> include/asm-generic/sections.h:36:13:
> note: previous declaration is here
> extern char _data[], _sdata[], _edata[];
>
> No functional change is intended.
>
> Signed-off-by: Yazen Ghannam <[email protected]>

Acked-by: Guenter Roeck <[email protected]>

> ---
> drivers/hwmon/k10temp.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/hwmon/k10temp.c b/drivers/hwmon/k10temp.c
> index a2d203304533..543526bac042 100644
> --- a/drivers/hwmon/k10temp.c
> +++ b/drivers/hwmon/k10temp.c
> @@ -269,11 +269,11 @@ static int k10temp_read(struct device *dev, enum hwmon_sensor_types type,
> }
> }
>
> -static umode_t k10temp_is_visible(const void *_data,
> +static umode_t k10temp_is_visible(const void *drvdata,
> enum hwmon_sensor_types type,
> u32 attr, int channel)
> {
> - const struct k10temp_data *data = _data;
> + const struct k10temp_data *data = drvdata;
> struct pci_dev *pdev = data->pdev;
> u32 reg;
>
>

2024-05-25 13:23:08

[permalink] [raw]

Subject: Re: [PATCH v3 2/8] EDAC/amd64: Check return value of amd_smn_read()

…
> Check the return value of amd_smn_read() before saving a value.
> This ensures invalid values aren't saved. …

Does such information indicate a need for the tag “Fixes”?

Regards,
Markus

2024-05-25 13:32:43

[permalink] [raw]

Subject: Re: [PATCH v3 3/8] hwmon: (k10temp) Check return value of amd_smn_read()

> Check the return value of amd_smn_read() before saving a value.
> This ensures invalid values aren't saved or used.
…

Does such information indicate a need for the tag “Fixes” once more?

Regards,
Markus

2024-05-25 13:56:13

[permalink] [raw]

Subject: Re: [PATCH v3 4/8] x86/amd_nb: Enhance SMN access error checking

> Furthermore, require error checking for callers of amd_smn_read() and
> amd_smn_write(). This is needed because many error conditions cannot
> be checked by these functions.

Does such information qualify for the tag “Fixes”?

> … And remove a warning
> that will not be trigger in many cases.

triggered?

I suggest to perform this source code cleanup in a separate update step.
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst?h=v6.9#n81

Regards,
Markus

2024-05-25 14:43:55

[permalink] [raw]

Subject: Re: [PATCH v3 6/8] hwmon: (k10temp) Reduce k10temp_get_ccd_support() parameters

…
> Drop the "pdev" input parameter as it is no longer needed.

Can such information be relevant for the tag “Fixes”?

Regards,
Markus

2024-05-25 15:00:51

[permalink] [raw]

Subject: Re: [PATCH v3 7/8] hwmon: (k10temp) Remove unused HAVE_TDIE() macro

> ...to address the following warning:
>
> drivers/hwmon/k10temp.c:104:9:
> warning: macro is not used [-Wunused-macros]

Can such a source code cleanup matter also for the tag “Fixes”?

Regards,
Markus

2024-06-05 09:47:24

[permalink] [raw]

Subject: Re: [PATCH v3 2/8] EDAC/amd64: Check return value of amd_smn_read()

On Thu, May 23, 2024 at 01:26:53PM -0500, Yazen Ghannam wrote:
> Check the return value of amd_smn_read() before saving a value. This
> ensures invalid values aren't saved. The struct umc instance is
> initialized to 0 during memory allocation. Therefore, a bad read will
> keep the value as 0 providing the expected Read-as-Zero behavior.
>
> Furthermore, the __must_check attribute will be added to amd_smn_read().
> Therefore, this change is required to avoid compile-time warnings.
>
> Signed-off-by: Yazen Ghannam <[email protected]>
> Reviewed-by: Mario Limonciello <[email protected]>
> Cc: [email protected]

Not really stable material. Zapped and applied.

Thx.

--
Regards/Gruss,
Boris.

https://people.kernel.org/tglx/notes-about-netiquette

2024-06-05 13:15:25

[permalink] [raw]

Subject: Re: [PATCH v3 3/8] hwmon: (k10temp) Check return value of amd_smn_read()

On Thu, May 23, 2024 at 01:26:54PM -0500, Yazen Ghannam wrote:
> Cc: [email protected]

So yeah, I'll drop the CC:stable tagging in all patches unless we're
talking about a concrete issue. You need to think about the downstream,
distro folks who need to go through gazillion of patches and wonder
whether they really need to backport them.

And I don't think misusing the stable process like that is the right
way.

Thx.

--
Regards/Gruss,
Boris.

https://people.kernel.org/tglx/notes-about-netiquette

2024-06-05 14:20:00

[permalink] [raw]

Subject: Re: [PATCH v3 3/8] hwmon: (k10temp) Check return value of amd_smn_read()

On 6/5/24 8:20 AM, Borislav Petkov wrote:
> On Thu, May 23, 2024 at 01:26:54PM -0500, Yazen Ghannam wrote:
>> Cc: [email protected]
>
> So yeah, I'll drop the CC:stable tagging in all patches unless we're
> talking about a concrete issue. You need to think about the downstream,
> distro folks who need to go through gazillion of patches and wonder
> whether they really need to backport them.
>
> And I don't think misusing the stable process like that is the right
> way.
>

I agree that patches 1-3 are not stable-worthy on their own. But I think
patch 4 is, and it requires 1-3 to avoid build errors.

Is there a preferred way to highlight this while patches are in review?

Thanks,
Yazen

2024-06-05 16:13:49

[permalink] [raw]

Subject: Re: [PATCH v3 3/8] hwmon: (k10temp) Check return value of amd_smn_read()

On Wed, Jun 05, 2024 at 09:41:51AM -0400, Yazen Ghannam wrote:
> I agree that patches 1-3 are not stable-worthy on their own. But I think
> patch 4 is, and it requires 1-3 to avoid build errors.

Which of the rules in the first section of
Documentation/process/stable-kernel-rules.rst apply for patch 4?

Because I don't see it.

--
Regards/Gruss,
Boris.

https://people.kernel.org/tglx/notes-about-netiquette

2024-06-05 16:30:50

[permalink] [raw]

Subject: Re: [PATCH v3 3/8] hwmon: (k10temp) Check return value of amd_smn_read()

On 6/5/24 12:12 PM, Borislav Petkov wrote:
> On Wed, Jun 05, 2024 at 09:41:51AM -0400, Yazen Ghannam wrote:
>> I agree that patches 1-3 are not stable-worthy on their own. But I think
>> patch 4 is, and it requires 1-3 to avoid build errors.
>
> Which of the rules in the first section of
> Documentation/process/stable-kernel-rules.rst apply for patch 4?
>
> Because I don't see it.
>

"It fixes a problem like ... a hardware quirk ..."

This is described in patch 4:
---
Most systems will return 0 for SMN addresses that are not accessible.
This is in line with AMD convention that unavailable registers are
Read-as-Zero/Writes-Ignored.

However, some systems will return a "PCI Error Response" instead. This
value, along with an error code of 0 from the PCI config access, will
confuse callers of the amd_smn_read() function.
---

But I think it's fine to drop the stable tag after reading through the
rules again. I'll do option 2 or 3 if there's interest for specific
branches. And the cherry-pick thing should be easy to do if all the
prerequisites are already upstream.

Thanks,
Yazen

2024-06-05 19:03:16

[permalink] [raw]

Subject: Re: [PATCH v3 3/8] hwmon: (k10temp) Check return value of amd_smn_read()

On 6/5/24 12:45 PM, Borislav Petkov wrote:
> On Wed, Jun 05, 2024 at 12:30:35PM -0400, Yazen Ghannam wrote:
>> "It fixes a problem like ... a hardware quirk ..."
>
> I'm pretty sure that means a patch which sets a magic bit in some MSR or
> does something else to make the hardware work again. Errata fix and some
> other hackery we get to do from time to time. Or my favourite - fix
> a BIOS f*ckup.
>

Yeah, makes sense. I agree.

>> Most systems will return 0 for SMN addresses that are not accessible.
>> This is in line with AMD convention that unavailable registers are
>> Read-as-Zero/Writes-Ignored.
>>
>> However, some systems will return a "PCI Error Response" instead. This
>> value, along with an error code of 0 from the PCI config access, will
>> confuse callers of the amd_smn_read() function.
>
> Yes, but it hasn't so far. It is all pretty-much, a hypothetical, "what
> if" thing.
>
> Sure, if that error would cause a serious issue on some system, by any
> means. But just because it might potentially happen... Meh.
>
>> But I think it's fine to drop the stable tag after reading through the
>> rules again. I'll do option 2 or 3 if there's interest for specific
>> branches. And the cherry-pick thing should be easy to do if all the
>> prerequisites are already upstream.
>
> Just wait until some real issue happens. Otherwise, you'll be pretty
> much wasting time and energy.
>
> And, btw, people should upgrade their kernels on a regular basis - not
> run old, Frankenstein backported crap and think they've got the best of
> both worlds.
>

Okay, no problem.

Thanks,
Yazen

2024-06-06 09:18:09