Messages in this thread | | | Date | Sat, 07 Mar 2015 06:38:53 -0800 | From | Guenter Roeck <> | Subject | Re: [PATCH 1/2] i2c_imc: New driver for Intel's iMC, found on LGA2011 chips |
| |
On 03/06/2015 06:50 PM, Andy Lutomirski wrote: > Sandy Bridge Xeon and Extreme chips have integrated memory > controllers with (rather limited) onboard SMBUS masters. This > driver gives access to the bus. > > There are various groups working on standardizing a way to arbitrate > access to the bus between the OS, SMM firmware, a BMC, hardware > thermal control, etc. In the mean time, running this driver is > unsafe except under special circumstances. Nonetheless, this driver > has real users. > > As a compromise, the driver will refuse to load unless > i2c_imc.allow_unsafe_access=Y. When safe access becomes available, > we can leave this option as a way for legacy users to run the > driver, and we'll allow the driver to load by default if safe bus > access is available. > > Signed-off-by: Andy Lutomirski <luto@amacapital.net> > ---
Please consider running your patch through checkpatch --strict, or at least checkpatch. [ I won't comment on the checkpatch problems below ]
> drivers/i2c/busses/Kconfig | 18 ++ > drivers/i2c/busses/Makefile | 1 + > drivers/i2c/busses/i2c-imc.c | 583 +++++++++++++++++++++++++++++++++++++++++++ > 3 files changed, 602 insertions(+) > create mode 100644 drivers/i2c/busses/i2c-imc.c > > diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig > index ab838d9e28b6..d6b9ce164fbf 100644 > --- a/drivers/i2c/busses/Kconfig > +++ b/drivers/i2c/busses/Kconfig > @@ -149,6 +149,24 @@ config I2C_ISMT > This driver can also be built as a module. If so, the module will be > called i2c-ismt. > > +config I2C_IMC > + tristate "Intel iMC (LGA 2011) SMBus Controller" > + depends on PCI && X86 > + select I2C_DIMM_BUS > + help > + If you say yes to this option, support will be included for the Intel > + Integrated Memory Controller SMBus host controller interface. This > + controller is found on LGA 2011 Xeons and Core i7 Extremes. > + > + There are currently no systems on which the kernel knows that it can > + safely enable this driver. For now, you need to pass this driver a > + scary module parameter, and you should only pass that parameter if you > + have a special motherboard and know exactly what you are doing. > + Special motherboards include the Supermicro X9DRH-iF-NV. > + > + This driver can also be built as a module. If so, the module will be > + called i2c-imc. > + > config I2C_PIIX4 > tristate "Intel PIIX4 and compatible (ATI/AMD/Serverworks/Broadcom/SMSC)" > depends on PCI > diff --git a/drivers/i2c/busses/Makefile b/drivers/i2c/busses/Makefile > index 56388f658d2f..4287c891e782 100644 > --- a/drivers/i2c/busses/Makefile > +++ b/drivers/i2c/busses/Makefile > @@ -15,6 +15,7 @@ obj-$(CONFIG_I2C_AMD8111) += i2c-amd8111.o > obj-$(CONFIG_I2C_I801) += i2c-i801.o > obj-$(CONFIG_I2C_ISCH) += i2c-isch.o > obj-$(CONFIG_I2C_ISMT) += i2c-ismt.o > +obj-$(CONFIG_I2C_IMC) += i2c-imc.o > obj-$(CONFIG_I2C_NFORCE2) += i2c-nforce2.o > obj-$(CONFIG_I2C_NFORCE2_S4985) += i2c-nforce2-s4985.o > obj-$(CONFIG_I2C_PIIX4) += i2c-piix4.o > diff --git a/drivers/i2c/busses/i2c-imc.c b/drivers/i2c/busses/i2c-imc.c > new file mode 100644 > index 000000000000..2dbf171114c6 > --- /dev/null > +++ b/drivers/i2c/busses/i2c-imc.c > @@ -0,0 +1,583 @@ > +/* > + * Copyright (c) 2013 Andrew Lutomirski <luto@amacapital.net> > + * > + * This program is free software; you can redistribute it and/or modify > + * it under the terms of the GNU General Public License version 2 > + * as published by the Free Software Foundation. > + * > + * This program is distributed in the hope that it will be useful, > + * but WITHOUT ANY WARRANTY; without even the implied warranty of > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > + * GNU General Public License for more details. > + * > + * You should have received a copy of the GNU General Public License > + * along with this program; if not, write to the Free Software > + * Foundation, Inc., 675 Mass Ave, Cambridge, MA 02139, USA. > + */ > + > +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt > + > +#include <linux/module.h> > +#include <linux/kernel.h> > +#include <linux/delay.h> > +#include <linux/pci.h> > +#include <linux/i2c.h> > + > +/* > + * The datasheet can be found here, for example: > + * http://www.intel.com/content/dam/www/public/us/en/documents/datasheets/xeon-e5-1600-2600-vol-2-datasheet.pdf > + * > + * There seem to be quite a few bugs or spec errors, though: > + * > + * - A successful transaction sets WOD and RDO. > + * > + * - The docs for TSOD_POLL_EN make no sense (see imc_channel_claim). > + * > + * - Erratum BT109, which says: > + * > + * The processor may not complete SMBus (System Management Bus) > + * transactions targeting the TSOD (Temperature Sensor On DIMM) > + * when Package C-States are enabled. Due to this erratum, if the > + * processor transitions into a Package C-State while an SMBus > + * transaction with the TSOD is in process, the processor will > + * suspend receipt of the transaction. The transaction completes > + * while the processor is in a Package C-State. Upon exiting > + * Package C-State, the processor will attempt to resume the > + * SMBus transaction, detect a protocol violation, and log an > + * error. > + * > + * The description notwithstanding, I've seen difficult-to-reproduce > + * issues when the system goes completely idle (so package C-states can > + * be entered) while software-initiated SMBUS transactions are in > + * progress. > + */ > + > +/* Register offsets (in PCI configuration space) */ > +#define SMBSTAT(i) (0x180 + 0x10*i) > +#define SMBCMD(i) (0x184 + 0x10*i) > +#define SMBCNTL(i) (0x188 + 0x10*i) > +#define SMB_TSOD_POLL_RATE_CNTR(i) (0x18C + 0x10*i)
You might want to use (i) to avoid undesirable side effects if i is an expression.
> +#define SMB_TSOD_POLL_RATE (0x1A8) > + > +/* SMBSTAT fields */ > +#define SMBSTAT_RDO (1U << 31) /* Read Data Valid */ > +#define SMBSTAT_WOD (1U << 30) /* Write Operation Done */ > +#define SMBSTAT_SBE (1U << 29) /* SMBus Error */ > +#define SMBSTAT_SMB_BUSY (1U << 28) /* SMBus Busy State */ > +/* 26:24 is the last automatically polled TSOD address */ > +#define SMBSTAT_RDATA_MASK 0xffff /* result of a read */ > + > +/* SMBCMD fields */ > +#define SMBCMD_TRIGGER (1U << 31) /* CMD Trigger */ > +#define SMBCMD_PNTR_SEL (1U << 30) /* HW polls TSOD with pointer */ > +#define SMBCMD_WORD_ACCESS (1U << 29) /* word (vs byte) access */ > +#define SMBCMD_TYPE_MASK (3U << 27) /* Mask for access type */ > +#define SMBCMD_TYPE_READ (0U << 27) /* Read */ > +#define SMBCMD_TYPE_WRITE (1U << 27) /* Write */ > +#define SMBCMD_TYPE_PNTR_WRITE (3U << 27) /* Write to pointer */ > +#define SMBCMD_SA_MASK (7U << 24) /* Slave Address high bits */ > +#define SMBCMD_SA_SHIFT 24 > +#define SMBCMD_BA_MASK 0xff0000 /* Bus Txn address */ > +#define SMBCMD_BA_SHIFT 16 > +#define SMBCMD_WDATA_MASK 0xffff /* data to write */ > + > +/* SMBCNTL fields */ > +#define SMBCNTL_DTI_MASK 0xf0000000 /* Slave Address low bits */ > +#define SMBCNTL_DTI_SHIFT 28 /* Slave Address low bits */ > +#define SMBCNTL_CKOVRD (1U << 27) /* # Clock Override */ > +#define SMBCNTL_DIS_WRT (1U << 26) /* Disable Write (sadly) */ > +#define SMBCNTL_SOFT_RST (1U << 10) /* Soft Reset */ > +#define SMBCNTL_TSOD_POLL_EN (1U << 8) /* TSOD Polling Enable */ > +/* Bits 0-3 and 4-6 indicate TSOD presence in various slots */ > + > +/* Bits that might randomly change if we race with something. */ > +#define SMBCMD_OUR_BITS (~(u32)SMBCMD_TRIGGER) > +#define SMBCNTL_OUR_BITS (SMBCNTL_DTI_MASK | SMBCNTL_TSOD_POLL_EN) > + > +/* System Address Controller, PCI dev 13 fn 6, 8086.3cf5 */ > +#define SAD_CONTROL 0xf4 > + > +#define PCI_DEVICE_ID_INTEL_SBRIDGE_BR 0x3cf5 /* 13.6 */ > +#define PCI_DEVICE_ID_INTEL_SBRIDGE_IMC_TA 0x3ca8 /* 15.0 */ > + > +static atomic_t imc_raced; /* Set permanently to 1 if we screw up. */ > + > +static bool allow_unsafe_access; > + > +struct imc_channel { > + struct i2c_adapter adapter; > + struct mutex mutex; > + bool can_write, suspended; > + bool prev_tsod_poll; > +}; > + > +struct imc_priv { > + struct pci_dev *pci_dev; > + struct imc_channel channels[2]; > +}; > + > +static int imc_wait_not_busy(struct imc_priv *priv, int chan, u32 *stat) > +{ > + /* > + * The clock is around 100kHz, and transactions are nine cycles > + * per byte plus a few start/stop cycles, plus whatever clock > + * streching is involved. This means that polling every 70us > + * or so will give decent performance. > + * > + * Ideally we would calculate a good estimate for the > + * transaction time and sleep, but busy-waiting is an effective > + * workaround for an apparent Sandy Bridge bug that causes bogus > + * output if the system enters a package C-state. (NB: these > + * states are systemwide -- we don't need be running on the > + * right package for this to work.) > + */ > + > + int i; > + for (i = 0; i < 50; i++) { > + pci_read_config_dword(priv->pci_dev, SMBSTAT(chan), stat); > + if (!(*stat & SMBSTAT_SMB_BUSY)) > + return 0; > + udelay(70); > + }
70 * 50 = 3500uS or 3.5ms is a long time. Can you use usleep_range ? Not sure if it would buy much, just asking.
> + > + return -ETIMEDOUT; > +} > + > +static void imc_channel_release(struct imc_priv *priv, int chan) > +{ > + /* Return to HW control. */ > + if (priv->channels[chan].prev_tsod_poll) { > + u32 cntl; > + pci_read_config_dword(priv->pci_dev, SMBCNTL(chan), &cntl); > + cntl |= SMBCNTL_TSOD_POLL_EN; > + pci_write_config_dword(priv->pci_dev, SMBCNTL(chan), cntl); > + } > +} > + > +static int imc_channel_claim(struct imc_priv *priv, int chan) > +{ > + /* > + * The docs are a bit confused here. We're supposed to disable TSOD > + * polling, then wait for busy to be cleared, then set > + * SMBCNTL_TSOD_POLL_EN to zero to switch to software control. But > + * SMBCNTL_TSOD_POLL_EN is the only documented way to turn off polling. > + */ > + > + u32 cntl, stat; > + > + if (priv->channels[chan].suspended) > + return -EIO; > + > + pci_read_config_dword(priv->pci_dev, SMBCNTL(chan), &cntl); > + priv->channels[chan].prev_tsod_poll = !!(cntl & SMBCNTL_TSOD_POLL_EN); > + cntl &= ~SMBCNTL_TSOD_POLL_EN; > + pci_write_config_dword(priv->pci_dev, SMBCNTL(chan), cntl); > + > + /* Sometimes the hardware won't let go. */ > + pci_read_config_dword(priv->pci_dev, SMBCNTL(chan), &cntl); > + if (cntl & SMBCNTL_TSOD_POLL_EN) > + return -EBUSY; > + > + if (imc_wait_not_busy(priv, chan, &stat) != 0) { > + imc_channel_release(priv, chan); > + return -EBUSY; > + } > + > + return 0; /* The channel is ours. */ > +} > + > +static bool imc_channel_can_claim(struct imc_priv *priv, int chan) > +{ > + u32 orig_cntl, cntl; > + > + /* See if we can turn off TSOD_POLL_EN. */ > + > + pci_read_config_dword(priv->pci_dev, SMBCNTL(chan), &orig_cntl); > + pci_write_config_dword(priv->pci_dev, SMBCNTL(chan), > + orig_cntl & ~SMBCNTL_TSOD_POLL_EN); > + > + pci_read_config_dword(priv->pci_dev, SMBCNTL(chan), &cntl); > + if (cntl & SMBCNTL_TSOD_POLL_EN) > + return false; /* Failed. */ > + > + pci_write_config_dword(priv->pci_dev, SMBCNTL(chan), orig_cntl); > + return true; > +} > + > +/* > + * The iMC supports five access types. The terminology is rather > + * inconsistent. These are the types: > + * > + * "Write to pointer register SMBus": I2C_SMBUS_WRITE, I2C_SMBUS_BYTE > + * > + * Read byte/word: I2C_SMBUS_READ, I2C_SMBUS_{BYTE|WORD}_DATA > + * > + * Write byte/word: I2C_SMBUS_WRITE, I2C_SMBUS_{BYTE|WORD}_DATA > + * > + * The pointer write operations is AFAICT completely useless for > + * software control, for two reasons. First, HW periodically polls any > + * TSODs on the bus, so it will corrupt the pointer in between SW > + * transactions. More importantly, the matching "read byte"/"receive > + * byte" (the address-less single-byte read) is not available for SW > + * control. Therefore, this driver doesn't implement pointer writes > + * > + * There is no PEC support. > + */ > + > +static u32 imc_func(struct i2c_adapter *adapter) > +{ > + int chan; > + struct imc_channel *ch; > + struct imc_priv *priv = i2c_get_adapdata(adapter); > + > + chan = (adapter == &priv->channels[0].adapter ? 0 : 1); > + ch = &priv->channels[chan]; > + > + if (ch->can_write) > + return I2C_FUNC_SMBUS_BYTE_DATA | I2C_FUNC_SMBUS_WORD_DATA; > + else > + return I2C_FUNC_SMBUS_READ_BYTE_DATA | > + I2C_FUNC_SMBUS_READ_WORD_DATA; > +} > + > +static s32 imc_smbus_xfer(struct i2c_adapter *adap, u16 addr, > + unsigned short flags, char read_write, u8 command, > + int size, union i2c_smbus_data *data) > +{ > + int ret, chan; > + u32 cmd = 0, cntl, final_cmd, final_cntl, stat; > + struct imc_channel *ch; > + struct imc_priv *priv = i2c_get_adapdata(adap); > + > + if (atomic_read(&imc_raced)) > + return -EIO; /* Minimize damage. */ > + > + chan = (adap == &priv->channels[0].adapter ? 0 : 1); > + ch = &priv->channels[chan]; > + > + if (addr > 0x7f) > + return -EOPNOTSUPP; /* No large address support */ > + if (flags) > + return -EOPNOTSUPP; /* No PEC */ > + if (size != I2C_SMBUS_BYTE_DATA && size != I2C_SMBUS_WORD_DATA) > + return -EOPNOTSUPP; > +
It is quite uncommon to have those checks in the code. Usually one trusts the infrastructure to not request unsupported functionality. Is this really necessary ?
> + /* Encode CMD part of addresses and access size */ > + cmd |= ((u32)addr & 0x7) << SMBCMD_SA_SHIFT;
Double space
> + cmd |= ((u32)command) << SMBCMD_BA_SHIFT; > + if (size == I2C_SMBUS_WORD_DATA) > + cmd |= SMBCMD_WORD_ACCESS; > + > + /* Encode read/write and data to write */ > + if (read_write == I2C_SMBUS_READ) { > + cmd |= SMBCMD_TYPE_READ; > + } else { > + cmd |= SMBCMD_TYPE_WRITE; > + cmd |= (size == I2C_SMBUS_WORD_DATA > + ? swab16(data->word) > + : data->byte); > + } > + > + mutex_lock(&ch->mutex); > + > + ret = imc_channel_claim(priv, chan); > + if (ret) > + goto out_unlock; > + > + pci_read_config_dword(priv->pci_dev, SMBCNTL(chan), &cntl); > + cntl &= ~SMBCNTL_DTI_MASK; > + cntl |= ((u32)addr >> 3) << SMBCNTL_DTI_SHIFT; > + pci_write_config_dword(priv->pci_dev, SMBCNTL(chan), cntl); > + > + /* > + * This clears SMBCMD_PNTR_SEL. We leave it cleared so that we don't > + * need to think about keeping the TSOD pointer state consistent with > + * the hardware's expectation. This probably has some miniscule > + * power cost, as TSOD polls will take 9 extra cycles. > + */ > + cmd |= SMBCMD_TRIGGER; > + pci_write_config_dword(priv->pci_dev, SMBCMD(chan), cmd); > + > + if (imc_wait_not_busy(priv, chan, &stat) != 0) { > + /* Timeout. TODO: Reset the controller? */ > + ret = -EIO;
timeout -> -ETIMEDOUT ?
> + dev_err(&priv->pci_dev->dev, "controller is wedged\n");
If this happens, it will presumably happen all the time and the message will pollute the log. Is the message really necessary ?
> + goto out_release; > + } > + > + /* > + * Be paranoid: try to detect races. This will only detect races > + * against BIOS, not against hardware. (I've never seen this happen.) > + */ > + pci_read_config_dword(priv->pci_dev, SMBCMD(chan), &final_cmd); > + pci_read_config_dword(priv->pci_dev, SMBCNTL(chan), &final_cntl); > + if (((cmd ^ final_cmd) & SMBCMD_OUR_BITS) || > + ((cntl ^ final_cntl) & SMBCNTL_OUR_BITS)) { > + WARN(1, "iMC SMBUS raced against firmware"); > + dev_emerg(&priv->pci_dev->dev,
Is a stack trace and dev_emerg really warranted here ?
> + "Access to channel %d raced: cmd 0x%08X->0x%08X, cntl 0x%08X->0x%08X\n", > + chan, cmd, final_cmd, cntl, final_cntl); > + atomic_set(&imc_raced, 1); > + ret = -EIO; > + goto out_release; > + } > + > + if (stat & SMBSTAT_SBE) { > + /* > + * Clear the error to re-enable TSOD polling. The docs say > + * that, as long as SBE is set, TSOD polling won't happen. > + * The docs also say that writing zero to this bit (which is > + * the only writable bit in the whole register) will clear > + * the error. Empirically, writing 0 does not clear SBE, but > + * it's probably still good to do the write in compliance with > + * the spec. (TSOD polling still happens and seems to > + * clear SBE on its own.) > + */ > + pci_write_config_dword(priv->pci_dev, SMBSTAT(chan), 0); > + ret = -ENXIO; > + goto out_release; > + } > + > + if (read_write == I2C_SMBUS_READ) { > + if (stat & SMBSTAT_RDO) { > + /* > + * The iMC SMBUS controller thinks of SMBUS > + * words as being big-endian (MSB first). > + * Linux treats them as little-endian, so we need > + * to swap them. > + * > + * Note: the controller will often (always?) set > + * WOD here. This is probably a bug. > + */ > + if (size == I2C_SMBUS_WORD_DATA) > + data->word = swab16(stat & SMBSTAT_RDATA_MASK); > + else > + data->byte = stat & 0xFF; > + ret = 0;
ret is already guaranteed to be 0 here.
> + } else { > + dev_err(&priv->pci_dev->dev, > + "Unexpected read status 0x%08X\n", stat); > + ret = -EIO; > + } > + } else { > + if (stat & SMBSTAT_WOD) { > + /* Same bug (?) as in the read case. */ > + ret = 0;
ret is already 0, so only the else case is really needed.
> + } else { > + dev_err(&priv->pci_dev->dev, > + "Unexpected write status 0x%08X\n", stat); > + ret = -EIO; > + } > + } > + > +out_release: > + imc_channel_release(priv, chan); > + > +out_unlock: > + mutex_unlock(&ch->mutex); > + > + return ret; > +} > + > +static const struct i2c_algorithm imc_smbus_algorithm = { > + .smbus_xfer = imc_smbus_xfer, > + .functionality = imc_func, > +}; > + > +static int imc_init_channel(struct imc_priv *priv, int i, int socket) > +{ > + int err; > + u32 val; > + struct imc_channel *ch = &priv->channels[i]; > + > + /* > + * With CLTT enabled, the hardware won't let us turn > + * off TSOD polling. The device is completely useless > + * when this happens (at least without help from Intel), > + * but we can at least minimize confusion. > + */ > + if (!imc_channel_can_claim(priv, i)) { > + dev_warn(&priv->pci_dev->dev, > + "iMC channel %d: we cannot control the HW. Is CLTT on?\n", > + i); > + return -EBUSY; > + } > + > + i2c_set_adapdata(&ch->adapter, priv); > + ch->adapter.owner = THIS_MODULE; > + ch->adapter.algo = &imc_smbus_algorithm; > + ch->adapter.dev.parent = &priv->pci_dev->dev; > + > + pci_read_config_dword(priv->pci_dev, SMBCNTL(i), &val); > + ch->can_write = !(val & SMBCNTL_DIS_WRT); > + > + /* > + * i2c_add_addapter can call imc_smbus_xfer, so we need to be > + * ready immediately. > + */
Seems to be a comment with no value. Every adapter needs to be ready by the time it registers.
> + mutex_init(&ch->mutex); > + > + snprintf(ch->adapter.name, sizeof(ch->adapter.name), > + "iMC socket %d channel %d", socket, i); > + err = i2c_add_adapter(&ch->adapter); > + if (err) { > + mutex_destroy(&ch->mutex); > + return err; > + } > + > + return 0; > +} > + > +static void imc_free_channel(struct imc_priv *priv, int i) > +{ > + struct imc_channel *ch = &priv->channels[i]; > + > + /* This can recurse into imc_smbus_xfer. */
So ?
> + i2c_del_adapter(&ch->adapter); > + > + mutex_destroy(&ch->mutex); > +} > + > +static struct pci_dev *imc_get_related_device(struct pci_bus *bus, unsigned int devfn, u16 devid) > +{ > + struct pci_dev *ret = pci_get_slot(bus, devfn);
ret is a bit unusual as variable name for a pointer. dev, maybe ?
> + if (!ret) > + return NULL; > + if (ret->vendor != PCI_VENDOR_ID_INTEL || ret->device != devid) { > + pci_dev_put(ret); > + return NULL; > + } > + return ret; > +} > + > +static int imc_probe(struct pci_dev *dev, const struct pci_device_id *id) > +{ > + int i, err; > + struct imc_priv *priv; > + struct pci_dev *sad; /* System Address Decoder */ > + u32 sad_control; > + > + /* Paranoia: the datasheet says this is always at 15.0 */ > + if (dev->devfn != PCI_DEVFN(15, 0)) > + return -ENODEV; > + > + /* > + * The socket number is hidden away on a different PCI device. > + * There's another copy at devfn 11.0 offset 0x40, and an even > + * less convincing copy at 5.0 0x140. The actual APICID register > + * is "not used ... and is still implemented in hardware because > + * of FUD". > + * > + * In principle we could double-check that the socket matches > + * the numa_node from SRAT, but this is probably not worth it. > + */ > + sad = imc_get_related_device(dev->bus, PCI_DEVFN(13, 6), > + PCI_DEVICE_ID_INTEL_SBRIDGE_BR); > + if (!sad) > + return -ENODEV; > + pci_read_config_dword(sad, SAD_CONTROL, &sad_control); > + pci_dev_put(sad); > + > + priv = kzalloc(sizeof(*priv), GFP_KERNEL);
devm_kzalloc() ?
> + if (!priv) > + return -ENOMEM; > + priv->pci_dev = dev; > + > + pci_set_drvdata(dev, priv); > + > + for (i = 0; i < 2; i++) { > + int j; > + err = imc_init_channel(priv, i, sad_control & 0x7); > + if (err) { > + for (j = 0; j < i; j++)
if (i) imc_free_channel(priv, 0);
would be a bit simpler and accomplish the same.
> + imc_free_channel(priv, j); > + goto exit_free; > + } > + } > + > + return 0; > + > +exit_free: > + kfree(priv); > + return err; > +} > + > +static void imc_remove(struct pci_dev *dev) > +{ > + int i; > + struct imc_priv *priv = pci_get_drvdata(dev); > + > + for (i = 0; i < 2; i++) > + imc_free_channel(priv, i); > + > + kfree(priv); > +} > + > +static int imc_suspend(struct pci_dev *dev, pm_message_t mesg) > +{ > + int i; > + struct imc_priv *priv = pci_get_drvdata(dev); > + > + /* BIOS is in charge. We should finish any pending transaction */ > + for (i = 0; i < 2; i++) { > + mutex_lock(&priv->channels[i].mutex); > + priv->channels[i].suspended = true; > + mutex_unlock(&priv->channels[i].mutex); > + } > + > + return 0; > +} > + > +static int imc_resume(struct pci_dev *dev) > +{ > + int i; > + struct imc_priv *priv = pci_get_drvdata(dev); > + > + for (i = 0; i < 2; i++) { > + mutex_lock(&priv->channels[i].mutex); > + priv->channels[i].suspended = false; > + mutex_unlock(&priv->channels[i].mutex); > + } > + > + return 0; > +} > + > +static DEFINE_PCI_DEVICE_TABLE(imc_ids) = { > + { PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_SBRIDGE_IMC_TA) }, > + { 0, } > +}; > +MODULE_DEVICE_TABLE(pci, imc_ids); > + > +static struct pci_driver imc_pci_driver = { > + .name = "imc_smbus", > + .id_table = imc_ids, > + .probe = imc_probe, > + .remove = imc_remove, > + .suspend = imc_suspend, > + .resume = imc_resume, > +}; > + > +static int __init i2c_imc_init(void) > +{ > + if (!allow_unsafe_access) { > + pr_info("disabled because we cannot safely arbitrate with firmware and hardware\n"); > + return -EBUSY;
Why -EBUSY and not -ENODEV ? Also not sure if the message is really warranted.
> + } > + > + pr_warn("using this driver is dangerous unless your firmware is specifically designed for it; use at your own risk\n");
Seems to me this is a bit noisy. User should already know.
> + return pci_register_driver(&imc_pci_driver); > +} > +module_init(i2c_imc_init); > + > +static void __exit i2c_imc_exit(void) > +{ > + pci_unregister_driver(&imc_pci_driver); > +} > +module_exit(i2c_imc_exit); > + > +module_param(allow_unsafe_access, bool, 0400); > +MODULE_PARM_DESC(allow_unsafe_access, "enable i2c_imc despite potential races against BIOS/hardware bus access"); > + > +MODULE_AUTHOR("Andrew Lutomirski <luto@amacapital.net>"); > +MODULE_DESCRIPTION("iMC SMBus driver"); > +MODULE_LICENSE("GPL"); >
| |