ERR_PTR pattern in phylib

ERR_PTR pattern in phylib

[Grant Likely]

Hi Andy,

I've been looking at the phylib code, and specifically at the use of
the ERR_PTR() pattern.  I'm personally not a big fan of ERR_PTR()
because the compiler cannot type check the result and it runs
completely counter to the pattern "if (!ptr)" than is common for
testing a pointer return value.

(That being said, I do understand the need for it in certain parts of
the kernel (like the filesystem code) where it is important to be both
efficient because it is a hot path and still able to accurately return
an error code that is used by userspace.)

It seems to me that phylib is one of the cases where the users (the
network drivers) don't actually care about the specific error code
when calling phylib functions.  The drivers only seem to care whether
or not the function failed, and if it did then bail out.  I've also
noticed that using the "if (!ptr)" test on phylib return values is a
common error for driver writers.

In the interest of making driver code easier to write and review,
would you be opposed to a set of patches to remove the ERR_PTR()
pattern from phylib and its users?

g.

-- 
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.

Re: ERR_PTR pattern in phylib

[David Miller]

From: Grant Likely <grant.likely@secretlab.ca>
Date: Wed, 1 Sep 2010 08:42:49 -0600

> It seems to me that phylib is one of the cases where the users (the
> network drivers) don't actually care about the specific error code
> when calling phylib functions.  The drivers only seem to care whether
> or not the function failed, and if it did then bail out.  I've also
> noticed that using the "if (!ptr)" test on phylib return values is a
> common error for driver writers.
> 
> In the interest of making driver code easier to write and review,
> would you be opposed to a set of patches to remove the ERR_PTR()
> pattern from phylib and its users?

I'm opposed to it because it means that if code actually does
care about the error code it will no longer be able to obtain
it.

Please don't make this change, thanks.

Re: ERR_PTR pattern in phylib

[Fleming Andy-AFLEMING]

[-- Attachment #1: Type: text/plain, Size: 681 bytes --]



On Sep 1, 2010, at 9:43, Grant Likely <grant.likely@secretlab.ca> wrote:

> 
> In the interest of making driver code easier to write and review,
> would you be opposed to a set of patches to remove the ERR_PTR()
> pattern from phylib and its users?


Not at all.  I was trying to make sure all the information was available, and I'm not really a big fan of passing in a **ptr to get set.  But I don't really have a strong opinion.  This is just the design i settled on.  Heck, 99.9% of the errors are unlikely to ever happen, a they're caused by hardware malfunction!

Andy


> 
> g.
> 
> -- 
> Grant Likely, B.Sc., P.Eng.
> Secret Lab Technologies Ltd.
> 

[-- Attachment #2: Type: text/html, Size: 1236 bytes --]

Re: ERR_PTR pattern in phylib

[Grant Likely]

On Wed, Sep 1, 2010 at 9:27 AM, David Miller <davem@davemloft.net> wrote:
> From: Grant Likely <grant.likely@secretlab.ca>
> Date: Wed, 1 Sep 2010 08:42:49 -0600
>
>> It seems to me that phylib is one of the cases where the users (the
>> network drivers) don't actually care about the specific error code
>> when calling phylib functions. =A0The drivers only seem to care whether
>> or not the function failed, and if it did then bail out. =A0I've also
>> noticed that using the "if (!ptr)" test on phylib return values is a
>> common error for driver writers.
>>
>> In the interest of making driver code easier to write and review,
>> would you be opposed to a set of patches to remove the ERR_PTR()
>> pattern from phylib and its users?
>
> I'm opposed to it because it means that if code actually does
> care about the error code it will no longer be able to obtain
> it.

The error codes in phylib are almost arbitrary and don't really give
enough information about where the a failure lies.  dev_err() is more
useful for debugging.

My experience has been that the encoding of error numbers into a
pointer return is a source of bugs for driver writers and should be
strongly avoided unless the return codes are actually important (ie.
userspace depends on them).  Especially so when the use-case for
keeping them is merely theoretical.  I've looked through the phylib
usage for ERR_PTR(), and it all is related to whether or not a
phy_device pointer can be located.  The error code is pretty much
irrelevant if a phy cannot be obtained.

How about this as a compromise:  I'll investigate all the users of
phylib and if I find even one situation where the specific return code
is actually important to a driver, then I'll back off.  phylib has
been around for 5 years now which should be enough time for that use
case to bubble to the surface.

g.

Re: ERR_PTR pattern in phylib

[David Miller]

From: Grant Likely <grant.likely@secretlab.ca>
Date: Wed, 1 Sep 2010 12:56:29 -0600

> The error codes in phylib are almost arbitrary and don't really give
> enough information about where the a failure lies.  dev_err() is more
> useful for debugging.

If it's using bad error codes, that's only indicative of a bug in
phylib.

> How about this as a compromise:  I'll investigate all the users of
> phylib and if I find even one situation where the specific return code
> is actually important to a driver, then I'll back off.  phylib has
> been around for 5 years now which should be enough time for that use
> case to bubble to the surface.

Sorry, I'm not agreeable to this.

If there are bugs where drivers are not checking the return
pointers correctly, please just fix those bugs.  Otherwise
I see things fine as they are.

Re: ERR_PTR pattern in phylib

[Maciej W. Rozycki]

On Wed, 1 Sep 2010, Grant Likely wrote:

> I've been looking at the phylib code, and specifically at the use of
> the ERR_PTR() pattern.  I'm personally not a big fan of ERR_PTR()
> because the compiler cannot type check the result and it runs
> completely counter to the pattern "if (!ptr)" than is common for
> testing a pointer return value.

 Arguably using a union here would make things cleaner and any reasonable 
ABI will place small unions used as arguments or return values in 
registers, but I'm not sure if the cost of the rewrite is worth the 
benefit.  Probably not, but feel free to propose a change.

  Maciej

Re: ERR_PTR pattern in phylib

[David Miller]

From: "Maciej W. Rozycki" <macro@linux-mips.org>
Date: Thu, 2 Sep 2010 16:20:34 +0100 (BST)

>  Arguably using a union here would make things cleaner and any reasonable 
> ABI will place small unions used as arguments or return values in 
> registers, but I'm not sure if the cost of the rewrite is worth the 

Well, sparc 32-bit's ABI for one is "not reasonable" and these return
values will always get stack slots and passed back by reference.

Re: ERR_PTR pattern in phylib

[Maciej W. Rozycki]

On Thu, 2 Sep 2010, David Miller wrote:

> >  Arguably using a union here would make things cleaner and any reasonable 
> > ABI will place small unions used as arguments or return values in 
> > registers, but I'm not sure if the cost of the rewrite is worth the 
> 
> Well, sparc 32-bit's ABI for one is "not reasonable" and these return
> values will always get stack slots and passed back by reference.

 So that makes it two -- together with MIPS o32. ;)  Still used by 
everybody concerned as contraptions tend to stick. :/  That doesn't make 
it reasonable though.  Nor me to push the union approach -- that's an 
elegant approach that would probably give us less than take in return.

  Maciej