From: Markus Elfring <[email protected]>
Date: Mon, 18 Dec 2017 22:23:45 +0100
Two update suggestions were taken into account
from static source code analysis.
Markus Elfring (2):
Delete an error message for a failed memory allocation in three functions
Improve a size determination in tpacpi_new_rfkill()
drivers/platform/x86/thinkpad_acpi.c | 11 +++--------
1 file changed, 3 insertions(+), 8 deletions(-)
--
2.15.1
From: Markus Elfring <[email protected]>
Date: Mon, 18 Dec 2017 22:08:49 +0100
Omit an extra message for a memory allocation failure in these functions.
This issue was detected by using the Coccinelle software.
Signed-off-by: Markus Elfring <[email protected]>
---
drivers/platform/x86/thinkpad_acpi.c | 9 ++-------
1 file changed, 2 insertions(+), 7 deletions(-)
diff --git a/drivers/platform/x86/thinkpad_acpi.c b/drivers/platform/x86/thinkpad_acpi.c
index 117be48ff4de..729144925880 100644
--- a/drivers/platform/x86/thinkpad_acpi.c
+++ b/drivers/platform/x86/thinkpad_acpi.c
@@ -816,10 +816,8 @@ static int __init register_tpacpi_subdriver(struct ibm_struct *ibm)
BUG_ON(!ibm->acpi);
ibm->acpi->driver = kzalloc(sizeof(struct acpi_driver), GFP_KERNEL);
- if (!ibm->acpi->driver) {
- pr_err("failed to allocate memory for ibm->acpi->driver\n");
+ if (!ibm->acpi->driver)
return -ENOMEM;
- }
sprintf(ibm->acpi->driver->name, "%s_%s", TPACPI_NAME, ibm->name);
ibm->acpi->driver->ids = ibm->acpi->hid;
@@ -3639,7 +3637,6 @@ static int __init hotkey_init(struct ibm_init_struct *iibm)
hotkey_keycode_map = kmalloc(TPACPI_HOTKEY_MAP_SIZE,
GFP_KERNEL);
if (!hotkey_keycode_map) {
- pr_err("failed to allocate memory for key map\n");
res = -ENOMEM;
goto err_exit;
}
@@ -5996,10 +5993,8 @@ static int __init led_init(struct ibm_init_struct *iibm)
tpacpi_leds = kzalloc(sizeof(*tpacpi_leds) * TPACPI_LED_NUMLEDS,
GFP_KERNEL);
- if (!tpacpi_leds) {
- pr_err("Out of memory for LED data\n");
+ if (!tpacpi_leds)
return -ENOMEM;
- }
for (i = 0; i < TPACPI_LED_NUMLEDS; i++) {
tpacpi_leds[i].led = -1;
--
2.15.1
From: Markus Elfring <[email protected]>
Date: Mon, 18 Dec 2017 22:13:53 +0100
Replace the specification of a data structure by a pointer dereference
as the parameter for the operator "sizeof" to make the corresponding size
determination a bit safer according to the Linux coding style convention.
This issue was detected by using the Coccinelle software.
Signed-off-by: Markus Elfring <[email protected]>
---
drivers/platform/x86/thinkpad_acpi.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/platform/x86/thinkpad_acpi.c b/drivers/platform/x86/thinkpad_acpi.c
index 729144925880..40b7a1b9a019 100644
--- a/drivers/platform/x86/thinkpad_acpi.c
+++ b/drivers/platform/x86/thinkpad_acpi.c
@@ -1258,7 +1258,7 @@ static int __init tpacpi_new_rfkill(const enum tpacpi_rfk_id id,
BUG_ON(id >= TPACPI_RFK_SW_MAX || tpacpi_rfkill_switches[id]);
- atp_rfk = kzalloc(sizeof(struct tpacpi_rfk), GFP_KERNEL);
+ atp_rfk = kzalloc(sizeof(*atp_rfk), GFP_KERNEL);
if (atp_rfk)
atp_rfk->rfkill = rfkill_alloc(name,
&tpacpi_pdev->dev,
--
2.15.1
On Mon, Dec 18, 2017 at 11:26 PM, SF Markus Elfring
<[email protected]> wrote:
> From: Markus Elfring <[email protected]>
> Date: Mon, 18 Dec 2017 22:23:45 +0100
>
> Two update suggestions were taken into account
> from static source code analysis.
>
> Markus Elfring (2):
> Delete an error message for a failed memory allocation in three functions
This one is questionable since it prints error messages at ->init() stage.
I would rather not touch this.
> Improve a size determination in tpacpi_new_rfkill()
Doesn't make any sense right now. One style over the other.
Nothing gets better or worth at this point.
Sorry, but NAK for both.
--
With Best Regards,
Andy Shevchenko
>> Delete an error message for a failed memory allocation in three functions
>
> This one is questionable since it prints error messages at ->init() stage.
> I would rather not touch this.
Do you find the Linux allocation failure report insufficient in this case?
>> Improve a size determination in tpacpi_new_rfkill()
>
> Doesn't make any sense right now. One style over the other.
> Nothing gets better or worth at this point.
Would you like to care for a bit more compliance with information
from the section “14) Allocating memory” in the document “coding-style.rst”?
Regards,
Markus
On Tue, Dec 19, 2017 at 6:23 PM, SF Markus Elfring
<[email protected]> wrote:
>>> Delete an error message for a failed memory allocation in three functions
>>
>> This one is questionable since it prints error messages at ->init() stage.
>> I would rather not touch this.
>
> Do you find the Linux allocation failure report insufficient in this case?
It is up to the current driver maintainer. For me it sounds better to
have than not to.
>>> Improve a size determination in tpacpi_new_rfkill()
>>
>> Doesn't make any sense right now. One style over the other.
>> Nothing gets better or worth at this point.
>
> Would you like to care for a bit more compliance with information
> from the section “14) Allocating memory” in the document “coding-style.rst”?
You know, in Russian we have an adverb: Заставь дурака б-гу молиться,
он и лоб расшибёт.
Which has English equivalent: Give a man enough rope and he’ll hang himself.
--
With Best Regards,
Andy Shevchenko
On Tue, 19 Dec 2017, Andy Shevchenko wrote:
> On Mon, Dec 18, 2017 at 11:26 PM, SF Markus Elfring
> <[email protected]> wrote:
> > From: Markus Elfring <[email protected]>
> > Date: Mon, 18 Dec 2017 22:23:45 +0100
> >
> > Two update suggestions were taken into account
> > from static source code analysis.
> >
> > Markus Elfring (2):
> > Delete an error message for a failed memory allocation in three functions
>
> This one is questionable since it prints error messages at ->init() stage.
> I would rather not touch this.
>
> > Improve a size determination in tpacpi_new_rfkill()
>
> Doesn't make any sense right now. One style over the other.
> Nothing gets better or worth at this point.
>
> Sorry, but NAK for both.
Agreed. NAK from me as well.
--
Henrique Holschuh
On Tue, 19 Dec 2017, SF Markus Elfring wrote:
> >> Delete an error message for a failed memory allocation in three functions
> >
> > This one is questionable since it prints error messages at ->init() stage.
> > I would rather not touch this.
>
> Do you find the Linux allocation failure report insufficient in this case?
Leave those pr_ messages alone, please, unless they are really causing
some sort of issue (which?).
> >> Improve a size determination in tpacpi_new_rfkill()
> >
> > Doesn't make any sense right now. One style over the other.
> > Nothing gets better or worth at this point.
>
> Would you like to care for a bit more compliance with information
> from the section “14) Allocating memory” in the document “coding-style.rst”?
No, unless the change is actually fixing something, or gives us a
down-to-earth, *real* advantage of some sort. In which case, the commit
message better do a rather good job of explaining it.
Doing it just for "compliance" with a doc isn't nearly good enough
reason.
--
Henrique Holschuh
>> Do you find the Linux allocation failure report insufficient in this case?
>
> Leave those pr_ messages alone, please,
Have you got special software development concerns?
> unless they are really causing some sort of issue (which?).
Can the code be redundant here?
> Doing it just for "compliance" with a doc isn't nearly good enough reason.
Do you give only a low value to coding style guidelines in this use case?
Regards,
Markus
On Sat, Dec 23, 2017 at 08:12:21AM +0100, SF Markus Elfring wrote:
> >> Do you find the Linux allocation failure report insufficient in this case?
> >
> > Leave those pr_ messages alone, please,
>
> Have you got special software development concerns?
>
>
> > unless they are really causing some sort of issue (which?).
>
> Can the code be redundant here?
>
>
> > Doing it just for "compliance" with a doc isn't nearly good enough reason.
>
> Do you give only a low value to coding style guidelines in this use case?
Hi Markus,
Thanks for submitting the patch. I understand it can be frustrating to
encounter different policies across kernel maintainers. You'll even run
in to this with maintainers of the same subsystem from time to time.
While we try to be as consistent as possible, and to document policy as
we resolve vague areas, this is unfortunately still part of kernel
development.
I'm supportive of cleaning up old code in general, and we routinely
apply such patches as these developed with cocci. We have also had them
fail in unexpected ways.
Andy and Henrique raised a few reasons why these patches should not be
accepted:
1. This is init code )so any space savings is short lived)
2. There is no functional fix, and the change is to code which supports
a lot of unique platforms, most of which we have no way to test. We are
particularly cautious with drivers such as the thinkpad driver for this
reason.
So it isn't that we place a low value on coding style guidelines, but
rather we place higher value on not perturbing code we can't fully test
without a demonstrable functional reasons to do so.
Thanks,
--
Darren Hart
VMware Open Source Technology Center
On Tue, 2018-01-02 at 16:10 -0800, Darren Hart wrote:
> > > Leave those pr_ messages alone, please,
[]
> Andy and Henrique raised a few reasons why these patches should not be
> accepted:
>
> 1. This is init code (so any space savings is short lived)
Not exactly true.
The object code itself is short lived, but the
string constant of the format is not as it is
placed in const and not discarded.
On Tue, Jan 02, 2018 at 04:49:43PM -0800, Joe Perches wrote:
> On Tue, 2018-01-02 at 16:10 -0800, Darren Hart wrote:
> > > > Leave those pr_ messages alone, please,
> []
> > Andy and Henrique raised a few reasons why these patches should not be
> > accepted:
> >
> > 1. This is init code (so any space savings is short lived)
>
> Not exactly true.
>
> The object code itself is short lived, but the
> string constant of the format is not as it is
> placed in const and not discarded.
Thanks for the clarification, appreciate it. I hadn't looked into it in enough
detail previously.
--
Darren Hart
VMware Open Source Technology Center
> I understand it can be frustrating to encounter different policies
> across kernel maintainers.
The change acceptance is varying for special transformation patterns.
> You'll even run in to this with maintainers of the same subsystem
> from time to time.
Interesting, isn't it?
> I'm supportive of cleaning up old code in general,
Nice.
> and we routinely apply such patches as these developed with cocci.
Good to know …
> 1. This is init code )so any space savings is short lived)
Would you dare to achieve another small improvement there?
> So it isn't that we place a low value on coding style guidelines,
> but rather we place higher value on not perturbing code
I can follow this view in principle.
> we can't fully test without a demonstrable functional reasons to do so.
How do you think about to get a bit nicer run time characteristics?
Regards,
Markus
On Wed, Jan 03, 2018 at 09:41:04AM +0100, SF Markus Elfring wrote:
> > I understand it can be frustrating to encounter different policies
> > across kernel maintainers.
>
> The change acceptance is varying for special transformation patterns.
>
>
> > You'll even run in to this with maintainers of the same subsystem
> > from time to time.
>
> Interesting, isn't it?
>
>
> > I'm supportive of cleaning up old code in general,
>
> Nice.
>
>
> > and we routinely apply such patches as these developed with cocci.
>
> Good to know …
>
>
> > 1. This is init code )so any space savings is short lived)
>
> Would you dare to achieve another small improvement there?
>
>
> > So it isn't that we place a low value on coding style guidelines,
> > but rather we place higher value on not perturbing code
>
> I can follow this view in principle.
>
>
> > we can't fully test without a demonstrable functional reasons to do so.
>
> How do you think about to get a bit nicer run time characteristics?
If this was code that affected all systems, the impact would be greater
- and it would be much easier to test. As it applies only to Thinkpad
systems, far fewer total systems are affected, and it is much harder to
test/verify. For something like this, we (Andy and I) will typically
defer to the driver-specific maintainer. Henrique has declined the
patch, and I think the reasoning is defensible.
If you feel that is the wrong call, you will need to present convincing
evidence to Henrique that this is worth the risk. From what I've seen of
the patch series thus far... I don't think the advantages can be argued
to outweigh the risks - or that it would be worth the effort.
Henrique, I'm going to stop there and let you chime in if you feel
differently about any of the above.
--
Darren Hart
VMware Open Source Technology Center
> If this was code that affected all systems, the impact would be greater
> - and it would be much easier to test.
I can follow such a view to some degree.
Would you dare to test the deletion of questionable error messages
more with any other software components?
> As it applies only to Thinkpad systems,
Are these models still popular enough in any areas?
> far fewer total systems are affected, and it is much harder to
> test/verify.
Do you care for the suggested transformation patterns (in principle)?
> If you feel that is the wrong call,
It seems that the usual indication was expressed for change resistance.
> you will need to present convincing evidence to Henrique that
> this is worth the risk.
Which risks have you got in mind for this small adjustment?
> … - or that it would be worth the effort.
Can a bit of “software fine-tuning” become useful also here?
Regards,
Markus