[char-misc-next 0/3] mei support more devices

[char-misc-next 0/3] mei support more devices

[Tomas Winkler]

There is a new use case to support secondary head
on mei devices hence we replace misc devices with 
char.

Alexander Usyskin (2):
  mei: move from misc to char device
  mei: add WPT second mei interface

Tomas Winkler (1):
  mei: sysfs: add Documentation mei class attributes

 Documentation/ABI/testing/sysfs-class-mei | 23 ++++++++
 drivers/misc/mei/hw-me-regs.h             |  1 +
 drivers/misc/mei/hw-me.c                  |  5 ++
 drivers/misc/mei/hw-me.h                  |  1 +
 drivers/misc/mei/main.c                   | 98 ++++++++++++++++++++++---------
 drivers/misc/mei/mei_dev.h                |  9 +++
 drivers/misc/mei/pci-me.c                 |  2 +-
 7 files changed, 111 insertions(+), 28 deletions(-)
 create mode 100644 Documentation/ABI/testing/sysfs-class-mei

-- 
1.9.0

[char-misc-next 1/3] mei: move from misc to char device

[Tomas Winkler]

From: Alexander Usyskin <alexander.usyskin@intel.com>

We need to support more then one mei interface
hence the simple misc devices is not longer an option.
We use char device now with to not break application
space we preserve /dev/mei for the first interface.

Signed-off-by: Alexander Usyskin <alexander.usyskin@intel.com>
Signed-off-by: Tomas Winkler <tomas.winkler@intel.com>
---
 drivers/misc/mei/main.c    | 98 +++++++++++++++++++++++++++++++++-------------
 drivers/misc/mei/mei_dev.h |  7 ++++
 drivers/misc/mei/pci-me.c  |  1 -
 3 files changed, 78 insertions(+), 28 deletions(-)

diff --git a/drivers/misc/mei/main.c b/drivers/misc/mei/main.c
index 66f0a1a..c58e059 100644
--- a/drivers/misc/mei/main.c
+++ b/drivers/misc/mei/main.c
@@ -32,7 +32,6 @@
 #include <linux/compat.h>
 #include <linux/jiffies.h>
 #include <linux/interrupt.h>
-#include <linux/miscdevice.h>
 
 #include <linux/mei.h>
 
@@ -49,19 +48,12 @@
  */
 static int mei_open(struct inode *inode, struct file *file)
 {
-	struct miscdevice *misc = file->private_data;
-	struct pci_dev *pdev;
 	struct mei_cl *cl;
 	struct mei_device *dev;
 
 	int err;
 
-	if (!misc->parent)
-		return -ENODEV;
-
-	pdev = container_of(misc->parent, struct pci_dev, dev);
-
-	dev = pci_get_drvdata(pdev);
+	dev = container_of(inode->i_cdev, struct mei_device, cdev);
 	if (!dev)
 		return -ENODEV;
 
@@ -667,26 +659,48 @@ static const struct file_operations mei_fops = {
 	.llseek = no_llseek
 };
 
-/*
- * Misc Device Struct
- */
-static struct miscdevice  mei_misc_device = {
-		.name = "mei",
-		.fops = &mei_fops,
-		.minor = MISC_DYNAMIC_MINOR,
-};
-
+static struct class *mei_class;
+static dev_t mei_devt;
+#define MAX_MEI_DEVS 5            /* Maximum number of mei devices */
 
 int mei_register(struct mei_device *dev)
 {
-	int ret;
-	mei_misc_device.parent = &dev->pdev->dev;
-	ret = misc_register(&mei_misc_device);
-	if (ret)
+
+	int ret, devno;
+	int id = 0; /* FIXME: retrieve interface version*/
+
+	/* Fill in the data structures */
+	devno = MKDEV(MAJOR(mei_devt), id);
+	cdev_init(&dev->cdev, &mei_fops);
+	dev->cdev.owner = mei_fops.owner;
+
+	/* Add the device */
+	ret = cdev_add(&dev->cdev, devno, 1);
+	if (ret) {
+		dev_err(&dev->pdev->dev, "mei%d unable to add device %d:%d\n",
+			id, MAJOR(mei_devt), id);
+		return ret;
+	}
+
+	/* Preserve /dev/mei for the first interface */
+	if (id)
+		dev->dev = device_create(mei_class, NULL, devno,
+					 NULL, "mei%d", id);
+	else
+		dev->dev = device_create(mei_class, NULL, devno,
+					NULL, "mei");
+
+	if (IS_ERR(dev->dev)) {
+		dev_err(&dev->pdev->dev, "mei%d unable to create device %d:%d\n",
+			id, MAJOR(mei_devt), id);
+		cdev_del(&dev->cdev);
+		ret = PTR_ERR(dev->dev);
 		return ret;
+	}
 
-	if (mei_dbgfs_register(dev, mei_misc_device.name))
-		dev_err(&dev->pdev->dev, "cannot register debugfs\n");
+	ret = mei_dbgfs_register(dev, dev->dev->kobj.name);
+	if (ret)
+		dev_err(dev->dev, "cannot register debugfs ret = %d\n", ret);
 
 	return 0;
 }
@@ -694,19 +708,49 @@ EXPORT_SYMBOL_GPL(mei_register);
 
 void mei_deregister(struct mei_device *dev)
 {
+	int devno;
+
+	devno = dev->cdev.dev;
+	cdev_del(&dev->cdev);
+
 	mei_dbgfs_deregister(dev);
-	misc_deregister(&mei_misc_device);
-	mei_misc_device.parent = NULL;
+
+	device_destroy(mei_class, devno);
 }
 EXPORT_SYMBOL_GPL(mei_deregister);
 
 static int __init mei_init(void)
 {
-	return mei_cl_bus_init();
+	int ret;
+
+	mei_class = class_create(THIS_MODULE, "mei");
+	if (IS_ERR(mei_class)) {
+		pr_err("couldn't create class\n");
+		return PTR_ERR(mei_class);
+	}
+
+	ret = alloc_chrdev_region(&mei_devt, 0, MAX_MEI_DEVS, "mei");
+	if (ret < 0) {
+		pr_err("unable to allocate char dev region\n");
+		goto err;
+	}
+
+	ret = mei_cl_bus_init();
+	if (ret < 0) {
+		pr_err("unable to initialize bus\n");
+		goto err;
+	}
+
+	return 0;
+err:
+	class_destroy(mei_class);
+	return ret;
 }
 
 static void __exit mei_exit(void)
 {
+	unregister_chrdev_region(mei_devt, MAX_MEI_DEVS);
+	class_destroy(mei_class);
 	mei_cl_bus_exit();
 }
 
diff --git a/drivers/misc/mei/mei_dev.h b/drivers/misc/mei/mei_dev.h
index 5c7e990..a418565 100644
--- a/drivers/misc/mei/mei_dev.h
+++ b/drivers/misc/mei/mei_dev.h
@@ -400,6 +400,10 @@ struct mei_cfg {
 /**
  * struct mei_device -  MEI private device struct
 
+ * @pdev - pointer to pci device struct
+ * @cdev - character device
+ * @dev - pointer to device object
+ *
  * @reset_count - limits the number of consecutive resets
  * @hbm_state - state of host bus message protocol
  * @pg_event - power gating event
@@ -412,6 +416,9 @@ struct mei_cfg {
  */
 struct mei_device {
 	struct pci_dev *pdev;	/* pointer to pci device struct */
+	struct cdev cdev;
+	struct device *dev;
+
 	/*
 	 * lists of queues
 	 */
diff --git a/drivers/misc/mei/pci-me.c b/drivers/misc/mei/pci-me.c
index 1b46c64..cf4f654 100644
--- a/drivers/misc/mei/pci-me.c
+++ b/drivers/misc/mei/pci-me.c
@@ -31,7 +31,6 @@
 #include <linux/compat.h>
 #include <linux/jiffies.h>
 #include <linux/interrupt.h>
-#include <linux/miscdevice.h>
 
 #include <linux/pm_runtime.h>
 
-- 
1.9.0

[char-misc-next 2/3] mei: sysfs: add Documentation mei class attributes

[Tomas Winkler]

Add sysfs attributes Documentation entries
for /sys/class/mei

Signed-off-by: Tomas Winkler <tomas.winkler@intel.com>
Signed-off-by: Alexander Usyskin <alexander.usyskin@intel.com>
---
 Documentation/ABI/testing/sysfs-class-mei | 23 +++++++++++++++++++++++
 1 file changed, 23 insertions(+)
 create mode 100644 Documentation/ABI/testing/sysfs-class-mei

diff --git a/Documentation/ABI/testing/sysfs-class-mei b/Documentation/ABI/testing/sysfs-class-mei
new file mode 100644
index 0000000..b691c2b
--- /dev/null
+++ b/Documentation/ABI/testing/sysfs-class-mei
@@ -0,0 +1,23 @@
+What:		/sys/class/mei/
+Date:		May 2014
+KernelVersion:	3.16
+Contact:	Tomas Winkler <tomas.winkler@intel.com>
+Description:
+		The mei/ class sub-directory belongs to mei device class
+
+What:		/sys/class/mei/mei/
+Date:		May 2014
+KernelVersion:	3.16
+Contact:	Tomas Winkler <tomas.winkler@intel.com>
+Description:
+		The /sys/class/mei/mei directory is created for the default
+		mei device
+
+What:		/sys/class/mei/meiN/
+Date:		May 2014
+KernelVersion:	3.16
+Contact:	Tomas Winkler <tomas.winkler@intel.com>
+Description:
+		The /sys/class/mei/meiN directory is created for
+		each probed non default mei device
+
-- 
1.9.0

[char-misc-next 3/3] mei: add WPT second mei interface

[Tomas Winkler]

From: Alexander Usyskin <alexander.usyskin@intel.com>

Add WPT second mei interface.
In order for correct device numbering we add mei device interface id
to to mei_cfg structure

Signed-off-by: Alexander Usyskin <alexander.usyskin@intel.com>
Signed-off-by: Tomas Winkler <tomas.winkler@intel.com>
---
 drivers/misc/mei/hw-me-regs.h | 1 +
 drivers/misc/mei/hw-me.c      | 5 +++++
 drivers/misc/mei/hw-me.h      | 1 +
 drivers/misc/mei/main.c       | 2 +-
 drivers/misc/mei/mei_dev.h    | 2 ++
 drivers/misc/mei/pci-me.c     | 1 +
 6 files changed, 11 insertions(+), 1 deletion(-)

diff --git a/drivers/misc/mei/hw-me-regs.h b/drivers/misc/mei/hw-me-regs.h
index a7856c0..c5feafd 100644
--- a/drivers/misc/mei/hw-me-regs.h
+++ b/drivers/misc/mei/hw-me-regs.h
@@ -115,6 +115,7 @@
 #define MEI_DEV_ID_LPT_HR     0x8CBA  /* Lynx Point H Refresh */
 
 #define MEI_DEV_ID_WPT_LP     0x9CBA  /* Wildcat Point LP */
+#define MEI_DEV_ID_WPT_LP_2   0x9CBB  /* Wildcat Point LP 2 */
 
 /* Host Firmware Status Registers in PCI Config Space */
 #define PCI_CFG_HFS_1         0x40
diff --git a/drivers/misc/mei/hw-me.c b/drivers/misc/mei/hw-me.c
index 6a2d272..9066bb1 100644
--- a/drivers/misc/mei/hw-me.c
+++ b/drivers/misc/mei/hw-me.c
@@ -844,6 +844,11 @@ const struct mei_cfg mei_me_pch_cfg = {
 	MEI_CFG_PCH_HFS,
 };
 
+/* PCH devices MEI 2 interface */
+const struct mei_cfg mei_me_pch_2_cfg = {
+	MEI_CFG_PCH_HFS,
+	.mei_id = 1
+};
 
 /* PCH Cougar Point and Patsburg with quirk for Node Manager exclusion */
 const struct mei_cfg mei_me_pch_cpt_pbg_cfg = {
diff --git a/drivers/misc/mei/hw-me.h b/drivers/misc/mei/hw-me.h
index 12b0f4b..6fb841f 100644
--- a/drivers/misc/mei/hw-me.h
+++ b/drivers/misc/mei/hw-me.h
@@ -41,6 +41,7 @@ struct mei_me_hw {
 extern const struct mei_cfg mei_me_legacy_cfg;
 extern const struct mei_cfg mei_me_ich_cfg;
 extern const struct mei_cfg mei_me_pch_cfg;
+extern const struct mei_cfg mei_me_pch_2_cfg;
 extern const struct mei_cfg mei_me_pch_cpt_pbg_cfg;
 extern const struct mei_cfg mei_me_lpt_cfg;
 
diff --git a/drivers/misc/mei/main.c b/drivers/misc/mei/main.c
index c58e059..cf4ed5a 100644
--- a/drivers/misc/mei/main.c
+++ b/drivers/misc/mei/main.c
@@ -667,7 +667,7 @@ int mei_register(struct mei_device *dev)
 {
 
 	int ret, devno;
-	int id = 0; /* FIXME: retrieve interface version*/
+	int id = dev->cfg->mei_id;
 
 	/* Fill in the data structures */
 	devno = MKDEV(MAJOR(mei_devt), id);
diff --git a/drivers/misc/mei/mei_dev.h b/drivers/misc/mei/mei_dev.h
index a418565..a521783 100644
--- a/drivers/misc/mei/mei_dev.h
+++ b/drivers/misc/mei/mei_dev.h
@@ -384,10 +384,12 @@ enum mei_pg_state {
  *
  * @fw_status - FW status
  * @quirk_probe - device exclusion quirk
+ * @mei_id - id of mei device
  */
 struct mei_cfg {
 	const struct mei_fw_status fw_status;
 	bool (*quirk_probe)(struct pci_dev *pdev);
+	int mei_id;
 };
 
 
diff --git a/drivers/misc/mei/pci-me.c b/drivers/misc/mei/pci-me.c
index cf4f654..bb1a8c9 100644
--- a/drivers/misc/mei/pci-me.c
+++ b/drivers/misc/mei/pci-me.c
@@ -81,6 +81,7 @@ static const struct pci_device_id mei_me_pci_tbl[] = {
 	{MEI_PCI_DEVICE(MEI_DEV_ID_LPT_LP, mei_me_pch_cfg)},
 	{MEI_PCI_DEVICE(MEI_DEV_ID_LPT_HR, mei_me_lpt_cfg)},
 	{MEI_PCI_DEVICE(MEI_DEV_ID_WPT_LP, mei_me_pch_cfg)},
+	{MEI_PCI_DEVICE(MEI_DEV_ID_WPT_LP_2, mei_me_pch_2_cfg)},
 
 	/* required last entry */
 	{0, }
-- 
1.9.0

Re: [char-misc-next 1/3] mei: move from misc to char device

[Greg KH]

On Tue, May 13, 2014 at 11:21:59AM +0300, Tomas Winkler wrote:
> From: Alexander Usyskin <alexander.usyskin@intel.com>
> 
> We need to support more then one mei interface
> hence the simple misc devices is not longer an option.
> We use char device now with to not break application
> space we preserve /dev/mei for the first interface.
> 
> Signed-off-by: Alexander Usyskin <alexander.usyskin@intel.com>
> Signed-off-by: Tomas Winkler <tomas.winkler@intel.com>
> ---
>  drivers/misc/mei/main.c    | 98 +++++++++++++++++++++++++++++++++-------------
>  drivers/misc/mei/mei_dev.h |  7 ++++
>  drivers/misc/mei/pci-me.c  |  1 -
>  3 files changed, 78 insertions(+), 28 deletions(-)
> 
> diff --git a/drivers/misc/mei/main.c b/drivers/misc/mei/main.c
> index 66f0a1a..c58e059 100644
> --- a/drivers/misc/mei/main.c
> +++ b/drivers/misc/mei/main.c
> @@ -32,7 +32,6 @@
>  #include <linux/compat.h>
>  #include <linux/jiffies.h>
>  #include <linux/interrupt.h>
> -#include <linux/miscdevice.h>
>  
>  #include <linux/mei.h>
>  
> @@ -49,19 +48,12 @@
>   */
>  static int mei_open(struct inode *inode, struct file *file)
>  {
> -	struct miscdevice *misc = file->private_data;
> -	struct pci_dev *pdev;
>  	struct mei_cl *cl;
>  	struct mei_device *dev;
>  
>  	int err;
>  
> -	if (!misc->parent)
> -		return -ENODEV;
> -
> -	pdev = container_of(misc->parent, struct pci_dev, dev);
> -
> -	dev = pci_get_drvdata(pdev);
> +	dev = container_of(inode->i_cdev, struct mei_device, cdev);
>  	if (!dev)
>  		return -ENODEV;
>  
> @@ -667,26 +659,48 @@ static const struct file_operations mei_fops = {
>  	.llseek = no_llseek
>  };
>  
> -/*
> - * Misc Device Struct
> - */
> -static struct miscdevice  mei_misc_device = {
> -		.name = "mei",
> -		.fops = &mei_fops,
> -		.minor = MISC_DYNAMIC_MINOR,
> -};
> -
> +static struct class *mei_class;
> +static dev_t mei_devt;
> +#define MAX_MEI_DEVS 5            /* Maximum number of mei devices */

Why have a max number at all?

>  
>  int mei_register(struct mei_device *dev)
>  {
> -	int ret;
> -	mei_misc_device.parent = &dev->pdev->dev;
> -	ret = misc_register(&mei_misc_device);
> -	if (ret)
> +
> +	int ret, devno;
> +	int id = 0; /* FIXME: retrieve interface version*/

You still are only allocating 1 mei device, so why do all of this work
at all?  Nothing changes, so why should I accept this patch?

greg k-h

Re: [char-misc-next 3/3] mei: add WPT second mei interface

[Greg KH]

On Tue, May 13, 2014 at 11:22:01AM +0300, Tomas Winkler wrote:
> From: Alexander Usyskin <alexander.usyskin@intel.com>
> 
> Add WPT second mei interface.
> In order for correct device numbering we add mei device interface id
> to to mei_cfg structure
> 
> Signed-off-by: Alexander Usyskin <alexander.usyskin@intel.com>
> Signed-off-by: Tomas Winkler <tomas.winkler@intel.com>
> ---
>  drivers/misc/mei/hw-me-regs.h | 1 +
>  drivers/misc/mei/hw-me.c      | 5 +++++
>  drivers/misc/mei/hw-me.h      | 1 +
>  drivers/misc/mei/main.c       | 2 +-
>  drivers/misc/mei/mei_dev.h    | 2 ++
>  drivers/misc/mei/pci-me.c     | 1 +
>  6 files changed, 11 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/misc/mei/hw-me-regs.h b/drivers/misc/mei/hw-me-regs.h
> index a7856c0..c5feafd 100644
> --- a/drivers/misc/mei/hw-me-regs.h
> +++ b/drivers/misc/mei/hw-me-regs.h
> @@ -115,6 +115,7 @@
>  #define MEI_DEV_ID_LPT_HR     0x8CBA  /* Lynx Point H Refresh */
>  
>  #define MEI_DEV_ID_WPT_LP     0x9CBA  /* Wildcat Point LP */
> +#define MEI_DEV_ID_WPT_LP_2   0x9CBB  /* Wildcat Point LP 2 */
>  
>  /* Host Firmware Status Registers in PCI Config Space */
>  #define PCI_CFG_HFS_1         0x40
> diff --git a/drivers/misc/mei/hw-me.c b/drivers/misc/mei/hw-me.c
> index 6a2d272..9066bb1 100644
> --- a/drivers/misc/mei/hw-me.c
> +++ b/drivers/misc/mei/hw-me.c
> @@ -844,6 +844,11 @@ const struct mei_cfg mei_me_pch_cfg = {
>  	MEI_CFG_PCH_HFS,
>  };
>  
> +/* PCH devices MEI 2 interface */
> +const struct mei_cfg mei_me_pch_2_cfg = {
> +	MEI_CFG_PCH_HFS,
> +	.mei_id = 1

That's going to be a recipe for disaster.  Have the MEI core allocate
the id numbers as things are registered, don't have the individual
drivers create their id.

> +};
>  
>  /* PCH Cougar Point and Patsburg with quirk for Node Manager exclusion */
>  const struct mei_cfg mei_me_pch_cpt_pbg_cfg = {
> diff --git a/drivers/misc/mei/hw-me.h b/drivers/misc/mei/hw-me.h
> index 12b0f4b..6fb841f 100644
> --- a/drivers/misc/mei/hw-me.h
> +++ b/drivers/misc/mei/hw-me.h
> @@ -41,6 +41,7 @@ struct mei_me_hw {
>  extern const struct mei_cfg mei_me_legacy_cfg;
>  extern const struct mei_cfg mei_me_ich_cfg;
>  extern const struct mei_cfg mei_me_pch_cfg;
> +extern const struct mei_cfg mei_me_pch_2_cfg;
>  extern const struct mei_cfg mei_me_pch_cpt_pbg_cfg;
>  extern const struct mei_cfg mei_me_lpt_cfg;
>  
> diff --git a/drivers/misc/mei/main.c b/drivers/misc/mei/main.c
> index c58e059..cf4ed5a 100644
> --- a/drivers/misc/mei/main.c
> +++ b/drivers/misc/mei/main.c
> @@ -667,7 +667,7 @@ int mei_register(struct mei_device *dev)
>  {
>  
>  	int ret, devno;
> -	int id = 0; /* FIXME: retrieve interface version*/
> +	int id = dev->cfg->mei_id;

Why can't you do this in patch 01 of this series?


>  
>  	/* Fill in the data structures */
>  	devno = MKDEV(MAJOR(mei_devt), id);
> diff --git a/drivers/misc/mei/mei_dev.h b/drivers/misc/mei/mei_dev.h
> index a418565..a521783 100644
> --- a/drivers/misc/mei/mei_dev.h
> +++ b/drivers/misc/mei/mei_dev.h
> @@ -384,10 +384,12 @@ enum mei_pg_state {
>   *
>   * @fw_status - FW status
>   * @quirk_probe - device exclusion quirk
> + * @mei_id - id of mei device
>   */
>  struct mei_cfg {
>  	const struct mei_fw_status fw_status;
>  	bool (*quirk_probe)(struct pci_dev *pdev);
> +	int mei_id;

This should be part of the device, not the configuration, for the reason
specified above.

thanks,

greg k-h

RE: [char-misc-next 3/3] mei: add WPT second mei interface

[Winkler, Tomas]

> > +/* PCH devices MEI 2 interface */
> > +const struct mei_cfg mei_me_pch_2_cfg = {
> > +	MEI_CFG_PCH_HFS,
> > +	.mei_id = 1
> 
> That's going to be a recipe for disaster.  Have the MEI core allocate
> the id numbers as things are registered, don't have the individual
> drivers create their id.

I'm don't think can ensure the enumeration order.
This is per device not per driver configuration structure.
Each pci device is actually just  another head to one MEI device but heads are not equal the name/id matters
Yes I assume it looks odd at the first glance, anyhow we are open to any reasonable suggestions 
 

> > diff --git a/drivers/misc/mei/main.c b/drivers/misc/mei/main.c
> > index c58e059..cf4ed5a 100644
> > --- a/drivers/misc/mei/main.c
> > +++ b/drivers/misc/mei/main.c
> > @@ -667,7 +667,7 @@ int mei_register(struct mei_device *dev)
> >  {
> >
> >  	int ret, devno;
> > -	int id = 0; /* FIXME: retrieve interface version*/
> > +	int id = dev->cfg->mei_id;
> 
> Why can't you do this in patch 01 of this series?

It was just progress of development we can refactor that, no problem 
> 
> 
> >
> >  	/* Fill in the data structures */
> >  	devno = MKDEV(MAJOR(mei_devt), id);
> > diff --git a/drivers/misc/mei/mei_dev.h b/drivers/misc/mei/mei_dev.h
> > index a418565..a521783 100644
> > --- a/drivers/misc/mei/mei_dev.h
> > +++ b/drivers/misc/mei/mei_dev.h
> > @@ -384,10 +384,12 @@ enum mei_pg_state {
> >   *
> >   * @fw_status - FW status
> >   * @quirk_probe - device exclusion quirk
> > + * @mei_id - id of mei device
> >   */
> >  struct mei_cfg {
> >  	const struct mei_fw_status fw_status;
> >  	bool (*quirk_probe)(struct pci_dev *pdev);
> > +	int mei_id;
> 
> This should be part of the device, not the configuration, for the reason
> specified above.

As stated above this is per devices configuration.



Thanks
Tomas

Re: [char-misc-next 3/3] mei: add WPT second mei interface

[Greg KH]

On Tue, May 27, 2014 at 09:42:19PM +0000, Winkler, Tomas wrote:
> > > +/* PCH devices MEI 2 interface */
> > > +const struct mei_cfg mei_me_pch_2_cfg = {
> > > +	MEI_CFG_PCH_HFS,
> > > +	.mei_id = 1
> > 
> > That's going to be a recipe for disaster.  Have the MEI core allocate
> > the id numbers as things are registered, don't have the individual
> > drivers create their id.
> 
> I'm don't think can ensure the enumeration order.

You should not be relying on the order to get anything right.

> This is per device not per driver configuration structure.
> Each pci device is actually just  another head to one MEI device but heads are not equal the name/id matters
> Yes I assume it looks odd at the first glance, anyhow we are open to any reasonable suggestions 

Just dynamically allocate the numbers like all other subsystems do?

Then userspace can open the device nodes it cares about, it should be
able to somehow tell what device is what somehow, right?  If not, you
are doing something wrong with the interface as you can't rely on minor
numbers.

thanks,

greg k-h

RE: [char-misc-next 3/3] mei: add WPT second mei interface

[Winkler, Tomas]

> -----Original Message-----
> From: Greg KH [mailto:gregkh@linuxfoundation.org]
> Sent: Wednesday, May 28, 2014 01:07
> To: Winkler, Tomas
> Cc: arnd@arndb.de; linux-kernel@vger.kernel.org; Usyskin, Alexander
> Subject: Re: [char-misc-next 3/3] mei: add WPT second mei interface
> 
> On Tue, May 27, 2014 at 09:42:19PM +0000, Winkler, Tomas wrote:
> > > > +/* PCH devices MEI 2 interface */
> > > > +const struct mei_cfg mei_me_pch_2_cfg = {
> > > > +	MEI_CFG_PCH_HFS,
> > > > +	.mei_id = 1
> > >
> > > That's going to be a recipe for disaster.  Have the MEI core allocate
> > > the id numbers as things are registered, don't have the individual
> > > drivers create their id.
> >
> > I'm don't think can ensure the enumeration order.
> 
> You should not be relying on the order to get anything right.
> 
> > This is per device not per driver configuration structure.
> > Each pci device is actually just  another head to one MEI device but heads are not
> equal the name/id matters
> > Yes I assume it looks odd at the first glance, anyhow we are open to any
> reasonable suggestions
> 
> Just dynamically allocate the numbers like all other subsystems do?
>
> Then userspace can open the device nodes it cares about, it should be
> able to somehow tell what device is what somehow, right?  If not, you
> are doing something wrong with the interface as you can't rely on minor
> numbers.
> 

The only way for user space to distinguish among interfaces is the same way as driver does by looking for device id. 
The assignment of the device name/id is pretty much static in the HW. Now the same device id database from the driver has to be copied 
to the udev rules or equivalent  and kept in sync.   
Most importantly all the legacy user space just opens /dev/mei  so I wanted to leave /dev/mei (not /dev/mei0) to be assigned always to the first mei head and to not break existing user space. 

Can you be a bit more specific on why we cannot depend on minor numbers and default naming rules, what is the scenario that will break? 

Thanks
Tomas

Re: [char-misc-next 3/3] mei: add WPT second mei interface

[Greg KH]

On Tue, May 27, 2014 at 11:47:44PM +0000, Winkler, Tomas wrote:
> 
> 
> > -----Original Message-----
> > From: Greg KH [mailto:gregkh@linuxfoundation.org]
> > Sent: Wednesday, May 28, 2014 01:07
> > To: Winkler, Tomas
> > Cc: arnd@arndb.de; linux-kernel@vger.kernel.org; Usyskin, Alexander
> > Subject: Re: [char-misc-next 3/3] mei: add WPT second mei interface
> > 
> > On Tue, May 27, 2014 at 09:42:19PM +0000, Winkler, Tomas wrote:
> > > > > +/* PCH devices MEI 2 interface */
> > > > > +const struct mei_cfg mei_me_pch_2_cfg = {
> > > > > +	MEI_CFG_PCH_HFS,
> > > > > +	.mei_id = 1
> > > >
> > > > That's going to be a recipe for disaster.  Have the MEI core allocate
> > > > the id numbers as things are registered, don't have the individual
> > > > drivers create their id.
> > >
> > > I'm don't think can ensure the enumeration order.
> > 
> > You should not be relying on the order to get anything right.
> > 
> > > This is per device not per driver configuration structure.
> > > Each pci device is actually just  another head to one MEI device but heads are not
> > equal the name/id matters
> > > Yes I assume it looks odd at the first glance, anyhow we are open to any
> > reasonable suggestions
> > 
> > Just dynamically allocate the numbers like all other subsystems do?
> >
> > Then userspace can open the device nodes it cares about, it should be
> > able to somehow tell what device is what somehow, right?  If not, you
> > are doing something wrong with the interface as you can't rely on minor
> > numbers.
> > 
> 
> The only way for user space to distinguish among interfaces is the same way as driver does by looking for device id. 

What's with the lack of line-wrapping?

Anyway, why does userspace care?  If it wants to look at the device id,
then just look at the symlink to the device id of the device node.  Then
you don't care what the device node is named / numbered.

> The assignment of the device name/id is pretty much static in the HW.
> Now the same device id database from the driver has to be copied to
> the udev rules or equivalent  and kept in sync.

What?  No.  Why would you want to do that?  Why does it matter what
order these devices are connected?  Are they talked to in different
ways?  Are they different "classes" of devices?  What are they and why
aren't they just "interchangeable" as far as userspace cares?

> Most importantly all the legacy user space just opens /dev/mei  so I
> wanted to leave /dev/mei (not /dev/mei0) to be assigned always to the
> first mei head and to not break existing user space. 

That's fine, I have no objection to that.

> Can you be a bit more specific on why we cannot depend on minor
> numbers and default naming rules, what is the scenario that will
> break? 

Why is the kernel picking a name for these with a specific minor number
in it?  What makes this different from a random tty device where you
don't care what the minor number is, you just care about where is it
connected so you look at the other things "around" it (pci path, vendor
id, etc.) and create a deterministic name from with a symlink, if you
care, with udev / mdev.

thanks,

greg k-h