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

[Michal Wajdeczko <michal.wajdeczko@intel.com>]

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.

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
...

> 
> 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

-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
>>>
> 
> 
>