| Message ID | 90aee333-6490-db08-01ae-c0de1e18368a@gmail.com |
|---|---|
| State | Accepted |
| Commit | 1a987c69ce2c5a406f7ef3713d3ed74a1af70c56 |
| Headers | show |
| Series | i2c: i801: Series with improvements | expand |
On Fri, Aug 06, 2021 at 11:13:29PM +0200, Heiner Kallweit wrote: > p2sb_spinlock is used in i801_add_tco_spt() only, and in process context > only. Therefore a mutex is sufficient, and we can make the definition > local to i801_add_tco_spt(). > > Reviewed-by: Jean Delvare <jdelvare@suse.de> > Reviewed-by: Mika Westerberg <mika.westerberg@linux.intel.com> > Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com> Applied to for-next, thanks! > #include <linux/platform_device.h> > #include <linux/platform_data/itco_wdt.h> > #include <linux/pm_runtime.h> > +#include <linux/mutex.h> Can you also sort the includes while cleaning up the driver? Then, it is more obvious that mutex.h was already needed for acpi_lock. That would be great!
On Fri, Aug 06, 2021 at 11:13:29PM +0200, Heiner Kallweit wrote: > p2sb_spinlock is used in i801_add_tco_spt() only, and in process context > only. Therefore a mutex is sufficient, and we can make the definition > local to i801_add_tco_spt(). The problem with either AFAICT is that it should actually hold PCI rescan lock. See the discussion with Message-ID 20210308122020.57071-1-andriy.shevchenko@linux.intel.com for the details. -- With Best Regards, Andy Shevchenko
On 11.08.2021 17:43, Andy Shevchenko wrote: > On Fri, Aug 06, 2021 at 11:13:29PM +0200, Heiner Kallweit wrote: >> p2sb_spinlock is used in i801_add_tco_spt() only, and in process context >> only. Therefore a mutex is sufficient, and we can make the definition >> local to i801_add_tco_spt(). > > The problem with either AFAICT is that it should actually hold PCI rescan lock. > See the discussion with Message-ID > 20210308122020.57071-1-andriy.shevchenko@linux.intel.com for the details. > Thanks for the link. I see that you use pci_lock_rescan_remove() but at a first glance didn't see this being discussed. Maybe because it's obvious .. i801 was discussed here: https://lore.kernel.org/linux-i2c/20210310155145.513a7165@endymion/ However discussion seems to have ended w/o result. What's the status of your p2sb series? Backgroud of the question is: Does it make sense to wait for your series to be applied, to make use of your new ps2b library functions? Or change the mutex to the rescan mutex for the time being?
On Wed, Aug 11, 2021 at 10:27:26PM +0200, Heiner Kallweit wrote: > On 11.08.2021 17:43, Andy Shevchenko wrote: > > On Fri, Aug 06, 2021 at 11:13:29PM +0200, Heiner Kallweit wrote: > >> p2sb_spinlock is used in i801_add_tco_spt() only, and in process context > >> only. Therefore a mutex is sufficient, and we can make the definition > >> local to i801_add_tco_spt(). > > > > The problem with either AFAICT is that it should actually hold PCI rescan lock. > > See the discussion with Message-ID > > 20210308122020.57071-1-andriy.shevchenko@linux.intel.com for the details. > > > Thanks for the link. I see that you use pci_lock_rescan_remove() but at a first > glance didn't see this being discussed. Maybe because it's obvious .. > > i801 was discussed here: > https://lore.kernel.org/linux-i2c/20210310155145.513a7165@endymion/ > However discussion seems to have ended w/o result. What's the status of your > p2sb series? Backgroud of the question is: Does it make sense to wait for > your series to be applied, to make use of your new ps2b library functions? > Or change the mutex to the rescan mutex for the time being? The problem with P2SB PCI device is that it's used as a holder for the firmware programmed value of the BAR which mustn't be relocated. If PCI runs rescan concurrently with this piece of code (still a probability higher than 0) then we will be in bad situation. So, yes, rescan lock is a minimum what has to be done. In regard to the series the idea is to unhide the P2SB in early PCI quirk and mark its resources unrelocatable. But I haven't time to research that. It's postponed currently due to lack of time on my side because higher priority tasks ongoing. -- With Best Regards, Andy Shevchenko
diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c index ef6dbb531..12e0c2ac3 100644 --- a/drivers/i2c/busses/i2c-i801.c +++ b/drivers/i2c/busses/i2c-i801.c @@ -110,6 +110,7 @@ #include <linux/platform_device.h> #include <linux/platform_data/itco_wdt.h> #include <linux/pm_runtime.h> +#include <linux/mutex.h> #if IS_ENABLED(CONFIG_I2C_MUX_GPIO) && defined CONFIG_DMI #include <linux/gpio/machine.h> @@ -1492,12 +1493,11 @@ static const struct itco_wdt_platform_data spt_tco_platform_data = { .version = 4, }; -static DEFINE_SPINLOCK(p2sb_spinlock); - static struct platform_device * i801_add_tco_spt(struct i801_priv *priv, struct pci_dev *pci_dev, struct resource *tco_res) { + static DEFINE_MUTEX(p2sb_mutex); struct resource *res; unsigned int devfn; u64 base64_addr; @@ -1510,7 +1510,7 @@ i801_add_tco_spt(struct i801_priv *priv, struct pci_dev *pci_dev, * enumerated by the PCI subsystem, so we need to unhide/hide it * to lookup the P2SB BAR. */ - spin_lock(&p2sb_spinlock); + mutex_lock(&p2sb_mutex); devfn = PCI_DEVFN(PCI_SLOT(pci_dev->devfn), 1); @@ -1528,7 +1528,7 @@ i801_add_tco_spt(struct i801_priv *priv, struct pci_dev *pci_dev, /* Hide the P2SB device, if it was hidden before */ if (hidden) pci_bus_write_config_byte(pci_dev->bus, devfn, 0xe1, hidden); - spin_unlock(&p2sb_spinlock); + mutex_unlock(&p2sb_mutex); res = &tco_res[1]; if (pci_dev->device == PCI_DEVICE_ID_INTEL_DNV_SMBUS)