| Message ID | 1446896126-13369-5-git-send-email-fetzer.ch@gmail.com |
|---|---|
| State | Superseded |
| Headers | show |
On Sat, 2015-11-07 at 12:35 +0100, Christian Fetzer wrote: > The SB800 chipset supports a multiplexed main SMBus controller with > four ports. The multiplexed ports share the same SMBus address and > register set. The port is selected by bits 2:1 of the smb_en register > (0x2C). > > Only one port can be active at any point in time therefore a mutex is > needed in order to synchronize access. > > Tested on HP ProLiant MicroServer G7 N54L (where this patch adds > support to access sensor data from the w83795adg). > > Cc: Thomas Brandon <tbrandonau@gmail.com> > Cc: Eddi De Pieri <eddi@depieri.net> > Signed-off-by: Christian Fetzer <fetzer.ch@gmail.com> > Reviewed-by: Mika Westerberg <mika.westerberg@linux.intel.com> > --- > drivers/i2c/busses/i2c-piix4.c | 105 > +++++++++++++++++++++++++++++++++++++++-- > 1 file changed, 100 insertions(+), 5 deletions(-) > > diff --git a/drivers/i2c/busses/i2c-piix4.c b/drivers/i2c/busses/i2c- > piix4.c > index 67ada1e..4beaa3d 100644 > --- a/drivers/i2c/busses/i2c-piix4.c > +++ b/drivers/i2c/busses/i2c-piix4.c > @@ -23,6 +23,9 @@ > > Note: we assume there can only be one device, with one or more > SMBus interfaces. > + The device can register multiple i2c_adapters (up to > PIIX4_MAX_ADAPTERS). > + For devices supporting multiple ports the i2c_adapter should > provide > + an i2c_algorithm to access them. > */ > > #include <linux/module.h> > @@ -37,6 +40,7 @@ > #include <linux/dmi.h> > #include <linux/acpi.h> > #include <linux/io.h> > +#include <linux/mutex.h> > > > /* PIIX4 SMBus address offsets */ > @@ -129,10 +133,12 @@ static const struct dmi_system_id > piix4_dmi_ibm[] = { > }; > > /* SB800 globals */ > +DEFINE_MUTEX(piix4_mutex_sb800); Same question as for patch 3. > static bool piix4_smb_idx_sb800; > > struct i2c_piix4_adapdata { > unsigned short smba; > + unsigned short port; > }; > > static int piix4_setup(struct pci_dev *PIIX4_dev, > @@ -312,6 +318,8 @@ static int piix4_setup_sb800(struct pci_dev > *PIIX4_dev, > else > dev_dbg(&PIIX4_dev->dev, "Using SMI# for SMBus\n"); > > + mutex_init(&piix4_mutex_sb800); > + > dev_info(&PIIX4_dev->dev, > "SMBus Host Controller at 0x%x, revision %d\n", > piix4_smba, i2ccfg >> 4); > @@ -526,6 +534,43 @@ static s32 piix4_access(struct i2c_adapter * > adap, u16 addr, > return 0; > } > > +/* > + * Handles access to multiple SMBus ports on the SB800. > + * The port is selected by bits 2:1 of the smb_en register (0x2C). > + * Returns negative errno on error. > + * > + * Note: The selected port must be returned to the initial selection > to avoid > + * problems on certain systems. > + */ > +static s32 piix4_access_sb800(struct i2c_adapter *adap, u16 addr, > + unsigned short flags, char read_write, > + u8 command, int size, union i2c_smbus_data *data) > +{ > + struct i2c_piix4_adapdata *adapdata = > i2c_get_adapdata(adap); > + u8 smba_en_lo, smb_en = 0x2c; > + u8 port; > + int retval; > + > + mutex_lock(&piix4_mutex_sb800); > + > + outb_p(smb_en, SB800_PIIX4_SMB_IDX); > + smba_en_lo = inb_p(SB800_PIIX4_SMB_IDX + 1); > + > + port = adapdata->port; > + if ((smba_en_lo & 6) != (port << 1)) > + outb_p((smba_en_lo & ~6) | (port << 1), > + SB800_PIIX4_SMB_IDX + 1); > + > + retval = piix4_access(adap, addr, flags, read_write, > + command, size, data); > + > + outb_p(smba_en_lo, SB800_PIIX4_SMB_IDX + 1); > + > + mutex_unlock(&piix4_mutex_sb800); > + > + return retval; > +} > + > static u32 piix4_func(struct i2c_adapter *adapter) > { > return I2C_FUNC_SMBUS_QUICK | I2C_FUNC_SMBUS_BYTE | > @@ -538,6 +583,11 @@ static const struct i2c_algorithm > smbus_algorithm = { > .functionality = piix4_func, > }; > > +static const struct i2c_algorithm piix4_smbus_algorithm_sb800 = { > + .smbus_xfer = piix4_access_sb800, > + .functionality = piix4_func, > +}; > + > static const struct pci_device_id piix4_ids[] = { > { PCI_DEVICE(PCI_VENDOR_ID_INTEL, > PCI_DEVICE_ID_INTEL_82371AB_3) }, > { PCI_DEVICE(PCI_VENDOR_ID_INTEL, > PCI_DEVICE_ID_INTEL_82443MX_3) }, > @@ -613,6 +663,42 @@ static int piix4_add_adapter(struct pci_dev > *dev, unsigned short smba, > return 0; > } > > +static int piix4_add_adapters_sb800(struct pci_dev *dev, unsigned > short smba) > +{ > + unsigned short port; This one has to be at least signed and int to be aligned with patch 2. > + int retval; > + struct i2c_piix4_adapdata *adapdata; These lines better to exchange. > + > + for (port = 0; port < PIIX4_MAX_ADAPTERS; port++) { > + retval = piix4_add_adapter(dev, smba, > + &piix4_main_adapters[port > ]); > + if (retval < 0) > + goto error; > + > + piix4_main_adapters[port]->algo = > &piix4_smbus_algorithm_sb800; > + > + adapdata = > i2c_get_adapdata(piix4_main_adapters[port]); > + adapdata->port = port; > + } > + > + return retval; > + > +error: > + dev_err(&dev->dev, > + "Error setting up SB800 adapters. > Unregistering!\n"); > + while (--port >= 0) { > + adapdata = > i2c_get_adapdata(piix4_main_adapters[port]); > + if (adapdata->smba) { > + i2c_del_adapter(piix4_main_adapters[port]); > + kfree(adapdata); > + kfree(piix4_main_adapters[port]); > + piix4_main_adapters[port] = NULL; > + } > + } > + > + return retval; > +} > + > static int piix4_probe(struct pci_dev *dev, const struct > pci_device_id *id) > { > int retval; > @@ -631,19 +717,28 @@ static int piix4_probe(struct pci_dev *dev, > const struct pci_device_id *id) > > /* base address location etc changed in SB800 */ > retval = piix4_setup_sb800(dev, id, 0); > + if (retval < 0) > + return retval; > + > + /* > + * Try to register multiplexed main SMBus adapter, > + * give up if we can't > + */ > + retval = piix4_add_adapters_sb800(dev, retval); > } else { > retval = piix4_setup(dev, id); > + if (retval < 0) > + return retval; > + > + /* Try to register main SMBus adapter, give up if we > can't */ > + retval = piix4_add_adapter(dev, retval, > + &piix4_main_adapters[0]); > } > > /* If no main SMBus found, give up */ > if (retval < 0) > return retval; > > - /* Try to register main SMBus adapter, give up if we can't > */ > - retval = piix4_add_adapter(dev, retval, > &piix4_main_adapters[0]); > - if (retval < 0) > - return retval; > - > /* Check for auxiliary SMBus on some AMD chipsets */ > retval = -ENODEV; >
Sorry for the late reaction, but... On Mon, 09 Nov 2015 12:45:42 +0200, Andy Shevchenko wrote: > On Sat, 2015-11-07 at 12:35 +0100, Christian Fetzer wrote: > (...) > > @@ -129,10 +133,12 @@ static const struct dmi_system_id > > piix4_dmi_ibm[] = { > > }; > > > > /* SB800 globals */ > > +DEFINE_MUTEX(piix4_mutex_sb800); > > Same question as for patch 3. Noooooo! The mutex is needed to ensure that only one of the 4 multiplexed SMBus ports is used at any given time. i2c_piix4_adapdata is per i2c_adapter, so if you move the mutex there you get one mutex per SMBus port, which completely voids the point of having a mutex. A per-port mutex doesn't protect anything, all it does is serialize the access to THAT port, but i2c-core already takes care of this (thankfully.) So moving this mutex to i2c_piix4_adapdata introduced a serious bug in the driver, which needs to be fixed ASAP.
On Fri, 2016-01-22 at 13:39 +0100, Jean Delvare wrote: > Sorry for the late reaction, but... > > On Mon, 09 Nov 2015 12:45:42 +0200, Andy Shevchenko wrote: > > On Sat, 2015-11-07 at 12:35 +0100, Christian Fetzer wrote: > > (...) > > > @@ -129,10 +133,12 @@ static const struct dmi_system_id > > > piix4_dmi_ibm[] = { > > > }; > > > > > > /* SB800 globals */ > > > +DEFINE_MUTEX(piix4_mutex_sb800); > > > > Same question as for patch 3. > > Noooooo! Calm down, it was only a question. I don't know why the author chose the way that is in upstream. Good you have explained this here.
diff --git a/drivers/i2c/busses/i2c-piix4.c b/drivers/i2c/busses/i2c-piix4.c index 67ada1e..4beaa3d 100644 --- a/drivers/i2c/busses/i2c-piix4.c +++ b/drivers/i2c/busses/i2c-piix4.c @@ -23,6 +23,9 @@ Note: we assume there can only be one device, with one or more SMBus interfaces. + The device can register multiple i2c_adapters (up to PIIX4_MAX_ADAPTERS). + For devices supporting multiple ports the i2c_adapter should provide + an i2c_algorithm to access them. */ #include <linux/module.h> @@ -37,6 +40,7 @@ #include <linux/dmi.h> #include <linux/acpi.h> #include <linux/io.h> +#include <linux/mutex.h> /* PIIX4 SMBus address offsets */ @@ -129,10 +133,12 @@ static const struct dmi_system_id piix4_dmi_ibm[] = { }; /* SB800 globals */ +DEFINE_MUTEX(piix4_mutex_sb800); static bool piix4_smb_idx_sb800; struct i2c_piix4_adapdata { unsigned short smba; + unsigned short port; }; static int piix4_setup(struct pci_dev *PIIX4_dev, @@ -312,6 +318,8 @@ static int piix4_setup_sb800(struct pci_dev *PIIX4_dev, else dev_dbg(&PIIX4_dev->dev, "Using SMI# for SMBus\n"); + mutex_init(&piix4_mutex_sb800); + dev_info(&PIIX4_dev->dev, "SMBus Host Controller at 0x%x, revision %d\n", piix4_smba, i2ccfg >> 4); @@ -526,6 +534,43 @@ static s32 piix4_access(struct i2c_adapter * adap, u16 addr, return 0; } +/* + * Handles access to multiple SMBus ports on the SB800. + * The port is selected by bits 2:1 of the smb_en register (0x2C). + * Returns negative errno on error. + * + * Note: The selected port must be returned to the initial selection to avoid + * problems on certain systems. + */ +static s32 piix4_access_sb800(struct i2c_adapter *adap, u16 addr, + unsigned short flags, char read_write, + u8 command, int size, union i2c_smbus_data *data) +{ + struct i2c_piix4_adapdata *adapdata = i2c_get_adapdata(adap); + u8 smba_en_lo, smb_en = 0x2c; + u8 port; + int retval; + + mutex_lock(&piix4_mutex_sb800); + + outb_p(smb_en, SB800_PIIX4_SMB_IDX); + smba_en_lo = inb_p(SB800_PIIX4_SMB_IDX + 1); + + port = adapdata->port; + if ((smba_en_lo & 6) != (port << 1)) + outb_p((smba_en_lo & ~6) | (port << 1), + SB800_PIIX4_SMB_IDX + 1); + + retval = piix4_access(adap, addr, flags, read_write, + command, size, data); + + outb_p(smba_en_lo, SB800_PIIX4_SMB_IDX + 1); + + mutex_unlock(&piix4_mutex_sb800); + + return retval; +} + static u32 piix4_func(struct i2c_adapter *adapter) { return I2C_FUNC_SMBUS_QUICK | I2C_FUNC_SMBUS_BYTE | @@ -538,6 +583,11 @@ static const struct i2c_algorithm smbus_algorithm = { .functionality = piix4_func, }; +static const struct i2c_algorithm piix4_smbus_algorithm_sb800 = { + .smbus_xfer = piix4_access_sb800, + .functionality = piix4_func, +}; + static const struct pci_device_id piix4_ids[] = { { PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_82371AB_3) }, { PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_82443MX_3) }, @@ -613,6 +663,42 @@ static int piix4_add_adapter(struct pci_dev *dev, unsigned short smba, return 0; } +static int piix4_add_adapters_sb800(struct pci_dev *dev, unsigned short smba) +{ + unsigned short port; + int retval; + struct i2c_piix4_adapdata *adapdata; + + for (port = 0; port < PIIX4_MAX_ADAPTERS; port++) { + retval = piix4_add_adapter(dev, smba, + &piix4_main_adapters[port]); + if (retval < 0) + goto error; + + piix4_main_adapters[port]->algo = &piix4_smbus_algorithm_sb800; + + adapdata = i2c_get_adapdata(piix4_main_adapters[port]); + adapdata->port = port; + } + + return retval; + +error: + dev_err(&dev->dev, + "Error setting up SB800 adapters. Unregistering!\n"); + while (--port >= 0) { + adapdata = i2c_get_adapdata(piix4_main_adapters[port]); + if (adapdata->smba) { + i2c_del_adapter(piix4_main_adapters[port]); + kfree(adapdata); + kfree(piix4_main_adapters[port]); + piix4_main_adapters[port] = NULL; + } + } + + return retval; +} + static int piix4_probe(struct pci_dev *dev, const struct pci_device_id *id) { int retval; @@ -631,19 +717,28 @@ static int piix4_probe(struct pci_dev *dev, const struct pci_device_id *id) /* base address location etc changed in SB800 */ retval = piix4_setup_sb800(dev, id, 0); + if (retval < 0) + return retval; + + /* + * Try to register multiplexed main SMBus adapter, + * give up if we can't + */ + retval = piix4_add_adapters_sb800(dev, retval); } else { retval = piix4_setup(dev, id); + if (retval < 0) + return retval; + + /* Try to register main SMBus adapter, give up if we can't */ + retval = piix4_add_adapter(dev, retval, + &piix4_main_adapters[0]); } /* If no main SMBus found, give up */ if (retval < 0) return retval; - /* Try to register main SMBus adapter, give up if we can't */ - retval = piix4_add_adapter(dev, retval, &piix4_main_adapters[0]); - if (retval < 0) - return retval; - /* Check for auxiliary SMBus on some AMD chipsets */ retval = -ENODEV;