i2c-imc-skylake: harden the checks for the v4 respin
All of this came out of a self-review of v3 plus one robot report; none of it changes what the driver supports. The read-back check required the whole command word to come back as written, and the interference check on the driven channel compared against the pre-transfer word. Both implicitly demanded that every undocumented bit of the command register reads as zero, which held on the test system but is not something to assume across steppings. Compare only the bits this driver produces, with the post-write read-back as the baseline for the driven channel; GO clearing remains the single permitted difference. Read the result before the interference check, not after, so the check also covers the window in which the data latch is read. Re-run the TSOD-polling check on resume and keep the adapters suspended if firmware enabled polling across suspend; the probe-time refusal cannot see that, and the S3 observation in the cover letter is exactly this scenario. Stop before writing a saved command word back if the config read that fetched it had already failed. Rename the register accessors: the command register is CMD and the data latch DATA. v3 called the command register DATA and the data latch CTRL, which read backwards. Cast ~COMMAND_OUR_BITS to u32 in the interference check. BIT() and GENMASK() are unsigned long, so the complement was computed in 64 bits and truncated on assignment; the truncated value was already correct, this only silences a clang W=1 -Wconstant-conversion warning. Reported-by: kernel test robot <[email protected]> Closes: https://lore.kernel.org/oe-kbuild-all/[email protected]/ The Kconfig help and the .rst now say that PCI ID 8086:2085 also matches Skylake-SP / Cascade Lake-SP servers, where a BMC is far more likely to own the engine, and the .rst states plainly that jc42 itself has not been run here, only the transactions it needs.
This commit is contained in:
Simone
@@ -19,8 +19,11 @@ config I2C_IMC_SKYLAKE
|
||||
refuses to bind unless the allow_unsafe_access module parameter
|
||||
is enabled. See Documentation/i2c/busses/i2c-imc-skylake.rst.
|
||||
|
||||
If unsure, say N. This driver is only useful on Intel X299 desktop
|
||||
/ HEDT systems with the Skylake-X or Cascade Lake-X CPU.
|
||||
If unsure, say N. This driver is aimed at Intel X299 desktop /
|
||||
HEDT systems with a Skylake-X or Cascade Lake-X CPU. The same PCI
|
||||
ID appears on Skylake-SP / Cascade Lake-SP servers, where a BMC is
|
||||
far more likely to be using the engine already; the driver applies
|
||||
the same checks there, but saying N is even more advisable.
|
||||
|
||||
This driver can also be built as a module. If so, the module
|
||||
will be called i2c-imc-skylake.
|
||||
|
||||
@@ -8,6 +8,10 @@ Supported adapters:
|
||||
* Intel Skylake-X / Cascade Lake-X integrated memory controller (iMC)
|
||||
SMBus engine, PCU function 8086:2085 (socket LGA 2066, platform X299)
|
||||
|
||||
The same PCI ID appears on Skylake-SP / Cascade Lake-SP servers. The driver
|
||||
behaves identically there, but on server boards a BMC is far more likely to be
|
||||
using the engine already; read the next section twice before opting in.
|
||||
|
||||
Author: Simone Chifari <[email protected]>
|
||||
|
||||
|
||||
@@ -66,10 +70,14 @@ Per transfer:
|
||||
* the previous command state of both channels is saved, the TSOD-active
|
||||
state is cleared for the duration of the transfer, and both are restored
|
||||
afterwards;
|
||||
* after each transfer the command registers are compared against what the
|
||||
driver left in them. A difference means another master used the engine,
|
||||
and the transfer is failed with ``-EAGAIN`` rather than reported as
|
||||
successful.
|
||||
* the command word is read back and must be the one just written, in the
|
||||
bits the driver produces; two identical consecutive commands are the
|
||||
one case this cannot cover, because bit 29 cannot serve as a nonce -
|
||||
the engine errors out any command with it clear;
|
||||
* the result is read, and only then are the command registers compared
|
||||
against what the driver left in them. A difference means another master
|
||||
used the engine, and the transfer is failed with ``-EAGAIN`` rather than
|
||||
reported as successful.
|
||||
|
||||
The last check is what makes an unarbitrated system detectable: if the log
|
||||
shows
|
||||
@@ -79,6 +87,11 @@ shows
|
||||
then firmware is using the engine and the driver should not be loaded on that
|
||||
system.
|
||||
|
||||
The TSOD-polling check runs again on resume from suspend, because firmware
|
||||
can enable the polling across a sleep state. If it did, the adapters stay
|
||||
suspended and transfers fail with ``-ESHUTDOWN`` instead of racing the memory
|
||||
controller.
|
||||
|
||||
|
||||
Config space access
|
||||
-------------------
|
||||
|
||||
+111
-65
@@ -12,13 +12,13 @@
|
||||
*
|
||||
* Per-channel register triple within the config space of the function:
|
||||
* ch0 ch1
|
||||
* CTRL (data) 0xB4 0xB8 write: data byte in bits[23:16]; read: low byte
|
||||
* DATA (cmd) 0x9C 0xA0 command toggle | GO | address | register
|
||||
* CMD 0x9C 0xA0 command toggle | GO | address | register
|
||||
* DATA 0xB4 0xB8 write: data byte in bits[23:16]; read: low byte
|
||||
* STATUS 0xA8 0xAC bit0 BUSY, bit1 ERROR/NACK,
|
||||
* bit2 READ_DONE, bit3 WRITE_DONE
|
||||
* SMBus command byte (DATA bits[15:8]) = (rw << 7) | addr7: the 7-bit slave
|
||||
* SMBus command byte (CMD bits[15:8]) = (rw << 7) | addr7: the 7-bit slave
|
||||
* address with bit7 = direction (1 write / 0 read). The register/offset goes
|
||||
* in DATA[7:0]. So 0x50 reads SPD EEPROM 0x50. This was decoded and
|
||||
* in CMD[7:0]. So 0x50 reads SPD EEPROM 0x50. This was decoded and
|
||||
* confirmed on hardware, and cross-checked against an independent Windows
|
||||
* implementation of the same engine.
|
||||
*
|
||||
@@ -71,16 +71,20 @@
|
||||
#define SMBCNTL_TSOD_PRESENT GENMASK(7, 0)
|
||||
|
||||
/* per-channel register offsets in the function's config space */
|
||||
#define CH0_CTRL 0xB4
|
||||
#define CH0_DATA 0x9C
|
||||
#define CH0_CMD 0x9C
|
||||
#define CH0_DATA 0xB4
|
||||
#define CH0_STAT 0xA8
|
||||
#define CH1_CTRL 0xB8
|
||||
#define CH1_DATA 0xA0
|
||||
#define CH1_CMD 0xA0
|
||||
#define CH1_DATA 0xB8
|
||||
#define CH1_STAT 0xAC
|
||||
|
||||
/*
|
||||
* Command word written to the DATA register:
|
||||
* bit29 = command toggle
|
||||
* Command word written to the CMD register:
|
||||
* bit29 = must be set for the engine to execute the command.
|
||||
* Measured on hardware: with the bit clear, GO is consumed
|
||||
* but the transaction completes with the error bit set, so
|
||||
* it cannot serve as a per-command nonce (see
|
||||
* imc_command_landed())
|
||||
* bit20 = TSOD active state, preserved across transfers
|
||||
* bit19 = GO
|
||||
* bit17 = word transfer
|
||||
@@ -88,7 +92,7 @@
|
||||
* rw bit (0x80): 1 = write, 0 = read
|
||||
* addr7 : 7-bit SMBus slave address
|
||||
* bits[7:0] = register/offset within the addressed device
|
||||
* For a write the data byte is latched into CTRL[23:16] beforehand. This
|
||||
* For a write the data byte is latched into DATA[23:16] beforehand. This
|
||||
* encoding was confirmed on hardware: command 0x50 reads SPD EEPROM 0x50
|
||||
* (DDR4 signature).
|
||||
*/
|
||||
@@ -145,13 +149,13 @@ MODULE_PARM_DESC(allow_unsafe_access,
|
||||
"allow access without firmware/SMM arbitration (unsafe)");
|
||||
|
||||
struct imc_chan {
|
||||
unsigned int ctrl, data, stat;
|
||||
unsigned int cmd, data, stat;
|
||||
int idx; /* channel index 0 or 1 */
|
||||
};
|
||||
|
||||
static const struct imc_chan imc_chans[2] = {
|
||||
{ CH0_CTRL, CH0_DATA, CH0_STAT, 0 },
|
||||
{ CH1_CTRL, CH1_DATA, CH1_STAT, 1 },
|
||||
{ CH0_CMD, CH0_DATA, CH0_STAT, 0 },
|
||||
{ CH1_CMD, CH1_DATA, CH1_STAT, 1 },
|
||||
};
|
||||
|
||||
/* one driver state object, shared by both per-channel adapters */
|
||||
@@ -167,6 +171,7 @@ struct imc_smbus {
|
||||
struct imc_xfer_state {
|
||||
u32 command[ARRAY_SIZE(imc_chans)];
|
||||
bool restore[ARRAY_SIZE(imc_chans)];
|
||||
u32 readback; /* own channel's CMD just after our write */
|
||||
};
|
||||
|
||||
/*
|
||||
@@ -230,7 +235,7 @@ static int imc_wait_go_clear(struct imc_smbus *s, const struct imc_chan *c)
|
||||
|
||||
return read_poll_timeout(imc_reg, val, !(val & GO_BIT) || s->io_err,
|
||||
IMC_POLL_US, IMC_GO_TIMEOUT_US, false,
|
||||
s, c->data);
|
||||
s, c->cmd);
|
||||
}
|
||||
|
||||
static int imc_wait_done(struct imc_smbus *s, const struct imc_chan *c,
|
||||
@@ -265,8 +270,8 @@ static int imc_wait_transfer(struct imc_smbus *s, const struct imc_chan *c,
|
||||
if (!(*status & STAT_BUSY) || (*status & STAT_ANY_DONE))
|
||||
return ret;
|
||||
|
||||
command = imc_reg(s, c->data);
|
||||
imc_set(s, c->data, command ^ COMMAND_TOGGLE);
|
||||
command = imc_reg(s, c->cmd);
|
||||
imc_set(s, c->cmd, command ^ COMMAND_TOGGLE);
|
||||
|
||||
ret = imc_wait_done(s, c, status);
|
||||
if (ret)
|
||||
@@ -304,23 +309,36 @@ static int imc_check_status(struct imc_smbus *s, const struct imc_chan *c,
|
||||
* command word is already clear, and the caller would receive the previous
|
||||
* transaction's data as valid.
|
||||
*
|
||||
* Read the command word back and require it to be the one just written. The
|
||||
* engine only ever clears GO, so that bit is the single permitted difference;
|
||||
* anything else means the write did not land. Checking the word rather than
|
||||
* a status transition matters because two identical transfers in a row leave
|
||||
* STATUS bit for bit the same.
|
||||
* Read the command word back and require it to be the one just written, in
|
||||
* every bit this driver produces except GO, which the engine clears itself.
|
||||
* Checking the word rather than a status transition matters because two
|
||||
* identical transfers in a row leave STATUS bit for bit the same.
|
||||
*
|
||||
* Known residual blind spot: those two identical transfers also leave CMD
|
||||
* identical except for GO, so a command write dropped between them cannot be
|
||||
* told from the stale word. Bit 29 would be the natural per-command nonce,
|
||||
* but the engine refuses it: measured on hardware, a command with bit 29
|
||||
* clear consumes GO and completes with the error bit set. Any dropped
|
||||
* write between two commands that differ in address, register, direction or
|
||||
* size is caught.
|
||||
*
|
||||
* Bits outside COMMAND_OUR_BITS are not compared; whether the hardware
|
||||
* preserves or clears them is its own business.
|
||||
*/
|
||||
static bool imc_command_landed(u32 command, u32 readback)
|
||||
{
|
||||
return !((command ^ readback) & ~GO_BIT);
|
||||
return !((command ^ readback) & COMMAND_OUR_BITS & ~GO_BIT);
|
||||
}
|
||||
|
||||
/*
|
||||
* Detect a second master. Compare the command registers against what this
|
||||
* driver left in them: for the channel it drove, only the bits it does not own;
|
||||
* for the other channel, all of them. A difference means firmware, a BMC or
|
||||
* CLTT touched the engine while the transfer was in flight, so the result
|
||||
* cannot be trusted.
|
||||
* driver left in them: for the channel it drove, only the bits it does not
|
||||
* own, with the read-back taken right after the command write as the
|
||||
* baseline, so a hardware bit that is simply preserved across our write does
|
||||
* not read as interference; for the other channel, all bits, against the
|
||||
* saved pre-transfer word. A difference means firmware, a BMC or CLTT
|
||||
* touched the engine while the transfer was in flight, so the result cannot
|
||||
* be trusted.
|
||||
*/
|
||||
static int imc_check_interference(struct imc_smbus *s, const struct imc_chan *c,
|
||||
const struct imc_xfer_state *state)
|
||||
@@ -328,9 +346,10 @@ static int imc_check_interference(struct imc_smbus *s, const struct imc_chan *c,
|
||||
int i;
|
||||
|
||||
for (i = 0; i < ARRAY_SIZE(imc_chans); i++) {
|
||||
u32 mask = i == c->idx ? ~COMMAND_OUR_BITS : ~0U;
|
||||
u32 expect = state->command[i] & COMMAND_KEEP_MASK;
|
||||
u32 now = imc_reg(s, imc_chans[i].data);
|
||||
u32 mask = i == c->idx ? (u32)~COMMAND_OUR_BITS : ~0U;
|
||||
u32 expect = i == c->idx ? state->readback :
|
||||
state->command[i] & COMMAND_KEEP_MASK;
|
||||
u32 now = imc_reg(s, imc_chans[i].cmd);
|
||||
|
||||
if (!((now ^ expect) & mask))
|
||||
continue;
|
||||
@@ -357,7 +376,7 @@ static void imc_restore_xfer(struct imc_smbus *s,
|
||||
* address, register and TSOD state back, never to re-arm a
|
||||
* transaction that was already in flight when we found it.
|
||||
*/
|
||||
imc_set(s, imc_chans[i].data, state->command[i] & ~GO_BIT);
|
||||
imc_set(s, imc_chans[i].cmd, state->command[i] & ~GO_BIT);
|
||||
}
|
||||
}
|
||||
|
||||
@@ -371,13 +390,22 @@ static int imc_prepare_xfer(struct imc_smbus *s, const struct imc_chan *c,
|
||||
return ret;
|
||||
|
||||
for (i = 0; i < ARRAY_SIZE(imc_chans); i++) {
|
||||
state->command[i] = imc_reg(s, imc_chans[i].data);
|
||||
state->command[i] = imc_reg(s, imc_chans[i].cmd);
|
||||
/*
|
||||
* A failed read makes the saved word garbage; stop before
|
||||
* writing it anywhere. Channels already modified by earlier
|
||||
* iterations are restored.
|
||||
*/
|
||||
if (s->io_err) {
|
||||
imc_restore_xfer(s, state);
|
||||
return s->io_err;
|
||||
}
|
||||
state->restore[i] = i == c->idx ||
|
||||
(state->command[i] & TSOD_ACTIVE_BIT);
|
||||
if (!(state->command[i] & TSOD_ACTIVE_BIT))
|
||||
continue;
|
||||
|
||||
imc_set(s, imc_chans[i].data,
|
||||
imc_set(s, imc_chans[i].cmd,
|
||||
state->command[i] & COMMAND_KEEP_MASK);
|
||||
ret = imc_wait_go_clear(s, &imc_chans[i]);
|
||||
if (ret) {
|
||||
@@ -408,12 +436,12 @@ static int imc_access(struct imc_smbus *s, const struct imc_chan *c, u8 addr,
|
||||
|
||||
if (read_write == I2C_SMBUS_WRITE) {
|
||||
raw = size == I2C_SMBUS_WORD_DATA ? swab16(*value) : *value;
|
||||
imc_set(s, c->ctrl, raw << 16);
|
||||
imc_set(s, c->data, raw << 16);
|
||||
command |= WRITE_OPERATION;
|
||||
}
|
||||
|
||||
imc_set(s, c->data, command);
|
||||
readback = imc_reg(s, c->data);
|
||||
imc_set(s, c->cmd, command);
|
||||
readback = imc_reg(s, c->cmd);
|
||||
if (!imc_command_landed(command, readback)) {
|
||||
dev_warn_ratelimited(s->dev,
|
||||
"ch%d command 0x%08x did not reach the register (read back 0x%08x)\n",
|
||||
@@ -421,6 +449,7 @@ static int imc_access(struct imc_smbus *s, const struct imc_chan *c, u8 addr,
|
||||
ret = -EIO;
|
||||
goto restore_command;
|
||||
}
|
||||
state.readback = readback;
|
||||
|
||||
ret = imc_wait_transfer(s, c, &status);
|
||||
if (ret)
|
||||
@@ -432,16 +461,19 @@ static int imc_access(struct imc_smbus *s, const struct imc_chan *c, u8 addr,
|
||||
if (ret)
|
||||
goto restore_command;
|
||||
|
||||
ret = imc_check_interference(s, c, &state);
|
||||
if (ret)
|
||||
goto restore_command;
|
||||
|
||||
if (read_write == I2C_SMBUS_READ) {
|
||||
raw = imc_reg(s, c->ctrl);
|
||||
raw = imc_reg(s, c->data);
|
||||
*value = size == I2C_SMBUS_WORD_DATA ?
|
||||
swab16(raw & 0xffff) : raw & 0xff;
|
||||
}
|
||||
|
||||
/*
|
||||
* Last, so that it covers the data read above as well: a foreign
|
||||
* transaction squeezed between the completion and the data read
|
||||
* would have replaced the command word and is caught here.
|
||||
*/
|
||||
ret = imc_check_interference(s, c, &state);
|
||||
|
||||
restore_command:
|
||||
imc_restore_xfer(s, &state);
|
||||
|
||||
@@ -553,30 +585,6 @@ static const struct i2c_algorithm imc_algo = {
|
||||
.functionality = imc_func,
|
||||
};
|
||||
|
||||
static int imc_suspend(struct device *dev)
|
||||
{
|
||||
struct imc_smbus *s = dev_get_drvdata(dev);
|
||||
int i;
|
||||
|
||||
for (i = 0; i < ARRAY_SIZE(s->adap); i++)
|
||||
i2c_mark_adapter_suspended(&s->adap[i]);
|
||||
|
||||
return 0;
|
||||
}
|
||||
|
||||
static int imc_resume(struct device *dev)
|
||||
{
|
||||
struct imc_smbus *s = dev_get_drvdata(dev);
|
||||
int i;
|
||||
|
||||
for (i = 0; i < ARRAY_SIZE(s->adap); i++)
|
||||
i2c_mark_adapter_resumed(&s->adap[i]);
|
||||
|
||||
return 0;
|
||||
}
|
||||
|
||||
static DEFINE_SIMPLE_DEV_PM_OPS(imc_pm_ops, imc_suspend, imc_resume);
|
||||
|
||||
/*
|
||||
* Refuse to share the engine with the memory controller's own thermal
|
||||
* throttling. SMBCNTL.TSOD_POLL_EN tells us directly whether the iMC is
|
||||
@@ -626,6 +634,44 @@ static int imc_check_firmware_polling(struct pci_dev *pdev)
|
||||
return 0;
|
||||
}
|
||||
|
||||
static int imc_suspend(struct device *dev)
|
||||
{
|
||||
struct imc_smbus *s = dev_get_drvdata(dev);
|
||||
int i;
|
||||
|
||||
for (i = 0; i < ARRAY_SIZE(s->adap); i++)
|
||||
i2c_mark_adapter_suspended(&s->adap[i]);
|
||||
|
||||
return 0;
|
||||
}
|
||||
|
||||
static int imc_resume(struct device *dev)
|
||||
{
|
||||
struct imc_smbus *s = dev_get_drvdata(dev);
|
||||
int i;
|
||||
|
||||
/*
|
||||
* Firmware demonstrably drives the engine across suspend (an SPD
|
||||
* read during memory training was observed on resume from S3), and
|
||||
* it may also leave TSOD polling enabled, a state the probe-time
|
||||
* check can no longer refuse. Re-check, and keep the adapters
|
||||
* suspended when the engine is no longer ours: transfers then fail
|
||||
* with -ESHUTDOWN instead of racing the memory controller.
|
||||
*/
|
||||
if (imc_check_firmware_polling(s->pdev)) {
|
||||
dev_err(s->dev,
|
||||
"TSOD polling was enabled while suspended; adapters stay disabled\n");
|
||||
return 0;
|
||||
}
|
||||
|
||||
for (i = 0; i < ARRAY_SIZE(s->adap); i++)
|
||||
i2c_mark_adapter_resumed(&s->adap[i]);
|
||||
|
||||
return 0;
|
||||
}
|
||||
|
||||
static DEFINE_SIMPLE_DEV_PM_OPS(imc_pm_ops, imc_suspend, imc_resume);
|
||||
|
||||
static int imc_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id)
|
||||
{
|
||||
struct imc_smbus *s;
|
||||
|
||||
Reference in New Issue
Block a user