[PATCH 0/4] i2c: i801: next set of improvements

[PATCH 0/4] i2c: i801: next set of improvements

[Heiner Kallweit]

Next set of improvements.

Heiner Kallweit (4):
  i2c: i801: Use i2c_mark_adapter_suspended/resumed
  i2c: i801: Replace acpi_lock with I2C bus lock
  i2c: i801: Improve i801_block_transaction_byte_by_byte
  i2c: i801: Switch to new macro DEFINE_SIMPLE_DEV_PM_OPS

 drivers/i2c/busses/i2c-i801.c | 32 +++++++++++++++-----------------
 1 file changed, 15 insertions(+), 17 deletions(-)

-- 
2.39.2

[PATCH 1/4] i2c: i801: Use i2c_mark_adapter_suspended/resumed

[Heiner Kallweit]

When entering the shutdown/remove/suspend callbacks, at first we should
ensure that transfers are finished and I2C core can't start further
transfers. Use i2c_mark_adapter_suspended() for this purpose.

Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
 drivers/i2c/busses/i2c-i801.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c
index ac5326747..d6a0c3b53 100644
--- a/drivers/i2c/busses/i2c-i801.c
+++ b/drivers/i2c/busses/i2c-i801.c
@@ -1773,6 +1773,8 @@ static void i801_remove(struct pci_dev *dev)
 {
 	struct i801_priv *priv = pci_get_drvdata(dev);
 
+	i2c_mark_adapter_suspended(&priv->adapter);
+
 	outb_p(priv->original_hstcnt, SMBHSTCNT(priv));
 	i801_disable_host_notify(priv);
 	i801_del_mux(priv);
@@ -1796,6 +1798,8 @@ static void i801_shutdown(struct pci_dev *dev)
 {
 	struct i801_priv *priv = pci_get_drvdata(dev);
 
+	i2c_mark_adapter_suspended(&priv->adapter);
+
 	/* Restore config registers to avoid hard hang on some systems */
 	outb_p(priv->original_hstcnt, SMBHSTCNT(priv));
 	i801_disable_host_notify(priv);
@@ -1807,6 +1811,7 @@ static int i801_suspend(struct device *dev)
 {
 	struct i801_priv *priv = dev_get_drvdata(dev);
 
+	i2c_mark_adapter_suspended(&priv->adapter);
 	outb_p(priv->original_hstcnt, SMBHSTCNT(priv));
 	pci_write_config_byte(priv->pci_dev, SMBHSTCFG, priv->original_hstcfg);
 	return 0;
@@ -1818,6 +1823,7 @@ static int i801_resume(struct device *dev)
 
 	i801_setup_hstcfg(priv);
 	i801_enable_host_notify(&priv->adapter);
+	i2c_mark_adapter_resumed(&priv->adapter);
 
 	return 0;
 }
-- 
2.39.2

[PATCH 2/4] i2c: i801: Replace acpi_lock with I2C bus lock

[Heiner Kallweit]

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;
-- 
2.39.2

[PATCH 3/4] i2c: i801: Improve i801_block_transaction_byte_by_byte

[Heiner Kallweit]

Here we don't have to write SMBHSTCNT in each iteration of the loop.
Bit SMBHSTCNT_START is internally cleared immediately, therefore
we don't have to touch the value of SMBHSTCNT until the last byte.

Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
 drivers/i2c/busses/i2c-i801.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c
index 7641bd0ac..e1350a8cc 100644
--- a/drivers/i2c/busses/i2c-i801.c
+++ b/drivers/i2c/busses/i2c-i801.c
@@ -677,11 +677,11 @@ static int i801_block_transaction_byte_by_byte(struct i801_priv *priv,
 	for (i = 1; i <= len; i++) {
 		if (i == len && read_write == I2C_SMBUS_READ)
 			smbcmd |= SMBHSTCNT_LAST_BYTE;
-		outb_p(smbcmd, SMBHSTCNT(priv));
 
 		if (i == 1)
-			outb_p(inb(SMBHSTCNT(priv)) | SMBHSTCNT_START,
-			       SMBHSTCNT(priv));
+			outb_p(smbcmd | SMBHSTCNT_START, SMBHSTCNT(priv));
+		else if (smbcmd & SMBHSTCNT_LAST_BYTE)
+			outb_p(smbcmd, SMBHSTCNT(priv));
 
 		status = i801_wait_byte_done(priv);
 		if (status)
-- 
2.39.2

[PATCH 4/4] i2c: i801: Switch to new macro DEFINE_SIMPLE_DEV_PM_OPS

[Heiner Kallweit]

By using the newer macro DEFINE_SIMPLE_DEV_PM_OPS we can get rid
of the conditional compiling.

Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
 drivers/i2c/busses/i2c-i801.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c
index e1350a8cc..bd2349768 100644
--- a/drivers/i2c/busses/i2c-i801.c
+++ b/drivers/i2c/busses/i2c-i801.c
@@ -1800,7 +1800,6 @@ static void i801_shutdown(struct pci_dev *dev)
 	pci_write_config_byte(dev, SMBHSTCFG, priv->original_hstcfg);
 }
 
-#ifdef CONFIG_PM_SLEEP
 static int i801_suspend(struct device *dev)
 {
 	struct i801_priv *priv = dev_get_drvdata(dev);
@@ -1821,9 +1820,8 @@ static int i801_resume(struct device *dev)
 
 	return 0;
 }
-#endif
 
-static SIMPLE_DEV_PM_OPS(i801_pm_ops, i801_suspend, i801_resume);
+static DEFINE_SIMPLE_DEV_PM_OPS(i801_pm_ops, i801_suspend, i801_resume);
 
 static struct pci_driver i801_driver = {
 	.name		= DRV_NAME,
@@ -1832,7 +1830,7 @@ static struct pci_driver i801_driver = {
 	.remove		= i801_remove,
 	.shutdown	= i801_shutdown,
 	.driver		= {
-		.pm	= &i801_pm_ops,
+		.pm	= pm_sleep_ptr(&i801_pm_ops),
 		.probe_type = PROBE_PREFER_ASYNCHRONOUS,
 	},
 };
-- 
2.39.2

Re: [PATCH 0/4] i2c: i801: next set of improvements

[Heiner Kallweit]

On 04.03.2023 22:30, Heiner Kallweit wrote:
> Next set of improvements.
> 
> Heiner Kallweit (4):
>   i2c: i801: Use i2c_mark_adapter_suspended/resumed
>   i2c: i801: Replace acpi_lock with I2C bus lock
>   i2c: i801: Improve i801_block_transaction_byte_by_byte
>   i2c: i801: Switch to new macro DEFINE_SIMPLE_DEV_PM_OPS
> 
>  drivers/i2c/busses/i2c-i801.c | 32 +++++++++++++++-----------------
>  1 file changed, 15 insertions(+), 17 deletions(-)
> 
Jean, any chance to have a look at it?

Re: [PATCH 1/4] i2c: i801: Use i2c_mark_adapter_suspended/resumed

[Andi Shyti]

Hi Heiner,

On Sat, Mar 04, 2023 at 10:31:23PM +0100, Heiner Kallweit wrote:
> When entering the shutdown/remove/suspend callbacks, at first we should
> ensure that transfers are finished and I2C core can't start further
> transfers. Use i2c_mark_adapter_suspended() for this purpose.
> 
> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
> ---
>  drivers/i2c/busses/i2c-i801.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c
> index ac5326747..d6a0c3b53 100644
> --- a/drivers/i2c/busses/i2c-i801.c
> +++ b/drivers/i2c/busses/i2c-i801.c
> @@ -1773,6 +1773,8 @@ static void i801_remove(struct pci_dev *dev)
>  {
>  	struct i801_priv *priv = pci_get_drvdata(dev);
>  
> +	i2c_mark_adapter_suspended(&priv->adapter);
> +
>  	outb_p(priv->original_hstcnt, SMBHSTCNT(priv));
>  	i801_disable_host_notify(priv);
>  	i801_del_mux(priv);
> @@ -1796,6 +1798,8 @@ static void i801_shutdown(struct pci_dev *dev)
>  {
>  	struct i801_priv *priv = pci_get_drvdata(dev);
>  
> +	i2c_mark_adapter_suspended(&priv->adapter);
> +

is this really needed in the shutdown and remove function?

I'm OK with it, though.

>  	/* Restore config registers to avoid hard hang on some systems */
>  	outb_p(priv->original_hstcnt, SMBHSTCNT(priv));
>  	i801_disable_host_notify(priv);
> @@ -1807,6 +1811,7 @@ static int i801_suspend(struct device *dev)
>  {
>  	struct i801_priv *priv = dev_get_drvdata(dev);
>  
> +	i2c_mark_adapter_suspended(&priv->adapter);
>  	outb_p(priv->original_hstcnt, SMBHSTCNT(priv));
>  	pci_write_config_byte(priv->pci_dev, SMBHSTCFG, priv->original_hstcfg);
>  	return 0;
> @@ -1818,6 +1823,7 @@ static int i801_resume(struct device *dev)
>  
>  	i801_setup_hstcfg(priv);
>  	i801_enable_host_notify(&priv->adapter);
> +	i2c_mark_adapter_resumed(&priv->adapter);

BTW, I see that very few drivers are using suspended and resumed
and I wonder why. Should these perhaps be added in the basic pm
functions?

I'm OK to r-b this, but i want first Jean to give it an ack.

Andi

Re: [PATCH 2/4] i2c: i801: Replace acpi_lock with I2C bus lock

[Andi Shyti]

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.

I think it makes sense... unless I missed something I don't see
anything else being racy in i801_access().

Have you checked i801_acpi_io_handler()?

Andi

> 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;
> -- 
> 2.39.2
> 
> 

Re: [PATCH 3/4] i2c: i801: Improve i801_block_transaction_byte_by_byte

[Andi Shyti]

Hi Heiner,

On Sat, Mar 04, 2023 at 10:36:34PM +0100, Heiner Kallweit wrote:
> Here we don't have to write SMBHSTCNT in each iteration of the loop.
> Bit SMBHSTCNT_START is internally cleared immediately, therefore
> we don't have to touch the value of SMBHSTCNT until the last byte.
> 
> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
> ---
>  drivers/i2c/busses/i2c-i801.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c
> index 7641bd0ac..e1350a8cc 100644
> --- a/drivers/i2c/busses/i2c-i801.c
> +++ b/drivers/i2c/busses/i2c-i801.c
> @@ -677,11 +677,11 @@ static int i801_block_transaction_byte_by_byte(struct i801_priv *priv,
>  	for (i = 1; i <= len; i++) {
>  		if (i == len && read_write == I2C_SMBUS_READ)
>  			smbcmd |= SMBHSTCNT_LAST_BYTE;
> -		outb_p(smbcmd, SMBHSTCNT(priv));
>  
>  		if (i == 1)
> -			outb_p(inb(SMBHSTCNT(priv)) | SMBHSTCNT_START,
> -			       SMBHSTCNT(priv));
> +			outb_p(smbcmd | SMBHSTCNT_START, SMBHSTCNT(priv));
> +		else if (smbcmd & SMBHSTCNT_LAST_BYTE)
> +			outb_p(smbcmd, SMBHSTCNT(priv));

Looks reasonable to me... Jean, any opinion here?

Andi

Re: [PATCH 4/4] i2c: i801: Switch to new macro DEFINE_SIMPLE_DEV_PM_OPS

[Andi Shyti]

Hi Heiner,

On Sat, Mar 04, 2023 at 10:37:34PM +0100, Heiner Kallweit wrote:
> By using the newer macro DEFINE_SIMPLE_DEV_PM_OPS we can get rid
> of the conditional compiling.
> 
> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
> ---
>  drivers/i2c/busses/i2c-i801.c | 6 ++----
>  1 file changed, 2 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c
> index e1350a8cc..bd2349768 100644
> --- a/drivers/i2c/busses/i2c-i801.c
> +++ b/drivers/i2c/busses/i2c-i801.c
> @@ -1800,7 +1800,6 @@ static void i801_shutdown(struct pci_dev *dev)
>  	pci_write_config_byte(dev, SMBHSTCFG, priv->original_hstcfg);
>  }
>  
> -#ifdef CONFIG_PM_SLEEP
>  static int i801_suspend(struct device *dev)
>  {
>  	struct i801_priv *priv = dev_get_drvdata(dev);
> @@ -1821,9 +1820,8 @@ static int i801_resume(struct device *dev)
>  
>  	return 0;
>  }
> -#endif
>  
> -static SIMPLE_DEV_PM_OPS(i801_pm_ops, i801_suspend, i801_resume);
> +static DEFINE_SIMPLE_DEV_PM_OPS(i801_pm_ops, i801_suspend, i801_resume);
>  
>  static struct pci_driver i801_driver = {
>  	.name		= DRV_NAME,
> @@ -1832,7 +1830,7 @@ static struct pci_driver i801_driver = {
>  	.remove		= i801_remove,
>  	.shutdown	= i801_shutdown,
>  	.driver		= {
> -		.pm	= &i801_pm_ops,
> +		.pm	= pm_sleep_ptr(&i801_pm_ops),

Reviewed-by: Andi Shyti <andi.shyti@kernel.org> 

Andi

Re: [PATCH 1/4] i2c: i801: Use i2c_mark_adapter_suspended/resumed

[Heiner Kallweit]

On 15.06.2023 00:24, Andi Shyti wrote:
> Hi Heiner,
> 
> On Sat, Mar 04, 2023 at 10:31:23PM +0100, Heiner Kallweit wrote:
>> When entering the shutdown/remove/suspend callbacks, at first we should
>> ensure that transfers are finished and I2C core can't start further
>> transfers. Use i2c_mark_adapter_suspended() for this purpose.
>>
>> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
>> ---
>>  drivers/i2c/busses/i2c-i801.c | 6 ++++++
>>  1 file changed, 6 insertions(+)
>>
>> diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c
>> index ac5326747..d6a0c3b53 100644
>> --- a/drivers/i2c/busses/i2c-i801.c
>> +++ b/drivers/i2c/busses/i2c-i801.c
>> @@ -1773,6 +1773,8 @@ static void i801_remove(struct pci_dev *dev)
>>  {
>>  	struct i801_priv *priv = pci_get_drvdata(dev);
>>  
>> +	i2c_mark_adapter_suspended(&priv->adapter);
>> +
>>  	outb_p(priv->original_hstcnt, SMBHSTCNT(priv));
>>  	i801_disable_host_notify(priv);
>>  	i801_del_mux(priv);
>> @@ -1796,6 +1798,8 @@ static void i801_shutdown(struct pci_dev *dev)
>>  {
>>  	struct i801_priv *priv = pci_get_drvdata(dev);
>>  
>> +	i2c_mark_adapter_suspended(&priv->adapter);
>> +
> 
> is this really needed in the shutdown and remove function?
> 
I think yes. Otherwise we may interrupt an active transfer, or a user
may start a transfer whilst we are in cleanup.
Note: i2c_mark_adapter_suspended() takes the I2C bus lock, therefore it
will sleep until an active transfer is finished.

> I'm OK with it, though.
> 
>>  	/* Restore config registers to avoid hard hang on some systems */
>>  	outb_p(priv->original_hstcnt, SMBHSTCNT(priv));
>>  	i801_disable_host_notify(priv);
>> @@ -1807,6 +1811,7 @@ static int i801_suspend(struct device *dev)
>>  {
>>  	struct i801_priv *priv = dev_get_drvdata(dev);
>>  
>> +	i2c_mark_adapter_suspended(&priv->adapter);
>>  	outb_p(priv->original_hstcnt, SMBHSTCNT(priv));
>>  	pci_write_config_byte(priv->pci_dev, SMBHSTCFG, priv->original_hstcfg);
>>  	return 0;
>> @@ -1818,6 +1823,7 @@ static int i801_resume(struct device *dev)
>>  
>>  	i801_setup_hstcfg(priv);
>>  	i801_enable_host_notify(&priv->adapter);
>> +	i2c_mark_adapter_resumed(&priv->adapter);
> 
> BTW, I see that very few drivers are using suspended and resumed
> and I wonder why. Should these perhaps be added in the basic pm
> functions?
> 
For my understanding, which functions are you referring to?

> I'm OK to r-b this, but i want first Jean to give it an ack.
> 
> Andi

Re: [PATCH 2/4] i2c: i801: Replace acpi_lock with I2C bus lock

[Heiner Kallweit]

On 15.06.2023 00:31, Andi Shyti wrote:
> 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.
> 
> I think it makes sense... unless I missed something I don't see
> anything else being racy in i801_access().
> 
> Have you checked i801_acpi_io_handler()?
> 
acpi_os_read_port() resolves to a simple inb() et al.
Therefore I don't see anything in i801_acpi_io_handler()
that would speak against using the I2C bus lock.

> Andi
> 
>> 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;
>> -- 
>> 2.39.2
>>
>>

Re: [PATCH 1/4] i2c: i801: Use i2c_mark_adapter_suspended/resumed

[Andi Shyti]

Hi Heiner,

On Thu, Jun 15, 2023 at 11:17:12PM +0200, Heiner Kallweit wrote:
> On 15.06.2023 00:24, Andi Shyti wrote:
> > Hi Heiner,
> > 
> > On Sat, Mar 04, 2023 at 10:31:23PM +0100, Heiner Kallweit wrote:
> >> When entering the shutdown/remove/suspend callbacks, at first we should
> >> ensure that transfers are finished and I2C core can't start further
> >> transfers. Use i2c_mark_adapter_suspended() for this purpose.
> >>
> >> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
> >> ---
> >>  drivers/i2c/busses/i2c-i801.c | 6 ++++++
> >>  1 file changed, 6 insertions(+)
> >>
> >> diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c
> >> index ac5326747..d6a0c3b53 100644
> >> --- a/drivers/i2c/busses/i2c-i801.c
> >> +++ b/drivers/i2c/busses/i2c-i801.c
> >> @@ -1773,6 +1773,8 @@ static void i801_remove(struct pci_dev *dev)
> >>  {
> >>  	struct i801_priv *priv = pci_get_drvdata(dev);
> >>  
> >> +	i2c_mark_adapter_suspended(&priv->adapter);
> >> +
> >>  	outb_p(priv->original_hstcnt, SMBHSTCNT(priv));
> >>  	i801_disable_host_notify(priv);
> >>  	i801_del_mux(priv);
> >> @@ -1796,6 +1798,8 @@ static void i801_shutdown(struct pci_dev *dev)
> >>  {
> >>  	struct i801_priv *priv = pci_get_drvdata(dev);
> >>  
> >> +	i2c_mark_adapter_suspended(&priv->adapter);
> >> +
> > 
> > is this really needed in the shutdown and remove function?
> > 
> I think yes. Otherwise we may interrupt an active transfer, or a user
> may start a transfer whilst we are in cleanup.
> Note: i2c_mark_adapter_suspended() takes the I2C bus lock, therefore it
> will sleep until an active transfer is finished.

yes, I think you are right.

> > I'm OK with it, though.
> > 
> >>  	/* Restore config registers to avoid hard hang on some systems */
> >>  	outb_p(priv->original_hstcnt, SMBHSTCNT(priv));
> >>  	i801_disable_host_notify(priv);
> >> @@ -1807,6 +1811,7 @@ static int i801_suspend(struct device *dev)
> >>  {
> >>  	struct i801_priv *priv = dev_get_drvdata(dev);
> >>  
> >> +	i2c_mark_adapter_suspended(&priv->adapter);
> >>  	outb_p(priv->original_hstcnt, SMBHSTCNT(priv));
> >>  	pci_write_config_byte(priv->pci_dev, SMBHSTCFG, priv->original_hstcfg);
> >>  	return 0;
> >> @@ -1818,6 +1823,7 @@ static int i801_resume(struct device *dev)
> >>  
> >>  	i801_setup_hstcfg(priv);
> >>  	i801_enable_host_notify(&priv->adapter);
> >> +	i2c_mark_adapter_resumed(&priv->adapter);
> > 
> > BTW, I see that very few drivers are using suspended and resumed
> > and I wonder why. Should these perhaps be added in the basic pm
> > functions?
> > 
> For my understanding, which functions are you referring to?

I am referring about having a more generalised pm function which
can mark the i2c adapter supsended or resumed even before or
after the driver specific functions are called.

This way all drivers can benefit from it.

In any case this out of the scope of this patch.

I'm going to give my approval, if then Jean has something to say,
I guess there is still time to chime in.

Reviewed-by: Andi Shyti <andi.shyti@kernel.org> 

Thank you,
Andi

> > I'm OK to r-b this, but i want first Jean to give it an ack.
> > 
> > Andi
> 

Re: [PATCH 3/4] i2c: i801: Improve i801_block_transaction_byte_by_byte

[Andi Shyti]

Hi Heiner,

On Sat, Mar 04, 2023 at 10:36:34PM +0100, Heiner Kallweit wrote:
> Here we don't have to write SMBHSTCNT in each iteration of the loop.
> Bit SMBHSTCNT_START is internally cleared immediately, therefore
> we don't have to touch the value of SMBHSTCNT until the last byte.
> 
> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>

Acked-by: Andi Shyti <andi.shyti@kernel.org> 

Andi

Re: [PATCH 2/4] i2c: i801: Replace acpi_lock with I2C bus lock

[Andi Shyti]

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

Re: [PATCH 1/4] i2c: i801: Use i2c_mark_adapter_suspended/resumed

[Heiner Kallweit]

On 15.06.2023 23:45, Andi Shyti wrote:
> Hi Heiner,
> 
> On Thu, Jun 15, 2023 at 11:17:12PM +0200, Heiner Kallweit wrote:
>> On 15.06.2023 00:24, Andi Shyti wrote:
>>> Hi Heiner,
>>>
>>> On Sat, Mar 04, 2023 at 10:31:23PM +0100, Heiner Kallweit wrote:
>>>> When entering the shutdown/remove/suspend callbacks, at first we should
>>>> ensure that transfers are finished and I2C core can't start further
>>>> transfers. Use i2c_mark_adapter_suspended() for this purpose.
>>>>
>>>> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
>>>> ---
>>>>  drivers/i2c/busses/i2c-i801.c | 6 ++++++
>>>>  1 file changed, 6 insertions(+)
>>>>
>>>> diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c
>>>> index ac5326747..d6a0c3b53 100644
>>>> --- a/drivers/i2c/busses/i2c-i801.c
>>>> +++ b/drivers/i2c/busses/i2c-i801.c
>>>> @@ -1773,6 +1773,8 @@ static void i801_remove(struct pci_dev *dev)
>>>>  {
>>>>  	struct i801_priv *priv = pci_get_drvdata(dev);
>>>>  
>>>> +	i2c_mark_adapter_suspended(&priv->adapter);
>>>> +
>>>>  	outb_p(priv->original_hstcnt, SMBHSTCNT(priv));
>>>>  	i801_disable_host_notify(priv);
>>>>  	i801_del_mux(priv);
>>>> @@ -1796,6 +1798,8 @@ static void i801_shutdown(struct pci_dev *dev)
>>>>  {
>>>>  	struct i801_priv *priv = pci_get_drvdata(dev);
>>>>  
>>>> +	i2c_mark_adapter_suspended(&priv->adapter);
>>>> +
>>>
>>> is this really needed in the shutdown and remove function?
>>>
>> I think yes. Otherwise we may interrupt an active transfer, or a user
>> may start a transfer whilst we are in cleanup.
>> Note: i2c_mark_adapter_suspended() takes the I2C bus lock, therefore it
>> will sleep until an active transfer is finished.
> 
> yes, I think you are right.
> 
>>> I'm OK with it, though.
>>>
>>>>  	/* Restore config registers to avoid hard hang on some systems */
>>>>  	outb_p(priv->original_hstcnt, SMBHSTCNT(priv));
>>>>  	i801_disable_host_notify(priv);
>>>> @@ -1807,6 +1811,7 @@ static int i801_suspend(struct device *dev)
>>>>  {
>>>>  	struct i801_priv *priv = dev_get_drvdata(dev);
>>>>  
>>>> +	i2c_mark_adapter_suspended(&priv->adapter);
>>>>  	outb_p(priv->original_hstcnt, SMBHSTCNT(priv));
>>>>  	pci_write_config_byte(priv->pci_dev, SMBHSTCFG, priv->original_hstcfg);
>>>>  	return 0;
>>>> @@ -1818,6 +1823,7 @@ static int i801_resume(struct device *dev)
>>>>  
>>>>  	i801_setup_hstcfg(priv);
>>>>  	i801_enable_host_notify(&priv->adapter);
>>>> +	i2c_mark_adapter_resumed(&priv->adapter);
>>>
>>> BTW, I see that very few drivers are using suspended and resumed
>>> and I wonder why. Should these perhaps be added in the basic pm
>>> functions?
>>>
>> For my understanding, which functions are you referring to?
> 
> I am referring about having a more generalised pm function which
> can mark the i2c adapter supsended or resumed even before or
> after the driver specific functions are called.
> 
In case of i801 this bus driver is a pci_driver and the remove callback
is handled by the PCI core (pci_device_remove()), I2C core isn't involved.
At a first glance I fail to see how we could inject I2C-specific code.

> This way all drivers can benefit from it.
> 
> In any case this out of the scope of this patch.
> 
> I'm going to give my approval, if then Jean has something to say,
> I guess there is still time to chime in.
> 
> Reviewed-by: Andi Shyti <andi.shyti@kernel.org> 
> 
> Thank you,
> Andi
> 
>>> I'm OK to r-b this, but i want first Jean to give it an ack.
>>>
>>> Andi
>>

Re: [PATCH 1/4] i2c: i801: Use i2c_mark_adapter_suspended/resumed

[Jean Delvare]

Hi Andi, Heiner,

Adding Wolfram Sang who introduced the i2c_mark_adapter_suspended() API
originally.

On Thu, 15 Jun 2023 00:24:39 +0200, Andi Shyti wrote:
> On Sat, Mar 04, 2023 at 10:31:23PM +0100, Heiner Kallweit wrote:
> > When entering the shutdown/remove/suspend callbacks, at first we should
> > ensure that transfers are finished and I2C core can't start further
> > transfers. Use i2c_mark_adapter_suspended() for this purpose.
> > 
> > Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
> > ---
> >  drivers/i2c/busses/i2c-i801.c | 6 ++++++
> >  1 file changed, 6 insertions(+)
> > 
> > diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c
> > index ac5326747..d6a0c3b53 100644
> > --- a/drivers/i2c/busses/i2c-i801.c
> > +++ b/drivers/i2c/busses/i2c-i801.c
> > @@ -1773,6 +1773,8 @@ static void i801_remove(struct pci_dev *dev)
> >  {
> >  	struct i801_priv *priv = pci_get_drvdata(dev);
> >  
> > +	i2c_mark_adapter_suspended(&priv->adapter);
> > +
> >  	outb_p(priv->original_hstcnt, SMBHSTCNT(priv));
> >  	i801_disable_host_notify(priv);
> >  	i801_del_mux(priv);
> > @@ -1796,6 +1798,8 @@ static void i801_shutdown(struct pci_dev *dev)
> >  {
> >  	struct i801_priv *priv = pci_get_drvdata(dev);
> >  
> > +	i2c_mark_adapter_suspended(&priv->adapter);
> > +  
> 
> is this really needed in the shutdown and remove function?

The very same question came to my mind. I would really expect the
driver core to do all the reference counting needed so that a device
can't possibly be removed when any of its children is still active. If
that's not the case then something is very wrong in the device driver
model itself, and I certainly hope that the proper fix wouldn't be
subsystem-specific and implemented in every device driver separately.

FWIW, I see 13 I2C bus drivers calling i2c_mark_adapter_suspended() at
the moment, and only one of them is calling it in shutdown
(i2c-qcom-geni). None of them is calling it in remove. If that's not
needed for other drivers then I can't see why that would be needed for
i2c-i801.

As far as the remove() path is concerned, my expectation is that if
everything is undone in the opposite way of the probe() path then
everything should be fine. It turns out this is not the case of the
current i2c-i801 driver. The original HSTCNT register value is being
restored too early in i801_remove(). I'm to blame for this, the bug was
introduced by commit 9b5bf5878138 ("i2c: i801: Restore INTREN on
unload") which is mine. This should be fixed separately before any
other change.

Once this is fixed, unless you are able to actually trigger a bug in
the remove() path, then I see no good reason to add
i2c_mark_adapter_suspended() to that code path.

For shutdown, I'm unsure. Wolfram, what's your take?

Thanks,
-- 
Jean Delvare
SUSE L3 Support

Re: [PATCH 2/4] i2c: i801: Replace acpi_lock with I2C bus lock

[Jean Delvare]

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,
-- 
Jean Delvare
SUSE L3 Support

Re: [PATCH 3/4] i2c: i801: Improve i801_block_transaction_byte_by_byte

[Jean Delvare]

Hi Heiner, Andi,

On Sat, 04 Mar 2023 22:36:34 +0100, Heiner Kallweit wrote:
> Here we don't have to write SMBHSTCNT in each iteration of the loop.
> Bit SMBHSTCNT_START is internally cleared immediately, therefore
> we don't have to touch the value of SMBHSTCNT until the last byte.
> 
> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
> ---
>  drivers/i2c/busses/i2c-i801.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c
> index 7641bd0ac..e1350a8cc 100644
> --- a/drivers/i2c/busses/i2c-i801.c
> +++ b/drivers/i2c/busses/i2c-i801.c
> @@ -677,11 +677,11 @@ static int i801_block_transaction_byte_by_byte(struct i801_priv *priv,
>  	for (i = 1; i <= len; i++) {
>  		if (i == len && read_write == I2C_SMBUS_READ)
>  			smbcmd |= SMBHSTCNT_LAST_BYTE;
> -		outb_p(smbcmd, SMBHSTCNT(priv));
>  
>  		if (i == 1)
> -			outb_p(inb(SMBHSTCNT(priv)) | SMBHSTCNT_START,
> -			       SMBHSTCNT(priv));
> +			outb_p(smbcmd | SMBHSTCNT_START, SMBHSTCNT(priv));
> +		else if (smbcmd & SMBHSTCNT_LAST_BYTE)
> +			outb_p(smbcmd, SMBHSTCNT(priv));
>  
>  		status = i801_wait_byte_done(priv);
>  		if (status)

I tested this and it works, but I don't understand how.

I thought that writing to SMBHSTCNT was what was telling the host
controller to proceed with the next byte. If writing to SMBHSTCNT for
each byte isn't needed, then what causes the next byte to be processed?
Does this happen as soon as SMBHSTSTS_BYTE_DONE is written? If so, then
what guarantees that we set SMBHSTCNT_LAST_BYTE *before* the last byte
is actually processed?

-- 
Jean Delvare
SUSE L3 Support

Re: [PATCH 4/4] i2c: i801: Switch to new macro DEFINE_SIMPLE_DEV_PM_OPS

[Jean Delvare]

On Sat, 04 Mar 2023 22:37:34 +0100, Heiner Kallweit wrote:
> By using the newer macro DEFINE_SIMPLE_DEV_PM_OPS we can get rid
> of the conditional compiling.
> 
> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
> ---
>  drivers/i2c/busses/i2c-i801.c | 6 ++----
>  1 file changed, 2 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c
> index e1350a8cc..bd2349768 100644
> --- a/drivers/i2c/busses/i2c-i801.c
> +++ b/drivers/i2c/busses/i2c-i801.c
> @@ -1800,7 +1800,6 @@ static void i801_shutdown(struct pci_dev *dev)
>  	pci_write_config_byte(dev, SMBHSTCFG, priv->original_hstcfg);
>  }
>  
> -#ifdef CONFIG_PM_SLEEP
>  static int i801_suspend(struct device *dev)
>  {
>  	struct i801_priv *priv = dev_get_drvdata(dev);
> @@ -1821,9 +1820,8 @@ static int i801_resume(struct device *dev)
>  
>  	return 0;
>  }
> -#endif
>  
> -static SIMPLE_DEV_PM_OPS(i801_pm_ops, i801_suspend, i801_resume);
> +static DEFINE_SIMPLE_DEV_PM_OPS(i801_pm_ops, i801_suspend, i801_resume);
>  
>  static struct pci_driver i801_driver = {
>  	.name		= DRV_NAME,
> @@ -1832,7 +1830,7 @@ static struct pci_driver i801_driver = {
>  	.remove		= i801_remove,
>  	.shutdown	= i801_shutdown,
>  	.driver		= {
> -		.pm	= &i801_pm_ops,
> +		.pm	= pm_sleep_ptr(&i801_pm_ops),
>  		.probe_type = PROBE_PREFER_ASYNCHRONOUS,
>  	},
>  };


Nice clean up.

Reviewed-by: Jean Delvare <jdelvare@suse.de>

-- 
Jean Delvare
SUSE L3 Support