[PATCH] platform/chrome: cros_ec_lpc: Fix error code in cros_ec_lpc_mec_read_bytes()

[PATCH] platform/chrome: cros_ec_lpc: Fix error code in cros_ec_lpc_mec_read_bytes()

[Dan Carpenter]

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 <dan.carpenter@linaro.org>
---
 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

Re: [PATCH] platform/chrome: cros_ec_lpc: Fix error code in cros_ec_lpc_mec_read_bytes()

[Guenter Roeck]

On Thu, Jun 13, 2024 at 6:55 AM Dan Carpenter <dan.carpenter@linaro.org> 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'.
>
> 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 <dan.carpenter@linaro.org>

Reviewed-by: Guenter Roeck <groeck@chromium.org>

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

Re: [PATCH] platform/chrome: cros_ec_lpc: Fix error code in cros_ec_lpc_mec_read_bytes()

[patchwork-bot+chrome-platform]

Hello:

This patch was applied to chrome-platform/linux.git (for-kernelci)
by Tzung-Bi Shih <tzungbi@kernel.org>:

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

Re: [PATCH] platform/chrome: cros_ec_lpc: Fix error code in cros_ec_lpc_mec_read_bytes()

[patchwork-bot+chrome-platform]

Hello:

This patch was applied to chrome-platform/linux.git (for-next)
by Tzung-Bi Shih <tzungbi@kernel.org>:

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

Re: [PATCH] platform/chrome: cros_ec_lpc: Fix error code in cros_ec_lpc_mec_read_bytes()

[Ben Walsh]

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 <dan.carpenter@linaro.org> 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 <dan.carpenter@linaro.org>
> ---
>  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

Re: [PATCH] platform/chrome: cros_ec_lpc: Fix error code in cros_ec_lpc_mec_read_bytes()

[Tzung-Bi Shih]

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 <dan.carpenter@linaro.org> 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 <dan.carpenter@linaro.org>
> > ---
> >  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
> 

Re: [PATCH] platform/chrome: cros_ec_lpc: Fix error code in cros_ec_lpc_mec_read_bytes()

[Ben Walsh]

Tzung-Bi Shih <tzungbi@kernel.org> 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 <dan.carpenter@linaro.org> 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 <dan.carpenter@linaro.org>
>> > ---
>> >  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
>> 

Re: [PATCH] platform/chrome: cros_ec_lpc: Fix error code in cros_ec_lpc_mec_read_bytes()

[Tzung-Bi Shih]

On Thu, Jun 13, 2024 at 08:14:14PM +0100, Ben Walsh wrote:
> Tzung-Bi Shih <tzungbi@kernel.org> 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.

Re: [PATCH] platform/chrome: cros_ec_lpc: Fix error code in cros_ec_lpc_mec_read_bytes()

[Ben Walsh]

Tzung-Bi Shih <tzungbi@kernel.org> 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 <dan.carpenter@linaro.org> 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 <dan.carpenter@linaro.org>
>> > ---
>> >  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
>> 

Re: [PATCH] platform/chrome: cros_ec_lpc: Fix error code in cros_ec_lpc_mec_read_bytes()

[Dan Carpenter]

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

Re: [PATCH] platform/chrome: cros_ec_lpc: Fix error code in cros_ec_lpc_mec_read_bytes()

[Ben Walsh]

Dan Carpenter <dan.carpenter@linaro.org> 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.

Re: [PATCH] platform/chrome: cros_ec_lpc: Fix error code in cros_ec_lpc_mec_read_bytes()

[Dan Carpenter]

On Thu, Jun 13, 2024 at 07:20:32PM +0100, Ben Walsh wrote:
> 
> Dan Carpenter <dan.carpenter@linaro.org> 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

Re: [PATCH] platform/chrome: cros_ec_lpc: Fix error code in cros_ec_lpc_mec_read_bytes()

[Ben Walsh]

Dan Carpenter <dan.carpenter@linaro.org> 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!