[PATCH 0/2] platform/x86/thinkpad_acpi: Adjustments for four function implementations

[PATCH 0/2] platform/x86/thinkpad_acpi: Adjustments for four function implementations

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
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

[PATCH 1/2] platform/x86/thinkpad_acpi: Delete an error message for a failed memory allocation in th

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
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 <elfring@users.sourceforge.net>
---
 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

[PATCH 2/2] platform/x86/thinkpad_acpi: Improve a size determination in tpacpi_new_rfkill()

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
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 <elfring@users.sourceforge.net>
---
 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

Re: [PATCH 0/2] platform/x86/thinkpad_acpi: Adjustments for four function implementations

[Andy Shevchenko]

On Mon, Dec 18, 2017 at 11:26 PM, SF Markus Elfring
<elfring@users.sourceforge.net> wrote:
> From: Markus Elfring <elfring@users.sourceforge.net>
> 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

Re: platform/x86/thinkpad_acpi: Adjustments for four function implementations

[SF Markus Elfring]

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

Re: platform/x86/thinkpad_acpi: Adjustments for four function implementations

[Andy Shevchenko]

On Tue, Dec 19, 2017 at 6:23 PM, SF Markus Elfring
<elfring@users.sourceforge.net> 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

Re: [PATCH 0/2] platform/x86/thinkpad_acpi: Adjustments for four function implementations

[Henrique de Moraes Holschuh]

On Tue, 19 Dec 2017, Andy Shevchenko wrote:
> On Mon, Dec 18, 2017 at 11:26 PM, SF Markus Elfring
> <elfring@users.sourceforge.net> wrote:
> > From: Markus Elfring <elfring@users.sourceforge.net>
> > 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

Re: platform/x86/thinkpad_acpi: Adjustments for four function implementations

[Henrique de Moraes 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

Re: platform/x86/thinkpad_acpi: Adjustments for four function implementations

[SF Markus Elfring]

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

Re: platform/x86/thinkpad_acpi: Adjustments for four function implementations

[Darren Hart]

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

Re: platform/x86/thinkpad_acpi: Adjustments for four function implementations

[Joe Perches]

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.

Re: platform/x86/thinkpad_acpi: Adjustments for four function implementations

[Darren Hart]

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

Re: platform/x86/thinkpad_acpi: Adjustments for four function implementations

[SF Markus Elfring]

> 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
--
To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: platform/x86/thinkpad_acpi: Adjustments for four function implementations

[Darren Hart]

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
--
To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: platform/x86/thinkpad_acpi: Adjustments for four function implementations

[SF Markus Elfring]

> 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