On Fri, Oct 27, 2017 at 11:55:05AM -0500, eaja...@linux.vnet.ibm.com wrote:
> From: "Edward A. James" <eaja...@us.ibm.com>
>
> The pmbus core may call read/write word data functions with a page value
> of -1, intending to perform the operation without setting the page.
> However, the read/write word data functions accept only unsigned 8-bit
> page numbers, and therefore cannot check for negative page number to
> avoid setting the page. This results in setting the page number to 0xFF.
> This may result in errors or undefined behavior of some devices
> (specifically the ir35221, which allows the page to be set to 0xFF,
> but some subsequent operations to read registers may fail).
>
> Switch the pmbus_set_page page parameter to an integer and perform the
> check for negative page there. Make read/write functions consistent in
> accepting an integer page number parameter.
>
> Signed-off-by: Edward A. James <eaja...@us.ibm.com>
Applied. Note that the problem was introduced with commit cbcdec6202c9
("hwmon: (pmbus): Access word data for STATUS_WORD"). I marked the patch
accordingly.
Thanks,
Guenter
> ---
>
> Changes since v1:
> * Move check for negative page to pmbus_set_page and change pmbus_set_page
> page parameter to int.
>
> drivers/hwmon/pmbus/pmbus.h | 6 +++---
> drivers/hwmon/pmbus/pmbus_core.c | 25 +++++++++++--------------
> 2 files changed, 14 insertions(+), 17 deletions(-)
>
> diff --git a/drivers/hwmon/pmbus/pmbus.h b/drivers/hwmon/pmbus/pmbus.h
> index 4efa2bd..fa613bd 100644
> --- a/drivers/hwmon/pmbus/pmbus.h
> +++ b/drivers/hwmon/pmbus/pmbus.h
> @@ -404,9 +404,9 @@ struct pmbus_driver_info {
> /* Function declarations */
>
> void pmbus_clear_cache(struct i2c_client *client);
> -int pmbus_set_page(struct i2c_client *client, u8 page);
> -int pmbus_read_word_data(struct i2c_client *client, u8 page, u8 reg);
> -int pmbus_write_word_data(struct i2c_client *client, u8 page, u8 reg, u16
> word);
> +int pmbus_set_page(struct i2c_client *client, int page);
> +int pmbus_read_word_data(struct i2c_client *client, int page, u8 reg);
> +int pmbus_write_word_data(struct i2c_client *client, int page, u8 reg, u16
> word);
> int pmbus_read_byte_data(struct i2c_client *client, int page, u8 reg);
> int pmbus_write_byte(struct i2c_client *client, int page, u8 value);
> int pmbus_write_byte_data(struct i2c_client *client, int page, u8 reg,
> diff --git a/drivers/hwmon/pmbus/pmbus_core.c
> b/drivers/hwmon/pmbus/pmbus_core.c
> index 302f0ae..52a58b8 100644
> --- a/drivers/hwmon/pmbus/pmbus_core.c
> +++ b/drivers/hwmon/pmbus/pmbus_core.c
> @@ -136,13 +136,13 @@ void pmbus_clear_cache(struct i2c_client *client)
> }
> EXPORT_SYMBOL_GPL(pmbus_clear_cache);
>
> -int pmbus_set_page(struct i2c_client *client, u8 page)
> +int pmbus_set_page(struct i2c_client *client, int page)
> {
> struct pmbus_data *data = i2c_get_clientdata(client);
> int rv = 0;
> int newpage;
>
> - if (page != data->currpage) {
> + if (page >= 0 && page != data->currpage) {
> rv = i2c_smbus_write_byte_data(client, PMBUS_PAGE, page);
> newpage = i2c_smbus_read_byte_data(client, PMBUS_PAGE);
> if (newpage != page)
> @@ -158,11 +158,9 @@ int pmbus_write_byte(struct i2c_client *client, int
> page, u8 value)
> {
> int rv;
>
> - if (page >= 0) {
> - rv = pmbus_set_page(client, page);
> - if (rv < 0)
> - return rv;
> - }
> + rv = pmbus_set_page(client, page);
> + if (rv < 0)
> + return rv;
>
> return i2c_smbus_write_byte(client, value);
> }
> @@ -186,7 +184,8 @@ static int _pmbus_write_byte(struct i2c_client *client,
> int page, u8 value)
> return pmbus_write_byte(client, page, value);
> }
>
> -int pmbus_write_word_data(struct i2c_client *client, u8 page, u8 reg, u16
> word)
> +int pmbus_write_word_data(struct i2c_client *client, int page, u8 reg,
> + u16 word)
> {
> int rv;
>
> @@ -219,7 +218,7 @@ static int _pmbus_write_word_data(struct i2c_client
> *client, int page, int reg,
> return pmbus_write_word_data(client, page, reg, word);
> }
>
> -int pmbus_read_word_data(struct i2c_client *client, u8 page, u8 reg)
> +int pmbus_read_word_data(struct i2c_client *client, int page, u8 reg)
> {
> int rv;
>
> @@ -255,11 +254,9 @@ int pmbus_read_byte_data(struct i2c_client *client, int
> page, u8 reg)
> {
> int rv;
>
> - if (page >= 0) {
> - rv = pmbus_set_page(client, page);
> - if (rv < 0)
> - return rv;
> - }
> + rv = pmbus_set_page(client, page);
> + if (rv < 0)
> + return rv;
>
> return i2c_smbus_read_byte_data(client, reg);
> }
