ERR_PTR(0) in a couple of places

public inbox for linux-kernel@vger.kernel.org
 help / color / mirror / Atom feed

* ERR_PTR(0) in a couple of places
@ 2023-09-24  0:41 Dr. David Alan Gilbert
  2023-09-24 11:41 ` Krzysztof Kozlowski
  2023-09-25  4:18 ` Matthew Brost
  0 siblings, 2 replies; 6+ messages in thread
From: Dr. David Alan Gilbert @ 2023-09-24  0:41 UTC (permalink / raw)
  To: matthew.brost, mgreer, krzysztof.kozlowski
  Cc: linux-kernel, airlied, intel-gfx

Hi,
  I randomly noticed there are a couple of places in the kernel that
do
   ERR_PTR(0);

and thought that was odd - shouldn't those just be NULL's ?

1) i915
  drivers/gpu/drm/i915/gt/uc/selftest_guc_multi_lrc.c : 47

    if (i <= 1)
      return ERR_PTR(0);

  from f9d72092cb490 

2) trf7970a
  drivers/nfc/trf7970a.c : 896

      trf->ignore_timeout =
         !cancel_delayed_work(&trf->timeout_work);
      trf->rx_skb = ERR_PTR(0);
      trf7970a_send_upstream(trf);

   from 1961843ceeca0

Dave
-- 
 -----Open up your eyes, open up your mind, open up your code -------   
/ Dr. David Alan Gilbert    |       Running GNU/Linux       | Happy  \ 
\        dave @ treblig.org |                               | In Hex /
 \ _________________________|_____ http://www.treblig.org   |_______/

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: ERR_PTR(0) in a couple of places
  2023-09-24  0:41 ERR_PTR(0) in a couple of places Dr. David Alan Gilbert
@ 2023-09-24 11:41 ` Krzysztof Kozlowski
  2023-09-24 12:16   ` Dr. David Alan Gilbert
  2023-09-25  4:18 ` Matthew Brost
  1 sibling, 1 reply; 6+ messages in thread
From: Krzysztof Kozlowski @ 2023-09-24 11:41 UTC (permalink / raw)
  To: Dr. David Alan Gilbert, matthew.brost, mgreer
  Cc: linux-kernel, airlied, intel-gfx

On 24/09/2023 02:41, Dr. David Alan Gilbert wrote:
> Hi,
>   I randomly noticed there are a couple of places in the kernel that
> do
>    ERR_PTR(0);
> 
> and thought that was odd - shouldn't those just be NULL's ?
> 
> 1) i915
>   drivers/gpu/drm/i915/gt/uc/selftest_guc_multi_lrc.c : 47
> 
>     if (i <= 1)
>       return ERR_PTR(0);
> 
>   from f9d72092cb490 
> 
> 2) trf7970a
>   drivers/nfc/trf7970a.c : 896
> 
>       trf->ignore_timeout =
>          !cancel_delayed_work(&trf->timeout_work);
>       trf->rx_skb = ERR_PTR(0);

I would guess that code is relying on rx_skb being valid pointer or ERR
(if (!IS_ERR(...))).

Best regards,
Krzysztof

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: ERR_PTR(0) in a couple of places
  2023-09-24 11:41 ` Krzysztof Kozlowski
@ 2023-09-24 12:16   ` Dr. David Alan Gilbert
  0 siblings, 0 replies; 6+ messages in thread
From: Dr. David Alan Gilbert @ 2023-09-24 12:16 UTC (permalink / raw)
  To: Krzysztof Kozlowski
  Cc: matthew.brost, mgreer, linux-kernel, airlied, intel-gfx

* Krzysztof Kozlowski (krzysztof.kozlowski@linaro.org) wrote:
> On 24/09/2023 02:41, Dr. David Alan Gilbert wrote:
> > Hi,
> >   I randomly noticed there are a couple of places in the kernel that
> > do
> >    ERR_PTR(0);
> > 
> > and thought that was odd - shouldn't those just be NULL's ?
> > 
> > 1) i915
> >   drivers/gpu/drm/i915/gt/uc/selftest_guc_multi_lrc.c : 47
> > 
> >     if (i <= 1)
> >       return ERR_PTR(0);
> > 
> >   from f9d72092cb490 
> > 
> > 2) trf7970a
> >   drivers/nfc/trf7970a.c : 896
> > 
> >       trf->ignore_timeout =
> >          !cancel_delayed_work(&trf->timeout_work);
> >       trf->rx_skb = ERR_PTR(0);
> 
> I would guess that code is relying on rx_skb being valid pointer or ERR
> (if (!IS_ERR(...))).

If seems mixed, that function calls trf7970a_send_upstream which has
both:

  if (trf->rx_skb && !IS_ERR(trf->rx_skb) && !trf->aborting)
    print_hex_dump_debug("trf7970a rx data: ", DUMP_PREFIX_NONE,
             16, 1, trf->rx_skb->data, trf->rx_skb->len,
             false);
and
    if (!IS_ERR(trf->rx_skb)) {
      kfree_skb(trf->rx_skb);
      trf->rx_skb = ERR_PTR(-ECANCELED);
    }

It's not clear to me whether it's expecteing that 2nd if to happen or
not.

I notice err.h gained a IS_ERR_OR_NULL to help that case as well.

Dave

> Best regards,
> Krzysztof
> 
-- 
 -----Open up your eyes, open up your mind, open up your code -------   
/ Dr. David Alan Gilbert    |       Running GNU/Linux       | Happy  \ 
\        dave @ treblig.org |                               | In Hex /
 \ _________________________|_____ http://www.treblig.org   |_______/

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: ERR_PTR(0) in a couple of places
  2023-09-24  0:41 ERR_PTR(0) in a couple of places Dr. David Alan Gilbert
  2023-09-24 11:41 ` Krzysztof Kozlowski
@ 2023-09-25  4:18 ` Matthew Brost
  2023-09-25  4:25   ` Randy Dunlap
  1 sibling, 1 reply; 6+ messages in thread
From: Matthew Brost @ 2023-09-25  4:18 UTC (permalink / raw)
  To: Dr. David Alan Gilbert
  Cc: mgreer, krzysztof.kozlowski, linux-kernel, airlied, intel-gfx

On Sun, Sep 24, 2023 at 12:41:07AM +0000, Dr. David Alan Gilbert wrote:
> Hi,
>   I randomly noticed there are a couple of places in the kernel that
> do
>    ERR_PTR(0);
> 
> and thought that was odd - shouldn't those just be NULL's ?
> 
> 1) i915
>   drivers/gpu/drm/i915/gt/uc/selftest_guc_multi_lrc.c : 47
> 
>     if (i <= 1)
>       return ERR_PTR(0);

Yes, s/ERR_PTR(0)/ERR_PTR(NULL)/

Matt

> 
>   from f9d72092cb490 
> 
> 2) trf7970a
>   drivers/nfc/trf7970a.c : 896
> 
>       trf->ignore_timeout =
>          !cancel_delayed_work(&trf->timeout_work);
>       trf->rx_skb = ERR_PTR(0);
>       trf7970a_send_upstream(trf);
> 
>    from 1961843ceeca0
> 
> Dave
> -- 
>  -----Open up your eyes, open up your mind, open up your code -------   
> / Dr. David Alan Gilbert    |       Running GNU/Linux       | Happy  \ 
> \        dave @ treblig.org |                               | In Hex /
>  \ _________________________|_____ http://www.treblig.org   |_______/

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: ERR_PTR(0) in a couple of places
  2023-09-25  4:18 ` Matthew Brost
@ 2023-09-25  4:25   ` Randy Dunlap
  2023-09-25 15:09     ` [Intel-gfx] " Jani Nikula
  0 siblings, 1 reply; 6+ messages in thread
From: Randy Dunlap @ 2023-09-25  4:25 UTC (permalink / raw)
  To: Matthew Brost, Dr. David Alan Gilbert
  Cc: mgreer, krzysztof.kozlowski, linux-kernel, airlied, intel-gfx



On 9/24/23 21:18, Matthew Brost wrote:
> On Sun, Sep 24, 2023 at 12:41:07AM +0000, Dr. David Alan Gilbert wrote:
>> Hi,
>>   I randomly noticed there are a couple of places in the kernel that
>> do
>>    ERR_PTR(0);
>>
>> and thought that was odd - shouldn't those just be NULL's ?
>>
>> 1) i915
>>   drivers/gpu/drm/i915/gt/uc/selftest_guc_multi_lrc.c : 47
>>
>>     if (i <= 1)
>>       return ERR_PTR(0);
> 
> Yes, s/ERR_PTR(0)/ERR_PTR(NULL)/
> 
> Matt

I agree with Dave's original suggestion since casting NULL isn't needed.

> 
>>
>>   from f9d72092cb490 
>>
>> 2) trf7970a
>>   drivers/nfc/trf7970a.c : 896
>>
>>       trf->ignore_timeout =
>>          !cancel_delayed_work(&trf->timeout_work);
>>       trf->rx_skb = ERR_PTR(0);
>>       trf7970a_send_upstream(trf);
>>
>>    from 1961843ceeca0
>>
>> Dave
>> -- 
>>  -----Open up your eyes, open up your mind, open up your code -------   
>> / Dr. David Alan Gilbert    |       Running GNU/Linux       | Happy  \ 
>> \        dave @ treblig.org |                               | In Hex /
>>  \ _________________________|_____ http://www.treblig.org   |_______/

-- 
~Randy

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [Intel-gfx] ERR_PTR(0) in a couple of places
  2023-09-25  4:25   ` Randy Dunlap
@ 2023-09-25 15:09     ` Jani Nikula
  0 siblings, 0 replies; 6+ messages in thread
From: Jani Nikula @ 2023-09-25 15:09 UTC (permalink / raw)
  To: Randy Dunlap, Matthew Brost, Dr. David Alan Gilbert
  Cc: krzysztof.kozlowski, airlied, mgreer, intel-gfx, linux-kernel

On Sun, 24 Sep 2023, Randy Dunlap <rdunlap@infradead.org> wrote:
> On 9/24/23 21:18, Matthew Brost wrote:
>> On Sun, Sep 24, 2023 at 12:41:07AM +0000, Dr. David Alan Gilbert wrote:
>>> Hi,
>>>   I randomly noticed there are a couple of places in the kernel that
>>> do
>>>    ERR_PTR(0);
>>>
>>> and thought that was odd - shouldn't those just be NULL's ?
>>>
>>> 1) i915
>>>   drivers/gpu/drm/i915/gt/uc/selftest_guc_multi_lrc.c : 47
>>>
>>>     if (i <= 1)
>>>       return ERR_PTR(0);
>> 
>> Yes, s/ERR_PTR(0)/ERR_PTR(NULL)/
>> 
>> Matt
>
> I agree with Dave's original suggestion since casting NULL isn't needed.

Yeah, s/ERR_PTR(0)/NULL/ would be my choice as well.

As a side note, I generally think it's better not to mix NULL and error
pointers in error return values for a function, because they're harder
to handle properly.

BR,
Jani.

>
>> 
>>>
>>>   from f9d72092cb490 
>>>
>>> 2) trf7970a
>>>   drivers/nfc/trf7970a.c : 896
>>>
>>>       trf->ignore_timeout =
>>>          !cancel_delayed_work(&trf->timeout_work);
>>>       trf->rx_skb = ERR_PTR(0);
>>>       trf7970a_send_upstream(trf);
>>>
>>>    from 1961843ceeca0
>>>
>>> Dave
>>> -- 
>>>  -----Open up your eyes, open up your mind, open up your code -------   
>>> / Dr. David Alan Gilbert    |       Running GNU/Linux       | Happy  \ 
>>> \        dave @ treblig.org |                               | In Hex /
>>>  \ _________________________|_____ http://www.treblig.org   |_______/

-- 
Jani Nikula, Intel

^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2023-09-25 15:09 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2023-09-24  0:41 ERR_PTR(0) in a couple of places Dr. David Alan Gilbert
2023-09-24 11:41 ` Krzysztof Kozlowski
2023-09-24 12:16   ` Dr. David Alan Gilbert
2023-09-25  4:18 ` Matthew Brost
2023-09-25  4:25   ` Randy Dunlap
2023-09-25 15:09     ` [Intel-gfx] " Jani Nikula

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox