[PATCH] TC: Delete an error message for a failed memory allocation in tc_bus_add_devices()

[PATCH] TC: Delete an error message for a failed memory allocation in tc_bus_add_devices()

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Sun, 10 Dec 2017 18:42:42 +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/tc/tc.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/tc/tc.c b/drivers/tc/tc.c
index 3be9519654e5..2deb3768a9f6 100644
--- a/drivers/tc/tc.c
+++ b/drivers/tc/tc.c
@@ -82,10 +82,9 @@ static void __init tc_bus_add_devices(struct tc_bus *tbus)
 
 		/* Found a board, allocate it an entry in the list */
 		tdev = kzalloc(sizeof(*tdev), GFP_KERNEL);
-		if (!tdev) {
-			pr_err("tc%x: unable to allocate tc_dev\n", slot);
+		if (!tdev)
 			goto out_err;
-		}
+
 		dev_set_name(&tdev->dev, "tc%x", slot);
 		tdev->bus = tbus;
 		tdev->dev.parent = &tbus->dev;
-- 
2.15.1

Re: [PATCH] TC: Delete an error message for a failed memory allocation in tc_bus_add_devices()

[Maciej W. Rozycki]

On Sun, 10 Dec 2017, SF Markus Elfring wrote:

> Omit an extra message for a memory allocation failure in this function.
> 
> This issue was detected by using the Coccinelle software.

 And the problem here is?

  Maciej

Re: [PATCH] TC: Delete an error message for a failed memory allocation in tc_bus_add_devices()

[Joe Perches]

On Sun, 2017-12-10 at 21:41 +0000, Maciej W. Rozycki wrote:
> On Sun, 10 Dec 2017, SF Markus Elfring wrote:
> 
> > Omit an extra message for a memory allocation failure in this function.
> > 
> > This issue was detected by using the Coccinelle software.
> 
>  And the problem here is?

Markus' terrible commit messages.

Generically, any OOM via a malloc like call has a dump_stack()
which shows a stack trace unless __GFP_NOWARN is used.

So this message is generally unnecessary as the dump_stack()
will show the tc_bus_add_devices function name and (now hashed)
value of the function address.

What will be different is the particular slot # of the tc_dev
will no longer be shown.

Really though, if there's an OOM on the init, there are larger
problems and the system will be unusable.

Re: TC: Delete an error message for a failed memory allocation in tc_bus_add_devices()

[SF Markus Elfring]

>> Omit an extra message for a memory allocation failure in this function.
>>
>> This issue was detected by using the Coccinelle software.
> 
>  And the problem here is?

I suggest to reconsider the relevance of another error message.
Would you like to achieve a better explanation for this use case?

* Do you find the wording “WARNING: Possible unnecessary 'out of memory' message”
  from the script “checkpatch.pl” more appropriate?

* Can a default Linux allocation failure report be sufficient?

Regards,
Markus

Re: [PATCH] TC: Delete an error message for a failed memory allocation in tc_bus_add_devices()

[Maciej W. Rozycki]

On Sun, 10 Dec 2017, Joe Perches wrote:

> > > Omit an extra message for a memory allocation failure in this function.
> > > 
> > > This issue was detected by using the Coccinelle software.
> > 
> >  And the problem here is?
> 
> Markus' terrible commit messages.
> 
> Generically, any OOM via a malloc like call has a dump_stack()
> which shows a stack trace unless __GFP_NOWARN is used.
> 
> So this message is generally unnecessary as the dump_stack()
> will show the tc_bus_add_devices function name and (now hashed)
> value of the function address.

 Fair enough.

> What will be different is the particular slot # of the tc_dev
> will no longer be shown.
> 
> Really though, if there's an OOM on the init, there are larger
> problems and the system will be unusable.

 Well, the problem may well be the caller doing something silly, so we do 
want to have it identified, hence the extra message.  Given that, as you 
say, `kzalloc' already provides the necessary diagnostic I agree the 
message can go.

 However I would indeed prefer that a commit description is at least 
exhaustive enough for such a dumb reviewer as I am to understand what is 
going on right away.  So please make it say at least:

"Remove an extraneous message that duplicates the diagnostic already 
provided by `kzalloc' for a memory allocation failure in this function."

or suchlike in v2 for me to apply a Reviewed-by: tag.

 Thanks,

  Maciej

Re: [PATCH] TC: Delete an error message for a failed memory allocation in tc_bus_add_devices()

[SF Markus Elfring]

>  However I would indeed prefer that a commit description is at least 
> exhaustive enough for such a dumb reviewer as I am to understand what is 
> going on right away.

I can follow your desire in principle.


> So please make it say at least:
> 
> "Remove an extraneous message that duplicates the diagnostic already 
> provided by `kzalloc' for a memory allocation failure in this function."

How much clarification can such a wording bring to the software situation
if the desired data structures might be still unclear for “the diagnostic”?


> or suchlike in v2 for me to apply a Reviewed-by: tag.

Are you interested in a safe description for a default Linux allocation
failure report?

How do you think about corresponding reference documentation
(besides source code)?

Regards,
Markus