Re: [PATCH v2] staging: greybus: replace PTR_ERR() with %pe in error messages

[Jeremy Dean Clifton <gservice341@gmail.com>]

On 10/13/26 3:43 PM, Yalagada Pavan Kumar wrote:
> On Tue, Oct 13, 2026 at 03:11:51PM -0500, Alex Elder wrote:
>> On 10/12/26 9:24 PM, pavankumaryalagada@gmail.com wrote:
>>> From: Yalagada Pavan Kumar <pavankumaryalagada@gmail.com>
>>>
>>> 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@gmail.com>
>>
>>
>> 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?
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().

Jeremy Clifton