| Message ID | 1469470451-111822-2-git-send-email-briannorris@chromium.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
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,
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>
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 -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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
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 -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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
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;
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(-)