[PATCH 0/5] net: hwmon fixes

[PATCH 0/5] net: hwmon fixes

[Guenter Roeck]

The hwmon subsystem is used by various network drivers to report temperature
sensor and other information. Unfortunately, its use is often not correct.
Typical errors are that the mandatory name sysfs attribute is not created,
that the temperature sensor index starts with 0 instead of 1, and/or that
sysfs attributes are created after the hwmon device was created.
The following sequence of patches fixes most of the problems.

The igb patches have been tested with real hardware; the others are compile
tested only.

[PATCH 1/5] tg3: Convert to use hwmon_device_register_with_groups

[Guenter Roeck]

Use new hwmon API to simplify code, provide missing mandatory 'name'
sysfs attribute, and attach hwmon attributes to hwmon device instead
of pci device.

Signed-off-by: Guenter Roeck <linux@roeck-us.net>
---
 drivers/net/ethernet/broadcom/tg3.c |   25 ++++++-------------------
 1 file changed, 6 insertions(+), 19 deletions(-)

diff --git a/drivers/net/ethernet/broadcom/tg3.c b/drivers/net/ethernet/broadcom/tg3.c
index a9e0684..369b736 100644
--- a/drivers/net/ethernet/broadcom/tg3.c
+++ b/drivers/net/ethernet/broadcom/tg3.c
@@ -10629,10 +10629,8 @@ static void tg3_sd_scan_scratchpad(struct tg3 *tp, struct tg3_ocir *ocir)
 static ssize_t tg3_show_temp(struct device *dev,
 			     struct device_attribute *devattr, char *buf)
 {
-	struct pci_dev *pdev = to_pci_dev(dev);
-	struct net_device *netdev = pci_get_drvdata(pdev);
-	struct tg3 *tp = netdev_priv(netdev);
 	struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
+	struct tg3 *tp = dev_get_drvdata(dev);
 	u32 temperature;
 
 	spin_lock_bh(&tp->lock);
@@ -10650,29 +10648,25 @@ static SENSOR_DEVICE_ATTR(temp1_crit, S_IRUGO, tg3_show_temp, NULL,
 static SENSOR_DEVICE_ATTR(temp1_max, S_IRUGO, tg3_show_temp, NULL,
 			  TG3_TEMP_MAX_OFFSET);
 
-static struct attribute *tg3_attributes[] = {
+static struct attribute *tg3_attrs[] = {
 	&sensor_dev_attr_temp1_input.dev_attr.attr,
 	&sensor_dev_attr_temp1_crit.dev_attr.attr,
 	&sensor_dev_attr_temp1_max.dev_attr.attr,
 	NULL
 };
-
-static const struct attribute_group tg3_group = {
-	.attrs = tg3_attributes,
-};
+ATTRIBUTE_GROUPS(tg3);
 
 static void tg3_hwmon_close(struct tg3 *tp)
 {
 	if (tp->hwmon_dev) {
 		hwmon_device_unregister(tp->hwmon_dev);
 		tp->hwmon_dev = NULL;
-		sysfs_remove_group(&tp->pdev->dev.kobj, &tg3_group);
 	}
 }
 
 static void tg3_hwmon_open(struct tg3 *tp)
 {
-	int i, err;
+	int i;
 	u32 size = 0;
 	struct pci_dev *pdev = tp->pdev;
 	struct tg3_ocir ocirs[TG3_SD_NUM_RECS];
@@ -10690,18 +10684,11 @@ static void tg3_hwmon_open(struct tg3 *tp)
 	if (!size)
 		return;
 
-	/* Register hwmon sysfs hooks */
-	err = sysfs_create_group(&pdev->dev.kobj, &tg3_group);
-	if (err) {
-		dev_err(&pdev->dev, "Cannot create sysfs group, aborting\n");
-		return;
-	}
-
-	tp->hwmon_dev = hwmon_device_register(&pdev->dev);
+	tp->hwmon_dev = hwmon_device_register_with_groups(&pdev->dev, "tg3",
+							  tp, tg3_groups);
 	if (IS_ERR(tp->hwmon_dev)) {
 		tp->hwmon_dev = NULL;
 		dev_err(&pdev->dev, "Cannot register hwmon device, aborting\n");
-		sysfs_remove_group(&pdev->dev.kobj, &tg3_group);
 	}
 }
 
-- 
1.7.9.7

Re: [lm-sensors] [PATCH 1/5] tg3: Convert to use hwmon_device_register_with_groups

[Jean Delvare]

Hi Guenter,

On Fri, 22 Nov 2013 22:07:57 -0800, Guenter Roeck wrote:
> Use new hwmon API to simplify code, provide missing mandatory 'name'
> sysfs attribute, and attach hwmon attributes to hwmon device instead
> of pci device.
> 
> Signed-off-by: Guenter Roeck <linux@roeck-us.net>
> ---
>  drivers/net/ethernet/broadcom/tg3.c |   25 ++++++-------------------
>  1 file changed, 6 insertions(+), 19 deletions(-)
> 
> diff --git a/drivers/net/ethernet/broadcom/tg3.c b/drivers/net/ethernet/broadcom/tg3.c
> index a9e0684..369b736 100644
> --- a/drivers/net/ethernet/broadcom/tg3.c
> +++ b/drivers/net/ethernet/broadcom/tg3.c
> @@ -10629,10 +10629,8 @@ static void tg3_sd_scan_scratchpad(struct tg3 *tp, struct tg3_ocir *ocir)
>  static ssize_t tg3_show_temp(struct device *dev,
>  			     struct device_attribute *devattr, char *buf)
>  {
> -	struct pci_dev *pdev = to_pci_dev(dev);
> -	struct net_device *netdev = pci_get_drvdata(pdev);
> -	struct tg3 *tp = netdev_priv(netdev);
>  	struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
> +	struct tg3 *tp = dev_get_drvdata(dev);
>  	u32 temperature;
>  
>  	spin_lock_bh(&tp->lock);
> @@ -10650,29 +10648,25 @@ static SENSOR_DEVICE_ATTR(temp1_crit, S_IRUGO, tg3_show_temp, NULL,
>  static SENSOR_DEVICE_ATTR(temp1_max, S_IRUGO, tg3_show_temp, NULL,
>  			  TG3_TEMP_MAX_OFFSET);
>  
> -static struct attribute *tg3_attributes[] = {
> +static struct attribute *tg3_attrs[] = {
>  	&sensor_dev_attr_temp1_input.dev_attr.attr,
>  	&sensor_dev_attr_temp1_crit.dev_attr.attr,
>  	&sensor_dev_attr_temp1_max.dev_attr.attr,
>  	NULL
>  };
> -
> -static const struct attribute_group tg3_group = {
> -	.attrs = tg3_attributes,
> -};
> +ATTRIBUTE_GROUPS(tg3);
>  
>  static void tg3_hwmon_close(struct tg3 *tp)
>  {
>  	if (tp->hwmon_dev) {
>  		hwmon_device_unregister(tp->hwmon_dev);
>  		tp->hwmon_dev = NULL;
> -		sysfs_remove_group(&tp->pdev->dev.kobj, &tg3_group);
>  	}
>  }
>  
>  static void tg3_hwmon_open(struct tg3 *tp)
>  {
> -	int i, err;
> +	int i;
>  	u32 size = 0;
>  	struct pci_dev *pdev = tp->pdev;
>  	struct tg3_ocir ocirs[TG3_SD_NUM_RECS];
> @@ -10690,18 +10684,11 @@ static void tg3_hwmon_open(struct tg3 *tp)
>  	if (!size)
>  		return;
>  
> -	/* Register hwmon sysfs hooks */
> -	err = sysfs_create_group(&pdev->dev.kobj, &tg3_group);
> -	if (err) {
> -		dev_err(&pdev->dev, "Cannot create sysfs group, aborting\n");
> -		return;
> -	}
> -
> -	tp->hwmon_dev = hwmon_device_register(&pdev->dev);
> +	tp->hwmon_dev = hwmon_device_register_with_groups(&pdev->dev, "tg3",
> +							  tp, tg3_groups);
>  	if (IS_ERR(tp->hwmon_dev)) {
>  		tp->hwmon_dev = NULL;
>  		dev_err(&pdev->dev, "Cannot register hwmon device, aborting\n");
> -		sysfs_remove_group(&pdev->dev.kobj, &tg3_group);
>  	}
>  }
>  

I can't test this, but the changes look good to me.

Reviewed-by: Jean Delvare <khali@linux-fr.org>

-- 
Jean Delvare

Re: [PATCH 1/5] tg3: Convert to use hwmon_device_register_with_groups

[Nithin Nayak Sujir]

On 11/22/2013 10:07 PM, Guenter Roeck wrote:
> Use new hwmon API to simplify code, provide missing mandatory 'name'
> sysfs attribute, and attach hwmon attributes to hwmon device instead
> of pci device.
>
> Signed-off-by: Guenter Roeck <linux@roeck-us.net>
> ---
>   drivers/net/ethernet/broadcom/tg3.c |   25 ++++++-------------------
>   1 file changed, 6 insertions(+), 19 deletions(-)
>
> diff --git a/drivers/net/ethernet/broadcom/tg3.c b/drivers/net/ethernet/broadcom/tg3.c
> index a9e0684..369b736 100644
> --- a/drivers/net/ethernet/broadcom/tg3.c
> +++ b/drivers/net/ethernet/broadcom/tg3.c
> @@ -10629,10 +10629,8 @@ static void tg3_sd_scan_scratchpad(struct tg3 *tp, struct tg3_ocir *ocir)
>   static ssize_t tg3_show_temp(struct device *dev,
>   			     struct device_attribute *devattr, char *buf)
>   {
> -	struct pci_dev *pdev = to_pci_dev(dev);
> -	struct net_device *netdev = pci_get_drvdata(pdev);
> -	struct tg3 *tp = netdev_priv(netdev);
>   	struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
> +	struct tg3 *tp = dev_get_drvdata(dev);


Shouldn't this be
struct tg3 *tp = netdev_priv(dev_get_drvdata(dev));

Re: [PATCH 1/5] tg3: Convert to use hwmon_device_register_with_groups

[Guenter Roeck]

On 11/25/2013 05:52 PM, Nithin Nayak Sujir wrote:
>
>
> On 11/22/2013 10:07 PM, Guenter Roeck wrote:
>> Use new hwmon API to simplify code, provide missing mandatory 'name'
>> sysfs attribute, and attach hwmon attributes to hwmon device instead
>> of pci device.
>>
>> Signed-off-by: Guenter Roeck <linux@roeck-us.net>
>> ---
>>   drivers/net/ethernet/broadcom/tg3.c |   25 ++++++-------------------
>>   1 file changed, 6 insertions(+), 19 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/broadcom/tg3.c b/drivers/net/ethernet/broadcom/tg3.c
>> index a9e0684..369b736 100644
>> --- a/drivers/net/ethernet/broadcom/tg3.c
>> +++ b/drivers/net/ethernet/broadcom/tg3.c
>> @@ -10629,10 +10629,8 @@ static void tg3_sd_scan_scratchpad(struct tg3 *tp, struct tg3_ocir *ocir)
>>   static ssize_t tg3_show_temp(struct device *dev,
>>                    struct device_attribute *devattr, char *buf)
>>   {
>> -    struct pci_dev *pdev = to_pci_dev(dev);
>> -    struct net_device *netdev = pci_get_drvdata(pdev);
>> -    struct tg3 *tp = netdev_priv(netdev);
>>       struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
>> +    struct tg3 *tp = dev_get_drvdata(dev);
>
>
> Shouldn't this be
> struct tg3 *tp = netdev_priv(dev_get_drvdata(dev));
>

'struct tg3 *tp' is attached to the hwmon device in
hwmon_device_register_with_groups(), so it can be retrieved
with dev_get_drvdata() from there. Keep in mind that 'dev'
is no longer the pci device but the hwmon device.

Guenter



------------------------------------------------------------------------------
Shape the Mobile Experience: Free Subscription
Software experts and developers: Be at the forefront of tech innovation.
Intel(R) Software Adrenaline delivers strategic insight and game-changing 
conversations that shape the rapidly evolving mobile landscape. Sign up now. 
http://pubads.g.doubleclick.net/gampad/clk?id=63431311&iu=/4140/ostg.clktrk
_______________________________________________
E1000-devel mailing list
E1000-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/e1000-devel
To learn more about Intel&#174; Ethernet, visit http://communities.intel.com/community/wired

Re: [PATCH 1/5] tg3: Convert to use hwmon_device_register_with_groups

[Nithin Nayak Sujir]

On 11/25/2013 06:47 PM, Guenter Roeck wrote:
> On 11/25/2013 05:52 PM, Nithin Nayak Sujir wrote:
>>
>>
>> On 11/22/2013 10:07 PM, Guenter Roeck wrote:
>>> Use new hwmon API to simplify code, provide missing mandatory 'name'
>>> sysfs attribute, and attach hwmon attributes to hwmon device instead
>>> of pci device.
>>>
>>> Signed-off-by: Guenter Roeck <linux@roeck-us.net>
>>> ---
>>>   drivers/net/ethernet/broadcom/tg3.c |   25 ++++++-------------------
>>>   1 file changed, 6 insertions(+), 19 deletions(-)
>>>
>>> diff --git a/drivers/net/ethernet/broadcom/tg3.c
>>> b/drivers/net/ethernet/broadcom/tg3.c
>>> index a9e0684..369b736 100644
>>> --- a/drivers/net/ethernet/broadcom/tg3.c
>>> +++ b/drivers/net/ethernet/broadcom/tg3.c
>>> @@ -10629,10 +10629,8 @@ static void tg3_sd_scan_scratchpad(struct tg3 *tp,
>>> struct tg3_ocir *ocir)
>>>   static ssize_t tg3_show_temp(struct device *dev,
>>>                    struct device_attribute *devattr, char *buf)
>>>   {
>>> -    struct pci_dev *pdev = to_pci_dev(dev);
>>> -    struct net_device *netdev = pci_get_drvdata(pdev);
>>> -    struct tg3 *tp = netdev_priv(netdev);
>>>       struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
>>> +    struct tg3 *tp = dev_get_drvdata(dev);
>>
>>
>> Shouldn't this be
>> struct tg3 *tp = netdev_priv(dev_get_drvdata(dev));
>>
>
> 'struct tg3 *tp' is attached to the hwmon device in
> hwmon_device_register_with_groups(), so it can be retrieved
> with dev_get_drvdata() from there. Keep in mind that 'dev'
> is no longer the pci device but the hwmon device.
>

Ah, I see.

Acked-by: Nithin Nayak Sujir <nsujir@broadcom.com>


> Guenter
>
>
> --
> 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
>

Re: [PATCH 1/5] tg3: Convert to use hwmon_device_register_with_groups

[David Miller]

From: Guenter Roeck <linux@roeck-us.net>
Date: Fri, 22 Nov 2013 22:07:57 -0800

> Use new hwmon API to simplify code, provide missing mandatory 'name'
> sysfs attribute, and attach hwmon attributes to hwmon device instead
> of pci device.
> 
> Signed-off-by: Guenter Roeck <linux@roeck-us.net>

Applied, thanks.

Jeff Kirsher will pull the rest of this series in via his Intel ethernet
driver tree.

[PATCH 2/5] igb: Convert to use devm_hwmon_device_register_with_groups

[Guenter Roeck]

Simplify the code. Attach hwmon sysfs attributes to hwmon device
instead of pci device. Avoid race conditions caused by attributes
being created after registration and provide mandatory 'name'
attribute by using new hwmon API.

Other cleanup:

Instead of allocating memory for hwmon attributes, move attributes
and all other hwmon related data into struct hwmon_buff and allocate
the entire structure using devm_kzalloc.

Check return value from calls to igb_add_hwmon_attr() one by one instead
of logically combining them all together.

Signed-off-by: Guenter Roeck <linux@roeck-us.net>
---
 drivers/net/ethernet/intel/igb/igb.h       |    8 ++-
 drivers/net/ethernet/intel/igb/igb_hwmon.c |  100 +++++++++++++---------------
 2 files changed, 53 insertions(+), 55 deletions(-)

diff --git a/drivers/net/ethernet/intel/igb/igb.h b/drivers/net/ethernet/intel/igb/igb.h
index 5e9ed89..99c3d33 100644
--- a/drivers/net/ethernet/intel/igb/igb.h
+++ b/drivers/net/ethernet/intel/igb/igb.h
@@ -337,8 +337,10 @@ struct hwmon_attr {
 	};
 
 struct hwmon_buff {
-	struct device *device;
-	struct hwmon_attr *hwmon_list;
+	struct attribute_group group;
+	const struct attribute_group *groups[2];
+	struct attribute *attrs[E1000_MAX_SENSORS * 4 + 1];
+	struct hwmon_attr hwmon_list[E1000_MAX_SENSORS * 4];
 	unsigned int n_hwmon;
 	};
 #endif
@@ -440,7 +442,7 @@ struct igb_adapter {
 
 	char fw_version[32];
 #ifdef CONFIG_IGB_HWMON
-	struct hwmon_buff igb_hwmon_buff;
+	struct hwmon_buff *igb_hwmon_buff;
 	bool ets;
 #endif
 	struct i2c_algo_bit_data i2c_algo;
diff --git a/drivers/net/ethernet/intel/igb/igb_hwmon.c b/drivers/net/ethernet/intel/igb/igb_hwmon.c
index 58f1ce9..2e7ef2d 100644
--- a/drivers/net/ethernet/intel/igb/igb_hwmon.c
+++ b/drivers/net/ethernet/intel/igb/igb_hwmon.c
@@ -117,8 +117,8 @@ static int igb_add_hwmon_attr(struct igb_adapter *adapter,
 	unsigned int n_attr;
 	struct hwmon_attr *igb_attr;
 
-	n_attr = adapter->igb_hwmon_buff.n_hwmon;
-	igb_attr = &adapter->igb_hwmon_buff.hwmon_list[n_attr];
+	n_attr = adapter->igb_hwmon_buff->n_hwmon;
+	igb_attr = &adapter->igb_hwmon_buff->hwmon_list[n_attr];
 
 	switch (type) {
 	case IGB_HWMON_TYPE_LOC:
@@ -154,30 +154,16 @@ static int igb_add_hwmon_attr(struct igb_adapter *adapter,
 	igb_attr->dev_attr.attr.mode = S_IRUGO;
 	igb_attr->dev_attr.attr.name = igb_attr->name;
 	sysfs_attr_init(&igb_attr->dev_attr.attr);
-	rc = device_create_file(&adapter->pdev->dev,
-				&igb_attr->dev_attr);
-	if (rc == 0)
-		++adapter->igb_hwmon_buff.n_hwmon;
 
-	return rc;
+	adapter->igb_hwmon_buff->attrs[n_attr] = &igb_attr->dev_attr.attr;
+
+	++adapter->igb_hwmon_buff->n_hwmon;
+
+	return 0;
 }
 
 static void igb_sysfs_del_adapter(struct igb_adapter *adapter)
 {
-	int i;
-
-	if (adapter == NULL)
-		return;
-
-	for (i = 0; i < adapter->igb_hwmon_buff.n_hwmon; i++) {
-		device_remove_file(&adapter->pdev->dev,
-			   &adapter->igb_hwmon_buff.hwmon_list[i].dev_attr);
-	}
-
-	kfree(adapter->igb_hwmon_buff.hwmon_list);
-
-	if (adapter->igb_hwmon_buff.device)
-		hwmon_device_unregister(adapter->igb_hwmon_buff.device);
 }
 
 /* called from igb_main.c */
@@ -189,11 +175,11 @@ void igb_sysfs_exit(struct igb_adapter *adapter)
 /* called from igb_main.c */
 int igb_sysfs_init(struct igb_adapter *adapter)
 {
-	struct hwmon_buff *igb_hwmon = &adapter->igb_hwmon_buff;
+	struct hwmon_buff *igb_hwmon;
+	struct i2c_client *client;
+	struct device *hwmon_dev;
 	unsigned int i;
-	int n_attrs;
 	int rc = 0;
-	struct i2c_client *client = NULL;
 
 	/* If this method isn't defined we don't support thermals */
 	if (adapter->hw.mac.ops.init_thermal_sensor_thresh == NULL)
@@ -201,34 +187,16 @@ int igb_sysfs_init(struct igb_adapter *adapter)
 
 	/* Don't create thermal hwmon interface if no sensors present */
 	rc = (adapter->hw.mac.ops.init_thermal_sensor_thresh(&adapter->hw));
-		if (rc)
-			goto exit;
-
-	/* init i2c_client */
-	client = i2c_new_device(&adapter->i2c_adap, &i350_sensor_info);
-	if (client == NULL) {
-		dev_info(&adapter->pdev->dev,
-			"Failed to create new i2c device..\n");
+	if (rc)
 		goto exit;
-	}
-	adapter->i2c_client = client;
 
-	/* Allocation space for max attributes
-	 * max num sensors * values (loc, temp, max, caution)
-	 */
-	n_attrs = E1000_MAX_SENSORS * 4;
-	igb_hwmon->hwmon_list = kcalloc(n_attrs, sizeof(struct hwmon_attr),
-					GFP_KERNEL);
-	if (!igb_hwmon->hwmon_list) {
+	igb_hwmon = devm_kzalloc(&adapter->pdev->dev, sizeof(*igb_hwmon),
+				 GFP_KERNEL);
+	if (!igb_hwmon) {
 		rc = -ENOMEM;
-		goto err;
-	}
-
-	igb_hwmon->device = hwmon_device_register(&adapter->pdev->dev);
-	if (IS_ERR(igb_hwmon->device)) {
-		rc = PTR_ERR(igb_hwmon->device);
-		goto err;
+		goto exit;
 	}
+	adapter->igb_hwmon_buff = igb_hwmon;
 
 	for (i = 0; i < E1000_MAX_SENSORS; i++) {
 
@@ -240,11 +208,39 @@ int igb_sysfs_init(struct igb_adapter *adapter)
 
 		/* Bail if any hwmon attr struct fails to initialize */
 		rc = igb_add_hwmon_attr(adapter, i, IGB_HWMON_TYPE_CAUTION);
-		rc |= igb_add_hwmon_attr(adapter, i, IGB_HWMON_TYPE_LOC);
-		rc |= igb_add_hwmon_attr(adapter, i, IGB_HWMON_TYPE_TEMP);
-		rc |= igb_add_hwmon_attr(adapter, i, IGB_HWMON_TYPE_MAX);
 		if (rc)
-			goto err;
+			goto exit;
+		rc = igb_add_hwmon_attr(adapter, i, IGB_HWMON_TYPE_LOC);
+		if (rc)
+			goto exit;
+		rc = igb_add_hwmon_attr(adapter, i, IGB_HWMON_TYPE_TEMP);
+		if (rc)
+			goto exit;
+		rc = igb_add_hwmon_attr(adapter, i, IGB_HWMON_TYPE_MAX);
+		if (rc)
+			goto exit;
+	}
+
+	/* init i2c_client */
+	client = i2c_new_device(&adapter->i2c_adap, &i350_sensor_info);
+	if (client == NULL) {
+		dev_info(&adapter->pdev->dev,
+			 "Failed to create new i2c device.\n");
+		rc = -ENODEV;
+		goto exit;
+	}
+	adapter->i2c_client = client;
+
+	igb_hwmon->groups[0] = &igb_hwmon->group;
+	igb_hwmon->group.attrs = igb_hwmon->attrs;
+
+	hwmon_dev = devm_hwmon_device_register_with_groups(&adapter->pdev->dev,
+							   client->name,
+							   igb_hwmon,
+							   igb_hwmon->groups);
+	if (IS_ERR(hwmon_dev)) {
+		rc = PTR_ERR(hwmon_dev);
+		goto err;
 	}
 
 	goto exit;
-- 
1.7.9.7

Re: [PATCH 2/5] igb: Convert to use devm_hwmon_device_register_with_groups

[Jeff Kirsher]

[-- Attachment #1: Type: text/plain, Size: 941 bytes --]

On Fri, 2013-11-22 at 22:07 -0800, Guenter Roeck wrote:
> Simplify the code. Attach hwmon sysfs attributes to hwmon device
> instead of pci device. Avoid race conditions caused by attributes
> being created after registration and provide mandatory 'name'
> attribute by using new hwmon API.
> 
> Other cleanup:
> 
> Instead of allocating memory for hwmon attributes, move attributes
> and all other hwmon related data into struct hwmon_buff and allocate
> the entire structure using devm_kzalloc.
> 
> Check return value from calls to igb_add_hwmon_attr() one by one
> instead
> of logically combining them all together.
> 
> Signed-off-by: Guenter Roeck <linux@roeck-us.net>
> ---
>  drivers/net/ethernet/intel/igb/igb.h       |    8 ++-
>  drivers/net/ethernet/intel/igb/igb_hwmon.c |  100
> +++++++++++++---------------
>  2 files changed, 53 insertions(+), 55 deletions(-)

I have added this to my queue, thanks!

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

[PATCH 3/5] ixgbe: Convert to use devm_hwmon_device_register_with_groups

[Guenter Roeck]

Simplify the code. Attach hwmon sysfs attributes to hwmon device
instead of pci device. Avoid race conditions caused by attributes
being created after hwmon device registration. Implicitly
(through hwmon API) add mandatory 'name' sysfs attribute.

Other cleanup:

Instead of allocating memory for hwmon attributes, move attributes
and all other hwmon related data into struct hwmon_buff and allocate
the entire structure using devm_kzalloc.

Check return value from calls to igb_add_hwmon_attr() one by one instead
of logically combining them all together.

Signed-off-by: Guenter Roeck <linux@roeck-us.net>
---
 drivers/net/ethernet/intel/ixgbe/ixgbe.h       |    8 ++-
 drivers/net/ethernet/intel/ixgbe/ixgbe_sysfs.c |   76 ++++++++++--------------
 2 files changed, 36 insertions(+), 48 deletions(-)

diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe.h b/drivers/net/ethernet/intel/ixgbe/ixgbe.h
index f38fc0a..49531cd 100644
--- a/drivers/net/ethernet/intel/ixgbe/ixgbe.h
+++ b/drivers/net/ethernet/intel/ixgbe/ixgbe.h
@@ -552,8 +552,10 @@ struct hwmon_attr {
 };
 
 struct hwmon_buff {
-	struct device *device;
-	struct hwmon_attr *hwmon_list;
+	struct attribute_group group;
+	const struct attribute_group *groups[2];
+	struct attribute *attrs[IXGBE_MAX_SENSORS * 4 + 1];
+	struct hwmon_attr hwmon_list[IXGBE_MAX_SENSORS * 4];
 	unsigned int n_hwmon;
 };
 #endif /* CONFIG_IXGBE_HWMON */
@@ -775,7 +777,7 @@ struct ixgbe_adapter {
 	u32 vferr_refcount;
 	struct kobject *info_kobj;
 #ifdef CONFIG_IXGBE_HWMON
-	struct hwmon_buff ixgbe_hwmon_buff;
+	struct hwmon_buff *ixgbe_hwmon_buff;
 #endif /* CONFIG_IXGBE_HWMON */
 #ifdef CONFIG_DEBUG_FS
 	struct dentry *ixgbe_dbg_adapter;
diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_sysfs.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_sysfs.c
index d118def..3081974 100644
--- a/drivers/net/ethernet/intel/ixgbe/ixgbe_sysfs.c
+++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_sysfs.c
@@ -111,8 +111,8 @@ static int ixgbe_add_hwmon_attr(struct ixgbe_adapter *adapter,
 	unsigned int n_attr;
 	struct hwmon_attr *ixgbe_attr;
 
-	n_attr = adapter->ixgbe_hwmon_buff.n_hwmon;
-	ixgbe_attr = &adapter->ixgbe_hwmon_buff.hwmon_list[n_attr];
+	n_attr = adapter->ixgbe_hwmon_buff->n_hwmon;
+	ixgbe_attr = &adapter->ixgbe_hwmon_buff->hwmon_list[n_attr];
 
 	switch (type) {
 	case IXGBE_HWMON_TYPE_LOC:
@@ -147,32 +147,17 @@ static int ixgbe_add_hwmon_attr(struct ixgbe_adapter *adapter,
 	ixgbe_attr->dev_attr.store = NULL;
 	ixgbe_attr->dev_attr.attr.mode = S_IRUGO;
 	ixgbe_attr->dev_attr.attr.name = ixgbe_attr->name;
+	sysfs_attr_init(&ixgbe_attr->dev_attr.attr);
 
-	rc = device_create_file(&adapter->pdev->dev,
-				&ixgbe_attr->dev_attr);
+	adapter->ixgbe_hwmon_buff->attrs[n_attr] = &ixgbe_attr->dev_attr.attr;
 
-	if (rc == 0)
-		++adapter->ixgbe_hwmon_buff.n_hwmon;
+	++adapter->ixgbe_hwmon_buff->n_hwmon;
 
-	return rc;
+	return 0;
 }
 
 static void ixgbe_sysfs_del_adapter(struct ixgbe_adapter *adapter)
 {
-	int i;
-
-	if (adapter == NULL)
-		return;
-
-	for (i = 0; i < adapter->ixgbe_hwmon_buff.n_hwmon; i++) {
-		device_remove_file(&adapter->pdev->dev,
-			   &adapter->ixgbe_hwmon_buff.hwmon_list[i].dev_attr);
-	}
-
-	kfree(adapter->ixgbe_hwmon_buff.hwmon_list);
-
-	if (adapter->ixgbe_hwmon_buff.device)
-		hwmon_device_unregister(adapter->ixgbe_hwmon_buff.device);
 }
 
 /* called from ixgbe_main.c */
@@ -184,9 +169,9 @@ void ixgbe_sysfs_exit(struct ixgbe_adapter *adapter)
 /* called from ixgbe_main.c */
 int ixgbe_sysfs_init(struct ixgbe_adapter *adapter)
 {
-	struct hwmon_buff *ixgbe_hwmon = &adapter->ixgbe_hwmon_buff;
+	struct hwmon_buff *ixgbe_hwmon;
+	struct device *hwmon_dev;
 	unsigned int i;
-	int n_attrs;
 	int rc = 0;
 
 	/* If this method isn't defined we don't support thermals */
@@ -198,23 +183,13 @@ int ixgbe_sysfs_init(struct ixgbe_adapter *adapter)
 	if (adapter->hw.mac.ops.init_thermal_sensor_thresh(&adapter->hw))
 		goto exit;
 
-	/*
-	 * Allocation space for max attributs
-	 * max num sensors * values (loc, temp, max, caution)
-	 */
-	n_attrs = IXGBE_MAX_SENSORS * 4;
-	ixgbe_hwmon->hwmon_list = kcalloc(n_attrs, sizeof(struct hwmon_attr),
-					  GFP_KERNEL);
-	if (!ixgbe_hwmon->hwmon_list) {
+	ixgbe_hwmon = devm_kzalloc(&adapter->pdev->dev, sizeof(*ixgbe_hwmon),
+				   GFP_KERNEL);
+	if (ixgbe_hwmon == NULL) {
 		rc = -ENOMEM;
-		goto err;
-	}
-
-	ixgbe_hwmon->device = hwmon_device_register(&adapter->pdev->dev);
-	if (IS_ERR(ixgbe_hwmon->device)) {
-		rc = PTR_ERR(ixgbe_hwmon->device);
-		goto err;
+		goto exit;
 	}
+	adapter->ixgbe_hwmon_buff = ixgbe_hwmon;
 
 	for (i = 0; i < IXGBE_MAX_SENSORS; i++) {
 		/*
@@ -226,17 +201,28 @@ int ixgbe_sysfs_init(struct ixgbe_adapter *adapter)
 
 		/* Bail if any hwmon attr struct fails to initialize */
 		rc = ixgbe_add_hwmon_attr(adapter, i, IXGBE_HWMON_TYPE_CAUTION);
-		rc |= ixgbe_add_hwmon_attr(adapter, i, IXGBE_HWMON_TYPE_LOC);
-		rc |= ixgbe_add_hwmon_attr(adapter, i, IXGBE_HWMON_TYPE_TEMP);
-		rc |= ixgbe_add_hwmon_attr(adapter, i, IXGBE_HWMON_TYPE_MAX);
 		if (rc)
-			goto err;
+			goto exit;
+		rc = ixgbe_add_hwmon_attr(adapter, i, IXGBE_HWMON_TYPE_LOC);
+		if (rc)
+			goto exit;
+		rc = ixgbe_add_hwmon_attr(adapter, i, IXGBE_HWMON_TYPE_TEMP);
+		if (rc)
+			goto exit;
+		rc = ixgbe_add_hwmon_attr(adapter, i, IXGBE_HWMON_TYPE_MAX);
+		if (rc)
+			goto exit;
 	}
 
-	goto exit;
+	ixgbe_hwmon->groups[0] = &ixgbe_hwmon->group;
+	ixgbe_hwmon->group.attrs = ixgbe_hwmon->attrs;
 
-err:
-	ixgbe_sysfs_del_adapter(adapter);
+	hwmon_dev = devm_hwmon_device_register_with_groups(&adapter->pdev->dev,
+							   "ixgbe",
+							   ixgbe_hwmon,
+							   ixgbe_hwmon->groups);
+	if (IS_ERR(hwmon_dev))
+		rc = PTR_ERR(hwmon_dev);
 exit:
 	return rc;
 }
-- 
1.7.9.7

Re: [PATCH 3/5] ixgbe: Convert to use devm_hwmon_device_register_with_groups

[Jeff Kirsher]

[-- Attachment #1: Type: text/plain, Size: 965 bytes --]

On Fri, 2013-11-22 at 22:07 -0800, Guenter Roeck wrote:
> Simplify the code. Attach hwmon sysfs attributes to hwmon device
> instead of pci device. Avoid race conditions caused by attributes
> being created after hwmon device registration. Implicitly
> (through hwmon API) add mandatory 'name' sysfs attribute.
> 
> Other cleanup:
> 
> Instead of allocating memory for hwmon attributes, move attributes
> and all other hwmon related data into struct hwmon_buff and allocate
> the entire structure using devm_kzalloc.
> 
> Check return value from calls to igb_add_hwmon_attr() one by one
> instead
> of logically combining them all together.
> 
> Signed-off-by: Guenter Roeck <linux@roeck-us.net>
> ---
>  drivers/net/ethernet/intel/ixgbe/ixgbe.h       |    8 ++-
>  drivers/net/ethernet/intel/ixgbe/ixgbe_sysfs.c |   76
> ++++++++++--------------
>  2 files changed, 36 insertions(+), 48 deletions(-)

I have added this to my queue, thanks!

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

[PATCH 4/5] igb: Start temperature sensor attribute index with 1

[Guenter Roeck]

Per hwmon ABI, temperature sensor attribute index starts with 1, not 0.

Signed-off-by: Guenter Roeck <linux@roeck-us.net>
---
 drivers/net/ethernet/intel/igb/igb_hwmon.c |    8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/net/ethernet/intel/igb/igb_hwmon.c b/drivers/net/ethernet/intel/igb/igb_hwmon.c
index 2e7ef2d..e0af5bc 100644
--- a/drivers/net/ethernet/intel/igb/igb_hwmon.c
+++ b/drivers/net/ethernet/intel/igb/igb_hwmon.c
@@ -124,22 +124,22 @@ static int igb_add_hwmon_attr(struct igb_adapter *adapter,
 	case IGB_HWMON_TYPE_LOC:
 		igb_attr->dev_attr.show = igb_hwmon_show_location;
 		snprintf(igb_attr->name, sizeof(igb_attr->name),
-			 "temp%u_label", offset);
+			 "temp%u_label", offset + 1);
 		break;
 	case IGB_HWMON_TYPE_TEMP:
 		igb_attr->dev_attr.show = igb_hwmon_show_temp;
 		snprintf(igb_attr->name, sizeof(igb_attr->name),
-			 "temp%u_input", offset);
+			 "temp%u_input", offset + 1);
 		break;
 	case IGB_HWMON_TYPE_CAUTION:
 		igb_attr->dev_attr.show = igb_hwmon_show_cautionthresh;
 		snprintf(igb_attr->name, sizeof(igb_attr->name),
-			 "temp%u_max", offset);
+			 "temp%u_max", offset + 1);
 		break;
 	case IGB_HWMON_TYPE_MAX:
 		igb_attr->dev_attr.show = igb_hwmon_show_maxopthresh;
 		snprintf(igb_attr->name, sizeof(igb_attr->name),
-			 "temp%u_crit", offset);
+			 "temp%u_crit", offset + 1);
 		break;
 	default:
 		rc = -EPERM;
-- 
1.7.9.7


------------------------------------------------------------------------------
Shape the Mobile Experience: Free Subscription
Software experts and developers: Be at the forefront of tech innovation.
Intel(R) Software Adrenaline delivers strategic insight and game-changing 
conversations that shape the rapidly evolving mobile landscape. Sign up now. 
http://pubads.g.doubleclick.net/gampad/clk?id=63431311&iu=/4140/ostg.clktrk
_______________________________________________
E1000-devel mailing list
E1000-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/e1000-devel
To learn more about Intel&#174; Ethernet, visit http://communities.intel.com/community/wired

Re: [lm-sensors] [PATCH 4/5] igb: Start temperature sensor attribute index with 1

[Jean Delvare]

On Fri, 22 Nov 2013 22:08:00 -0800, Guenter Roeck wrote:
> Per hwmon ABI, temperature sensor attribute index starts with 1, not 0.
> 
> Signed-off-by: Guenter Roeck <linux@roeck-us.net>
> ---
>  drivers/net/ethernet/intel/igb/igb_hwmon.c |    8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/net/ethernet/intel/igb/igb_hwmon.c b/drivers/net/ethernet/intel/igb/igb_hwmon.c
> index 2e7ef2d..e0af5bc 100644
> --- a/drivers/net/ethernet/intel/igb/igb_hwmon.c
> +++ b/drivers/net/ethernet/intel/igb/igb_hwmon.c
> @@ -124,22 +124,22 @@ static int igb_add_hwmon_attr(struct igb_adapter *adapter,
>  	case IGB_HWMON_TYPE_LOC:
>  		igb_attr->dev_attr.show = igb_hwmon_show_location;
>  		snprintf(igb_attr->name, sizeof(igb_attr->name),
> -			 "temp%u_label", offset);
> +			 "temp%u_label", offset + 1);
>  		break;
>  	case IGB_HWMON_TYPE_TEMP:
>  		igb_attr->dev_attr.show = igb_hwmon_show_temp;
>  		snprintf(igb_attr->name, sizeof(igb_attr->name),
> -			 "temp%u_input", offset);
> +			 "temp%u_input", offset + 1);
>  		break;
>  	case IGB_HWMON_TYPE_CAUTION:
>  		igb_attr->dev_attr.show = igb_hwmon_show_cautionthresh;
>  		snprintf(igb_attr->name, sizeof(igb_attr->name),
> -			 "temp%u_max", offset);
> +			 "temp%u_max", offset + 1);
>  		break;
>  	case IGB_HWMON_TYPE_MAX:
>  		igb_attr->dev_attr.show = igb_hwmon_show_maxopthresh;
>  		snprintf(igb_attr->name, sizeof(igb_attr->name),
> -			 "temp%u_crit", offset);
> +			 "temp%u_crit", offset + 1);
>  		break;
>  	default:
>  		rc = -EPERM;

Reviewed-by: Jean Delvare <khali@linux-fr.org>

-- 
Jean Delvare

Re: [PATCH 4/5] igb: Start temperature sensor attribute index with 1

[Jeff Kirsher]

[-- Attachment #1: Type: text/plain, Size: 364 bytes --]

On Fri, 2013-11-22 at 22:08 -0800, Guenter Roeck wrote:
> Per hwmon ABI, temperature sensor attribute index starts with 1, not
> 0.
> 
> Signed-off-by: Guenter Roeck <linux@roeck-us.net>
> ---
>  drivers/net/ethernet/intel/igb/igb_hwmon.c |    8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)

I have also added this to my queue, thank you.

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

[PATCH 5/5] ixgbe: Start temperature sensor attribute index with 1

[Guenter Roeck]

Per hwmon ABI, temperature sensor attribute index starts with 1, not 0.

Signed-off-by: Guenter Roeck <linux@roeck-us.net>
---
 drivers/net/ethernet/intel/ixgbe/ixgbe_sysfs.c |    8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_sysfs.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_sysfs.c
index 3081974..e74ae36 100644
--- a/drivers/net/ethernet/intel/ixgbe/ixgbe_sysfs.c
+++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_sysfs.c
@@ -118,22 +118,22 @@ static int ixgbe_add_hwmon_attr(struct ixgbe_adapter *adapter,
 	case IXGBE_HWMON_TYPE_LOC:
 		ixgbe_attr->dev_attr.show = ixgbe_hwmon_show_location;
 		snprintf(ixgbe_attr->name, sizeof(ixgbe_attr->name),
-			 "temp%u_label", offset);
+			 "temp%u_label", offset + 1);
 		break;
 	case IXGBE_HWMON_TYPE_TEMP:
 		ixgbe_attr->dev_attr.show = ixgbe_hwmon_show_temp;
 		snprintf(ixgbe_attr->name, sizeof(ixgbe_attr->name),
-			 "temp%u_input", offset);
+			 "temp%u_input", offset + 1);
 		break;
 	case IXGBE_HWMON_TYPE_CAUTION:
 		ixgbe_attr->dev_attr.show = ixgbe_hwmon_show_cautionthresh;
 		snprintf(ixgbe_attr->name, sizeof(ixgbe_attr->name),
-			 "temp%u_max", offset);
+			 "temp%u_max", offset + 1);
 		break;
 	case IXGBE_HWMON_TYPE_MAX:
 		ixgbe_attr->dev_attr.show = ixgbe_hwmon_show_maxopthresh;
 		snprintf(ixgbe_attr->name, sizeof(ixgbe_attr->name),
-			 "temp%u_crit", offset);
+			 "temp%u_crit", offset + 1);
 		break;
 	default:
 		rc = -EPERM;
-- 
1.7.9.7

Re: [lm-sensors] [PATCH 5/5] ixgbe: Start temperature sensor attribute index with 1

[Jean Delvare]

On Fri, 22 Nov 2013 22:08:01 -0800, Guenter Roeck wrote:
> Per hwmon ABI, temperature sensor attribute index starts with 1, not 0.
> 
> Signed-off-by: Guenter Roeck <linux@roeck-us.net>
> ---
>  drivers/net/ethernet/intel/ixgbe/ixgbe_sysfs.c |    8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_sysfs.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_sysfs.c
> index 3081974..e74ae36 100644
> --- a/drivers/net/ethernet/intel/ixgbe/ixgbe_sysfs.c
> +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_sysfs.c
> @@ -118,22 +118,22 @@ static int ixgbe_add_hwmon_attr(struct ixgbe_adapter *adapter,
>  	case IXGBE_HWMON_TYPE_LOC:
>  		ixgbe_attr->dev_attr.show = ixgbe_hwmon_show_location;
>  		snprintf(ixgbe_attr->name, sizeof(ixgbe_attr->name),
> -			 "temp%u_label", offset);
> +			 "temp%u_label", offset + 1);
>  		break;
>  	case IXGBE_HWMON_TYPE_TEMP:
>  		ixgbe_attr->dev_attr.show = ixgbe_hwmon_show_temp;
>  		snprintf(ixgbe_attr->name, sizeof(ixgbe_attr->name),
> -			 "temp%u_input", offset);
> +			 "temp%u_input", offset + 1);
>  		break;
>  	case IXGBE_HWMON_TYPE_CAUTION:
>  		ixgbe_attr->dev_attr.show = ixgbe_hwmon_show_cautionthresh;
>  		snprintf(ixgbe_attr->name, sizeof(ixgbe_attr->name),
> -			 "temp%u_max", offset);
> +			 "temp%u_max", offset + 1);
>  		break;
>  	case IXGBE_HWMON_TYPE_MAX:
>  		ixgbe_attr->dev_attr.show = ixgbe_hwmon_show_maxopthresh;
>  		snprintf(ixgbe_attr->name, sizeof(ixgbe_attr->name),
> -			 "temp%u_crit", offset);
> +			 "temp%u_crit", offset + 1);
>  		break;
>  	default:
>  		rc = -EPERM;

Reviewed-by: Jean Delvare <khali@linux-fr.org>

-- 
Jean Delvare

Re: [lm-sensors] [PATCH 5/5] ixgbe: Start temperature sensor attribute index with 1

[Guenter Roeck]

On Sat, Nov 23, 2013 at 02:01:58PM +0100, Jean Delvare wrote:
> On Fri, 22 Nov 2013 22:08:01 -0800, Guenter Roeck wrote:
> > Per hwmon ABI, temperature sensor attribute index starts with 1, not 0.
> > 
> > Signed-off-by: Guenter Roeck <linux@roeck-us.net>
> > ---
> >  drivers/net/ethernet/intel/ixgbe/ixgbe_sysfs.c |    8 ++++----
> >  1 file changed, 4 insertions(+), 4 deletions(-)
> > 
> > diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_sysfs.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_sysfs.c
> > index 3081974..e74ae36 100644
> > --- a/drivers/net/ethernet/intel/ixgbe/ixgbe_sysfs.c
> > +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_sysfs.c
> > @@ -118,22 +118,22 @@ static int ixgbe_add_hwmon_attr(struct ixgbe_adapter *adapter,
> >  	case IXGBE_HWMON_TYPE_LOC:
> >  		ixgbe_attr->dev_attr.show = ixgbe_hwmon_show_location;
> >  		snprintf(ixgbe_attr->name, sizeof(ixgbe_attr->name),
> > -			 "temp%u_label", offset);
> > +			 "temp%u_label", offset + 1);
> >  		break;
> >  	case IXGBE_HWMON_TYPE_TEMP:
> >  		ixgbe_attr->dev_attr.show = ixgbe_hwmon_show_temp;
> >  		snprintf(ixgbe_attr->name, sizeof(ixgbe_attr->name),
> > -			 "temp%u_input", offset);
> > +			 "temp%u_input", offset + 1);
> >  		break;
> >  	case IXGBE_HWMON_TYPE_CAUTION:
> >  		ixgbe_attr->dev_attr.show = ixgbe_hwmon_show_cautionthresh;
> >  		snprintf(ixgbe_attr->name, sizeof(ixgbe_attr->name),
> > -			 "temp%u_max", offset);
> > +			 "temp%u_max", offset + 1);
> >  		break;
> >  	case IXGBE_HWMON_TYPE_MAX:
> >  		ixgbe_attr->dev_attr.show = ixgbe_hwmon_show_maxopthresh;
> >  		snprintf(ixgbe_attr->name, sizeof(ixgbe_attr->name),
> > -			 "temp%u_crit", offset);
> > +			 "temp%u_crit", offset + 1);
> >  		break;
> >  	default:
> >  		rc = -EPERM;
> 
> Reviewed-by: Jean Delvare <khali@linux-fr.org>
> 
Hi Jean,

thanks a lot for the reviews!

Guenter

Re: [PATCH 5/5] ixgbe: Start temperature sensor attribute index with 1

[Jeff Kirsher]

[-- Attachment #1: Type: text/plain, Size: 368 bytes --]

On Fri, 2013-11-22 at 22:08 -0800, Guenter Roeck wrote:
> Per hwmon ABI, temperature sensor attribute index starts with 1, not
> 0.
> 
> Signed-off-by: Guenter Roeck <linux@roeck-us.net>
> ---
>  drivers/net/ethernet/intel/ixgbe/ixgbe_sysfs.c |    8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)

I have also added this to my queue, thank you.

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH 0/5] net: hwmon fixes

[Ben Hutchings]

On Fri, 2013-11-22 at 22:07 -0800, Guenter Roeck wrote:
> The hwmon subsystem is used by various network drivers to report temperature
> sensor and other information. Unfortunately, its use is often not correct.
> Typical errors are that the mandatory name sysfs attribute is not created,
> that the temperature sensor index starts with 0 instead of 1, and/or that
> sysfs attributes are created after the hwmon device was created.

As it happens, I was just looking at what we do in sfc
(drivers/net/ethernet/sfc/mcdi_mon.c) and wondering why I made it create
the hwmon device before the attributes.  I think I avoided the other
bugs though.

Ben.

> The following sequence of patches fixes most of the problems.
> 
> The igb patches have been tested with real hardware; the others are compile
> tested only.

-- 
Ben Hutchings, Staff Engineer, Solarflare
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.

Re: [PATCH 0/5] net: hwmon fixes

[Guenter Roeck]

On 11/23/2013 08:48 AM, Ben Hutchings wrote:
> On Fri, 2013-11-22 at 22:07 -0800, Guenter Roeck wrote:
>> The hwmon subsystem is used by various network drivers to report temperature
>> sensor and other information. Unfortunately, its use is often not correct.
>> Typical errors are that the mandatory name sysfs attribute is not created,
>> that the temperature sensor index starts with 0 instead of 1, and/or that
>> sysfs attributes are created after the hwmon device was created.
>
> As it happens, I was just looking at what we do in sfc
> (drivers/net/ethernet/sfc/mcdi_mon.c) and wondering why I made it create
> the hwmon device before the attributes.  I think I avoided the other
> bugs though.
>
Hi Ben,

Yes, I know about that one. It concluded that it would be too invasive
and risky to try to fix it without access to hardware to test the results.
That is why I said "fixes _most_ of the problems".

As for why the attributes are created after registration, it was most likely
because there was no API available to attach the sysfs attributes to
the hwmon device in a clean way. The new APIs fix that.

Thanks,
Guenter

> Ben.
>
>> The following sequence of patches fixes most of the problems.
>>
>> The igb patches have been tested with real hardware; the others are compile
>> tested only.
>

Re: [PATCH 0/5] net: hwmon fixes

[Ben Hutchings]

On Sat, 2013-11-23 at 09:07 -0800, Guenter Roeck wrote:
> On 11/23/2013 08:48 AM, Ben Hutchings wrote:
> > On Fri, 2013-11-22 at 22:07 -0800, Guenter Roeck wrote:
> >> The hwmon subsystem is used by various network drivers to report temperature
> >> sensor and other information. Unfortunately, its use is often not correct.
> >> Typical errors are that the mandatory name sysfs attribute is not created,
> >> that the temperature sensor index starts with 0 instead of 1, and/or that
> >> sysfs attributes are created after the hwmon device was created.
> >
> > As it happens, I was just looking at what we do in sfc
> > (drivers/net/ethernet/sfc/mcdi_mon.c) and wondering why I made it create
> > the hwmon device before the attributes.  I think I avoided the other
> > bugs though.
> >
> Hi Ben,
> 
> Yes, I know about that one. It concluded that it would be too invasive
> and risky to try to fix it without access to hardware to test the results.
> That is why I said "fixes _most_ of the problems".
> 
> As for why the attributes are created after registration, it was most likely
> because there was no API available to attach the sysfs attributes to
> the hwmon device in a clean way. The new APIs fix that.

We don't attach them to the hwmon device either, and I would rather not
change that yet because lm-sensors 2 is still widely used.

Ben.

-- 
Ben Hutchings, Staff Engineer, Solarflare
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.

Re: [PATCH 0/5] net: hwmon fixes

[Guenter Roeck]

On 11/25/2013 09:15 AM, Ben Hutchings wrote:
> On Sat, 2013-11-23 at 09:07 -0800, Guenter Roeck wrote:
>> On 11/23/2013 08:48 AM, Ben Hutchings wrote:
>>> On Fri, 2013-11-22 at 22:07 -0800, Guenter Roeck wrote:
>>>> The hwmon subsystem is used by various network drivers to report temperature
>>>> sensor and other information. Unfortunately, its use is often not correct.
>>>> Typical errors are that the mandatory name sysfs attribute is not created,
>>>> that the temperature sensor index starts with 0 instead of 1, and/or that
>>>> sysfs attributes are created after the hwmon device was created.
>>>
>>> As it happens, I was just looking at what we do in sfc
>>> (drivers/net/ethernet/sfc/mcdi_mon.c) and wondering why I made it create
>>> the hwmon device before the attributes.  I think I avoided the other
>>> bugs though.
>>>
>> Hi Ben,
>>
>> Yes, I know about that one. It concluded that it would be too invasive
>> and risky to try to fix it without access to hardware to test the results.
>> That is why I said "fixes _most_ of the problems".
>>
>> As for why the attributes are created after registration, it was most likely
>> because there was no API available to attach the sysfs attributes to
>> the hwmon device in a clean way. The new APIs fix that.
>
> We don't attach them to the hwmon device either, and I would rather not
> change that yet because lm-sensors 2 is still widely used.
>

Hmm .. then there should be no good reason to create the attributes
only after hwmon registration.

As for lm-sensors 2 ... really ? Seems odd that people would use the
latest kernel with 5+ years old versions of applications / libraries.
but I guess the world is full of such oddities, so maybe I should not
be surprised.

Note that the drivers in the hwmon subsystem are expected to move
to the new API over time, and some of them already did. New drivers
will likely not work with lm-sensors 2. So it may be time for those
still using lm-sensors 2 to consider an update, at least if they plan
to use those drivers with the latest kernel.

Thanks,
Guenter

Re: [PATCH 0/5] net: hwmon fixes

[Ben Hutchings]

On Mon, 2013-11-25 at 09:48 -0800, Guenter Roeck wrote:
> On 11/25/2013 09:15 AM, Ben Hutchings wrote:
> > On Sat, 2013-11-23 at 09:07 -0800, Guenter Roeck wrote:
> >> On 11/23/2013 08:48 AM, Ben Hutchings wrote:
> >>> On Fri, 2013-11-22 at 22:07 -0800, Guenter Roeck wrote:
> >>>> The hwmon subsystem is used by various network drivers to report temperature
> >>>> sensor and other information. Unfortunately, its use is often not correct.
> >>>> Typical errors are that the mandatory name sysfs attribute is not created,
> >>>> that the temperature sensor index starts with 0 instead of 1, and/or that
> >>>> sysfs attributes are created after the hwmon device was created.
> >>>
> >>> As it happens, I was just looking at what we do in sfc
> >>> (drivers/net/ethernet/sfc/mcdi_mon.c) and wondering why I made it create
> >>> the hwmon device before the attributes.  I think I avoided the other
> >>> bugs though.
> >>>
> >> Hi Ben,
> >>
> >> Yes, I know about that one. It concluded that it would be too invasive
> >> and risky to try to fix it without access to hardware to test the results.
> >> That is why I said "fixes _most_ of the problems".
> >>
> >> As for why the attributes are created after registration, it was most likely
> >> because there was no API available to attach the sysfs attributes to
> >> the hwmon device in a clean way. The new APIs fix that.
> >
> > We don't attach them to the hwmon device either, and I would rather not
> > change that yet because lm-sensors 2 is still widely used.
> >
> 
> Hmm .. then there should be no good reason to create the attributes
> only after hwmon registration.
>
> As for lm-sensors 2 ... really ? Seems odd that people would use the
> latest kernel with 5+ years old versions of applications / libraries.
> but I guess the world is full of such oddities, so maybe I should not
> be surprised.
[...]

As Jean pointed out, the net drivers implementing hwmon aren't supported
by lm-sensors 2 anyway.  So we should go ahead and use the new API in
sfc.  I've opened an internal bug report for this, but it is likely to
be low priority for the team.  But if you write a patch I can test it.

Ben.

-- 
Ben Hutchings, Staff Engineer, Solarflare
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.


------------------------------------------------------------------------------
Rapidly troubleshoot problems before they affect your business. Most IT 
organizations don't have a clear picture of how application performance 
affects their revenue. With AppDynamics, you get 100% visibility into your 
Java,.NET, & PHP application. Start your 15-day FREE TRIAL of AppDynamics Pro!
http://pubads.g.doubleclick.net/gampad/clk?id=84349351&iu=/4140/ostg.clktrk
_______________________________________________
E1000-devel mailing list
E1000-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/e1000-devel
To learn more about Intel® Ethernet, visit http://communities.intel.com/community/wired

Re: [PATCH 0/5] net: hwmon fixes

[Guenter Roeck]

On 11/26/2013 12:08 PM, Ben Hutchings wrote:
> On Mon, 2013-11-25 at 09:48 -0800, Guenter Roeck wrote:
>> On 11/25/2013 09:15 AM, Ben Hutchings wrote:
>>> On Sat, 2013-11-23 at 09:07 -0800, Guenter Roeck wrote:
>>>> On 11/23/2013 08:48 AM, Ben Hutchings wrote:
>>>>> On Fri, 2013-11-22 at 22:07 -0800, Guenter Roeck wrote:
>>>>>> The hwmon subsystem is used by various network drivers to report temperature
>>>>>> sensor and other information. Unfortunately, its use is often not correct.
>>>>>> Typical errors are that the mandatory name sysfs attribute is not created,
>>>>>> that the temperature sensor index starts with 0 instead of 1, and/or that
>>>>>> sysfs attributes are created after the hwmon device was created.
>>>>>
>>>>> As it happens, I was just looking at what we do in sfc
>>>>> (drivers/net/ethernet/sfc/mcdi_mon.c) and wondering why I made it create
>>>>> the hwmon device before the attributes.  I think I avoided the other
>>>>> bugs though.
>>>>>
>>>> Hi Ben,
>>>>
>>>> Yes, I know about that one. It concluded that it would be too invasive
>>>> and risky to try to fix it without access to hardware to test the results.
>>>> That is why I said "fixes _most_ of the problems".
>>>>
>>>> As for why the attributes are created after registration, it was most likely
>>>> because there was no API available to attach the sysfs attributes to
>>>> the hwmon device in a clean way. The new APIs fix that.
>>>
>>> We don't attach them to the hwmon device either, and I would rather not
>>> change that yet because lm-sensors 2 is still widely used.
>>>
>>
>> Hmm .. then there should be no good reason to create the attributes
>> only after hwmon registration.
>>
>> As for lm-sensors 2 ... really ? Seems odd that people would use the
>> latest kernel with 5+ years old versions of applications / libraries.
>> but I guess the world is full of such oddities, so maybe I should not
>> be surprised.
> [...]
>
> As Jean pointed out, the net drivers implementing hwmon aren't supported
> by lm-sensors 2 anyway.  So we should go ahead and use the new API in
> sfc.  I've opened an internal bug report for this, but it is likely to
> be low priority for the team.  But if you write a patch I can test it.
>

You'll get one shortly.

Thanks,
Guenter

Re: [lm-sensors] [PATCH 0/5] net: hwmon fixes

[Jean Delvare]

On Mon, 25 Nov 2013 17:15:50 +0000, Ben Hutchings wrote:
> On Sat, 2013-11-23 at 09:07 -0800, Guenter Roeck wrote:
> > Yes, I know about that one. It concluded that it would be too invasive
> > and risky to try to fix it without access to hardware to test the results.
> > That is why I said "fixes _most_ of the problems".
> > 
> > As for why the attributes are created after registration, it was most likely
> > because there was no API available to attach the sysfs attributes to
> > the hwmon device in a clean way. The new APIs fix that.
> 
> We don't attach them to the hwmon device either, and I would rather not
> change that yet because lm-sensors 2 is still widely used.

Mouahahahah.

No, seriously, it's not. And lm-sensors 2 doesn't even support your
device so this is a totally moot point.

-- 
Jean Delvare

Re: [lm-sensors] [PATCH 0/5] net: hwmon fixes

[Ben Hutchings]

On Mon, 2013-11-25 at 19:23 +0100, Jean Delvare wrote:
> On Mon, 25 Nov 2013 17:15:50 +0000, Ben Hutchings wrote:
> > On Sat, 2013-11-23 at 09:07 -0800, Guenter Roeck wrote:
> > > Yes, I know about that one. It concluded that it would be too invasive
> > > and risky to try to fix it without access to hardware to test the results.
> > > That is why I said "fixes _most_ of the problems".
> > > 
> > > As for why the attributes are created after registration, it was most likely
> > > because there was no API available to attach the sysfs attributes to
> > > the hwmon device in a clean way. The new APIs fix that.
> > 
> > We don't attach them to the hwmon device either, and I would rather not
> > change that yet because lm-sensors 2 is still widely used.
> 
> Mouahahahah.
> 
> No, seriously, it's not.

RHEL 5 has it, and that is widely used - even with recent mainline
kernels, in some cases.

> And lm-sensors 2 doesn't even support your
> device so this is a totally moot point.

I thought it did work with arbitrary devices providing the right
attributes, but obviously I misremembered.  So there's no reason not to
change.  Thanks.

Ben.

-- 
Ben Hutchings, Staff Engineer, Solarflare
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.

Re: [lm-sensors] [PATCH 0/5] net: hwmon fixes

[Jean Delvare]

On Mon, 25 Nov 2013 18:56:26 +0000, Ben Hutchings wrote:
> On Mon, 2013-11-25 at 19:23 +0100, Jean Delvare wrote:
> > On Mon, 25 Nov 2013 17:15:50 +0000, Ben Hutchings wrote:
> > > We don't attach them to the hwmon device either, and I would rather not
> > > change that yet because lm-sensors 2 is still widely used.
> > 
> > Mouahahahah.
> > 
> > No, seriously, it's not.
> 
> RHEL 5 has it, and that is widely used - even with recent mainline
> kernels, in some cases.

RHEL 5 comes with kernel 2.6.18, which isn't exactly recent. I very
much doubt a significant share of users dare to use a brand new kernel
on such an old distribution. And if they do, then there are several
packages which need to be updated (udev, kernel-firmware...),
lm-sensors is only one of them, and the user should be aware of that.

> > And lm-sensors 2 doesn't even support your
> > device so this is a totally moot point.
> 
> I thought it did work with arbitrary devices providing the right
> attributes, but obviously I misremembered.

This is how lm-sensors 3 works. But lm-sensors 2 needs explicit support
for each and every device. Which is exactly why version 2 sucked and
nobody should be using it any longer.

> So there's no reason not to change.  Thanks.

Good to hear :)

-- 
Jean Delvare