[PATCH 1/3] hwmon (occ): Store error condition for rate-limited polls

[PATCH 1/3] hwmon (occ): Store error condition for rate-limited polls

[Eddie James]

The OCC driver limits the rate of sending poll commands to the OCC. If a
user reads a hwmon entry after a poll response resulted in an error and
is rate-limited, the error is invisible to the user. Fix this by storing
the last error and returning that in the rate-limited case.

Signed-off-by: Eddie James <eajames@linux.ibm.com>
---
 drivers/hwmon/occ/common.c | 4 ++++
 drivers/hwmon/occ/common.h | 3 ++-
 2 files changed, 6 insertions(+), 1 deletion(-)

diff --git a/drivers/hwmon/occ/common.c b/drivers/hwmon/occ/common.c
index 9d197e9..13a6290 100644
--- a/drivers/hwmon/occ/common.c
+++ b/drivers/hwmon/occ/common.c
@@ -141,6 +141,7 @@ static int occ_poll(struct occ *occ)
 	/* mutex should already be locked if necessary */
 	rc = occ->send_cmd(occ, cmd);
 	if (rc) {
+		occ->last_error = rc;
 		if (occ->error_count++ > OCC_ERROR_COUNT_THRESHOLD)
 			occ->error = rc;
 
@@ -149,6 +150,7 @@ static int occ_poll(struct occ *occ)
 
 	/* clear error since communication was successful */
 	occ->error_count = 0;
+	occ->last_error = 0;
 	occ->error = 0;
 
 	/* check for safe state */
@@ -210,6 +212,8 @@ int occ_update_response(struct occ *occ)
 	if (time_after(jiffies, occ->last_update + OCC_UPDATE_FREQUENCY)) {
 		rc = occ_poll(occ);
 		occ->last_update = jiffies;
+	} else {
+		rc = occ->last_error;
 	}
 
 	mutex_unlock(&occ->lock);
diff --git a/drivers/hwmon/occ/common.h b/drivers/hwmon/occ/common.h
index ed2cf42..fc13f3c 100644
--- a/drivers/hwmon/occ/common.h
+++ b/drivers/hwmon/occ/common.h
@@ -106,7 +106,8 @@ struct occ {
 	struct attribute_group group;
 	const struct attribute_group *groups[2];
 
-	int error;                      /* latest transfer error */
+	int error;                      /* final transfer error after retry */
+	int last_error;			/* latest transfer error */
 	unsigned int error_count;       /* number of xfr errors observed */
 	unsigned long last_safe;        /* time OCC entered "safe" state */
 
-- 
2.7.4

[PATCH 2/3] hwmon (occ): Prevent sysfs error attribute from returning error

[Eddie James]

The error sysfs attribute returns the stored error state of the OCC and
doesn't depend on the OCC poll response. Therefore, split the error
attribute into it's own function to avoid failing out of the function if
the poll response fails.

Signed-off-by: Eddie James <eajames@linux.ibm.com>
---
 drivers/hwmon/occ/sysfs.c | 19 +++++++++++++------
 1 file changed, 13 insertions(+), 6 deletions(-)

diff --git a/drivers/hwmon/occ/sysfs.c b/drivers/hwmon/occ/sysfs.c
index 1cb1e65..c73be07 100644
--- a/drivers/hwmon/occ/sysfs.c
+++ b/drivers/hwmon/occ/sysfs.c
@@ -63,9 +63,6 @@ static ssize_t occ_sysfs_show(struct device *dev,
 		else
 			val = 1;
 		break;
-	case 8:
-		val = occ->error;
-		break;
 	default:
 		return -EINVAL;
 	}
@@ -73,6 +70,16 @@ static ssize_t occ_sysfs_show(struct device *dev,
 	return snprintf(buf, PAGE_SIZE - 1, "%d\n", val);
 }
 
+static ssize_t occ_error_show(struct device *dev,
+			      struct device_attribute *attr, char *buf)
+{
+	struct occ *occ = dev_get_drvdata(dev);
+
+	occ_update_response(occ);
+
+	return snprintf(buf, PAGE_SIZE - 1, "%d\n", occ->error);
+}
+
 static SENSOR_DEVICE_ATTR(occ_master, 0444, occ_sysfs_show, NULL, 0);
 static SENSOR_DEVICE_ATTR(occ_active, 0444, occ_sysfs_show, NULL, 1);
 static SENSOR_DEVICE_ATTR(occ_dvfs_overtemp, 0444, occ_sysfs_show, NULL, 2);
@@ -81,7 +88,7 @@ static SENSOR_DEVICE_ATTR(occ_mem_throttle, 0444, occ_sysfs_show, NULL, 4);
 static SENSOR_DEVICE_ATTR(occ_quick_pwr_drop, 0444, occ_sysfs_show, NULL, 5);
 static SENSOR_DEVICE_ATTR(occ_state, 0444, occ_sysfs_show, NULL, 6);
 static SENSOR_DEVICE_ATTR(occs_present, 0444, occ_sysfs_show, NULL, 7);
-static SENSOR_DEVICE_ATTR(occ_error, 0444, occ_sysfs_show, NULL, 8);
+static DEVICE_ATTR_RO(occ_error);
 
 static struct attribute *occ_attributes[] = {
 	&sensor_dev_attr_occ_master.dev_attr.attr,
@@ -92,7 +99,7 @@ static struct attribute *occ_attributes[] = {
 	&sensor_dev_attr_occ_quick_pwr_drop.dev_attr.attr,
 	&sensor_dev_attr_occ_state.dev_attr.attr,
 	&sensor_dev_attr_occs_present.dev_attr.attr,
-	&sensor_dev_attr_occ_error.dev_attr.attr,
+	&dev_attr_occ_error.attr,
 	NULL
 };
 
@@ -156,7 +163,7 @@ void occ_sysfs_poll_done(struct occ *occ)
 	}
 
 	if (occ->error && occ->error != occ->prev_error) {
-		name = sensor_dev_attr_occ_error.dev_attr.attr.name;
+		name = dev_attr_occ_error.attr.name;
 		sysfs_notify(&occ->bus_dev->kobj, NULL, name);
 	}
 
-- 
2.7.4

Re: [PATCH 2/3] hwmon (occ): Prevent sysfs error attribute from returning error

[Guenter Roeck]

On Tue, Apr 16, 2019 at 03:43:49PM +0000, Eddie James wrote:
> The error sysfs attribute returns the stored error state of the OCC and
> doesn't depend on the OCC poll response. Therefore, split the error
> attribute into it's own function to avoid failing out of the function if
> the poll response fails.
> 
> Signed-off-by: Eddie James <eajames@linux.ibm.com>

Applied.

Thanks,
Guenter

> ---
>  drivers/hwmon/occ/sysfs.c | 19 +++++++++++++------
>  1 file changed, 13 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/hwmon/occ/sysfs.c b/drivers/hwmon/occ/sysfs.c
> index 1cb1e65..c73be07 100644
> --- a/drivers/hwmon/occ/sysfs.c
> +++ b/drivers/hwmon/occ/sysfs.c
> @@ -63,9 +63,6 @@ static ssize_t occ_sysfs_show(struct device *dev,
>  		else
>  			val = 1;
>  		break;
> -	case 8:
> -		val = occ->error;
> -		break;
>  	default:
>  		return -EINVAL;
>  	}
> @@ -73,6 +70,16 @@ static ssize_t occ_sysfs_show(struct device *dev,
>  	return snprintf(buf, PAGE_SIZE - 1, "%d\n", val);
>  }
>  
> +static ssize_t occ_error_show(struct device *dev,
> +			      struct device_attribute *attr, char *buf)
> +{
> +	struct occ *occ = dev_get_drvdata(dev);
> +
> +	occ_update_response(occ);
> +
> +	return snprintf(buf, PAGE_SIZE - 1, "%d\n", occ->error);
> +}
> +
>  static SENSOR_DEVICE_ATTR(occ_master, 0444, occ_sysfs_show, NULL, 0);
>  static SENSOR_DEVICE_ATTR(occ_active, 0444, occ_sysfs_show, NULL, 1);
>  static SENSOR_DEVICE_ATTR(occ_dvfs_overtemp, 0444, occ_sysfs_show, NULL, 2);
> @@ -81,7 +88,7 @@ static SENSOR_DEVICE_ATTR(occ_mem_throttle, 0444, occ_sysfs_show, NULL, 4);
>  static SENSOR_DEVICE_ATTR(occ_quick_pwr_drop, 0444, occ_sysfs_show, NULL, 5);
>  static SENSOR_DEVICE_ATTR(occ_state, 0444, occ_sysfs_show, NULL, 6);
>  static SENSOR_DEVICE_ATTR(occs_present, 0444, occ_sysfs_show, NULL, 7);
> -static SENSOR_DEVICE_ATTR(occ_error, 0444, occ_sysfs_show, NULL, 8);
> +static DEVICE_ATTR_RO(occ_error);
>  
>  static struct attribute *occ_attributes[] = {
>  	&sensor_dev_attr_occ_master.dev_attr.attr,
> @@ -92,7 +99,7 @@ static struct attribute *occ_attributes[] = {
>  	&sensor_dev_attr_occ_quick_pwr_drop.dev_attr.attr,
>  	&sensor_dev_attr_occ_state.dev_attr.attr,
>  	&sensor_dev_attr_occs_present.dev_attr.attr,
> -	&sensor_dev_attr_occ_error.dev_attr.attr,
> +	&dev_attr_occ_error.attr,
>  	NULL
>  };
>  
> @@ -156,7 +163,7 @@ void occ_sysfs_poll_done(struct occ *occ)
>  	}
>  
>  	if (occ->error && occ->error != occ->prev_error) {
> -		name = sensor_dev_attr_occ_error.dev_attr.attr.name;
> +		name = dev_attr_occ_error.attr.name;
>  		sysfs_notify(&occ->bus_dev->kobj, NULL, name);
>  	}
>  

[PATCH 3/3] hwmon (occ): Add more details to Kconfig help text

[Eddie James]

From: Eddie James <eajames@us.ibm.com>

The help text needs to spell out how the driver runs on a BMC, as it
previously seemed to indicate it ran on a POWER processor.

Signed-off-by: Eddie James <eajames@us.ibm.com>
---
 drivers/hwmon/occ/Kconfig | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

diff --git a/drivers/hwmon/occ/Kconfig b/drivers/hwmon/occ/Kconfig
index 2a3869f..1658634 100644
--- a/drivers/hwmon/occ/Kconfig
+++ b/drivers/hwmon/occ/Kconfig
@@ -9,8 +9,10 @@ config SENSORS_OCC_P8_I2C
 	select SENSORS_OCC
 	help
 	 This option enables support for monitoring sensors provided by the
-	 On-Chip Controller (OCC) on a POWER8 processor. Communications with
-	 the OCC are established through I2C bus.
+	 On-Chip Controller (OCC) on a POWER8 processor. However, this driver
+	 can only run on a baseboard management controller (BMC) connected to
+	 the P8, not the POWER processor itself. Communications with the OCC are
+	 established through I2C bus.
 
 	 This driver can also be built as a module. If so, the module will be
 	 called occ-p8-hwmon.
@@ -22,8 +24,10 @@ config SENSORS_OCC_P9_SBE
 	select SENSORS_OCC
 	help
 	 This option enables support for monitoring sensors provided by the
-	 On-Chip Controller (OCC) on a POWER9 processor. Communications with
-	 the OCC are established through SBE fifo on an FSI bus.
+	 On-Chip Controller (OCC) on a POWER9 processor. However, this driver
+	 can only run on a baseboard management controller (BMC) connected to
+	 the P9, not the POWER processor itself. Communications with the OCC are
+	 established through SBE fifo on an FSI bus.
 
 	 This driver can also be built as a module. If so, the module will be
 	 called occ-p9-hwmon.
-- 
2.7.4

Re: [PATCH 3/3] hwmon (occ): Add more details to Kconfig help text

[Guenter Roeck]

On Tue, Apr 16, 2019 at 03:43:50PM +0000, Eddie James wrote:
> From: Eddie James <eajames@us.ibm.com>
> 
> The help text needs to spell out how the driver runs on a BMC, as it
> previously seemed to indicate it ran on a POWER processor.
> 
> Signed-off-by: Eddie James <eajames@us.ibm.com>

Applied.

Thanks,
Guenter

> ---
>  drivers/hwmon/occ/Kconfig | 12 ++++++++----
>  1 file changed, 8 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/hwmon/occ/Kconfig b/drivers/hwmon/occ/Kconfig
> index 2a3869f..1658634 100644
> --- a/drivers/hwmon/occ/Kconfig
> +++ b/drivers/hwmon/occ/Kconfig
> @@ -9,8 +9,10 @@ config SENSORS_OCC_P8_I2C
>  	select SENSORS_OCC
>  	help
>  	 This option enables support for monitoring sensors provided by the
> -	 On-Chip Controller (OCC) on a POWER8 processor. Communications with
> -	 the OCC are established through I2C bus.
> +	 On-Chip Controller (OCC) on a POWER8 processor. However, this driver
> +	 can only run on a baseboard management controller (BMC) connected to
> +	 the P8, not the POWER processor itself. Communications with the OCC are
> +	 established through I2C bus.
>  
>  	 This driver can also be built as a module. If so, the module will be
>  	 called occ-p8-hwmon.
> @@ -22,8 +24,10 @@ config SENSORS_OCC_P9_SBE
>  	select SENSORS_OCC
>  	help
>  	 This option enables support for monitoring sensors provided by the
> -	 On-Chip Controller (OCC) on a POWER9 processor. Communications with
> -	 the OCC are established through SBE fifo on an FSI bus.
> +	 On-Chip Controller (OCC) on a POWER9 processor. However, this driver
> +	 can only run on a baseboard management controller (BMC) connected to
> +	 the P9, not the POWER processor itself. Communications with the OCC are
> +	 established through SBE fifo on an FSI bus.
>  
>  	 This driver can also be built as a module. If so, the module will be
>  	 called occ-p9-hwmon.

Re: [PATCH 1/3] hwmon (occ): Store error condition for rate-limited polls

[Guenter Roeck]

On Tue, Apr 16, 2019 at 03:43:48PM +0000, Eddie James wrote:
> The OCC driver limits the rate of sending poll commands to the OCC. If a
> user reads a hwmon entry after a poll response resulted in an error and
> is rate-limited, the error is invisible to the user. Fix this by storing
> the last error and returning that in the rate-limited case.
> 
> Signed-off-by: Eddie James <eajames@linux.ibm.com>

Applied.

Thanks,
Guenter

> ---
>  drivers/hwmon/occ/common.c | 4 ++++
>  drivers/hwmon/occ/common.h | 3 ++-
>  2 files changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/hwmon/occ/common.c b/drivers/hwmon/occ/common.c
> index 9d197e9..13a6290 100644
> --- a/drivers/hwmon/occ/common.c
> +++ b/drivers/hwmon/occ/common.c
> @@ -141,6 +141,7 @@ static int occ_poll(struct occ *occ)
>  	/* mutex should already be locked if necessary */
>  	rc = occ->send_cmd(occ, cmd);
>  	if (rc) {
> +		occ->last_error = rc;
>  		if (occ->error_count++ > OCC_ERROR_COUNT_THRESHOLD)
>  			occ->error = rc;
>  
> @@ -149,6 +150,7 @@ static int occ_poll(struct occ *occ)
>  
>  	/* clear error since communication was successful */
>  	occ->error_count = 0;
> +	occ->last_error = 0;
>  	occ->error = 0;
>  
>  	/* check for safe state */
> @@ -210,6 +212,8 @@ int occ_update_response(struct occ *occ)
>  	if (time_after(jiffies, occ->last_update + OCC_UPDATE_FREQUENCY)) {
>  		rc = occ_poll(occ);
>  		occ->last_update = jiffies;
> +	} else {
> +		rc = occ->last_error;
>  	}
>  
>  	mutex_unlock(&occ->lock);
> diff --git a/drivers/hwmon/occ/common.h b/drivers/hwmon/occ/common.h
> index ed2cf42..fc13f3c 100644
> --- a/drivers/hwmon/occ/common.h
> +++ b/drivers/hwmon/occ/common.h
> @@ -106,7 +106,8 @@ struct occ {
>  	struct attribute_group group;
>  	const struct attribute_group *groups[2];
>  
> -	int error;                      /* latest transfer error */
> +	int error;                      /* final transfer error after retry */
> +	int last_error;			/* latest transfer error */
>  	unsigned int error_count;       /* number of xfr errors observed */
>  	unsigned long last_safe;        /* time OCC entered "safe" state */
>