* [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 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
* 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
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).