[PATCH linux dev-4.10 0/3] pmbus: Misc work-arounds for the PAGE command

[PATCH linux dev-4.10 0/3] pmbus: Misc work-arounds for the PAGE command

[Andrew Jeffery]

Hello,

Occasionally the MAX31785 NACKs setting the page on the device. The consequence
is the PMBus core either thinks some functionality is unsupported by the device
or it inspects features on the wrong page after swallowing the error.

The most obvious side-effect is that some hwmon attributes are not created when
they should be, often causing issues for fan monitoring and control in
userspace, preventing the host from booting.

The series introduces a one-shot retry in the PMBus core for the PAGE command.
This is generally terrible, but the alternatives seem worse. Also this is not
yet appropriate to upstream as I haven't succeeded in getting the MAX31785
driver applied there. As a result this dirty secret can live in the OpenBMC
tree until we properly understand the problem.

The first couple of patches help avoid setting PAGE to values that never make
sense. Again this involves a questionable change to the PMBus core, but I think
we can live with it.

Some brief testing of the series indicates that all one-shot attempts succeed
in the face of a previous failure, much like the FAN_CONFIG_* issues.

Let me know what you think!

Andrew Jeffery (3):
  hwmon: pmbus: core: Add virtual page config bit
  hwmon: pmbus: max31785: Mark virtual pages as virtual
  hwmon: pmbus: core: One-shot retries for failure to set page

 drivers/hwmon/pmbus/max31785.c   |  1 +
 drivers/hwmon/pmbus/pmbus.h      |  2 ++
 drivers/hwmon/pmbus/pmbus_core.c | 23 +++++++++++++++++++----
 3 files changed, 22 insertions(+), 4 deletions(-)

-- 
2.11.0

[PATCH linux dev-4.10 1/3] hwmon: pmbus: core: Add virtual page config bit

[Andrew Jeffery]

Some circumstances call for virtual pages to expose multiple values
packed into an extended PMBus register in a manner non-compliant with
the PMBus standard. We should not try to set virtual pages on the
device; add a flag so we can avoid doing so.

Cc: Eddie James <eajames@linux.vnet.ibm.com>
Cc: Matt Spinler <mspinler@linux.vnet.ibm.com>
Signed-off-by: Andrew Jeffery <andrew@aj.id.au>
---
 drivers/hwmon/pmbus/pmbus.h      |  2 ++
 drivers/hwmon/pmbus/pmbus_core.c | 12 ++++++++----
 2 files changed, 10 insertions(+), 4 deletions(-)

diff --git a/drivers/hwmon/pmbus/pmbus.h b/drivers/hwmon/pmbus/pmbus.h
index a863b8fed16f..fd422cd76452 100644
--- a/drivers/hwmon/pmbus/pmbus.h
+++ b/drivers/hwmon/pmbus/pmbus.h
@@ -367,6 +367,8 @@ enum pmbus_sensor_classes {
 #define PMBUS_HAVE_PWM12	BIT(20)
 #define PMBUS_HAVE_PWM34	BIT(21)
 
+#define PMBUS_PAGE_VIRTUAL	BIT(31)
+
 enum pmbus_data_format { linear = 0, direct, vid };
 enum vrm_version { vr11 = 0, vr12 };
 
diff --git a/drivers/hwmon/pmbus/pmbus_core.c b/drivers/hwmon/pmbus/pmbus_core.c
index f72f966f5251..390a7b02b836 100644
--- a/drivers/hwmon/pmbus/pmbus_core.c
+++ b/drivers/hwmon/pmbus/pmbus_core.c
@@ -164,14 +164,18 @@ int pmbus_set_page(struct i2c_client *client, u8 page)
 	int rv = 0;
 	int newpage;
 
-	if (page != data->currpage) {
+	if (page == data->currpage)
+		return 0;
+
+	if (!(data->info->func[page] & PMBUS_PAGE_VIRTUAL)) {
 		rv = i2c_smbus_write_byte_data(client, PMBUS_PAGE, page);
 		newpage = i2c_smbus_read_byte_data(client, PMBUS_PAGE);
 		if (newpage != page)
-			rv = -EIO;
-		else
-			data->currpage = page;
+			return -EIO;
 	}
+
+	data->currpage = page;
+
 	return rv;
 }
 EXPORT_SYMBOL_GPL(pmbus_set_page);
-- 
2.11.0

[PATCH linux dev-4.10 2/3] hwmon: pmbus: max31785: Mark virtual pages as virtual

[Andrew Jeffery]

Avoid setting PAGE to a unsupported value on the MAX31785 to avoid
errors from the hardware in circumstances that we know it will fail.

The virtual pages are used to expose the non-standard tacho measurements
for the second rotor of a dual-rotor fan. Maxim's implementation breaks
with the PMBus standard by providing a four-byte response to what is
usually a two-byte command.

Cc: Eddie James <eajames@linux.vnet.ibm.com>
Cc: Matt Spinler <mspinler@linux.vnet.ibm.com>
Signed-off-by: Andrew Jeffery <andrew@aj.id.au>
---
 drivers/hwmon/pmbus/max31785.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/hwmon/pmbus/max31785.c b/drivers/hwmon/pmbus/max31785.c
index 6a0d289ad346..c862ab51e3af 100644
--- a/drivers/hwmon/pmbus/max31785.c
+++ b/drivers/hwmon/pmbus/max31785.c
@@ -436,6 +436,7 @@ static int max31785_of_fan_config(struct i2c_client *client,
 
 			info->pages = max(info->pages, virtual + 1);
 			info->func[virtual] |= PMBUS_HAVE_FAN12;
+			info->func[virtual] |= PMBUS_PAGE_VIRTUAL;
 		}
 	}
 
-- 
2.11.0

[PATCH linux dev-4.10 3/3] hwmon: pmbus: core: One-shot retries for failure to set page

[Andrew Jeffery]

Work around the shonky behaviour seen with the MAX31785 where we fail
to set the page register in some circumstances.

There's no real elegant way to do this. We can propagate the error up,
but that forces us to retry the operation way up the call tree in any
number of places. It also forces callers to split out pmbus_set_page()
from the pmbus_{read,write}_{byte,word}_data() functions in order to
differentiate between a failure to set the page and a failure to read a
register (that might not exist, in which case an error is anticiptated).

Cc: Eddie James <eajames@linux.vnet.ibm.com>
Cc: Matt Spinler <mspinler@linux.vnet.ibm.com>
Signed-off-by: Andrew Jeffery <andrew@aj.id.au>
---
 drivers/hwmon/pmbus/pmbus_core.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/drivers/hwmon/pmbus/pmbus_core.c b/drivers/hwmon/pmbus/pmbus_core.c
index 390a7b02b836..d4e959bda491 100644
--- a/drivers/hwmon/pmbus/pmbus_core.c
+++ b/drivers/hwmon/pmbus/pmbus_core.c
@@ -168,7 +168,18 @@ int pmbus_set_page(struct i2c_client *client, u8 page)
 		return 0;
 
 	if (!(data->info->func[page] & PMBUS_PAGE_VIRTUAL)) {
+		dev_dbg(&client->dev, "Want page %u, %u cached\n", page,
+			data->currpage);
+
 		rv = i2c_smbus_write_byte_data(client, PMBUS_PAGE, page);
+		if (rv) {
+			rv = i2c_smbus_write_byte_data(client, PMBUS_PAGE,
+						       page);
+			dev_warn(&client->dev,
+				 "Failed to set page %u, performed one-shot retry %s: %d\n",
+				 page, rv ? "and failed" : "with success", rv);
+		}
+
 		newpage = i2c_smbus_read_byte_data(client, PMBUS_PAGE);
 		if (newpage != page)
 			return -EIO;
-- 
2.11.0

Re: [PATCH linux dev-4.10 3/3] hwmon: pmbus: core: One-shot retries for failure to set page

[Matt Spinler]

Reviewed-by: Matt Spinler mspinler@linux.vnet.ibm.com


On 10/26/2017 1:19 AM, Andrew Jeffery wrote:
> Work around the shonky behaviour seen with the MAX31785 where we fail
> to set the page register in some circumstances.
>
> There's no real elegant way to do this. We can propagate the error up,
> but that forces us to retry the operation way up the call tree in any
> number of places. It also forces callers to split out pmbus_set_page()
> from the pmbus_{read,write}_{byte,word}_data() functions in order to
> differentiate between a failure to set the page and a failure to read a
> register (that might not exist, in which case an error is anticiptated).
>
> Cc: Eddie James <eajames@linux.vnet.ibm.com>
> Cc: Matt Spinler <mspinler@linux.vnet.ibm.com>
> Signed-off-by: Andrew Jeffery <andrew@aj.id.au>
> ---
>   drivers/hwmon/pmbus/pmbus_core.c | 11 +++++++++++
>   1 file changed, 11 insertions(+)
>
> diff --git a/drivers/hwmon/pmbus/pmbus_core.c b/drivers/hwmon/pmbus/pmbus_core.c
> index 390a7b02b836..d4e959bda491 100644
> --- a/drivers/hwmon/pmbus/pmbus_core.c
> +++ b/drivers/hwmon/pmbus/pmbus_core.c
> @@ -168,7 +168,18 @@ int pmbus_set_page(struct i2c_client *client, u8 page)
>   		return 0;
>
>   	if (!(data->info->func[page] & PMBUS_PAGE_VIRTUAL)) {
> +		dev_dbg(&client->dev, "Want page %u, %u cached\n", page,
> +			data->currpage);
> +
>   		rv = i2c_smbus_write_byte_data(client, PMBUS_PAGE, page);
> +		if (rv) {
> +			rv = i2c_smbus_write_byte_data(client, PMBUS_PAGE,
> +						       page);
> +			dev_warn(&client->dev,
> +				 "Failed to set page %u, performed one-shot retry %s: %d\n",
> +				 page, rv ? "and failed" : "with success", rv);
> +		}
> +
>   		newpage = i2c_smbus_read_byte_data(client, PMBUS_PAGE);
>   		if (newpage != page)
>   			return -EIO;

Re: [PATCH linux dev-4.10 2/3] hwmon: pmbus: max31785: Mark virtual pages as virtual

[Matt Spinler]

Reviewed-by: Matt Spinler mspinler@linux.vnet.ibm.com


On 10/26/2017 1:19 AM, Andrew Jeffery wrote:
> Avoid setting PAGE to a unsupported value on the MAX31785 to avoid
> errors from the hardware in circumstances that we know it will fail.
>
> The virtual pages are used to expose the non-standard tacho measurements
> for the second rotor of a dual-rotor fan. Maxim's implementation breaks
> with the PMBus standard by providing a four-byte response to what is
> usually a two-byte command.
>
> Cc: Eddie James <eajames@linux.vnet.ibm.com>
> Cc: Matt Spinler <mspinler@linux.vnet.ibm.com>
> Signed-off-by: Andrew Jeffery <andrew@aj.id.au>
> ---
>   drivers/hwmon/pmbus/max31785.c | 1 +
>   1 file changed, 1 insertion(+)
>
> diff --git a/drivers/hwmon/pmbus/max31785.c b/drivers/hwmon/pmbus/max31785.c
> index 6a0d289ad346..c862ab51e3af 100644
> --- a/drivers/hwmon/pmbus/max31785.c
> +++ b/drivers/hwmon/pmbus/max31785.c
> @@ -436,6 +436,7 @@ static int max31785_of_fan_config(struct i2c_client *client,
>
>   			info->pages = max(info->pages, virtual + 1);
>   			info->func[virtual] |= PMBUS_HAVE_FAN12;
> +			info->func[virtual] |= PMBUS_PAGE_VIRTUAL;
>   		}
>   	}
>

Re: [PATCH linux dev-4.10 1/3] hwmon: pmbus: core: Add virtual page config bit

[Matt Spinler]

Reviewed-by: Matt Spinler mspinler@linux.vnet.ibm.com


On 10/26/2017 1:19 AM, Andrew Jeffery wrote:
> Some circumstances call for virtual pages to expose multiple values
> packed into an extended PMBus register in a manner non-compliant with
> the PMBus standard. We should not try to set virtual pages on the
> device; add a flag so we can avoid doing so.
>
> Cc: Eddie James <eajames@linux.vnet.ibm.com>
> Cc: Matt Spinler <mspinler@linux.vnet.ibm.com>
> Signed-off-by: Andrew Jeffery <andrew@aj.id.au>
> ---
>   drivers/hwmon/pmbus/pmbus.h      |  2 ++
>   drivers/hwmon/pmbus/pmbus_core.c | 12 ++++++++----
>   2 files changed, 10 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/hwmon/pmbus/pmbus.h b/drivers/hwmon/pmbus/pmbus.h
> index a863b8fed16f..fd422cd76452 100644
> --- a/drivers/hwmon/pmbus/pmbus.h
> +++ b/drivers/hwmon/pmbus/pmbus.h
> @@ -367,6 +367,8 @@ enum pmbus_sensor_classes {
>   #define PMBUS_HAVE_PWM12	BIT(20)
>   #define PMBUS_HAVE_PWM34	BIT(21)
>
> +#define PMBUS_PAGE_VIRTUAL	BIT(31)
> +
>   enum pmbus_data_format { linear = 0, direct, vid };
>   enum vrm_version { vr11 = 0, vr12 };
>
> diff --git a/drivers/hwmon/pmbus/pmbus_core.c b/drivers/hwmon/pmbus/pmbus_core.c
> index f72f966f5251..390a7b02b836 100644
> --- a/drivers/hwmon/pmbus/pmbus_core.c
> +++ b/drivers/hwmon/pmbus/pmbus_core.c
> @@ -164,14 +164,18 @@ int pmbus_set_page(struct i2c_client *client, u8 page)
>   	int rv = 0;
>   	int newpage;
>
> -	if (page != data->currpage) {
> +	if (page == data->currpage)
> +		return 0;
> +
> +	if (!(data->info->func[page] & PMBUS_PAGE_VIRTUAL)) {
>   		rv = i2c_smbus_write_byte_data(client, PMBUS_PAGE, page);
>   		newpage = i2c_smbus_read_byte_data(client, PMBUS_PAGE);
>   		if (newpage != page)
> -			rv = -EIO;
> -		else
> -			data->currpage = page;
> +			return -EIO;
>   	}
> +
> +	data->currpage = page;
> +
>   	return rv;
>   }
>   EXPORT_SYMBOL_GPL(pmbus_set_page);

Re: [PATCH linux dev-4.10 0/3] pmbus: Misc work-arounds for the PAGE command

[Andrew Jeffery]

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

On Thu, 2017-10-26 at 16:49 +1030, Andrew Jeffery wrote:
> Hello,
> 
> Occasionally the MAX31785 NACKs setting the page on the device. The consequence
> is the PMBus core either thinks some functionality is unsupported by the device
> or it inspects features on the wrong page after swallowing the error.
> 
> The most obvious side-effect is that some hwmon attributes are not created when
> they should be, often causing issues for fan monitoring and control in
> userspace, preventing the host from booting.
> 
> The series introduces a one-shot retry in the PMBus core for the PAGE command.
> This is generally terrible, but the alternatives seem worse. Also this is not
> yet appropriate to upstream as I haven't succeeded in getting the MAX31785
> driver applied there. As a result this dirty secret can live in the OpenBMC
> tree until we properly understand the problem.
> 
> The first couple of patches help avoid setting PAGE to values that never make
> sense. Again this involves a questionable change to the PMBus core, but I think
> we can live with it.
> 
> Some brief testing of the series indicates that all one-shot attempts succeed
> in the face of a previous failure, much like the FAN_CONFIG_* issues.
> 
> Let me know what you think!
> 
> Andrew Jeffery (3):
>   hwmon: pmbus: core: Add virtual page config bit
>   hwmon: pmbus: max31785: Mark virtual pages as virtual
>   hwmon: pmbus: core: One-shot retries for failure to set page

I've applied these to dev-4.10 with Matt Spinler's Reviewed-by tags.

Cheers,

Andrew

> 
>  drivers/hwmon/pmbus/max31785.c   |  1 +
>  drivers/hwmon/pmbus/pmbus.h      |  2 ++
>  drivers/hwmon/pmbus/pmbus_core.c | 23 +++++++++++++++++++----
>  3 files changed, 22 insertions(+), 4 deletions(-)
> 

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