lkml.org 
[lkml]   [2017]   [Oct]   [28]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [v2] hwmon: (pmbus/core): Prevent unintentional setting of page to 0xFF
On Fri, Oct 27, 2017 at 11:55:05AM -0500, eajames@linux.vnet.ibm.com wrote:
> From: "Edward A. James" <eajames@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 <eajames@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);
> }

\
 
 \ /
  Last update: 2017-10-28 20:22    [W:0.061 / U:10.024 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site