On Thu, Jun 13, 2024 at 05:51:39PM +0100, Ben Walsh wrote: > > Thanks for fixing this! Unfortunately `in_range` returns -EINVAL if > length == 0 (see the definition of `fwk_ec_lpc_mec_in_range`). I'm sure > this broke something in my testing, but I can't find what it was now. I don't think fwk_ec_lpc_mec_in_range() is upstream. This email is the only reference I can find to it on the internet. > > My original suggestion was to add a test for "length == 0" before the > "in_range" test, then do the test as you have done. But we decided to > defer this to a later, separate patch. > > There's also a similar "in_range" test in `fwk_ec_lpc_mec_write_bytes`. > > We could: > > 1. Revert this and change the `data & EC_LPC_STATUS_BUSY_MASK` to > `res & EC_LPC_STATUS_BUSY_MASK`. This is the same logic as before the > negative error code change. > > or 2. Put in a check for length == 0. > > or 3. Change the logic in `fwk_ec_lpc_mec_in_range`. Although I'm not > sure what the correct answer is to "zero length is in range?" > > I prefer option 2. What do you think? diff --git a/drivers/platform/chrome/cros_ec_lpc_mec.c b/drivers/platform/chrome/cros_ec_lpc_mec.c index dfad934e65ca..9bf74656164f 100644 --- a/drivers/platform/chrome/cros_ec_lpc_mec.c +++ b/drivers/platform/chrome/cros_ec_lpc_mec.c @@ -94,7 +94,7 @@ static void cros_ec_lpc_mec_emi_write_address(u16 addr, int cros_ec_lpc_mec_in_range(unsigned int offset, unsigned int length) { if (length == 0) - return -EINVAL; + return 0; if (WARN_ON(mec_emi_base == 0 || mec_emi_end == 0)) return -EINVAL; But I don't like how subtle that is. Probably adding a check for for if (length == 0) to the to cros_ec_lpc_mec_read_bytes() seems like the best option. I guess option 2 is the best option. So far as I can see this is the only caller which passes "length == 0" is in cros_ec_cmd_xfer_lpc(). /* Read response and update checksum */ ret = cros_ec_lpc_ops.read(EC_LPC_ADDR_HOST_PARAM, args.data_size, ^^^^^^^^^^^^^^^ msg->data); regards, dan carpenter