Re: [PATCH v2] staging: greybus: replace PTR_ERR() with %pe in error messages
From: Jeremy Dean Clifton
Date: Wed Oct 14 2026 - 12:47:18 EST
On 10/13/26 3:43 PM, Yalagada Pavan Kumar wrote:
On Tue, Oct 13, 2026 at 03:11:51PM -0500, Alex Elder wrote:Also, you could coordinate something with Julia to use Coccinelle for this. Using regular expressions, you could programmatically find all the places where %pe and connection can be in place of %d or %ld and the return from PTR_ERR().
On 10/12/26 9:24 PM, pavankumaryalagada@xxxxxxxxx wrote:
From: Yalagada Pavan Kumar <pavankumaryalagada@xxxxxxxxx>
Use %pe to print error pointers directly and display their
error names.
Keep the other PTR_ERR() call because its return value is
needed for ret.
Signed-off-by: Yalagada Pavan Kumar <pavankumaryalagada@xxxxxxxxx>
Generally I think what you're doing is OK. It looks
like %pe is better than just %d, because it prints
the error *name* for the pointer-coded error.
But I have two general questions about this effort:
- There are other places in the Greybus code (even in
just the staging source files) where this exact same
thing could be done. Why limit this to "fw-core.c" only?
I initially limited the change to "fw-core.c" because I was
focusing on the error messages in that file.
- Do you think that all Greybus error messages should be
converted to use %pe (passing ERR_PTR() if necessary)
instead of %d for showing the error number?
Using %pe prints the error name instead of just the
error number, which makes the error messages easier
to understand.
For example, the current message is:
failed to create download connection (-12)
with %pe, it becomes:
failed to create download connection (-ENOMEM)
I think the second message is easier to understand because
it shows the error name instead of just error number.
I also found other PTR_ERR() calls in Greybus. some of them
are used to return error codes and should remain unchanged.
For example, in fw-core.c:
if (IS_ERR(connection)) {
ret = PTR_ERR(connection);
dev_err(&bundle->dev,
"failed to create management connection (%pe)\n",
connection);
goto err_destroy_connections;
}
Here, PTR_ERR(connection) is needed to store the error code in ret
for the error-handling path, while passing connection to %pe makes
the error message more readable.
I think your suggested change is a net positive, but
doing it in some spots but not others means it is
adding a bit of inconsistency.
I agree that similar cases elsewhere in Greybus are worth
checking to keep error reporting consistent. I will look
at the other Greybus files for cases where using %pe would
be appropriate.
Would you prefer me to extend this change to the other
relevant Greybus files, or keep this patch focused on
fw-core.c and submit a separate patch for the remaining
changes?
Jeremy Clifton