We changed these functions to returning negative error codes, but this
first error path was accidentally overlooked. It leads to a Smatch
warning:
drivers/platform/chrome/cros_ec_lpc.c:181 ec_response_timed_out()
error: uninitialized symbol 'data'.
Fix this by returning the error code instead of success.
Fixes: 68dbac0a58ef ("platform/chrome: cros_ec_lpc: MEC access can return error code")
Signed-off-by: Dan Carpenter <[email protected]>
---
drivers/platform/chrome/cros_ec_lpc.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/platform/chrome/cros_ec_lpc.c b/drivers/platform/chrome/cros_ec_lpc.c
index ebe9fb143840..f0470248b109 100644
--- a/drivers/platform/chrome/cros_ec_lpc.c
+++ b/drivers/platform/chrome/cros_ec_lpc.c
@@ -139,7 +139,7 @@ static int cros_ec_lpc_mec_read_bytes(unsigned int offset, unsigned int length,
int in_range = cros_ec_lpc_mec_in_range(offset, length);
if (in_range < 0)
- return 0;
+ return in_range;
return in_range ?
cros_ec_lpc_io_bytes_mec(MEC_IO_READ,
@@ -158,7 +158,7 @@ static int cros_ec_lpc_mec_write_bytes(unsigned int offset, unsigned int length,
int in_range = cros_ec_lpc_mec_in_range(offset, length);
if (in_range < 0)
- return 0;
+ return in_range;
return in_range ?
cros_ec_lpc_io_bytes_mec(MEC_IO_WRITE,
--
2.43.0
Hello:
This patch was applied to chrome-platform/linux.git (for-kernelci)
by Tzung-Bi Shih <[email protected]>:
On Thu, 13 Jun 2024 16:55:14 +0300 you wrote:
> We changed these functions to returning negative error codes, but this
> first error path was accidentally overlooked. It leads to a Smatch
> warning:
>
> drivers/platform/chrome/cros_ec_lpc.c:181 ec_response_timed_out()
> error: uninitialized symbol 'data'.
>
> [...]
Here is the summary with links:
- platform/chrome: cros_ec_lpc: Fix error code in cros_ec_lpc_mec_read_bytes()
https://git.kernel.org/chrome-platform/c/77a714325d09
You are awesome, thank you!
--
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html
Hello:
This patch was applied to chrome-platform/linux.git (for-next)
by Tzung-Bi Shih <[email protected]>:
On Thu, 13 Jun 2024 16:55:14 +0300 you wrote:
> We changed these functions to returning negative error codes, but this
> first error path was accidentally overlooked. It leads to a Smatch
> warning:
>
> drivers/platform/chrome/cros_ec_lpc.c:181 ec_response_timed_out()
> error: uninitialized symbol 'data'.
>
> [...]
Here is the summary with links:
- platform/chrome: cros_ec_lpc: Fix error code in cros_ec_lpc_mec_read_bytes()
https://git.kernel.org/chrome-platform/c/77a714325d09
You are awesome, thank you!
--
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html
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.
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?
Dan Carpenter <[email protected]> writes:
> We changed these functions to returning negative error codes, but this
> first error path was accidentally overlooked. It leads to a Smatch
> warning:
>
> drivers/platform/chrome/cros_ec_lpc.c:181 ec_response_timed_out()
> error: uninitialized symbol 'data'.
>
> Fix this by returning the error code instead of success.
>
> Fixes: 68dbac0a58ef ("platform/chrome: cros_ec_lpc: MEC access can return error code")
> Signed-off-by: Dan Carpenter <[email protected]>
> ---
> drivers/platform/chrome/cros_ec_lpc.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/platform/chrome/cros_ec_lpc.c b/drivers/platform/chrome/cros_ec_lpc.c
> index ebe9fb143840..f0470248b109 100644
> --- a/drivers/platform/chrome/cros_ec_lpc.c
> +++ b/drivers/platform/chrome/cros_ec_lpc.c
> @@ -139,7 +139,7 @@ static int cros_ec_lpc_mec_read_bytes(unsigned int offset, unsigned int length,
> int in_range = cros_ec_lpc_mec_in_range(offset, length);
>
> if (in_range < 0)
> - return 0;
> + return in_range;
>
> return in_range ?
> cros_ec_lpc_io_bytes_mec(MEC_IO_READ,
> @@ -158,7 +158,7 @@ static int cros_ec_lpc_mec_write_bytes(unsigned int offset, unsigned int length,
> int in_range = cros_ec_lpc_mec_in_range(offset, length);
>
> if (in_range < 0)
> - return 0;
> + return in_range;
>
> return in_range ?
> cros_ec_lpc_io_bytes_mec(MEC_IO_WRITE,
> --
> 2.43.0
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.
Somewhere like [1] could accidentally get the -EINVAL.
[1]: https://elixir.bootlin.com/linux/v6.9/source/drivers/platform/chrome/cros_ec_lpc.c#L232
>
> 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?
How about drop the length check at [2]?
[2]: https://elixir.bootlin.com/linux/v6.9/source/drivers/platform/chrome/cros_ec_lpc_mec.c#L44
>
> Dan Carpenter <[email protected]> writes:
>
> > We changed these functions to returning negative error codes, but this
> > first error path was accidentally overlooked. It leads to a Smatch
> > warning:
> >
> > drivers/platform/chrome/cros_ec_lpc.c:181 ec_response_timed_out()
> > error: uninitialized symbol 'data'.
> >
> > Fix this by returning the error code instead of success.
> >
> > Fixes: 68dbac0a58ef ("platform/chrome: cros_ec_lpc: MEC access can return error code")
> > Signed-off-by: Dan Carpenter <[email protected]>
> > ---
> > drivers/platform/chrome/cros_ec_lpc.c | 4 ++--
> > 1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/platform/chrome/cros_ec_lpc.c b/drivers/platform/chrome/cros_ec_lpc.c
> > index ebe9fb143840..f0470248b109 100644
> > --- a/drivers/platform/chrome/cros_ec_lpc.c
> > +++ b/drivers/platform/chrome/cros_ec_lpc.c
> > @@ -139,7 +139,7 @@ static int cros_ec_lpc_mec_read_bytes(unsigned int offset, unsigned int length,
> > int in_range = cros_ec_lpc_mec_in_range(offset, length);
> >
> > if (in_range < 0)
> > - return 0;
> > + return in_range;
> >
> > return in_range ?
> > cros_ec_lpc_io_bytes_mec(MEC_IO_READ,
> > @@ -158,7 +158,7 @@ static int cros_ec_lpc_mec_write_bytes(unsigned int offset, unsigned int length,
> > int in_range = cros_ec_lpc_mec_in_range(offset, length);
> >
> > if (in_range < 0)
> > - return 0;
> > + return in_range;
> >
> > return in_range ?
> > cros_ec_lpc_io_bytes_mec(MEC_IO_WRITE,
> > --
> > 2.43.0
>
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
Dan Carpenter <[email protected]> writes:
> 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.
Sorry, I mean cros_ec_lpc_mec_in_range().
> 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.
Thanks. I'll check out Tzung-Bi's suggestions as well before we decide.
On Thu, Jun 13, 2024 at 07:20:32PM +0100, Ben Walsh wrote:
>
> Dan Carpenter <[email protected]> writes:
>
> > 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.
>
> Sorry, I mean cros_ec_lpc_mec_in_range().
>
> > 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.
>
> Thanks. I'll check out Tzung-Bi's suggestions as well before we decide.
Writing length 0 bytes to cros_ec_lpc_io_bytes_mec() changes the
function to basically this:
cros_ec_lpc_mec_lock();
/* Initialize I/O at desired address */
cros_ec_lpc_mec_emi_write_address(offset, access);
cros_ec_lpc_mec_unlock();
return 0;
I was a little concerned about the cros_ec_lpc_mec_emi_write_address()
But I don't know this subsystem at all so it might be fine.
Perhaps the cleanest thing is to delete the length == 0 check in
cros_ec_lpc_mec_in_range() and add one to the start of
cros_ec_lpc_io_bytes_mec().
I think that's a good solution.
regards,
dan carpenter
Tzung-Bi Shih <[email protected]> writes:
> 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.
>
> Somewhere like [1] could accidentally get the -EINVAL.
>
> [1]: https://elixir.bootlin.com/linux/v6.9/source/drivers/platform/chrome/cros_ec_lpc.c#L232
Yes. It turns out I'm getting it in:
cros_ec_query_all -> cros_ec_proto_info -> ... -> cros_ec_pkt_xfer_lpc
/* Read response and update checksum */
ret = cros_ec_lpc_ops.read(EC_LPC_ADDR_HOST_PARAM, args.data_size,
^^^^^^^^^^^^^^^
msg->data);
(as Dan suggested in his email).
>> 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?
>
> How about drop the length check at [2]?
>
> [2]: https://elixir.bootlin.com/linux/v6.9/source/drivers/platform/chrome/cros_ec_lpc_mec.c#L44
>
This works, but we still end up calling cros_ec_lpc_io_bytes_mec() with
zero length. Although this seems to work fine, we could put a length
check at the top of cros_ec_lpc_read_bytes() to avoid it.
>>
>> Dan Carpenter <[email protected]> writes:
>>
>> > We changed these functions to returning negative error codes, but this
>> > first error path was accidentally overlooked. It leads to a Smatch
>> > warning:
>> >
>> > drivers/platform/chrome/cros_ec_lpc.c:181 ec_response_timed_out()
>> > error: uninitialized symbol 'data'.
>> >
>> > Fix this by returning the error code instead of success.
>> >
>> > Fixes: 68dbac0a58ef ("platform/chrome: cros_ec_lpc: MEC access can return error code")
>> > Signed-off-by: Dan Carpenter <[email protected]>
>> > ---
>> > drivers/platform/chrome/cros_ec_lpc.c | 4 ++--
>> > 1 file changed, 2 insertions(+), 2 deletions(-)
>> >
>> > diff --git a/drivers/platform/chrome/cros_ec_lpc.c b/drivers/platform/chrome/cros_ec_lpc.c
>> > index ebe9fb143840..f0470248b109 100644
>> > --- a/drivers/platform/chrome/cros_ec_lpc.c
>> > +++ b/drivers/platform/chrome/cros_ec_lpc.c
>> > @@ -139,7 +139,7 @@ static int cros_ec_lpc_mec_read_bytes(unsigned int offset, unsigned int length,
>> > int in_range = cros_ec_lpc_mec_in_range(offset, length);
>> >
>> > if (in_range < 0)
>> > - return 0;
>> > + return in_range;
>> >
>> > return in_range ?
>> > cros_ec_lpc_io_bytes_mec(MEC_IO_READ,
>> > @@ -158,7 +158,7 @@ static int cros_ec_lpc_mec_write_bytes(unsigned int offset, unsigned int length,
>> > int in_range = cros_ec_lpc_mec_in_range(offset, length);
>> >
>> > if (in_range < 0)
>> > - return 0;
>> > + return in_range;
>> >
>> > return in_range ?
>> > cros_ec_lpc_io_bytes_mec(MEC_IO_WRITE,
>> > --
>> > 2.43.0
>>
On Thu, Jun 13, 2024 at 08:14:14PM +0100, Ben Walsh wrote:
> Tzung-Bi Shih <[email protected]> writes:
> > On Thu, Jun 13, 2024 at 05:51:39PM +0100, Ben Walsh wrote:
> >> 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?
> >
> > How about drop the length check at [2]?
> >
> > [2]: https://elixir.bootlin.com/linux/v6.9/source/drivers/platform/chrome/cros_ec_lpc_mec.c#L44
> >
>
> This works, but we still end up calling cros_ec_lpc_io_bytes_mec() with
> zero length. Although this seems to work fine, we could put a length
> check at the top of cros_ec_lpc_read_bytes() to avoid it.
I guess you mean: cros_ec_lpc_io_bytes_mec(). Ack.
Dan Carpenter <[email protected]> writes:
> Perhaps the cleanest thing is to delete the length == 0 check in
> cros_ec_lpc_mec_in_range() and add one to the start of
> cros_ec_lpc_io_bytes_mec().
>
> I think that's a good solution.
I think it's a good solution too. I'll send a patch. Thanks!
Tzung-Bi Shih <[email protected]> writes:
> Somewhere like [1] could accidentally get the -EINVAL.
>
> [1]: https://elixir.bootlin.com/linux/v6.9/source/drivers/platform/chrome/cros_ec_lpc.c#L232
Sorry, it happens at:
cros_ec_query_all -> cros_ec_proto_info -> ... -> cros_ec_pkt_xfer_lpc
/* Read response and process checksum */
ret = fwk_ec_lpc_ops.read(EC_LPC_ADDR_HOST_PACKET +
sizeof(response), response.data_len,
^^^^^^^^^^^^^^^^^
msg->data);
>>
>> Dan Carpenter <[email protected]> writes:
>>
>> > We changed these functions to returning negative error codes, but this
>> > first error path was accidentally overlooked. It leads to a Smatch
>> > warning:
>> >
>> > drivers/platform/chrome/cros_ec_lpc.c:181 ec_response_timed_out()
>> > error: uninitialized symbol 'data'.
>> >
>> > Fix this by returning the error code instead of success.
>> >
>> > Fixes: 68dbac0a58ef ("platform/chrome: cros_ec_lpc: MEC access can return error code")
>> > Signed-off-by: Dan Carpenter <[email protected]>
>> > ---
>> > drivers/platform/chrome/cros_ec_lpc.c | 4 ++--
>> > 1 file changed, 2 insertions(+), 2 deletions(-)
>> >
>> > diff --git a/drivers/platform/chrome/cros_ec_lpc.c b/drivers/platform/chrome/cros_ec_lpc.c
>> > index ebe9fb143840..f0470248b109 100644
>> > --- a/drivers/platform/chrome/cros_ec_lpc.c
>> > +++ b/drivers/platform/chrome/cros_ec_lpc.c
>> > @@ -139,7 +139,7 @@ static int cros_ec_lpc_mec_read_bytes(unsigned int offset, unsigned int length,
>> > int in_range = cros_ec_lpc_mec_in_range(offset, length);
>> >
>> > if (in_range < 0)
>> > - return 0;
>> > + return in_range;
>> >
>> > return in_range ?
>> > cros_ec_lpc_io_bytes_mec(MEC_IO_READ,
>> > @@ -158,7 +158,7 @@ static int cros_ec_lpc_mec_write_bytes(unsigned int offset, unsigned int length,
>> > int in_range = cros_ec_lpc_mec_in_range(offset, length);
>> >
>> > if (in_range < 0)
>> > - return 0;
>> > + return in_range;
>> >
>> > return in_range ?
>> > cros_ec_lpc_io_bytes_mec(MEC_IO_WRITE,
>> > --
>> > 2.43.0
>>