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

[Julia Lawall]

On Wed, 14 Oct 2026, Jeremy Dean Clifton wrote:

> 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@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?
> 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().

Coccinelle could probably be used for finding opportunities.  Modifying
strings can be a bit awkward, but is not impossible.

julia

>
> Jeremy Clifton
>
>