| Message ID | 1482456149-4841-7-git-send-email-even.xu@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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.
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 -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 */