Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1451846 > unrolled thread

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

Back to article view | Back to linux.kernel

Contents

  [PATCH 0/5] i2c: Host Notify / i801 fixes Benjamin Tissoires <benjamin.tissoires@redhat.com> - 2016-07-28 12:00 +0200
    [PATCH 5/5] i2c: i801: warn on i2c_handle_smbus_host_notify() errors Benjamin Tissoires <benjamin.tissoires@redhat.com> - 2016-07-28 12:00 +0200
      Re: [PATCH 5/5] i2c: i801: warn on i2c_handle_smbus_host_notify()  errors Jean Delvare <jdelvare@suse.de> - 2016-08-01 16:40 +0200
    [PATCH 2/5] i2c: i801: minor formatting issues Benjamin Tissoires <benjamin.tissoires@redhat.com> - 2016-07-28 12:00 +0200
      Re: [PATCH 2/5] i2c: i801: minor formatting issues Jean Delvare <jdelvare@suse.de> - 2016-07-29 11:20 +0200
    [PATCH 1/5] i2c: i2c-smbus: prevent races on remove when Host Notify is used Benjamin Tissoires <benjamin.tissoires@redhat.com> - 2016-07-28 12:00 +0200
      Re: [PATCH 1/5] i2c: i2c-smbus: prevent races on remove when Host  Notify is used Jean Delvare <jdelvare@suse.de> - 2016-07-29 11:20 +0200
        Re: [PATCH 1/5] i2c: i2c-smbus: prevent races on remove when Host  Notify is used Benjamin Tissoires <benjamin.tissoires@redhat.com> - 2016-07-29 18:40 +0200
        Re: [PATCH 1/5] i2c: i2c-smbus: prevent races on remove when Host  Notify is used Jean Delvare <jdelvare@suse.de> - 2016-08-01 15:40 +0200
    [PATCH 4/5] i2c: i801: do not report an error if FEATURE_HOST_NOTIFY is not set Benjamin Tissoires <benjamin.tissoires@redhat.com> - 2016-07-28 12:00 +0200
      Re: [PATCH 4/5] i2c: i801: do not report an error if  FEATURE_HOST_NOTIFY is not set Jean Delvare <jdelvare@suse.de> - 2016-07-29 12:50 +0200

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

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

[toc] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web