On Mon, Jan 14, 2019 at 6:04 PM Brian Norris <[email protected]> wrote: > > Hi Gwendal, > > On Mon, Jan 14, 2019 at 3:50 PM Gwendal Grignou <[email protected]> wrote: > > On Fri, Dec 7, 2018 at 2:22 PM Brian Norris <[email protected]> > > wrote: > > > On Thu, Nov 29, 2018 at 11:55:48AM -0800, [email protected] wrote: > > > > --- a/drivers/platform/chrome/cros_ec_proto.c > > > > +++ b/drivers/platform/chrome/cros_ec_proto.c > > > > @@ -420,10 +420,14 @@ int cros_ec_query_all(struct cros_ec_device > > > > *ec_dev) > > > > ret = cros_ec_get_host_command_version_mask(ec_dev, > > > > EC_CMD_GET_NEXT_EVENT, > > > > &ver_mask); > > > > > > It's not exactly new here (although you're using 'ver_mask' in new > > > ways), but cros_ec_get_host_command_version_mask() doesn't look 100% > > > right. It doesn't look at msg->result, and instead just assumes that if > > > we got some data back (send_command() > 0), then it must have been a > > > success. I don't think that's really guaranteed in general, although it > > > might be for the specific case of EC_CMD_GET_CMD_VERSIONS. > > > It is guaranteed: if msg->result is not EC_RES_SUCCESS, then ret can > > not be greater than 0. At best it will be 0, or a negative number if > > we can already qualify the error in the errno space (see > > T() for instance). > > Sorry, where do you guarantee that? The only enforcements I see in > those xfer implementation are: > (1) if result == EC_RES_IN_PROGRESS, we convert that to an errno > (2) if the expected length or checksum are bad, we turn that to an errno > > So technically, the EC *could* be sending a valid, checksummed > response of the expected length, while still setting the ->result > field to something besides EC_RES_SUCCESS or EC_RES_IN_PROGRESS. And > we would treat that as a valid 'ver_mask'. You're right, I misread cros_ec_pkt_xfer_i2c(). > Albeit, that seems unlikely, given understanding of how the EC is > supposed to behave, but our code is not properly defensive AIUI. This > is basically why cros_ec_cmd_xfer_status() exists -- so that > sub-drivers don't get lazy and use cros_ec_cmd_xfer() without handling > the ->result field properly. send_command is called for a very small subset of command where the ec_dev mutex is already held. We indeed need to be careful when calling it directly.
Gwendal. > > Brian
