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

Groups > linux.kernel > #1550747 > unrolled thread

Re: [PATCH 7/7] misc: intel-ish-client: add intel ishtp clients driver

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH 7/7] misc: intel-ish-client: add intel ishtp clients  driver Greg KH <gregkh@linuxfoundation.org> - 2017-01-04 14:10 +0100
    Re: [PATCH 7/7] misc: intel-ish-client: add intel ishtp clients  driver Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com> - 2017-01-04 18:20 +0100
      Re: [PATCH 7/7] misc: intel-ish-client: add intel ishtp clients  driver Greg KH <gregkh@linuxfoundation.org> - 2017-01-04 19:00 +0100
        Re: [PATCH 7/7] misc: intel-ish-client: add intel ishtp clients  driver Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com> - 2017-01-04 19:50 +0100
          Re: [PATCH 7/7] misc: intel-ish-client: add intel ishtp clients  driver Greg KH <gregkh@linuxfoundation.org> - 2017-01-04 20:50 +0100
            RE: [PATCH 7/7] misc: intel-ish-client: add intel ishtp clients  driver "Xu, Even" <even.xu@intel.com> - 2017-01-05 06:40 +0100

#1550747 — Re: [PATCH 7/7] misc: intel-ish-client: add intel ishtp clients driver

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

[toc] | [next] | [standalone]

#1550998

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

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

#1551041

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

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

#1551108

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

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

#1551133

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

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

#1551641

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

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web