On Mon, Feb 12, 2018 at 11:10:41AM +0100, Jean Delvare wrote:
> Hi Guneter,
>
> Sorry for the delay :(
>
> On Sat, 30 Dec 2017 08:50:57 -0800, Guenter Roeck wrote:
> > Accesses to SB800_PIIX4_SMB_IDX can occur from multiple drivers.
> > Use request_muxed_region() to ensure synchronization.
>
> Which ones? Documenting it, at least in the commit message, would seem
> useful. Out of curiosity, have these other drivers been converted to
> use request_muxed_region already?
>
Primarily watchdog, but there is also unprotected initialization code
in several locations. I did convert the watchdog driver, and the changes
will be in v4.16. I did not touch the other code since none of the calls
has an error return.
> >
> > Signed-off-by: Guenter Roeck <linux@xxxxxxxxxxxx>
> > ---
> > drivers/i2c/busses/i2c-piix4.c | 50 ++++++++++++++++++------------------------
> > 1 file changed, 21 insertions(+), 29 deletions(-)
> >
> > diff --git a/drivers/i2c/busses/i2c-piix4.c b/drivers/i2c/busses/i2c-piix4.c
> > index 462948e2c535..78dd5951d6e7 100644
> > --- a/drivers/i2c/busses/i2c-piix4.c
> > +++ b/drivers/i2c/busses/i2c-piix4.c
> > @@ -153,10 +153,7 @@ static const struct dmi_system_id piix4_dmi_ibm[] = {
> >
> > /*
> > * SB800 globals
> > - * piix4_mutex_sb800 protects piix4_port_sel_sb800 and the pair
> > - * of I/O ports at SB800_PIIX4_SMB_IDX.
> > */
> > -static DEFINE_MUTEX(piix4_mutex_sb800);
>
> With this gone, you can remove #include <linux/mutex.h>.
>
> > static u8 piix4_port_sel_sb800;
> > static u8 piix4_port_mask_sb800;
> > static u8 piix4_port_shift_sb800;
> > @@ -298,12 +295,15 @@ static int piix4_setup_sb800(struct pci_dev *PIIX4_dev,
> > else
> > smb_en = (aux) ? 0x28 : 0x2c;
> >
> > - mutex_lock(&piix4_mutex_sb800);
> > + if (!request_muxed_region(SB800_PIIX4_SMB_IDX, 2, "sb800_piix4_smb"))
> > + return -EBUSY;
>
> This would happen if and only if another driver has requested the
> region already but without IORESOURCE_MUXED, right? Don't you want to
Or if its call to alloc_resource() fails.
> write an error message then? I don't think request_muxed_region() will
> do, and probe failing with -EBUSY but no error message logged would be
> hard to diagnose.
>
NP, though the analysis is quite simple - /proc/iomem will show the culprit.
> > +
> > outb_p(smb_en, SB800_PIIX4_SMB_IDX);
> > smba_en_lo = inb_p(SB800_PIIX4_SMB_IDX + 1);
> > outb_p(smb_en + 1, SB800_PIIX4_SMB_IDX);
> > smba_en_hi = inb_p(SB800_PIIX4_SMB_IDX + 1);
> > - mutex_unlock(&piix4_mutex_sb800);
> > +
> > + release_region(SB800_PIIX4_SMB_IDX, 2);
> >
> > if (!smb_en) {
> > smb_en_status = smba_en_lo & 0x10;
> > @@ -373,7 +373,12 @@ static int piix4_setup_sb800(struct pci_dev *PIIX4_dev,
> > break;
> > }
> > } else {
> > - mutex_lock(&piix4_mutex_sb800);
> > + if (!request_muxed_region(SB800_PIIX4_SMB_IDX, 2,
> > + "sb800_piix4_smb")) {
> > + release_region(piix4_smba, SMBIOSIZE);
> > + return -EBUSY;
> > + }
> > +
> > outb_p(SB800_PIIX4_PORT_IDX_SEL, SB800_PIIX4_SMB_IDX);
> > port_sel = inb_p(SB800_PIIX4_SMB_IDX + 1);
> > piix4_port_sel_sb800 = (port_sel & 0x01) ?
> > @@ -381,7 +386,7 @@ static int piix4_setup_sb800(struct pci_dev *PIIX4_dev,
> > SB800_PIIX4_PORT_IDX;
> > piix4_port_mask_sb800 = SB800_PIIX4_PORT_IDX_MASK;
> > piix4_port_shift_sb800 = SB800_PIIX4_PORT_IDX_SHIFT;
> > - mutex_unlock(&piix4_mutex_sb800);
> > + release_region(SB800_PIIX4_SMB_IDX, 2);
> > }
> >
> > dev_info(&PIIX4_dev->dev,
> > @@ -679,7 +684,8 @@ static s32 piix4_access_sb800(struct i2c_adapter *adap, u16 addr,
> > u8 port;
> > int retval;
> >
> > - mutex_lock(&piix4_mutex_sb800);
> > + if (!request_muxed_region(SB800_PIIX4_SMB_IDX, 2, "sb800_piix4_smb"))
> > + return -EBUSY;
>
> Did you check the performance cost? I thought that
> request_muxed_region() was meant for driver setup, I did not expect it
> to be used at driver run-time. Requesting the region again for every
> transaction seems quite costly?
>
I did check why the driver has such a bad performance, which is why
I submitted the other patch to change msleep() to usleep_range().
Evaulating the actual per-call overhead seems to be quite pointless, unless
someone volunteers to introduce a specific access API for situations like this.
It is definitely not a unique situation - I have to do something similar
in the out-of-tree it87 driver, for example.
> That being said, being slow is certainly better than failing, as is
> currently the case, so I'm fine with this change anyway. Just curious.
>
> >
> > /* Request the SMBUS semaphore, avoid conflicts with the IMC */
> > smbslvcnt = inb_p(SMBSLVCNT);
> > @@ -695,8 +701,8 @@ static s32 piix4_access_sb800(struct i2c_adapter *adap, u16 addr,
> > } while (--retries);
> > /* SMBus is still owned by the IMC, we give up */
> > if (!retries) {
> > - mutex_unlock(&piix4_mutex_sb800);
> > - return -EBUSY;
> > + retval = -EBUSY;
> > + goto release;
> > }
> >
> > /*
> > @@ -753,8 +759,8 @@ static s32 piix4_access_sb800(struct i2c_adapter *adap, u16 addr,
> > if ((size == I2C_SMBUS_BLOCK_DATA) && adapdata->notify_imc)
> > piix4_imc_wakeup();
> >
> > - mutex_unlock(&piix4_mutex_sb800);
> > -
> > +release:
> > + release_region(SB800_PIIX4_SMB_IDX, 2);
> > return retval;
> > }
> >
> > @@ -899,13 +905,6 @@ static int piix4_probe(struct pci_dev *dev, const struct pci_device_id *id)
> > bool notify_imc = false;
> > is_sb800 = true;
> >
> > - if (!request_region(SB800_PIIX4_SMB_IDX, 2, "smba_idx")) {
> > - dev_err(&dev->dev,
> > - "SMBus base address index region 0x%x already in use!\n",
> > - SB800_PIIX4_SMB_IDX);
> > - return -EBUSY;
> > - }
> > -
> > if (dev->vendor == PCI_VENDOR_ID_AMD &&
> > dev->device == PCI_DEVICE_ID_AMD_KERNCZ_SMBUS) {
> > u8 imc;
> > @@ -922,20 +921,16 @@ 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) {
> > - release_region(SB800_PIIX4_SMB_IDX, 2);
> > + 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, notify_imc);
> > - if (retval < 0) {
> > - release_region(SB800_PIIX4_SMB_IDX, 2);
> > + if (retval < 0)
> > return retval;
> > - }
> > } else {
> > retval = piix4_setup(dev, id);
> > if (retval < 0)
> > @@ -983,11 +978,8 @@ static void piix4_adap_remove(struct i2c_adapter *adap)
> >
> > if (adapdata->smba) {
> > i2c_del_adapter(adap);
> > - if (adapdata->port == (0 << piix4_port_shift_sb800)) {
> > + if (adapdata->port == (0 << piix4_port_shift_sb800))
> > release_region(adapdata->smba, SMBIOSIZE);
> > - if (adapdata->sb800_main)
> > - release_region(SB800_PIIX4_SMB_IDX, 2);
> > - }
> > kfree(adapdata);
> > kfree(adap);
> > }
>
> Everything else looks good to me, thanks.
>
> I assume you have tested this patch on real hardware?
>
I have been running the code on several systems since I submitted
the patch, together with the related changes in the watchdog driver.
Guenter
