Skip to contentSkip to conversation

drivers: hwmon: occ: sysfs_notify and presence detect

PATCH Superseded Eddie James 2 participants 9 messages Oct 6, 2017 – Oct 16, 2017 Hardware monitoring

v1 of 2 Superseded Latest is v2 →

lore
Share & tools
lore

Eddie James

author
#0 lore raw

cover drivers: hwmon: occ: sysfs_notify and presence detect

From: "Edward A. James" <[email protected]>

This patch set removes the error checking for the number of OCCs present. Instead, export a new sysfs attribute to indicate the number of OCCs present from the master poll response. In addition, add sysfs_notify both to the new occs_present attribute and the throttling attributes.

Edward A. James (2):
drivers: hwmon: occ: Add occs_present attribute
drivers: hwmon: occ: Add sysfs_notify to throttling attributes

 drivers/hwmon/occ/common.c | 49 ++++++++++++++++++++++++++++++++++------------
 drivers/hwmon/occ/common.h |  3 ---
 drivers/hwmon/occ/p8_i2c.c |  4 ----
 drivers/hwmon/occ/p9_sbe.c |  4 ----
 4 files changed, 37 insertions(+), 23 deletions(-)
Signature · 1 line
1.8.3.1
author
#1 lore raw

PATCH 1/2 drivers: hwmon: occ: Add occs_present attribute

From: "Edward A. James" <[email protected]>

Remove the error on detecting occs_present mismatch. Add a new sysfs entry for occs_present field. Add sysfs_notify for this field, if it changes.

Signed-off-by Edward A. James <[email protected]>
---
 drivers/hwmon/occ/common.c | 32 ++++++++++++++++++++------------
 drivers/hwmon/occ/common.h |  3 ---
 drivers/hwmon/occ/p8_i2c.c |  4 ----
 drivers/hwmon/occ/p9_sbe.c |  4 ----
 4 files changed, 20 insertions(+), 23 deletions(-)
drivers/hwmon/occ/common.c +20 -12
index 34002fb..e8579b6 100644
@@ -10,7 +10,7 @@
 #include <asm/unaligned.h>
 #include "common.h"
 
-#define OCC_NUM_STATUS_ATTRS		8
+#define OCC_NUM_STATUS_ATTRS		9
 
 #define OCC_STAT_MASTER			0x80
 #define OCC_STAT_ACTIVE			0x01

@@ -19,8 +19,6 @@
 #define OCC_EXT_STAT_MEM_THROTTLE	0x20
 #define OCC_EXT_STAT_QUICK_DROP		0x10
 
-atomic_t occ_num_occs = ATOMIC_INIT(0);
-
 struct temp_sensor_1 {
 	u16 sensor_id;
 	u16 value;

@@ -179,9 +177,11 @@ void occ_reset_error(struct occ *occ)
 int occ_poll(struct occ *occ)
 {
 	int rc, error = occ->error;
-	struct occ_poll_response_header *header;
+	struct occ_poll_response_header *header =
+		(struct occ_poll_response_header *)occ->resp.data;
 	u16 checksum = occ->poll_cmd_data + 1;
 	u8 cmd[8];
+	u8 occs_present = header->occs_present;
 
 	cmd[0] = 0;
 	cmd[1] = 0;

@@ -196,8 +196,6 @@ int occ_poll(struct occ *occ)
 	if (rc)
 		goto done;
 
-	header = (struct occ_poll_response_header *)occ->resp.data;
-
 	if (header->occ_state == OCC_STATE_SAFE) {
 		if (occ->last_safe) {
 			if (time_after(jiffies,

@@ -208,10 +206,10 @@ int occ_poll(struct occ *occ)
 	} else
 		occ->last_safe = 0;
 
-	if (header->status & OCC_STAT_MASTER) {
-		if (hweight8(header->occs_present) !=
-		    atomic_read(&occ_num_occs))
-			occ->error = -ENXIO;
+	if (occs_present != header->occs_present && occ->hwmon &&
+	    (header->status & OCC_STAT_MASTER)) {
+		sysfs_notify(&occ->bus_dev->kobj, NULL,
+			     occ->status_attrs[7].dev_attr.attr.name);
 	}
 
 done:

@@ -321,6 +319,12 @@ static ssize_t occ_show_status(struct device *dev,
 	case 6:
 		val = header->occ_state;
 		break;
+	case 7:
+		if (header->status & OCC_STAT_MASTER)
+			val = hweight8(header->occs_present);
+		else
+			val = 1;
+		break;
 	default:
 		return -EINVAL;
 	}

@@ -1203,12 +1207,16 @@ int occ_create_status_attrs(struct occ *occ)
 		(struct sensor_device_attribute)SENSOR_ATTR(occ_status, 0444,
 							    occ_show_status,
 							    NULL, 6);
-
 	occ->status_attrs[7] =
+		(struct sensor_device_attribute)SENSOR_ATTR(occs_present, 0444,
+							    occ_show_status,
+							    NULL, 7);
+
+	occ->status_attrs[8] =
 		(struct sensor_device_attribute)SENSOR_ATTR(occ_error, 0444,
 							    occ_show_error,
 							    NULL, 0);
-	occ->error_attr_name = occ->status_attrs[7].dev_attr.attr.name;
+	occ->error_attr_name = occ->status_attrs[8].dev_attr.attr.name;
 
 	for (i = 0; i < OCC_NUM_STATUS_ATTRS; ++i) {
 		rc = device_create_file(dev, &occ->status_attrs[i].dev_attr);
drivers/hwmon/occ/common.h +0 -3
index acb50bc..6670a4b 100644
@@ -83,7 +83,6 @@ struct occ_sensor {
 	u8 version;
 	void *data;
 };
-
 struct occ_sensors {
 	struct occ_sensor temp;
 	struct occ_sensor freq;

@@ -140,8 +139,6 @@ struct occ {
 	((struct sensor_device_attribute_2)				\
 		SENSOR_ATTR_OCC(_name, _mode, _show, _store, _nr, _index))
 
-extern atomic_t occ_num_occs;
-
 void occ_parse_poll_response(struct occ *occ);
 void occ_reset_error(struct occ *occ);
 void occ_set_error(struct occ *occ, int error);
drivers/hwmon/occ/p8_i2c.c +0 -4
index e1c6234..020a53b 100644
@@ -248,8 +248,6 @@ static int p8_i2c_occ_probe(struct i2c_client *client,
 		return rc;
 	}
 
-	atomic_inc(&occ_num_occs);
-
 	return 0;
 }
 

@@ -259,8 +257,6 @@ static int p8_i2c_occ_remove(struct i2c_client *client)
 
 	occ_remove_status_attrs(occ);
 
-	atomic_dec(&occ_num_occs);
-
 	return 0;
 }
drivers/hwmon/occ/p9_sbe.c +0 -4
index 2d50a94..7c17070 100644
@@ -156,8 +156,6 @@ static int p9_sbe_occ_probe(struct platform_device *pdev)
 	if (rc)
 		return rc;
 
-	atomic_inc(&occ_num_occs);
-
 	return rc;
 }
 

@@ -167,8 +165,6 @@ static int p9_sbe_occ_remove(struct platform_device *pdev)
 
 	occ_remove_status_attrs(occ);
 
-	atomic_dec(&occ_num_occs);
-
 	return 0;
 }
Signature · 1 line
1.8.3.1
author
#2 lore raw

PATCH 2/2 drivers: hwmon: occ: Add sysfs_notify to throttling attributes

In reply to Eddie James

From: "Edward A. James" <[email protected]>

Need to alert user space when we change throttling state.

Signed-off-by Edward A. James <[email protected]>
---
 drivers/hwmon/occ/common.c | 25 +++++++++++++++++++++----
 1 file changed, 21 insertions(+), 4 deletions(-)
drivers/hwmon/occ/common.c +21 -4
index e8579b6..242ba40 100644
@@ -182,6 +182,7 @@ int occ_poll(struct occ *occ)
 	u16 checksum = occ->poll_cmd_data + 1;
 	u8 cmd[8];
 	u8 occs_present = header->occs_present;
+	u8 ext_status = header->ext_status;
 
 	cmd[0] = 0;
 	cmd[1] = 0;

@@ -206,10 +207,26 @@ int occ_poll(struct occ *occ)
 	} else
 		occ->last_safe = 0;
 
-	if (occs_present != header->occs_present && occ->hwmon &&
-	    (header->status & OCC_STAT_MASTER)) {
-		sysfs_notify(&occ->bus_dev->kobj, NULL,
-			     occ->status_attrs[7].dev_attr.attr.name);
+	if (occ->hwmon) {
+		if (occs_present != header->occs_present &&
+		    (header->status & OCC_STAT_MASTER))
+			sysfs_notify(&occ->bus_dev->kobj, NULL,
+				     occ->status_attrs[7].dev_attr.attr.name);
+
+		if ((header->ext_status & OCC_EXT_STAT_DVFS_OT) != 
+		    (ext_status & OCC_EXT_STAT_DVFS_OT))
+			sysfs_notify(&occ->bus_dev->kobj, NULL,
+				     occ->status_attrs[2].dev_attr.attr.name);
+
+		if ((header->ext_status & OCC_EXT_STAT_DVFS_POWER) != 
+		    (ext_status & OCC_EXT_STAT_DVFS_POWER))
+			sysfs_notify(&occ->bus_dev->kobj, NULL,
+				     occ->status_attrs[3].dev_attr.attr.name);
+
+		if ((header->ext_status & OCC_EXT_STAT_MEM_THROTTLE) != 
+		    (ext_status & OCC_EXT_STAT_MEM_THROTTLE))
+			sysfs_notify(&occ->bus_dev->kobj, NULL,
+				     occ->status_attrs[4].dev_attr.attr.name);
 	}
 
 done:
Signature · 1 line
1.8.3.1
#3 lore raw

Re: [PATCH linux dev-4.10 2/2] drivers: hwmon: occ: Add sysfs_notify to throttling attributes

Quoted 25 lines · Eddie James

From: "Edward A. James" <[email protected]>

Need to alert user space when we change throttling state.

Signed-off-by Edward A. James <[email protected]>

---
 drivers/hwmon/occ/common.c | 25 +++++++++++++++++++++----

 1 file changed, 21 insertions(+), 4 deletions(-)

drivers/hwmon/occ/common.c +1 -0
index e8579b6..242ba40 100644
@@ -182,6 +182,7 @@ int occ_poll(struct occ *occ)
  	u16 checksum = occ->poll_cmd_data + 1;
  	u8 cmd[8];
  	u8 occs_present = header->occs_present;
+	u8 ext_status = header->ext_status;


  cmd[0] = 0;
  cmd[1] = 0;

drivers/hwmon/occ/common.c +0 -0
@@ -206,10 +207,26 @@ int occ_poll(struct occ *occ)
  	} else
  		occ->last_safe = 0;

drivers/hwmon/occ/common.c int occ_poll(struct occ *occ) · line 214

Quoted 9 lines
drivers/hwmon/occ/common.c +5 -4
-	if (occs_present != header->occs_present && occ->hwmon &&
-	    (header->status & OCC_STAT_MASTER)) {
-		sysfs_notify(&occ->bus_dev->kobj, NULL,
-			     occ->status_attrs[7].dev_attr.attr.name);
+	if (occ->hwmon) {
+		if (occs_present != header->occs_present &&
+		    (header->status & OCC_STAT_MASTER))
+			sysfs_notify(&occ->bus_dev->kobj, NULL,
+				     occ->status_attrs[7].dev_attr.attr.name);

Why are we now protecting this under the occ->hwmon condition? Shouldn't we have done that in the previous patch which introduced the notify for occs_present?

Andrew

author
#4 lore raw

Re: [PATCH linux dev-4.10 2/2] drivers: hwmon: occ: Add sysfs_notify to throttling attributes

Quoted 26 lines · Andrew Jeffery
Quoted 25 lines · Eddie James

From: "Edward A. James" <[email protected]>

Need to alert user space when we change throttling state.

Signed-off-by Edward A. James <[email protected]>
---
  drivers/hwmon/occ/common.c | 25 +++++++++++++++++++++----
  1 file changed, 21 insertions(+), 4 deletions(-)
drivers/hwmon/occ/common.c +1 -0
index e8579b6..242ba40 100644
@@ -182,6 +182,7 @@ int occ_poll(struct occ *occ)
  	u16 checksum = occ->poll_cmd_data + 1;
  	u8 cmd[8];
  	u8 occs_present = header->occs_present;
+	u8 ext_status = header->ext_status;
 
  	cmd[0] = 0;
  	cmd[1] = 0;

@@ -206,10 +207,26 @@ int occ_poll(struct occ *occ)
  	} else
  		occ->last_safe = 0;
Quoted 12 lines

drivers/hwmon/occ/common.c int occ_poll(struct occ *occ) · line 214

Quoted 9 lines
drivers/hwmon/occ/common.c +5 -4
-	if (occs_present != header->occs_present && occ->hwmon &&
-	    (header->status & OCC_STAT_MASTER)) {
-		sysfs_notify(&occ->bus_dev->kobj, NULL,
-			     occ->status_attrs[7].dev_attr.attr.name);
+	if (occ->hwmon) {
+		if (occs_present != header->occs_present &&
+		    (header->status & OCC_STAT_MASTER))
+			sysfs_notify(&occ->bus_dev->kobj, NULL,
+				     occ->status_attrs[7].dev_attr.attr.name);

Why are we now protecting this under the occ->hwmon condition? Shouldn't we have done that in the previous patch which introduced the notify for occs_present?

Checking for occ->hwmon was there previously.

Thanks,
Eddie

Quoted 2 lines

Andrew

#5 lore raw

Re: [PATCH linux dev-4.10 2/2] drivers: hwmon: occ: Add sysfs_notify to throttling attributes

Quoted 41 lines · Eddie James

Quoted 31 lines · Andrew Jeffery
Quoted 30 lines · Eddie James
Quoted 1 line
Quoted 1 line
Quoted 1 line

From: "Edward A. James" <[email protected]>


Need to alert user space when we change throttling state.

Signed-off-by Edward A. James <[email protected]>

---
  drivers/hwmon/occ/common.c | 25 +++++++++++++++++++++----

  1 file changed, 21 insertions(+), 4 deletions(-)

drivers/hwmon/occ/common.c +1 -0
index e8579b6..242ba40 100644
@@ -182,6 +182,7 @@ int occ_poll(struct occ *occ)
   	u16 checksum = occ->poll_cmd_data + 1;
   	u8 cmd[8];
   	u8 occs_present = header->occs_present;
+	u8 ext_status = header->ext_status;


   cmd[0] = 0;
   cmd[1] = 0;

drivers/hwmon/occ/common.c +0 -0
@@ -206,10 +207,26 @@ int occ_poll(struct occ *occ)
   	} else
   		occ->last_safe = 0;

drivers/hwmon/occ/common.c +1 -4
-	if (occs_present != header->occs_present && occ->hwmon &&
-	    (header->status & OCC_STAT_MASTER)) {
-		sysfs_notify(&occ->bus_dev->kobj, NULL,
-			     occ->status_attrs[7].dev_attr.attr.name);
+	if (occ->hwmon) {
Quoted 8 lines

drivers/hwmon/occ/common.c int occ_poll(struct occ *occ) · line 214

Quoted 4 lines
drivers/hwmon/occ/common.c +4 -0
+		if (occs_present != header->occs_present &&
+		    (header->status & OCC_STAT_MASTER))
+			sysfs_notify(&occ->bus_dev->kobj, NULL,
+				     occ->status_attrs[7].dev_attr.attr.name);


Why are we now protecting this under the occ->hwmon condition? Shouldn't we have done that in the previous patch which introduced the notify for occs_present?

Quoted 2 lines


Checking for occ->hwmon was there previously.

Ugh, yep, glad that was the point I put the laptop away for the evening. It would be less jarring to introduce the if (occ-hwmon) in the first patch, but lets not let that hold us up.

Cheers,

Andrew

#6 lore raw
In reply to Eddie James
Quoted 3 lines · Eddie James
Quoted 1 line

From: "Edward A. James" <[email protected]>

This patch set removes the error checking for the number of OCCs present.

I'm interested in the back-story here. Why are we making this change in behaviour?

Cheers,

Andrew

Quoted 14 lines

Instead, export a new sysfs attribute to indicate the number of OCCs present from the master poll response. In addition, add sysfs_notify both to the new occs_present attribute and the throttling attributes.

Edward A. James (2):
  drivers: hwmon: occ: Add occs_present attribute
  drivers: hwmon: occ: Add sysfs_notify to throttling attributes

 drivers/hwmon/occ/common.c | 49 ++++++++++++++++++++++++++++++++++------------
 drivers/hwmon/occ/common.h |  3 ---
 drivers/hwmon/occ/p8_i2c.c |  4 ----
 drivers/hwmon/occ/p9_sbe.c |  4 ----
 4 files changed, 37 insertions(+), 23 deletions(-)
author
#7 lore raw
Quoted 5 lines · Andrew Jeffery
Quoted 2 lines · Eddie James
Quoted 1 line

From: "Edward A. James" <[email protected]>

This patch set removes the error checking for the number of OCCs present.

I'm interested in the back-story here. Why are we making this change in behaviour?

Sure, basically two reasons:

1) userspace app was having trouble handling this error code, since if you bind the master OCC device first, you'll immediately get an error that we haven't bound the same number of OCCs as the OCC reports are available.

2) This error checking for "number of OCC devices bound" isn't designed well anyway, and makes more sense in userspace. The original intent was to implement all of the OCC requirements in the driver, but we have an application running on top that can pick up the slack, as it were.

Thanks,
Eddie

Quoted 18 lines

Cheers,

Andrew

Quoted 13 lines

Instead, export a new sysfs attribute to indicate the number of OCCs present from the master poll response. In addition, add sysfs_notify both to the new occs_present attribute and the throttling attributes.

Edward A. James (2):
drivers: hwmon: occ: Add occs_present attribute
drivers: hwmon: occ: Add sysfs_notify to throttling attributes

  drivers/hwmon/occ/common.c | 49 ++++++++++++++++++++++++++++++++++------------
  drivers/hwmon/occ/common.h |  3 ---
  drivers/hwmon/occ/p8_i2c.c |  4 ----
  drivers/hwmon/occ/p9_sbe.c |  4 ----
  4 files changed, 37 insertions(+), 23 deletions(-)
#8 lore raw
In reply to Eddie James
Quoted 10 lines · Eddie James
Quoted 1 line

From: "Edward A. James" <[email protected]>

This patch set removes the error checking for the number of OCCs present. Instead, export a new sysfs attribute to indicate the number of OCCs present from the master poll response. In addition, add sysfs_notify both to the new occs_present attribute and the throttling attributes.

Edward A. James (2):
  drivers: hwmon: occ: Add occs_present attribute
  drivers: hwmon: occ: Add sysfs_notify to throttling attributes

Hi Eddie,

I just tried applying these to dev-4.10 and got a conflict. Can you please rebase and send a v2? Whilst you're doing so can you incorporate your reply expanding on why this was necessary into the commit messages?

Cheers,

Andrew