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 |_______/
* Krzysztof Kozlowski ([email protected]) 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 |_______/
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
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 |_______/
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
On Sun, 24 Sep 2023, Randy Dunlap <[email protected]> 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