From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751920AbdJYVXI (ORCPT ); Wed, 25 Oct 2017 17:23:08 -0400 Received: from mx0b-001b2d01.pphosted.com ([148.163.158.5]:39108 "EHLO mx0a-001b2d01.pphosted.com" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1751753AbdJYVXG (ORCPT ); Wed, 25 Oct 2017 17:23:06 -0400 Subject: Re: [prefix=PATCH] drivers: pmbus: core: Prevent calling pmbus_set_page with negative page To: Guenter Roeck Cc: jdelvare@suse.com, linux-hwmon@vger.kernel.org, linux-kernel@vger.kernel.org, andrew@aj.id.au, "Edward A. James" References: <1508961173-22686-1-git-send-email-eajames@linux.vnet.ibm.com> <20171025211234.GA3273@roeck-us.net> From: Eddie James Date: Wed, 25 Oct 2017 16:23:01 -0500 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.4.0 MIME-Version: 1.0 In-Reply-To: <20171025211234.GA3273@roeck-us.net> Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 7bit Content-Language: en-US X-TM-AS-GCONF: 00 x-cbid: 17102521-0012-0000-0000-000015328DD6 X-IBM-SpamModules-Scores: X-IBM-SpamModules-Versions: BY=3.00007949; HX=3.00000241; KW=3.00000007; PH=3.00000004; SC=3.00000239; SDB=6.00936419; UDB=6.00471876; IPR=6.00716684; BA=6.00005660; NDR=6.00000001; ZLA=6.00000005; ZF=6.00000009; ZB=6.00000000; ZP=6.00000000; ZH=6.00000000; ZU=6.00000002; MB=3.00017714; XFM=3.00000015; UTC=2017-10-25 21:23:04 X-IBM-AV-DETECTION: SAVI=unused REMOTE=unused XFE=unused x-cbparentid: 17102521-0013-0000-0000-000050042442 Message-Id: <6ad453dc-5d89-4ac8-a379-b5cd3a7b1bbd@linux.vnet.ibm.com> X-Proofpoint-Virus-Version: vendor=fsecure engine=2.50.10432:,, definitions=2017-10-25_10:,, signatures=0 X-Proofpoint-Spam-Details: rule=outbound_notspam policy=outbound score=0 priorityscore=1501 malwarescore=0 suspectscore=0 phishscore=0 bulkscore=0 spamscore=0 clxscore=1015 lowpriorityscore=0 impostorscore=0 adultscore=0 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.0.1-1707230000 definitions=main-1710250281 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 10/25/2017 04:12 PM, Guenter Roeck wrote: > On Wed, Oct 25, 2017 at 02:52:53PM -0500, Eddie James wrote: >> From: "Edward A. James" >> >> For devices with word data registers, the pmbus core may call read/write >> word data functions with a page value of -1, in order to perform the >> operation without setting the page. However, the read/write word data >> functions accept only unsigned 8-bit page numbers, resulting in setting >> a page of 0xFF in this situation. 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). >> > Good catch. > > Subject came off a bit odd. > > [prefix=PATCH] drivers: pmbus: core: Prevent ... > > Please use "[PATCH] hwmon: (pmbus/core) Prevent ..." Yea I hit = instead of - in "--subject-prefix"... > > This me wonder if we should move the check into pmbus_set_page() > instead. > > int pmbus_set_page(struct i2c_client *client, int page) > { > ... > if (page >= 0 && page != data->currpage) { > ... > } > > What do you think ? Yea that works well too, and reduces the code size. Though I suppose it makes the interface slightly less intuitive, as someone might think they can pass a value larger than u8. At least for the read/write functions, a negative value has a meaning - "don't change the page." Up to you, either way. I'm happy to change it. Thanks, Eddie > > Thanks, > Guenter > >> Signed-off-by: Edward A. James >> --- >> drivers/hwmon/pmbus/pmbus.h | 4 ++-- >> drivers/hwmon/pmbus/pmbus_core.c | 29 ++++++++++++++++++----------- >> 2 files changed, 20 insertions(+), 13 deletions(-) >> >> diff --git a/drivers/hwmon/pmbus/pmbus.h b/drivers/hwmon/pmbus/pmbus.h >> index 4efa2bd..4a16da6 100644 >> --- a/drivers/hwmon/pmbus/pmbus.h >> +++ b/drivers/hwmon/pmbus/pmbus.h >> @@ -405,8 +405,8 @@ struct pmbus_driver_info { >> >> 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_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..6d6e030 100644 >> --- a/drivers/hwmon/pmbus/pmbus_core.c >> +++ b/drivers/hwmon/pmbus/pmbus_core.c >> @@ -186,13 +186,16 @@ 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; >> >> - rv = pmbus_set_page(client, page); >> - if (rv < 0) >> - return rv; >> + if (page >= 0) { >> + rv = pmbus_set_page(client, page); >> + if (rv < 0) >> + return rv; >> + } >> >> return i2c_smbus_write_word_data(client, reg, word); >> } >> @@ -219,13 +222,15 @@ 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; >> >> - rv = pmbus_set_page(client, page); >> - if (rv < 0) >> - return rv; >> + if (page >= 0) { >> + rv = pmbus_set_page(client, page); >> + if (rv < 0) >> + return rv; >> + } >> >> return i2c_smbus_read_word_data(client, reg); >> } >> @@ -269,9 +274,11 @@ int pmbus_write_byte_data(struct i2c_client *client, int page, u8 reg, u8 value) >> { >> int rv; >> >> - rv = pmbus_set_page(client, page); >> - if (rv < 0) >> - return rv; >> + if (page >= 0) { >> + rv = pmbus_set_page(client, page); >> + if (rv < 0) >> + return rv; >> + } >> >> return i2c_smbus_write_byte_data(client, reg, value); >> } >> -- >> 1.8.3.1 >>