| Message ID | 1514652658-6228-1-git-send-email-linux@roeck-us.net |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [1/2] i2c: piix4: Use request_muxed_region | expand |
On Sat, Dec 30, 2017 at 08:50:57AM -0800, Guenter Roeck wrote: > Accesses to SB800_PIIX4_SMB_IDX can occur from multiple drivers. > Use request_muxed_region() to ensure synchronization. > > Signed-off-by: Guenter Roeck <linux@roeck-us.net> Any comments on this patch ? Thanks, Guenter > --- > 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); > 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; > + > 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; > > /* 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); > }
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? > > Signed-off-by: Guenter Roeck <linux@roeck-us.net> > --- > 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 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. > + > 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? 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?
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@roeck-us.net> > > --- > > 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
2018-02-12 19:51 keltezéssel, Guenter Roeck írta: > 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. There are three locations touching the same I/O ports: the PCI-USB quirks code (still doesn't use locking AFAIK), i2c-piix4 and the sp5100_tco watchdog drivers. Best regards, Zoltán Böszörményi
On 02/13/2018 03:48 AM, Böszörményi Zoltán wrote: > 2018-02-12 19:51 keltezéssel, Guenter Roeck írta: >> 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. > > There are three locations touching the same I/O ports: > the PCI-USB quirks code (still doesn't use locking AFAIK), i2c-piix4 and > the sp5100_tco watchdog drivers. > There are a few more in arch code, though probably not all of those (or maybe none ?) are relevant. arch/x86/kernel/quirks.c arch/x86/kernel/early-quirks.c arch/x86/pci/fixup.c arch/mips/loongson64/loongson-3/acpi_init.c drivers/usb/host/pci-quirks.c drivers/watchdog/sp5100_tco.[ch] drivers/i2c/busses/i2c-piix4.c Guenter
On Mon, 12 Feb 2018 10:51:52 -0800, Guenter Roeck wrote: > On Mon, Feb 12, 2018 at 11:10:41AM +0100, Jean Delvare wrote: > > On Sat, 30 Dec 2017 08:50:57 -0800, Guenter Roeck wrote: > > > @@ -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. OK, two things which are not supposed to happen, so failing is the right thing to do. > > 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. I'm confused. How would the user know what to look for in /proc/iomem (or, I believe, /proc/ioports actually) if the driver does not print which resource allocation failed? If the information is already printed somewhere, then I agree there's no point adding a message. But from the code I could not find it.
On Wed, Feb 14, 2018 at 03:23:05PM +0100, Jean Delvare wrote: > On Mon, 12 Feb 2018 10:51:52 -0800, Guenter Roeck wrote: > > On Mon, Feb 12, 2018 at 11:10:41AM +0100, Jean Delvare wrote: > > > On Sat, 30 Dec 2017 08:50:57 -0800, Guenter Roeck wrote: > > > > @@ -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. > > OK, two things which are not supposed to happen, so failing is the > right thing to do. > > > > 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. > > I'm confused. How would the user know what to look for in /proc/iomem > (or, I believe, /proc/ioports actually) if the driver does not print > which resource allocation failed? > > If the information is already printed somewhere, then I agree there's no > point adding a message. But from the code I could not find it. I don't see any ack or rev from Jean, so I guess there are open issues?
On Mon, Feb 26, 2018 at 08:55:21PM +0100, Wolfram Sang wrote: > On Wed, Feb 14, 2018 at 03:23:05PM +0100, Jean Delvare wrote: > > On Mon, 12 Feb 2018 10:51:52 -0800, Guenter Roeck wrote: > > > On Mon, Feb 12, 2018 at 11:10:41AM +0100, Jean Delvare wrote: > > > > On Sat, 30 Dec 2017 08:50:57 -0800, Guenter Roeck wrote: > > > > > @@ -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. > > > > OK, two things which are not supposed to happen, so failing is the > > right thing to do. > > > > > > 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. > > > > I'm confused. How would the user know what to look for in /proc/iomem > > (or, I believe, /proc/ioports actually) if the driver does not print > > which resource allocation failed? > > > > If the information is already printed somewhere, then I agree there's no > > point adding a message. But from the code I could not find it. > > I don't see any ack or rev from Jean, so I guess there are open issues? > I was suppopsed to send v2, and I had prepared it, but it looks like I forgot to actually send it out. I'll try to do that today. Guenter
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); 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; + 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; /* 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); }
Accesses to SB800_PIIX4_SMB_IDX can occur from multiple drivers. Use request_muxed_region() to ensure synchronization. Signed-off-by: Guenter Roeck <linux@roeck-us.net> --- drivers/i2c/busses/i2c-piix4.c | 50 ++++++++++++++++++------------------------ 1 file changed, 21 insertions(+), 29 deletions(-)