On Thu, Jan 3, 2019 at 7:31 AM Roland Hieber <rhi@xxxxxxxxxxxxxx> wrote:
>
> On Wed, Dec 12, 2018 at 11:03:33PM -0800, Andrey Smirnov wrote:
> > A number of custom error codes used by e1000 driver will be propagated
> > all the way up to generic networking code and will end up being fed to
> > strerror(). As a result of that, some of the current error codes will
> > result in not very helpful failure messages. For example, trying to
> > ping a host on a system where access to i210's EEPROM fails results in
> > the following message:
> >
> > barebox@ZII RDU2 Board:/ ping 192.168.53.7
> > ping failed: Operation not permitted
> >
> > In order to make message like that one a little bit more helpful,
> > change definitions of various E1000_ERR_* constants to map to a bit
> > more appropriate error codes.
> >
> > While at it, remove E1000_ERR_MASTER_REQUESTS_PENDING and
> > E1000_ERR_HOST_INTERFACE_COMMAND that are not referenced anywhere in
> > the codebase.
> >
> > Signed-off-by: Andrey Smirnov <andrew.smirnov@xxxxxxxxx>
> > ---
> > drivers/net/e1000/e1000.h | 24 +++++++++++-------------
> > 1 file changed, 11 insertions(+), 13 deletions(-)
> >
> > diff --git a/drivers/net/e1000/e1000.h b/drivers/net/e1000/e1000.h
> > index 4a1a1aa33..0a9e107c0 100644
> > --- a/drivers/net/e1000/e1000.h
> > +++ b/drivers/net/e1000/e1000.h
> > @@ -95,19 +95,17 @@ typedef enum {
> >
> > /* Error Codes */
> > #define E1000_SUCCESS 0
> > -#define E1000_ERR_EEPROM 1
> > -#define E1000_ERR_PHY 2
> > -#define E1000_ERR_CONFIG 3
> > -#define E1000_ERR_PARAM 4
> > -#define E1000_ERR_MAC_TYPE 5
> > -#define E1000_ERR_PHY_TYPE 6
> > -#define E1000_ERR_NOLINK 7
> > -#define E1000_ERR_TIMEOUT 8
> > -#define E1000_ERR_RESET 9
> > -#define E1000_ERR_MASTER_REQUESTS_PENDING 10
> > -#define E1000_ERR_HOST_INTERFACE_COMMAND 11
> > -#define E1000_BLK_PHY_RESET 12
> > -#define E1000_ERR_SWFW_SYNC 13
> > +#define E1000_ERR_EEPROM EIO
> > +#define E1000_ERR_PHY EIO
> > +#define E1000_ERR_CONFIG EINVAL
> > +#define E1000_ERR_PARAM EINVAL
> > +#define E1000_ERR_MAC_TYPE EINVAL
> > +#define E1000_ERR_PHY_TYPE EINVAL
> > +#define E1000_ERR_NOLINK ENETDOWN
> > +#define E1000_ERR_TIMEOUT ETIMEDOUT
> > +#define E1000_ERR_RESET EIO
> > +#define E1000_BLK_PHY_RESET EWOULDBLOCK
> > +#define E1000_ERR_SWFW_SYNC EBUSY
>
> Just a small nitpick, and I'm not sure it's relevant somewhere in the
> code: previously the mapping of symbols to numbers was bijective, but
> now it is no longer surjective. This may result in confusion when
> checking return codes, for example:
>
> int ret = e1000_set_phy_mode(...;
> if (ret == E1000_ERR_EEPROM) {
> printf("could not read from eeprom\n");
> } else if (ret == E1000_ERR_PHY) {
> printf("could not write to phy register\n");
> }
>
> In this case, when the e1000_set_phy_mode() code path returns
> E1000_ERR_PHY, it is interpreted as E1000_ERR_EEPROM because both are
> defined to EIO.
>
That's definitely true. When writing this patch I looked for usages
similar to what you describe in the actual code of the driver, but
didn't see anything that should cause a problem. The driver has a
pretty sizable set of error codes, but AFAICT none of them are really
used as anything more than a negative number passed up the call chain.
Grepping for "E1000_ERR_*" in drivers/net/e1000 doesn't seem to show
any usages in any comparison statements. Finding all of the points in
the driver where error codes cross over into generic codebase and
doing errno re-mapping there seemed like a more invasive/complicated
alternative, so I did go for it.
That's how my thinking went, anyway. I can re-do the patch if we
decide that maintaining unique ID for each E1000_ERR_* code is
desired.
Thanks,
Andrey Smirnov
_______________________________________________
barebox mailing list
barebox@xxxxxxxxxxxxxxxxxxx
http://lists.infradead.org/mailman/listinfo/barebox
