Re: [Intel-gfx] [PATCH 2/4] drm/i915/guc: Print error name on CTB (de)registration failure

[Daniel Vetter <daniel@ffwll.ch>]

On Wed, Aug 18, 2021 at 09:12:32PM +0200, Michal Wajdeczko wrote:
> 
> 
> On 18.08.2021 18:35, Daniel Vetter wrote:
> > On Wed, Aug 18, 2021 at 5:11 PM Michal Wajdeczko
> > <michal.wajdeczko@intel.com> wrote:
> >>
> >>
> >>
> >> On 18.08.2021 16:20, Daniel Vetter wrote:
> >>> On Thu, Jul 01, 2021 at 05:55:11PM +0200, Michal Wajdeczko wrote:
> >>>> Instead of plain error value (%d) print more user friendly error
> >>>> name (%pe).
> >>>>
> >>>> Signed-off-by: Michal Wajdeczko <michal.wajdeczko@intel.com>
> >>>> ---
> >>>>  drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c | 8 ++++----
> >>>>  1 file changed, 4 insertions(+), 4 deletions(-)
> >>>>
> >>>> diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c b/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c
> >>>> index a26bb55c0898..18d52c39f0c2 100644
> >>>> --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c
> >>>> +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c
> >>>> @@ -167,8 +167,8 @@ static int ct_register_buffer(struct intel_guc_ct *ct, u32 type,
> >>>>      err = guc_action_register_ct_buffer(ct_to_guc(ct), type,
> >>>>                                          desc_addr, buff_addr, size);
> >>>>      if (unlikely(err))
> >>>> -            CT_ERROR(ct, "Failed to register %s buffer (err=%d)\n",
> >>>> -                     guc_ct_buffer_type_to_str(type), err);
> >>>> +            CT_ERROR(ct, "Failed to register %s buffer (%pe)\n",
> >>>> +                     guc_ct_buffer_type_to_str(type), ERR_PTR(err));
> >>>
> >>> errname() is what you want here, not this convoluted jumping through hoops
> >>> to fake an error pointer.
> >>
> >> nope, I was already trying that shortcut, but errname() is not exported
> >> so we fail with
> >>
> >> ERROR: modpost: "errname" [drivers/gpu/drm/i915/i915.ko] undefined!
> >>
> >> so unless we add that export we must follow initial approach [1]
> > 
> > Then we export that function. This is all open source, we can actually
> > fix things up, there should _never_ be a need to hack around things
> > like this.
> 
> simple export might be not sufficient, as this function returns NULL for
> unrecognized error codes, and it might be hard to print that code in
> plain format, as it %pe does it for us for free.

"(%s:%i)", errname(ret), ret

Would work, but maybe not so pretty. Otoh %pe for unrecognized integers is
also not very pretty.

> is that big problem to use ERR_PTR? I'm not the only/first one
> 
> see
> drivers/net/can/usb/etas_es58x/es58x_core.c
> drivers/net/ethernet/freescale/enetc/enetc_pf.c
> drivers/net/phy/phylink.c
> ...

Ah yeah, looking through grep more than half the users do this pattern.
Which still feels extremely silly, because it's not a pointer we're
printing, and when it's not an errno we should probably print it as an
integer or something. But also meh. On both patches, as-is:

Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>

> > And yes I'm keenly aware that there's a pile of i915-gem code which
> > outright flaunts this principle, but that doesn't mean we should
> > continue with that. scripts/get_maintainers.pl can help with finding
> > all the people you need to cc on that export patch.
> 
> I'm perfectly fine with updating/fixing shared code (did that before,
> have few more ideas on my todo-list) but in this case I'm not so sure

I think an %ie extension or something like that for printk would make some
sense. There's absolute enormous amounts of this kind of casting going on,
and it just doesn't make sense to me.

It would be pretty easy way to get like 100 patches into the kernel to
clean it all up :-)
-Daniel

> 
> -Michal
> 
> > -Daniel
> > 
> >>
> >> -Michal
> >>
> >> [1]
> >> https://cgit.freedesktop.org/drm/drm-tip/commit/?id=57f5677e535ba24b8926a7125be2ef8d7f09323c
> >>
> >>>
> >>> With that: Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> >>>>      return err;
> >>>>  }
> >>>>
> >>>> @@ -195,8 +195,8 @@ static int ct_deregister_buffer(struct intel_guc_ct *ct, u32 type)
> >>>>      int err = guc_action_deregister_ct_buffer(ct_to_guc(ct), type);
> >>>>
> >>>>      if (unlikely(err))
> >>>> -            CT_ERROR(ct, "Failed to deregister %s buffer (err=%d)\n",
> >>>> -                     guc_ct_buffer_type_to_str(type), err);
> >>>> +            CT_ERROR(ct, "Failed to deregister %s buffer (%pe)\n",
> >>>> +                     guc_ct_buffer_type_to_str(type), ERR_PTR(err));
> >>>>      return err;
> >>>>  }
> >>>>
> >>>> --
> >>>> 2.25.1
> >>>>
> >>>> _______________________________________________
> >>>> Intel-gfx mailing list
> >>>> Intel-gfx@lists.freedesktop.org
> >>>> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> >>>
> > 
> > 
> > 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch