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

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:

quoted

On Mon, Jul 25, 2016 at 10:43:13PM +0200, Wolfram Sang wrote:

quoted

On Mon, Jul 25, 2016 at 11:14:10AM -0700, Brian Norris wrote:

quoted

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 <redacted>

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: <redacted> # 9798ac6d32c1 mfd: cros_ec: Add cros_ec_cmd_xfer_status() helper

Regards,
Brian