linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 1/7] hid: intel-ish-hid: ishtp: add helper function for driver data get/set
@ 2016-12-23  1:22 Even Xu
  2016-12-23  1:22 ` [PATCH 2/7] hid: intel-ish-hid: use helper function for private driver data set/get Even Xu
                   ` (5 more replies)
  0 siblings, 6 replies; 19+ messages in thread
From: Even Xu @ 2016-12-23  1:22 UTC (permalink / raw)
  To: jikos, benjamin.tissoires, srinivas.pandruvada, arnd, gregkh,
	andriy.shevchenko
  Cc: linux-input, linux-kernel, Even Xu

Add helper function ishtp_set_drvdata() and ishtp_get_drvdata() for
different ISH client drivers to set/get private driver data.

Signed-off-by: Even Xu <even.xu@intel.com>
Reviewed-by: Andriy Shevchenko <andriy.shevchenko@intel.com>
Acked-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>
---
 drivers/hid/intel-ish-hid/ishtp/bus.c | 27 +++++++++++++++++++++++++++
 drivers/hid/intel-ish-hid/ishtp/bus.h |  3 +++
 2 files changed, 30 insertions(+)

diff --git a/drivers/hid/intel-ish-hid/ishtp/bus.c b/drivers/hid/intel-ish-hid/ishtp/bus.c
index 2565215..40e13f1 100644
--- a/drivers/hid/intel-ish-hid/ishtp/bus.c
+++ b/drivers/hid/intel-ish-hid/ishtp/bus.c
@@ -562,6 +562,33 @@ void ishtp_put_device(struct ishtp_cl_device *cl_device)
 EXPORT_SYMBOL(ishtp_put_device);
 
 /**
+ * ishtp_set_drvdata() - set client driver data
+ * @cl_device:	client device instance
+ * @data:	driver data need to be set
+ *
+ * Set client driver data to cl_device->driver_data.
+ */
+void ishtp_set_drvdata(struct ishtp_cl_device *cl_device, void *data)
+{
+	cl_device->driver_data = data;
+}
+EXPORT_SYMBOL(ishtp_set_drvdata);
+
+/**
+ * ishtp_get_drvdata() - get client driver data
+ * @cl_device:	client device instance
+ *
+ * Get client driver data from cl_device->driver_data.
+ *
+ * Return: pointer of driver data
+ */
+void *ishtp_get_drvdata(struct ishtp_cl_device *cl_device)
+{
+	return cl_device->driver_data;
+}
+EXPORT_SYMBOL(ishtp_get_drvdata);
+
+/**
  * ishtp_bus_new_client() - Create a new client
  * @dev:	ISHTP device instance
  *
diff --git a/drivers/hid/intel-ish-hid/ishtp/bus.h b/drivers/hid/intel-ish-hid/ishtp/bus.h
index a1ffae7..8888331 100644
--- a/drivers/hid/intel-ish-hid/ishtp/bus.h
+++ b/drivers/hid/intel-ish-hid/ishtp/bus.h
@@ -101,6 +101,9 @@ void	ishtp_reset_compl_handler(struct ishtp_device *dev);
 void	ishtp_put_device(struct ishtp_cl_device *);
 void	ishtp_get_device(struct ishtp_cl_device *);
 
+void	ishtp_set_drvdata(struct ishtp_cl_device *cl_device, void *data);
+void	*ishtp_get_drvdata(struct ishtp_cl_device *cl_device);
+
 int	__ishtp_cl_driver_register(struct ishtp_cl_driver *driver,
 				   struct module *owner);
 #define ishtp_cl_driver_register(driver)		\
-- 
2.7.4

^ permalink raw reply related	[flat|nested] 19+ messages in thread

* [PATCH 2/7] hid: intel-ish-hid: use helper function for private driver data set/get
  2016-12-23  1:22 [PATCH 1/7] hid: intel-ish-hid: ishtp: add helper function for driver data get/set Even Xu
@ 2016-12-23  1:22 ` Even Xu
  2016-12-23  1:22 ` [PATCH 3/7] hid: intel-ish-hid: ishtp: add helper functions for client buffer operation Even Xu
                   ` (4 subsequent siblings)
  5 siblings, 0 replies; 19+ messages in thread
From: Even Xu @ 2016-12-23  1:22 UTC (permalink / raw)
  To: jikos, benjamin.tissoires, srinivas.pandruvada, arnd, gregkh,
	andriy.shevchenko
  Cc: linux-input, linux-kernel, Even Xu

Use helper set/get function to set/get driver data in ishtp-hid-client
driver instead of directly accessing cl_device driver_data member.

Signed-off-by: Even Xu <even.xu@intel.com>
Reviewed-by: Andriy Shevchenko <andriy.shevchenko@intel.com>
Acked-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>
---
 drivers/hid/intel-ish-hid/ishtp-hid-client.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/drivers/hid/intel-ish-hid/ishtp-hid-client.c b/drivers/hid/intel-ish-hid/ishtp-hid-client.c
index 5c643d7a..3a3203d 100644
--- a/drivers/hid/intel-ish-hid/ishtp-hid-client.c
+++ b/drivers/hid/intel-ish-hid/ishtp-hid-client.c
@@ -321,7 +321,7 @@ static void process_recv(struct ishtp_cl *hid_ishtp_cl, void *recv_buf,
  */
 static void ish_cl_event_cb(struct ishtp_cl_device *device)
 {
-	struct ishtp_cl	*hid_ishtp_cl = device->driver_data;
+	struct ishtp_cl	*hid_ishtp_cl = ishtp_get_drvdata(device);
 	struct ishtp_cl_rb *rb_in_proc;
 	size_t r_length;
 	unsigned long flags;
@@ -770,7 +770,7 @@ static void hid_ishtp_cl_reset_handler(struct work_struct *work)
 	if (!hid_ishtp_cl)
 		return;
 
-	cl_device->driver_data = hid_ishtp_cl;
+	ishtp_set_drvdata(cl_device, hid_ishtp_cl);
 	hid_ishtp_cl->client_data = client_data;
 	client_data->hid_ishtp_cl = hid_ishtp_cl;
 
@@ -819,7 +819,7 @@ static int hid_ishtp_cl_probe(struct ishtp_cl_device *cl_device)
 	if (!hid_ishtp_cl)
 		return -ENOMEM;
 
-	cl_device->driver_data = hid_ishtp_cl;
+	ishtp_set_drvdata(cl_device, hid_ishtp_cl);
 	hid_ishtp_cl->client_data = client_data;
 	client_data->hid_ishtp_cl = hid_ishtp_cl;
 	client_data->cl_device = cl_device;
@@ -849,7 +849,7 @@ static int hid_ishtp_cl_probe(struct ishtp_cl_device *cl_device)
  */
 static int hid_ishtp_cl_remove(struct ishtp_cl_device *cl_device)
 {
-	struct ishtp_cl *hid_ishtp_cl = cl_device->driver_data;
+	struct ishtp_cl *hid_ishtp_cl = ishtp_get_drvdata(cl_device);
 	struct ishtp_cl_data *client_data = hid_ishtp_cl->client_data;
 
 	hid_ishtp_trace(client_data, "%s hid_ishtp_cl %p\n", __func__,
@@ -879,7 +879,7 @@ static int hid_ishtp_cl_remove(struct ishtp_cl_device *cl_device)
  */
 static int hid_ishtp_cl_reset(struct ishtp_cl_device *cl_device)
 {
-	struct ishtp_cl *hid_ishtp_cl = cl_device->driver_data;
+	struct ishtp_cl *hid_ishtp_cl = ishtp_get_drvdata(cl_device);
 	struct ishtp_cl_data *client_data = hid_ishtp_cl->client_data;
 
 	hid_ishtp_trace(client_data, "%s hid_ishtp_cl %p\n", __func__,
@@ -903,7 +903,7 @@ static int hid_ishtp_cl_reset(struct ishtp_cl_device *cl_device)
 static int hid_ishtp_cl_suspend(struct device *device)
 {
 	struct ishtp_cl_device *cl_device = to_ishtp_cl_device(device);
-	struct ishtp_cl *hid_ishtp_cl = cl_device->driver_data;
+	struct ishtp_cl *hid_ishtp_cl = ishtp_get_drvdata(cl_device);
 	struct ishtp_cl_data *client_data = hid_ishtp_cl->client_data;
 
 	hid_ishtp_trace(client_data, "%s hid_ishtp_cl %p\n", __func__,
@@ -924,7 +924,7 @@ static int hid_ishtp_cl_suspend(struct device *device)
 static int hid_ishtp_cl_resume(struct device *device)
 {
 	struct ishtp_cl_device *cl_device = to_ishtp_cl_device(device);
-	struct ishtp_cl *hid_ishtp_cl = cl_device->driver_data;
+	struct ishtp_cl *hid_ishtp_cl = ishtp_get_drvdata(cl_device);
 	struct ishtp_cl_data *client_data = hid_ishtp_cl->client_data;
 
 	hid_ishtp_trace(client_data, "%s hid_ishtp_cl %p\n", __func__,
-- 
2.7.4

^ permalink raw reply related	[flat|nested] 19+ messages in thread

* [PATCH 3/7] hid: intel-ish-hid: ishtp: add helper functions for client buffer operation
  2016-12-23  1:22 [PATCH 1/7] hid: intel-ish-hid: ishtp: add helper function for driver data get/set Even Xu
  2016-12-23  1:22 ` [PATCH 2/7] hid: intel-ish-hid: use helper function for private driver data set/get Even Xu
@ 2016-12-23  1:22 ` Even Xu
  2016-12-23  1:22 ` [PATCH 4/7] hid: intel-ish-hid: use helper function to access client buffer Even Xu
                   ` (3 subsequent siblings)
  5 siblings, 0 replies; 19+ messages in thread
From: Even Xu @ 2016-12-23  1:22 UTC (permalink / raw)
  To: jikos, benjamin.tissoires, srinivas.pandruvada, arnd, gregkh,
	andriy.shevchenko
  Cc: linux-input, linux-kernel, Even Xu

Add helper ishtp_cl_tx_empty() and ishtp_cl_rx_get_rb() to hide internal
details from callers, who needs this functionality.

Signed-off-by: Even Xu <even.xu@intel.com>
Reviewed-by: Andriy Shevchenko <andriy.shevchenko@intel.com>
Acked-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>
---
 drivers/hid/intel-ish-hid/ishtp/client-buffers.c | 45 ++++++++++++++++++++++++
 drivers/hid/intel-ish-hid/ishtp/client.h         |  2 ++
 2 files changed, 47 insertions(+)

diff --git a/drivers/hid/intel-ish-hid/ishtp/client-buffers.c b/drivers/hid/intel-ish-hid/ishtp/client-buffers.c
index b9b917d..12d6130 100644
--- a/drivers/hid/intel-ish-hid/ishtp/client-buffers.c
+++ b/drivers/hid/intel-ish-hid/ishtp/client-buffers.c
@@ -255,3 +255,48 @@ int ishtp_cl_io_rb_recycle(struct ishtp_cl_rb *rb)
 	return	rets;
 }
 EXPORT_SYMBOL(ishtp_cl_io_rb_recycle);
+
+/**
+ * ishtp_cl_tx_empty() -test whether client device tx buffer is empty
+ * @cl: Pointer to client device instance
+ *
+ * Look client device tx buffer list, and check whether this list is empty
+ *
+ * Return: true if client tx buffer list is empty else false
+ */
+bool ishtp_cl_tx_empty(struct ishtp_cl *cl)
+{
+	int tx_list_empty;
+	unsigned long tx_flags;
+
+	spin_lock_irqsave(&cl->tx_list_spinlock, tx_flags);
+	tx_list_empty = list_empty(&cl->tx_list.list);
+	spin_unlock_irqrestore(&cl->tx_list_spinlock, tx_flags);
+
+	return !!tx_list_empty;
+}
+EXPORT_SYMBOL(ishtp_cl_tx_empty);
+
+/**
+ * ishtp_cl_rx_get_rb() -Get a rb from client device rx buffer list
+ * @cl: Pointer to client device instance
+ *
+ * Check client device in-processing buffer list and get a rb from it.
+ *
+ * Return: rb pointer if buffer list isn't empty else NULL
+ */
+struct ishtp_cl_rb *ishtp_cl_rx_get_rb(struct ishtp_cl *cl)
+{
+	unsigned long rx_flags;
+	struct ishtp_cl_rb *rb;
+
+	spin_lock_irqsave(&cl->in_process_spinlock, rx_flags);
+	rb = list_first_entry_or_null(&cl->in_process_list.list,
+				struct ishtp_cl_rb, list);
+	if (rb)
+		list_del_init(&rb->list);
+	spin_unlock_irqrestore(&cl->in_process_spinlock, rx_flags);
+
+	return rb;
+}
+EXPORT_SYMBOL(ishtp_cl_rx_get_rb);
diff --git a/drivers/hid/intel-ish-hid/ishtp/client.h b/drivers/hid/intel-ish-hid/ishtp/client.h
index 444d069..8ab2833 100644
--- a/drivers/hid/intel-ish-hid/ishtp/client.h
+++ b/drivers/hid/intel-ish-hid/ishtp/client.h
@@ -178,5 +178,7 @@ int ishtp_cl_flush_queues(struct ishtp_cl *cl);
 
 /* exported functions from ISHTP client buffer management scope */
 int ishtp_cl_io_rb_recycle(struct ishtp_cl_rb *rb);
+bool ishtp_cl_tx_empty(struct ishtp_cl *cl);
+struct ishtp_cl_rb *ishtp_cl_rx_get_rb(struct ishtp_cl *cl);
 
 #endif /* _ISHTP_CLIENT_H_ */
-- 
2.7.4

^ permalink raw reply related	[flat|nested] 19+ messages in thread

* [PATCH 4/7] hid: intel-ish-hid: use helper function to access client buffer
  2016-12-23  1:22 [PATCH 1/7] hid: intel-ish-hid: ishtp: add helper function for driver data get/set Even Xu
  2016-12-23  1:22 ` [PATCH 2/7] hid: intel-ish-hid: use helper function for private driver data set/get Even Xu
  2016-12-23  1:22 ` [PATCH 3/7] hid: intel-ish-hid: ishtp: add helper functions for client buffer operation Even Xu
@ 2016-12-23  1:22 ` Even Xu
  2016-12-23  1:22 ` [PATCH 5/7] hid: intel-ish-hid: ishtp: add helper function for client search Even Xu
                   ` (2 subsequent siblings)
  5 siblings, 0 replies; 19+ messages in thread
From: Even Xu @ 2016-12-23  1:22 UTC (permalink / raw)
  To: jikos, benjamin.tissoires, srinivas.pandruvada, arnd, gregkh,
	andriy.shevchenko
  Cc: linux-input, linux-kernel, Even Xu

ishtp bus driver exposed helper functions for client buffer accessing,
so change to use these functions in ishtp-hid-client driver to avoid
access client buffer directly.

Signed-off-by: Even Xu <even.xu@intel.com>
Reviewed-by: Andriy Shevchenko <andriy.shevchenko@intel.com>
Acked-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>
---
 drivers/hid/intel-ish-hid/ishtp-hid-client.c | 13 +------------
 1 file changed, 1 insertion(+), 12 deletions(-)

diff --git a/drivers/hid/intel-ish-hid/ishtp-hid-client.c b/drivers/hid/intel-ish-hid/ishtp-hid-client.c
index 3a3203d..0a7270c 100644
--- a/drivers/hid/intel-ish-hid/ishtp-hid-client.c
+++ b/drivers/hid/intel-ish-hid/ishtp-hid-client.c
@@ -324,20 +324,11 @@ static void ish_cl_event_cb(struct ishtp_cl_device *device)
 	struct ishtp_cl	*hid_ishtp_cl = ishtp_get_drvdata(device);
 	struct ishtp_cl_rb *rb_in_proc;
 	size_t r_length;
-	unsigned long flags;
 
 	if (!hid_ishtp_cl)
 		return;
 
-	spin_lock_irqsave(&hid_ishtp_cl->in_process_spinlock, flags);
-	while (!list_empty(&hid_ishtp_cl->in_process_list.list)) {
-		rb_in_proc = list_entry(
-			hid_ishtp_cl->in_process_list.list.next,
-			struct ishtp_cl_rb, list);
-		list_del_init(&rb_in_proc->list);
-		spin_unlock_irqrestore(&hid_ishtp_cl->in_process_spinlock,
-			flags);
-
+	while ((rb_in_proc = ishtp_cl_rx_get_rb(hid_ishtp_cl)) != NULL) {
 		if (!rb_in_proc->buffer.data)
 			return;
 
@@ -347,9 +338,7 @@ static void ish_cl_event_cb(struct ishtp_cl_device *device)
 		process_recv(hid_ishtp_cl, rb_in_proc->buffer.data, r_length);
 
 		ishtp_cl_io_rb_recycle(rb_in_proc);
-		spin_lock_irqsave(&hid_ishtp_cl->in_process_spinlock, flags);
 	}
-	spin_unlock_irqrestore(&hid_ishtp_cl->in_process_spinlock, flags);
 }
 
 /**
-- 
2.7.4

^ permalink raw reply related	[flat|nested] 19+ messages in thread

* [PATCH 5/7] hid: intel-ish-hid: ishtp: add helper function for client search
  2016-12-23  1:22 [PATCH 1/7] hid: intel-ish-hid: ishtp: add helper function for driver data get/set Even Xu
                   ` (2 preceding siblings ...)
  2016-12-23  1:22 ` [PATCH 4/7] hid: intel-ish-hid: use helper function to access client buffer Even Xu
@ 2016-12-23  1:22 ` Even Xu
  2016-12-23  1:22 ` [PATCH 6/7] hid: intel-ish-hid: use helper function to search client id Even Xu
  2016-12-23  1:22 ` [PATCH 7/7] misc: intel-ish-client: add intel ishtp clients driver Even Xu
  5 siblings, 0 replies; 19+ messages in thread
From: Even Xu @ 2016-12-23  1:22 UTC (permalink / raw)
  To: jikos, benjamin.tissoires, srinivas.pandruvada, arnd, gregkh,
	andriy.shevchenko
  Cc: linux-input, linux-kernel, Even Xu

Add helper function ishtp_fw_cl_get_client() for client driver searching
client information to hide internal details from callers.

Signed-off-by: Even Xu <even.xu@intel.com>
Reviewed-by: Andriy Shevchenko <andriy.shevchenko@intel.com>
Acked-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>
---
 drivers/hid/intel-ish-hid/ishtp/bus.c | 25 +++++++++++++++++++++++++
 drivers/hid/intel-ish-hid/ishtp/bus.h |  2 ++
 2 files changed, 27 insertions(+)

diff --git a/drivers/hid/intel-ish-hid/ishtp/bus.c b/drivers/hid/intel-ish-hid/ishtp/bus.c
index 40e13f1..f07af7a 100644
--- a/drivers/hid/intel-ish-hid/ishtp/bus.c
+++ b/drivers/hid/intel-ish-hid/ishtp/bus.c
@@ -149,6 +149,31 @@ int ishtp_fw_cl_by_uuid(struct ishtp_device *dev, const uuid_le *uuid)
 EXPORT_SYMBOL(ishtp_fw_cl_by_uuid);
 
 /**
+ * ishtp_fw_cl_get_client() - return client information to client
+ * @dev: the ishtp device structure
+ * @uuid: uuid of the client to search
+ *
+ * Search firmware client using UUID and reture related client information.
+ *
+ * Return: pointer of client information on success, NULL on failure.
+ */
+struct ishtp_fw_client *ishtp_fw_cl_get_client(struct ishtp_device *dev,
+						const uuid_le *uuid)
+{
+	int i;
+	unsigned long flags;
+
+	spin_lock_irqsave(&dev->fw_clients_lock, flags);
+	i = ishtp_fw_cl_by_uuid(dev, uuid);
+	spin_unlock_irqrestore(&dev->fw_clients_lock, flags);
+	if (i < 0 || dev->fw_clients[i].props.fixed_address)
+		return NULL;
+
+	return &dev->fw_clients[i];
+}
+EXPORT_SYMBOL(ishtp_fw_cl_get_client);
+
+/**
  * ishtp_fw_cl_by_id() - return index to fw_clients for client_id
  * @dev: the ishtp device structure
  * @client_id: fw client id to search
diff --git a/drivers/hid/intel-ish-hid/ishtp/bus.h b/drivers/hid/intel-ish-hid/ishtp/bus.h
index 8888331..b8a5bcc 100644
--- a/drivers/hid/intel-ish-hid/ishtp/bus.h
+++ b/drivers/hid/intel-ish-hid/ishtp/bus.h
@@ -113,5 +113,7 @@ void	ishtp_cl_driver_unregister(struct ishtp_cl_driver *driver);
 int	ishtp_register_event_cb(struct ishtp_cl_device *device,
 				void (*read_cb)(struct ishtp_cl_device *));
 int	ishtp_fw_cl_by_uuid(struct ishtp_device *dev, const uuid_le *cuuid);
+struct	ishtp_fw_client *ishtp_fw_cl_get_client(struct ishtp_device *dev,
+						const uuid_le *uuid);
 
 #endif /* _LINUX_ISHTP_CL_BUS_H */
-- 
2.7.4

^ permalink raw reply related	[flat|nested] 19+ messages in thread

* [PATCH 6/7] hid: intel-ish-hid: use helper function to search client id
  2016-12-23  1:22 [PATCH 1/7] hid: intel-ish-hid: ishtp: add helper function for driver data get/set Even Xu
                   ` (3 preceding siblings ...)
  2016-12-23  1:22 ` [PATCH 5/7] hid: intel-ish-hid: ishtp: add helper function for client search Even Xu
@ 2016-12-23  1:22 ` Even Xu
  2016-12-23  1:22 ` [PATCH 7/7] misc: intel-ish-client: add intel ishtp clients driver Even Xu
  5 siblings, 0 replies; 19+ messages in thread
From: Even Xu @ 2016-12-23  1:22 UTC (permalink / raw)
  To: jikos, benjamin.tissoires, srinivas.pandruvada, arnd, gregkh,
	andriy.shevchenko
  Cc: linux-input, linux-kernel, Even Xu

ishtp exposed helper ishtp_fw_cl_get_client() function for client
information searching, so switch to use it.

Signed-off-by: Even Xu <even.xu@intel.com>
Reviewed-by: Andriy Shevchenko <andriy.shevchenko@intel.com>
Acked-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>
---
 drivers/hid/intel-ish-hid/ishtp-hid-client.c | 14 ++++++--------
 1 file changed, 6 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 0a7270c..34847ed 100644
--- a/drivers/hid/intel-ish-hid/ishtp-hid-client.c
+++ b/drivers/hid/intel-ish-hid/ishtp-hid-client.c
@@ -631,8 +631,8 @@ static int ishtp_get_report_descriptor(struct ishtp_cl *hid_ishtp_cl,
 static int hid_ishtp_cl_init(struct ishtp_cl *hid_ishtp_cl, int reset)
 {
 	struct ishtp_device *dev;
-	unsigned long flags;
 	struct ishtp_cl_data *client_data = hid_ishtp_cl->client_data;
+	struct ishtp_fw_client *fw_client;
 	int i;
 	int rv;
 
@@ -654,16 +654,14 @@ static int hid_ishtp_cl_init(struct ishtp_cl *hid_ishtp_cl, int reset)
 	hid_ishtp_cl->rx_ring_size = HID_CL_RX_RING_SIZE;
 	hid_ishtp_cl->tx_ring_size = HID_CL_TX_RING_SIZE;
 
-	spin_lock_irqsave(&dev->fw_clients_lock, flags);
-	i = ishtp_fw_cl_by_uuid(dev, &hid_ishtp_guid);
-	if (i < 0) {
-		spin_unlock_irqrestore(&dev->fw_clients_lock, flags);
+	fw_client = ishtp_fw_cl_get_client(dev, &hid_ishtp_guid);
+	if (!fw_client) {
 		dev_err(&client_data->cl_device->dev,
 			"ish client uuid not found\n");
-		return i;
+		return -ENOENT;
 	}
-	hid_ishtp_cl->fw_client_id = dev->fw_clients[i].client_id;
-	spin_unlock_irqrestore(&dev->fw_clients_lock, flags);
+
+	hid_ishtp_cl->fw_client_id = fw_client->client_id;
 	hid_ishtp_cl->state = ISHTP_CL_CONNECTING;
 
 	rv = ishtp_cl_connect(hid_ishtp_cl);
-- 
2.7.4

^ permalink raw reply related	[flat|nested] 19+ messages in thread

* [PATCH 7/7] misc: intel-ish-client: add intel ishtp clients driver
  2016-12-23  1:22 [PATCH 1/7] hid: intel-ish-hid: ishtp: add helper function for driver data get/set Even Xu
                   ` (4 preceding siblings ...)
  2016-12-23  1:22 ` [PATCH 6/7] hid: intel-ish-hid: use helper function to search client id Even Xu
@ 2016-12-23  1:22 ` Even Xu
  2017-01-03  9:54   ` Jiri Kosina
                     ` (3 more replies)
  5 siblings, 4 replies; 19+ messages in thread
From: Even Xu @ 2016-12-23  1:22 UTC (permalink / raw)
  To: jikos, benjamin.tissoires, srinivas.pandruvada, arnd, gregkh,
	andriy.shevchenko
  Cc: linux-input, linux-kernel, Even Xu

Intel ISHFW supports many different clients, in
hid/intel-ish-hid/ishtp bus driver, it creates following client devices:
HID client:
	interface of sensor configure and sensor event report.
SMHI client:
	interface of sensor calibration, ISHFW debug, ISHFW performance
	analysis and manufacture support.
Trace client:
	interface of ISHFW debug log output.
Trace configure client:
	interface of ISHFW debug log configuration, such as output port,
	log level, filter.
ISHFW loader client:
	interface of customized ISHFW loader.
HID client has been handle by hid/intel-ish-hid/intel-ishtp-hid client
driver, and rest of the clients export interface using miscellaneous
drivers. This interface is used by user space tools for debugging and
calibration of sensors.

Signed-off-by: Even Xu <even.xu@intel.com>
Reviewed-by: Andriy Shevchenko <andriy.shevchenko@intel.com>
Reviewed-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>
---
 drivers/misc/Kconfig                               |   1 +
 drivers/misc/Makefile                              |   1 +
 drivers/misc/intel-ish-client/Kconfig              |  15 +
 drivers/misc/intel-ish-client/Makefile             |   8 +
 .../misc/intel-ish-client/intel-ishtp-clients.c    | 884 +++++++++++++++++++++
 include/uapi/linux/intel-ishtp-clients.h           |  73 ++
 6 files changed, 982 insertions(+)
 create mode 100644 drivers/misc/intel-ish-client/Kconfig
 create mode 100644 drivers/misc/intel-ish-client/Makefile
 create mode 100644 drivers/misc/intel-ish-client/intel-ishtp-clients.c
 create mode 100644 include/uapi/linux/intel-ishtp-clients.h

diff --git a/drivers/misc/Kconfig b/drivers/misc/Kconfig
index 64971ba..a89849f 100644
--- a/drivers/misc/Kconfig
+++ b/drivers/misc/Kconfig
@@ -778,4 +778,5 @@ source "drivers/misc/mic/Kconfig"
 source "drivers/misc/genwqe/Kconfig"
 source "drivers/misc/echo/Kconfig"
 source "drivers/misc/cxl/Kconfig"
+source "drivers/misc/intel-ish-client/Kconfig"
 endmenu
diff --git a/drivers/misc/Makefile b/drivers/misc/Makefile
index 3198336..c54015d 100644
--- a/drivers/misc/Makefile
+++ b/drivers/misc/Makefile
@@ -53,6 +53,7 @@ obj-$(CONFIG_ECHO)		+= echo/
 obj-$(CONFIG_VEXPRESS_SYSCFG)	+= vexpress-syscfg.o
 obj-$(CONFIG_CXL_BASE)		+= cxl/
 obj-$(CONFIG_PANEL)             += panel.o
+obj-$(CONFIG_INTEL_ISH_CLIENT)	+= intel-ish-client/
 
 lkdtm-$(CONFIG_LKDTM)		+= lkdtm_core.o
 lkdtm-$(CONFIG_LKDTM)		+= lkdtm_bugs.o
diff --git a/drivers/misc/intel-ish-client/Kconfig b/drivers/misc/intel-ish-client/Kconfig
new file mode 100644
index 0000000..6fa9cc0
--- /dev/null
+++ b/drivers/misc/intel-ish-client/Kconfig
@@ -0,0 +1,15 @@
+menu "Intel ISH Client support"
+	depends on INTEL_ISH_HID
+
+config INTEL_ISH_CLIENT
+	tristate "Intel Integrated Sensor Hub Client"
+	help
+	  The Integrated Sensor Hub (ISH) supports many clients, Intel ISH client
+	  driver supports following clients:
+	  SMHI client: calibration & perfermance & menufacture tool interface
+	  trace configure client: ISHFW trace parameter configure interface
+	  trace tool client: ISHFW trace log output interface
+	  loader client: loading customized ISHFW interface
+
+	  Say Y here if you want to support Intel ISH clients. If unsure, say N.
+endmenu
diff --git a/drivers/misc/intel-ish-client/Makefile b/drivers/misc/intel-ish-client/Makefile
new file mode 100644
index 0000000..29a5461
--- /dev/null
+++ b/drivers/misc/intel-ish-client/Makefile
@@ -0,0 +1,8 @@
+#
+# Makefile - Intel ISH client driver
+# Copyright (c) 2014-2016, Intel Corporation.
+#
+#
+obj-$(CONFIG_INTEL_ISH_CLIENT) += intel-ishtp-clients.o
+
+ccflags-y += -I$(srctree)/drivers/hid/intel-ish-hid/ishtp
diff --git a/drivers/misc/intel-ish-client/intel-ishtp-clients.c b/drivers/misc/intel-ish-client/intel-ishtp-clients.c
new file mode 100644
index 0000000..d2b3ffd
--- /dev/null
+++ b/drivers/misc/intel-ish-client/intel-ishtp-clients.c
@@ -0,0 +1,884 @@
+/*
+ * ISHTP clients driver
+ *
+ * Copyright (c) 2016, Intel Corporation.
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms and conditions of the GNU General Public License,
+ * version 2, as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License for
+ * more details.
+ */
+
+#include <linux/capability.h>
+#include <linux/delay.h>
+#include <linux/device.h>
+#include <linux/errno.h>
+#include <linux/fs.h>
+#include <linux/interrupt.h>
+#include <linux/intel-ishtp-clients.h>
+#include <linux/ioctl.h>
+#include <linux/kernel.h>
+#include <linux/miscdevice.h>
+#include <linux/module.h>
+#include <linux/slab.h>
+#include <linux/types.h>
+#include <linux/uuid.h>
+#include <linux/uaccess.h>
+
+#include "ishtp-dev.h"
+#include "client.h"
+
+/*
+ * ISH client misc driver structure
+ */
+struct ishtp_cl_miscdev {
+	struct miscdevice cl_miscdev;
+	struct ishtp_cl_device *cl_device;
+	struct ishtp_cl *cl;
+
+	/* Wait queue for waiting ISHFW event/message */
+	wait_queue_head_t read_wait;
+	/* Read buffer, will point to available ISHFW message */
+	struct ishtp_cl_rb *read_rb;
+
+	/*
+	 * cl member can be freed/changed by ISHFW reset and release() calling,
+	 * so must pay attention to protect cl while try to access it. This
+	 * mutex is used to protect cl member.
+	 */
+	struct mutex cl_mutex;
+
+	struct work_struct reset_work;
+};
+
+/* ISH client GUIDs */
+/* SMHI client UUID: bb579a2e-cc54-4450-b1d0-5e7520dcad25 */
+static const uuid_le ishtp_smhi_guid =
+			UUID_LE(0xbb579a2e, 0xcc54, 0x4450,
+				0xb1, 0xd0, 0x5e, 0x75, 0x20, 0xdc, 0xad, 0x25);
+
+/* Trace log client UUID: c1cc78b9-b693-4e54-9191-5169cb027c25 */
+static const uuid_le ishtp_trace_guid =
+			UUID_LE(0xc1cc78b9, 0xb693, 0x4e54,
+				0x91, 0x91, 0x51, 0x69, 0xcb, 0x02, 0x7c, 0x25);
+
+/* Trace config client UUID: 1f050626-d505-4e94-b189-535d7de19cf2 */
+static const uuid_le ishtp_traceconfig_guid =
+			UUID_LE(0x1f050626, 0xd505, 0x4e94,
+				0xb1, 0x89, 0x53, 0x5d, 0x7d, 0xe1, 0x9c, 0xf2);
+
+/* ISHFW loader client UUID: c804d06a-55bd-4ea7-aded-1e31228c76dc */
+static const uuid_le ishtp_loader_guid =
+			UUID_LE(0xc804d06a, 0x55bd, 0x4ea7,
+				0xad, 0xed, 0x1e, 0x31, 0x22, 0x8c, 0x76, 0xdc);
+
+static int ishtp_cl_open(struct inode *inode, struct file *file)
+{
+	struct miscdevice *misc = file->private_data;
+	struct ishtp_cl *cl;
+	struct ishtp_device *dev;
+	struct ishtp_cl_miscdev *ishtp_cl_misc;
+	int ret;
+
+	/* Non-blocking semantics are not supported */
+	if (file->f_flags & O_NONBLOCK)
+		return -EINVAL;
+
+	ishtp_cl_misc = container_of(misc,
+				struct ishtp_cl_miscdev, cl_miscdev);
+	if (!ishtp_cl_misc || !ishtp_cl_misc->cl_device)
+		return -ENODEV;
+
+	dev = ishtp_cl_misc->cl_device->ishtp_dev;
+	if (!dev)
+		return -ENODEV;
+
+	mutex_lock(&ishtp_cl_misc->cl_mutex);
+
+	/*
+	 * Every client only supports one opened instance
+	 * at the sametime.
+	 */
+	if (ishtp_cl_misc->cl) {
+		ret = -EBUSY;
+		goto out_unlock;
+	}
+
+	cl = ishtp_cl_allocate(dev);
+	if (!cl) {
+		ret = -ENOMEM;
+		goto out_free;
+	}
+
+	if (dev->dev_state != ISHTP_DEV_ENABLED) {
+		ret = -ENODEV;
+		goto out_free;
+	}
+
+	ret = ishtp_cl_link(cl, ISHTP_HOST_CLIENT_ID_ANY);
+	if (ret)
+		goto out_free;
+
+	ishtp_cl_misc->cl = cl;
+
+	file->private_data = ishtp_cl_misc;
+
+	mutex_unlock(&ishtp_cl_misc->cl_mutex);
+
+	return nonseekable_open(inode, file);
+
+out_free:
+	kfree(cl);
+out_unlock:
+	mutex_unlock(&ishtp_cl_misc->cl_mutex);
+	return ret;
+}
+
+#define WAIT_FOR_SEND_SLICE_MS		100
+#define WAIT_FOR_SEND_COUNT		10
+
+static int ishtp_cl_release(struct inode *inode, struct file *file)
+{
+	struct ishtp_cl_miscdev *ishtp_cl_misc = file->private_data;
+	struct ishtp_cl *cl;
+	struct ishtp_cl_rb *rb;
+	struct ishtp_device *dev;
+	int try = WAIT_FOR_SEND_COUNT;
+	int ret;
+
+	mutex_lock(&ishtp_cl_misc->cl_mutex);
+
+	/* Wake up from waiting if anyone wait on it */
+	wake_up_interruptible(&ishtp_cl_misc->read_wait);
+
+	cl = ishtp_cl_misc->cl;
+	dev = cl->dev;
+
+	/*
+	 * May happen if device sent FW reset or was intentionally
+	 * halted by host SW. The client is then invalid.
+	 */
+	if ((dev->dev_state == ISHTP_DEV_ENABLED) &&
+			(cl->state == ISHTP_CL_CONNECTED)) {
+		/*
+		 * Check and wait 1s for message in tx_list to be sent.
+		 */
+		do {
+			if (!ishtp_cl_tx_empty(cl))
+				msleep_interruptible(WAIT_FOR_SEND_SLICE_MS);
+			else
+				break;
+		} while (--try);
+
+		cl->state = ISHTP_CL_DISCONNECTING;
+		ret = ishtp_cl_disconnect(cl);
+	}
+
+	ishtp_cl_unlink(cl);
+	ishtp_cl_flush_queues(cl);
+	/* Disband and free all Tx and Rx client-level rings */
+	ishtp_cl_free(cl);
+
+	ishtp_cl_misc->cl = NULL;
+
+	rb = ishtp_cl_misc->read_rb;
+	if (rb) {
+		ishtp_cl_io_rb_recycle(rb);
+		ishtp_cl_misc->read_rb = NULL;
+	}
+
+	file->private_data = NULL;
+
+	mutex_unlock(&ishtp_cl_misc->cl_mutex);
+
+	return ret;
+}
+
+static ssize_t ishtp_cl_read(struct file *file, char __user *ubuf,
+			size_t length, loff_t *offset)
+{
+	struct ishtp_cl_miscdev *ishtp_cl_misc = file->private_data;
+	struct ishtp_cl *cl;
+	struct ishtp_cl_rb *rb;
+	struct ishtp_device *dev;
+	int ret = 0;
+
+	/* Non-blocking semantics are not supported */
+	if (file->f_flags & O_NONBLOCK)
+		return -EINVAL;
+
+	mutex_lock(&ishtp_cl_misc->cl_mutex);
+
+	cl = ishtp_cl_misc->cl;
+
+	/*
+	 * ISHFW reset will cause cl be freed and re-allocated.
+	 * So must make sure cl is re-allocated successfully.
+	 */
+	if (!cl || !cl->dev) {
+		ret = -ENODEV;
+		goto out_unlock;
+	}
+
+	dev = cl->dev;
+	if (dev->dev_state != ISHTP_DEV_ENABLED) {
+		ret = -ENODEV;
+		goto out_unlock;
+	}
+
+	if (ishtp_cl_misc->read_rb)
+		goto get_rb;
+
+	rb = ishtp_cl_rx_get_rb(cl);
+	if (rb)
+		goto copy_buffer;
+
+	/*
+	 * Release mutex for other operation can be processed parallelly
+	 * during waiting.
+	 */
+	mutex_unlock(&ishtp_cl_misc->cl_mutex);
+
+	if (wait_event_interruptible(ishtp_cl_misc->read_wait,
+			ishtp_cl_misc->read_rb != NULL)) {
+		dev_err(&ishtp_cl_misc->cl_device->dev,
+			"Wake up not successful;"
+			"signal pending = %d signal = %08lX\n",
+			signal_pending(current),
+			current->pending.signal.sig[0]);
+		return -ERESTARTSYS;
+	}
+
+	mutex_lock(&ishtp_cl_misc->cl_mutex);
+
+	/*
+	 * waitqueue can be woken up in many cases, so must check
+	 * if dev and cl is still available.
+	 */
+	if (dev->dev_state != ISHTP_DEV_ENABLED) {
+		ret = -ENODEV;
+		goto out_unlock;
+	}
+
+	cl = ishtp_cl_misc->cl;
+	if (!cl) {
+		ret = -ENODEV;
+		goto out_unlock;
+	}
+
+	if (cl->state == ISHTP_CL_INITIALIZING ||
+		cl->state == ISHTP_CL_DISCONNECTED ||
+		cl->state == ISHTP_CL_DISCONNECTING) {
+		ret = -EBUSY;
+		goto out_unlock;
+	}
+
+get_rb:
+	rb = ishtp_cl_misc->read_rb;
+	if (!rb) {
+		ret = -ENODEV;
+		goto out_unlock;
+	}
+
+copy_buffer:
+	/* Now copy the data to user space */
+	if (!length || !ubuf || *offset > rb->buf_idx) {
+		ret = -EMSGSIZE;
+		goto out_unlock;
+	}
+
+	/*
+	 * length is being truncated, however buf_idx may
+	 * point beyond that.
+	 */
+	length = min_t(size_t, length, rb->buf_idx - *offset);
+
+	if (copy_to_user(ubuf, rb->buffer.data + *offset, length)) {
+		ret = -EFAULT;
+		goto out_unlock;
+	}
+
+	*offset += length;
+	if ((unsigned long)*offset < rb->buf_idx)
+		goto out_unlock;
+
+	ishtp_cl_io_rb_recycle(rb);
+	ishtp_cl_misc->read_rb = NULL;
+	*offset = 0;
+
+out_unlock:
+	mutex_unlock(&ishtp_cl_misc->cl_mutex);
+	return ret < 0 ? ret : length;
+}
+
+static ssize_t ishtp_cl_write(struct file *file, const char __user *ubuf,
+	size_t length, loff_t *offset)
+{
+	struct ishtp_cl_miscdev *ishtp_cl_misc = file->private_data;
+	struct ishtp_cl *cl;
+	void *write_buf;
+	struct ishtp_device *dev;
+	int ret;
+
+	/* Non-blocking semantics are not supported */
+	if (file->f_flags & O_NONBLOCK) {
+		ret = -EINVAL;
+		goto out_unlock;
+	}
+
+	mutex_lock(&ishtp_cl_misc->cl_mutex);
+
+	cl = ishtp_cl_misc->cl;
+
+	/*
+	 * ISHFW reset will cause cl be freed and re-allocated.
+	 * So must make sure cl is re-allocated successfully.
+	 */
+	if (!cl || !cl->dev) {
+		ret = -ENODEV;
+		goto out_unlock;
+	}
+
+	dev = cl->dev;
+
+	if (dev->dev_state != ISHTP_DEV_ENABLED) {
+		ret = -ENODEV;
+		goto out_unlock;
+	}
+
+	if (cl->state != ISHTP_CL_CONNECTED) {
+		dev_err(&ishtp_cl_misc->cl_device->dev,
+			"host client = %d isn't connected to fw client = %d\n",
+			cl->host_client_id, cl->fw_client_id);
+		ret = -ENODEV;
+		goto out_unlock;
+	}
+
+	if (length <= 0 || length > cl->device->fw_client->props.max_msg_length) {
+		ret = -EMSGSIZE;
+		goto out_unlock;
+	}
+
+	write_buf = memdup_user(ubuf, length);
+	if (IS_ERR(write_buf)) {
+		ret = PTR_ERR(write_buf);
+		goto out_unlock;
+	}
+
+	ret = ishtp_cl_send(cl, write_buf, length);
+
+	kfree(write_buf);
+
+out_unlock:
+	mutex_unlock(&ishtp_cl_misc->cl_mutex);
+
+	return ret < 0 ? ret : length;
+}
+
+static int ishtp_cl_ioctl_connect_client(struct file *file,
+	struct ishtp_connect_client_data *data)
+{
+	struct ishtp_cl_miscdev *ishtp_cl_misc = file->private_data;
+	struct ishtp_device *dev;
+	struct ishtp_client *client;
+	struct ishtp_cl_device *cl_device;
+	struct ishtp_cl *cl = ishtp_cl_misc->cl;
+	struct ishtp_fw_client *fw_client;
+
+	if (!cl || !cl->dev)
+		return -ENODEV;
+
+	dev = cl->dev;
+
+	if (dev->dev_state != ISHTP_DEV_ENABLED)
+		return -ENODEV;
+
+	if (cl->state != ISHTP_CL_INITIALIZING &&
+			cl->state != ISHTP_CL_DISCONNECTED)
+		return -EBUSY;
+
+	cl_device = ishtp_cl_misc->cl_device;
+
+	if (uuid_le_cmp(data->in_client_uuid,
+			cl_device->fw_client->props.protocol_name) != 0) {
+		dev_err(&ishtp_cl_misc->cl_device->dev,
+			"Required uuid don't match current client uuid\n");
+		return -EFAULT;
+	}
+
+	/* Find the fw client we're trying to connect to */
+	fw_client = ishtp_fw_cl_get_client(dev, &data->in_client_uuid);
+	if (!fw_client) {
+		dev_err(&ishtp_cl_misc->cl_device->dev,
+			"Don't find current client uuid\n");
+		return -ENOENT;
+	}
+
+	cl->fw_client_id = fw_client->client_id;
+	cl->state = ISHTP_CL_CONNECTING;
+
+	/* Prepare the output buffer */
+	client = &data->out_client_properties;
+	client->max_msg_length = fw_client->props.max_msg_length;
+	client->protocol_version = fw_client->props.protocol_version;
+
+	return ishtp_cl_connect(cl);
+}
+
+static long ishtp_cl_ioctl(struct file *file, unsigned int cmd,
+			unsigned long data)
+{
+	struct ishtp_cl_miscdev *ishtp_cl_misc = file->private_data;
+	struct ishtp_cl *cl;
+	struct ishtp_device *dev;
+	struct ishtp_connect_client_data *connect_data;
+	char fw_stat_buf[20];
+	unsigned int ring_size;
+	int ret = 0;
+
+	if (!capable(CAP_SYS_ADMIN))
+		return -EPERM;
+
+	mutex_lock(&ishtp_cl_misc->cl_mutex);
+
+	cl = ishtp_cl_misc->cl;
+
+	/*
+	 * ISHFW reset will cause cl be freed and re-allocated.
+	 * So must make sure cl is re-allocated successfully.
+	 */
+	if (!cl || !cl->dev) {
+		mutex_unlock(&ishtp_cl_misc->cl_mutex);
+		return -ENODEV;
+	}
+
+	dev = cl->dev;
+
+	switch (cmd) {
+	case IOCTL_ISH_HW_RESET:
+		ret = ish_hw_reset(dev);
+		break;
+
+	case IOCTL_ISHTP_SET_RX_FIFO_SIZE:
+		ring_size = data;
+
+		if (ring_size > CL_MAX_RX_RING_SIZE) {
+			ret = -EINVAL;
+			break;
+		}
+
+		if (cl->state != ISHTP_CL_INITIALIZING) {
+			ret = -EBUSY;
+			break;
+		}
+
+		cl->rx_ring_size = ring_size;
+		break;
+
+	case IOCTL_ISHTP_SET_TX_FIFO_SIZE:
+		ring_size = data;
+
+		if (ring_size > CL_MAX_TX_RING_SIZE) {
+			ret = -EINVAL;
+			break;
+		}
+
+		if (cl->state != ISHTP_CL_INITIALIZING) {
+			ret = -EBUSY;
+			break;
+		}
+
+		cl->tx_ring_size = ring_size;
+		break;
+
+	case IOCTL_ISH_GET_FW_STATUS:
+		if (!data) {
+			ret = -ENOMEM;
+			break;
+		}
+
+		snprintf(fw_stat_buf, sizeof(fw_stat_buf),
+			"%08X\n", dev->ops->get_fw_status(dev));
+
+		if (copy_to_user((char __user *)data, fw_stat_buf,
+				strlen(fw_stat_buf))) {
+			ret = -EFAULT;
+			break;
+		}
+
+		ret = strlen(fw_stat_buf);
+		break;
+
+	case IOCTL_ISHTP_CONNECT_CLIENT:
+		if (dev->dev_state != ISHTP_DEV_ENABLED) {
+			ret = -ENODEV;
+			break;
+		}
+
+		connect_data = memdup_user((char __user *)data,
+					sizeof(struct ishtp_connect_client_data));
+		if (IS_ERR(connect_data)) {
+			ret = PTR_ERR(connect_data);
+			break;
+		}
+
+		ret = ishtp_cl_ioctl_connect_client(file, connect_data);
+		if (ret) {
+			kfree(connect_data);
+			break;
+		}
+
+		/* If all is ok, copying the data back to user. */
+		if (copy_to_user((char __user *)data, connect_data,
+				sizeof(struct ishtp_connect_client_data)))
+			ret = -EFAULT;
+
+		kfree(connect_data);
+
+		break;
+
+	default:
+		ret = -EINVAL;
+		break;
+	}
+
+	mutex_unlock(&ishtp_cl_misc->cl_mutex);
+
+	return ret;
+}
+
+/*
+ * File operations structure will be used for ishtp client misc device.
+ */
+static const struct file_operations ishtp_cl_fops = {
+	.owner = THIS_MODULE,
+	.read = ishtp_cl_read,
+	.unlocked_ioctl = ishtp_cl_ioctl,
+	.open = ishtp_cl_open,
+	.release = ishtp_cl_release,
+	.write = ishtp_cl_write,
+	.llseek = no_llseek
+};
+
+/**
+ * ishtp_cl_event_cb() - ISHTP client driver event callback
+ * @cl_device:		ISHTP client device instance
+ *
+ * This function gets called on related event recevied from ISHFW.
+ * It will remove event buffer exists on in_process list to related
+ * client device and wait up client driver to process.
+ */
+static void ishtp_cl_event_cb(struct ishtp_cl_device *cl_device)
+{
+	struct ishtp_cl_miscdev *ishtp_cl_misc;
+	struct ishtp_cl *cl;
+	struct ishtp_cl_rb *rb;
+
+	ishtp_cl_misc = ishtp_get_drvdata(cl_device);
+	if (!ishtp_cl_misc)
+		return;
+
+	mutex_lock(&ishtp_cl_misc->cl_mutex);
+
+	/*
+	 * If this waitqueue is active, cl_mutex is locked by read(), it's safe
+	 * to access ishtp_cl_misc and cl.
+	 */
+	if (waitqueue_active(&ishtp_cl_misc->read_wait)) {
+
+		/*
+		 * If already has read_rb, wake up waitqueue directly.
+		 */
+		if (ishtp_cl_misc->read_rb) {
+			mutex_unlock(&ishtp_cl_misc->cl_mutex);
+			wake_up_interruptible(&ishtp_cl_misc->read_wait);
+			return;
+		}
+
+		cl = ishtp_cl_misc->cl;
+
+		rb = ishtp_cl_rx_get_rb(cl);
+		if (rb)
+			ishtp_cl_misc->read_rb = rb;
+
+		wake_up_interruptible(&ishtp_cl_misc->read_wait);
+	}
+
+	mutex_unlock(&ishtp_cl_misc->cl_mutex);
+}
+
+/**
+ * ishtp_cl_reset_handler() - ISHTP client driver reset work handler
+ * @work:		work struct
+ *
+ * This function gets called on reset workqueue scheduled when ISHFW
+ * reset happen. It will disconnect and remove current ishtp_cl, then
+ * create a new ishtp_cl and re-connect again.
+ */
+static void ishtp_cl_reset_handler(struct work_struct *work)
+{
+	struct ishtp_cl_miscdev *ishtp_cl_misc;
+	struct ishtp_device *dev;
+	struct ishtp_cl_device *cl_device;
+	struct ishtp_cl *cl;
+	struct ishtp_fw_client *fw_client;
+	int ret = 0;
+
+	ishtp_cl_misc = container_of(work,
+			struct ishtp_cl_miscdev, reset_work);
+
+	dev = ishtp_cl_misc->cl_device->ishtp_dev;
+	if (!dev) {
+		dev_err(&ishtp_cl_misc->cl_device->dev,
+			"This cl_device not link to ishtp_dev\n");
+		return;
+	}
+
+	cl_device = ishtp_cl_misc->cl_device;
+
+	mutex_lock(&ishtp_cl_misc->cl_mutex);
+
+	/* Wake up from waiting if anyone wait on it */
+	wake_up_interruptible(&ishtp_cl_misc->read_wait);
+
+	cl = ishtp_cl_misc->cl;
+	if (cl) {
+		ishtp_cl_flush_queues(cl);
+		ishtp_cl_free(cl);
+
+		cl = NULL;
+
+		cl = ishtp_cl_allocate(dev);
+		if (!cl) {
+			dev_err(&ishtp_cl_misc->cl_device->dev,
+				"Allocate ishtp_cl failed\n");
+			ret = -ENOMEM;
+			goto out_unlock;
+		}
+
+		if (dev->dev_state != ISHTP_DEV_ENABLED) {
+			dev_err(&ishtp_cl_misc->cl_device->dev,
+				"Ishtp dev isn't enabled\n");
+			ret = -ENODEV;
+			goto out_free;
+		}
+
+		ret = ishtp_cl_link(cl, ISHTP_HOST_CLIENT_ID_ANY);
+		if (ret) {
+			dev_err(&ishtp_cl_misc->cl_device->dev,
+				"Can not link to ishtp\n");
+			goto out_free;
+		}
+
+		fw_client = ishtp_fw_cl_get_client(dev,
+				&cl_device->fw_client->props.protocol_name);
+		if (!fw_client) {
+			dev_err(&ishtp_cl_misc->cl_device->dev,
+				"Don't find related fw client\n");
+			ret = -ENOENT;
+			goto out_free;
+		}
+
+		cl->fw_client_id = fw_client->client_id;
+		cl->state = ISHTP_CL_CONNECTING;
+
+		ret = ishtp_cl_connect(cl);
+		if (ret) {
+			dev_err(&ishtp_cl_misc->cl_device->dev,
+				"Connect to fw failed\n");
+			goto out_free;
+		}
+
+		ishtp_cl_misc->cl = cl;
+	}
+
+	/* After reset, must register event callback again */
+	ishtp_register_event_cb(cl_device, ishtp_cl_event_cb);
+
+out_free:
+	if (ret) {
+		ishtp_cl_free(cl);
+		ishtp_cl_misc->cl = NULL;
+
+		dev_err(&ishtp_cl_misc->cl_device->dev, "Reset failed\n");
+	}
+
+out_unlock:
+	mutex_unlock(&ishtp_cl_misc->cl_mutex);
+}
+
+/**
+ * ishtp_cl_probe() - ISHTP client driver probe
+ * @cl_device:		ISHTP client device instance
+ *
+ * This function gets called on device create on ISHTP bus
+ *
+ * Return: 0 on success, non zero on error
+ */
+static int ishtp_cl_probe(struct ishtp_cl_device *cl_device)
+{
+	struct ishtp_cl_miscdev *ishtp_cl_misc;
+	int ret;
+
+	if (!cl_device)
+		return -ENODEV;
+
+	ishtp_cl_misc = kzalloc(sizeof(struct ishtp_cl_miscdev),
+				GFP_KERNEL);
+	if (!ishtp_cl_misc)
+		return -ENOMEM;
+
+	if (uuid_le_cmp(ishtp_smhi_guid,
+			cl_device->fw_client->props.protocol_name) == 0) {
+		ishtp_cl_misc->cl_miscdev.name = "ish-smhi";
+	} else if (uuid_le_cmp(ishtp_trace_guid,
+			cl_device->fw_client->props.protocol_name) == 0) {
+		ishtp_cl_misc->cl_miscdev.name = "ish-trace";
+	} else if (uuid_le_cmp(ishtp_traceconfig_guid,
+			cl_device->fw_client->props.protocol_name) == 0) {
+		ishtp_cl_misc->cl_miscdev.name = "ish-tracec";
+	} else if (uuid_le_cmp(ishtp_loader_guid,
+			cl_device->fw_client->props.protocol_name) == 0) {
+		ishtp_cl_misc->cl_miscdev.name = "ish-loader";
+	} else {
+		dev_err(&cl_device->dev, "Not supported client\n");
+		ret = -ENODEV;
+		goto release_mem;
+	}
+
+	ishtp_cl_misc->cl_miscdev.parent = &cl_device->dev;
+	ishtp_cl_misc->cl_miscdev.fops = &ishtp_cl_fops;
+	ishtp_cl_misc->cl_miscdev.minor = MISC_DYNAMIC_MINOR,
+
+	ret = misc_register(&ishtp_cl_misc->cl_miscdev);
+	if (ret) {
+		dev_err(&cl_device->dev, "misc device register failed\n");
+		goto release_mem;
+	}
+
+	ishtp_cl_misc->cl_device = cl_device;
+
+	init_waitqueue_head(&ishtp_cl_misc->read_wait);
+
+	ishtp_set_drvdata(cl_device, ishtp_cl_misc);
+
+	ishtp_get_device(cl_device);
+
+	mutex_init(&ishtp_cl_misc->cl_mutex);
+
+	INIT_WORK(&ishtp_cl_misc->reset_work, ishtp_cl_reset_handler);
+
+	/* Register event callback */
+	ishtp_register_event_cb(cl_device, ishtp_cl_event_cb);
+
+	return 0;
+
+release_mem:
+	kfree(ishtp_cl_misc);
+
+	return ret;
+}
+
+/**
+ * ishtp_cl_remove() - ISHTP client driver remove
+ * @cl_device:		ISHTP client device instance
+ *
+ * This function gets called on device remove on ISHTP bus
+ *
+ * Return: 0
+ */
+static int ishtp_cl_remove(struct ishtp_cl_device *cl_device)
+{
+	struct ishtp_cl_miscdev *ishtp_cl_misc;
+	struct ishtp_cl *cl;
+
+	ishtp_cl_misc = ishtp_get_drvdata(cl_device);
+	if (!ishtp_cl_misc)
+		return -ENODEV;
+
+	if (!ishtp_cl_misc->cl_miscdev.parent)
+		return -ENODEV;
+
+	/* Wake up from waiting if anyone wait on it */
+	wake_up_interruptible(&ishtp_cl_misc->read_wait);
+
+	mutex_lock(&ishtp_cl_misc->cl_mutex);
+
+	cl = ishtp_cl_misc->cl;
+	if (cl) {
+		cl->state = ISHTP_CL_DISCONNECTING;
+		ishtp_cl_disconnect(cl);
+		ishtp_cl_unlink(cl);
+		ishtp_cl_flush_queues(cl);
+		ishtp_cl_free(cl);
+		ishtp_cl_misc->cl = NULL;
+	}
+
+	mutex_unlock(&ishtp_cl_misc->cl_mutex);
+
+	mutex_destroy(&ishtp_cl_misc->cl_mutex);
+
+	misc_deregister(&ishtp_cl_misc->cl_miscdev);
+
+	ishtp_put_device(cl_device);
+
+	kfree(ishtp_cl_misc);
+
+	return 0;
+}
+
+/**
+ * ishtp_cl_reset() - ISHTP client driver reset
+ * @cl_device:		ISHTP client device instance
+ *
+ * This function gets called on device reset on ISHTP bus.
+ * If client is connected, needs to disconnect client and
+ * reconnect again.
+ *
+ * Return: 0
+ */
+static int ishtp_cl_reset(struct ishtp_cl_device *cl_device)
+{
+	struct ishtp_cl_miscdev *ishtp_cl_misc;
+
+	ishtp_cl_misc = ishtp_get_drvdata(cl_device);
+	if (!ishtp_cl_misc) {
+		dev_err(&cl_device->dev, "Client driver not ready yet\n");
+		return -ENODEV;
+	}
+
+	schedule_work(&ishtp_cl_misc->reset_work);
+
+	return 0;
+}
+
+static struct ishtp_cl_driver ishtp_cl_driver = {
+	.name = "ishtp-client",
+	.probe = ishtp_cl_probe,
+	.remove = ishtp_cl_remove,
+	.reset = ishtp_cl_reset,
+};
+
+static int __init ishtp_client_init(void)
+{
+	/* Register ISHTP client device driver with ISHTP Bus */
+	return ishtp_cl_driver_register(&ishtp_cl_driver);
+}
+
+static void __exit ishtp_client_exit(void)
+{
+	ishtp_cl_driver_unregister(&ishtp_cl_driver);
+}
+
+/* To make sure ISHTP bus driver loaded first */
+late_initcall(ishtp_client_init);
+module_exit(ishtp_client_exit);
+
+MODULE_DESCRIPTION("ISH ISHTP client driver");
+MODULE_AUTHOR("Even Xu <even.xu@intel.com>");
+MODULE_LICENSE("GPL");
+MODULE_ALIAS("ishtp:*");
diff --git a/include/uapi/linux/intel-ishtp-clients.h b/include/uapi/linux/intel-ishtp-clients.h
new file mode 100644
index 0000000..792500a
--- /dev/null
+++ b/include/uapi/linux/intel-ishtp-clients.h
@@ -0,0 +1,73 @@
+/*
+ * Intel ISHTP Clients Interface Header
+ *
+ * Copyright (c) 2016, Intel Corporation.
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms and conditions of the GNU General Public License,
+ * version 2, as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License for
+ * more details.
+ *
+ */
+
+#ifndef _INTEL_ISHTP_CLIENTS_H
+#define _INTEL_ISHTP_CLIENTS_H
+
+#include <linux/ioctl.h>
+#include <linux/miscdevice.h>
+#include <linux/mutex.h>
+#include <linux/types.h>
+#include <linux/uuid.h>
+
+/*
+ * This IOCTL is used to associate the current file descriptor with a
+ * FW Client (given by UUID). This opens a communication channel
+ * between a host client and a FW client. From this point every read and write
+ * will communicate with the associated FW client.
+ * Only in close() (file_operation release()) the communication between
+ * the clients is disconnected
+ *
+ * The IOCTL argument is a struct with a union that contains
+ * the input parameter and the output parameter for this IOCTL.
+ *
+ * The input parameter is UUID of the FW Client.
+ * The output parameter is the properties of the FW client
+ * (FW protocol version and max message size).
+ *
+ */
+#define IOCTL_ISHTP_CONNECT_CLIENT	_IOWR('H', 0x81,	\
+				struct ishtp_connect_client_data)
+
+/* Configuration: set number of Rx/Tx buffers. Must be used before connection */
+#define IOCTL_ISHTP_SET_RX_FIFO_SIZE	_IOWR('H', 0x82, long)
+#define IOCTL_ISHTP_SET_TX_FIFO_SIZE	_IOWR('H', 0x83, long)
+
+/* Get FW status */
+#define IOCTL_ISH_GET_FW_STATUS	_IO('H', 0x84)
+
+#define IOCTL_ISH_HW_RESET	_IO('H', 0x85)
+
+/*
+ * Intel ISHTP client information struct
+ */
+struct ishtp_client {
+	__u32 max_msg_length;
+	__u8 protocol_version;
+	__u8 reserved[3];
+};
+
+/*
+ * IOCTL Connect client data structure
+ */
+struct ishtp_connect_client_data {
+	union {
+		uuid_le			in_client_uuid;
+		struct ishtp_client 	out_client_properties;
+	};
+};
+
+#endif /* _INTEL_ISHTP_CLIENTS_H */
-- 
2.7.4

^ permalink raw reply related	[flat|nested] 19+ messages in thread

* Re: [PATCH 7/7] misc: intel-ish-client: add intel ishtp clients driver
  2016-12-23  1:22 ` [PATCH 7/7] misc: intel-ish-client: add intel ishtp clients driver Even Xu
@ 2017-01-03  9:54   ` Jiri Kosina
  2017-01-04  6:55     ` Xu, Even
  2017-01-04 13:03   ` Greg KH
                     ` (2 subsequent siblings)
  3 siblings, 1 reply; 19+ messages in thread
From: Jiri Kosina @ 2017-01-03  9:54 UTC (permalink / raw)
  To: Even Xu
  Cc: benjamin.tissoires, srinivas.pandruvada, arnd, gregkh,
	andriy.shevchenko, linux-input, linux-kernel

On Fri, 23 Dec 2016, Even Xu wrote:

[ ... snip ... ]
> +static ssize_t ishtp_cl_write(struct file *file, const char __user *ubuf,
> +	size_t length, loff_t *offset)
> +{
> +	struct ishtp_cl_miscdev *ishtp_cl_misc = file->private_data;
> +	struct ishtp_cl *cl;
> +	void *write_buf;
> +	struct ishtp_device *dev;
> +	int ret;
> +
> +	/* Non-blocking semantics are not supported */
> +	if (file->f_flags & O_NONBLOCK) {
> +		ret = -EINVAL;
> +		goto out_unlock;

When taking the error path here you'd try to unlock 
ishtp_cl_misc->cl_mutex before actually acquiring it.

-- 
Jiri Kosina
SUSE Labs

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH 7/7] misc: intel-ish-client: add intel ishtp clients driver
  2017-01-03  9:54   ` Jiri Kosina
@ 2017-01-04  6:55     ` Xu, Even
  2017-01-04  9:36       ` Jiri Kosina
  0 siblings, 1 reply; 19+ messages in thread
From: Xu, Even @ 2017-01-04  6:55 UTC (permalink / raw)
  To: jikos
  Cc: srinivas.pandruvada, linux-input, benjamin.tissoires,
	linux-kernel, gregkh, arnd, Shevchenko, Andriy

On 二, 2017-01-03 at 10:54 +0100, Jiri Kosina wrote:
> On Fri, 23 Dec 2016, Even Xu wrote:
> 
> [ ... snip ... ]
> > 
> > +static ssize_t ishtp_cl_write(struct file *file, const char __user
> > *ubuf,
> > +	size_t length, loff_t *offset)
> > +{
> > +	struct ishtp_cl_miscdev *ishtp_cl_misc = file-
> > >private_data;
> > +	struct ishtp_cl *cl;
> > +	void *write_buf;
> > +	struct ishtp_device *dev;
> > +	int ret;
> > +
> > +	/* Non-blocking semantics are not supported */
> > +	if (file->f_flags & O_NONBLOCK) {
> > +		ret = -EINVAL;
> > +		goto out_unlock;
> When taking the error path here you'd try to unlock 
> ishtp_cl_misc->cl_mutex before actually acquiring it.
> 

Thanks for your comments, Jiri, I update my patch below:
===============================================================

Intel ISHFW supports many different clients, in
hid/intel-ish-hid/ishtp bus driver, it creates following client devices:
HID client:
	interface of sensor configure and sensor event report.
SMHI client:
	interface of sensor calibration, ISHFW debug, ISHFW performance
	analysis and manufacture support.
Trace client:
	interface of ISHFW debug log output.
Trace configure client:
	interface of ISHFW debug log configuration, such as output port,
	log level, filter.
ISHFW loader client:
	interface of customized ISHFW loader.
HID client has been handle by hid/intel-ish-hid/intel-ishtp-hid client
driver, and rest of the clients export interface using miscellaneous
drivers. This interface is used by user space tools for debugging and
calibration of sensors.

Signed-off-by: Even Xu <even.xu@intel.com>
Reviewed-by: Andriy Shevchenko <andriy.shevchenko@intel.com>
Reviewed-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>
---
 drivers/misc/Kconfig                               |   1 +
 drivers/misc/Makefile                              |   1 +
 drivers/misc/intel-ish-client/Kconfig              |  15 +
 drivers/misc/intel-ish-client/Makefile             |   8 +
 .../misc/intel-ish-client/intel-ishtp-clients.c    | 882 +++++++++++++++++++++
 include/uapi/linux/intel-ishtp-clients.h           |  73 ++
 6 files changed, 980 insertions(+)
 create mode 100644 drivers/misc/intel-ish-client/Kconfig
 create mode 100644 drivers/misc/intel-ish-client/Makefile
 create mode 100644 drivers/misc/intel-ish-client/intel-ishtp-clients.c
 create mode 100644 include/uapi/linux/intel-ishtp-clients.h

diff --git a/drivers/misc/Kconfig b/drivers/misc/Kconfig
index 64971ba..a89849f 100644
--- a/drivers/misc/Kconfig
+++ b/drivers/misc/Kconfig
@@ -778,4 +778,5 @@ source "drivers/misc/mic/Kconfig"
 source "drivers/misc/genwqe/Kconfig"
 source "drivers/misc/echo/Kconfig"
 source "drivers/misc/cxl/Kconfig"
+source "drivers/misc/intel-ish-client/Kconfig"
 endmenu
diff --git a/drivers/misc/Makefile b/drivers/misc/Makefile
index 3198336..c54015d 100644
--- a/drivers/misc/Makefile

+++ b/drivers/misc/Makefile
@@ -53,6 +53,7 @@ obj-$(CONFIG_ECHO)		+= echo/
 obj-$(CONFIG_VEXPRESS_SYSCFG)	+= vexpress-syscfg.o
 obj-$(CONFIG_CXL_BASE)		+= cxl/
 obj-$(CONFIG_PANEL)             += panel.o
+obj-$(CONFIG_INTEL_ISH_CLIENT)	+= intel-ish-client/
 
 lkdtm-$(CONFIG_LKDTM)		+= lkdtm_core.o
 lkdtm-$(CONFIG_LKDTM)		+= lkdtm_bugs.o
diff --git a/drivers/misc/intel-ish-client/Kconfig b/drivers/misc/intel-ish-client/Kconfig
new file mode 100644
index 0000000..6fa9cc0
--- /dev/null
+++ b/drivers/misc/intel-ish-client/Kconfig
@@ -0,0 +1,15 @@
+menu "Intel ISH Client support"
+	depends on INTEL_ISH_HID
+
+config INTEL_ISH_CLIENT
+	tristate "Intel Integrated Sensor Hub Client"
+	help
+	  The Integrated Sensor Hub (ISH) supports many clients, Intel ISH client
+	  driver supports following clients:
+	  SMHI client: calibration & perfermance & menufacture tool interface
+	  trace configure client: ISHFW trace parameter configure interface
+	  trace tool client: ISHFW trace log output interface
+	  loader client: loading customized ISHFW interface
+
+	  Say Y here if you want to support Intel ISH clients. If unsure, say N.
+endmenu
diff --git a/drivers/misc/intel-ish-client/Makefile b/drivers/misc/intel-ish-client/Makefile
new file mode 100644
index 0000000..29a5461
--- /dev/null
+++ b/drivers/misc/intel-ish-client/Makefile
@@ -0,0 +1,8 @@
+#
+# Makefile - Intel ISH client driver
+# Copyright (c) 2014-2016, Intel Corporation.
+#
+#
+obj-$(CONFIG_INTEL_ISH_CLIENT) += intel-ishtp-clients.o
+
+ccflags-y += -I$(srctree)/drivers/hid/intel-ish-hid/ishtp
diff --git a/drivers/misc/intel-ish-client/intel-ishtp-clients.c b/drivers/misc/intel-ish-client/intel-ishtp-clients.c
new file mode 100644
index 0000000..4ca3ab8
--- /dev/null
+++ b/drivers/misc/intel-ish-client/intel-ishtp-clients.c
@@ -0,0 +1,882 @@
+/*
+ * ISHTP clients driver
+ *
+ * Copyright (c) 2016, Intel Corporation.
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms and conditions of the GNU General Public License,
+ * version 2, as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License for
+ * more details.
+ */
+
+#include <linux/capability.h>
+#include <linux/delay.h>
+#include <linux/device.h>
+#include <linux/errno.h>
+#include <linux/fs.h>
+#include <linux/interrupt.h>
+#include <linux/intel-ishtp-clients.h>
+#include <linux/ioctl.h>
+#include <linux/kernel.h>
+#include <linux/miscdevice.h>
+#include <linux/module.h>
+#include <linux/slab.h>
+#include <linux/types.h>
+#include <linux/uuid.h>
+#include <linux/uaccess.h>
+
+#include "ishtp-dev.h"
+#include "client.h"
+
+/*
+ * ISH client misc driver structure
+ */
+struct ishtp_cl_miscdev {
+	struct miscdevice cl_miscdev;
+	struct ishtp_cl_device *cl_device;
+	struct ishtp_cl *cl;
+
+	/* Wait queue for waiting ISHFW event/message */
+	wait_queue_head_t read_wait;
+	/* Read buffer, will point to available ISHFW message */
+	struct ishtp_cl_rb *read_rb;
+
+	/*
+	 * cl member can be freed/changed by ISHFW reset and release() calling,
+	 * so must pay attention to protect cl while try to access it. This
+	 * mutex is used to protect cl member.
+	 */
+	struct mutex cl_mutex;
+
+	struct work_struct reset_work;
+};
+
+/* ISH client GUIDs */
+/* SMHI client UUID: bb579a2e-cc54-4450-b1d0-5e7520dcad25 */
+static const uuid_le ishtp_smhi_guid =
+			UUID_LE(0xbb579a2e, 0xcc54, 0x4450,
+				0xb1, 0xd0, 0x5e, 0x75, 0x20, 0xdc, 0xad, 0x25);
+
+/* Trace log client UUID: c1cc78b9-b693-4e54-9191-5169cb027c25 */
+static const uuid_le ishtp_trace_guid =
+			UUID_LE(0xc1cc78b9, 0xb693, 0x4e54,
+				0x91, 0x91, 0x51, 0x69, 0xcb, 0x02, 0x7c, 0x25);
+
+/* Trace config client UUID: 1f050626-d505-4e94-b189-535d7de19cf2 */
+static const uuid_le ishtp_traceconfig_guid =
+			UUID_LE(0x1f050626, 0xd505, 0x4e94,
+				0xb1, 0x89, 0x53, 0x5d, 0x7d, 0xe1, 0x9c, 0xf2);
+
+/* ISHFW loader client UUID: c804d06a-55bd-4ea7-aded-1e31228c76dc */
+static const uuid_le ishtp_loader_guid =
+			UUID_LE(0xc804d06a, 0x55bd, 0x4ea7,
+				0xad, 0xed, 0x1e, 0x31, 0x22, 0x8c, 0x76, 0xdc);
+
+static int ishtp_cl_open(struct inode *inode, struct file *file)
+{
+	struct miscdevice *misc = file->private_data;
+	struct ishtp_cl *cl;
+	struct ishtp_device *dev;
+	struct ishtp_cl_miscdev *ishtp_cl_misc;
+	int ret;
+
+	/* Non-blocking semantics are not supported */
+	if (file->f_flags & O_NONBLOCK)
+		return -EINVAL;
+
+	ishtp_cl_misc = container_of(misc,
+				struct ishtp_cl_miscdev, cl_miscdev);
+	if (!ishtp_cl_misc || !ishtp_cl_misc->cl_device)
+		return -ENODEV;
+
+	dev = ishtp_cl_misc->cl_device->ishtp_dev;
+	if (!dev)
+		return -ENODEV;
+
+	mutex_lock(&ishtp_cl_misc->cl_mutex);
+
+	/*
+	 * Every client only supports one opened instance
+	 * at the sametime.
+	 */
+	if (ishtp_cl_misc->cl) {
+		ret = -EBUSY;
+		goto out_unlock;
+	}
+
+	cl = ishtp_cl_allocate(dev);
+	if (!cl) {
+		ret = -ENOMEM;
+		goto out_free;
+	}
+
+	if (dev->dev_state != ISHTP_DEV_ENABLED) {
+		ret = -ENODEV;
+		goto out_free;
+	}
+
+	ret = ishtp_cl_link(cl, ISHTP_HOST_CLIENT_ID_ANY);
+	if (ret)
+		goto out_free;
+
+	ishtp_cl_misc->cl = cl;
+
+	file->private_data = ishtp_cl_misc;
+
+	mutex_unlock(&ishtp_cl_misc->cl_mutex);
+
+	return nonseekable_open(inode, file);
+
+out_free:
+	kfree(cl);
+out_unlock:
+	mutex_unlock(&ishtp_cl_misc->cl_mutex);
+	return ret;
+}
+
+#define WAIT_FOR_SEND_SLICE_MS		100
+#define WAIT_FOR_SEND_COUNT		10
+
+static int ishtp_cl_release(struct inode *inode, struct file *file)
+{
+	struct ishtp_cl_miscdev *ishtp_cl_misc = file->private_data;
+	struct ishtp_cl *cl;
+	struct ishtp_cl_rb *rb;
+	struct ishtp_device *dev;
+	int try = WAIT_FOR_SEND_COUNT;
+	int ret;
+
+	mutex_lock(&ishtp_cl_misc->cl_mutex);
+
+	/* Wake up from waiting if anyone wait on it */
+	wake_up_interruptible(&ishtp_cl_misc->read_wait);
+
+	cl = ishtp_cl_misc->cl;
+	dev = cl->dev;
+
+	/*
+	 * May happen if device sent FW reset or was intentionally
+	 * halted by host SW. The client is then invalid.
+	 */
+	if ((dev->dev_state == ISHTP_DEV_ENABLED) &&
+			(cl->state == ISHTP_CL_CONNECTED)) {
+		/*
+		 * Check and wait 1s for message in tx_list to be sent.
+		 */
+		do {
+			if (!ishtp_cl_tx_empty(cl))
+				msleep_interruptible(WAIT_FOR_SEND_SLICE_MS);
+			else
+				break;
+		} while (--try);
+
+		cl->state = ISHTP_CL_DISCONNECTING;
+		ret = ishtp_cl_disconnect(cl);
+	}
+
+	ishtp_cl_unlink(cl);
+	ishtp_cl_flush_queues(cl);
+	/* Disband and free all Tx and Rx client-level rings */
+	ishtp_cl_free(cl);
+
+	ishtp_cl_misc->cl = NULL;
+
+	rb = ishtp_cl_misc->read_rb;
+	if (rb) {
+		ishtp_cl_io_rb_recycle(rb);
+		ishtp_cl_misc->read_rb = NULL;
+	}
+
+	file->private_data = NULL;
+
+	mutex_unlock(&ishtp_cl_misc->cl_mutex);
+
+	return ret;
+}
+
+static ssize_t ishtp_cl_read(struct file *file, char __user *ubuf,
+			size_t length, loff_t *offset)
+{
+	struct ishtp_cl_miscdev *ishtp_cl_misc = file->private_data;
+	struct ishtp_cl *cl;
+	struct ishtp_cl_rb *rb;
+	struct ishtp_device *dev;
+	int ret = 0;
+
+	/* Non-blocking semantics are not supported */
+	if (file->f_flags & O_NONBLOCK)
+		return -EINVAL;
+
+	mutex_lock(&ishtp_cl_misc->cl_mutex);
+
+	cl = ishtp_cl_misc->cl;
+
+	/*
+	 * ISHFW reset will cause cl be freed and re-allocated.
+	 * So must make sure cl is re-allocated successfully.
+	 */
+	if (!cl || !cl->dev) {
+		ret = -ENODEV;
+		goto out_unlock;
+	}
+
+	dev = cl->dev;
+	if (dev->dev_state != ISHTP_DEV_ENABLED) {
+		ret = -ENODEV;
+		goto out_unlock;
+	}
+
+	if (ishtp_cl_misc->read_rb)
+		goto get_rb;
+
+	rb = ishtp_cl_rx_get_rb(cl);
+	if (rb)
+		goto copy_buffer;
+
+	/*
+	 * Release mutex for other operation can be processed parallelly
+	 * during waiting.
+	 */
+	mutex_unlock(&ishtp_cl_misc->cl_mutex);
+
+	if (wait_event_interruptible(ishtp_cl_misc->read_wait,
+			ishtp_cl_misc->read_rb != NULL)) {
+		dev_err(&ishtp_cl_misc->cl_device->dev,
+			"Wake up not successful;"
+			"signal pending = %d signal = %08lX\n",
+			signal_pending(current),
+			current->pending.signal.sig[0]);
+		return -ERESTARTSYS;
+	}
+
+	mutex_lock(&ishtp_cl_misc->cl_mutex);
+
+	/*
+	 * waitqueue can be woken up in many cases, so must check
+	 * if dev and cl is still available.
+	 */
+	if (dev->dev_state != ISHTP_DEV_ENABLED) {
+		ret = -ENODEV;
+		goto out_unlock;
+	}
+
+	cl = ishtp_cl_misc->cl;
+	if (!cl) {
+		ret = -ENODEV;
+		goto out_unlock;
+	}
+
+	if (cl->state == ISHTP_CL_INITIALIZING ||
+		cl->state == ISHTP_CL_DISCONNECTED ||
+		cl->state == ISHTP_CL_DISCONNECTING) {
+		ret = -EBUSY;
+		goto out_unlock;
+	}
+
+get_rb:
+	rb = ishtp_cl_misc->read_rb;
+	if (!rb) {
+		ret = -ENODEV;
+		goto out_unlock;
+	}
+
+copy_buffer:
+	/* Now copy the data to user space */
+	if (!length || !ubuf || *offset > rb->buf_idx) {
+		ret = -EMSGSIZE;
+		goto out_unlock;
+	}
+
+	/*
+	 * length is being truncated, however buf_idx may
+	 * point beyond that.
+	 */
+	length = min_t(size_t, length, rb->buf_idx - *offset);
+
+	if (copy_to_user(ubuf, rb->buffer.data + *offset, length)) {
+		ret = -EFAULT;
+		goto out_unlock;
+	}
+
+	*offset += length;
+	if ((unsigned long)*offset < rb->buf_idx)
+		goto out_unlock;
+
+	ishtp_cl_io_rb_recycle(rb);
+	ishtp_cl_misc->read_rb = NULL;
+	*offset = 0;
+
+out_unlock:
+	mutex_unlock(&ishtp_cl_misc->cl_mutex);
+	return ret < 0 ? ret : length;
+}
+
+static ssize_t ishtp_cl_write(struct file *file, const char __user *ubuf,
+	size_t length, loff_t *offset)
+{
+	struct ishtp_cl_miscdev *ishtp_cl_misc = file->private_data;
+	struct ishtp_cl *cl;
+	void *write_buf;
+	struct ishtp_device *dev;
+	int ret;
+
+	/* Non-blocking semantics are not supported */
+	if (file->f_flags & O_NONBLOCK)
+		return -EINVAL;
+
+	mutex_lock(&ishtp_cl_misc->cl_mutex);
+
+	cl = ishtp_cl_misc->cl;
+
+	/*
+	 * ISHFW reset will cause cl be freed and re-allocated.
+	 * So must make sure cl is re-allocated successfully.
+	 */
+	if (!cl || !cl->dev) {
+		ret = -ENODEV;
+		goto out_unlock;
+	}
+
+	dev = cl->dev;
+
+	if (dev->dev_state != ISHTP_DEV_ENABLED) {
+		ret = -ENODEV;
+		goto out_unlock;
+	}
+
+	if (cl->state != ISHTP_CL_CONNECTED) {
+		dev_err(&ishtp_cl_misc->cl_device->dev,
+			"host client = %d isn't connected to fw client = %d\n",
+			cl->host_client_id, cl->fw_client_id);
+		ret = -ENODEV;
+		goto out_unlock;
+	}
+
+	if (length <= 0 || length > cl->device->fw_client->props.max_msg_length) {
+		ret = -EMSGSIZE;
+		goto out_unlock;
+	}
+
+	write_buf = memdup_user(ubuf, length);
+	if (IS_ERR(write_buf)) {
+		ret = PTR_ERR(write_buf);
+		goto out_unlock;
+	}
+
+	ret = ishtp_cl_send(cl, write_buf, length);
+
+	kfree(write_buf);
+
+out_unlock:
+	mutex_unlock(&ishtp_cl_misc->cl_mutex);
+
+	return ret < 0 ? ret : length;
+}
+
+static int ishtp_cl_ioctl_connect_client(struct file *file,
+	struct ishtp_connect_client_data *data)
+{
+	struct ishtp_cl_miscdev *ishtp_cl_misc = file->private_data;
+	struct ishtp_device *dev;
+	struct ishtp_client *client;
+	struct ishtp_cl_device *cl_device;
+	struct ishtp_cl *cl = ishtp_cl_misc->cl;
+	struct ishtp_fw_client *fw_client;
+
+	if (!cl || !cl->dev)
+		return -ENODEV;
+
+	dev = cl->dev;
+
+	if (dev->dev_state != ISHTP_DEV_ENABLED)
+		return -ENODEV;
+
+	if (cl->state != ISHTP_CL_INITIALIZING &&
+			cl->state != ISHTP_CL_DISCONNECTED)
+		return -EBUSY;
+
+	cl_device = ishtp_cl_misc->cl_device;
+
+	if (uuid_le_cmp(data->in_client_uuid,
+			cl_device->fw_client->props.protocol_name) != 0) {
+		dev_err(&ishtp_cl_misc->cl_device->dev,
+			"Required uuid don't match current client uuid\n");
+		return -EFAULT;
+	}
+
+	/* Find the fw client we're trying to connect to */
+	fw_client = ishtp_fw_cl_get_client(dev, &data->in_client_uuid);
+	if (!fw_client) {
+		dev_err(&ishtp_cl_misc->cl_device->dev,
+			"Don't find current client uuid\n");
+		return -ENOENT;
+	}
+
+	cl->fw_client_id = fw_client->client_id;
+	cl->state = ISHTP_CL_CONNECTING;
+
+	/* Prepare the output buffer */
+	client = &data->out_client_properties;
+	client->max_msg_length = fw_client->props.max_msg_length;
+	client->protocol_version = fw_client->props.protocol_version;
+
+	return ishtp_cl_connect(cl);
+}
+
+static long ishtp_cl_ioctl(struct file *file, unsigned int cmd,
+			unsigned long data)
+{
+	struct ishtp_cl_miscdev *ishtp_cl_misc = file->private_data;
+	struct ishtp_cl *cl;
+	struct ishtp_device *dev;
+	struct ishtp_connect_client_data *connect_data;
+	char fw_stat_buf[20];
+	unsigned int ring_size;
+	int ret = 0;
+
+	if (!capable(CAP_SYS_ADMIN))
+		return -EPERM;
+
+	mutex_lock(&ishtp_cl_misc->cl_mutex);
+
+	cl = ishtp_cl_misc->cl;
+
+	/*
+	 * ISHFW reset will cause cl be freed and re-allocated.
+	 * So must make sure cl is re-allocated successfully.
+	 */
+	if (!cl || !cl->dev) {
+		mutex_unlock(&ishtp_cl_misc->cl_mutex);
+		return -ENODEV;
+	}
+
+	dev = cl->dev;
+
+	switch (cmd) {
+	case IOCTL_ISH_HW_RESET:
+		ret = ish_hw_reset(dev);
+		break;
+
+	case IOCTL_ISHTP_SET_RX_FIFO_SIZE:
+		ring_size = data;
+
+		if (ring_size > CL_MAX_RX_RING_SIZE) {
+			ret = -EINVAL;
+			break;
+		}
+
+		if (cl->state != ISHTP_CL_INITIALIZING) {
+			ret = -EBUSY;
+			break;
+		}
+
+		cl->rx_ring_size = ring_size;
+		break;
+
+	case IOCTL_ISHTP_SET_TX_FIFO_SIZE:
+		ring_size = data;
+
+		if (ring_size > CL_MAX_TX_RING_SIZE) {
+			ret = -EINVAL;
+			break;
+		}
+
+		if (cl->state != ISHTP_CL_INITIALIZING) {
+			ret = -EBUSY;
+			break;
+		}
+
+		cl->tx_ring_size = ring_size;
+		break;
+
+	case IOCTL_ISH_GET_FW_STATUS:
+		if (!data) {
+			ret = -ENOMEM;
+			break;
+		}
+
+		snprintf(fw_stat_buf, sizeof(fw_stat_buf),
+			"%08X\n", dev->ops->get_fw_status(dev));
+
+		if (copy_to_user((char __user *)data, fw_stat_buf,
+				strlen(fw_stat_buf))) {
+			ret = -EFAULT;
+			break;
+		}
+
+		ret = strlen(fw_stat_buf);
+		break;
+
+	case IOCTL_ISHTP_CONNECT_CLIENT:
+		if (dev->dev_state != ISHTP_DEV_ENABLED) {
+			ret = -ENODEV;
+			break;
+		}
+
+		connect_data = memdup_user((char __user *)data,
+					sizeof(struct ishtp_connect_client_data));
+		if (IS_ERR(connect_data)) {
+			ret = PTR_ERR(connect_data);
+			break;
+		}
+
+		ret = ishtp_cl_ioctl_connect_client(file, connect_data);
+		if (ret) {
+			kfree(connect_data);
+			break;
+		}
+
+		/* If all is ok, copying the data back to user. */
+		if (copy_to_user((char __user *)data, connect_data,
+				sizeof(struct ishtp_connect_client_data)))
+			ret = -EFAULT;
+
+		kfree(connect_data);
+
+		break;
+
+	default:
+		ret = -EINVAL;
+		break;
+	}
+
+	mutex_unlock(&ishtp_cl_misc->cl_mutex);
+
+	return ret;
+}
+
+/*
+ * File operations structure will be used for ishtp client misc device.
+ */
+static const struct file_operations ishtp_cl_fops = {
+	.owner = THIS_MODULE,
+	.read = ishtp_cl_read,
+	.unlocked_ioctl = ishtp_cl_ioctl,
+	.open = ishtp_cl_open,
+	.release = ishtp_cl_release,
+	.write = ishtp_cl_write,
+	.llseek = no_llseek
+};
+
+/**
+ * ishtp_cl_event_cb() - ISHTP client driver event callback
+ * @cl_device:		ISHTP client device instance
+ *
+ * This function gets called on related event recevied from ISHFW.
+ * It will remove event buffer exists on in_process list to related
+ * client device and wait up client driver to process.
+ */
+static void ishtp_cl_event_cb(struct ishtp_cl_device *cl_device)
+{
+	struct ishtp_cl_miscdev *ishtp_cl_misc;
+	struct ishtp_cl *cl;
+	struct ishtp_cl_rb *rb;
+
+	ishtp_cl_misc = ishtp_get_drvdata(cl_device);
+	if (!ishtp_cl_misc)
+		return;
+
+	mutex_lock(&ishtp_cl_misc->cl_mutex);
+
+	/*
+	 * If this waitqueue is active, cl_mutex is locked by read(), it's safe
+	 * to access ishtp_cl_misc and cl.
+	 */
+	if (waitqueue_active(&ishtp_cl_misc->read_wait)) {
+
+		/*
+		 * If already has read_rb, wake up waitqueue directly.
+		 */
+		if (ishtp_cl_misc->read_rb) {
+			mutex_unlock(&ishtp_cl_misc->cl_mutex);
+			wake_up_interruptible(&ishtp_cl_misc->read_wait);
+			return;
+		}
+
+		cl = ishtp_cl_misc->cl;
+
+		rb = ishtp_cl_rx_get_rb(cl);
+		if (rb)
+			ishtp_cl_misc->read_rb = rb;
+
+		wake_up_interruptible(&ishtp_cl_misc->read_wait);
+	}
+
+	mutex_unlock(&ishtp_cl_misc->cl_mutex);
+}
+
+/**
+ * ishtp_cl_reset_handler() - ISHTP client driver reset work handler
+ * @work:		work struct
+ *
+ * This function gets called on reset workqueue scheduled when ISHFW
+ * reset happen. It will disconnect and remove current ishtp_cl, then
+ * create a new ishtp_cl and re-connect again.
+ */
+static void ishtp_cl_reset_handler(struct work_struct *work)
+{
+	struct ishtp_cl_miscdev *ishtp_cl_misc;
+	struct ishtp_device *dev;
+	struct ishtp_cl_device *cl_device;
+	struct ishtp_cl *cl;
+	struct ishtp_fw_client *fw_client;
+	int ret = 0;
+
+	ishtp_cl_misc = container_of(work,
+			struct ishtp_cl_miscdev, reset_work);
+
+	dev = ishtp_cl_misc->cl_device->ishtp_dev;
+	if (!dev) {
+		dev_err(&ishtp_cl_misc->cl_device->dev,
+			"This cl_device not link to ishtp_dev\n");
+		return;
+	}
+
+	cl_device = ishtp_cl_misc->cl_device;
+
+	mutex_lock(&ishtp_cl_misc->cl_mutex);
+
+	/* Wake up from waiting if anyone wait on it */
+	wake_up_interruptible(&ishtp_cl_misc->read_wait);
+
+	cl = ishtp_cl_misc->cl;
+	if (cl) {
+		ishtp_cl_flush_queues(cl);
+		ishtp_cl_free(cl);
+
+		cl = NULL;
+
+		cl = ishtp_cl_allocate(dev);
+		if (!cl) {
+			dev_err(&ishtp_cl_misc->cl_device->dev,
+				"Allocate ishtp_cl failed\n");
+			ret = -ENOMEM;
+			goto out_unlock;
+		}
+
+		if (dev->dev_state != ISHTP_DEV_ENABLED) {
+			dev_err(&ishtp_cl_misc->cl_device->dev,
+				"Ishtp dev isn't enabled\n");
+			ret = -ENODEV;
+			goto out_free;
+		}
+
+		ret = ishtp_cl_link(cl, ISHTP_HOST_CLIENT_ID_ANY);
+		if (ret) {
+			dev_err(&ishtp_cl_misc->cl_device->dev,
+				"Can not link to ishtp\n");
+			goto out_free;
+		}
+
+		fw_client = ishtp_fw_cl_get_client(dev,
+				&cl_device->fw_client->props.protocol_name);
+		if (!fw_client) {
+			dev_err(&ishtp_cl_misc->cl_device->dev,
+				"Don't find related fw client\n");
+			ret = -ENOENT;
+			goto out_free;
+		}
+
+		cl->fw_client_id = fw_client->client_id;
+		cl->state = ISHTP_CL_CONNECTING;
+
+		ret = ishtp_cl_connect(cl);
+		if (ret) {
+			dev_err(&ishtp_cl_misc->cl_device->dev,
+				"Connect to fw failed\n");
+			goto out_free;
+		}
+
+		ishtp_cl_misc->cl = cl;
+	}
+
+	/* After reset, must register event callback again */
+	ishtp_register_event_cb(cl_device, ishtp_cl_event_cb);
+
+out_free:
+	if (ret) {
+		ishtp_cl_free(cl);
+		ishtp_cl_misc->cl = NULL;
+
+		dev_err(&ishtp_cl_misc->cl_device->dev, "Reset failed\n");
+	}
+
+out_unlock:
+	mutex_unlock(&ishtp_cl_misc->cl_mutex);
+}
+
+/**
+ * ishtp_cl_probe() - ISHTP client driver probe
+ * @cl_device:		ISHTP client device instance
+ *
+ * This function gets called on device create on ISHTP bus
+ *
+ * Return: 0 on success, non zero on error
+ */
+static int ishtp_cl_probe(struct ishtp_cl_device *cl_device)
+{
+	struct ishtp_cl_miscdev *ishtp_cl_misc;
+	int ret;
+
+	if (!cl_device)
+		return -ENODEV;
+
+	ishtp_cl_misc = kzalloc(sizeof(struct ishtp_cl_miscdev),
+				GFP_KERNEL);
+	if (!ishtp_cl_misc)
+		return -ENOMEM;
+
+	if (uuid_le_cmp(ishtp_smhi_guid,
+			cl_device->fw_client->props.protocol_name) == 0) {
+		ishtp_cl_misc->cl_miscdev.name = "ish-smhi";
+	} else if (uuid_le_cmp(ishtp_trace_guid,
+			cl_device->fw_client->props.protocol_name) == 0) {
+		ishtp_cl_misc->cl_miscdev.name = "ish-trace";
+	} else if (uuid_le_cmp(ishtp_traceconfig_guid,
+			cl_device->fw_client->props.protocol_name) == 0) {
+		ishtp_cl_misc->cl_miscdev.name = "ish-tracec";
+	} else if (uuid_le_cmp(ishtp_loader_guid,
+			cl_device->fw_client->props.protocol_name) == 0) {
+		ishtp_cl_misc->cl_miscdev.name = "ish-loader";
+	} else {
+		dev_err(&cl_device->dev, "Not supported client\n");
+		ret = -ENODEV;
+		goto release_mem;
+	}
+
+	ishtp_cl_misc->cl_miscdev.parent = &cl_device->dev;
+	ishtp_cl_misc->cl_miscdev.fops = &ishtp_cl_fops;
+	ishtp_cl_misc->cl_miscdev.minor = MISC_DYNAMIC_MINOR,
+
+	ret = misc_register(&ishtp_cl_misc->cl_miscdev);
+	if (ret) {
+		dev_err(&cl_device->dev, "misc device register failed\n");
+		goto release_mem;
+	}
+
+	ishtp_cl_misc->cl_device = cl_device;
+
+	init_waitqueue_head(&ishtp_cl_misc->read_wait);
+
+	ishtp_set_drvdata(cl_device, ishtp_cl_misc);
+
+	ishtp_get_device(cl_device);
+
+	mutex_init(&ishtp_cl_misc->cl_mutex);
+
+	INIT_WORK(&ishtp_cl_misc->reset_work, ishtp_cl_reset_handler);
+
+	/* Register event callback */
+	ishtp_register_event_cb(cl_device, ishtp_cl_event_cb);
+
+	return 0;
+
+release_mem:
+	kfree(ishtp_cl_misc);
+
+	return ret;
+}
+
+/**
+ * ishtp_cl_remove() - ISHTP client driver remove
+ * @cl_device:		ISHTP client device instance
+ *
+ * This function gets called on device remove on ISHTP bus
+ *
+ * Return: 0
+ */
+static int ishtp_cl_remove(struct ishtp_cl_device *cl_device)
+{
+	struct ishtp_cl_miscdev *ishtp_cl_misc;
+	struct ishtp_cl *cl;
+
+	ishtp_cl_misc = ishtp_get_drvdata(cl_device);
+	if (!ishtp_cl_misc)
+		return -ENODEV;
+
+	if (!ishtp_cl_misc->cl_miscdev.parent)
+		return -ENODEV;
+
+	/* Wake up from waiting if anyone wait on it */
+	wake_up_interruptible(&ishtp_cl_misc->read_wait);
+
+	mutex_lock(&ishtp_cl_misc->cl_mutex);
+
+	cl = ishtp_cl_misc->cl;
+	if (cl) {
+		cl->state = ISHTP_CL_DISCONNECTING;
+		ishtp_cl_disconnect(cl);
+		ishtp_cl_unlink(cl);
+		ishtp_cl_flush_queues(cl);
+		ishtp_cl_free(cl);
+		ishtp_cl_misc->cl = NULL;
+	}
+
+	mutex_unlock(&ishtp_cl_misc->cl_mutex);
+
+	mutex_destroy(&ishtp_cl_misc->cl_mutex);
+
+	misc_deregister(&ishtp_cl_misc->cl_miscdev);
+
+	ishtp_put_device(cl_device);
+
+	kfree(ishtp_cl_misc);
+
+	return 0;
+}
+
+/**
+ * ishtp_cl_reset() - ISHTP client driver reset
+ * @cl_device:		ISHTP client device instance
+ *
+ * This function gets called on device reset on ISHTP bus.
+ * If client is connected, needs to disconnect client and
+ * reconnect again.
+ *
+ * Return: 0
+ */
+static int ishtp_cl_reset(struct ishtp_cl_device *cl_device)
+{
+	struct ishtp_cl_miscdev *ishtp_cl_misc;
+
+	ishtp_cl_misc = ishtp_get_drvdata(cl_device);
+	if (!ishtp_cl_misc) {
+		dev_err(&cl_device->dev, "Client driver not ready yet\n");
+		return -ENODEV;
+	}
+
+	schedule_work(&ishtp_cl_misc->reset_work);
+
+	return 0;
+}
+
+static struct ishtp_cl_driver ishtp_cl_driver = {
+	.name = "ishtp-client",
+	.probe = ishtp_cl_probe,
+	.remove = ishtp_cl_remove,
+	.reset = ishtp_cl_reset,
+};
+
+static int __init ishtp_client_init(void)
+{
+	/* Register ISHTP client device driver with ISHTP Bus */
+	return ishtp_cl_driver_register(&ishtp_cl_driver);
+}
+
+static void __exit ishtp_client_exit(void)
+{
+	ishtp_cl_driver_unregister(&ishtp_cl_driver);
+}
+
+/* To make sure ISHTP bus driver loaded first */
+late_initcall(ishtp_client_init);
+module_exit(ishtp_client_exit);
+
+MODULE_DESCRIPTION("ISH ISHTP client driver");
+MODULE_AUTHOR("Even Xu <even.xu@intel.com>");
+MODULE_LICENSE("GPL");
+MODULE_ALIAS("ishtp:*");
diff --git a/include/uapi/linux/intel-ishtp-clients.h b/include/uapi/linux/intel-ishtp-clients.h
new file mode 100644
index 0000000..792500a
--- /dev/null
+++ b/include/uapi/linux/intel-ishtp-clients.h
@@ -0,0 +1,73 @@
+/*
+ * Intel ISHTP Clients Interface Header
+ *
+ * Copyright (c) 2016, Intel Corporation.
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms and conditions of the GNU General Public License,
+ * version 2, as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License for
+ * more details.
+ *
+ */
+
+#ifndef _INTEL_ISHTP_CLIENTS_H
+#define _INTEL_ISHTP_CLIENTS_H
+
+#include <linux/ioctl.h>
+#include <linux/miscdevice.h>
+#include <linux/mutex.h>
+#include <linux/types.h>
+#include <linux/uuid.h>
+
+/*
+ * This IOCTL is used to associate the current file descriptor with a
+ * FW Client (given by UUID). This opens a communication channel
+ * between a host client and a FW client. From this point every read and write
+ * will communicate with the associated FW client.
+ * Only in close() (file_operation release()) the communication between
+ * the clients is disconnected
+ *
+ * The IOCTL argument is a struct with a union that contains
+ * the input parameter and the output parameter for this IOCTL.
+ *
+ * The input parameter is UUID of the FW Client.
+ * The output parameter is the properties of the FW client
+ * (FW protocol version and max message size).
+ *
+ */
+#define IOCTL_ISHTP_CONNECT_CLIENT	_IOWR('H', 0x81,	\
+				struct ishtp_connect_client_data)
+
+/* Configuration: set number of Rx/Tx buffers. Must be used before connection */
+#define IOCTL_ISHTP_SET_RX_FIFO_SIZE	_IOWR('H', 0x82, long)
+#define IOCTL_ISHTP_SET_TX_FIFO_SIZE	_IOWR('H', 0x83, long)
+
+/* Get FW status */
+#define IOCTL_ISH_GET_FW_STATUS	_IO('H', 0x84)
+
+#define IOCTL_ISH_HW_RESET	_IO('H', 0x85)
+
+/*
+ * Intel ISHTP client information struct
+ */
+struct ishtp_client {
+	__u32 max_msg_length;
+	__u8 protocol_version;
+	__u8 reserved[3];
+};
+
+/*
+ * IOCTL Connect client data structure
+ */
+struct ishtp_connect_client_data {
+	union {
+		uuid_le			in_client_uuid;
+		struct ishtp_client 	out_client_properties;
+	};
+};
+
+#endif /* _INTEL_ISHTP_CLIENTS_H */
-- 
2.7.4

^ permalink raw reply related	[flat|nested] 19+ messages in thread

* Re: [PATCH 7/7] misc: intel-ish-client: add intel ishtp clients driver
  2017-01-04  6:55     ` Xu, Even
@ 2017-01-04  9:36       ` Jiri Kosina
  2017-01-04 12:59         ` gregkh
  0 siblings, 1 reply; 19+ messages in thread
From: Jiri Kosina @ 2017-01-04  9:36 UTC (permalink / raw)
  To: Xu, Even
  Cc: srinivas.pandruvada, linux-input, benjamin.tissoires,
	linux-kernel, gregkh, arnd, Shevchenko, Andriy

On Wed, 4 Jan 2017, Xu, Even wrote:

> > [ ... snip ... ]
> > > 
> > > +static ssize_t ishtp_cl_write(struct file *file, const char __user
> > > *ubuf,
> > > +	size_t length, loff_t *offset)
> > > +{
> > > +	struct ishtp_cl_miscdev *ishtp_cl_misc = file-
> > > >private_data;
> > > +	struct ishtp_cl *cl;
> > > +	void *write_buf;
> > > +	struct ishtp_device *dev;
> > > +	int ret;
> > > +
> > > +	/* Non-blocking semantics are not supported */
> > > +	if (file->f_flags & O_NONBLOCK) {
> > > +		ret = -EINVAL;
> > > +		goto out_unlock;
> > When taking the error path here you'd try to unlock 
> > ishtp_cl_misc->cl_mutex before actually acquiring it.
> > 
> 
> Thanks for your comments, Jiri, I update my patch below:

Greg, how would you like to handle this (after you're done with reviewing 
it of course), given that this driver depends on the preceeding 6 patches 
to intel-ish-hid driver?

Thanks,

-- 
Jiri Kosina
SUSE Labs

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH 7/7] misc: intel-ish-client: add intel ishtp clients driver
  2017-01-04  9:36       ` Jiri Kosina
@ 2017-01-04 12:59         ` gregkh
  0 siblings, 0 replies; 19+ messages in thread
From: gregkh @ 2017-01-04 12:59 UTC (permalink / raw)
  To: Jiri Kosina
  Cc: Xu, Even, srinivas.pandruvada, linux-input, benjamin.tissoires,
	linux-kernel, arnd, Shevchenko, Andriy

On Wed, Jan 04, 2017 at 10:36:18AM +0100, Jiri Kosina wrote:
> On Wed, 4 Jan 2017, Xu, Even wrote:
> 
> > > [ ... snip ... ]
> > > > 
> > > > +static ssize_t ishtp_cl_write(struct file *file, const char __user
> > > > *ubuf,
> > > > +	size_t length, loff_t *offset)
> > > > +{
> > > > +	struct ishtp_cl_miscdev *ishtp_cl_misc = file-
> > > > >private_data;
> > > > +	struct ishtp_cl *cl;
> > > > +	void *write_buf;
> > > > +	struct ishtp_device *dev;
> > > > +	int ret;
> > > > +
> > > > +	/* Non-blocking semantics are not supported */
> > > > +	if (file->f_flags & O_NONBLOCK) {
> > > > +		ret = -EINVAL;
> > > > +		goto out_unlock;
> > > When taking the error path here you'd try to unlock 
> > > ishtp_cl_misc->cl_mutex before actually acquiring it.
> > > 
> > 
> > Thanks for your comments, Jiri, I update my patch below:
> 
> Greg, how would you like to handle this (after you're done with reviewing 
> it of course), given that this driver depends on the preceeding 6 patches 
> to intel-ish-hid driver?

Let me go review it now, but if it's ok, I have no problem taking it in
my tree, or if you want to take it through yours, that's fine with me,
which ever is easiest for you.

thanks,

greg k-h

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH 7/7] misc: intel-ish-client: add intel ishtp clients driver
  2016-12-23  1:22 ` [PATCH 7/7] misc: intel-ish-client: add intel ishtp clients driver Even Xu
  2017-01-03  9:54   ` Jiri Kosina
@ 2017-01-04 13:03   ` Greg KH
  2017-01-04 17:11     ` Srinivas Pandruvada
  2017-01-04 13:09   ` Greg KH
  2017-01-04 13:13   ` Greg KH
  3 siblings, 1 reply; 19+ messages in thread
From: Greg KH @ 2017-01-04 13:03 UTC (permalink / raw)
  To: Even Xu
  Cc: jikos, benjamin.tissoires, srinivas.pandruvada, arnd,
	andriy.shevchenko, linux-input, linux-kernel

On Fri, Dec 23, 2016 at 09:22:29AM +0800, Even Xu wrote:
> Intel ISHFW supports many different clients, in
> hid/intel-ish-hid/ishtp bus driver, it creates following client devices:
> HID client:
> 	interface of sensor configure and sensor event report.
> SMHI client:
> 	interface of sensor calibration, ISHFW debug, ISHFW performance
> 	analysis and manufacture support.
> Trace client:
> 	interface of ISHFW debug log output.
> Trace configure client:
> 	interface of ISHFW debug log configuration, such as output port,
> 	log level, filter.
> ISHFW loader client:
> 	interface of customized ISHFW loader.
> HID client has been handle by hid/intel-ish-hid/intel-ishtp-hid client
> driver, and rest of the clients export interface using miscellaneous
> drivers. This interface is used by user space tools for debugging and
> calibration of sensors.
> 
> Signed-off-by: Even Xu <even.xu@intel.com>
> Reviewed-by: Andriy Shevchenko <andriy.shevchenko@intel.com>
> Reviewed-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>
> ---
>  drivers/misc/Kconfig                               |   1 +
>  drivers/misc/Makefile                              |   1 +
>  drivers/misc/intel-ish-client/Kconfig              |  15 +
>  drivers/misc/intel-ish-client/Makefile             |   8 +
>  .../misc/intel-ish-client/intel-ishtp-clients.c    | 884 +++++++++++++++++++++
>  include/uapi/linux/intel-ishtp-clients.h           |  73 ++


Why create a whole new subdirectory for just one .c file?  Is that
really needed?

And I'm not quite sure why you need a misc driver, what exactly is this
code doing?

Let me look at your uapi header file:

> --- /dev/null
> +++ b/include/uapi/linux/intel-ishtp-clients.h
> @@ -0,0 +1,73 @@
> +/*
> + * Intel ISHTP Clients Interface Header
> + *
> + * Copyright (c) 2016, Intel Corporation.
> + *
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms and conditions of the GNU General Public License,
> + * version 2, as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope it will be useful, but WITHOUT
> + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
> + * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License for
> + * more details.
> + *
> + */
> +
> +#ifndef _INTEL_ISHTP_CLIENTS_H
> +#define _INTEL_ISHTP_CLIENTS_H
> +
> +#include <linux/ioctl.h>
> +#include <linux/miscdevice.h>
> +#include <linux/mutex.h>
> +#include <linux/types.h>
> +#include <linux/uuid.h>
> +
> +/*
> + * This IOCTL is used to associate the current file descriptor with a
> + * FW Client (given by UUID). This opens a communication channel
> + * between a host client and a FW client. From this point every read and write
> + * will communicate with the associated FW client.
> + * Only in close() (file_operation release()) the communication between
> + * the clients is disconnected

Why do you want to do this?  What will read/write do with this device
now?

> + *
> + * The IOCTL argument is a struct with a union that contains
> + * the input parameter and the output parameter for this IOCTL.

Is that sentance really needed?

> + *
> + * The input parameter is UUID of the FW Client.
> + * The output parameter is the properties of the FW client
> + * (FW protocol version and max message size).
> + *
> + */
> +#define IOCTL_ISHTP_CONNECT_CLIENT	_IOWR('H', 0x81,	\
> +				struct ishtp_connect_client_data)
> +
> +/* Configuration: set number of Rx/Tx buffers. Must be used before connection */
> +#define IOCTL_ISHTP_SET_RX_FIFO_SIZE	_IOWR('H', 0x82, long)
> +#define IOCTL_ISHTP_SET_TX_FIFO_SIZE	_IOWR('H', 0x83, long)

Before connection to what?

> +
> +/* Get FW status */
> +#define IOCTL_ISH_GET_FW_STATUS	_IO('H', 0x84)

What is this?

> +
> +#define IOCTL_ISH_HW_RESET	_IO('H', 0x85)

No documentation?

> +
> +/*
> + * Intel ISHTP client information struct
> + */
> +struct ishtp_client {
> +	__u32 max_msg_length;
> +	__u8 protocol_version;
> +	__u8 reserved[3];
> +};
> +

Nice job using the correct types.

I still don't know what this api does, let me go look at the .c code
now...

thanks,

greg k-h

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH 7/7] misc: intel-ish-client: add intel ishtp clients driver
  2016-12-23  1:22 ` [PATCH 7/7] misc: intel-ish-client: add intel ishtp clients driver Even Xu
  2017-01-03  9:54   ` Jiri Kosina
  2017-01-04 13:03   ` Greg KH
@ 2017-01-04 13:09   ` Greg KH
  2017-01-04 13:13   ` Greg KH
  3 siblings, 0 replies; 19+ messages in thread
From: Greg KH @ 2017-01-04 13:09 UTC (permalink / raw)
  To: Even Xu
  Cc: jikos, benjamin.tissoires, srinivas.pandruvada, arnd,
	andriy.shevchenko, linux-input, linux-kernel

On Fri, Dec 23, 2016 at 09:22:29AM +0800, Even Xu wrote:
> +/**
> + * ishtp_cl_probe() - ISHTP client driver probe
> + * @cl_device:		ISHTP client device instance
> + *
> + * This function gets called on device create on ISHTP bus
> + *
> + * Return: 0 on success, non zero on error
> + */
> +static int ishtp_cl_probe(struct ishtp_cl_device *cl_device)
> +{
> +	struct ishtp_cl_miscdev *ishtp_cl_misc;
> +	int ret;
> +
> +	if (!cl_device)
> +		return -ENODEV;
> +
> +	ishtp_cl_misc = kzalloc(sizeof(struct ishtp_cl_miscdev),
> +				GFP_KERNEL);
> +	if (!ishtp_cl_misc)
> +		return -ENOMEM;
> +
> +	if (uuid_le_cmp(ishtp_smhi_guid,
> +			cl_device->fw_client->props.protocol_name) == 0) {
> +		ishtp_cl_misc->cl_miscdev.name = "ish-smhi";
> +	} else if (uuid_le_cmp(ishtp_trace_guid,
> +			cl_device->fw_client->props.protocol_name) == 0) {
> +		ishtp_cl_misc->cl_miscdev.name = "ish-trace";
> +	} else if (uuid_le_cmp(ishtp_traceconfig_guid,
> +			cl_device->fw_client->props.protocol_name) == 0) {
> +		ishtp_cl_misc->cl_miscdev.name = "ish-tracec";
> +	} else if (uuid_le_cmp(ishtp_loader_guid,
> +			cl_device->fw_client->props.protocol_name) == 0) {
> +		ishtp_cl_misc->cl_miscdev.name = "ish-loader";
> +	} else {
> +		dev_err(&cl_device->dev, "Not supported client\n");
> +		ret = -ENODEV;
> +		goto release_mem;
> +	}
> +
> +	ishtp_cl_misc->cl_miscdev.parent = &cl_device->dev;
> +	ishtp_cl_misc->cl_miscdev.fops = &ishtp_cl_fops;
> +	ishtp_cl_misc->cl_miscdev.minor = MISC_DYNAMIC_MINOR,
> +
> +	ret = misc_register(&ishtp_cl_misc->cl_miscdev);
> +	if (ret) {
> +		dev_err(&cl_device->dev, "misc device register failed\n");
> +		goto release_mem;
> +	}

Now your userspace device node is created and can be opened up and
accessed.  But:

> +
> +	ishtp_cl_misc->cl_device = cl_device;
> +
> +	init_waitqueue_head(&ishtp_cl_misc->read_wait);
> +
> +	ishtp_set_drvdata(cl_device, ishtp_cl_misc);
> +
> +	ishtp_get_device(cl_device);
> +
> +	mutex_init(&ishtp_cl_misc->cl_mutex);
> +
> +	INIT_WORK(&ishtp_cl_misc->reset_work, ishtp_cl_reset_handler);
> +
> +	/* Register event callback */
> +	ishtp_register_event_cb(cl_device, ishtp_cl_event_cb);

You were not done setting up the device.  What nasty races just
happened?

And the above functions can never fail?  Why are you grabbing a refernce
to the cl_device yet doing nothing with it?  That feels really broken
and wrong.

thanks,

greg k-h

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH 7/7] misc: intel-ish-client: add intel ishtp clients driver
  2016-12-23  1:22 ` [PATCH 7/7] misc: intel-ish-client: add intel ishtp clients driver Even Xu
                     ` (2 preceding siblings ...)
  2017-01-04 13:09   ` Greg KH
@ 2017-01-04 13:13   ` Greg KH
  3 siblings, 0 replies; 19+ messages in thread
From: Greg KH @ 2017-01-04 13:13 UTC (permalink / raw)
  To: Even Xu
  Cc: jikos, benjamin.tissoires, srinivas.pandruvada, arnd,
	andriy.shevchenko, linux-input, linux-kernel

On Fri, Dec 23, 2016 at 09:22:29AM +0800, Even Xu wrote:
> +static int ishtp_cl_open(struct inode *inode, struct file *file)
> +{
> +	struct miscdevice *misc = file->private_data;
> +	struct ishtp_cl *cl;
> +	struct ishtp_device *dev;
> +	struct ishtp_cl_miscdev *ishtp_cl_misc;
> +	int ret;
> +
> +	/* Non-blocking semantics are not supported */
> +	if (file->f_flags & O_NONBLOCK)
> +		return -EINVAL;
> +
> +	ishtp_cl_misc = container_of(misc,
> +				struct ishtp_cl_miscdev, cl_miscdev);
> +	if (!ishtp_cl_misc || !ishtp_cl_misc->cl_device)
> +		return -ENODEV;

How can ishtp_cl_misc ever be NULL?  Are you sure you know what
container_of() really does here?  :)

> +	dev = ishtp_cl_misc->cl_device->ishtp_dev;
> +	if (!dev)
> +		return -ENODEV;

How can dev be NULL?

> +
> +	mutex_lock(&ishtp_cl_misc->cl_mutex);
> +
> +	/*
> +	 * Every client only supports one opened instance
> +	 * at the sametime.
> +	 */
> +	if (ishtp_cl_misc->cl) {
> +		ret = -EBUSY;
> +		goto out_unlock;
> +	}
> +
> +	cl = ishtp_cl_allocate(dev);
> +	if (!cl) {
> +		ret = -ENOMEM;
> +		goto out_free;
> +	}
> +
> +	if (dev->dev_state != ISHTP_DEV_ENABLED) {
> +		ret = -ENODEV;
> +		goto out_free;
> +	}
> +
> +	ret = ishtp_cl_link(cl, ISHTP_HOST_CLIENT_ID_ANY);
> +	if (ret)
> +		goto out_free;
> +
> +	ishtp_cl_misc->cl = cl;
> +
> +	file->private_data = ishtp_cl_misc;
> +
> +	mutex_unlock(&ishtp_cl_misc->cl_mutex);
> +
> +	return nonseekable_open(inode, file);
> +
> +out_free:
> +	kfree(cl);
> +out_unlock:
> +	mutex_unlock(&ishtp_cl_misc->cl_mutex);
> +	return ret;
> +}
> +
> +#define WAIT_FOR_SEND_SLICE_MS		100
> +#define WAIT_FOR_SEND_COUNT		10
> +
> +static int ishtp_cl_release(struct inode *inode, struct file *file)
> +{
> +	struct ishtp_cl_miscdev *ishtp_cl_misc = file->private_data;
> +	struct ishtp_cl *cl;
> +	struct ishtp_cl_rb *rb;
> +	struct ishtp_device *dev;
> +	int try = WAIT_FOR_SEND_COUNT;
> +	int ret;
> +
> +	mutex_lock(&ishtp_cl_misc->cl_mutex);
> +
> +	/* Wake up from waiting if anyone wait on it */
> +	wake_up_interruptible(&ishtp_cl_misc->read_wait);
> +
> +	cl = ishtp_cl_misc->cl;
> +	dev = cl->dev;
> +
> +	/*
> +	 * May happen if device sent FW reset or was intentionally
> +	 * halted by host SW. The client is then invalid.
> +	 */
> +	if ((dev->dev_state == ISHTP_DEV_ENABLED) &&
> +			(cl->state == ISHTP_CL_CONNECTED)) {
> +		/*
> +		 * Check and wait 1s for message in tx_list to be sent.
> +		 */
> +		do {
> +			if (!ishtp_cl_tx_empty(cl))
> +				msleep_interruptible(WAIT_FOR_SEND_SLICE_MS);
> +			else
> +				break;
> +		} while (--try);

So just try it a bunch and hope it all works out?  No error message if
something went wrong?  Why not try forever?  Why that specific number of
trys?  This feels hacky.


> +
> +		cl->state = ISHTP_CL_DISCONNECTING;
> +		ret = ishtp_cl_disconnect(cl);
> +	}
> +
> +	ishtp_cl_unlink(cl);
> +	ishtp_cl_flush_queues(cl);
> +	/* Disband and free all Tx and Rx client-level rings */
> +	ishtp_cl_free(cl);
> +
> +	ishtp_cl_misc->cl = NULL;
> +
> +	rb = ishtp_cl_misc->read_rb;
> +	if (rb) {
> +		ishtp_cl_io_rb_recycle(rb);
> +		ishtp_cl_misc->read_rb = NULL;
> +	}
> +
> +	file->private_data = NULL;
> +
> +	mutex_unlock(&ishtp_cl_misc->cl_mutex);
> +
> +	return ret;
> +}
> +
> +static ssize_t ishtp_cl_read(struct file *file, char __user *ubuf,
> +			size_t length, loff_t *offset)
> +{
> +	struct ishtp_cl_miscdev *ishtp_cl_misc = file->private_data;
> +	struct ishtp_cl *cl;
> +	struct ishtp_cl_rb *rb;
> +	struct ishtp_device *dev;
> +	int ret = 0;
> +
> +	/* Non-blocking semantics are not supported */
> +	if (file->f_flags & O_NONBLOCK)
> +		return -EINVAL;
> +
> +	mutex_lock(&ishtp_cl_misc->cl_mutex);
> +
> +	cl = ishtp_cl_misc->cl;
> +
> +	/*
> +	 * ISHFW reset will cause cl be freed and re-allocated.
> +	 * So must make sure cl is re-allocated successfully.
> +	 */
> +	if (!cl || !cl->dev) {
> +		ret = -ENODEV;
> +		goto out_unlock;
> +	}

How can these ever be true?

> +
> +	dev = cl->dev;
> +	if (dev->dev_state != ISHTP_DEV_ENABLED) {
> +		ret = -ENODEV;
> +		goto out_unlock;
> +	}
> +
> +	if (ishtp_cl_misc->read_rb)
> +		goto get_rb;
> +
> +	rb = ishtp_cl_rx_get_rb(cl);
> +	if (rb)
> +		goto copy_buffer;
> +
> +	/*
> +	 * Release mutex for other operation can be processed parallelly
> +	 * during waiting.
> +	 */
> +	mutex_unlock(&ishtp_cl_misc->cl_mutex);
> +
> +	if (wait_event_interruptible(ishtp_cl_misc->read_wait,
> +			ishtp_cl_misc->read_rb != NULL)) {
> +		dev_err(&ishtp_cl_misc->cl_device->dev,
> +			"Wake up not successful;"
> +			"signal pending = %d signal = %08lX\n",
> +			signal_pending(current),
> +			current->pending.signal.sig[0]);

Why spam the error log for this?

> +		return -ERESTARTSYS;
> +	}
> +
> +	mutex_lock(&ishtp_cl_misc->cl_mutex);
> +
> +	/*
> +	 * waitqueue can be woken up in many cases, so must check
> +	 * if dev and cl is still available.
> +	 */
> +	if (dev->dev_state != ISHTP_DEV_ENABLED) {
> +		ret = -ENODEV;
> +		goto out_unlock;
> +	}
> +
> +	cl = ishtp_cl_misc->cl;
> +	if (!cl) {
> +		ret = -ENODEV;
> +		goto out_unlock;
> +	}
> +
> +	if (cl->state == ISHTP_CL_INITIALIZING ||
> +		cl->state == ISHTP_CL_DISCONNECTED ||
> +		cl->state == ISHTP_CL_DISCONNECTING) {
> +		ret = -EBUSY;
> +		goto out_unlock;
> +	}
> +
> +get_rb:
> +	rb = ishtp_cl_misc->read_rb;
> +	if (!rb) {
> +		ret = -ENODEV;
> +		goto out_unlock;
> +	}
> +
> +copy_buffer:
> +	/* Now copy the data to user space */
> +	if (!length || !ubuf || *offset > rb->buf_idx) {
> +		ret = -EMSGSIZE;
> +		goto out_unlock;
> +	}
> +
> +	/*
> +	 * length is being truncated, however buf_idx may
> +	 * point beyond that.
> +	 */
> +	length = min_t(size_t, length, rb->buf_idx - *offset);
> +
> +	if (copy_to_user(ubuf, rb->buffer.data + *offset, length)) {
> +		ret = -EFAULT;
> +		goto out_unlock;
> +	}
> +
> +	*offset += length;
> +	if ((unsigned long)*offset < rb->buf_idx)
> +		goto out_unlock;
> +
> +	ishtp_cl_io_rb_recycle(rb);
> +	ishtp_cl_misc->read_rb = NULL;
> +	*offset = 0;
> +
> +out_unlock:
> +	mutex_unlock(&ishtp_cl_misc->cl_mutex);
> +	return ret < 0 ? ret : length;
> +}

I still don't understand what is being read/written through this
character device node.  What is it used for?

thanks,

greg k-h

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH 7/7] misc: intel-ish-client: add intel ishtp clients driver
  2017-01-04 13:03   ` Greg KH
@ 2017-01-04 17:11     ` Srinivas Pandruvada
  2017-01-04 17:18       ` Greg KH
  0 siblings, 1 reply; 19+ messages in thread
From: Srinivas Pandruvada @ 2017-01-04 17:11 UTC (permalink / raw)
  To: Greg KH, Even Xu
  Cc: jikos, benjamin.tissoires, arnd, andriy.shevchenko, linux-input,
	linux-kernel

On Wed, 2017-01-04 at 14:03 +0100, Greg KH wrote:
> On Fri, Dec 23, 2016 at 09:22:29AM +0800, Even Xu wrote:
> > 
> > Intel ISHFW supports many different clients, in
> > hid/intel-ish-hid/ishtp bus driver, it creates following client
> > devices:
> > HID client:
> > 	interface of sensor configure and sensor event report.
> > SMHI client:
> > 	interface of sensor calibration, ISHFW debug, ISHFW performance
> > 	analysis and manufacture support.
> > Trace client:
> > 	interface of ISHFW debug log output.
> > Trace configure client:
> > 	interface of ISHFW debug log configuration, such as output
> > port,
> > 	log level, filter.
> > ISHFW loader client:
> > 	interface of customized ISHFW loader.
> > HID client has been handle by hid/intel-ish-hid/intel-ishtp-hid
> > client
> > driver, and rest of the clients export interface using
> > miscellaneous
> > drivers. This interface is used by user space tools for debugging
> > and
> > calibration of sensors.
> > 
> > Signed-off-by: Even Xu <even.xu@intel.com>
> > Reviewed-by: Andriy Shevchenko <andriy.shevchenko@intel.com>
> > Reviewed-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.c
> > om>
> > ---
> >  drivers/misc/Kconfig                               |   1 +
> >  drivers/misc/Makefile                              |   1 +
> >  drivers/misc/intel-ish-client/Kconfig              |  15 +
> >  drivers/misc/intel-ish-client/Makefile             |   8 +
> >  .../misc/intel-ish-client/intel-ishtp-clients.c    | 884
> > +++++++++++++++++++++
> >  include/uapi/linux/intel-ishtp-clients.h           |  73 ++
> 
> 
> Why create a whole new subdirectory for just one .c file?  Is that
> really needed?
The other option is to move this .c file to drivers/hid/intel-ish-hid/.
I think the folders inside drivers/hid/ is mostly for just implementing
transport layer for hid devices.


> 
> And I'm not quite sure why you need a misc driver, what exactly is
> this
> code doing?
As described in the description, this driver is a companion driver for
ISH user space tools for calibration, production and debug.

Basically the ISH provided a standalone low power processor to
developers and manufacturers  to do download some custom algorithms for
sensors, which may not be compliant to USB HID sensor specifications
(mostly for IOT space). In that case the user space for those can
communicate using misc driver interface, without adding new kernel
drivers.

Thanks,
Srinivas

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH 7/7] misc: intel-ish-client: add intel ishtp clients driver
  2017-01-04 17:11     ` Srinivas Pandruvada
@ 2017-01-04 17:18       ` Greg KH
  2017-01-04 18:41         ` Srinivas Pandruvada
  0 siblings, 1 reply; 19+ messages in thread
From: Greg KH @ 2017-01-04 17:18 UTC (permalink / raw)
  To: Srinivas Pandruvada
  Cc: Even Xu, jikos, benjamin.tissoires, arnd, andriy.shevchenko,
	linux-input, linux-kernel

On Wed, Jan 04, 2017 at 09:11:34AM -0800, Srinivas Pandruvada wrote:
> On Wed, 2017-01-04 at 14:03 +0100, Greg KH wrote:
> > On Fri, Dec 23, 2016 at 09:22:29AM +0800, Even Xu wrote:
> > > 
> > > Intel ISHFW supports many different clients, in
> > > hid/intel-ish-hid/ishtp bus driver, it creates following client
> > > devices:
> > > HID client:
> > > 	interface of sensor configure and sensor event report.
> > > SMHI client:
> > > 	interface of sensor calibration, ISHFW debug, ISHFW performance
> > > 	analysis and manufacture support.
> > > Trace client:
> > > 	interface of ISHFW debug log output.
> > > Trace configure client:
> > > 	interface of ISHFW debug log configuration, such as output
> > > port,
> > > 	log level, filter.
> > > ISHFW loader client:
> > > 	interface of customized ISHFW loader.
> > > HID client has been handle by hid/intel-ish-hid/intel-ishtp-hid
> > > client
> > > driver, and rest of the clients export interface using
> > > miscellaneous
> > > drivers. This interface is used by user space tools for debugging
> > > and
> > > calibration of sensors.
> > > 
> > > Signed-off-by: Even Xu <even.xu@intel.com>
> > > Reviewed-by: Andriy Shevchenko <andriy.shevchenko@intel.com>
> > > Reviewed-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.c
> > > om>
> > > ---
> > >  drivers/misc/Kconfig                               |   1 +
> > >  drivers/misc/Makefile                              |   1 +
> > >  drivers/misc/intel-ish-client/Kconfig              |  15 +
> > >  drivers/misc/intel-ish-client/Makefile             |   8 +
> > >  .../misc/intel-ish-client/intel-ishtp-clients.c    | 884
> > > +++++++++++++++++++++
> > >  include/uapi/linux/intel-ishtp-clients.h           |  73 ++
> > 
> > 
> > Why create a whole new subdirectory for just one .c file?  Is that
> > really needed?
> The other option is to move this .c file to drivers/hid/intel-ish-hid/.
> I think the folders inside drivers/hid/ is mostly for just implementing
> transport layer for hid devices.
> 
> 
> > 
> > And I'm not quite sure why you need a misc driver, what exactly is
> > this
> > code doing?
> As described in the description, this driver is a companion driver for
> ISH user space tools for calibration, production and debug.

debug should not require a char device node, use debugfs, that is what
it is there for.

For "calibration", why not use configfs or even sysfs?

> Basically the ISH provided a standalone low power processor to
> developers and manufacturers  to do download some custom algorithms for
> sensors, which may not be compliant to USB HID sensor specifications
> (mostly for IOT space). In that case the user space for those can
> communicate using misc driver interface, without adding new kernel
> drivers.

So you hide it behind a char device node?  That's not very descriptive
or easy to understand :)

Again, use the interfaces that the kernel gives you for this type of
stuff, and don't create new one-off ones if at all possible.

thanks,

greg k-h

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH 7/7] misc: intel-ish-client: add intel ishtp clients driver
  2017-01-04 17:18       ` Greg KH
@ 2017-01-04 18:41         ` Srinivas Pandruvada
  2017-01-04 19:40           ` Greg KH
  0 siblings, 1 reply; 19+ messages in thread
From: Srinivas Pandruvada @ 2017-01-04 18:41 UTC (permalink / raw)
  To: Greg KH
  Cc: Even Xu, jikos, benjamin.tissoires, arnd, andriy.shevchenko,
	linux-input, linux-kernel

On Wed, 2017-01-04 at 18:18 +0100, Greg KH wrote:
> On Wed, Jan 04, 2017 at 09:11:34AM -0800, Srinivas Pandruvada wrote:
> > 
> > On Wed, 2017-01-04 at 14:03 +0100, Greg KH wrote:
> > > 
> > > On Fri, Dec 23, 2016 at 09:22:29AM +0800, Even Xu wrote
> > > > 
[...]

> debug should not require a char device node, use debugfs, that is
> what
> it is there for.
> 
> For "calibration", why not use configfs or even sysfs?

We will check on this. There is some legacy with the deployed user
space tools.

> > 
> > Basically the ISH provided a standalone low power processor to
> > developers and manufacturers  to do download some custom algorithms
> > for
> > sensors, which may not be compliant to USB HID sensor
> > specifications
> > (mostly for IOT space). In that case the user space for those can
> > communicate using misc driver interface, without adding new kernel
> > drivers.
> 
> So you hide it behind a char device node?  That's not very
> descriptive
> or easy to understand :)
We added several new sensors to IIO and in process of adding new
sensors to standardize ABI for sensors defined in HID sensor spec.

Customers can develop and download some algorithm which uses output of
several sensors and come up with some fusion sensor to detect some
activity. Either some kernel driver needs to read this and pass this
event to user space or directly let the user space communicate with the
firmware using character device. 
Is there any better way to handle this?

We want customers to use upstream kernel without out of tree kernel
drivers.

Thanks,
Srinivas

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH 7/7] misc: intel-ish-client: add intel ishtp clients driver
  2017-01-04 18:41         ` Srinivas Pandruvada
@ 2017-01-04 19:40           ` Greg KH
  2017-01-05  5:38             ` Xu, Even
  0 siblings, 1 reply; 19+ messages in thread
From: Greg KH @ 2017-01-04 19:40 UTC (permalink / raw)
  To: Srinivas Pandruvada
  Cc: Even Xu, jikos, benjamin.tissoires, arnd, andriy.shevchenko,
	linux-input, linux-kernel

On Wed, Jan 04, 2017 at 10:41:26AM -0800, Srinivas Pandruvada wrote:
> On Wed, 2017-01-04 at 18:18 +0100, Greg KH wrote:
> > On Wed, Jan 04, 2017 at 09:11:34AM -0800, Srinivas Pandruvada wrote:
> > > 
> > > On Wed, 2017-01-04 at 14:03 +0100, Greg KH wrote:
> > > > 
> > > > On Fri, Dec 23, 2016 at 09:22:29AM +0800, Even Xu wrote
> > > > > 
> [...]
> 
> > debug should not require a char device node, use debugfs, that is
> > what
> > it is there for.
> > 
> > For "calibration", why not use configfs or even sysfs?
> 
> We will check on this. There is some legacy with the deployed user
> space tools.

Um, you do know that's not a good reason/excuse at all to take incorrect
kernel code, right?  Please don't use that as any kind of excuse.

> > > Basically the ISH provided a standalone low power processor to
> > > developers and manufacturers  to do download some custom algorithms
> > > for
> > > sensors, which may not be compliant to USB HID sensor
> > > specifications
> > > (mostly for IOT space). In that case the user space for those can
> > > communicate using misc driver interface, without adding new kernel
> > > drivers.
> > 
> > So you hide it behind a char device node?  That's not very
> > descriptive
> > or easy to understand :)
> We added several new sensors to IIO and in process of adding new
> sensors to standardize ABI for sensors defined in HID sensor spec.
> 
> Customers can develop and download some algorithm which uses output of
> several sensors and come up with some fusion sensor to detect some
> activity. Either some kernel driver needs to read this and pass this
> event to user space or directly let the user space communicate with the
> firmware using character device. 
> Is there any better way to handle this?
> 
> We want customers to use upstream kernel without out of tree kernel
> drivers.

Why are you somehow claiming this is an either/or kind of situation?
What out-of-tree kernel modules are there?  Why can't they just be
merged if they are somewhere?

Having an interface to add new types of "functionality" is great, and
fine, but why you think a char device node is that type of api is
confusing to me when I already pointed out an number of other potential
solutions.  Have you tried them out and found they do not work?  If so,
great, please explain what is lacking and we can go from there.

If not, please do some basic research first before trying to claim that
a char device is the only possible solution.

You have run this code through the internal Intel kernel developer
review process on their mailing list, correct?  What did they say about
your current design?  If not, why have you not taken advantage of this
resource?

thanks,

greg k-h

^ permalink raw reply	[flat|nested] 19+ messages in thread

* RE: [PATCH 7/7] misc: intel-ish-client: add intel ishtp clients driver
  2017-01-04 19:40           ` Greg KH
@ 2017-01-05  5:38             ` Xu, Even
  0 siblings, 0 replies; 19+ messages in thread
From: Xu, Even @ 2017-01-05  5:38 UTC (permalink / raw)
  To: Greg KH, Srinivas Pandruvada
  Cc: jikos, benjamin.tissoires, arnd, Shevchenko, Andriy, linux-input,
	linux-kernel

Hi, Greg,

Thanks for your review and suggestion, I will rework my patch based on your suggestion and then submit again.

Best Regards,
Even Xu

-----Original Message-----
From: Greg KH [mailto:gregkh@linuxfoundation.org] 
Sent: Thursday, January 5, 2017 3:41 AM
To: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>
Cc: Xu, Even <even.xu@intel.com>; jikos@kernel.org; benjamin.tissoires@redhat.com; arnd@arndb.de; Shevchenko, Andriy <andriy.shevchenko@intel.com>; linux-input@vger.kernel.org; linux-kernel@vger.kernel.org
Subject: Re: [PATCH 7/7] misc: intel-ish-client: add intel ishtp clients driver

On Wed, Jan 04, 2017 at 10:41:26AM -0800, Srinivas Pandruvada wrote:
> On Wed, 2017-01-04 at 18:18 +0100, Greg KH wrote:
> > On Wed, Jan 04, 2017 at 09:11:34AM -0800, Srinivas Pandruvada wrote:
> > > 
> > > On Wed, 2017-01-04 at 14:03 +0100, Greg KH wrote:
> > > > 
> > > > On Fri, Dec 23, 2016 at 09:22:29AM +0800, Even Xu wrote
> > > > > 
> [...]
> 
> > debug should not require a char device node, use debugfs, that is 
> > what it is there for.
> > 
> > For "calibration", why not use configfs or even sysfs?
> 
> We will check on this. There is some legacy with the deployed user 
> space tools.

Um, you do know that's not a good reason/excuse at all to take incorrect kernel code, right?  Please don't use that as any kind of excuse.

> > > Basically the ISH provided a standalone low power processor to 
> > > developers and manufacturers  to do download some custom 
> > > algorithms for sensors, which may not be compliant to USB HID 
> > > sensor specifications (mostly for IOT space). In that case the 
> > > user space for those can communicate using misc driver interface, 
> > > without adding new kernel drivers.
> > 
> > So you hide it behind a char device node?  That's not very 
> > descriptive or easy to understand :)
> We added several new sensors to IIO and in process of adding new 
> sensors to standardize ABI for sensors defined in HID sensor spec.
> 
> Customers can develop and download some algorithm which uses output of 
> several sensors and come up with some fusion sensor to detect some 
> activity. Either some kernel driver needs to read this and pass this 
> event to user space or directly let the user space communicate with 
> the firmware using character device.
> Is there any better way to handle this?
> 
> We want customers to use upstream kernel without out of tree kernel 
> drivers.

Why are you somehow claiming this is an either/or kind of situation?
What out-of-tree kernel modules are there?  Why can't they just be merged if they are somewhere?

Having an interface to add new types of "functionality" is great, and fine, but why you think a char device node is that type of api is confusing to me when I already pointed out an number of other potential solutions.  Have you tried them out and found they do not work?  If so, great, please explain what is lacking and we can go from there.

If not, please do some basic research first before trying to claim that a char device is the only possible solution.

You have run this code through the internal Intel kernel developer review process on their mailing list, correct?  What did they say about your current design?  If not, why have you not taken advantage of this resource?

thanks,

greg k-h

^ permalink raw reply	[flat|nested] 19+ messages in thread

end of thread, other threads:[~2017-01-05  5:39 UTC | newest]

Thread overview: 19+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-12-23  1:22 [PATCH 1/7] hid: intel-ish-hid: ishtp: add helper function for driver data get/set Even Xu
2016-12-23  1:22 ` [PATCH 2/7] hid: intel-ish-hid: use helper function for private driver data set/get Even Xu
2016-12-23  1:22 ` [PATCH 3/7] hid: intel-ish-hid: ishtp: add helper functions for client buffer operation Even Xu
2016-12-23  1:22 ` [PATCH 4/7] hid: intel-ish-hid: use helper function to access client buffer Even Xu
2016-12-23  1:22 ` [PATCH 5/7] hid: intel-ish-hid: ishtp: add helper function for client search Even Xu
2016-12-23  1:22 ` [PATCH 6/7] hid: intel-ish-hid: use helper function to search client id Even Xu
2016-12-23  1:22 ` [PATCH 7/7] misc: intel-ish-client: add intel ishtp clients driver Even Xu
2017-01-03  9:54   ` Jiri Kosina
2017-01-04  6:55     ` Xu, Even
2017-01-04  9:36       ` Jiri Kosina
2017-01-04 12:59         ` gregkh
2017-01-04 13:03   ` Greg KH
2017-01-04 17:11     ` Srinivas Pandruvada
2017-01-04 17:18       ` Greg KH
2017-01-04 18:41         ` Srinivas Pandruvada
2017-01-04 19:40           ` Greg KH
2017-01-05  5:38             ` Xu, Even
2017-01-04 13:09   ` Greg KH
2017-01-04 13:13   ` Greg KH

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).