[PATCH 0/2] cros_ec: utilize cros_ec_cmd_xfer_status()

[PATCH 0/2] cros_ec: utilize cros_ec_cmd_xfer_status()

[Brian Norris]

Hi all,

I was sorting through some out-of-tree patches to these drivers, and I realized
we should probably start making use of cros_ec_cmd_xfer_status() in these
drivers, now that Thierry has queued them up for v4.8; see:

  git://git.kernel.org/pub/scm/linux/kernel/git/thierry.reding/linux-pwm.git
  refs/heads/for-4.8/mfd

  9798ac6d32c1 mfd: cros_ec: Add cros_ec_cmd_xfer_status() helper

Thierry originally provided the above branch for Lee's sake, but I don't see
why (if these are deemed fixes worthy of v4.8) that can't be pulled by Wolfram
and/or Dmitry for their respective subsystems' patches.

Please review.

Regards,
Brian

Brian Norris (2):
  i2c: cros-ec-tunnel: Fix usage of cros_ec_cmd_xfer()
  Input: cros_ec_keyb - Fix usage of cros_ec_cmd_xfer()

 drivers/i2c/busses/i2c-cros-ec-tunnel.c | 2 +-
 drivers/input/keyboard/cros_ec_keyb.c   | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

-- 
2.8.0.rc3.226.g39d4020

[PATCH 1/2] i2c: cros-ec-tunnel: Fix usage of cros_ec_cmd_xfer()

[Brian Norris]

cros_ec_cmd_xfer returns success status if the command transport
completes successfully, but the execution result is incorrectly ignored.
In many cases, the execution result is assumed to be successful, leading
to ignored errors and operating on uninitialized data.

We've recently introduced the cros_ec_cmd_xfer_status() helper to avoid these
problems. Let's use it.

Signed-off-by: Brian Norris <briannorris@chromium.org>
---
 drivers/i2c/busses/i2c-cros-ec-tunnel.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/i2c/busses/i2c-cros-ec-tunnel.c b/drivers/i2c/busses/i2c-cros-ec-tunnel.c
index a0d95ff682ae..2d5ff86398d0 100644
--- a/drivers/i2c/busses/i2c-cros-ec-tunnel.c
+++ b/drivers/i2c/busses/i2c-cros-ec-tunnel.c
@@ -215,7 +215,7 @@ static int ec_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg i2c_msgs[],
 	msg->outsize = request_len;
 	msg->insize = response_len;
 
-	result = cros_ec_cmd_xfer(bus->ec, msg);
+	result = cros_ec_cmd_xfer_status(bus->ec, msg);
 	if (result < 0) {
 		dev_err(dev, "Error transferring EC i2c message %d\n", result);
 		goto exit;
-- 
2.8.0.rc3.226.g39d4020

[PATCH 2/2] Input: cros_ec_keyb - Fix usage of cros_ec_cmd_xfer()

[Brian Norris]

cros_ec_cmd_xfer returns success status if the command transport
completes successfully, but the execution result is incorrectly ignored.
In many cases, the execution result is assumed to be successful, leading
to ignored errors and operating on uninitialized data.

We've recently introduced the cros_ec_cmd_xfer_status() helper to avoid these
problems. Let's use it.

Signed-off-by: Brian Norris <briannorris@chromium.org>
---
 drivers/input/keyboard/cros_ec_keyb.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/input/keyboard/cros_ec_keyb.c b/drivers/input/keyboard/cros_ec_keyb.c
index b01966dc7eb3..6e48616a3a88 100644
--- a/drivers/input/keyboard/cros_ec_keyb.c
+++ b/drivers/input/keyboard/cros_ec_keyb.c
@@ -160,7 +160,7 @@ static int cros_ec_keyb_get_state(struct cros_ec_keyb *ckdev, uint8_t *kb_state)
 	msg->insize = ckdev->cols;
 	msg->outsize = 0;
 
-	ret = cros_ec_cmd_xfer(ckdev->ec, msg);
+	ret = cros_ec_cmd_xfer_status(ckdev->ec, msg);
 	if (ret < 0) {
 		dev_err(ckdev->dev, "Error transferring EC message %d\n", ret);
 		goto exit;
-- 
2.8.0.rc3.226.g39d4020

Re: [PATCH 2/2] Input: cros_ec_keyb - Fix usage of cros_ec_cmd_xfer()

[Dmitry Torokhov]

On Mon, Jul 25, 2016 at 11:14:11AM -0700, Brian Norris wrote:
> cros_ec_cmd_xfer returns success status if the command transport
> completes successfully, but the execution result is incorrectly ignored.
> In many cases, the execution result is assumed to be successful, leading
> to ignored errors and operating on uninitialized data.
> 
> We've recently introduced the cros_ec_cmd_xfer_status() helper to avoid these
> problems. Let's use it.
> 
> Signed-off-by: Brian Norris <briannorris@chromium.org>

Instead of me pulling in pwm/mfd branch maybe Thierry can push through
his branch?

Acked-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>

> ---
>  drivers/input/keyboard/cros_ec_keyb.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/input/keyboard/cros_ec_keyb.c b/drivers/input/keyboard/cros_ec_keyb.c
> index b01966dc7eb3..6e48616a3a88 100644
> --- a/drivers/input/keyboard/cros_ec_keyb.c
> +++ b/drivers/input/keyboard/cros_ec_keyb.c
> @@ -160,7 +160,7 @@ static int cros_ec_keyb_get_state(struct cros_ec_keyb *ckdev, uint8_t *kb_state)
>  	msg->insize = ckdev->cols;
>  	msg->outsize = 0;
>  
> -	ret = cros_ec_cmd_xfer(ckdev->ec, msg);
> +	ret = cros_ec_cmd_xfer_status(ckdev->ec, msg);
>  	if (ret < 0) {
>  		dev_err(ckdev->dev, "Error transferring EC message %d\n", ret);
>  		goto exit;
> -- 
> 2.8.0.rc3.226.g39d4020
> 

-- 
Dmitry

Re: [PATCH 1/2] i2c: cros-ec-tunnel: Fix usage of cros_ec_cmd_xfer()

[kbuild test robot]

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

Hi,

[auto build test ERROR on wsa/i2c/for-next]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Brian-Norris/cros_ec-utilize-cros_ec_cmd_xfer_status/20160726-021919
base:   https://git.kernel.org/pub/scm/linux/kernel/git/wsa/linux.git i2c/for-next
config: i386-randconfig-x016-201630 (attached as .config)
compiler: gcc-6 (Debian 6.1.1-9) 6.1.1 20160705
reproduce:
        # save the attached .config to linux build tree
        make ARCH=i386 

All errors (new ones prefixed by >>):

   drivers/i2c/busses/i2c-cros-ec-tunnel.c: In function 'ec_i2c_xfer':
>> drivers/i2c/busses/i2c-cros-ec-tunnel.c:218:11: error: implicit declaration of function 'cros_ec_cmd_xfer_status' [-Werror=implicit-function-declaration]
     result = cros_ec_cmd_xfer_status(bus->ec, msg);
              ^~~~~~~~~~~~~~~~~~~~~~~
   cc1: some warnings being treated as errors

vim +/cros_ec_cmd_xfer_status +218 drivers/i2c/busses/i2c-cros-ec-tunnel.c

   212	
   213		msg->version = 0;
   214		msg->command = EC_CMD_I2C_PASSTHRU;
   215		msg->outsize = request_len;
   216		msg->insize = response_len;
   217	
 > 218		result = cros_ec_cmd_xfer_status(bus->ec, msg);
   219		if (result < 0) {
   220			dev_err(dev, "Error transferring EC i2c message %d\n", result);
   221			goto exit;

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/octet-stream, Size: 21093 bytes --]

Re: [PATCH 2/2] Input: cros_ec_keyb - Fix usage of cros_ec_cmd_xfer()

[kbuild test robot]

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

Hi,

[auto build test ERROR on wsa/i2c/for-next]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Brian-Norris/cros_ec-utilize-cros_ec_cmd_xfer_status/20160726-021919
base:   https://git.kernel.org/pub/scm/linux/kernel/git/wsa/linux.git i2c/for-next
config: i386-randconfig-x011-201630 (attached as .config)
compiler: gcc-6 (Debian 6.1.1-9) 6.1.1 20160705
reproduce:
        # save the attached .config to linux build tree
        make ARCH=i386 

All errors (new ones prefixed by >>):

   drivers/input/keyboard/cros_ec_keyb.c: In function 'cros_ec_keyb_get_state':
>> drivers/input/keyboard/cros_ec_keyb.c:163:8: error: implicit declaration of function 'cros_ec_cmd_xfer_status' [-Werror=implicit-function-declaration]
     ret = cros_ec_cmd_xfer_status(ckdev->ec, msg);
           ^~~~~~~~~~~~~~~~~~~~~~~
   cc1: some warnings being treated as errors

vim +/cros_ec_cmd_xfer_status +163 drivers/input/keyboard/cros_ec_keyb.c

   157	
   158		msg->version = 0;
   159		msg->command = EC_CMD_MKBP_STATE;
   160		msg->insize = ckdev->cols;
   161		msg->outsize = 0;
   162	
 > 163		ret = cros_ec_cmd_xfer_status(ckdev->ec, msg);
   164		if (ret < 0) {
   165			dev_err(ckdev->dev, "Error transferring EC message %d\n", ret);
   166			goto exit;

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/octet-stream, Size: 22547 bytes --]

Re: [PATCH 1/2] i2c: cros-ec-tunnel: Fix usage of cros_ec_cmd_xfer()

[Javier Martinez Canillas]

Hello Brian,

On 07/25/2016 02:14 PM, Brian Norris wrote:
> cros_ec_cmd_xfer returns success status if the command transport
> completes successfully, but the execution result is incorrectly ignored.
> In many cases, the execution result is assumed to be successful, leading
> to ignored errors and operating on uninitialized data.
> 
> We've recently introduced the cros_ec_cmd_xfer_status() helper to avoid these
> problems. Let's use it.
> 
> Signed-off-by: Brian Norris <briannorris@chromium.org>
> ---

Patch looks good to me.

Reviewed-by: Javier Martinez Canillas <javier@osg.samsung.com>

Best regards,
-- 
Javier Martinez Canillas
Open Source Group
Samsung Research America

Re: [PATCH 2/2] Input: cros_ec_keyb - Fix usage of cros_ec_cmd_xfer()

[Javier Martinez Canillas]

Hello Brian,

On 07/25/2016 02:14 PM, Brian Norris wrote:
> cros_ec_cmd_xfer returns success status if the command transport
> completes successfully, but the execution result is incorrectly ignored.
> In many cases, the execution result is assumed to be successful, leading
> to ignored errors and operating on uninitialized data.
> 
> We've recently introduced the cros_ec_cmd_xfer_status() helper to avoid these
> problems. Let's use it.
> 
> Signed-off-by: Brian Norris <briannorris@chromium.org>
> ---

Reviewed-by: Javier Martinez Canillas <javier@osg.samsung.com>

Best regards,
-- 
Javier Martinez Canillas
Open Source Group
Samsung Research America

Re: [PATCH 1/2] i2c: cros-ec-tunnel: Fix usage of cros_ec_cmd_xfer()

[Wolfram Sang]

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

On Mon, Jul 25, 2016 at 11:14:10AM -0700, Brian Norris wrote:
> cros_ec_cmd_xfer returns success status if the command transport
> completes successfully, but the execution result is incorrectly ignored.
> In many cases, the execution result is assumed to be successful, leading
> to ignored errors and operating on uninitialized data.
> 
> We've recently introduced the cros_ec_cmd_xfer_status() helper to avoid these
> problems. Let's use it.
> 
> Signed-off-by: Brian Norris <briannorris@chromium.org>

I agree with Dmitry about Thierry pushing the patch. So:

Acked-by: Wolfram Sang <wsa@the-dreams.de>


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH 1/2] i2c: cros-ec-tunnel: Fix usage of cros_ec_cmd_xfer()

[Brian Norris]

On Mon, Jul 25, 2016 at 10:43:13PM +0200, Wolfram Sang wrote:
> On Mon, Jul 25, 2016 at 11:14:10AM -0700, Brian Norris wrote:
> > cros_ec_cmd_xfer returns success status if the command transport
> > completes successfully, but the execution result is incorrectly ignored.
> > In many cases, the execution result is assumed to be successful, leading
> > to ignored errors and operating on uninitialized data.
> > 
> > We've recently introduced the cros_ec_cmd_xfer_status() helper to avoid these
> > problems. Let's use it.
> > 
> > Signed-off-by: Brian Norris <briannorris@chromium.org>
> 
> I agree with Dmitry about Thierry pushing the patch. So:
> 
> Acked-by: Wolfram Sang <wsa@the-dreams.de>

Fine with me, as long as Thierry is up for it.

BTW, I think the dependency is on target for v4.8-rc1, so if Thierry
misses this, then you should be able to apply this yourself after the
merge window.

Regards,
Brian

Re: [PATCH 1/2] i2c: cros-ec-tunnel: Fix usage of cros_ec_cmd_xfer()

[Thierry Reding]

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

On Mon, Jul 25, 2016 at 01:48:25PM -0700, Brian Norris wrote:
> On Mon, Jul 25, 2016 at 10:43:13PM +0200, Wolfram Sang wrote:
> > On Mon, Jul 25, 2016 at 11:14:10AM -0700, Brian Norris wrote:
> > > cros_ec_cmd_xfer returns success status if the command transport
> > > completes successfully, but the execution result is incorrectly ignored.
> > > In many cases, the execution result is assumed to be successful, leading
> > > to ignored errors and operating on uninitialized data.
> > > 
> > > We've recently introduced the cros_ec_cmd_xfer_status() helper to avoid these
> > > problems. Let's use it.
> > > 
> > > Signed-off-by: Brian Norris <briannorris@chromium.org>
> > 
> > I agree with Dmitry about Thierry pushing the patch. So:
> > 
> > Acked-by: Wolfram Sang <wsa@the-dreams.de>
> 
> Fine with me, as long as Thierry is up for it.
> 
> BTW, I think the dependency is on target for v4.8-rc1, so if Thierry
> misses this, then you should be able to apply this yourself after the
> merge window.

Why the rush? The behaviour of the cros_ec_cmd_xfer() function has not
changed in at least a year, so this can't be very urgent. I merged the
original patch because it is a dependency for another patch, but given
the above I think it's fine if we wait until after v4.8-rc1 and let
subsystem maintainers pick them up individually.

On another note, the commit message makes it sound like this might fix
potential bugs. Since it's been like that for a couple of releases, do
we need to Cc: stable@vger.kernel.org?

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH 1/2] i2c: cros-ec-tunnel: Fix usage of cros_ec_cmd_xfer()

[Brian Norris]

Hi Thierry,

On Tue, Jul 26, 2016 at 11:14:33AM +0200, Thierry Reding wrote:
> On Mon, Jul 25, 2016 at 01:48:25PM -0700, Brian Norris wrote:
> > On Mon, Jul 25, 2016 at 10:43:13PM +0200, Wolfram Sang wrote:
> > > On Mon, Jul 25, 2016 at 11:14:10AM -0700, Brian Norris wrote:
> > > > cros_ec_cmd_xfer returns success status if the command transport
> > > > completes successfully, but the execution result is incorrectly ignored.
> > > > In many cases, the execution result is assumed to be successful, leading
> > > > to ignored errors and operating on uninitialized data.
> > > > 
> > > > We've recently introduced the cros_ec_cmd_xfer_status() helper to avoid these
> > > > problems. Let's use it.
> > > > 
> > > > Signed-off-by: Brian Norris <briannorris@chromium.org>
> > > 
> > > I agree with Dmitry about Thierry pushing the patch. So:
> > > 
> > > Acked-by: Wolfram Sang <wsa@the-dreams.de>
> > 
> > Fine with me, as long as Thierry is up for it.
> > 
> > BTW, I think the dependency is on target for v4.8-rc1, so if Thierry
> > misses this, then you should be able to apply this yourself after the
> > merge window.
> 
> Why the rush? The behaviour of the cros_ec_cmd_xfer() function has not
> changed in at least a year, so this can't be very urgent. I merged the
> original patch because it is a dependency for another patch, but given
> the above I think it's fine if we wait until after v4.8-rc1 and let
> subsystem maintainers pick them up individually.

I wasn't personally suggesting it was a rush -- actually, the contrary.
I was just informing Wolfram and Dmitry that the dependency only was
relevant *if* they were rushing to have the patches applied.

Regarding timeline: some form of this patch was authored and submitted
to our downstream tree over a year ago. I just happened to notice
recently, now that the ..._status() helper is going upstream.

> On another note, the commit message makes it sound like this might fix
> potential bugs. Since it's been like that for a couple of releases, do
> we need to Cc: stable@vger.kernel.org?

It does potentially fix bugs. I suspect those bugs would probably occur
mostly in cases of poorly-configured software (e.g., using the wrong EC
protocol) or prototype hardware, but it's certainly possible this could
head off in-the-field bugs. Perhaps Gwendal or Shawn could elaborate.

At any rate, if you Cc: stable@vger.kernel.org, you'll want to include
the dependency in the commit message. I think the format is something
like this:

Fixes: SHA ("i2c: wherever this driver was introduced")
Cc: <stable@vger.kernel.org> # 9798ac6d32c1 mfd: cros_ec: Add cros_ec_cmd_xfer_status() helper

Regards,
Brian

Re: [PATCH 1/2] i2c: cros-ec-tunnel: Fix usage of cros_ec_cmd_xfer()

[Thierry Reding]

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

On Tue, Jul 26, 2016 at 11:38:20AM -0700, Brian Norris wrote:
> Hi Thierry,
> 
> On Tue, Jul 26, 2016 at 11:14:33AM +0200, Thierry Reding wrote:
> > On Mon, Jul 25, 2016 at 01:48:25PM -0700, Brian Norris wrote:
> > > On Mon, Jul 25, 2016 at 10:43:13PM +0200, Wolfram Sang wrote:
> > > > On Mon, Jul 25, 2016 at 11:14:10AM -0700, Brian Norris wrote:
> > > > > cros_ec_cmd_xfer returns success status if the command transport
> > > > > completes successfully, but the execution result is incorrectly ignored.
> > > > > In many cases, the execution result is assumed to be successful, leading
> > > > > to ignored errors and operating on uninitialized data.
> > > > > 
> > > > > We've recently introduced the cros_ec_cmd_xfer_status() helper to avoid these
> > > > > problems. Let's use it.
> > > > > 
> > > > > Signed-off-by: Brian Norris <briannorris@chromium.org>
> > > > 
> > > > I agree with Dmitry about Thierry pushing the patch. So:
> > > > 
> > > > Acked-by: Wolfram Sang <wsa@the-dreams.de>
> > > 
> > > Fine with me, as long as Thierry is up for it.
> > > 
> > > BTW, I think the dependency is on target for v4.8-rc1, so if Thierry
> > > misses this, then you should be able to apply this yourself after the
> > > merge window.
> > 
> > Why the rush? The behaviour of the cros_ec_cmd_xfer() function has not
> > changed in at least a year, so this can't be very urgent. I merged the
> > original patch because it is a dependency for another patch, but given
> > the above I think it's fine if we wait until after v4.8-rc1 and let
> > subsystem maintainers pick them up individually.
> 
> I wasn't personally suggesting it was a rush -- actually, the contrary.
> I was just informing Wolfram and Dmitry that the dependency only was
> relevant *if* they were rushing to have the patches applied.

Okay, I'll let Wolfram and Dmitry pick these up after v4.8-rc1 then.

> > On another note, the commit message makes it sound like this might fix
> > potential bugs. Since it's been like that for a couple of releases, do
> > we need to Cc: stable@vger.kernel.org?
> 
> It does potentially fix bugs. I suspect those bugs would probably occur
> mostly in cases of poorly-configured software (e.g., using the wrong EC
> protocol) or prototype hardware, but it's certainly possible this could
> head off in-the-field bugs. Perhaps Gwendal or Shawn could elaborate.
> 
> At any rate, if you Cc: stable@vger.kernel.org, you'll want to include
> the dependency in the commit message. I think the format is something
> like this:
> 
> Fixes: SHA ("i2c: wherever this driver was introduced")
> Cc: <stable@vger.kernel.org> # 9798ac6d32c1 mfd: cros_ec: Add cros_ec_cmd_xfer_status() helper

That's information you're supposed to add to your patch as the author,
so as a courtesy to upstream maintainers, perhaps resend these two
patches with a complete set of tags once v4.8-rc1 has been released?

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]