diff mbox

[net-next,8/9] ixgbe: add interface to export thermal data

Message ID 1324631357-31789-9-git-send-email-jeffrey.t.kirsher@intel.com
State Changes Requested, archived
Delegated to: David Miller
Headers show

Commit Message

Kirsher, Jeffrey T Dec. 23, 2011, 9:09 a.m. UTC
From: Don Skidmore <donald.c.skidmore@intel.com>

Some of our adapters have thermal data available, this patch exports this
data via a read-only sysfs interface.  More patches will follow that will
contain additional information to be exported.

Signed-off-by: Don Skidmore <donald.c.skidmore@intel.com>
Signed-off-by: Peter P Waskiewicz Jr <peter.p.waskiewicz.jr@intel.com>
Tested-by: Stephen Ko <stephen.s.ko@intel.com>
Signed-off-by: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
---
 drivers/net/ethernet/intel/ixgbe/Makefile      |    2 +-
 drivers/net/ethernet/intel/ixgbe/ixgbe.h       |    4 +
 drivers/net/ethernet/intel/ixgbe/ixgbe_82599.c |    2 +
 drivers/net/ethernet/intel/ixgbe/ixgbe_main.c  |    6 +
 drivers/net/ethernet/intel/ixgbe/ixgbe_sysfs.c |  305 ++++++++++++++++++++++++
 5 files changed, 318 insertions(+), 1 deletions(-)
 create mode 100644 drivers/net/ethernet/intel/ixgbe/ixgbe_sysfs.c

Comments

Francois Romieu Dec. 23, 2011, 1:01 p.m. UTC | #1
Jeff Kirsher <jeffrey.t.kirsher@intel.com> :
> From: Don Skidmore <donald.c.skidmore@intel.com>
> 
> Some of our adapters have thermal data available, this patch exports this
> data via a read-only sysfs interface.  More patches will follow that will
> contain additional information to be exported.
> 
> Signed-off-by: Don Skidmore <donald.c.skidmore@intel.com>
> Signed-off-by: Peter P Waskiewicz Jr <peter.p.waskiewicz.jr@intel.com>
> Tested-by: Stephen Ko <stephen.s.ko@intel.com>
> Signed-off-by: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
> ---
>  drivers/net/ethernet/intel/ixgbe/Makefile      |    2 +-
>  drivers/net/ethernet/intel/ixgbe/ixgbe.h       |    4 +
>  drivers/net/ethernet/intel/ixgbe/ixgbe_82599.c |    2 +
>  drivers/net/ethernet/intel/ixgbe/ixgbe_main.c  |    6 +
>  drivers/net/ethernet/intel/ixgbe/ixgbe_sysfs.c |  305 ++++++++++++++++++++++++
[...]
> diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_82599.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_82599.c
> index 7720721..1a3810e 100644
> --- a/drivers/net/ethernet/intel/ixgbe/ixgbe_82599.c
> +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_82599.c
> @@ -2137,6 +2137,8 @@ static struct ixgbe_mac_operations mac_ops_82599 = {
>  	.set_vlan_anti_spoofing = &ixgbe_set_vlan_anti_spoofing,
>  	.acquire_swfw_sync      = &ixgbe_acquire_swfw_sync,
>  	.release_swfw_sync      = &ixgbe_release_swfw_sync,
> +	.get_thermal_sensor_data = &ixgbe_get_thermal_sensor_data_generic,
> +	.init_thermal_sensor_thresh = &ixgbe_init_thermal_sensor_thresh_generic,

.get_thermal_sensor_data and .init_thermal_sensor_thresh do not seem to
be read anywhere.

>  
>  };
>  
> diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
> index e27e4d1..b5cef7e 100644
> --- a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
> +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
> @@ -7717,6 +7717,10 @@ static int __devinit ixgbe_probe(struct pci_dev *pdev,
>  
>  	e_dev_info("Intel(R) 10 Gigabit Network Connection\n");
>  	cards_found++;
> +
> +	if (ixgbe_sysfs_init(adapter))
> +		e_err(probe, "failed to allocate sysfs resources\n");
> +
>  	return 0;
>  
>  err_register:
> @@ -7764,6 +7768,8 @@ static void __devexit ixgbe_remove(struct pci_dev *pdev)
>  	}
>  
>  #endif
> +	ixgbe_sysfs_exit(adapter);
> +
>  #ifdef IXGBE_FCOE
>  	if (adapter->flags & IXGBE_FLAG_FCOE_ENABLED)
>  		ixgbe_cleanup_fcoe(adapter);
> diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_sysfs.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_sysfs.c
> new file mode 100644
> index 0000000..db818ae
> --- /dev/null
> +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_sysfs.c
[...]
> +static void ixgbe_del_adapter(struct ixgbe_adapter *adapter)

Please name it ixgbe_sysfs_del_adapter or anything better.

> +{
> +	int i;
> +
> +	if (adapter == NULL)
> +		return;
> +
> +	for (i = 0; i < IXGBE_MAX_SENSORS; i++) {
> +		if (adapter->therm_kobj[i] == NULL)
> +			continue;
> +		sysfs_remove_group(adapter->therm_kobj[i], &therm_attr_group);
> +		kobject_put(adapter->therm_kobj[i]);
> +	}
> +	if (adapter->info_kobj != NULL)
> +		kobject_put(adapter->info_kobj);
> +}
> +
> +/* called from ixgbe_main.c */
> +void ixgbe_sysfs_exit(struct ixgbe_adapter *adapter)
> +{
> +	ixgbe_del_adapter(adapter);
> +}
> +
> +/* called from ixgbe_main.c */
> +int ixgbe_sysfs_init(struct ixgbe_adapter *adapter)
> +{
> +	struct net_device *netdev;
> +	int rc = 0;
> +	int i;
> +	char buf[16];
> +
> +	if (adapter == NULL)
> +		goto err;

ixgbe_sysfs_init is only used in ixgbe_probe at a place where adapter can
not be NULL.

> +	netdev = adapter->netdev;
> +	if (netdev == NULL)
> +		goto err;

netdev can not be NULL.

> +
> +	adapter->info_kobj = NULL;
> +	for (i = 0; i < IXGBE_MAX_SENSORS; i++)
> +		adapter->therm_kobj[i] = NULL;
> +
> +	/* create info kobj and attribute listings in kobj */
> +	adapter->info_kobj = kobject_create_and_add("info", 
> +					&(netdev->dev.kobj));

Remove comment and parenthesis. Everything can fit in a single line.

> +	if (adapter->info_kobj == NULL)
> +		goto err;
> +
> +	/* Don't create thermal subkobjs if no data present */
> +	if (ixgbe_thermal_present(adapter->info_kobj) != true)
> +		goto exit;
> +
> +	for (i = 0; i < IXGBE_MAX_SENSORS; i++) {

'buf' ought to be declared here.

> +
> +		/*
> +		 * Likewise only create individual kobjs that have
> +		 * meaningful data.
> +		 */
> +		if (adapter->hw.mac.thermal_sensor_data.sensor[i].location == 0)
> +			continue;
> +
> +		/* directory named after sensor offset */
> +		snprintf(buf, sizeof(buf), "sensor_%d", i);
> +		adapter->therm_kobj[i] =
> +			kobject_create_and_add(buf, adapter->info_kobj);
> +		if (adapter->therm_kobj[i] == NULL)
> +			goto err;
> +		if (sysfs_create_group(adapter->therm_kobj[i],
> +				       &therm_attr_group))
> +			goto err;
> +	}
> +
> +	goto exit;
> +
> +err:
> +	ixgbe_del_adapter(adapter);
> +	rc = -1;
> +exit:
> +	return rc;
> +}
> +
> -- 
> 1.7.7.4
> 
> --
> To unsubscribe from this list: send the line "unsubscribe netdev" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
=?ISO-8859-2?Q?Micha=B3_Miros=B3aw?= Dec. 23, 2011, 5:58 p.m. UTC | #2
2011/12/23 Jeff Kirsher <jeffrey.t.kirsher@intel.com>:
> From: Don Skidmore <donald.c.skidmore@intel.com>
>
> Some of our adapters have thermal data available, this patch exports this
> data via a read-only sysfs interface.

Just curious: can't this use the hwmon subsystem to be consistent with
other system monitoring devices?

Best Regards,
Michał Mirosław
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Skidmore, Donald C Dec. 23, 2011, 6:29 p.m. UTC | #3
>-----Original Message-----
>From: Francois Romieu [mailto:romieu@fr.zoreil.com]
>Sent: Friday, December 23, 2011 5:02 AM
>To: Kirsher, Jeffrey T
>Cc: davem@davemloft.net; Skidmore, Donald C; netdev@vger.kernel.org;
>gospo@redhat.com; sassmann@redhat.com; Waskiewicz Jr, Peter P
>Subject: Re: [net-next 8/9] ixgbe: add interface to export thermal data
>
>Jeff Kirsher <jeffrey.t.kirsher@intel.com> :
>> From: Don Skidmore <donald.c.skidmore@intel.com>
>>
>> Some of our adapters have thermal data available, this patch exports
>this
>> data via a read-only sysfs interface.  More patches will follow that
>will
>> contain additional information to be exported.
>>
>> Signed-off-by: Don Skidmore <donald.c.skidmore@intel.com>
>> Signed-off-by: Peter P Waskiewicz Jr <peter.p.waskiewicz.jr@intel.com>
>> Tested-by: Stephen Ko <stephen.s.ko@intel.com>
>> Signed-off-by: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
>> ---
>>  drivers/net/ethernet/intel/ixgbe/Makefile      |    2 +-
>>  drivers/net/ethernet/intel/ixgbe/ixgbe.h       |    4 +
>>  drivers/net/ethernet/intel/ixgbe/ixgbe_82599.c |    2 +
>>  drivers/net/ethernet/intel/ixgbe/ixgbe_main.c  |    6 +
>>  drivers/net/ethernet/intel/ixgbe/ixgbe_sysfs.c |  305
>++++++++++++++++++++++++
>[...]
>> diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_82599.c
>b/drivers/net/ethernet/intel/ixgbe/ixgbe_82599.c
>> index 7720721..1a3810e 100644
>> --- a/drivers/net/ethernet/intel/ixgbe/ixgbe_82599.c
>> +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_82599.c
>> @@ -2137,6 +2137,8 @@ static struct ixgbe_mac_operations mac_ops_82599
>= {
>>  	.set_vlan_anti_spoofing = &ixgbe_set_vlan_anti_spoofing,
>>  	.acquire_swfw_sync      = &ixgbe_acquire_swfw_sync,
>>  	.release_swfw_sync      = &ixgbe_release_swfw_sync,
>> +	.get_thermal_sensor_data = &ixgbe_get_thermal_sensor_data_generic,
>> +	.init_thermal_sensor_thresh =
>&ixgbe_init_thermal_sensor_thresh_generic,
>
>.get_thermal_sensor_data and .init_thermal_sensor_thresh do not seem to
>be read anywhere.
>
>>
>>  };
>>
>> diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
>b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
>> index e27e4d1..b5cef7e 100644
>> --- a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
>> +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
>> @@ -7717,6 +7717,10 @@ static int __devinit ixgbe_probe(struct pci_dev
>*pdev,
>>
>>  	e_dev_info("Intel(R) 10 Gigabit Network Connection\n");
>>  	cards_found++;
>> +
>> +	if (ixgbe_sysfs_init(adapter))
>> +		e_err(probe, "failed to allocate sysfs resources\n");
>> +
>>  	return 0;
>>
>>  err_register:
>> @@ -7764,6 +7768,8 @@ static void __devexit ixgbe_remove(struct
>pci_dev *pdev)
>>  	}
>>
>>  #endif
>> +	ixgbe_sysfs_exit(adapter);
>> +
>>  #ifdef IXGBE_FCOE
>>  	if (adapter->flags & IXGBE_FLAG_FCOE_ENABLED)
>>  		ixgbe_cleanup_fcoe(adapter);
>> diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_sysfs.c
>b/drivers/net/ethernet/intel/ixgbe/ixgbe_sysfs.c
>> new file mode 100644
>> index 0000000..db818ae
>> --- /dev/null
>> +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_sysfs.c
>[...]
>> +static void ixgbe_del_adapter(struct ixgbe_adapter *adapter)
>
>Please name it ixgbe_sysfs_del_adapter or anything better.
>
>> +{
>> +	int i;
>> +
>> +	if (adapter == NULL)
>> +		return;
>> +
>> +	for (i = 0; i < IXGBE_MAX_SENSORS; i++) {
>> +		if (adapter->therm_kobj[i] == NULL)
>> +			continue;
>> +		sysfs_remove_group(adapter->therm_kobj[i],
>&therm_attr_group);
>> +		kobject_put(adapter->therm_kobj[i]);
>> +	}
>> +	if (adapter->info_kobj != NULL)
>> +		kobject_put(adapter->info_kobj);
>> +}
>> +
>> +/* called from ixgbe_main.c */
>> +void ixgbe_sysfs_exit(struct ixgbe_adapter *adapter)
>> +{
>> +	ixgbe_del_adapter(adapter);
>> +}
>> +
>> +/* called from ixgbe_main.c */
>> +int ixgbe_sysfs_init(struct ixgbe_adapter *adapter)
>> +{
>> +	struct net_device *netdev;
>> +	int rc = 0;
>> +	int i;
>> +	char buf[16];
>> +
>> +	if (adapter == NULL)
>> +		goto err;
>
>ixgbe_sysfs_init is only used in ixgbe_probe at a place where adapter
>can
>not be NULL.
>
>> +	netdev = adapter->netdev;
>> +	if (netdev == NULL)
>> +		goto err;
>
>netdev can not be NULL.
>
>> +
>> +	adapter->info_kobj = NULL;
>> +	for (i = 0; i < IXGBE_MAX_SENSORS; i++)
>> +		adapter->therm_kobj[i] = NULL;
>> +
>> +	/* create info kobj and attribute listings in kobj */
>> +	adapter->info_kobj = kobject_create_and_add("info",
>> +					&(netdev->dev.kobj));
>
>Remove comment and parenthesis. Everything can fit in a single line.
>
>> +	if (adapter->info_kobj == NULL)
>> +		goto err;
>> +
>> +	/* Don't create thermal subkobjs if no data present */
>> +	if (ixgbe_thermal_present(adapter->info_kobj) != true)
>> +		goto exit;
>> +
>> +	for (i = 0; i < IXGBE_MAX_SENSORS; i++) {
>
>'buf' ought to be declared here.
>
>> +
>> +		/*
>> +		 * Likewise only create individual kobjs that have
>> +		 * meaningful data.
>> +		 */
>> +		if (adapter->hw.mac.thermal_sensor_data.sensor[i].location
>== 0)
>> +			continue;
>> +
>> +		/* directory named after sensor offset */
>> +		snprintf(buf, sizeof(buf), "sensor_%d", i);
>> +		adapter->therm_kobj[i] =
>> +			kobject_create_and_add(buf, adapter->info_kobj);
>> +		if (adapter->therm_kobj[i] == NULL)
>> +			goto err;
>> +		if (sysfs_create_group(adapter->therm_kobj[i],
>> +				       &therm_attr_group))
>> +			goto err;
>> +	}
>> +
>> +	goto exit;
>> +
>> +err:
>> +	ixgbe_del_adapter(adapter);
>> +	rc = -1;
>> +exit:
>> +	return rc;
>> +}
>> +
>> --
>> 1.7.7.4
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe netdev" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html


I like all your suggestions and will do the clean up.

Thanks again for the great review. :)

-Don Skidmore <donald.c.skidmore@intel.com>

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Francois Romieu Dec. 23, 2011, 8:45 p.m. UTC | #4
Skidmore, Donald C <donald.c.skidmore@intel.com> :
[...]
> I like all your suggestions and will do the clean up.
> 
> Thanks again for the great review. :)

Depending on how you plan to use ixgbe_init_thermal_sensor_thresh_generic,
the memset(..., 0, ...) may be removed since it is applied within the
device own kzalloced struct ixgbe_hw.

I must confess I have not looked too closely at the sysfs code.
Ben Hutchings Dec. 24, 2011, 10:22 a.m. UTC | #5
On Fri, 2011-12-23 at 18:58 +0100, Michał Mirosław wrote:
> 2011/12/23 Jeff Kirsher <jeffrey.t.kirsher@intel.com>:
> > From: Don Skidmore <donald.c.skidmore@intel.com>
> >
> > Some of our adapters have thermal data available, this patch exports this
> > data via a read-only sysfs interface.
> 
> Just curious: can't this use the hwmon subsystem to be consistent with
> other system monitoring devices?

That would certainly be the proper way to expose this...

Ben.
Skidmore, Donald C Jan. 5, 2012, 5:53 p.m. UTC | #6
>-----Original Message-----
>From: Michał Mirosław [mailto:mirqus@gmail.com]
>Sent: Friday, December 23, 2011 9:59 AM
>To: Kirsher, Jeffrey T
>Cc: davem@davemloft.net; Skidmore, Donald C; netdev@vger.kernel.org;
>gospo@redhat.com; sassmann@redhat.com; Waskiewicz Jr, Peter P
>Subject: Re: [net-next 8/9] ixgbe: add interface to export thermal data
>
>2011/12/23 Jeff Kirsher <jeffrey.t.kirsher@intel.com>:
>> From: Don Skidmore <donald.c.skidmore@intel.com>
>>
>> Some of our adapters have thermal data available, this patch exports
>this
>> data via a read-only sysfs interface.
>
>Just curious: can't this use the hwmon subsystem to be consistent with
>other system monitoring devices?
>
>Best Regards,
>Michał Mirosław

Sorry about the slow response, first vacation then I hadn't heard of hwmon and wanted to look into it a bit.  I can see why you mentioned it as it looks to be close to what I'm trying to do here.  However I don't think it quite matches.  I'll list my thoughts below:

- We are trying to export a large set of data that our customers are requesting.  The thermals were just the first patch and the other data items wouldn't really fit well in the hwmon (i.e. FW version, secondary MAC address).
- Didn't seem like we had much data to offer hwmon anyway just sensor temp, caution threshold, maxop threshold and location of sensor.  All the other data (which you haven't seen yet so couldn't have known :) wasn't related.
- The thermal data we do have is defined in our FW and could change (number of sensors) based on that FW.  I wasn't sure whether that would be an issue for hwmon.
- I went with sysfs based on a conversation with Peter Waskiewicz.  He mentioned that there was discussion on how to export generic data at netconf and sysfs was brought up as the best choice. 

Thanks for reviewing the patch and bring up this question. :)
-Don Skidmore <donald.c.skidmore@intel.com>




 
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Ben Hutchings Jan. 5, 2012, 6:36 p.m. UTC | #7
On Thu, 2012-01-05 at 17:53 +0000, Skidmore, Donald C wrote:
> >-----Original Message-----
> >From: Michał Mirosław [mailto:mirqus@gmail.com]
> >Sent: Friday, December 23, 2011 9:59 AM
> >To: Kirsher, Jeffrey T
> >Cc: davem@davemloft.net; Skidmore, Donald C; netdev@vger.kernel.org;
> >gospo@redhat.com; sassmann@redhat.com; Waskiewicz Jr, Peter P
> >Subject: Re: [net-next 8/9] ixgbe: add interface to export thermal data
> >
> >2011/12/23 Jeff Kirsher <jeffrey.t.kirsher@intel.com>:
> >> From: Don Skidmore <donald.c.skidmore@intel.com>
> >>
> >> Some of our adapters have thermal data available, this patch exports
> >this
> >> data via a read-only sysfs interface.
> >
> >Just curious: can't this use the hwmon subsystem to be consistent with
> >other system monitoring devices?
> >
> >Best Regards,
> >Michał Mirosław
> 
> Sorry about the slow response, first vacation then I hadn't heard of
> hwmon and wanted to look into it a bit.  I can see why you mentioned
> it as it looks to be close to what I'm trying to do here.  However I
> don't think it quite matches.  I'll list my thoughts below:
> 
> - We are trying to export a large set of data that our customers are
> requesting.  The thermals were just the first patch and the other data
> items wouldn't really fit well in the hwmon (i.e. FW version,
> secondary MAC address).

Nevertheless, there is a specific way that the thermal information
should be exposed.

> - Didn't seem like we had much data to offer hwmon anyway just sensor
> temp, caution threshold, maxop threshold and location of sensor.  All
> the other data (which you haven't seen yet so couldn't have known :)
> wasn't related.
> - The thermal data we do have is defined in our FW and could change
> (number of sensors) based on that FW.  I wasn't sure whether that
> would be an issue for hwmon.

I have the same issue with the current Solarflare controllers, as the
driver has no static information about specific boards.  It is possible
to generate hwmon attributes dynamically though I've not yet got round
to completing my implementation of this.  (Since firmware is also
responsible for thermal shutdown, handling any of this stuff in the
driver has been a low priority.)

> - I went with sysfs based on a conversation with Peter Waskiewicz.  He
> mentioned that there was discussion on how to export generic data at
> netconf and sysfs was brought up as the best choice. 

Sensors are also exposed through sysfs, but following a specific naming
convention and using a separate hwmon device.

(Oddly, though, the sensor attributes are must be attached to the hwmon
device's parent - which will be your PCI device.  So you may not need to
make many changes.)

Ben.

> Thanks for reviewing the patch and bring up this question. :)
> -Don Skidmore <donald.c.skidmore@intel.com>
Skidmore, Donald C Jan. 5, 2012, 10:57 p.m. UTC | #8
>-----Original Message-----

>From: Ben Hutchings [mailto:bhutchings@solarflare.com]

>Sent: Thursday, January 05, 2012 10:36 AM

>To: Skidmore, Donald C

>Cc: Michal Miroslaw; Kirsher, Jeffrey T; davem@davemloft.net;

>netdev@vger.kernel.org; gospo@redhat.com; sassmann@redhat.com;

>Waskiewicz Jr, Peter P

>Subject: RE: [net-next 8/9] ixgbe: add interface to export thermal data

>

>On Thu, 2012-01-05 at 17:53 +0000, Skidmore, Donald C wrote:

>> >-----Original Message-----

>> >From: Michał Mirosław [mailto:mirqus@gmail.com]

>> >Sent: Friday, December 23, 2011 9:59 AM

>> >To: Kirsher, Jeffrey T

>> >Cc: davem@davemloft.net; Skidmore, Donald C; netdev@vger.kernel.org;

>> >gospo@redhat.com; sassmann@redhat.com; Waskiewicz Jr, Peter P

>> >Subject: Re: [net-next 8/9] ixgbe: add interface to export thermal

>data

>> >

>> >2011/12/23 Jeff Kirsher <jeffrey.t.kirsher@intel.com>:

>> >> From: Don Skidmore <donald.c.skidmore@intel.com>

>> >>

>> >> Some of our adapters have thermal data available, this patch

>exports

>> >this

>> >> data via a read-only sysfs interface.

>> >

>> >Just curious: can't this use the hwmon subsystem to be consistent

>with

>> >other system monitoring devices?

>> >

>> >Best Regards,

>> >Michał Mirosław

>>

>> Sorry about the slow response, first vacation then I hadn't heard of

>> hwmon and wanted to look into it a bit.  I can see why you mentioned

>> it as it looks to be close to what I'm trying to do here.  However I

>> don't think it quite matches.  I'll list my thoughts below:

>>

>> - We are trying to export a large set of data that our customers are

>> requesting.  The thermals were just the first patch and the other data

>> items wouldn't really fit well in the hwmon (i.e. FW version,

>> secondary MAC address).

>

>Nevertheless, there is a specific way that the thermal information

>should be exposed.

>

>> - Didn't seem like we had much data to offer hwmon anyway just sensor

>> temp, caution threshold, maxop threshold and location of sensor.  All

>> the other data (which you haven't seen yet so couldn't have known :)

>> wasn't related.

>> - The thermal data we do have is defined in our FW and could change

>> (number of sensors) based on that FW.  I wasn't sure whether that

>> would be an issue for hwmon.

>

>I have the same issue with the current Solarflare controllers, as the

>driver has no static information about specific boards.  It is possible

>to generate hwmon attributes dynamically though I've not yet got round

>to completing my implementation of this.  (Since firmware is also

>responsible for thermal shutdown, handling any of this stuff in the

>driver has been a low priority.)

>

>> - I went with sysfs based on a conversation with Peter Waskiewicz.  He

>> mentioned that there was discussion on how to export generic data at

>> netconf and sysfs was brought up as the best choice.

>

>Sensors are also exposed through sysfs, but following a specific naming

>convention and using a separate hwmon device.

>

>(Oddly, though, the sensor attributes are must be attached to the hwmon

>device's parent - which will be your PCI device.  So you may not need to

>make many changes.)

>

>Ben.

>


Hey Ben,

Thanks for all the clarifications.  I think I have a better idea what would be required from an hwmon interface. 

My one real concern with this implementation is that the ixgbe driver will now have a dependency on hwmon.  The customers that are requesting this data aren't interested in using hwmon and would most likely just grab the thermal information (which is a small portion of the data they are asking for us to export) directly from sysfs.  Originally they wanted the FW to export the data directly, when this was found to be impossible the driver became the fall back option.

I do see your point on this being a defined way to expose thermals and I want to do this the correct way.  It just seems to put a lot of overhead on the driver (dependence on hwmon) for 4 fields that the user of this data won't even be using hwmon to look at.  I guess I would feel better if we had more data to provide hwmon then its support would seem more useful.

Thanks again for bring all this up.
-Don Skidmore <donald.c.skidmore@intel.com>
=?ISO-8859-2?Q?Micha=B3_Miros=B3aw?= Jan. 5, 2012, 11:50 p.m. UTC | #9
2012/1/5 Skidmore, Donald C <donald.c.skidmore@intel.com>:
>>-----Original Message-----
>>From: Ben Hutchings [mailto:bhutchings@solarflare.com]
>>Sent: Thursday, January 05, 2012 10:36 AM
>>To: Skidmore, Donald C
>>Cc: Michal Miroslaw; Kirsher, Jeffrey T; davem@davemloft.net;
>>netdev@vger.kernel.org; gospo@redhat.com; sassmann@redhat.com;
>>Waskiewicz Jr, Peter P
>>Subject: RE: [net-next 8/9] ixgbe: add interface to export thermal data
>>
>>On Thu, 2012-01-05 at 17:53 +0000, Skidmore, Donald C wrote:
>>> >-----Original Message-----
>>> >From: Michał Mirosław [mailto:mirqus@gmail.com]
>>> >Sent: Friday, December 23, 2011 9:59 AM
>>> >To: Kirsher, Jeffrey T
>>> >Cc: davem@davemloft.net; Skidmore, Donald C; netdev@vger.kernel.org;
>>> >gospo@redhat.com; sassmann@redhat.com; Waskiewicz Jr, Peter P
>>> >Subject: Re: [net-next 8/9] ixgbe: add interface to export thermal
>>data
>>> >
>>> >2011/12/23 Jeff Kirsher <jeffrey.t.kirsher@intel.com>:
>>> >> From: Don Skidmore <donald.c.skidmore@intel.com>
>>> >>
>>> >> Some of our adapters have thermal data available, this patch
>>exports
>>> >this
>>> >> data via a read-only sysfs interface.
>>> >
>>> >Just curious: can't this use the hwmon subsystem to be consistent
>>with
>>> >other system monitoring devices?
>>> >
>>> >Best Regards,
>>> >Michał Mirosław
>>>
>>> Sorry about the slow response, first vacation then I hadn't heard of
>>> hwmon and wanted to look into it a bit.  I can see why you mentioned
>>> it as it looks to be close to what I'm trying to do here.  However I
>>> don't think it quite matches.  I'll list my thoughts below:
>>>
>>> - We are trying to export a large set of data that our customers are
>>> requesting.  The thermals were just the first patch and the other data
>>> items wouldn't really fit well in the hwmon (i.e. FW version,
>>> secondary MAC address).
>>
>>Nevertheless, there is a specific way that the thermal information
>>should be exposed.
>>
>>> - Didn't seem like we had much data to offer hwmon anyway just sensor
>>> temp, caution threshold, maxop threshold and location of sensor.  All
>>> the other data (which you haven't seen yet so couldn't have known :)
>>> wasn't related.
>>> - The thermal data we do have is defined in our FW and could change
>>> (number of sensors) based on that FW.  I wasn't sure whether that
>>> would be an issue for hwmon.
>>
>>I have the same issue with the current Solarflare controllers, as the
>>driver has no static information about specific boards.  It is possible
>>to generate hwmon attributes dynamically though I've not yet got round
>>to completing my implementation of this.  (Since firmware is also
>>responsible for thermal shutdown, handling any of this stuff in the
>>driver has been a low priority.)
>>
>>> - I went with sysfs based on a conversation with Peter Waskiewicz.  He
>>> mentioned that there was discussion on how to export generic data at
>>> netconf and sysfs was brought up as the best choice.
>>
>>Sensors are also exposed through sysfs, but following a specific naming
>>convention and using a separate hwmon device.
>>
>>(Oddly, though, the sensor attributes are must be attached to the hwmon
>>device's parent - which will be your PCI device.  So you may not need to
>>make many changes.)
>>
>>Ben.
>>
>
> Hey Ben,
>
> Thanks for all the clarifications.  I think I have a better idea what would be required from an hwmon interface.
>
> My one real concern with this implementation is that the ixgbe driver will now have a dependency on hwmon.  The customers that are requesting this data aren't interested in using hwmon and would most likely just grab the thermal information (which is a small portion of the data they are asking for us to export) directly from sysfs.  Originally they wanted the FW to export the data directly, when this was found to be impossible the driver became the fall back option.
>
> I do see your point on this being a defined way to expose thermals and I want to do this the correct way.  It just seems to put a lot of overhead on the driver (dependence on hwmon) for 4 fields that the user of this data won't even be using hwmon to look at.  I guess I would feel better if we had more data to provide hwmon then its support would seem more useful.

Drivers outside of drivers/hwmon just select HWMON in Kconfig. This
adds a 3kB .c file to the kernel build for the first one that needs
it.

Best Regards,
Michał Mirosław
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
stephen hemminger Jan. 6, 2012, 3:03 a.m. UTC | #10
On Fri, 23 Dec 2011 18:58:48 +0100
Michał Mirosław <mirqus@gmail.com> wrote:

> 2011/12/23 Jeff Kirsher <jeffrey.t.kirsher@intel.com>:
> > From: Don Skidmore <donald.c.skidmore@intel.com>
> >
> > Some of our adapters have thermal data available, this patch exports this
> > data via a read-only sysfs interface.
> 
> Just curious: can't this use the hwmon subsystem to be consistent with
> other system monitoring devices?
> 
> Best Regards,
> Michał Mirosław

There is also libsensors support in net-snmp, so if you use standard API's
the values can be monitored with standard management tools.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Waskiewicz Jr, Peter P Jan. 6, 2012, 5:55 p.m. UTC | #11
> -----Original Message-----
> From: Michał Mirosław [mailto:mirqus@gmail.com]
> Sent: Thursday, January 05, 2012 3:51 PM
> To: Skidmore, Donald C
> Cc: Ben Hutchings; Kirsher, Jeffrey T; davem@davemloft.net;
> netdev@vger.kernel.org; gospo@redhat.com; sassmann@redhat.com;
> Waskiewicz Jr, Peter P
> Subject: Re: [net-next 8/9] ixgbe: add interface to export thermal data
> 
> Drivers outside of drivers/hwmon just select HWMON in Kconfig. This adds a
> 3kB .c file to the kernel build for the first one that needs it.

This is where we run into issues though.  We can put this dependency in, but
customers don't pull upstream kernels, they rely on the OSV's to distribute
updates.  The customer doesn't want HWMON, and if their kernel doesn't ship
with HWMON support for ixgbe, then we're sunk.

The point is what we're trying to implement, based on what our customers'
requirements are, is something completely different than what HWMON is
providing.  Trying to wedge HWMON onto this framework we're trying to
provide just overcomplicates the entire thing we're trying to provide.  It's
a generic interface to generic data in our drivers, which just happens to
include thermal data on this round of patches.  If we put this under HWMON,
then we'll have multiple places for this generic data, which just
complicates the whole thing, which is what we're trying to avoid.

We did dig into the HWMON specifics on how to set it up and use it, and it's
not the right tool for what we're doing.  I don't see how changing these
patches to use HWMON improves anything, and would rather not.

Cheers,
-PJ Waskiewicz
Ben Hutchings Jan. 6, 2012, 6:09 p.m. UTC | #12
On Fri, 2012-01-06 at 17:55 +0000, Waskiewicz Jr, Peter P wrote:
> > -----Original Message-----
> > From: Michał Mirosław [mailto:mirqus@gmail.com]
> > Sent: Thursday, January 05, 2012 3:51 PM
> > To: Skidmore, Donald C
> > Cc: Ben Hutchings; Kirsher, Jeffrey T; davem@davemloft.net;
> > netdev@vger.kernel.org; gospo@redhat.com; sassmann@redhat.com;
> > Waskiewicz Jr, Peter P
> > Subject: Re: [net-next 8/9] ixgbe: add interface to export thermal data
> > 
> > Drivers outside of drivers/hwmon just select HWMON in Kconfig. This adds a
> > 3kB .c file to the kernel build for the first one that needs it.
> 
> This is where we run into issues though.  We can put this dependency in, but
> customers don't pull upstream kernels, they rely on the OSV's to distribute
> updates.  The customer doesn't want HWMON, and if their kernel doesn't ship
> with HWMON support for ixgbe, then we're sunk.

So if !IS_ENABLED(CONFIG_HWMON), don't try to create an hwmon device,
but create the attributes anyway.

> The point is what we're trying to implement, based on what our customers'
> requirements are, is something completely different than what HWMON is
> providing.  Trying to wedge HWMON onto this framework we're trying to
> provide just overcomplicates the entire thing we're trying to provide.  It's
> a generic interface to generic data in our drivers,
[...]

It sounds like you want to provide a "generic" ixgbe interface on
different operating systems.  But that is not a valid argument for an
in-tree driver.

Ben.
Waskiewicz Jr, Peter P Jan. 6, 2012, 6:20 p.m. UTC | #13
> -----Original Message-----
> From: Ben Hutchings [mailto:bhutchings@solarflare.com]
> Sent: Friday, January 06, 2012 10:09 AM
> To: Waskiewicz Jr, Peter P
> Cc: Michal Miroslaw; Skidmore, Donald C; Kirsher, Jeffrey T;
> davem@davemloft.net; netdev@vger.kernel.org; gospo@redhat.com;
> sassmann@redhat.com
> Subject: RE: [net-next 8/9] ixgbe: add interface to export thermal data
> 
> On Fri, 2012-01-06 at 17:55 +0000, Waskiewicz Jr, Peter P wrote:
> > > -----Original Message-----
> > > From: Michał Mirosław [mailto:mirqus@gmail.com]
> > > Sent: Thursday, January 05, 2012 3:51 PM
> > > To: Skidmore, Donald C
> > > Cc: Ben Hutchings; Kirsher, Jeffrey T; davem@davemloft.net;
> > > netdev@vger.kernel.org; gospo@redhat.com; sassmann@redhat.com;
> > > Waskiewicz Jr, Peter P
> > > Subject: Re: [net-next 8/9] ixgbe: add interface to export thermal
> > > data
> > >
> > > Drivers outside of drivers/hwmon just select HWMON in Kconfig. This
> > > adds a 3kB .c file to the kernel build for the first one that needs it.
> >
> > This is where we run into issues though.  We can put this dependency
> > in, but customers don't pull upstream kernels, they rely on the OSV's
> > to distribute updates.  The customer doesn't want HWMON, and if their
> > kernel doesn't ship with HWMON support for ixgbe, then we're sunk.
> 
> So if !IS_ENABLED(CONFIG_HWMON), don't try to create an hwmon device,
> but create the attributes anyway.

And if CONFIG_HWMON is not enabled, are we then responsible for creating the hwmon area in sysfs to link in our attributes?  That would make even less sense to include HWMON attributes if HWMON isn't built in.

> > The point is what we're trying to implement, based on what our customers'
> > requirements are, is something completely different than what HWMON is
> > providing.  Trying to wedge HWMON onto this framework we're trying to
> > provide just overcomplicates the entire thing we're trying to provide.
> > It's a generic interface to generic data in our drivers,
> [...]
> 
> It sounds like you want to provide a "generic" ixgbe interface on different
> operating systems.  But that is not a valid argument for an in-tree driver.

No, we're trying to provide a generic ixgbe interface on Linux systems.  The reality is different OSV's pull our driver from upstream to build their distros, and they cherry pick patches from our drivers to achieve this.  Upstream feeds the OSV's.  Ignoring that is wrong.  We're using a common interface, sysfs, which we discussed at NetConf, and the general consensus supported these types of data to be exported by a driver.  Wrapping HWMON around this just for the sake of wrapping it around it makes no sense.  There are no plans for anyone using ixgbe to use or deploy HWMON.  So it just adds extra useless crap to our drivers and the kernel, and complicates our job of maintaining the driver by using a framework that provides no benefit to our driver or those using it.

The reality is these attributes of our thermal sensors are about 5% of the data we need to export.  The other 95% of the data, which Don stated are coming soon (still finalizing everything and testing), have nothing to do with thermal sensors.  So from a maintenance perspective, it makes it much uglier to maintain all this exported data when it's in multiple locations.  That's why we chose a contained model within the driver's sysfs space.  It doesn't affect anything else in the kernel space, and it keeps it tidy and easy to find all the data.

-PJ
Ben Hutchings Jan. 6, 2012, 6:25 p.m. UTC | #14
On Fri, 2012-01-06 at 18:20 +0000, Waskiewicz Jr, Peter P wrote:
> > -----Original Message-----
> > From: Ben Hutchings [mailto:bhutchings@solarflare.com]
> > Sent: Friday, January 06, 2012 10:09 AM
> > To: Waskiewicz Jr, Peter P
> > Cc: Michal Miroslaw; Skidmore, Donald C; Kirsher, Jeffrey T;
> > davem@davemloft.net; netdev@vger.kernel.org; gospo@redhat.com;
> > sassmann@redhat.com
> > Subject: RE: [net-next 8/9] ixgbe: add interface to export thermal data
> > 
> > On Fri, 2012-01-06 at 17:55 +0000, Waskiewicz Jr, Peter P wrote:
> > > > -----Original Message-----
> > > > From: Michał Mirosław [mailto:mirqus@gmail.com]
> > > > Sent: Thursday, January 05, 2012 3:51 PM
> > > > To: Skidmore, Donald C
> > > > Cc: Ben Hutchings; Kirsher, Jeffrey T; davem@davemloft.net;
> > > > netdev@vger.kernel.org; gospo@redhat.com; sassmann@redhat.com;
> > > > Waskiewicz Jr, Peter P
> > > > Subject: Re: [net-next 8/9] ixgbe: add interface to export thermal
> > > > data
> > > >
> > > > Drivers outside of drivers/hwmon just select HWMON in Kconfig. This
> > > > adds a 3kB .c file to the kernel build for the first one that needs it.
> > >
> > > This is where we run into issues though.  We can put this dependency
> > > in, but customers don't pull upstream kernels, they rely on the OSV's
> > > to distribute updates.  The customer doesn't want HWMON, and if their
> > > kernel doesn't ship with HWMON support for ixgbe, then we're sunk.
> > 
> > So if !IS_ENABLED(CONFIG_HWMON), don't try to create an hwmon device,
> > but create the attributes anyway.
> 
> And if CONFIG_HWMON is not enabled, are we then responsible for
> creating the hwmon area in sysfs to link in our attributes?  That
> would make even less sense to include HWMON attributes if HWMON isn't
> built in.

No, the attributes actually appear under the parent device.

> > > The point is what we're trying to implement, based on what our customers'
> > > requirements are, is something completely different than what HWMON is
> > > providing.  Trying to wedge HWMON onto this framework we're trying to
> > > provide just overcomplicates the entire thing we're trying to provide.
> > > It's a generic interface to generic data in our drivers,
> > [...]
> > 
> > It sounds like you want to provide a "generic" ixgbe interface on different
> > operating systems.  But that is not a valid argument for an in-tree driver.
> 
> No, we're trying to provide a generic ixgbe interface on Linux
> systems.  The reality is different OSV's pull our driver from upstream
> to build their distros,

Yes we know.

[...]
> The reality is these attributes of our thermal sensors are about 5% of
> the data we need to export.
[...]

And I think all you need to do is (1) give these particular attributes
the right names and (2) create a child hwmon device if possible.  Is
that so hard?

Ben.
Waskiewicz Jr, Peter P Jan. 9, 2012, 6:08 p.m. UTC | #15
> -----Original Message-----
> From: netdev-owner@vger.kernel.org [mailto:netdev-
> owner@vger.kernel.org] On Behalf Of Ben Hutchings
> Sent: Friday, January 06, 2012 10:25 AM
> To: Waskiewicz Jr, Peter P
> Cc: Michal Miroslaw; Skidmore, Donald C; Kirsher, Jeffrey T;
> davem@davemloft.net; netdev@vger.kernel.org; gospo@redhat.com;
> sassmann@redhat.com
> Subject: RE: [net-next 8/9] ixgbe: add interface to export thermal data
> 
> On Fri, 2012-01-06 at 18:20 +0000, Waskiewicz Jr, Peter P wrote:
> > > -----Original Message-----
> > > From: Ben Hutchings [mailto:bhutchings@solarflare.com]
> > > Sent: Friday, January 06, 2012 10:09 AM
> > > To: Waskiewicz Jr, Peter P
> > > Cc: Michal Miroslaw; Skidmore, Donald C; Kirsher, Jeffrey T;
> > > davem@davemloft.net; netdev@vger.kernel.org; gospo@redhat.com;
> > > sassmann@redhat.com
> > > Subject: RE: [net-next 8/9] ixgbe: add interface to export thermal
> > > data
> > >
> > > On Fri, 2012-01-06 at 17:55 +0000, Waskiewicz Jr, Peter P wrote:
> > > > > -----Original Message-----
> > > > > From: Michał Mirosław [mailto:mirqus@gmail.com]
> > > > > Sent: Thursday, January 05, 2012 3:51 PM
> > > > > To: Skidmore, Donald C
> > > > > Cc: Ben Hutchings; Kirsher, Jeffrey T; davem@davemloft.net;
> > > > > netdev@vger.kernel.org; gospo@redhat.com;
> sassmann@redhat.com;
> > > > > Waskiewicz Jr, Peter P
> > > > > Subject: Re: [net-next 8/9] ixgbe: add interface to export
> > > > > thermal data
> > > > >
> > > > > Drivers outside of drivers/hwmon just select HWMON in Kconfig.
> > > > > This adds a 3kB .c file to the kernel build for the first one that needs it.
> > > >
> > > > This is where we run into issues though.  We can put this
> > > > dependency in, but customers don't pull upstream kernels, they
> > > > rely on the OSV's to distribute updates.  The customer doesn't
> > > > want HWMON, and if their kernel doesn't ship with HWMON support
> for ixgbe, then we're sunk.
> > >
> > > So if !IS_ENABLED(CONFIG_HWMON), don't try to create an hwmon
> > > device, but create the attributes anyway.
> >
> > And if CONFIG_HWMON is not enabled, are we then responsible for
> > creating the hwmon area in sysfs to link in our attributes?  That
> > would make even less sense to include HWMON attributes if HWMON isn't
> > built in.
> 
> No, the attributes actually appear under the parent device.

Ok, so how we have it now in the current patches.

> > > > The point is what we're trying to implement, based on what our
> customers'
> > > > requirements are, is something completely different than what
> > > > HWMON is providing.  Trying to wedge HWMON onto this framework
> > > > we're trying to provide just overcomplicates the entire thing we're trying
> to provide.
> > > > It's a generic interface to generic data in our drivers,
> > > [...]
> > >
> > > It sounds like you want to provide a "generic" ixgbe interface on
> > > different operating systems.  But that is not a valid argument for an in-
> tree driver.
> >
> > No, we're trying to provide a generic ixgbe interface on Linux
> > systems.  The reality is different OSV's pull our driver from upstream
> > to build their distros,
> 
> Yes we know.
> 
> [...]
> > The reality is these attributes of our thermal sensors are about 5% of
> > the data we need to export.
> [...]
> 
> And I think all you need to do is (1) give these particular attributes the right
> names and (2) create a child hwmon device if possible.  Is that so hard?

Yes, it is.  The sensors that we have defined for this SKU are defined by the customers who this board was built for, so it fits with their BMC.  We don't have much control over the names.

Again, I'm failing to see why you are requiring us to put in this support when we don't benefit at all from it, we don't need it, and don't want it.  Perhaps you can add the HWMON support to ixgbe when you get around to adding the same support to your Solarflare driver that hasn't been completed yet?  We'd be happy to accept those patches.  Please don't hold us to a standard you won't hold your own driver to.

-PJ
diff mbox

Patch

diff --git a/drivers/net/ethernet/intel/ixgbe/Makefile b/drivers/net/ethernet/intel/ixgbe/Makefile
index 7d7387f..852bf86 100644
--- a/drivers/net/ethernet/intel/ixgbe/Makefile
+++ b/drivers/net/ethernet/intel/ixgbe/Makefile
@@ -34,7 +34,7 @@  obj-$(CONFIG_IXGBE) += ixgbe.o
 
 ixgbe-objs := ixgbe_main.o ixgbe_common.o ixgbe_ethtool.o \
               ixgbe_82599.o ixgbe_82598.o ixgbe_phy.o ixgbe_sriov.o \
-              ixgbe_mbx.o ixgbe_x540.o
+              ixgbe_mbx.o ixgbe_x540.o ixgbe_sysfs.o
 
 ixgbe-$(CONFIG_IXGBE_DCB) +=  ixgbe_dcb.o ixgbe_dcb_82598.o \
                               ixgbe_dcb_82599.o ixgbe_dcb_nl.o
diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe.h b/drivers/net/ethernet/intel/ixgbe/ixgbe.h
index a8368d5..3531dc9 100644
--- a/drivers/net/ethernet/intel/ixgbe/ixgbe.h
+++ b/drivers/net/ethernet/intel/ixgbe/ixgbe.h
@@ -518,6 +518,8 @@  struct ixgbe_adapter {
 	int fdir_filter_count;
 	u32 timer_event_accumulator;
 	u32 vferr_refcount;
+	struct kobject *info_kobj;
+	struct kobject *therm_kobj[IXGBE_MAX_SENSORS];
 };
 
 struct ixgbe_fdir_filter {
@@ -606,6 +608,8 @@  extern void ixgbe_set_rx_mode(struct net_device *netdev);
 extern int ixgbe_setup_tc(struct net_device *dev, u8 tc);
 extern void ixgbe_tx_ctxtdesc(struct ixgbe_ring *, u32, u32, u32, u32);
 extern void ixgbe_do_reset(struct net_device *netdev);
+extern void ixgbe_sysfs_exit(struct ixgbe_adapter *adapter);
+extern int ixgbe_sysfs_init(struct ixgbe_adapter *adapter);
 #ifdef IXGBE_FCOE
 extern void ixgbe_configure_fcoe(struct ixgbe_adapter *adapter);
 extern int ixgbe_fso(struct ixgbe_ring *tx_ring, struct sk_buff *skb,
diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_82599.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_82599.c
index 7720721..1a3810e 100644
--- a/drivers/net/ethernet/intel/ixgbe/ixgbe_82599.c
+++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_82599.c
@@ -2137,6 +2137,8 @@  static struct ixgbe_mac_operations mac_ops_82599 = {
 	.set_vlan_anti_spoofing = &ixgbe_set_vlan_anti_spoofing,
 	.acquire_swfw_sync      = &ixgbe_acquire_swfw_sync,
 	.release_swfw_sync      = &ixgbe_release_swfw_sync,
+	.get_thermal_sensor_data = &ixgbe_get_thermal_sensor_data_generic,
+	.init_thermal_sensor_thresh = &ixgbe_init_thermal_sensor_thresh_generic,
 
 };
 
diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
index e27e4d1..b5cef7e 100644
--- a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
+++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
@@ -7717,6 +7717,10 @@  static int __devinit ixgbe_probe(struct pci_dev *pdev,
 
 	e_dev_info("Intel(R) 10 Gigabit Network Connection\n");
 	cards_found++;
+
+	if (ixgbe_sysfs_init(adapter))
+		e_err(probe, "failed to allocate sysfs resources\n");
+
 	return 0;
 
 err_register:
@@ -7764,6 +7768,8 @@  static void __devexit ixgbe_remove(struct pci_dev *pdev)
 	}
 
 #endif
+	ixgbe_sysfs_exit(adapter);
+
 #ifdef IXGBE_FCOE
 	if (adapter->flags & IXGBE_FLAG_FCOE_ENABLED)
 		ixgbe_cleanup_fcoe(adapter);
diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_sysfs.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_sysfs.c
new file mode 100644
index 0000000..db818ae
--- /dev/null
+++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_sysfs.c
@@ -0,0 +1,305 @@ 
+/*******************************************************************************
+
+  Intel 10 Gigabit PCI Express Linux driver
+  Copyright(c) 1999 - 2011 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.
+
+  You should have received a copy of the GNU General Public License along with
+  this program; if not, write to the Free Software Foundation, Inc.,
+  51 Franklin St - Fifth Floor, Boston, MA 02110-1301 USA.
+
+  The full GNU General Public License is included in this distribution in
+  the file called "COPYING".
+
+  Contact Information:
+  e1000-devel Mailing List <e1000-devel@lists.sourceforge.net>
+  Intel Corporation, 5200 N.E. Elam Young Parkway, Hillsboro, OR 97124-6497
+
+*******************************************************************************/
+
+#include "ixgbe.h"
+#include "ixgbe_common.h"
+#include "ixgbe_type.h"
+
+#include <linux/module.h>
+#include <linux/types.h>
+#include <linux/sysfs.h>
+#include <linux/kobject.h>
+#include <linux/device.h>
+#include <linux/netdevice.h>
+
+/*
+ * This file provides a sysfs interface to export information from the
+ * driver.  The information presented is READ-ONLY.
+ */
+
+static struct net_device *ixgbe_get_netdev(struct kobject *kobj)
+{
+	struct net_device *netdev;
+	struct device *device_info_kobj;
+
+	if (kobj == NULL)
+		return NULL;
+
+	device_info_kobj = container_of(kobj->parent, struct device, kobj);
+	if (device_info_kobj == NULL)
+		return NULL;
+
+	netdev = container_of(device_info_kobj, struct net_device, dev);
+	return netdev;
+}
+
+static struct ixgbe_adapter *ixgbe_get_adapter(struct kobject *kobj)
+{
+	struct ixgbe_adapter *adapter;
+	struct net_device *netdev = ixgbe_get_netdev(kobj);
+
+	if (netdev == NULL)
+		return NULL;
+
+	adapter = netdev_priv(netdev);
+	return adapter;
+}
+
+static bool ixgbe_thermal_present(struct kobject *kobj)
+{
+	s32 status;
+	struct ixgbe_adapter *adapter = ixgbe_get_adapter(kobj);
+
+	if (adapter == NULL)
+		return false;
+
+	status = ixgbe_init_thermal_sensor_thresh_generic(&(adapter->hw));
+	if (status != 0)
+		return false;
+
+	return true;
+}
+
+/*
+ * ixgbe_name_to_idx - Convert the directory name to the sensor offset.
+ * @ c: pointer to the directory name string
+ *
+ * The directory name is in the form "sensor_n" where n is '0' -
+ * 'IXGBE_MAX_SENSORS'.  IXGBE_MAX_SENSORS will never be greater than
+ * 9.  This function takes advantage of that to keep it simple.
+ */
+static int ixgbe_name_to_idx(const char *c)
+{
+	/* find first digit */
+	while (*c < '0' || *c > '9') {
+		if (*c == '\n')
+			return -1;
+		c++;
+	}
+
+	return (int)(*c - '0');
+}
+
+static s32 ixgbe_sysfs_get_thermal_data(struct kobject *kobj, char *buf)
+{
+	struct ixgbe_adapter *adapter = ixgbe_get_adapter(kobj->parent);
+	s32 status;
+
+	if (adapter == NULL) {
+		snprintf(buf, PAGE_SIZE, "error: missing adapter\n");
+		return 0;
+	}
+
+	if (&adapter->hw == NULL) {
+		snprintf(buf, PAGE_SIZE, "error: missing hw\n");
+		return 0;
+	}
+
+	status = ixgbe_get_thermal_sensor_data_generic(&adapter->hw);
+
+	return status;
+}
+
+static ssize_t ixgbe_sysfs_location(struct kobject *kobj,
+				    struct kobj_attribute *attr, char *buf)
+{
+	struct ixgbe_adapter *adapter = ixgbe_get_adapter(kobj->parent);
+	int idx;
+
+	if (adapter == NULL)
+		return snprintf(buf, PAGE_SIZE, "error: no adapter\n");
+
+	idx = ixgbe_name_to_idx(kobj->name);
+	if (idx == -1)
+		return snprintf(buf, PAGE_SIZE,
+			"error: invalid sensor name %s\n", kobj->name);
+
+	return snprintf(buf, PAGE_SIZE, "%d\n",
+		adapter->hw.mac.thermal_sensor_data.sensor[idx].location);
+}
+
+static ssize_t ixgbe_sysfs_temp(struct kobject *kobj,
+				struct kobj_attribute *attr, char *buf)
+{
+	struct ixgbe_adapter *adapter = ixgbe_get_adapter(kobj->parent);
+	int idx;
+
+	s32 status = ixgbe_sysfs_get_thermal_data(kobj, buf);
+
+	if (status != 0)
+		return snprintf(buf, PAGE_SIZE, "error: status %d returned",
+				status);
+
+	idx = ixgbe_name_to_idx(kobj->name);
+	if (idx == -1)
+		return snprintf(buf, PAGE_SIZE,
+			"error: invalid sensor name %s\n", kobj->name);
+
+	return snprintf(buf, PAGE_SIZE, "%d\n",
+		adapter->hw.mac.thermal_sensor_data.sensor[idx].temp);
+}
+
+static ssize_t ixgbe_sysfs_maxopthresh(struct kobject *kobj,
+				       struct kobj_attribute *attr, char *buf)
+{
+	struct ixgbe_adapter *adapter = ixgbe_get_adapter(kobj->parent);
+	int idx;
+
+	if (adapter == NULL)
+		return snprintf(buf, PAGE_SIZE, "error: no adapter\n");
+
+	idx = ixgbe_name_to_idx(kobj->name);
+	if (idx == -1)
+		return snprintf(buf, PAGE_SIZE,
+			"error: invalid sensor name %s\n", kobj->name);
+
+	return snprintf(buf, PAGE_SIZE, "%d\n",
+		adapter->hw.mac.thermal_sensor_data.sensor[idx].max_op_thresh);
+}
+
+static ssize_t ixgbe_sysfs_cautionthresh(struct kobject *kobj,
+					 struct kobj_attribute *attr, char *buf)
+{
+	struct ixgbe_adapter *adapter = ixgbe_get_adapter(kobj->parent);
+	int idx;
+
+	if (adapter == NULL)
+		return snprintf(buf, PAGE_SIZE, "error: no adapter\n");
+
+	idx = ixgbe_name_to_idx(kobj->name);
+	if (idx == -1)
+		return snprintf(buf, PAGE_SIZE,
+			"error: invalid sensor name %s\n", kobj->name);
+
+	return snprintf(buf, PAGE_SIZE, "%d\n",
+		adapter->hw.mac.thermal_sensor_data.sensor[idx].caution_thresh);
+}
+
+/* Initialize the attributes */
+static struct kobj_attribute ixgbe_sysfs_location_attr =
+	__ATTR(location, 0444, ixgbe_sysfs_location, NULL);
+static struct kobj_attribute ixgbe_sysfs_temp_attr =
+	__ATTR(temp, 0444, ixgbe_sysfs_temp, NULL);
+static struct kobj_attribute ixgbe_sysfs_cautionthresh_attr =
+	__ATTR(cautionthresh, 0444, ixgbe_sysfs_cautionthresh, NULL);
+static struct kobj_attribute ixgbe_sysfs_maxopthresh_attr =
+	__ATTR(maxopthresh, 0444, ixgbe_sysfs_maxopthresh, NULL);
+
+/* Add the attributes into an array, to be added to a group */
+static struct attribute *therm_attrs[] = {
+	&ixgbe_sysfs_location_attr.attr,
+	&ixgbe_sysfs_temp_attr.attr,
+	&ixgbe_sysfs_cautionthresh_attr.attr,
+	&ixgbe_sysfs_maxopthresh_attr.attr,
+	NULL
+};
+
+/* add attributes to a group */
+static struct attribute_group therm_attr_group = {
+	.attrs = therm_attrs,
+};
+
+static void ixgbe_del_adapter(struct ixgbe_adapter *adapter)
+{
+	int i;
+
+	if (adapter == NULL)
+		return;
+
+	for (i = 0; i < IXGBE_MAX_SENSORS; i++) {
+		if (adapter->therm_kobj[i] == NULL)
+			continue;
+		sysfs_remove_group(adapter->therm_kobj[i], &therm_attr_group);
+		kobject_put(adapter->therm_kobj[i]);
+	}
+	if (adapter->info_kobj != NULL)
+		kobject_put(adapter->info_kobj);
+}
+
+/* called from ixgbe_main.c */
+void ixgbe_sysfs_exit(struct ixgbe_adapter *adapter)
+{
+	ixgbe_del_adapter(adapter);
+}
+
+/* called from ixgbe_main.c */
+int ixgbe_sysfs_init(struct ixgbe_adapter *adapter)
+{
+	struct net_device *netdev;
+	int rc = 0;
+	int i;
+	char buf[16];
+
+	if (adapter == NULL)
+		goto err;
+	netdev = adapter->netdev;
+	if (netdev == NULL)
+		goto err;
+
+	adapter->info_kobj = NULL;
+	for (i = 0; i < IXGBE_MAX_SENSORS; i++)
+		adapter->therm_kobj[i] = NULL;
+
+	/* create info kobj and attribute listings in kobj */
+	adapter->info_kobj = kobject_create_and_add("info",
+					&(netdev->dev.kobj));
+	if (adapter->info_kobj == NULL)
+		goto err;
+
+	/* Don't create thermal subkobjs if no data present */
+	if (ixgbe_thermal_present(adapter->info_kobj) != true)
+		goto exit;
+
+	for (i = 0; i < IXGBE_MAX_SENSORS; i++) {
+
+		/*
+		 * Likewise only create individual kobjs that have
+		 * meaningful data.
+		 */
+		if (adapter->hw.mac.thermal_sensor_data.sensor[i].location == 0)
+			continue;
+
+		/* directory named after sensor offset */
+		snprintf(buf, sizeof(buf), "sensor_%d", i);
+		adapter->therm_kobj[i] =
+			kobject_create_and_add(buf, adapter->info_kobj);
+		if (adapter->therm_kobj[i] == NULL)
+			goto err;
+		if (sysfs_create_group(adapter->therm_kobj[i],
+				       &therm_attr_group))
+			goto err;
+	}
+
+	goto exit;
+
+err:
+	ixgbe_del_adapter(adapter);
+	rc = -1;
+exit:
+	return rc;
+}
+