Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1449714 > unrolled thread

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

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [PATCH 1/2] i2c: cros-ec-tunnel: Fix usage of cros_ec_cmd_xfer() Brian Norris <briannorris@chromium.org> - 2016-07-25 20:20 +0200
    Re: [PATCH 1/2] i2c: cros-ec-tunnel: Fix usage of cros_ec_cmd_xfer() kbuild test robot <lkp@intel.com> - 2016-07-25 20:40 +0200
    Re: [PATCH 1/2] i2c: cros-ec-tunnel: Fix usage of cros_ec_cmd_xfer() Javier Martinez Canillas <javier@osg.samsung.com> - 2016-07-25 21:50 +0200
    Re: [PATCH 1/2] i2c: cros-ec-tunnel: Fix usage of cros_ec_cmd_xfer() Brian Norris <briannorris@chromium.org> - 2016-07-25 22:50 +0200
      Re: [PATCH 1/2] i2c: cros-ec-tunnel: Fix usage of cros_ec_cmd_xfer() Thierry Reding <thierry.reding@gmail.com> - 2016-07-26 11:20 +0200
        Re: [PATCH 1/2] i2c: cros-ec-tunnel: Fix usage of cros_ec_cmd_xfer() Brian Norris <briannorris@chromium.org> - 2016-07-26 20:40 +0200
          Re: [PATCH 1/2] i2c: cros-ec-tunnel: Fix usage of cros_ec_cmd_xfer() Thierry Reding <thierry.reding@gmail.com> - 2016-07-28 16:20 +0200
    Re: [PATCH 1/2] i2c: cros-ec-tunnel: Fix usage of cros_ec_cmd_xfer() Wolfram Sang <wsa@the-dreams.de> - 2016-07-25 22:50 +0200

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

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

[toc] | [next] | [standalone]

#1449733

[Multipart message — attachments visible in raw view] — view raw

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

[toc] | [prev] | [next] | [standalone]

#1449775

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

[toc] | [prev] | [next] | [standalone]

#1449794

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

[toc] | [prev] | [next] | [standalone]

#1450489

[Multipart message — attachments visible in raw view] — view raw

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

[toc] | [prev] | [next] | [standalone]

#1450780

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

[toc] | [prev] | [next] | [standalone]

#1451952

[Multipart message — attachments visible in raw view] — view raw

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

[toc] | [prev] | [next] | [standalone]

#1449797

[Multipart message — attachments visible in raw view] — view raw

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>

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web