[PATCH] HID: intel-ish-hid: use async resume function

[PATCH] HID: intel-ish-hid: use async resume function

[Ye Xiang]

ISH IPC driver uses asynchronous workqueue to do resume now, but there is
a potential timing issue: when child devices resume before bus driver, it
will cause child devices resume failed and cannot be recovered until
reboot. The current implementation in this case do wait for IPC to resume
but fail to accommodate for a case when there is no ISH reboot and soft
resume is taking time. This issue is apparent on Tiger Lake platform with
5.11.13 kernel when doing suspend to idle then resume(s0ix) test. To
resolve this issue, we change ISHTP HID client to use asynchronous resume
callback too. In the asynchronous resume callback, it waits for the ISHTP
resume done event, and then notify ISHTP HID client link ready.

Signed-off-by: Ye Xiang <xiang.ye@intel.com>
---
 drivers/hid/intel-ish-hid/ishtp-hid-client.c | 15 +++++++++-
 drivers/hid/intel-ish-hid/ishtp-hid.h        |  1 +
 drivers/hid/intel-ish-hid/ishtp/bus.c        | 29 +++++++++++++++-----
 include/linux/intel-ish-client-if.h          |  2 ++
 4 files changed, 39 insertions(+), 8 deletions(-)

diff --git a/drivers/hid/intel-ish-hid/ishtp-hid-client.c b/drivers/hid/intel-ish-hid/ishtp-hid-client.c
index 7167412d89d9..9d53e85fdef3 100644
--- a/drivers/hid/intel-ish-hid/ishtp-hid-client.c
+++ b/drivers/hid/intel-ish-hid/ishtp-hid-client.c
@@ -779,6 +779,17 @@ static void hid_ishtp_cl_reset_handler(struct work_struct *work)
 	}
 }
 
+static void hid_ishtp_cl_resume_handler(struct work_struct *work)
+{
+	struct ishtp_cl_data *client_data = container_of(work, struct ishtp_cl_data, resume_work);
+	struct ishtp_cl *hid_ishtp_cl = client_data->hid_ishtp_cl;
+
+	if (ishtp_wait_resume(ishtp_get_ishtp_device(hid_ishtp_cl))) {
+		client_data->suspended = false;
+		wake_up_interruptible(&client_data->ishtp_resume_wait);
+	}
+}
+
 void (*hid_print_trace)(void *unused, const char *format, ...);
 
 /**
@@ -817,6 +828,8 @@ static int hid_ishtp_cl_probe(struct ishtp_cl_device *cl_device)
 	init_waitqueue_head(&client_data->ishtp_resume_wait);
 
 	INIT_WORK(&client_data->work, hid_ishtp_cl_reset_handler);
+	INIT_WORK(&client_data->resume_work, hid_ishtp_cl_resume_handler);
+
 
 	hid_print_trace = ishtp_trace_callback(cl_device);
 
@@ -918,7 +931,7 @@ static int hid_ishtp_cl_resume(struct device *device)
 
 	hid_ishtp_trace(client_data, "%s hid_ishtp_cl %p\n", __func__,
 			hid_ishtp_cl);
-	client_data->suspended = false;
+	schedule_work(&client_data->resume_work);
 	return 0;
 }
 
diff --git a/drivers/hid/intel-ish-hid/ishtp-hid.h b/drivers/hid/intel-ish-hid/ishtp-hid.h
index 5ffd0da3cf1f..e5fa753fe92f 100644
--- a/drivers/hid/intel-ish-hid/ishtp-hid.h
+++ b/drivers/hid/intel-ish-hid/ishtp-hid.h
@@ -140,6 +140,7 @@ struct ishtp_cl_data {
 	int multi_packet_cnt;
 
 	struct work_struct work;
+	struct work_struct resume_work;
 	struct ishtp_cl_device *cl_device;
 };
 
diff --git a/drivers/hid/intel-ish-hid/ishtp/bus.c b/drivers/hid/intel-ish-hid/ishtp/bus.c
index 0d6465f0eaa8..ead6c8f32759 100644
--- a/drivers/hid/intel-ish-hid/ishtp/bus.c
+++ b/drivers/hid/intel-ish-hid/ishtp/bus.c
@@ -329,13 +329,6 @@ static int ishtp_cl_device_resume(struct device *dev)
 	if (!device)
 		return 0;
 
-	/*
-	 * When ISH needs hard reset, it is done asynchrnously, hence bus
-	 * resume will  be called before full ISH resume
-	 */
-	if (device->ishtp_dev->resume_flag)
-		return 0;
-
 	driver = to_ishtp_cl_driver(dev->driver);
 	if (driver && driver->driver.pm) {
 		if (driver->driver.pm->resume)
@@ -863,6 +856,28 @@ struct device *ishtp_device(struct ishtp_cl_device *device)
 }
 EXPORT_SYMBOL(ishtp_device);
 
+/**
+ * ishtp_wait_resume() - Wait for IPC resume
+ *
+ * Wait for IPC resume
+ *
+ * Return: resume complete or not
+ */
+bool ishtp_wait_resume(struct ishtp_device *dev)
+{
+	/* 50ms to get resume response */
+	#define WAIT_FOR_RESUME_ACK_MS		50
+
+	/* Waiting to get resume response */
+	if (dev->resume_flag)
+		wait_event_interruptible_timeout(dev->resume_wait,
+						 !dev->resume_flag,
+						 msecs_to_jiffies(WAIT_FOR_RESUME_ACK_MS));
+
+	return (!dev->resume_flag);
+}
+EXPORT_SYMBOL_GPL(ishtp_wait_resume);
+
 /**
  * ishtp_get_pci_device() - Return PCI device dev pointer
  * This interface is used to return PCI device pointer
diff --git a/include/linux/intel-ish-client-if.h b/include/linux/intel-ish-client-if.h
index 1153e0030133..ec3a6ccbece4 100644
--- a/include/linux/intel-ish-client-if.h
+++ b/include/linux/intel-ish-client-if.h
@@ -76,6 +76,8 @@ int ishtp_register_event_cb(struct ishtp_cl_device *device,
 
 /* Get the device * from ishtp device instance */
 struct device *ishtp_device(struct ishtp_cl_device *cl_device);
+/* wait for IPC resume */
+bool ishtp_wait_resume(struct ishtp_device *dev);
 /* Trace interface for clients */
 void *ishtp_trace_callback(struct ishtp_cl_device *cl_device);
 /* Get device pointer of PCI device for DMA acces */

base-commit: f5711311bfa1abcc64c6dd1e912666a8c0b29a1a
-- 
2.17.1

Re: [PATCH] HID: intel-ish-hid: use async resume function

[Srinivas Pandruvada]

On Sun, 2021-06-13 at 11:25 +0800, Ye Xiang wrote:
> ISH IPC driver uses asynchronous workqueue to do resume now, but
> there is
> a potential timing issue: when child devices resume before bus
> driver, it
> will cause child devices resume failed and cannot be recovered until
> reboot. The current implementation in this case do wait for IPC to
> resume
> but fail to accommodate for a case when there is no ISH reboot and
> soft
> resume is taking time. This issue is apparent on Tiger Lake platform
> with
> 5.11.13 kernel when doing suspend to idle then resume(s0ix) test. To
> resolve this issue, we change ISHTP HID client to use asynchronous
> resume
> callback too. In the asynchronous resume callback, it waits for the
> ISHTP
> resume done event, and then notify ISHTP HID client link ready.
> 
> Signed-off-by: Ye Xiang <xiang.ye@intel.com>
Acked-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>

> ---
>  drivers/hid/intel-ish-hid/ishtp-hid-client.c | 15 +++++++++-
>  drivers/hid/intel-ish-hid/ishtp-hid.h        |  1 +
>  drivers/hid/intel-ish-hid/ishtp/bus.c        | 29 +++++++++++++++---
> --
>  include/linux/intel-ish-client-if.h          |  2 ++
>  4 files changed, 39 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/hid/intel-ish-hid/ishtp-hid-client.c
> b/drivers/hid/intel-ish-hid/ishtp-hid-client.c
> index 7167412d89d9..9d53e85fdef3 100644
> --- a/drivers/hid/intel-ish-hid/ishtp-hid-client.c
> +++ b/drivers/hid/intel-ish-hid/ishtp-hid-client.c
> @@ -779,6 +779,17 @@ static void hid_ishtp_cl_reset_handler(struct
> work_struct *work)
>  	}
>  }
>  
> +static void hid_ishtp_cl_resume_handler(struct work_struct *work)
> +{
> +	struct ishtp_cl_data *client_data = container_of(work, struct
> ishtp_cl_data, resume_work);
> +	struct ishtp_cl *hid_ishtp_cl = client_data->hid_ishtp_cl;
> +
> +	if (ishtp_wait_resume(ishtp_get_ishtp_device(hid_ishtp_cl))) {
> +		client_data->suspended = false;
> +		wake_up_interruptible(&client_data->ishtp_resume_wait);
> +	}
> +}
> +
>  void (*hid_print_trace)(void *unused, const char *format, ...);
>  
>  /**
> @@ -817,6 +828,8 @@ static int hid_ishtp_cl_probe(struct
> ishtp_cl_device *cl_device)
>  	init_waitqueue_head(&client_data->ishtp_resume_wait);
>  
>  	INIT_WORK(&client_data->work, hid_ishtp_cl_reset_handler);
> +	INIT_WORK(&client_data->resume_work,
> hid_ishtp_cl_resume_handler);
> +
>  
>  	hid_print_trace = ishtp_trace_callback(cl_device);
>  
> @@ -918,7 +931,7 @@ static int hid_ishtp_cl_resume(struct device
> *device)
>  
>  	hid_ishtp_trace(client_data, "%s hid_ishtp_cl %p\n", __func__,
>  			hid_ishtp_cl);
> -	client_data->suspended = false;
> +	schedule_work(&client_data->resume_work);
>  	return 0;
>  }
>  
> diff --git a/drivers/hid/intel-ish-hid/ishtp-hid.h
> b/drivers/hid/intel-ish-hid/ishtp-hid.h
> index 5ffd0da3cf1f..e5fa753fe92f 100644
> --- a/drivers/hid/intel-ish-hid/ishtp-hid.h
> +++ b/drivers/hid/intel-ish-hid/ishtp-hid.h
> @@ -140,6 +140,7 @@ struct ishtp_cl_data {
>  	int multi_packet_cnt;
>  
>  	struct work_struct work;
> +	struct work_struct resume_work;
>  	struct ishtp_cl_device *cl_device;
>  };
>  
> diff --git a/drivers/hid/intel-ish-hid/ishtp/bus.c
> b/drivers/hid/intel-ish-hid/ishtp/bus.c
> index 0d6465f0eaa8..ead6c8f32759 100644
> --- a/drivers/hid/intel-ish-hid/ishtp/bus.c
> +++ b/drivers/hid/intel-ish-hid/ishtp/bus.c
> @@ -329,13 +329,6 @@ static int ishtp_cl_device_resume(struct device
> *dev)
>  	if (!device)
>  		return 0;
>  
> -	/*
> -	 * When ISH needs hard reset, it is done asynchrnously, hence
> bus
> -	 * resume will  be called before full ISH resume
> -	 */
> -	if (device->ishtp_dev->resume_flag)
> -		return 0;
> -
>  	driver = to_ishtp_cl_driver(dev->driver);
>  	if (driver && driver->driver.pm) {
>  		if (driver->driver.pm->resume)
> @@ -863,6 +856,28 @@ struct device *ishtp_device(struct
> ishtp_cl_device *device)
>  }
>  EXPORT_SYMBOL(ishtp_device);
>  
> +/**
> + * ishtp_wait_resume() - Wait for IPC resume
> + *
> + * Wait for IPC resume
> + *
> + * Return: resume complete or not
> + */
> +bool ishtp_wait_resume(struct ishtp_device *dev)
> +{
> +	/* 50ms to get resume response */
> +	#define WAIT_FOR_RESUME_ACK_MS		50
> +
> +	/* Waiting to get resume response */
> +	if (dev->resume_flag)
> +		wait_event_interruptible_timeout(dev->resume_wait,
> +						 !dev->resume_flag,
> +						 msecs_to_jiffies(WAIT_
> FOR_RESUME_ACK_MS));
> +
> +	return (!dev->resume_flag);
> +}
> +EXPORT_SYMBOL_GPL(ishtp_wait_resume);
> +
>  /**
>   * ishtp_get_pci_device() - Return PCI device dev pointer
>   * This interface is used to return PCI device pointer
> diff --git a/include/linux/intel-ish-client-if.h
> b/include/linux/intel-ish-client-if.h
> index 1153e0030133..ec3a6ccbece4 100644
> --- a/include/linux/intel-ish-client-if.h
> +++ b/include/linux/intel-ish-client-if.h
> @@ -76,6 +76,8 @@ int ishtp_register_event_cb(struct ishtp_cl_device
> *device,
>  
>  /* Get the device * from ishtp device instance */
>  struct device *ishtp_device(struct ishtp_cl_device *cl_device);
> +/* wait for IPC resume */
> +bool ishtp_wait_resume(struct ishtp_device *dev);
>  /* Trace interface for clients */
>  void *ishtp_trace_callback(struct ishtp_cl_device *cl_device);
>  /* Get device pointer of PCI device for DMA acces */
> 
> base-commit: f5711311bfa1abcc64c6dd1e912666a8c0b29a1a

Re: [PATCH] HID: intel-ish-hid: use async resume function

[Jiri Kosina]

On Sun, 13 Jun 2021, Ye Xiang wrote:

> ISH IPC driver uses asynchronous workqueue to do resume now, but there is
> a potential timing issue: when child devices resume before bus driver, it
> will cause child devices resume failed and cannot be recovered until
> reboot. The current implementation in this case do wait for IPC to resume
> but fail to accommodate for a case when there is no ISH reboot and soft
> resume is taking time. This issue is apparent on Tiger Lake platform with
> 5.11.13 kernel when doing suspend to idle then resume(s0ix) test. To
> resolve this issue, we change ISHTP HID client to use asynchronous resume
> callback too. In the asynchronous resume callback, it waits for the ISHTP
> resume done event, and then notify ISHTP HID client link ready.
> 
> Signed-off-by: Ye Xiang <xiang.ye@intel.com>

Srinivas, can I please get your Ack for this one?

Thanks,

-- 
Jiri Kosina
SUSE Labs

Re: [PATCH] HID: intel-ish-hid: use async resume function

[srinivas pandruvada]

On Sun, 2021-06-13 at 11:25 +0800, Ye Xiang wrote:
> ISH IPC driver uses asynchronous workqueue to do resume now, but there
> is
> a potential timing issue: when child devices resume before bus driver,
> it
> will cause child devices resume failed and cannot be recovered until
> reboot. The current implementation in this case do wait for IPC to
> resume
> but fail to accommodate for a case when there is no ISH reboot and soft
> resume is taking time. This issue is apparent on Tiger Lake platform
> with
> 5.11.13 kernel when doing suspend to idle then resume(s0ix) test. To
> resolve this issue, we change ISHTP HID client to use asynchronous
> resume
> callback too. In the asynchronous resume callback, it waits for the
> ISHTP
> resume done event, and then notify ISHTP HID client link ready.
> 
> Signed-off-by: Ye Xiang <xiang.ye@intel.com>
Acked-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>

> ---
>  drivers/hid/intel-ish-hid/ishtp-hid-client.c | 15 +++++++++-
>  drivers/hid/intel-ish-hid/ishtp-hid.h        |  1 +
>  drivers/hid/intel-ish-hid/ishtp/bus.c        | 29 +++++++++++++++-----
>  include/linux/intel-ish-client-if.h          |  2 ++
>  4 files changed, 39 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/hid/intel-ish-hid/ishtp-hid-client.c
> b/drivers/hid/intel-ish-hid/ishtp-hid-client.c
> index 7167412d89d9..9d53e85fdef3 100644
> --- a/drivers/hid/intel-ish-hid/ishtp-hid-client.c
> +++ b/drivers/hid/intel-ish-hid/ishtp-hid-client.c
> @@ -779,6 +779,17 @@ static void hid_ishtp_cl_reset_handler(struct
> work_struct *work)
>         }
>  }
>  
> +static void hid_ishtp_cl_resume_handler(struct work_struct *work)
> +{
> +       struct ishtp_cl_data *client_data = container_of(work, struct
> ishtp_cl_data, resume_work);
> +       struct ishtp_cl *hid_ishtp_cl = client_data->hid_ishtp_cl;
> +
> +       if (ishtp_wait_resume(ishtp_get_ishtp_device(hid_ishtp_cl))) {
> +               client_data->suspended = false;
> +               wake_up_interruptible(&client_data->ishtp_resume_wait);
> +       }
> +}
> +
>  void (*hid_print_trace)(void *unused, const char *format, ...);
>  
>  /**
> @@ -817,6 +828,8 @@ static int hid_ishtp_cl_probe(struct
> ishtp_cl_device *cl_device)
>         init_waitqueue_head(&client_data->ishtp_resume_wait);
>  
>         INIT_WORK(&client_data->work, hid_ishtp_cl_reset_handler);
> +       INIT_WORK(&client_data->resume_work,
> hid_ishtp_cl_resume_handler);
> +
>  
>         hid_print_trace = ishtp_trace_callback(cl_device);
>  
> @@ -918,7 +931,7 @@ static int hid_ishtp_cl_resume(struct device
> *device)
>  
>         hid_ishtp_trace(client_data, "%s hid_ishtp_cl %p\n", __func__,
>                         hid_ishtp_cl);
> -       client_data->suspended = false;
> +       schedule_work(&client_data->resume_work);
>         return 0;
>  }
>  
> diff --git a/drivers/hid/intel-ish-hid/ishtp-hid.h b/drivers/hid/intel-
> ish-hid/ishtp-hid.h
> index 5ffd0da3cf1f..e5fa753fe92f 100644
> --- a/drivers/hid/intel-ish-hid/ishtp-hid.h
> +++ b/drivers/hid/intel-ish-hid/ishtp-hid.h
> @@ -140,6 +140,7 @@ struct ishtp_cl_data {
>         int multi_packet_cnt;
>  
>         struct work_struct work;
> +       struct work_struct resume_work;
>         struct ishtp_cl_device *cl_device;
>  };
>  
> diff --git a/drivers/hid/intel-ish-hid/ishtp/bus.c b/drivers/hid/intel-
> ish-hid/ishtp/bus.c
> index 0d6465f0eaa8..ead6c8f32759 100644
> --- a/drivers/hid/intel-ish-hid/ishtp/bus.c
> +++ b/drivers/hid/intel-ish-hid/ishtp/bus.c
> @@ -329,13 +329,6 @@ static int ishtp_cl_device_resume(struct device
> *dev)
>         if (!device)
>                 return 0;
>  
> -       /*
> -        * When ISH needs hard reset, it is done asynchrnously, hence
> bus
> -        * resume will  be called before full ISH resume
> -        */
> -       if (device->ishtp_dev->resume_flag)
> -               return 0;
> -
>         driver = to_ishtp_cl_driver(dev->driver);
>         if (driver && driver->driver.pm) {
>                 if (driver->driver.pm->resume)
> @@ -863,6 +856,28 @@ struct device *ishtp_device(struct ishtp_cl_device
> *device)
>  }
>  EXPORT_SYMBOL(ishtp_device);
>  
> +/**
> + * ishtp_wait_resume() - Wait for IPC resume
> + *
> + * Wait for IPC resume
> + *
> + * Return: resume complete or not
> + */
> +bool ishtp_wait_resume(struct ishtp_device *dev)
> +{
> +       /* 50ms to get resume response */
> +       #define WAIT_FOR_RESUME_ACK_MS          50
> +
> +       /* Waiting to get resume response */
> +       if (dev->resume_flag)
> +               wait_event_interruptible_timeout(dev->resume_wait,
> +                                                !dev->resume_flag,
> +                                               
> msecs_to_jiffies(WAIT_FOR_RESUME_ACK_MS));
> +
> +       return (!dev->resume_flag);
> +}
> +EXPORT_SYMBOL_GPL(ishtp_wait_resume);
> +
>  /**
>   * ishtp_get_pci_device() - Return PCI device dev pointer
>   * This interface is used to return PCI device pointer
> diff --git a/include/linux/intel-ish-client-if.h b/include/linux/intel-
> ish-client-if.h
> index 1153e0030133..ec3a6ccbece4 100644
> --- a/include/linux/intel-ish-client-if.h
> +++ b/include/linux/intel-ish-client-if.h
> @@ -76,6 +76,8 @@ int ishtp_register_event_cb(struct ishtp_cl_device
> *device,
>  
>  /* Get the device * from ishtp device instance */
>  struct device *ishtp_device(struct ishtp_cl_device *cl_device);
> +/* wait for IPC resume */
> +bool ishtp_wait_resume(struct ishtp_device *dev);
>  /* Trace interface for clients */
>  void *ishtp_trace_callback(struct ishtp_cl_device *cl_device);
>  /* Get device pointer of PCI device for DMA acces */
> 
> base-commit: f5711311bfa1abcc64c6dd1e912666a8c0b29a1a

Re: [PATCH] HID: intel-ish-hid: use async resume function

[srinivas pandruvada]

On Fri, 2021-06-25 at 14:12 +0200, Jiri Kosina wrote:
> On Sun, 13 Jun 2021, Ye Xiang wrote:
> 
> > ISH IPC driver uses asynchronous workqueue to do resume now, but
> > there is
> > a potential timing issue: when child devices resume before bus
> > driver, it
> > will cause child devices resume failed and cannot be recovered
> > until
> > reboot. The current implementation in this case do wait for IPC to
> > resume
> > but fail to accommodate for a case when there is no ISH reboot and
> > soft
> > resume is taking time. This issue is apparent on Tiger Lake
> > platform with
> > 5.11.13 kernel when doing suspend to idle then resume(s0ix) test.
> > To
> > resolve this issue, we change ISHTP HID client to use asynchronous
> > resume
> > callback too. In the asynchronous resume callback, it waits for the
> > ISHTP
> > resume done event, and then notify ISHTP HID client link ready.
> > 
> > Signed-off-by: Ye Xiang <xiang.ye@intel.com>
> 
> Srinivas, can I please get your Ack for this one?
Done.

Thanks,
Srinivas

> 
> Thanks,
> 

Re: [PATCH] HID: intel-ish-hid: use async resume function

[Jiri Kosina]

On Sun, 13 Jun 2021, Ye Xiang wrote:

> ISH IPC driver uses asynchronous workqueue to do resume now, but there is
> a potential timing issue: when child devices resume before bus driver, it
> will cause child devices resume failed and cannot be recovered until
> reboot. The current implementation in this case do wait for IPC to resume
> but fail to accommodate for a case when there is no ISH reboot and soft
> resume is taking time. This issue is apparent on Tiger Lake platform with
> 5.11.13 kernel when doing suspend to idle then resume(s0ix) test. To
> resolve this issue, we change ISHTP HID client to use asynchronous resume
> callback too. In the asynchronous resume callback, it waits for the ISHTP
> resume done event, and then notify ISHTP HID client link ready.
> 
> Signed-off-by: Ye Xiang <xiang.ye@intel.com>

Applied, thank you.

-- 
Jiri Kosina
SUSE Labs