| Message ID | 1519678013-32749-2-git-send-email-linux@roeck-us.net |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [v2,1/2] i2c: piix4: Use usleep_range() | expand |
On Mon, Feb 26, 2018 at 12:46:53PM -0800, Guenter Roeck wrote: > Accesses to SB800_PIIX4_SMB_IDX can occur from multiple drivers. > One example for another driver is the sp5100_tco driver. > > Use request_muxed_region() to ensure synchronization. > > Signed-off-by: Guenter Roeck <linux@roeck-us.net> To understand better: How does this relate to the series Zoltan Boszormenyi was proposing (in one sentence)? > --- > v2: Dropped now unnecessary include of linux/mutex.h > Added error message if request_muxed_region() fails > Spell out affected driver in commit message > > Note that I did not try to do a performance analysis. > Doing so seemed excessive, even more so in the context of > piix4_imc_sleep() already using request_muxed_region(). > > drivers/i2c/busses/i2c-piix4.c | 55 +++++++++++++++++++----------------------- > 1 file changed, 25 insertions(+), 30 deletions(-) > > diff --git a/drivers/i2c/busses/i2c-piix4.c b/drivers/i2c/busses/i2c-piix4.c > index 4c1f6aaec0fc..90946a8b9a75 100644 > --- a/drivers/i2c/busses/i2c-piix4.c > +++ b/drivers/i2c/busses/i2c-piix4.c > @@ -40,7 +40,6 @@ > #include <linux/dmi.h> > #include <linux/acpi.h> > #include <linux/io.h> > -#include <linux/mutex.h> > > > /* PIIX4 SMBus address offsets */ > @@ -153,10 +152,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); > static u8 piix4_port_sel_sb800; > static u8 piix4_port_mask_sb800; > static u8 piix4_port_shift_sb800; > @@ -298,12 +294,19 @@ 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")) { > + dev_err(&PIIX4_dev->dev, > + "SMB base address index region 0x%x already in use.\n", > + SB800_PIIX4_SMB_IDX); > + return -EBUSY; > + } > + > 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 +376,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 +389,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 +687,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; > > /* Request the SMBUS semaphore, avoid conflicts with the IMC */ > smbslvcnt = inb_p(SMBSLVCNT); > @@ -695,8 +704,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 +762,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 +908,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 +924,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 +981,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); > } > -- > 2.7.4 >
On Mon, Feb 26, 2018 at 09:52:04PM +0100, Wolfram Sang wrote: > On Mon, Feb 26, 2018 at 12:46:53PM -0800, Guenter Roeck wrote: > > Accesses to SB800_PIIX4_SMB_IDX can occur from multiple drivers. > > One example for another driver is the sp5100_tco driver. > > > > Use request_muxed_region() to ensure synchronization. > > > > Signed-off-by: Guenter Roeck <linux@roeck-us.net> > > To understand better: How does this relate to the series Zoltan > Boszormenyi was proposing (in one sentence)? > It is an alternate and somewhat simpler solution which does not attempt to change the kernel API. My primary goal was to find a solution for the immediate problem between the watchdog and i2c drivers. My solution does not attempt to fix the access problem for (initialization) functions with no return code. At the same time, I prefer to use functions which _do_ return an error if that is possible since it _may_ after all be that there are other users calling request_region(). Having said that, I'll be just as happy if Zoltan's series is accepted instead of this patch (except for patch #5; the watchdog driver has already been converted to use request_muxed_region). Thanks, Guenter > > --- > > v2: Dropped now unnecessary include of linux/mutex.h > > Added error message if request_muxed_region() fails > > Spell out affected driver in commit message > > > > Note that I did not try to do a performance analysis. > > Doing so seemed excessive, even more so in the context of > > piix4_imc_sleep() already using request_muxed_region(). > > > > drivers/i2c/busses/i2c-piix4.c | 55 +++++++++++++++++++----------------------- > > 1 file changed, 25 insertions(+), 30 deletions(-) > > > > diff --git a/drivers/i2c/busses/i2c-piix4.c b/drivers/i2c/busses/i2c-piix4.c > > index 4c1f6aaec0fc..90946a8b9a75 100644 > > --- a/drivers/i2c/busses/i2c-piix4.c > > +++ b/drivers/i2c/busses/i2c-piix4.c > > @@ -40,7 +40,6 @@ > > #include <linux/dmi.h> > > #include <linux/acpi.h> > > #include <linux/io.h> > > -#include <linux/mutex.h> > > > > > > /* PIIX4 SMBus address offsets */ > > @@ -153,10 +152,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); > > static u8 piix4_port_sel_sb800; > > static u8 piix4_port_mask_sb800; > > static u8 piix4_port_shift_sb800; > > @@ -298,12 +294,19 @@ 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")) { > > + dev_err(&PIIX4_dev->dev, > > + "SMB base address index region 0x%x already in use.\n", > > + SB800_PIIX4_SMB_IDX); > > + return -EBUSY; > > + } > > + > > 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 +376,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 +389,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 +687,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; > > > > /* Request the SMBUS semaphore, avoid conflicts with the IMC */ > > smbslvcnt = inb_p(SMBSLVCNT); > > @@ -695,8 +704,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 +762,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 +908,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 +924,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 +981,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); > > } > > -- > > 2.7.4 > >
> It is an alternate and somewhat simpler solution which does not > attempt to change the kernel API. My primary goal was to find > a solution for the immediate problem between the watchdog and > i2c drivers. My solution does not attempt to fix the access problem > for (initialization) functions with no return code. At the same time, > I prefer to use functions which _do_ return an error if that is possible > since it _may_ after all be that there are other users calling > request_region(). > > Having said that, I'll be just as happy if Zoltan's series is accepted > instead of this patch (except for patch #5; the watchdog driver has > already been converted to use request_muxed_region). Thanks for the explanation! So, as I understand: I can apply this patch to fix the immediate (and long standing) problem and decouple it from the API change. The latter can then still be worked on and the changes for the piix driver should then be minimal. Correct?
On Mon, Feb 26, 2018 at 10:43:07PM +0100, Wolfram Sang wrote: > > > It is an alternate and somewhat simpler solution which does not > > attempt to change the kernel API. My primary goal was to find > > a solution for the immediate problem between the watchdog and > > i2c drivers. My solution does not attempt to fix the access problem > > for (initialization) functions with no return code. At the same time, > > I prefer to use functions which _do_ return an error if that is possible > > since it _may_ after all be that there are other users calling > > request_region(). > > > > Having said that, I'll be just as happy if Zoltan's series is accepted > > instead of this patch (except for patch #5; the watchdog driver has > > already been converted to use request_muxed_region). > > Thanks for the explanation! > > So, as I understand: I can apply this patch to fix the immediate (and > long standing) problem and decouple it from the API change. The latter > can then still be worked on and the changes for the piix driver should > then be minimal. Correct? And we should have Zoltan on CC.
On Mon, Feb 26, 2018 at 10:43:07PM +0100, Wolfram Sang wrote: > > > It is an alternate and somewhat simpler solution which does not > > attempt to change the kernel API. My primary goal was to find > > a solution for the immediate problem between the watchdog and > > i2c drivers. My solution does not attempt to fix the access problem > > for (initialization) functions with no return code. At the same time, > > I prefer to use functions which _do_ return an error if that is possible > > since it _may_ after all be that there are other users calling > > request_region(). > > > > Having said that, I'll be just as happy if Zoltan's series is accepted > > instead of this patch (except for patch #5; the watchdog driver has > > already been converted to use request_muxed_region). > > Thanks for the explanation! > > So, as I understand: I can apply this patch to fix the immediate (and > long standing) problem and decouple it from the API change. The latter > can then still be worked on and the changes for the piix driver should > then be minimal. Correct? > Correct. Actually, you won't need any further piix driver changes; my understanding is that patches 1-3 from Zoltan's series can be applied independently. Guenter
On Mon, Feb 26, 2018 at 10:44:14PM +0100, Wolfram Sang wrote: > On Mon, Feb 26, 2018 at 10:43:07PM +0100, Wolfram Sang wrote: > > > > > It is an alternate and somewhat simpler solution which does not > > > attempt to change the kernel API. My primary goal was to find > > > a solution for the immediate problem between the watchdog and > > > i2c drivers. My solution does not attempt to fix the access problem > > > for (initialization) functions with no return code. At the same time, > > > I prefer to use functions which _do_ return an error if that is possible > > > since it _may_ after all be that there are other users calling > > > request_region(). > > > > > > Having said that, I'll be just as happy if Zoltan's series is accepted > > > instead of this patch (except for patch #5; the watchdog driver has > > > already been converted to use request_muxed_region). > > > > Thanks for the explanation! > > > > So, as I understand: I can apply this patch to fix the immediate (and > > long standing) problem and decouple it from the API change. The latter > > can then still be worked on and the changes for the piix driver should > > then be minimal. Correct? > > And we should have Zoltan on CC. > Yes. Sorry, things are a bit hectic today. Guenter
On Mon, 26 Feb 2018 12:46:53 -0800, Guenter Roeck wrote: > Accesses to SB800_PIIX4_SMB_IDX can occur from multiple drivers. > One example for another driver is the sp5100_tco driver. > > Use request_muxed_region() to ensure synchronization. > > Signed-off-by: Guenter Roeck <linux@roeck-us.net> > --- > v2: Dropped now unnecessary include of linux/mutex.h > Added error message if request_muxed_region() fails > Spell out affected driver in commit message > > Note that I did not try to do a performance analysis. > Doing so seemed excessive, even more so in the context of > piix4_imc_sleep() already using request_muxed_region(). > > drivers/i2c/busses/i2c-piix4.c | 55 +++++++++++++++++++----------------------- > 1 file changed, 25 insertions(+), 30 deletions(-) > > diff --git a/drivers/i2c/busses/i2c-piix4.c b/drivers/i2c/busses/i2c-piix4.c > index 4c1f6aaec0fc..90946a8b9a75 100644 > --- a/drivers/i2c/busses/i2c-piix4.c > +++ b/drivers/i2c/busses/i2c-piix4.c > @@ -40,7 +40,6 @@ > #include <linux/dmi.h> > #include <linux/acpi.h> > #include <linux/io.h> > -#include <linux/mutex.h> > > > /* PIIX4 SMBus address offsets */ > @@ -153,10 +152,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); > static u8 piix4_port_sel_sb800; > static u8 piix4_port_mask_sb800; > static u8 piix4_port_shift_sb800; > @@ -298,12 +294,19 @@ 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")) { > + dev_err(&PIIX4_dev->dev, > + "SMB base address index region 0x%x already in use.\n", > + SB800_PIIX4_SMB_IDX); > + return -EBUSY; > + } > + > 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 +376,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 +389,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 +687,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; > > /* Request the SMBUS semaphore, avoid conflicts with the IMC */ > smbslvcnt = inb_p(SMBSLVCNT); > @@ -695,8 +704,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 +762,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 +908,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 +924,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 +981,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); > } Reviewed-by: Jean Delvare <jdelvare@suse.de>
On Mon, Feb 26, 2018 at 12:46:53PM -0800, Guenter Roeck wrote: > Accesses to SB800_PIIX4_SMB_IDX can occur from multiple drivers. > One example for another driver is the sp5100_tco driver. > > Use request_muxed_region() to ensure synchronization. > > Signed-off-by: Guenter Roeck <linux@roeck-us.net> Applied to for-next, thanks!
diff --git a/drivers/i2c/busses/i2c-piix4.c b/drivers/i2c/busses/i2c-piix4.c index 4c1f6aaec0fc..90946a8b9a75 100644 --- a/drivers/i2c/busses/i2c-piix4.c +++ b/drivers/i2c/busses/i2c-piix4.c @@ -40,7 +40,6 @@ #include <linux/dmi.h> #include <linux/acpi.h> #include <linux/io.h> -#include <linux/mutex.h> /* PIIX4 SMBus address offsets */ @@ -153,10 +152,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); static u8 piix4_port_sel_sb800; static u8 piix4_port_mask_sb800; static u8 piix4_port_shift_sb800; @@ -298,12 +294,19 @@ 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")) { + dev_err(&PIIX4_dev->dev, + "SMB base address index region 0x%x already in use.\n", + SB800_PIIX4_SMB_IDX); + return -EBUSY; + } + 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 +376,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 +389,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 +687,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; /* Request the SMBUS semaphore, avoid conflicts with the IMC */ smbslvcnt = inb_p(SMBSLVCNT); @@ -695,8 +704,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 +762,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 +908,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 +924,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 +981,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); }
Accesses to SB800_PIIX4_SMB_IDX can occur from multiple drivers. One example for another driver is the sp5100_tco driver. Use request_muxed_region() to ensure synchronization. Signed-off-by: Guenter Roeck <linux@roeck-us.net> --- v2: Dropped now unnecessary include of linux/mutex.h Added error message if request_muxed_region() fails Spell out affected driver in commit message Note that I did not try to do a performance analysis. Doing so seemed excessive, even more so in the context of piix4_imc_sleep() already using request_muxed_region(). drivers/i2c/busses/i2c-piix4.c | 55 +++++++++++++++++++----------------------- 1 file changed, 25 insertions(+), 30 deletions(-)