| Message ID | 8f88906f-c7da-eb3a-2f14-0f4d46202517@gmail.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | i2c: i801: next set of improvements | expand |
Hi Heiner, On Sat, Mar 04, 2023 at 10:33:05PM +0100, Heiner Kallweit wrote: > I2C core ensures in i2c_smbus_xfer() that the I2C bus lock is held when > calling the smbus_xfer callback. That's i801_access() in our case. > I think it's safe in general to assume that the I2C bus lock is held > when the smbus_xfer callback is called. > Therefore I see no need to define an own mutex. > > Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com> Reviewed-by: Andi Shyti <andi.shyti@kernel.org> Thanks, Andi
Hi Heiner, On Sat, 04 Mar 2023 22:33:05 +0100, Heiner Kallweit wrote: > I2C core ensures in i2c_smbus_xfer() that the I2C bus lock is held when > calling the smbus_xfer callback. That's i801_access() in our case. > I think it's safe in general to assume that the I2C bus lock is held > when the smbus_xfer callback is called. > Therefore I see no need to define an own mutex. > > Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com> > --- > drivers/i2c/busses/i2c-i801.c | 14 ++++---------- > 1 file changed, 4 insertions(+), 10 deletions(-) > > diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c > index d6a0c3b53..7641bd0ac 100644 > --- a/drivers/i2c/busses/i2c-i801.c > +++ b/drivers/i2c/busses/i2c-i801.c > @@ -289,10 +289,9 @@ struct i801_priv { > > /* > * If set to true the host controller registers are reserved for > - * ACPI AML use. Protected by acpi_lock. > + * ACPI AML use. > */ > bool acpi_reserved; > - struct mutex acpi_lock; > }; > > #define FEATURE_SMBUS_PEC BIT(0) > @@ -871,11 +870,8 @@ static s32 i801_access(struct i2c_adapter *adap, u16 addr, > int hwpec, ret; > struct i801_priv *priv = i2c_get_adapdata(adap); > > - mutex_lock(&priv->acpi_lock); > - if (priv->acpi_reserved) { > - mutex_unlock(&priv->acpi_lock); > + if (priv->acpi_reserved) > return -EBUSY; > - } > > pm_runtime_get_sync(&priv->pci_dev->dev); > > @@ -916,7 +912,6 @@ static s32 i801_access(struct i2c_adapter *adap, u16 addr, > > pm_runtime_mark_last_busy(&priv->pci_dev->dev); > pm_runtime_put_autosuspend(&priv->pci_dev->dev); > - mutex_unlock(&priv->acpi_lock); > return ret; > } > > @@ -1566,7 +1561,7 @@ i801_acpi_io_handler(u32 function, acpi_physical_address address, u32 bits, > * further access from the driver itself. This device is now owned > * by the system firmware. > */ > - mutex_lock(&priv->acpi_lock); > + i2c_lock_bus(&priv->adapter, I2C_LOCK_SEGMENT); > > if (!priv->acpi_reserved && i801_acpi_is_smbus_ioport(priv, address)) { > priv->acpi_reserved = true; > @@ -1586,7 +1581,7 @@ i801_acpi_io_handler(u32 function, acpi_physical_address address, u32 bits, > else > status = acpi_os_write_port(address, (u32)*value, bits); > > - mutex_unlock(&priv->acpi_lock); > + i2c_unlock_bus(&priv->adapter, I2C_LOCK_SEGMENT); > > return status; > } > @@ -1640,7 +1635,6 @@ static int i801_probe(struct pci_dev *dev, const struct pci_device_id *id) > priv->adapter.dev.parent = &dev->dev; > ACPI_COMPANION_SET(&priv->adapter.dev, ACPI_COMPANION(&dev->dev)); > priv->adapter.retries = 3; > - mutex_init(&priv->acpi_lock); > > priv->pci_dev = dev; > priv->features = id->driver_data; Looks reasonable, I also can't see any reason why that wouldn't work. But locking and power management can be tricky of course. I'll test this for some time, but I don't think my system actually suffers from this ACPI resource conflict, so this most probably won't be testing much in practice. Thanks,
On 26.06.2023 19:59, Jean Delvare wrote: > Hi Heiner, > > On Sat, 04 Mar 2023 22:33:05 +0100, Heiner Kallweit wrote: >> I2C core ensures in i2c_smbus_xfer() that the I2C bus lock is held when >> calling the smbus_xfer callback. That's i801_access() in our case. >> I think it's safe in general to assume that the I2C bus lock is held >> when the smbus_xfer callback is called. >> Therefore I see no need to define an own mutex. >> >> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com> >> --- >> drivers/i2c/busses/i2c-i801.c | 14 ++++---------- >> 1 file changed, 4 insertions(+), 10 deletions(-) >> >> diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c >> index d6a0c3b53..7641bd0ac 100644 >> --- a/drivers/i2c/busses/i2c-i801.c >> +++ b/drivers/i2c/busses/i2c-i801.c >> @@ -289,10 +289,9 @@ struct i801_priv { >> >> /* >> * If set to true the host controller registers are reserved for >> - * ACPI AML use. Protected by acpi_lock. >> + * ACPI AML use. >> */ >> bool acpi_reserved; >> - struct mutex acpi_lock; >> }; >> >> #define FEATURE_SMBUS_PEC BIT(0) >> @@ -871,11 +870,8 @@ static s32 i801_access(struct i2c_adapter *adap, u16 addr, >> int hwpec, ret; >> struct i801_priv *priv = i2c_get_adapdata(adap); >> >> - mutex_lock(&priv->acpi_lock); >> - if (priv->acpi_reserved) { >> - mutex_unlock(&priv->acpi_lock); >> + if (priv->acpi_reserved) >> return -EBUSY; >> - } >> >> pm_runtime_get_sync(&priv->pci_dev->dev); >> >> @@ -916,7 +912,6 @@ static s32 i801_access(struct i2c_adapter *adap, u16 addr, >> >> pm_runtime_mark_last_busy(&priv->pci_dev->dev); >> pm_runtime_put_autosuspend(&priv->pci_dev->dev); >> - mutex_unlock(&priv->acpi_lock); >> return ret; >> } >> >> @@ -1566,7 +1561,7 @@ i801_acpi_io_handler(u32 function, acpi_physical_address address, u32 bits, >> * further access from the driver itself. This device is now owned >> * by the system firmware. >> */ >> - mutex_lock(&priv->acpi_lock); >> + i2c_lock_bus(&priv->adapter, I2C_LOCK_SEGMENT); >> >> if (!priv->acpi_reserved && i801_acpi_is_smbus_ioport(priv, address)) { >> priv->acpi_reserved = true; >> @@ -1586,7 +1581,7 @@ i801_acpi_io_handler(u32 function, acpi_physical_address address, u32 bits, >> else >> status = acpi_os_write_port(address, (u32)*value, bits); >> >> - mutex_unlock(&priv->acpi_lock); >> + i2c_unlock_bus(&priv->adapter, I2C_LOCK_SEGMENT); >> >> return status; >> } >> @@ -1640,7 +1635,6 @@ static int i801_probe(struct pci_dev *dev, const struct pci_device_id *id) >> priv->adapter.dev.parent = &dev->dev; >> ACPI_COMPANION_SET(&priv->adapter.dev, ACPI_COMPANION(&dev->dev)); >> priv->adapter.retries = 3; >> - mutex_init(&priv->acpi_lock); >> >> priv->pci_dev = dev; >> priv->features = id->driver_data; > > Looks reasonable, I also can't see any reason why that wouldn't work. > But locking and power management can be tricky of course. I'll test > this for some time, but I don't think my system actually suffers from > this ACPI resource conflict, so this most probably won't be testing > much in practice. > What's your opinion after more testing? > Thanks,
Hi Heiner, On Sun, 27 Aug 2023 18:21:43 +0200, Heiner Kallweit wrote: > On 26.06.2023 19:59, Jean Delvare wrote: > > Looks reasonable, I also can't see any reason why that wouldn't work. > > But locking and power management can be tricky of course. I'll test > > this for some time, but I don't think my system actually suffers from > > this ACPI resource conflict, so this most probably won't be testing > > much in practice. > > What's your opinion after more testing? Positive, as I did not hit any problem. As said before, my testing is limited by design and thus is no guarantee that the change is OK in all cases, but at least it's good enough to merge it and see what happens.
On 28.08.2023 09:01, Jean Delvare wrote: > Hi Heiner, > > On Sun, 27 Aug 2023 18:21:43 +0200, Heiner Kallweit wrote: >> On 26.06.2023 19:59, Jean Delvare wrote: >>> Looks reasonable, I also can't see any reason why that wouldn't work. >>> But locking and power management can be tricky of course. I'll test >>> this for some time, but I don't think my system actually suffers from >>> this ACPI resource conflict, so this most probably won't be testing >>> much in practice. >> >> What's your opinion after more testing? > > Positive, as I did not hit any problem. As said before, my testing is > limited by design and thus is no guarantee that the change is OK in all > cases, but at least it's good enough to merge it and see what happens. > Then I'll resubmit the patch because the series has been broken up.
diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c index d6a0c3b53..7641bd0ac 100644 --- a/drivers/i2c/busses/i2c-i801.c +++ b/drivers/i2c/busses/i2c-i801.c @@ -289,10 +289,9 @@ struct i801_priv { /* * If set to true the host controller registers are reserved for - * ACPI AML use. Protected by acpi_lock. + * ACPI AML use. */ bool acpi_reserved; - struct mutex acpi_lock; }; #define FEATURE_SMBUS_PEC BIT(0) @@ -871,11 +870,8 @@ static s32 i801_access(struct i2c_adapter *adap, u16 addr, int hwpec, ret; struct i801_priv *priv = i2c_get_adapdata(adap); - mutex_lock(&priv->acpi_lock); - if (priv->acpi_reserved) { - mutex_unlock(&priv->acpi_lock); + if (priv->acpi_reserved) return -EBUSY; - } pm_runtime_get_sync(&priv->pci_dev->dev); @@ -916,7 +912,6 @@ static s32 i801_access(struct i2c_adapter *adap, u16 addr, pm_runtime_mark_last_busy(&priv->pci_dev->dev); pm_runtime_put_autosuspend(&priv->pci_dev->dev); - mutex_unlock(&priv->acpi_lock); return ret; } @@ -1566,7 +1561,7 @@ i801_acpi_io_handler(u32 function, acpi_physical_address address, u32 bits, * further access from the driver itself. This device is now owned * by the system firmware. */ - mutex_lock(&priv->acpi_lock); + i2c_lock_bus(&priv->adapter, I2C_LOCK_SEGMENT); if (!priv->acpi_reserved && i801_acpi_is_smbus_ioport(priv, address)) { priv->acpi_reserved = true; @@ -1586,7 +1581,7 @@ i801_acpi_io_handler(u32 function, acpi_physical_address address, u32 bits, else status = acpi_os_write_port(address, (u32)*value, bits); - mutex_unlock(&priv->acpi_lock); + i2c_unlock_bus(&priv->adapter, I2C_LOCK_SEGMENT); return status; } @@ -1640,7 +1635,6 @@ static int i801_probe(struct pci_dev *dev, const struct pci_device_id *id) priv->adapter.dev.parent = &dev->dev; ACPI_COMPANION_SET(&priv->adapter.dev, ACPI_COMPANION(&dev->dev)); priv->adapter.retries = 3; - mutex_init(&priv->acpi_lock); priv->pci_dev = dev; priv->features = id->driver_data;
I2C core ensures in i2c_smbus_xfer() that the I2C bus lock is held when calling the smbus_xfer callback. That's i801_access() in our case. I think it's safe in general to assume that the I2C bus lock is held when the smbus_xfer callback is called. Therefore I see no need to define an own mutex. Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com> --- drivers/i2c/busses/i2c-i801.c | 14 ++++---------- 1 file changed, 4 insertions(+), 10 deletions(-)