* 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