[PATCH] platform/chrome: cros_ec_proto: Fix check_features ret val

[PATCH] platform/chrome: cros_ec_proto: Fix check_features ret val

[Prashant Malani]

The kerneldoc for cros_ec_check_features() states that it returns 1 or 0
depedending on whether a feature is supported or not, but it instead
returns a negative error number in one case, and a non-1 bitmask in
other cases.

Since all call-sites only check for a 1 or 0 return value, update
the function to return boolean values.

Signed-off-by: Prashant Malani <pmalani@chromium.org>
---
 drivers/platform/chrome/cros_ec_proto.c     | 12 +++++++-----
 include/linux/platform_data/cros_ec_proto.h |  2 +-
 2 files changed, 8 insertions(+), 6 deletions(-)

diff --git a/drivers/platform/chrome/cros_ec_proto.c b/drivers/platform/chrome/cros_ec_proto.c
index a7404d69b2d3..a34cf58c5ef7 100644
--- a/drivers/platform/chrome/cros_ec_proto.c
+++ b/drivers/platform/chrome/cros_ec_proto.c
@@ -808,9 +808,9 @@ EXPORT_SYMBOL(cros_ec_get_host_event);
  *
  * Call this function to test whether the ChromeOS EC supports a feature.
  *
- * Return: 1 if supported, 0 if not
+ * Return: true if supported, false if not (or if an error was encountered).
  */
-int cros_ec_check_features(struct cros_ec_dev *ec, int feature)
+bool cros_ec_check_features(struct cros_ec_dev *ec, int feature)
 {
 	struct cros_ec_command *msg;
 	int ret;
@@ -818,8 +818,10 @@ int cros_ec_check_features(struct cros_ec_dev *ec, int feature)
 	if (ec->features[0] == -1U && ec->features[1] == -1U) {
 		/* features bitmap not read yet */
 		msg = kzalloc(sizeof(*msg) + sizeof(ec->features), GFP_KERNEL);
-		if (!msg)
-			return -ENOMEM;
+		if (!msg) {
+			dev_err(ec->dev, "failed to allocate memory to get EC features\n");
+			return false;
+		}
 
 		msg->command = EC_CMD_GET_FEATURES + ec->cmd_offset;
 		msg->insize = sizeof(ec->features);
@@ -839,7 +841,7 @@ int cros_ec_check_features(struct cros_ec_dev *ec, int feature)
 		kfree(msg);
 	}
 
-	return ec->features[feature / 32] & EC_FEATURE_MASK_0(feature);
+	return !!(ec->features[feature / 32] & EC_FEATURE_MASK_0(feature));
 }
 EXPORT_SYMBOL_GPL(cros_ec_check_features);
 
diff --git a/include/linux/platform_data/cros_ec_proto.h b/include/linux/platform_data/cros_ec_proto.h
index 02599687770c..55844ece0b32 100644
--- a/include/linux/platform_data/cros_ec_proto.h
+++ b/include/linux/platform_data/cros_ec_proto.h
@@ -227,7 +227,7 @@ int cros_ec_get_next_event(struct cros_ec_device *ec_dev,
 
 u32 cros_ec_get_host_event(struct cros_ec_device *ec_dev);
 
-int cros_ec_check_features(struct cros_ec_dev *ec, int feature);
+bool cros_ec_check_features(struct cros_ec_dev *ec, int feature);
 
 int cros_ec_get_sensor_count(struct cros_ec_dev *ec);
 
-- 
2.33.0.309.g3052b89438-goog

Re: [PATCH] platform/chrome: cros_ec_proto: Fix check_features ret val

[Guenter Roeck]

On Wed, Sep 15, 2021 at 6:47 PM Prashant Malani <pmalani@chromium.org> wrote:
>
> The kerneldoc for cros_ec_check_features() states that it returns 1 or 0
> depedending on whether a feature is supported or not, but it instead
> returns a negative error number in one case, and a non-1 bitmask in
> other cases.
>
> Since all call-sites only check for a 1 or 0 return value, update
> the function to return boolean values.
>
> Signed-off-by: Prashant Malani <pmalani@chromium.org>

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

> ---
>  drivers/platform/chrome/cros_ec_proto.c     | 12 +++++++-----
>  include/linux/platform_data/cros_ec_proto.h |  2 +-
>  2 files changed, 8 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/platform/chrome/cros_ec_proto.c b/drivers/platform/chrome/cros_ec_proto.c
> index a7404d69b2d3..a34cf58c5ef7 100644
> --- a/drivers/platform/chrome/cros_ec_proto.c
> +++ b/drivers/platform/chrome/cros_ec_proto.c
> @@ -808,9 +808,9 @@ EXPORT_SYMBOL(cros_ec_get_host_event);
>   *
>   * Call this function to test whether the ChromeOS EC supports a feature.
>   *
> - * Return: 1 if supported, 0 if not
> + * Return: true if supported, false if not (or if an error was encountered).
>   */
> -int cros_ec_check_features(struct cros_ec_dev *ec, int feature)
> +bool cros_ec_check_features(struct cros_ec_dev *ec, int feature)
>  {
>         struct cros_ec_command *msg;
>         int ret;
> @@ -818,8 +818,10 @@ int cros_ec_check_features(struct cros_ec_dev *ec, int feature)
>         if (ec->features[0] == -1U && ec->features[1] == -1U) {
>                 /* features bitmap not read yet */
>                 msg = kzalloc(sizeof(*msg) + sizeof(ec->features), GFP_KERNEL);
> -               if (!msg)
> -                       return -ENOMEM;
> +               if (!msg) {
> +                       dev_err(ec->dev, "failed to allocate memory to get EC features\n");
> +                       return false;
> +               }
>
>                 msg->command = EC_CMD_GET_FEATURES + ec->cmd_offset;
>                 msg->insize = sizeof(ec->features);
> @@ -839,7 +841,7 @@ int cros_ec_check_features(struct cros_ec_dev *ec, int feature)
>                 kfree(msg);
>         }
>
> -       return ec->features[feature / 32] & EC_FEATURE_MASK_0(feature);
> +       return !!(ec->features[feature / 32] & EC_FEATURE_MASK_0(feature));
>  }
>  EXPORT_SYMBOL_GPL(cros_ec_check_features);
>
> diff --git a/include/linux/platform_data/cros_ec_proto.h b/include/linux/platform_data/cros_ec_proto.h
> index 02599687770c..55844ece0b32 100644
> --- a/include/linux/platform_data/cros_ec_proto.h
> +++ b/include/linux/platform_data/cros_ec_proto.h
> @@ -227,7 +227,7 @@ int cros_ec_get_next_event(struct cros_ec_device *ec_dev,
>
>  u32 cros_ec_get_host_event(struct cros_ec_device *ec_dev);
>
> -int cros_ec_check_features(struct cros_ec_dev *ec, int feature);
> +bool cros_ec_check_features(struct cros_ec_dev *ec, int feature);
>
>  int cros_ec_get_sensor_count(struct cros_ec_dev *ec);
>
> --
> 2.33.0.309.g3052b89438-goog
>

Re: [PATCH] platform/chrome: cros_ec_proto: Fix check_features ret val

[Enric Balletbo i Serra]

Hi Prashant,

Thank you for the patch. Just one comment below ...

On 16/9/21 3:46, Prashant Malani wrote:
> The kerneldoc for cros_ec_check_features() states that it returns 1 or 0
> depedending on whether a feature is supported or not, but it instead
> returns a negative error number in one case, and a non-1 bitmask in
> other cases.
> 
> Since all call-sites only check for a 1 or 0 return value, update
> the function to return boolean values.
> 
> Signed-off-by: Prashant Malani <pmalani@chromium.org>
> ---
>  drivers/platform/chrome/cros_ec_proto.c     | 12 +++++++-----
>  include/linux/platform_data/cros_ec_proto.h |  2 +-
>  2 files changed, 8 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/platform/chrome/cros_ec_proto.c b/drivers/platform/chrome/cros_ec_proto.c
> index a7404d69b2d3..a34cf58c5ef7 100644
> --- a/drivers/platform/chrome/cros_ec_proto.c
> +++ b/drivers/platform/chrome/cros_ec_proto.c
> @@ -808,9 +808,9 @@ EXPORT_SYMBOL(cros_ec_get_host_event);
>   *
>   * Call this function to test whether the ChromeOS EC supports a feature.
>   *
> - * Return: 1 if supported, 0 if not
> + * Return: true if supported, false if not (or if an error was encountered).
>   */
> -int cros_ec_check_features(struct cros_ec_dev *ec, int feature)
> +bool cros_ec_check_features(struct cros_ec_dev *ec, int feature)
>  {
>  	struct cros_ec_command *msg;
>  	int ret;
> @@ -818,8 +818,10 @@ int cros_ec_check_features(struct cros_ec_dev *ec, int feature)
>  	if (ec->features[0] == -1U && ec->features[1] == -1U) {
>  		/* features bitmap not read yet */
>  		msg = kzalloc(sizeof(*msg) + sizeof(ec->features), GFP_KERNEL);
> -		if (!msg)
> -			return -ENOMEM;
> +		if (!msg) {
> +			dev_err(ec->dev, "failed to allocate memory to get EC features\n");

In case of failure you will be noticed by the allocator, prints after
[k|v][m|z|c]alloc() functions are not needed, so I think you can just return
false here.

Other than that the patch looks good to me.

Thanks,
  Enric

> +			return false;
> +		}
>  
>  		msg->command = EC_CMD_GET_FEATURES + ec->cmd_offset;
>  		msg->insize = sizeof(ec->features);
> @@ -839,7 +841,7 @@ int cros_ec_check_features(struct cros_ec_dev *ec, int feature)
>  		kfree(msg);
>  	}
>  
> -	return ec->features[feature / 32] & EC_FEATURE_MASK_0(feature);
> +	return !!(ec->features[feature / 32] & EC_FEATURE_MASK_0(feature));
>  }
>  EXPORT_SYMBOL_GPL(cros_ec_check_features);
>  
> diff --git a/include/linux/platform_data/cros_ec_proto.h b/include/linux/platform_data/cros_ec_proto.h
> index 02599687770c..55844ece0b32 100644
> --- a/include/linux/platform_data/cros_ec_proto.h
> +++ b/include/linux/platform_data/cros_ec_proto.h
> @@ -227,7 +227,7 @@ int cros_ec_get_next_event(struct cros_ec_device *ec_dev,
>  
>  u32 cros_ec_get_host_event(struct cros_ec_device *ec_dev);
>  
> -int cros_ec_check_features(struct cros_ec_dev *ec, int feature);
> +bool cros_ec_check_features(struct cros_ec_dev *ec, int feature);
>  
>  int cros_ec_get_sensor_count(struct cros_ec_dev *ec);
>  
> 

Re: [PATCH] platform/chrome: cros_ec_proto: Fix check_features ret val

[Prashant Malani]

Hi Enric,

Thanks for reviewing the patch.

On Tue, Sep 21, 2021 at 01:42:04PM +0200, Enric Balletbo i Serra wrote:
> Hi Prashant,
> 
> Thank you for the patch. Just one comment below ...
> 
> On 16/9/21 3:46, Prashant Malani wrote:
> > The kerneldoc for cros_ec_check_features() states that it returns 1 or 0
> > depedending on whether a feature is supported or not, but it instead
> > returns a negative error number in one case, and a non-1 bitmask in
> > other cases.
> > 
> > Since all call-sites only check for a 1 or 0 return value, update
> > the function to return boolean values.
> > 
> > Signed-off-by: Prashant Malani <pmalani@chromium.org>
> > ---
> >  drivers/platform/chrome/cros_ec_proto.c     | 12 +++++++-----
> >  include/linux/platform_data/cros_ec_proto.h |  2 +-
> >  2 files changed, 8 insertions(+), 6 deletions(-)
> > 
> > diff --git a/drivers/platform/chrome/cros_ec_proto.c b/drivers/platform/chrome/cros_ec_proto.c
> > index a7404d69b2d3..a34cf58c5ef7 100644
> > --- a/drivers/platform/chrome/cros_ec_proto.c
> > +++ b/drivers/platform/chrome/cros_ec_proto.c
> > @@ -808,9 +808,9 @@ EXPORT_SYMBOL(cros_ec_get_host_event);
> >   *
> >   * Call this function to test whether the ChromeOS EC supports a feature.
> >   *
> > - * Return: 1 if supported, 0 if not
> > + * Return: true if supported, false if not (or if an error was encountered).
> >   */
> > -int cros_ec_check_features(struct cros_ec_dev *ec, int feature)
> > +bool cros_ec_check_features(struct cros_ec_dev *ec, int feature)
> >  {
> >  	struct cros_ec_command *msg;
> >  	int ret;
> > @@ -818,8 +818,10 @@ int cros_ec_check_features(struct cros_ec_dev *ec, int feature)
> >  	if (ec->features[0] == -1U && ec->features[1] == -1U) {
> >  		/* features bitmap not read yet */
> >  		msg = kzalloc(sizeof(*msg) + sizeof(ec->features), GFP_KERNEL);
> > -		if (!msg)
> > -			return -ENOMEM;
> > +		if (!msg) {
> > +			dev_err(ec->dev, "failed to allocate memory to get EC features\n");
> 
> In case of failure you will be noticed by the allocator, prints after
> [k|v][m|z|c]alloc() functions are not needed, so I think you can just return
> false here.
> > 

Makes sense; I can make the change, but I had one question:

If we solely return false, how will we tell from the logs that the
allocation error message was associated with this driver? Only returning
false means the driver probe (e.g cros-ec-typec) will continue (just assuming a certain feature
is not present). Wouldn't having this error message make this clear?

Best regards,

-Prashant

Re: [PATCH] platform/chrome: cros_ec_proto: Fix check_features ret val

[Enric Balletbo i Serra]

Hi Prashant,

On 21/9/21 19:29, Prashant Malani wrote:
> Hi Enric,
> 
> Thanks for reviewing the patch.
> 
> On Tue, Sep 21, 2021 at 01:42:04PM +0200, Enric Balletbo i Serra wrote:
>> Hi Prashant,
>>
>> Thank you for the patch. Just one comment below ...
>>
>> On 16/9/21 3:46, Prashant Malani wrote:
>>> The kerneldoc for cros_ec_check_features() states that it returns 1 or 0
>>> depedending on whether a feature is supported or not, but it instead
>>> returns a negative error number in one case, and a non-1 bitmask in
>>> other cases.
>>>
>>> Since all call-sites only check for a 1 or 0 return value, update
>>> the function to return boolean values.
>>>
>>> Signed-off-by: Prashant Malani <pmalani@chromium.org>
>>> ---
>>>  drivers/platform/chrome/cros_ec_proto.c     | 12 +++++++-----
>>>  include/linux/platform_data/cros_ec_proto.h |  2 +-
>>>  2 files changed, 8 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/drivers/platform/chrome/cros_ec_proto.c b/drivers/platform/chrome/cros_ec_proto.c
>>> index a7404d69b2d3..a34cf58c5ef7 100644
>>> --- a/drivers/platform/chrome/cros_ec_proto.c
>>> +++ b/drivers/platform/chrome/cros_ec_proto.c
>>> @@ -808,9 +808,9 @@ EXPORT_SYMBOL(cros_ec_get_host_event);
>>>   *
>>>   * Call this function to test whether the ChromeOS EC supports a feature.
>>>   *
>>> - * Return: 1 if supported, 0 if not
>>> + * Return: true if supported, false if not (or if an error was encountered).
>>>   */
>>> -int cros_ec_check_features(struct cros_ec_dev *ec, int feature)
>>> +bool cros_ec_check_features(struct cros_ec_dev *ec, int feature)
>>>  {
>>>  	struct cros_ec_command *msg;
>>>  	int ret;
>>> @@ -818,8 +818,10 @@ int cros_ec_check_features(struct cros_ec_dev *ec, int feature)
>>>  	if (ec->features[0] == -1U && ec->features[1] == -1U) {
>>>  		/* features bitmap not read yet */
>>>  		msg = kzalloc(sizeof(*msg) + sizeof(ec->features), GFP_KERNEL);
>>> -		if (!msg)
>>> -			return -ENOMEM;
>>> +		if (!msg) {
>>> +			dev_err(ec->dev, "failed to allocate memory to get EC features\n");
>>
>> In case of failure you will be noticed by the allocator, prints after
>> [k|v][m|z|c]alloc() functions are not needed, so I think you can just return
>> false here.
>>>
> 
> Makes sense; I can make the change, but I had one question:
> 
> If we solely return false, how will we tell from the logs that the
> allocation error message was associated with this driver? Only returning
> false means the driver probe (e.g cros-ec-typec) will continue (just assuming a certain feature
> is not present). Wouldn't having this error message make this clear?
> 

So I tried to find some doc about this without luck. But I think it has been an
unwritten rule that GFP_KERNEL allocations for small allocations will never
fail. If you system fails to allocate that small amount of memory you probably
have bigger problems to solve and the above message is not really useful, even
confusing, as the focus, likely, shouldn't be in this driver to solve the problem.

Thinking a bit more about this change, and after your question, I don't really
like functions not returning an error in the unlikely case that fails. On the
other hand, I like this function return a bool as is a bit more clear IMO, so
I'm wondering if wouldn't be better don't use dynamic memory here (I know that
this is not really related to your patch)

And another thing that I detected, now that you're returning a bool is that in
drivers/platform/chrome/cros_ec_typec.c there is:

   typec->typec_cmd_supported = !!cros_ec_check_features(ec_dev,
   typec->needs_mux_ack = !!cros_ec_check_features(ec_dev,

I think that you can remove the !! now. That could be in another patch.

Thanks,
  Enric

> Best regards,
> 
> -Prashant
> 

Re: [PATCH] platform/chrome: cros_ec_proto: Fix check_features ret val

[Prashant Malani]

Hi Enric,

On Wed, Sep 22, 2021 at 2:12 AM Enric Balletbo i Serra
<enric.balletbo@collabora.com> wrote:
>
> Hi Prashant,
>
> On 21/9/21 19:29, Prashant Malani wrote:
> > Hi Enric,
> >
> > Thanks for reviewing the patch.
> >
> > On Tue, Sep 21, 2021 at 01:42:04PM +0200, Enric Balletbo i Serra wrote:
> >> Hi Prashant,
> >>
> >> Thank you for the patch. Just one comment below ...
> >>
> >> On 16/9/21 3:46, Prashant Malani wrote:
> >>> The kerneldoc for cros_ec_check_features() states that it returns 1 or 0
> >>> depedending on whether a feature is supported or not, but it instead
> >>> returns a negative error number in one case, and a non-1 bitmask in
> >>> other cases.
> >>>
> >>> Since all call-sites only check for a 1 or 0 return value, update
> >>> the function to return boolean values.
> >>>
> >>> Signed-off-by: Prashant Malani <pmalani@chromium.org>
> >>> ---
> >>>  drivers/platform/chrome/cros_ec_proto.c     | 12 +++++++-----
> >>>  include/linux/platform_data/cros_ec_proto.h |  2 +-
> >>>  2 files changed, 8 insertions(+), 6 deletions(-)
> >>>
> >>> diff --git a/drivers/platform/chrome/cros_ec_proto.c b/drivers/platform/chrome/cros_ec_proto.c
> >>> index a7404d69b2d3..a34cf58c5ef7 100644
> >>> --- a/drivers/platform/chrome/cros_ec_proto.c
> >>> +++ b/drivers/platform/chrome/cros_ec_proto.c
> >>> @@ -808,9 +808,9 @@ EXPORT_SYMBOL(cros_ec_get_host_event);
> >>>   *
> >>>   * Call this function to test whether the ChromeOS EC supports a feature.
> >>>   *
> >>> - * Return: 1 if supported, 0 if not
> >>> + * Return: true if supported, false if not (or if an error was encountered).
> >>>   */
> >>> -int cros_ec_check_features(struct cros_ec_dev *ec, int feature)
> >>> +bool cros_ec_check_features(struct cros_ec_dev *ec, int feature)
> >>>  {
> >>>     struct cros_ec_command *msg;
> >>>     int ret;
> >>> @@ -818,8 +818,10 @@ int cros_ec_check_features(struct cros_ec_dev *ec, int feature)
> >>>     if (ec->features[0] == -1U && ec->features[1] == -1U) {
> >>>             /* features bitmap not read yet */
> >>>             msg = kzalloc(sizeof(*msg) + sizeof(ec->features), GFP_KERNEL);
> >>> -           if (!msg)
> >>> -                   return -ENOMEM;
> >>> +           if (!msg) {
> >>> +                   dev_err(ec->dev, "failed to allocate memory to get EC features\n");
> >>
> >> In case of failure you will be noticed by the allocator, prints after
> >> [k|v][m|z|c]alloc() functions are not needed, so I think you can just return
> >> false here.
> >>>
> >
> > Makes sense; I can make the change, but I had one question:
> >
> > If we solely return false, how will we tell from the logs that the
> > allocation error message was associated with this driver? Only returning
> > false means the driver probe (e.g cros-ec-typec) will continue (just assuming a certain feature
> > is not present). Wouldn't having this error message make this clear?
> >
>
> So I tried to find some doc about this without luck. But I think it has been an
> unwritten rule that GFP_KERNEL allocations for small allocations will never
> fail.

That might be the case, but kzalloc() still returns the error value,
so even if it is very unlikely, we
still need to handle that error.

If you system fails to allocate that small amount of memory you probably
> have bigger problems to solve and the above message is not really useful, even
> confusing, as the focus, likely, shouldn't be in this driver to solve the problem.

I don't know if I necessarily concur with that rationale ("if it
fails, there are likely bigger issues").
There could be situations (hypothetical) where a series of allocations
might lead to a failure (or this might be a transient allocation
failure),
and it might be useful to know which driver is contributing the alloc
that finally precipitates the failure.

Also, although it is very unlikely, returning true without an error
can mean the typec driver silently continues to function
wrongly assuming a feature flag to be set a certain way. That is
something we need to flag through the logs.
I certainly can't see the log message as confusing the reader of a log
any further.

>
> Thinking a bit more about this change, and after your question, I don't really
> like functions not returning an error in the unlikely case that fails. On the
> other hand, I like this function return a bool as is a bit more clear IMO, so
> I'm wondering if wouldn't be better don't use dynamic memory here (I know that
> this is not really related to your patch)

That sounds right to me. Can I make that change as a follow up patch?
I'd like to contain the scope
of this patch to just fix the return value.

>
> And another thing that I detected, now that you're returning a bool is that in
> drivers/platform/chrome/cros_ec_typec.c there is:
>
>    typec->typec_cmd_supported = !!cros_ec_check_features(ec_dev,
>    typec->needs_mux_ack = !!cros_ec_check_features(ec_dev,
>
> I think that you can remove the !! now. That could be in another patch.

Yes, I was planning on doing that in a later patch.

Best regards,

Re: [PATCH] platform/chrome: cros_ec_proto: Fix check_features ret val

[Enric Balletbo i Serra]

Hi Prashant,

On 22/9/21 12:55, Prashant Malani wrote:
> Hi Enric,
> 
> On Wed, Sep 22, 2021 at 2:12 AM Enric Balletbo i Serra
> <enric.balletbo@collabora.com> wrote:
>>
>> Hi Prashant,
>>
>> On 21/9/21 19:29, Prashant Malani wrote:
>>> Hi Enric,
>>>
>>> Thanks for reviewing the patch.
>>>
>>> On Tue, Sep 21, 2021 at 01:42:04PM +0200, Enric Balletbo i Serra wrote:
>>>> Hi Prashant,
>>>>
>>>> Thank you for the patch. Just one comment below ...
>>>>
>>>> On 16/9/21 3:46, Prashant Malani wrote:
>>>>> The kerneldoc for cros_ec_check_features() states that it returns 1 or 0
>>>>> depedending on whether a feature is supported or not, but it instead
>>>>> returns a negative error number in one case, and a non-1 bitmask in
>>>>> other cases.
>>>>>
>>>>> Since all call-sites only check for a 1 or 0 return value, update
>>>>> the function to return boolean values.
>>>>>
>>>>> Signed-off-by: Prashant Malani <pmalani@chromium.org>
>>>>> ---
>>>>>  drivers/platform/chrome/cros_ec_proto.c     | 12 +++++++-----
>>>>>  include/linux/platform_data/cros_ec_proto.h |  2 +-
>>>>>  2 files changed, 8 insertions(+), 6 deletions(-)
>>>>>
>>>>> diff --git a/drivers/platform/chrome/cros_ec_proto.c b/drivers/platform/chrome/cros_ec_proto.c
>>>>> index a7404d69b2d3..a34cf58c5ef7 100644
>>>>> --- a/drivers/platform/chrome/cros_ec_proto.c
>>>>> +++ b/drivers/platform/chrome/cros_ec_proto.c
>>>>> @@ -808,9 +808,9 @@ EXPORT_SYMBOL(cros_ec_get_host_event);
>>>>>   *
>>>>>   * Call this function to test whether the ChromeOS EC supports a feature.
>>>>>   *
>>>>> - * Return: 1 if supported, 0 if not
>>>>> + * Return: true if supported, false if not (or if an error was encountered).
>>>>>   */
>>>>> -int cros_ec_check_features(struct cros_ec_dev *ec, int feature)
>>>>> +bool cros_ec_check_features(struct cros_ec_dev *ec, int feature)
>>>>>  {
>>>>>     struct cros_ec_command *msg;
>>>>>     int ret;
>>>>> @@ -818,8 +818,10 @@ int cros_ec_check_features(struct cros_ec_dev *ec, int feature)
>>>>>     if (ec->features[0] == -1U && ec->features[1] == -1U) {
>>>>>             /* features bitmap not read yet */
>>>>>             msg = kzalloc(sizeof(*msg) + sizeof(ec->features), GFP_KERNEL);
>>>>> -           if (!msg)
>>>>> -                   return -ENOMEM;
>>>>> +           if (!msg) {
>>>>> +                   dev_err(ec->dev, "failed to allocate memory to get EC features\n");
>>>>
>>>> In case of failure you will be noticed by the allocator, prints after
>>>> [k|v][m|z|c]alloc() functions are not needed, so I think you can just return
>>>> false here.
>>>>>
>>>
>>> Makes sense; I can make the change, but I had one question:
>>>
>>> If we solely return false, how will we tell from the logs that the
>>> allocation error message was associated with this driver? Only returning
>>> false means the driver probe (e.g cros-ec-typec) will continue (just assuming a certain feature
>>> is not present). Wouldn't having this error message make this clear?
>>>
>>
>> So I tried to find some doc about this without luck. But I think it has been an
>> unwritten rule that GFP_KERNEL allocations for small allocations will never
>> fail.
> 
> That might be the case, but kzalloc() still returns the error value,
> so even if it is very unlikely, we
> still need to handle that error.
> 

I'm not saying that we don't need to handle that error, we *must* handle the
allocation errors, there is no doubts about this, but in this specific function
we're ignoring all the errors and not noticing to the users. The discussion, or
my nitpick (it's just a nit), is about if logging that the allocation failed in
this driver is useful or not.


>> If you system fails to allocate that small amount of memory you probably
>> have bigger problems to solve and the above message is not really useful, even
>> confusing, as the focus, likely, shouldn't be in this driver to solve the problem.
> 
> I don't know if I necessarily concur with that rationale ("if it
> fails, there are likely bigger issues").
> There could be situations (hypothetical) where a series of allocations
> might lead to a failure (or this might be a transient allocation
> failure),
> and it might be useful to know which driver is contributing the alloc
> that finally precipitates the failure.
> 

If the kernel is not able to give you a small amount of memory it is just a
coincidence that this happened in your driver. Dynamic allocations and
deallocations happens often, the print doesn't gives you more information as it
could be any other driver because your system runs out of memory, and everybody
can be affected.

> Also, although it is very unlikely, returning true without an error

I'm not saying this, I'm saying returning false but don't log the error to the
console.

> can mean the typec driver silently continues to function

I think the consensus on the interface is, and was clear:

1/True if the feature is supported, 0/false if is not supported or there is an
error. At some point we decided that the callers don't need to differentiate
about if the feature is not supported or there is and unlikely error.

> wrongly assuming a feature flag to be set a certain way. That is
> something we need to flag through the logs.

If you run out of memory there will be a lot of logs, trust me ;-)

> I certainly can't see the log message as confusing the reader of a log
> any further.
> 

As I said there is an unwritten rule (didn't find written doc about it tbh,
although I know other maintainers follow) that prints after [k|v][m|z|c]alloc()
functions are not needed.

And as I said, this is a nit, I won't strongly oppose if you think that this
message is really needed.

Cheers,
  Enric

>>
>> Thinking a bit more about this change, and after your question, I don't really
>> like functions not returning an error in the unlikely case that fails. On the
>> other hand, I like this function return a bool as is a bit more clear IMO, so
>> I'm wondering if wouldn't be better don't use dynamic memory here (I know that
>> this is not really related to your patch)
> 
> That sounds right to me. Can I make that change as a follow up patch?
> I'd like to contain the scope
> of this patch to just fix the return value.
> 
>>
>> And another thing that I detected, now that you're returning a bool is that in
>> drivers/platform/chrome/cros_ec_typec.c there is:
>>
>>    typec->typec_cmd_supported = !!cros_ec_check_features(ec_dev,
>>    typec->needs_mux_ack = !!cros_ec_check_features(ec_dev,
>>
>> I think that you can remove the !! now. That could be in another patch.
> 
> Yes, I was planning on doing that in a later patch.
> 
> Best regards,
> 

Re: [PATCH] platform/chrome: cros_ec_proto: Fix check_features ret val

[Prashant Malani]

Hi Enric,

On Wed, Sep 22, 2021 at 7:09 AM Enric Balletbo i Serra
<enric.balletbo@collabora.com> wrote:
>
> Hi Prashant,
>
> On 22/9/21 12:55, Prashant Malani wrote:
> > Hi Enric,
> >
> > On Wed, Sep 22, 2021 at 2:12 AM Enric Balletbo i Serra
> > <enric.balletbo@collabora.com> wrote:
> >>
> >> Hi Prashant,
> >>
> >> On 21/9/21 19:29, Prashant Malani wrote:
> >>> Hi Enric,
> >>>
> >>> Thanks for reviewing the patch.
> >>>
> >>> On Tue, Sep 21, 2021 at 01:42:04PM +0200, Enric Balletbo i Serra wrote:
> >>>> Hi Prashant,
> >>>>
> >>>> Thank you for the patch. Just one comment below ...
> >>>>
> >>>> On 16/9/21 3:46, Prashant Malani wrote:
> >>>>> The kerneldoc for cros_ec_check_features() states that it returns 1 or 0
> >>>>> depedending on whether a feature is supported or not, but it instead
> >>>>> returns a negative error number in one case, and a non-1 bitmask in
> >>>>> other cases.
> >>>>>
> >>>>> Since all call-sites only check for a 1 or 0 return value, update
> >>>>> the function to return boolean values.
> >>>>>
> >>>>> Signed-off-by: Prashant Malani <pmalani@chromium.org>
> >>>>> ---
> >>>>>  drivers/platform/chrome/cros_ec_proto.c     | 12 +++++++-----
> >>>>>  include/linux/platform_data/cros_ec_proto.h |  2 +-
> >>>>>  2 files changed, 8 insertions(+), 6 deletions(-)
> >>>>>
> >>>>> diff --git a/drivers/platform/chrome/cros_ec_proto.c b/drivers/platform/chrome/cros_ec_proto.c
> >>>>> index a7404d69b2d3..a34cf58c5ef7 100644
> >>>>> --- a/drivers/platform/chrome/cros_ec_proto.c
> >>>>> +++ b/drivers/platform/chrome/cros_ec_proto.c
> >>>>> @@ -808,9 +808,9 @@ EXPORT_SYMBOL(cros_ec_get_host_event);
> >>>>>   *
> >>>>>   * Call this function to test whether the ChromeOS EC supports a feature.
> >>>>>   *
> >>>>> - * Return: 1 if supported, 0 if not
> >>>>> + * Return: true if supported, false if not (or if an error was encountered).
> >>>>>   */
> >>>>> -int cros_ec_check_features(struct cros_ec_dev *ec, int feature)
> >>>>> +bool cros_ec_check_features(struct cros_ec_dev *ec, int feature)
> >>>>>  {
> >>>>>     struct cros_ec_command *msg;
> >>>>>     int ret;
> >>>>> @@ -818,8 +818,10 @@ int cros_ec_check_features(struct cros_ec_dev *ec, int feature)
> >>>>>     if (ec->features[0] == -1U && ec->features[1] == -1U) {
> >>>>>             /* features bitmap not read yet */
> >>>>>             msg = kzalloc(sizeof(*msg) + sizeof(ec->features), GFP_KERNEL);
> >>>>> -           if (!msg)
> >>>>> -                   return -ENOMEM;
> >>>>> +           if (!msg) {
> >>>>> +                   dev_err(ec->dev, "failed to allocate memory to get EC features\n");
> >>>>
> >>>> In case of failure you will be noticed by the allocator, prints after
> >>>> [k|v][m|z|c]alloc() functions are not needed, so I think you can just return
> >>>> false here.
> >>>>>
> >>>
> >>> Makes sense; I can make the change, but I had one question:
> >>>
> >>> If we solely return false, how will we tell from the logs that the
> >>> allocation error message was associated with this driver? Only returning
> >>> false means the driver probe (e.g cros-ec-typec) will continue (just assuming a certain feature
> >>> is not present). Wouldn't having this error message make this clear?
> >>>
> >>
> >> So I tried to find some doc about this without luck. But I think it has been an
> >> unwritten rule that GFP_KERNEL allocations for small allocations will never
> >> fail.
> >
> > That might be the case, but kzalloc() still returns the error value,
> > so even if it is very unlikely, we
> > still need to handle that error.
> >
>
> I'm not saying that we don't need to handle that error, we *must* handle the
> allocation errors, there is no doubts about this, but in this specific function
> we're ignoring all the errors and not noticing to the users. The discussion, or
> my nitpick (it's just a nit), is about if logging that the allocation failed in
> this driver is useful or not.
>
>
> >> If you system fails to allocate that small amount of memory you probably
> >> have bigger problems to solve and the above message is not really useful, even
> >> confusing, as the focus, likely, shouldn't be in this driver to solve the problem.
> >
> > I don't know if I necessarily concur with that rationale ("if it
> > fails, there are likely bigger issues").
> > There could be situations (hypothetical) where a series of allocations
> > might lead to a failure (or this might be a transient allocation
> > failure),
> > and it might be useful to know which driver is contributing the alloc
> > that finally precipitates the failure.
> >
>
> If the kernel is not able to give you a small amount of memory it is just a
> coincidence that this happened in your driver. Dynamic allocations and
> deallocations happens often, the print doesn't gives you more information as it
> could be any other driver because your system runs out of memory, and everybody
> can be affected.
>
> > Also, although it is very unlikely, returning true without an error
>
> I'm not saying this, I'm saying returning false but don't log the error to the
> console.
>
> > can mean the typec driver silently continues to function
>
> I think the consensus on the interface is, and was clear:
>
> 1/True if the feature is supported, 0/false if is not supported or there is an
> error. At some point we decided that the callers don't need to differentiate
> about if the feature is not supported or there is and unlikely error.
>

I wasn't present when this was agreed on, but I don't think that's the
right approach; moreover the current
implementation doesn't adhere to it.
In general I prefer either returning the appropriate error code, or
parsing the error and logging it.

> > wrongly assuming a feature flag to be set a certain way. That is
> > something we need to flag through the logs.
>
> If you run out of memory there will be a lot of logs, trust me ;-)
>
> > I certainly can't see the log message as confusing the reader of a log
> > any further.
> >
>
> As I said there is an unwritten rule (didn't find written doc about it tbh,
> although I know other maintainers follow) that prints after [k|v][m|z|c]alloc()
> functions are not needed.

Right, but in most cases they return -ENOMEM; here the boolean return
type prevents that.

>
> And as I said, this is a nit, I won't strongly oppose if you think that this
> message is really needed.

I'd prefer sticking with this if it's possible; in any case we'll have
a follow up patch soon which gets rid of
the kzalloc entirely (as you suggested) so this discussion will become moot.

Best regards,

Re: [PATCH] platform/chrome: cros_ec_proto: Fix check_features ret val

[Enric Balletbo i Serra]

On Wed, 15 Sep 2021 18:46:27 -0700, Prashant Malani wrote:
> The kerneldoc for cros_ec_check_features() states that it returns 1 or 0
> depedending on whether a feature is supported or not, but it instead
> returns a negative error number in one case, and a non-1 bitmask in
> other cases.
> 
> Since all call-sites only check for a 1 or 0 return value, update
> the function to return boolean values.

Applied, thanks!

[1/1] platform/chrome: cros_ec_proto: Fix check_features ret val
      commit: d50497c4a05e73e76874fd0d4942036375a7ec0f

Best regards,
-- 
Enric Balletbo i Serra <enric.balletbo@collabora.com>