[PATCH 0/5] i2c: Host Notify / i801 fixes

[PATCH 0/5] i2c: Host Notify / i801 fixes

[Benjamin Tissoires]

Hi,

this series comes to fix most of the remarks from Jean after reviewing my
Host Notify series.

I couldn't find a better way for 1/5 to automatically kill the workqueue, so
I am open to any suggestions.
Regarding 3/5 (use of BIT), I am pretty sure I did not mess up, but I'd be glad
to have an extra eye on the various hex to BIT changes.

Cheers,
Benjamin

Benjamin Tissoires (5):
  i2c: i2c-smbus: prevent races on remove when Host Notify is used
  i2c: i801: minor formatting issues
  i2c: i801: use BIT() macro for bits definition
  i2c: i801: do not report an error if FEATURE_HOST_NOTIFY is not set
  i2c: i801: warn on i2c_handle_smbus_host_notify() errors

 drivers/i2c/busses/i2c-i801.c | 93 ++++++++++++++++++++++++++-----------------
 drivers/i2c/i2c-smbus.c       | 18 +++++++++
 include/linux/i2c-smbus.h     |  1 +
 3 files changed, 76 insertions(+), 36 deletions(-)

-- 
2.5.5

[PATCH 1/5] i2c: i2c-smbus: prevent races on remove when Host Notify is used

[Benjamin Tissoires]

struct host_notify contains its own workqueue, so there is a race when
the adapter gets removed:
- the adapter schedules a notification
- the notification is on hold
- the adapter gets removed and all its children too
- the worker fires and access illegal memory

Add an API to actually kill the workqueue and prevent it to access such
illegal memory. I couldn't find a reliable way of automatically calling
this, so it's the responsibility of the adapter driver to clean up after
itself.

Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
---
 drivers/i2c/busses/i2c-i801.c | 14 ++++++++++++++
 drivers/i2c/i2c-smbus.c       | 18 ++++++++++++++++++
 include/linux/i2c-smbus.h     |  1 +
 3 files changed, 33 insertions(+)

diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c
index 5ef9b73..cde9a7c 100644
--- a/drivers/i2c/busses/i2c-i801.c
+++ b/drivers/i2c/busses/i2c-i801.c
@@ -959,6 +959,19 @@ static int i801_enable_host_notify(struct i2c_adapter *adapter)
 	return 0;
 }
 
+static void i801_disable_host_notify(struct i2c_adapter *adapter)
+{
+	struct i801_priv *priv = i2c_get_adapdata(adapter);
+
+	if (!(priv->features & FEATURE_HOST_NOTIFY))
+		return;
+
+	/* disable Host Notify... */
+	outb_p(0, SMBSLVCMD(priv));
+	/* ...and kill the already queued notifications */
+	i2c_cancel_smbus_host_notify(priv->host_notify);
+}
+
 static const struct i2c_algorithm smbus_algorithm = {
 	.smbus_xfer	= i801_access,
 	.functionality	= i801_func,
@@ -1648,6 +1661,7 @@ static void i801_remove(struct pci_dev *dev)
 	pm_runtime_forbid(&dev->dev);
 	pm_runtime_get_noresume(&dev->dev);
 
+	i801_disable_host_notify(&priv->adapter);
 	i801_del_mux(priv);
 	i2c_del_adapter(&priv->adapter);
 	i801_acpi_remove(priv);
diff --git a/drivers/i2c/i2c-smbus.c b/drivers/i2c/i2c-smbus.c
index b0d2679..60705dd 100644
--- a/drivers/i2c/i2c-smbus.c
+++ b/drivers/i2c/i2c-smbus.c
@@ -279,6 +279,8 @@ static void smbus_host_notify_work(struct work_struct *work)
  * Returns a struct smbus_host_notify pointer on success, and NULL on failure.
  * The resulting smbus_host_notify must not be freed afterwards, it is a
  * managed resource already.
+ * To prevent races on remove, the caller needs to stop the embedded worker
+ * by calling i2c_cancel_smbus_host_notify().
  */
 struct smbus_host_notify *i2c_setup_smbus_host_notify(struct i2c_adapter *adap)
 {
@@ -299,6 +301,22 @@ struct smbus_host_notify *i2c_setup_smbus_host_notify(struct i2c_adapter *adap)
 EXPORT_SYMBOL_GPL(i2c_setup_smbus_host_notify);
 
 /**
+ * i2c_cancel_smbus_host_notify - Terminate any active Host Notification.
+ * @host_notify: the host_notify object to terminate
+ *
+ * Cancel any pending Host Notifcation. Must be called to ensure no races
+ * between the adaptor being removed and the Host Notify process being treated.
+ */
+void i2c_cancel_smbus_host_notify(struct smbus_host_notify *host_notify)
+{
+	if (!host_notify)
+		return;
+
+	cancel_work_sync(&host_notify->work);
+}
+EXPORT_SYMBOL_GPL(i2c_cancel_smbus_host_notify);
+
+/**
  * i2c_handle_smbus_host_notify - Forward a Host Notify event to the correct
  * I2C client.
  * @host_notify: the struct host_notify attached to the relevant adapter
diff --git a/include/linux/i2c-smbus.h b/include/linux/i2c-smbus.h
index c2e3324..ac02827 100644
--- a/include/linux/i2c-smbus.h
+++ b/include/linux/i2c-smbus.h
@@ -76,5 +76,6 @@ struct smbus_host_notify {
 struct smbus_host_notify *i2c_setup_smbus_host_notify(struct i2c_adapter *adap);
 int i2c_handle_smbus_host_notify(struct smbus_host_notify *host_notify,
 				 unsigned short addr, unsigned int data);
+void i2c_cancel_smbus_host_notify(struct smbus_host_notify *host_notify);
 
 #endif /* _LINUX_I2C_SMBUS_H */
-- 
2.5.5

[PATCH 2/5] i2c: i801: minor formatting issues

[Benjamin Tissoires]

No functional changes, just typos and remove unused #define.

Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.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 cde9a7c..80d67b8 100644
--- a/drivers/i2c/busses/i2c-i801.c
+++ b/drivers/i2c/busses/i2c-i801.c
@@ -186,10 +186,10 @@
 #define SMBHSTSTS_INTR		0x02
 #define SMBHSTSTS_HOST_BUSY	0x01
 
-/* Host Notify Status registers bits */
+/* Host Notify Status register bits */
 #define SMBSLVSTS_HST_NTFY_STS	1
 
-/* Host Notify Command registers bits */
+/* Host Notify Command register bits */
 #define SMBSLVCMD_HST_NTFY_INTREN	0x01
 
 #define STATUS_ERROR_FLAGS	(SMBHSTSTS_FAILED | SMBHSTSTS_BUS_ERR | \
@@ -269,8 +269,6 @@ struct i801_priv {
 	struct smbus_host_notify *host_notify;
 };
 
-#define SMBHSTNTFY_SIZE		8
-
 #define FEATURE_SMBUS_PEC	(1 << 0)
 #define FEATURE_BLOCK_BUFFER	(1 << 1)
 #define FEATURE_BLOCK_PROC	(1 << 2)
-- 
2.5.5

[PATCH 3/5] i2c: i801: use BIT() macro for bits definition

[Benjamin Tissoires]

i801 mixes hexadecimal and decimal values for defining bits. However,
we have a nice BIT() macro for this exact purpose.

No functional changes, cleanup only.

Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
---
 drivers/i2c/busses/i2c-i801.c | 50 +++++++++++++++++++++----------------------
 1 file changed, 25 insertions(+), 25 deletions(-)

diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c
index 80d67b8..3321956 100644
--- a/drivers/i2c/busses/i2c-i801.c
+++ b/drivers/i2c/busses/i2c-i801.c
@@ -136,26 +136,26 @@
 #define SBREG_SMBCTRL		0xc6000c
 
 /* Host status bits for SMBPCISTS */
-#define SMBPCISTS_INTS		0x08
+#define SMBPCISTS_INTS		BIT(3)
 
 /* Control bits for SMBPCICTL */
-#define SMBPCICTL_INTDIS	0x0400
+#define SMBPCICTL_INTDIS	BIT(10)
 
 /* Host configuration bits for SMBHSTCFG */
-#define SMBHSTCFG_HST_EN	1
-#define SMBHSTCFG_SMB_SMI_EN	2
-#define SMBHSTCFG_I2C_EN	4
+#define SMBHSTCFG_HST_EN	BIT(0)
+#define SMBHSTCFG_SMB_SMI_EN	BIT(1)
+#define SMBHSTCFG_I2C_EN	BIT(2)
 
 /* TCO configuration bits for TCOCTL */
-#define TCOCTL_EN		0x0100
+#define TCOCTL_EN		BIT(8)
 
 /* Auxiliary status register bits, ICH4+ only */
-#define SMBAUXSTS_CRCE		1
-#define SMBAUXSTS_STCO		2
+#define SMBAUXSTS_CRCE		BIT(0)
+#define SMBAUXSTS_STCO		BIT(1)
 
 /* Auxiliary control register bits, ICH4+ only */
-#define SMBAUXCTL_CRC		1
-#define SMBAUXCTL_E32B		2
+#define SMBAUXCTL_CRC		BIT(0)
+#define SMBAUXCTL_E32B		BIT(1)
 
 /* Other settings */
 #define MAX_RETRIES		400
@@ -170,27 +170,27 @@
 #define I801_I2C_BLOCK_DATA	0x18	/* ICH5 and later */
 
 /* I801 Host Control register bits */
-#define SMBHSTCNT_INTREN	0x01
-#define SMBHSTCNT_KILL		0x02
-#define SMBHSTCNT_LAST_BYTE	0x20
-#define SMBHSTCNT_START		0x40
-#define SMBHSTCNT_PEC_EN	0x80	/* ICH3 and later */
+#define SMBHSTCNT_INTREN	BIT(0)
+#define SMBHSTCNT_KILL		BIT(1)
+#define SMBHSTCNT_LAST_BYTE	BIT(5)
+#define SMBHSTCNT_START		BIT(6)
+#define SMBHSTCNT_PEC_EN	BIT(7)	/* ICH3 and later */
 
 /* I801 Hosts Status register bits */
-#define SMBHSTSTS_BYTE_DONE	0x80
-#define SMBHSTSTS_INUSE_STS	0x40
-#define SMBHSTSTS_SMBALERT_STS	0x20
-#define SMBHSTSTS_FAILED	0x10
-#define SMBHSTSTS_BUS_ERR	0x08
-#define SMBHSTSTS_DEV_ERR	0x04
-#define SMBHSTSTS_INTR		0x02
-#define SMBHSTSTS_HOST_BUSY	0x01
+#define SMBHSTSTS_BYTE_DONE	BIT(7)
+#define SMBHSTSTS_INUSE_STS	BIT(6)
+#define SMBHSTSTS_SMBALERT_STS	BIT(5)
+#define SMBHSTSTS_FAILED	BIT(4)
+#define SMBHSTSTS_BUS_ERR	BIT(3)
+#define SMBHSTSTS_DEV_ERR	BIT(2)
+#define SMBHSTSTS_INTR		BIT(1)
+#define SMBHSTSTS_HOST_BUSY	BIT(0)
 
 /* Host Notify Status register bits */
-#define SMBSLVSTS_HST_NTFY_STS	1
+#define SMBSLVSTS_HST_NTFY_STS	BIT(0)
 
 /* Host Notify Command register bits */
-#define SMBSLVCMD_HST_NTFY_INTREN	0x01
+#define SMBSLVCMD_HST_NTFY_INTREN	BIT(0)
 
 #define STATUS_ERROR_FLAGS	(SMBHSTSTS_FAILED | SMBHSTSTS_BUS_ERR | \
 				 SMBHSTSTS_DEV_ERR)
-- 
2.5.5

[PATCH 4/5] i2c: i801: do not report an error if FEATURE_HOST_NOTIFY is not set

[Benjamin Tissoires]

we can skip one test when calling i801_enable_host_notify(). Given
that we call it all the time, it's better to consider the fact that
the adapter doesn't support Host Notify as not an error.

Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
---
 drivers/i2c/busses/i2c-i801.c | 13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)

diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c
index 3321956..a9b9bb4 100644
--- a/drivers/i2c/busses/i2c-i801.c
+++ b/drivers/i2c/busses/i2c-i801.c
@@ -943,12 +943,13 @@ static int i801_enable_host_notify(struct i2c_adapter *adapter)
 	struct i801_priv *priv = i2c_get_adapdata(adapter);
 
 	if (!(priv->features & FEATURE_HOST_NOTIFY))
-		return -ENOTSUPP;
+		return 0; /* not an error actually */
 
-	if (!priv->host_notify)
+	if (!priv->host_notify) {
 		priv->host_notify = i2c_setup_smbus_host_notify(adapter);
-	if (!priv->host_notify)
-		return -ENOMEM;
+		if (!priv->host_notify)
+			return -ENOMEM;
+	}
 
 	outb_p(SMBSLVCMD_HST_NTFY_INTREN, SMBSLVCMD(priv));
 	/* clear Host Notify bit to allow a new notification */
@@ -1635,7 +1636,7 @@ static int i801_probe(struct pci_dev *dev, const struct pci_device_id *id)
 	 * is not used if i2c_add_adapter() fails.
 	 */
 	err = i801_enable_host_notify(&priv->adapter);
-	if (err && err != -ENOTSUPP)
+	if (err)
 		dev_warn(&dev->dev, "Unable to enable SMBus Host Notify\n");
 
 	i801_probe_optional_slaves(priv);
@@ -1690,7 +1691,7 @@ static int i801_resume(struct device *dev)
 	int err;
 
 	err = i801_enable_host_notify(&priv->adapter);
-	if (err && err != -ENOTSUPP)
+	if (err)
 		dev_warn(dev, "Unable to enable SMBus Host Notify\n");
 
 	return 0;
-- 
2.5.5

[PATCH 5/5] i2c: i801: warn on i2c_handle_smbus_host_notify() errors

[Benjamin Tissoires]

i2c_handle_smbus_host_notify() returns 1 on success, something else on
errors.
There is a chance we get called while Host Notify is not available (yet),
so we need to clear the Host Notify bit in those rare case.

Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
---
 drivers/i2c/busses/i2c-i801.c | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c
index a9b9bb4..f02b248 100644
--- a/drivers/i2c/busses/i2c-i801.c
+++ b/drivers/i2c/busses/i2c-i801.c
@@ -578,12 +578,20 @@ static irqreturn_t i801_host_notify_isr(struct i801_priv *priv)
 {
 	unsigned short addr;
 	unsigned int data;
+	int ret;
+
+	if (unlikely(!priv->host_notify))
+		goto out;
 
 	addr = inb_p(SMBNTFDADD(priv)) >> 1;
 	data = inw_p(SMBNTFDDAT(priv));
 
-	i2c_handle_smbus_host_notify(priv->host_notify, addr, data);
+	ret = i2c_handle_smbus_host_notify(priv->host_notify, addr, data);
+	if (ret < 0)
+		dev_warn(&priv->pci_dev->dev,
+			 "Host Notify handling failed: %d\n", ret);
 
+out:
 	/* clear Host Notify bit and return */
 	outb_p(SMBSLVSTS_HST_NTFY_STS, SMBSLVSTS(priv));
 	return IRQ_HANDLED;
-- 
2.5.5

Re: [PATCH 1/5] i2c: i2c-smbus: prevent races on remove when Host Notify is used

[Jean Delvare]

Salut Benjamin,

On Thu, 28 Jul 2016 11:50:39 +0200, Benjamin Tissoires wrote:
> struct host_notify contains its own workqueue, so there is a race when
> the adapter gets removed:
> - the adapter schedules a notification
> - the notification is on hold
> - the adapter gets removed and all its children too
> - the worker fires and access illegal memory
> 
> Add an API to actually kill the workqueue and prevent it to access such
> illegal memory. I couldn't find a reliable way of automatically calling
> this, so it's the responsibility of the adapter driver to clean up after
> itself.

I'm no expert on the matter but I wonder if you could not just add it
to i2c_adapter_dev_release()?

> Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
> ---
>  drivers/i2c/busses/i2c-i801.c | 14 ++++++++++++++
>  drivers/i2c/i2c-smbus.c       | 18 ++++++++++++++++++
>  include/linux/i2c-smbus.h     |  1 +
>  3 files changed, 33 insertions(+)
> 
> diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c
> index 5ef9b73..cde9a7c 100644
> --- a/drivers/i2c/busses/i2c-i801.c
> +++ b/drivers/i2c/busses/i2c-i801.c
> @@ -959,6 +959,19 @@ static int i801_enable_host_notify(struct i2c_adapter *adapter)
>  	return 0;
>  }
>  
> +static void i801_disable_host_notify(struct i2c_adapter *adapter)
> +{
> +	struct i801_priv *priv = i2c_get_adapdata(adapter);

You pass the adapter as the parameter, but don't need it. All you need
is priv, which the caller has too. So you could pass priv as the
parameter directly and avoid the glue code.

> +
> +	if (!(priv->features & FEATURE_HOST_NOTIFY))
> +		return;
> +
> +	/* disable Host Notify... */
> +	outb_p(0, SMBSLVCMD(priv));

This assumes there's only one bit in the register, which is not true.
There are 3 bits. I did not notice the problem during my original
review, but in i801_enable_host_notify() you are silently zero-ing the
other 2 bits too, which isn't nice. You should only touch the bit that
matters to you, both here and in i801_enable_host_notify().

> +	/* ...and kill the already queued notifications */
> +	i2c_cancel_smbus_host_notify(priv->host_notify);

I thought we would rather process them than cancel them. But I suppose
it makes no difference if the system is being shut down anyway.

> +}
> +
>  static const struct i2c_algorithm smbus_algorithm = {
>  	.smbus_xfer	= i801_access,
>  	.functionality	= i801_func,
> @@ -1648,6 +1661,7 @@ static void i801_remove(struct pci_dev *dev)
>  	pm_runtime_forbid(&dev->dev);
>  	pm_runtime_get_noresume(&dev->dev);
>  
> +	i801_disable_host_notify(&priv->adapter);
>  	i801_del_mux(priv);
>  	i2c_del_adapter(&priv->adapter);
>  	i801_acpi_remove(priv);
> diff --git a/drivers/i2c/i2c-smbus.c b/drivers/i2c/i2c-smbus.c
> index b0d2679..60705dd 100644
> --- a/drivers/i2c/i2c-smbus.c
> +++ b/drivers/i2c/i2c-smbus.c
> @@ -279,6 +279,8 @@ static void smbus_host_notify_work(struct work_struct *work)
>   * Returns a struct smbus_host_notify pointer on success, and NULL on failure.
>   * The resulting smbus_host_notify must not be freed afterwards, it is a
>   * managed resource already.
> + * To prevent races on remove, the caller needs to stop the embedded worker
> + * by calling i2c_cancel_smbus_host_notify().
>   */
>  struct smbus_host_notify *i2c_setup_smbus_host_notify(struct i2c_adapter *adap)
>  {
> @@ -299,6 +301,22 @@ struct smbus_host_notify *i2c_setup_smbus_host_notify(struct i2c_adapter *adap)
>  EXPORT_SYMBOL_GPL(i2c_setup_smbus_host_notify);
>  
>  /**
> + * i2c_cancel_smbus_host_notify - Terminate any active Host Notification.
> + * @host_notify: the host_notify object to terminate
> + *
> + * Cancel any pending Host Notifcation. Must be called to ensure no races
> + * between the adaptor being removed and the Host Notify process being treated.

"... and the Host Notification being processed." would sound better
IMHO.

> + */
> +void i2c_cancel_smbus_host_notify(struct smbus_host_notify *host_notify)
> +{
> +	if (!host_notify)
> +		return;

Can this realistically happen (I mean without being a bug in the
driver)?

> +
> +	cancel_work_sync(&host_notify->work);
> +}
> +EXPORT_SYMBOL_GPL(i2c_cancel_smbus_host_notify);
> +
> +/**
>   * i2c_handle_smbus_host_notify - Forward a Host Notify event to the correct
>   * I2C client.
>   * @host_notify: the struct host_notify attached to the relevant adapter
> diff --git a/include/linux/i2c-smbus.h b/include/linux/i2c-smbus.h
> index c2e3324..ac02827 100644
> --- a/include/linux/i2c-smbus.h
> +++ b/include/linux/i2c-smbus.h
> @@ -76,5 +76,6 @@ struct smbus_host_notify {
>  struct smbus_host_notify *i2c_setup_smbus_host_notify(struct i2c_adapter *adap);
>  int i2c_handle_smbus_host_notify(struct smbus_host_notify *host_notify,
>  				 unsigned short addr, unsigned int data);
> +void i2c_cancel_smbus_host_notify(struct smbus_host_notify *host_notify);
>  
>  #endif /* _LINUX_I2C_SMBUS_H */


-- 
Jean Delvare
SUSE L3 Support

Re: [PATCH 2/5] i2c: i801: minor formatting issues

[Jean Delvare]

On Thu, 28 Jul 2016 11:50:40 +0200, Benjamin Tissoires wrote:
> No functional changes, just typos and remove unused #define.
> 
> Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.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 cde9a7c..80d67b8 100644
> --- a/drivers/i2c/busses/i2c-i801.c
> +++ b/drivers/i2c/busses/i2c-i801.c
> @@ -186,10 +186,10 @@
>  #define SMBHSTSTS_INTR		0x02
>  #define SMBHSTSTS_HOST_BUSY	0x01
>  
> -/* Host Notify Status registers bits */
> +/* Host Notify Status register bits */
>  #define SMBSLVSTS_HST_NTFY_STS	1
>  
> -/* Host Notify Command registers bits */
> +/* Host Notify Command register bits */
>  #define SMBSLVCMD_HST_NTFY_INTREN	0x01
>  
>  #define STATUS_ERROR_FLAGS	(SMBHSTSTS_FAILED | SMBHSTSTS_BUS_ERR | \
> @@ -269,8 +269,6 @@ struct i801_priv {
>  	struct smbus_host_notify *host_notify;
>  };
>  
> -#define SMBHSTNTFY_SIZE		8
> -
>  #define FEATURE_SMBUS_PEC	(1 << 0)
>  #define FEATURE_BLOCK_BUFFER	(1 << 1)
>  #define FEATURE_BLOCK_PROC	(1 << 2)

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

-- 
Jean Delvare
SUSE L3 Support

Re: [PATCH 3/5] i2c: i801: use BIT() macro for bits definition

[Jean Delvare]

On Thu, 28 Jul 2016 11:50:41 +0200, Benjamin Tissoires wrote:
> i801 mixes hexadecimal and decimal values for defining bits. However,
> we have a nice BIT() macro for this exact purpose.

Looks good to me. But maybe we could convert all the FEATURE_* defines
as well?

> No functional changes, cleanup only.
> 
> Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
> ---
>  drivers/i2c/busses/i2c-i801.c | 50 +++++++++++++++++++++----------------------
>  1 file changed, 25 insertions(+), 25 deletions(-)
> (...)

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

-- 
Jean Delvare
SUSE L3 Support

Re: [PATCH 4/5] i2c: i801: do not report an error if FEATURE_HOST_NOTIFY is not set

[Jean Delvare]

On Thu, 28 Jul 2016 11:50:42 +0200, Benjamin Tissoires wrote:
> we can skip one test when calling i801_enable_host_notify(). Given
> that we call it all the time, it's better to consider the fact that
> the adapter doesn't support Host Notify as not an error.
> 
> Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
> ---
>  drivers/i2c/busses/i2c-i801.c | 13 +++++++------
>  1 file changed, 7 insertions(+), 6 deletions(-)
> (...)

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

Thanks,
-- 
Jean Delvare
SUSE L3 Support

Re: [PATCH 1/5] i2c: i2c-smbus: prevent races on remove when Host Notify is used

[Benjamin Tissoires]

Salut Jean,

On Jul 29 2016 or thereabouts, Jean Delvare wrote:
> Salut Benjamin,
> 
> On Thu, 28 Jul 2016 11:50:39 +0200, Benjamin Tissoires wrote:
> > struct host_notify contains its own workqueue, so there is a race when
> > the adapter gets removed:
> > - the adapter schedules a notification
> > - the notification is on hold
> > - the adapter gets removed and all its children too
> > - the worker fires and access illegal memory
> > 
> > Add an API to actually kill the workqueue and prevent it to access such
> > illegal memory. I couldn't find a reliable way of automatically calling
> > this, so it's the responsibility of the adapter driver to clean up after
> > itself.
> 
> I'm no expert on the matter but I wonder if you could not just add it
> to i2c_adapter_dev_release()?

Hmm, 2 reasons I'd rather not:
1. in i2c_adapter_dev_release(), we already have removed the children
   (and called device_unregister() on the clients attached)
2. This would add a hard dependency on i2c-smbus and in the end the
   extension will get compiled in the kernel and not as a module anymore

For 1., we could also call this cleanup at the very beginning of
i2c_del_adapter(), but it feels weird to manually cancel this behind the
back of the driver.

For 2., that's something arguable.

Plus with this patche's implementation, it forces the driver (i2c-i801)
to unset the Host Notify bit which prevents new notifications to come in.

Thanks for the reviews on the other patches.

Cheers,
Benjamin

> 
> > Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
> > ---
> >  drivers/i2c/busses/i2c-i801.c | 14 ++++++++++++++
> >  drivers/i2c/i2c-smbus.c       | 18 ++++++++++++++++++
> >  include/linux/i2c-smbus.h     |  1 +
> >  3 files changed, 33 insertions(+)
> > 
> > diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c
> > index 5ef9b73..cde9a7c 100644
> > --- a/drivers/i2c/busses/i2c-i801.c
> > +++ b/drivers/i2c/busses/i2c-i801.c
> > @@ -959,6 +959,19 @@ static int i801_enable_host_notify(struct i2c_adapter *adapter)
> >  	return 0;
> >  }
> >  
> > +static void i801_disable_host_notify(struct i2c_adapter *adapter)
> > +{
> > +	struct i801_priv *priv = i2c_get_adapdata(adapter);
> 
> You pass the adapter as the parameter, but don't need it. All you need
> is priv, which the caller has too. So you could pass priv as the
> parameter directly and avoid the glue code.
> 
> > +
> > +	if (!(priv->features & FEATURE_HOST_NOTIFY))
> > +		return;
> > +
> > +	/* disable Host Notify... */
> > +	outb_p(0, SMBSLVCMD(priv));
> 
> This assumes there's only one bit in the register, which is not true.
> There are 3 bits. I did not notice the problem during my original
> review, but in i801_enable_host_notify() you are silently zero-ing the
> other 2 bits too, which isn't nice. You should only touch the bit that
> matters to you, both here and in i801_enable_host_notify().
> 
> > +	/* ...and kill the already queued notifications */
> > +	i2c_cancel_smbus_host_notify(priv->host_notify);
> 
> I thought we would rather process them than cancel them. But I suppose
> it makes no difference if the system is being shut down anyway.
> 
> > +}
> > +
> >  static const struct i2c_algorithm smbus_algorithm = {
> >  	.smbus_xfer	= i801_access,
> >  	.functionality	= i801_func,
> > @@ -1648,6 +1661,7 @@ static void i801_remove(struct pci_dev *dev)
> >  	pm_runtime_forbid(&dev->dev);
> >  	pm_runtime_get_noresume(&dev->dev);
> >  
> > +	i801_disable_host_notify(&priv->adapter);
> >  	i801_del_mux(priv);
> >  	i2c_del_adapter(&priv->adapter);
> >  	i801_acpi_remove(priv);
> > diff --git a/drivers/i2c/i2c-smbus.c b/drivers/i2c/i2c-smbus.c
> > index b0d2679..60705dd 100644
> > --- a/drivers/i2c/i2c-smbus.c
> > +++ b/drivers/i2c/i2c-smbus.c
> > @@ -279,6 +279,8 @@ static void smbus_host_notify_work(struct work_struct *work)
> >   * Returns a struct smbus_host_notify pointer on success, and NULL on failure.
> >   * The resulting smbus_host_notify must not be freed afterwards, it is a
> >   * managed resource already.
> > + * To prevent races on remove, the caller needs to stop the embedded worker
> > + * by calling i2c_cancel_smbus_host_notify().
> >   */
> >  struct smbus_host_notify *i2c_setup_smbus_host_notify(struct i2c_adapter *adap)
> >  {
> > @@ -299,6 +301,22 @@ struct smbus_host_notify *i2c_setup_smbus_host_notify(struct i2c_adapter *adap)
> >  EXPORT_SYMBOL_GPL(i2c_setup_smbus_host_notify);
> >  
> >  /**
> > + * i2c_cancel_smbus_host_notify - Terminate any active Host Notification.
> > + * @host_notify: the host_notify object to terminate
> > + *
> > + * Cancel any pending Host Notifcation. Must be called to ensure no races
> > + * between the adaptor being removed and the Host Notify process being treated.
> 
> "... and the Host Notification being processed." would sound better
> IMHO.
> 
> > + */
> > +void i2c_cancel_smbus_host_notify(struct smbus_host_notify *host_notify)
> > +{
> > +	if (!host_notify)
> > +		return;
> 
> Can this realistically happen (I mean without being a bug in the
> driver)?
> 
> > +
> > +	cancel_work_sync(&host_notify->work);
> > +}
> > +EXPORT_SYMBOL_GPL(i2c_cancel_smbus_host_notify);
> > +
> > +/**
> >   * i2c_handle_smbus_host_notify - Forward a Host Notify event to the correct
> >   * I2C client.
> >   * @host_notify: the struct host_notify attached to the relevant adapter
> > diff --git a/include/linux/i2c-smbus.h b/include/linux/i2c-smbus.h
> > index c2e3324..ac02827 100644
> > --- a/include/linux/i2c-smbus.h
> > +++ b/include/linux/i2c-smbus.h
> > @@ -76,5 +76,6 @@ struct smbus_host_notify {
> >  struct smbus_host_notify *i2c_setup_smbus_host_notify(struct i2c_adapter *adap);
> >  int i2c_handle_smbus_host_notify(struct smbus_host_notify *host_notify,
> >  				 unsigned short addr, unsigned int data);
> > +void i2c_cancel_smbus_host_notify(struct smbus_host_notify *host_notify);
> >  
> >  #endif /* _LINUX_I2C_SMBUS_H */
> 
> 
> -- 
> Jean Delvare
> SUSE L3 Support

Re: [PATCH 1/5] i2c: i2c-smbus: prevent races on remove when Host Notify is used

[Jean Delvare]

Hi Benjamin,

On Fri, 29 Jul 2016 11:12:12 +0200, Jean Delvare wrote:
> On Thu, 28 Jul 2016 11:50:39 +0200, Benjamin Tissoires wrote:
> > +static void i801_disable_host_notify(struct i2c_adapter *adapter)
> > +{
> > +	struct i801_priv *priv = i2c_get_adapdata(adapter);
> 
> You pass the adapter as the parameter, but don't need it. All you need
> is priv, which the caller has too. So you could pass priv as the
> parameter directly and avoid the glue code.
> 
> > +
> > +	if (!(priv->features & FEATURE_HOST_NOTIFY))
> > +		return;
> > +
> > +	/* disable Host Notify... */
> > +	outb_p(0, SMBSLVCMD(priv));
> 
> This assumes there's only one bit in the register, which is not true.
> There are 3 bits. I did not notice the problem during my original
> review, but in i801_enable_host_notify() you are silently zero-ing the
> other 2 bits too, which isn't nice. You should only touch the bit that
> matters to you, both here and in i801_enable_host_notify().

Thinking about it some more, I'd like to add: what if host notify was
enabled before the driver was loaded? The driver should leave the
controller in the same state it found it, ideally. We have learned in
the past that doing otherwise for PEC and I2C mode, for example, leads
to problems (lock-up on shutdown) on some systems.

This could happen with Host Notify as well. So I think you should save
the value at driver load time and restore it at unload time (same we do
with the hstcfg register.)

-- 
Jean Delvare
SUSE L3 Support

Re: [PATCH 5/5] i2c: i801: warn on i2c_handle_smbus_host_notify() errors

[Jean Delvare]

Hi Benjamin,

On Thu, 28 Jul 2016 11:50:43 +0200, Benjamin Tissoires wrote:
> i2c_handle_smbus_host_notify() returns 1 on success, something else on
> errors.

I did not notice before, but that doesn't make much sense IMHO. Your
function follows the "negative number on error" return model, so
returning the boolean value from schedule_work() doesn't look good.
Either you don't care about that value, and you should ignore it and
return 0 on success. Or you care and you should handle it in the
function. Right now value false/0 is in a gray zone, you don't consider
it as success according to the above comment, but you don't treat it as
an error in the code below either. I suppose it can't actually happen?
If so, I advise you don't check the value returned by schedule_work().

> There is a chance we get called while Host Notify is not available (yet),
> so we need to clear the Host Notify bit in those rare case.
> 
> Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
> ---
>  drivers/i2c/busses/i2c-i801.c | 10 +++++++++-
>  1 file changed, 9 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c
> index a9b9bb4..f02b248 100644
> --- a/drivers/i2c/busses/i2c-i801.c
> +++ b/drivers/i2c/busses/i2c-i801.c
> @@ -578,12 +578,20 @@ static irqreturn_t i801_host_notify_isr(struct i801_priv *priv)
>  {
>  	unsigned short addr;
>  	unsigned int data;
> +	int ret;
> +
> +	if (unlikely(!priv->host_notify))
> +		goto out;

This covers the case where you get an interrupt before the handler is
installed. Good catch, I never considered it.

>  
>  	addr = inb_p(SMBNTFDADD(priv)) >> 1;
>  	data = inw_p(SMBNTFDDAT(priv));
>  
> -	i2c_handle_smbus_host_notify(priv->host_notify, addr, data);
> +	ret = i2c_handle_smbus_host_notify(priv->host_notify, addr, data);
> +	if (ret < 0)
> +		dev_warn(&priv->pci_dev->dev,
> +			 "Host Notify handling failed: %d\n", ret);
>  
> +out:
>  	/* clear Host Notify bit and return */
>  	outb_p(SMBSLVSTS_HST_NTFY_STS, SMBSLVSTS(priv));
>  	return IRQ_HANDLED;

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

-- 
Jean Delvare
SUSE L3 Support

Re: [PATCH 1/5] i2c: i2c-smbus: prevent races on remove when Host Notify is used

[Benjamin Tissoires]

Hi Jean,

On Jul 29 2016 or thereabouts, Jean Delvare wrote:
> Salut Benjamin,
> 
> On Thu, 28 Jul 2016 11:50:39 +0200, Benjamin Tissoires wrote:
> > struct host_notify contains its own workqueue, so there is a race when
> > the adapter gets removed:
> > - the adapter schedules a notification
> > - the notification is on hold
> > - the adapter gets removed and all its children too
> > - the worker fires and access illegal memory
> > 
> > Add an API to actually kill the workqueue and prevent it to access such
> > illegal memory. I couldn't find a reliable way of automatically calling
> > this, so it's the responsibility of the adapter driver to clean up after
> > itself.
> 
> I'm no expert on the matter but I wonder if you could not just add it
> to i2c_adapter_dev_release()?

Looks like I did not replied to the other comments in this one:

> 
> > Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
> > ---
> >  drivers/i2c/busses/i2c-i801.c | 14 ++++++++++++++
> >  drivers/i2c/i2c-smbus.c       | 18 ++++++++++++++++++
> >  include/linux/i2c-smbus.h     |  1 +
> >  3 files changed, 33 insertions(+)
> > 
> > diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c
> > index 5ef9b73..cde9a7c 100644
> > --- a/drivers/i2c/busses/i2c-i801.c
> > +++ b/drivers/i2c/busses/i2c-i801.c
> > @@ -959,6 +959,19 @@ static int i801_enable_host_notify(struct i2c_adapter *adapter)
> >  	return 0;
> >  }
> >  
> > +static void i801_disable_host_notify(struct i2c_adapter *adapter)
> > +{
> > +	struct i801_priv *priv = i2c_get_adapdata(adapter);
> 
> You pass the adapter as the parameter, but don't need it. All you need
> is priv, which the caller has too. So you could pass priv as the
> parameter directly and avoid the glue code.

k, chenged in v2

> 
> > +
> > +	if (!(priv->features & FEATURE_HOST_NOTIFY))
> > +		return;
> > +
> > +	/* disable Host Notify... */
> > +	outb_p(0, SMBSLVCMD(priv));
> 
> This assumes there's only one bit in the register, which is not true.
> There are 3 bits. I did not notice the problem during my original
> review, but in i801_enable_host_notify() you are silently zero-ing the
> other 2 bits too, which isn't nice. You should only touch the bit that
> matters to you, both here and in i801_enable_host_notify().

agree. Will be fixed while (re)storing the boot state.

> 
> > +	/* ...and kill the already queued notifications */
> > +	i2c_cancel_smbus_host_notify(priv->host_notify);
> 
> I thought we would rather process them than cancel them. But I suppose
> it makes no difference if the system is being shut down anyway.

Actually, that's what is happening. cancel_work_sync prevents new works
to be added and waits for the current ones to be finished. I've amend
the comment in v2.

> 
> > +}
> > +
> >  static const struct i2c_algorithm smbus_algorithm = {
> >  	.smbus_xfer	= i801_access,
> >  	.functionality	= i801_func,
> > @@ -1648,6 +1661,7 @@ static void i801_remove(struct pci_dev *dev)
> >  	pm_runtime_forbid(&dev->dev);
> >  	pm_runtime_get_noresume(&dev->dev);
> >  
> > +	i801_disable_host_notify(&priv->adapter);
> >  	i801_del_mux(priv);
> >  	i2c_del_adapter(&priv->adapter);
> >  	i801_acpi_remove(priv);
> > diff --git a/drivers/i2c/i2c-smbus.c b/drivers/i2c/i2c-smbus.c
> > index b0d2679..60705dd 100644
> > --- a/drivers/i2c/i2c-smbus.c
> > +++ b/drivers/i2c/i2c-smbus.c
> > @@ -279,6 +279,8 @@ static void smbus_host_notify_work(struct work_struct *work)
> >   * Returns a struct smbus_host_notify pointer on success, and NULL on failure.
> >   * The resulting smbus_host_notify must not be freed afterwards, it is a
> >   * managed resource already.
> > + * To prevent races on remove, the caller needs to stop the embedded worker
> > + * by calling i2c_cancel_smbus_host_notify().
> >   */
> >  struct smbus_host_notify *i2c_setup_smbus_host_notify(struct i2c_adapter *adap)
> >  {
> > @@ -299,6 +301,22 @@ struct smbus_host_notify *i2c_setup_smbus_host_notify(struct i2c_adapter *adap)
> >  EXPORT_SYMBOL_GPL(i2c_setup_smbus_host_notify);
> >  
> >  /**
> > + * i2c_cancel_smbus_host_notify - Terminate any active Host Notification.
> > + * @host_notify: the host_notify object to terminate
> > + *
> > + * Cancel any pending Host Notifcation. Must be called to ensure no races
> > + * between the adaptor being removed and the Host Notify process being treated.
> 
> "... and the Host Notification being processed." would sound better
> IMHO.
> 
> > + */
> > +void i2c_cancel_smbus_host_notify(struct smbus_host_notify *host_notify)
> > +{
> > +	if (!host_notify)
> > +		return;
> 
> Can this realistically happen (I mean without being a bug in the
> driver)?

Sadly, this will have to be checked somewhere. In the i2c-i801 case,
given that I do not fail if Host Notify is not initialized, we may have
the feature declared but the struct smbus_host_notify not allocated
(memory issue?).

So we will need a check before calling this function from i2c-i801, and
we could have this in i2c-smbus instead to be more preemptive regarding
oopses.

Cheers,
Benjamin

> 
> > +
> > +	cancel_work_sync(&host_notify->work);
> > +}
> > +EXPORT_SYMBOL_GPL(i2c_cancel_smbus_host_notify);
> > +
> > +/**
> >   * i2c_handle_smbus_host_notify - Forward a Host Notify event to the correct
> >   * I2C client.
> >   * @host_notify: the struct host_notify attached to the relevant adapter
> > diff --git a/include/linux/i2c-smbus.h b/include/linux/i2c-smbus.h
> > index c2e3324..ac02827 100644
> > --- a/include/linux/i2c-smbus.h
> > +++ b/include/linux/i2c-smbus.h
> > @@ -76,5 +76,6 @@ struct smbus_host_notify {
> >  struct smbus_host_notify *i2c_setup_smbus_host_notify(struct i2c_adapter *adap);
> >  int i2c_handle_smbus_host_notify(struct smbus_host_notify *host_notify,
> >  				 unsigned short addr, unsigned int data);
> > +void i2c_cancel_smbus_host_notify(struct smbus_host_notify *host_notify);
> >  
> >  #endif /* _LINUX_I2C_SMBUS_H */
> 
> 
> -- 
> Jean Delvare
> SUSE L3 Support