From patchwork Wed Dec 7 18:44:08 2016 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Sudeep Holla X-Patchwork-Id: 9465077 Return-Path: Received: from mail.wl.linuxfoundation.org (pdx-wl-mail.web.codeaurora.org [172.30.200.125]) by pdx-korg-patchwork.web.codeaurora.org (Postfix) with ESMTP id 611266022E for ; Wed, 7 Dec 2016 18:44:47 +0000 (UTC) Received: from mail.wl.linuxfoundation.org (localhost [127.0.0.1]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id 32C2B28532 for ; Wed, 7 Dec 2016 18:44:47 +0000 (UTC) Received: by mail.wl.linuxfoundation.org (Postfix, from userid 486) id 230A82855A; Wed, 7 Dec 2016 18:44:47 +0000 (UTC) X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on pdx-wl-mail.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-4.2 required=2.0 tests=BAYES_00, RCVD_IN_DNSWL_MED autolearn=ham version=3.3.1 Received: from bombadil.infradead.org (bombadil.infradead.org [198.137.202.9]) (using TLSv1.2 with cipher AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by mail.wl.linuxfoundation.org (Postfix) with ESMTPS id A49FC28532 for ; Wed, 7 Dec 2016 18:44:46 +0000 (UTC) Received: from localhost ([127.0.0.1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.85_2 #1 (Red Hat Linux)) id 1cEhCn-0006M5-Ju; Wed, 07 Dec 2016 18:44:37 +0000 Received: from foss.arm.com ([217.140.101.70]) by bombadil.infradead.org with esmtp (Exim 4.85_2 #1 (Red Hat Linux)) id 1cEhCh-0006Dz-0u; Wed, 07 Dec 2016 18:44:35 +0000 Received: from usa-sjc-imap-foss1.foss.arm.com (unknown [10.72.51.249]) by usa-sjc-mx-foss1.foss.arm.com (Postfix) with ESMTP id 642F5AD7; Wed, 7 Dec 2016 10:44:10 -0800 (PST) Received: from [10.1.210.28] (e107155-lin.cambridge.arm.com [10.1.210.28]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id 8583B3F445; Wed, 7 Dec 2016 10:44:09 -0800 (PST) Subject: Re: [PATCH] firmware: arm_scpi: fix reading sensor values on pre-1.0 SCPI firmwares To: Martin Blumenstingl , linux-arm-kernel@lists.infradead.org, linux-amlogic@lists.infradead.org References: <20161124001845.20830-1-martin.blumenstingl@googlemail.com> <20161124001845.20830-2-martin.blumenstingl@googlemail.com> From: Sudeep Holla Organization: ARM Message-ID: <663deed4-fbbb-20ad-1943-dbd4e66e797f@arm.com> Date: Wed, 7 Dec 2016 18:44:08 +0000 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.5.1 MIME-Version: 1.0 In-Reply-To: <20161124001845.20830-2-martin.blumenstingl@googlemail.com> X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20161207_104431_119086_4E2AD5C5 X-CRM114-Status: GOOD ( 13.75 ) X-BeenThere: linux-amlogic@lists.infradead.org X-Mailman-Version: 2.1.20 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: narmstrong@baylibre.com, Sudeep Holla Sender: "linux-amlogic" Errors-To: linux-amlogic-bounces+patchwork-linux-amlogic=patchwork.kernel.org@lists.infradead.org X-Virus-Scanned: ClamAV using ClamSMTP On 24/11/16 00:18, Martin Blumenstingl wrote: > The pre-1.0 SCPI firmwares are using one __le32 as sensor value, while > the 1.0 SCPI protocol uses two __le32 as sensor values (a total of > 64bit, split into 32bit upper and 32bit lower value). > Using an "struct sensor_value" to read the sensor value on a pre-1.0 > SCPI firmware gives garbage in the "hi_val" field. Introducing a > separate function which handles scpi_ops.sensor_get_value for pre-1.0 > SCPI firmware implementations ensures that we do not read memory which > was not written by the SCPI firmware (which fixes for example the > temperature reported by scpi-hwmon). > > Signed-off-by: Martin Blumenstingl > --- > drivers/firmware/arm_scpi.c | 15 +++++++++++++++ > 1 file changed, 15 insertions(+) > > diff --git a/drivers/firmware/arm_scpi.c b/drivers/firmware/arm_scpi.c > index 70e1323..19f750d 100644 > --- a/drivers/firmware/arm_scpi.c > +++ b/drivers/firmware/arm_scpi.c > @@ -728,6 +728,20 @@ static int scpi_sensor_get_value(u16 sensor, u64 *val) > return ret; > } > > +static int legacy_scpi_sensor_get_value(u16 sensor, u64 *val) > +{ > + __le16 id = cpu_to_le16(sensor); > + __le32 value; > + int ret; > + > + ret = scpi_send_message(CMD_SENSOR_VALUE, &id, sizeof(id), > + &value, sizeof(value)); > + if (!ret) > + *val = le32_to_cpu(value); > + > + return ret; > +} Instead of duplicating the code so much, can we just manage something like this. If more commands need Rx len, then we can think of adding that. Even then reseting shared buffers is not good, we can clear the buffers on the stack. static int scpi_device_get_power_state(u16 dev_id) diff --git i/drivers/firmware/arm_scpi.c w/drivers/firmware/arm_scpi.c index 70e13230d8db..04aa873205e9 100644 --- i/drivers/firmware/arm_scpi.c +++ w/drivers/firmware/arm_scpi.c @@ -721,11 +721,15 @@ static int scpi_sensor_get_value(u16 sensor, u64 *val) ret = scpi_send_message(CMD_SENSOR_VALUE, &id, sizeof(id), &buf, sizeof(buf)); - if (!ret) + if (ret) + return ret; + + if (scpi_info->is_legacy) /* 32-bits supported, hi_val can be junk */ + *val = le32_to_cpu(buf.lo_val); + else *val = (u64)le32_to_cpu(buf.hi_val) << 32 | le32_to_cpu(buf.lo_val); - - return ret; + return 0; }