| Message ID | 0406765c-bdd1-1a82-cf66-1c248063ae4f@users.sourceforge.net (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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); -- To unsubscribe from this list: send the line "unsubscribe linux-iio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> 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 -- To unsubscribe from this list: send the line "unsubscribe linux-iio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 5 February 2018 18:26:59 GMT, SF Markus Elfring <elfring@users.sourceforge.net> 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
> Check patch is dumb and often gives false positives.
Would you like to improve this software situation anyhow?
Regards,
Markus
--
To unsubscribe from this list: send the line "unsubscribe linux-iio" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe linux-iio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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? -- To unsubscribe from this list: send the line "unsubscribe linux-iio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Sat, 10 Feb 2018 06:59:43 -0800 Joe Perches <joe@perches.com> 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 -- To unsubscribe from this list: send the line "unsubscribe linux-iio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Sat, 2018-02-10 at 15:57 +0000, Jonathan Cameron wrote: > On Sat, 10 Feb 2018 06:59:43 -0800 > Joe Perches <joe@perches.com> 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. -- To unsubscribe from this list: send the line "unsubscribe linux-iio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
>> 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 -- To unsubscribe from this list: send the line "unsubscribe linux-iio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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);