[char-misc-next 0/3] mei: export fw status register through sysfs

[char-misc-next 0/3] mei: export fw status register through sysfs

[Tomas Winkler]

Especially comments on the ABI documentation would be appreciated 
I'm not sure I've done that correctly


Tomas Winkler (3):
  mei me: add handler for me_hw_ops fw_status
  mei: adding sysfs fw_status attribute
  mei: update ABI for fw_status register exported through sysfs

 Documentation/ABI/testing/sysfs-devices-mei |  7 ++++
 drivers/misc/mei/hw-me-regs.h               |  9 +++++
 drivers/misc/mei/hw-me.c                    | 30 ++++++++++++++++
 drivers/misc/mei/main.c                     | 54 +++++++++++++++++++++++++++--
 4 files changed, 97 insertions(+), 3 deletions(-)
 create mode 100644 Documentation/ABI/testing/sysfs-devices-mei

-- 
1.8.1.2

[char-misc-next 1/3] mei me: add handler for me_hw_ops fw_status

[Tomas Winkler]

FW status is read from mei pci configuration space.
The register address depends on the plataform id

Signed-off-by: Tomas Winkler <tomas.winkler@intel.com>
---
 drivers/misc/mei/hw-me-regs.h |  9 +++++++++
 drivers/misc/mei/hw-me.c      | 30 ++++++++++++++++++++++++++++++
 2 files changed, 39 insertions(+)

diff --git a/drivers/misc/mei/hw-me-regs.h b/drivers/misc/mei/hw-me-regs.h
index 6a203b6..3f612ce 100644
--- a/drivers/misc/mei/hw-me-regs.h
+++ b/drivers/misc/mei/hw-me-regs.h
@@ -111,10 +111,19 @@
 
 #define MEI_DEV_ID_LPT        0x8C3A  /* Lynx Point */
 #define MEI_DEV_ID_LPT_LP     0x9C3A  /* Lynx Point LP */
+
 /*
  * MEI HW Section
  */
 
+/* Host Firmware Status in Platforms < LPT */
+#define PCI_CFG_HFS_PPT   0x40
+/* Host Firmware Status in Platforms >= LPT */
+#define PCI_CFG_HFS_LPT 0x60
+#  define PCI_CFG_HFS_STATE_MSK    0x0000000F
+#  define PCI_CFG_HFS_ISR_CNT_MSK  0x00000600
+#  define PCI_CFG_HFS_ERR_CODE_MSK 0x0000F000
+
 /* MEI registers */
 /* H_CB_WW - Host Circular Buffer (CB) Write Window register */
 #define H_CB_WW    0
diff --git a/drivers/misc/mei/hw-me.c b/drivers/misc/mei/hw-me.c
index b22c7e2..2346167 100644
--- a/drivers/misc/mei/hw-me.c
+++ b/drivers/misc/mei/hw-me.c
@@ -536,8 +536,38 @@ end:
 
 	return IRQ_HANDLED;
 }
+
+/**
+ * mei_me_fw_status  - retrieve fw status from the pci config space
+ *
+ * @dev: the device structure
+ * @fw_status: fw status register value
+ *
+ * returns 0 on success an error code otherwise
+ */
+static int mei_me_fw_status(struct mei_device *dev, u32 *fw_status)
+{
+	u32 reg;
+	if (!dev || !fw_status)
+		return -EINVAL;
+
+	switch (dev->pdev->device) {
+	case MEI_DEV_ID_LPT:
+	case MEI_DEV_ID_LPT_LP:
+		reg = PCI_CFG_HFS_LPT;
+		break;
+	default:
+		reg = PCI_CFG_HFS_PPT;
+		break;
+	}
+
+	return pci_read_config_dword(dev->pdev, reg, fw_status);
+}
+
 static const struct mei_hw_ops mei_me_hw_ops = {
 
+	.fw_status = mei_me_fw_status,
+
 	.host_is_ready = mei_me_host_is_ready,
 
 	.hw_is_ready = mei_me_hw_is_ready,
-- 
1.8.1.2

[char-misc-next 2/3] mei: adding sysfs fw_status attribute

[Tomas Winkler]

Export fw status register through sysfs interface.
The interface is for applications that monitors
fw health.

Signed-off-by: Natalia Ovsyanikov <natalia.ovsyanikov@intel.com>
Signed-off-by: Tomas Winkler <tomas.winkler@intel.com>
---
 drivers/misc/mei/main.c | 54 ++++++++++++++++++++++++++++++++++++++++++++++---
 1 file changed, 51 insertions(+), 3 deletions(-)

diff --git a/drivers/misc/mei/main.c b/drivers/misc/mei/main.c
index 173ff09..64e36a8 100644
--- a/drivers/misc/mei/main.c
+++ b/drivers/misc/mei/main.c
@@ -659,6 +659,44 @@ out:
 	return mask;
 }
 
+/**
+ * mei_show_fw_status - mei device attribute show method
+ *
+ * @dev:  device pointer
+ * @attr: attribute pointer
+ * @buf:  char out buffer
+ *
+ * returns number of the bytes, printed into the buffer
+ */
+
+static ssize_t mei_show_fw_status(struct device *device,
+		struct device_attribute *attr, char *buf)
+{
+	struct mei_device *dev = dev_get_drvdata(device);
+	u32 fw_status;
+	int err;
+
+	mutex_lock(&dev->device_lock);
+	err = mei_fw_status(dev, &fw_status);
+	mutex_unlock(&dev->device_lock);
+	if (err) {
+		dev_err(device, "attribute show: read fw_status error = %d\n",
+			err);
+		return err;
+	}
+
+	dev_dbg(device, "attribute show: fw_status 0x%x\n", fw_status);
+	return sprintf(buf, "%08X\n", fw_status);
+}
+static DEVICE_ATTR(fw_status, S_IRUGO, mei_show_fw_status, NULL);
+static struct attribute *mei_attributes[] = {
+	&dev_attr_fw_status.attr,
+	NULL
+};
+static const struct attribute_group mei_attr_group = {
+	.attrs  = mei_attributes,
+};
+
 /*
  * file operations structure will be used for mei char device.
  */
@@ -685,17 +723,22 @@ static struct miscdevice  mei_misc_device = {
 		.minor = MISC_DYNAMIC_MINOR,
 };
 
-
 int mei_register(struct mei_device *dev)
 {
+	struct device *device = &dev->pdev->dev;
 	int ret;
-	mei_misc_device.parent = &dev->pdev->dev;
+
+	mei_misc_device.parent = device;
 	ret = misc_register(&mei_misc_device);
 	if (ret)
 		return ret;
 
+	ret = sysfs_create_group(&device->kobj, &mei_attr_group);
+	if (ret)
+		return ret;
+
 	if (mei_dbgfs_register(dev, mei_misc_device.name))
-		dev_err(&dev->pdev->dev, "cannot register debugfs\n");
+		dev_err(device, "cannot register debugfs\n");
 
 	return 0;
 }
@@ -703,7 +746,12 @@ EXPORT_SYMBOL_GPL(mei_register);
 
 void mei_deregister(struct mei_device *dev)
 {
+	struct device *device = &dev->pdev->dev;
+
+	sysfs_remove_group(&device->kobj, &mei_attr_group);
+
 	mei_dbgfs_deregister(dev);
+
 	misc_deregister(&mei_misc_device);
 	mei_misc_device.parent = NULL;
 }
-- 
1.8.1.2

[char-misc-next 3/3] mei: update ABI for fw_status register exported through sysfs

[Tomas Winkler]

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

diff --git a/Documentation/ABI/testing/sysfs-devices-mei b/Documentation/ABI/testing/sysfs-devices-mei
new file mode 100644
index 0000000..e6860f6
--- /dev/null
+++ b/Documentation/ABI/testing/sysfs-devices-mei
@@ -0,0 +1,7 @@
+What:           /sys/devices/<bus-num>/<dev>/fw_status
+Date:           Jul 2013
+KernelVersion:	3.12
+Contact:        Tomas Winkler <tomas.winkler@intel.com>
+Description:	Display fw status register content for applications
+		monitors fw health. The register content is hw
+		dependent.
-- 
1.8.1.2

Re: [char-misc-next 2/3] mei: adding sysfs fw_status attribute

[Greg KH]

On Wed, Jul 24, 2013 at 06:41:56PM +0300, Tomas Winkler wrote:
> Export fw status register through sysfs interface.
> The interface is for applications that monitors
> fw health.
> 
> Signed-off-by: Natalia Ovsyanikov <natalia.ovsyanikov@intel.com>
> Signed-off-by: Tomas Winkler <tomas.winkler@intel.com>
> ---
>  drivers/misc/mei/main.c | 54 ++++++++++++++++++++++++++++++++++++++++++++++---
>  1 file changed, 51 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/misc/mei/main.c b/drivers/misc/mei/main.c
> index 173ff09..64e36a8 100644
> --- a/drivers/misc/mei/main.c
> +++ b/drivers/misc/mei/main.c
> @@ -659,6 +659,44 @@ out:
>  	return mask;
>  }
>  
> +/**
> + * mei_show_fw_status - mei device attribute show method
> + *
> + * @dev:  device pointer
> + * @attr: attribute pointer
> + * @buf:  char out buffer
> + *
> + * returns number of the bytes, printed into the buffer
> + */
> +
> +static ssize_t mei_show_fw_status(struct device *device,
> +		struct device_attribute *attr, char *buf)
> +{
> +	struct mei_device *dev = dev_get_drvdata(device);
> +	u32 fw_status;
> +	int err;
> +
> +	mutex_lock(&dev->device_lock);
> +	err = mei_fw_status(dev, &fw_status);
> +	mutex_unlock(&dev->device_lock);
> +	if (err) {
> +		dev_err(device, "attribute show: read fw_status error = %d\n",
> +			err);
> +		return err;
> +	}
> +
> +	dev_dbg(device, "attribute show: fw_status 0x%x\n", fw_status);
> +	return sprintf(buf, "%08X\n", fw_status);
> +}
> +static DEVICE_ATTR(fw_status, S_IRUGO, mei_show_fw_status, NULL);
> +static struct attribute *mei_attributes[] = {
> +	&dev_attr_fw_status.attr,
> +	NULL
> +};
> +static const struct attribute_group mei_attr_group = {
> +	.attrs  = mei_attributes,
> +};
> +
>  /*
>   * file operations structure will be used for mei char device.
>   */
> @@ -685,17 +723,22 @@ static struct miscdevice  mei_misc_device = {
>  		.minor = MISC_DYNAMIC_MINOR,
>  };
>  
> -
>  int mei_register(struct mei_device *dev)
>  {
> +	struct device *device = &dev->pdev->dev;
>  	int ret;
> -	mei_misc_device.parent = &dev->pdev->dev;
> +
> +	mei_misc_device.parent = device;
>  	ret = misc_register(&mei_misc_device);
>  	if (ret)
>  		return ret;
>  
> +	ret = sysfs_create_group(&device->kobj, &mei_attr_group);
> +	if (ret)
> +		return ret;

And you just lost the race with userspace (you told it you had a device,
it went and scanned your attributes and saved them, then you registered
more...)

Please use the dev_groups field for your class to properly have the
driver core register these before userspace is told about the device,
that's the correct way to do this.

thanks,

greg k-h

RE: [char-misc-next 2/3] mei: adding sysfs fw_status attribute

[Winkler, Tomas]

> >  int mei_register(struct mei_device *dev)  {
> > +	struct device *device = &dev->pdev->dev;
> >  	int ret;
> > -	mei_misc_device.parent = &dev->pdev->dev;
> > +
> > +	mei_misc_device.parent = device;
> >  	ret = misc_register(&mei_misc_device);
> >  	if (ret)
> >  		return ret;
> >
> > +	ret = sysfs_create_group(&device->kobj, &mei_attr_group);
> > +	if (ret)
> > +		return ret;
> 

> And you just lost the race with userspace (you told it you had a device, it
> went and scanned your attributes and saved them, then you registered
> more...)
> 
> Please use the dev_groups field for your class to properly have the driver
> core register these before userspace is told about the device, that's the
> correct way to do this.

Just to make sure, is this what you're suggesting ?

device->groups = mei_attr_groups
mei_misc_device.parent = device;
ret = misc_register(&mei_misc_device);

Thanks
Tomas 

Re: [char-misc-next 2/3] mei: adding sysfs fw_status attribute

[Greg KH]

On Wed, Jul 24, 2013 at 04:20:32PM +0000, Winkler, Tomas wrote:
> 
> 
> > >  int mei_register(struct mei_device *dev)  {
> > > +	struct device *device = &dev->pdev->dev;
> > >  	int ret;
> > > -	mei_misc_device.parent = &dev->pdev->dev;
> > > +
> > > +	mei_misc_device.parent = device;
> > >  	ret = misc_register(&mei_misc_device);
> > >  	if (ret)
> > >  		return ret;
> > >
> > > +	ret = sysfs_create_group(&device->kobj, &mei_attr_group);
> > > +	if (ret)
> > > +		return ret;
> > 
> 
> > And you just lost the race with userspace (you told it you had a device, it
> > went and scanned your attributes and saved them, then you registered
> > more...)
> > 
> > Please use the dev_groups field for your class to properly have the driver
> > core register these before userspace is told about the device, that's the
> > correct way to do this.
> 
> Just to make sure, is this what you're suggesting ?
> 
> device->groups = mei_attr_groups
> mei_misc_device.parent = device;
> ret = misc_register(&mei_misc_device);

Yes, that should work.  If not, please let me know, the code underwent
some changes in 3.11-rc2.

thanks,

greg k-h

RE: [char-misc-next 2/3] mei: adding sysfs fw_status attribute

[Winkler, Tomas]

> >
> > Just to make sure, is this what you're suggesting ?
> >
> > device->groups = mei_attr_groups
> > mei_misc_device.parent = device;
> > ret = misc_register(&mei_misc_device);
> 
> Yes, that should work.  If not, please let me know, the code underwent some
> changes in 3.11-rc2.

I don' t see the file being created.
Not sure if the  misc_register setup the parent sysfs as well.

Thanks
Tomas

Re: [char-misc-next 2/3] mei: adding sysfs fw_status attribute

[Greg KH]

On Wed, Jul 24, 2013 at 06:04:33PM +0000, Winkler, Tomas wrote:
> 
> 
> > >
> > > Just to make sure, is this what you're suggesting ?
> > >
> > > device->groups = mei_attr_groups
> > > mei_misc_device.parent = device;
> > > ret = misc_register(&mei_misc_device);
> > 
> > Yes, that should work.  If not, please let me know, the code underwent some
> > changes in 3.11-rc2.
> 
> I don' t see the file being created.
> Not sure if the  misc_register setup the parent sysfs as well.

Oops, wait, the parent is already registered, that's not going to work,
you want to create the files for the device itself that you are
creating, so set the ->groups field for that device.

sorry about that.

greg k-h

RE: [char-misc-next 2/3] mei: adding sysfs fw_status attribute

[Winkler, Tomas]

> On Wed, Jul 24, 2013 at 06:04:33PM +0000, Winkler, Tomas wrote:
> >
> >
> > > >
> > > > Just to make sure, is this what you're suggesting ?
> > > >
> > > > device->groups = mei_attr_groups
> > > > mei_misc_device.parent = device;
> > > > ret = misc_register(&mei_misc_device);
> > >
> > > Yes, that should work.  If not, please let me know, the code
> > > underwent some changes in 3.11-rc2.
> >
> > I don' t see the file being created.
> > Not sure if the  misc_register setup the parent sysfs as well.
> 
> Oops, wait, the parent is already registered, that's not going to work, you
> want to create the files for the device itself that you are creating, so set the -
> >groups field for that device.
> 
> sorry about that.

So is this okay to go as  is?
Thanks
Tomas

Re: [char-misc-next 2/3] mei: adding sysfs fw_status attribute

[Greg KH]

On Sun, Jul 28, 2013 at 08:48:24AM +0000, Winkler, Tomas wrote:
> > On Wed, Jul 24, 2013 at 06:04:33PM +0000, Winkler, Tomas wrote:
> > >
> > >
> > > > >
> > > > > Just to make sure, is this what you're suggesting ?
> > > > >
> > > > > device->groups = mei_attr_groups
> > > > > mei_misc_device.parent = device;
> > > > > ret = misc_register(&mei_misc_device);
> > > >
> > > > Yes, that should work.  If not, please let me know, the code
> > > > underwent some changes in 3.11-rc2.
> > >
> > > I don' t see the file being created.
> > > Not sure if the  misc_register setup the parent sysfs as well.
> > 
> > Oops, wait, the parent is already registered, that's not going to work, you
> > want to create the files for the device itself that you are creating, so set the -
> > >groups field for that device.
> > 
> > sorry about that.
> 
> So is this okay to go as  is?

What is "this"?

RE: [char-misc-next 2/3] mei: adding sysfs fw_status attribute

[Winkler, Tomas]

> 
> > > >
> > > >
> > > > > >
> > > > > > Just to make sure, is this what you're suggesting ?
> > > > > >
> > > > > > device->groups = mei_attr_groups
> > > > > > mei_misc_device.parent = device; ret =
> > > > > > misc_register(&mei_misc_device);
> > > > >
> > > > > Yes, that should work.  If not, please let me know, the code
> > > > > underwent some changes in 3.11-rc2.
> > > >
> > > > I don' t see the file being created.
> > > > Not sure if the  misc_register setup the parent sysfs as well.
> > >
> > > Oops, wait, the parent is already registered, that's not going to
> > > work, you want to create the files for the device itself that you
> > > are creating, so set the -
> > > >groups field for that device.
> > >
> > > sorry about that.
> >
> > So is this okay to go as  is?
> 
> What is "this"?

The intentions was to create the sysfs  on a  parent pci device, and  these are already created .
The only reason I wanted do it here as this is true for any parent pci device we support currently that registers with mei.

Either this code ode is still confusing because  I'm  doing something wrong here or  your mind
Is set on fixing this particular user space race issue.

Thanks
Tomas

Re: [char-misc-next 2/3] mei: adding sysfs fw_status attribute

[Greg KH]

On Mon, Jul 29, 2013 at 10:27:06PM +0000, Winkler, Tomas wrote:
> 
> > 
> > > > >
> > > > >
> > > > > > >
> > > > > > > Just to make sure, is this what you're suggesting ?
> > > > > > >
> > > > > > > device->groups = mei_attr_groups
> > > > > > > mei_misc_device.parent = device; ret =
> > > > > > > misc_register(&mei_misc_device);
> > > > > >
> > > > > > Yes, that should work.  If not, please let me know, the code
> > > > > > underwent some changes in 3.11-rc2.
> > > > >
> > > > > I don' t see the file being created.
> > > > > Not sure if the  misc_register setup the parent sysfs as well.
> > > >
> > > > Oops, wait, the parent is already registered, that's not going to
> > > > work, you want to create the files for the device itself that you
> > > > are creating, so set the -
> > > > >groups field for that device.
> > > >
> > > > sorry about that.
> > >
> > > So is this okay to go as  is?
> > 
> > What is "this"?
> 
> The intentions was to create the sysfs  on a  parent pci device, and
> these are already created .

You can't create attributes on a device you do not "own", that will be
racy and prone to cause big problems (your callback will not with the
"right" device usually, as your parent might not "be" a pci device in
the future...)

> The only reason I wanted do it here as this is true for any parent pci
> device we support currently that registers with mei.
> 
> Either this code ode is still confusing because  I'm  doing something
> wrong here or  your mind Is set on fixing this particular user space
> race issue.

Probably both...

Either way, you can't create sysfs files in a racy way, I'm going
through the whole kernel tree to clean them all up, I'm not going to add
new race conditions today that I need to fix up tomorrow, my patch count
is high enough as it is :)

thanks,

greg k-h

RE: [char-misc-next 2/3] mei: adding sysfs fw_status attribute

[Winkler, Tomas]

> >
> > >
> > > > > >
> > > > > >
> > > > > > > >
> > > > > > > > Just to make sure, is this what you're suggesting ?
> > > > > > > >
> > > > > > > > device->groups = mei_attr_groups
> > > > > > > > mei_misc_device.parent = device; ret =
> > > > > > > > misc_register(&mei_misc_device);
> > > > > > >
> > > > > > > Yes, that should work.  If not, please let me know, the code
> > > > > > > underwent some changes in 3.11-rc2.
> > > > > >
> > > > > > I don' t see the file being created.
> > > > > > Not sure if the  misc_register setup the parent sysfs as well.
> > > > >
> > > > > Oops, wait, the parent is already registered, that's not going
> > > > > to work, you want to create the files for the device itself that
> > > > > you are creating, so set the -
> > > > > >groups field for that device.
> > > > >
> > > > > sorry about that.
> > > >
> > > > So is this okay to go as  is?
> > >
> > > What is "this"?
> >
> > The intentions was to create the sysfs  on a  parent pci device, and
> > these are already created .
> 
> You can't create attributes on a device you do not "own", that will be racy
> and prone to cause big problems (your callback will not with the "right"
> device usually, as your parent might not "be" a pci device in the future...)
> 
> > The only reason I wanted do it here as this is true for any parent pci
> > device we support currently that registers with mei.
> >
> > Either this code ode is still confusing because  I'm  doing something
> > wrong here or  your mind Is set on fixing this particular user space
> > race issue.
> 
> Probably both...
> 
> Either way, you can't create sysfs files in a racy way, I'm going through the
> whole kernel tree to clean them all up, I'm not going to add new race
> conditions today that I need to fix up tomorrow, my patch count is high
> enough as it is :)

I understand your quest tough I'm asking you to throw some light on that
How do I add new device specific sysfs in non-race way if my entry point is the pci probe function.

Thanks
Tomas

Re: [char-misc-next 2/3] mei: adding sysfs fw_status attribute

[Greg KH]

On Mon, Jul 29, 2013 at 11:08:52PM +0000, Winkler, Tomas wrote:
> How do I add new device specific sysfs in non-race way if my entry
> point is the pci probe function.

You do it in the device creation for the device you add below the PCI
device in sysfs, by setting the groups field in the device.

You don't create files in the sysfs directory for the PCI device itself,
those are owned by the PCI bus core.

This is why you are your own "bus" here, use it :)

If you still have questions, how about we take it to code, post what you
have, and I'll see what needs to be changed.

hope this helps,

greg k-h

RE: [char-misc-next 2/3] mei: adding sysfs fw_status attribute

[Winkler, Tomas]

> 
> On Mon, Jul 29, 2013 at 11:08:52PM +0000, Winkler, Tomas wrote:
> > How do I add new device specific sysfs in non-race way if my entry
> > point is the pci probe function.
> 
> You do it in the device creation for the device you add below the PCI device
> in sysfs, by setting the groups field in the device.
> 
> You don't create files in the sysfs directory for the PCI device itself, those are
> owned by the PCI bus core.
> 
> This is why you are your own "bus" here, use it :)

> If you still have questions, how about we take it to code, post what you have,
> and I'll see what needs to be changed.
> 
> hope this helps,

Right but this is not related to mei_cl_bus,  which is an abstraction in MEI protocol level, I wanted to augment the pci device.

We'd I've tried to expose is the FW status register from pci config space, (I' know the pci config space
Is already exposed through syfs), the issue is that the  offset of the registers changes between different HW, so I wished that there
Is a sysfs entry called fw_status  always points to the correct offset.  An application that tries to query fw status can be oblivious to underlying HW SKU. 

I guess that sysfs is not good interface for that now.  I will just resend the two other patches w/o sysfs 

Thanks
Tomas

Re: [char-misc-next 2/3] mei: adding sysfs fw_status attribute

[Greg KH]

On Thu, Aug 01, 2013 at 07:21:30AM +0000, Winkler, Tomas wrote:
> > 
> > On Mon, Jul 29, 2013 at 11:08:52PM +0000, Winkler, Tomas wrote:
> > > How do I add new device specific sysfs in non-race way if my entry
> > > point is the pci probe function.
> > 
> > You do it in the device creation for the device you add below the PCI device
> > in sysfs, by setting the groups field in the device.
> > 
> > You don't create files in the sysfs directory for the PCI device itself, those are
> > owned by the PCI bus core.
> > 
> > This is why you are your own "bus" here, use it :)
> 
> > If you still have questions, how about we take it to code, post what you have,
> > and I'll see what needs to be changed.
> > 
> > hope this helps,
> 
> Right but this is not related to mei_cl_bus,  which is an abstraction
> in MEI protocol level, I wanted to augment the pci device.

Then do that in the pci core :)

> We'd I've tried to expose is the FW status register from pci config
> space, (I' know the pci config space Is already exposed through syfs),
> the issue is that the  offset of the registers changes between
> different HW, so I wished that there Is a sysfs entry called fw_status
> always points to the correct offset.  An application that tries to
> query fw status can be oblivious to underlying HW SKU. 
> 
> I guess that sysfs is not good interface for that now.  I will just
> resend the two other patches w/o sysfs 

No, you would be adding "random" files to a pci device in sysfs, how
would userspace ever know about something like this?

If this is a PCI-standard thing, then add it to the PCI core please.
Otherwise, it belongs on your "controller" as it is a child of the PCI
device, and should be able to live there just fine.

thanks,

greg k-h