[PATCH] hid-sensor-accel-3d: Delete an error message for a failed memory allocation in hid_accel_3d_probe()

[PATCH] hid-sensor-accel-3d: Delete an error message for a failed memory allocation in hid_accel_3d_probe()

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Wed, 31 Jan 2018 22:20:56 +0100

Omit an extra message for a memory allocation failure in this function.

This issue was detected by using the Coccinelle software.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 drivers/iio/accel/hid-sensor-accel-3d.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/drivers/iio/accel/hid-sensor-accel-3d.c b/drivers/iio/accel/hid-sensor-accel-3d.c
index c066a3bdbff7..3d0acde40285 100644
--- a/drivers/iio/accel/hid-sensor-accel-3d.c
+++ b/drivers/iio/accel/hid-sensor-accel-3d.c
@@ -383,11 +383,9 @@ static int hid_accel_3d_probe(struct platform_device *pdev)
 		return ret;
 	}
 	indio_dev->channels = kmemdup(channel_spec, channel_size, GFP_KERNEL);
-
-	if (!indio_dev->channels) {
-		dev_err(&pdev->dev, "failed to duplicate channels\n");
+	if (!indio_dev->channels)
 		return -ENOMEM;
-	}
+
 	ret = accel_3d_parse_report(pdev, hsdev,
 				(struct iio_chan_spec *)indio_dev->channels,
 				hsdev->usage, accel_state);
-- 
2.16.1

Re: [PATCH] hid-sensor-accel-3d: Delete an error message for a failed memory allocation in hid_accel_3d_probe()

[Jonathan Cameron]

On Wed, 31 Jan 2018 22:26:14 +0100
SF Markus Elfring <elfring@users.sourceforge.net> wrote:

> From: Markus Elfring <elfring@users.sourceforge.net>
> Date: Wed, 31 Jan 2018 22:20:56 +0100
> 
> Omit an extra message for a memory allocation failure in this function.
> 
> This issue was detected by using the Coccinelle software.
> 
> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>

Marcus,

If making changes like this I would suggest only sending one until
you have have a response from the relevant maintainer.

It would save you time as often these sorts of changes are
a matter of personal taste and weighing up of costs vs gains
- hence it is not obvious that they will be accepted.

Jonathan

> ---
>  drivers/iio/accel/hid-sensor-accel-3d.c | 6 ++----
>  1 file changed, 2 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/iio/accel/hid-sensor-accel-3d.c b/drivers/iio/accel/hid-sensor-accel-3d.c
> index c066a3bdbff7..3d0acde40285 100644
> --- a/drivers/iio/accel/hid-sensor-accel-3d.c
> +++ b/drivers/iio/accel/hid-sensor-accel-3d.c
> @@ -383,11 +383,9 @@ static int hid_accel_3d_probe(struct platform_device *pdev)
>  		return ret;
>  	}
>  	indio_dev->channels = kmemdup(channel_spec, channel_size, GFP_KERNEL);
> -
> -	if (!indio_dev->channels) {
> -		dev_err(&pdev->dev, "failed to duplicate channels\n");
> +	if (!indio_dev->channels)
>  		return -ENOMEM;
> -	}
> +
>  	ret = accel_3d_parse_report(pdev, hsdev,
>  				(struct iio_chan_spec *)indio_dev->channels,
>  				hsdev->usage, accel_state);

Re: hid-sensor-accel-3d: Delete an error message for a failed memory allocation in hid_accel_3d_probe()

[SF Markus Elfring]

> If making changes like this I would suggest only sending one until
> you have have a response from the relevant maintainer.

The corresponding feedback can become more positive for such
a transformation pattern after a while, can't it?


> It would save you time as often these sorts of changes are
> a matter of personal taste and weighing up of costs vs gains
> - hence it is not obvious that they will be accepted.

Can the wording “WARNING: Possible unnecessary 'out of memory' message”
(from the script “checkpatch.pl”) be another incentive?

Regards,
Markus

Re: hid-sensor-accel-3d: Delete an error message for a failed memory allocation in hid_accel_3d_probe()

[Jonathan Cameron]

On 5 February 2018 18:26:59 GMT, SF Markus Elfring <elfring-Rn4VEauK+AKRv+LV9MX5uipxlwaOVQ5f@public.gmane.org> wrote:
>> If making changes like this I would suggest only sending one until
>> you have have a response from the relevant maintainer.
>
>The corresponding feedback can become more positive for such
>a transformation pattern after a while, can't it?

Not in that sort of time period.
>
>
>> It would save you time as often these sorts of changes are
>> a matter of personal taste and weighing up of costs vs gains
>> - hence it is not obvious that they will be accepted.
>
>Can the wording “WARNING: Possible unnecessary 'out of memory' message”
>(from the script “checkpatch.pl”) be another incentive?

No. The key word is possible. Check patch is dumb and often gives false positives.
>
>Regards,
>Markus

-- 
Sent from my Android device with K-9 Mail. Please excuse my brevity.

Re: Software evolution around “checkpatch.pl”?

[SF Markus Elfring]

> Check patch is dumb and often gives false positives.

Would you like to improve this software situation anyhow?

Regards,
Markus

Re: Software evolution around “checkpatch.pl”?

[Jonathan Cameron]

On Tue, 6 Feb 2018 09:45:48 +0100
SF Markus Elfring <elfring@users.sourceforge.net> wrote:

> > Check patch is dumb and often gives false positives.  
> 
> Would you like to improve this software situation anyhow?

While it would be great to improve checkpatches false
positive rate, it's very nature as a string matcher makes
this hard.

Jonathan

> 
> Regards,
> Markus

Re: Software evolution around “checkpatch.pl”?

[Joe Perches]

On Sat, 2018-02-10 at 14:53 +0000, Jonathan Cameron wrote:
> While it would be great to improve checkpatches false
> positive rate, it's very nature as a string matcher makes
> this hard.

true.

what are the false positives you see?

Re: Software evolution around “checkpatch.pl”?

[Jonathan Cameron]

On Sat, 10 Feb 2018 06:59:43 -0800
Joe Perches <joe-6d6DIl74uiNBDgjK7y7TUQ@public.gmane.org> wrote:

> On Sat, 2018-02-10 at 14:53 +0000, Jonathan Cameron wrote:
> > While it would be great to improve checkpatches false
> > positive rate, it's very nature as a string matcher makes
> > this hard.  
> 
> true.
> 
> what are the false positives you see?
> 
This particular case is only 'sort of' a false positive
in the warning that a message printed on a memory allocation
failure 'may' not add any information over the generic case.

Very hard to judge on whether it is useful to know more than
an allocation failed somewhere or not.

Message makes this clear:
>“WARNING: Possible unnecessary 'out of memory' message”
>(from the script “checkpatch.pl”)  

We also have the balance between any changes to existing code
adding 'some' maintenance overhead vs changing this stuff
in a new driver - which is what checkpatch is really intended
for.

So I think checkpatch is striking the right balance here in
how it warns.  Obviously if it could assess the text
and come to an informed decision that would be great but
we are some way from that ;)

Jonathan

Re: Software evolution around “checkpatch.pl”?

[Joe Perches]

On Sat, 2018-02-10 at 15:57 +0000, Jonathan Cameron wrote:
> On Sat, 10 Feb 2018 06:59:43 -0800
> Joe Perches <joe-6d6DIl74uiNBDgjK7y7TUQ@public.gmane.org> wrote:
> 
> > On Sat, 2018-02-10 at 14:53 +0000, Jonathan Cameron wrote:
> > > While it would be great to improve checkpatches false
> > > positive rate, it's very nature as a string matcher makes
> > > this hard.  
> > 
> > true.
> > 
> > what are the false positives you see?
> > 
> 
> This particular case is only 'sort of' a false positive
> in the warning that a message printed on a memory allocation
> failure 'may' not add any information over the generic case.

Right.  So it's not a 'false positive' at all.
Are there any actual 'false positives' you know of?

> Very hard to judge on whether it is useful to know more than
> an allocation failed somewhere or not.
> 
> Message makes this clear:
> > “WARNING: Possible unnecessary 'out of memory' message”
> > (from the script “checkpatch.pl”)  
> 
> We also have the balance between any changes to existing code
> adding 'some' maintenance overhead vs changing this stuff
> in a new driver - which is what checkpatch is really intended
> for.

There's almost zero maintenance overhead here.
The time it takes for the back and forth
replies is likely larger.

> So I think checkpatch is striking the right balance here in
> how it warns.  Obviously if it could assess the text
> and come to an informed decision that would be great but
> we are some way from that ;)

The 'informed' bit is difficult as it is mostly
a political problem.

This particular message really is unnecessary as
the generic dump_stack on any normal allocation
(ie: without __GFP_WARN) already emits location
specific information.

Removing these messages can help make the kernel
image smaller and thereby help make these OOM
messages a tiny bit less likely.

I just wish Markus would improve his consistently
terrible commit messages that just restate the
action being done and detail _why_ a particular
thing _should_ be done.

His acceptance rate would improve as many of these
back and forth replies for what trivialities he
posts as patches would be minimized.

Re: Software evolution around “checkpatch.pl”?

[SF Markus Elfring]

>> So I think checkpatch is striking the right balance here in
>> how it warns.  Obviously if it could assess the text
>> and come to an informed decision that would be great but
>> we are some way from that ;)
> 
> The 'informed' bit is difficult as it is mostly a political problem.

I find such a view very interesting.


> I just wish Markus would improve his consistently terrible commit messages

I tried to achieve another clarification a few times.


> that just restate the action being done and detail
> _why_ a particular thing _should_ be done.

Unfortunately, it seems that no other contributors picked
corresponding opportunities up so far.

You indicated also special software development challenges in your commit
“checkpatch: attempt to find unnecessary 'out of memory' messages”.

https://lkml.org/lkml/2014/6/10/382
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=ebfdc40969f24fc0cdd1349835d36e8ebae05374


> His acceptance rate would improve as many of these back and forth
> replies for what trivialities he posts as patches would be minimized.

My selection of change possibilities leads to mixed integration results.

I stumbled on variations for general change resistance.

Regards,
Markus