netdev - Re: [PATCH net-next v14 2/7] net: dsa: Add convenience functions for frame handling

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <YyjrrbSjhJjaQ/bK@lunn.ch>
Date:   Tue, 20 Sep 2022 00:22:37 +0200
From:   Andrew Lunn <andrew@...n.ch>
To:     Vladimir Oltean <olteanv@...il.com>
Cc:     Mattias Forsblad <mattias.forsblad@...il.com>,
        netdev@...r.kernel.org, Vivien Didelot <vivien.didelot@...il.com>,
        Florian Fainelli <f.fainelli@...il.com>,
        "David S . Miller" <davem@...emloft.net>,
        Eric Dumazet <edumazet@...gle.com>,
        Jakub Kicinski <kuba@...nel.org>,
        Paolo Abeni <pabeni@...hat.com>, linux@...linux.org.uk,
        ansuelsmth@...il.com
Subject: Re: [PATCH net-next v14 2/7] net: dsa: Add convenience functions for
 frame handling

> > +
> > +	return wait_for_completion_timeout(com, msecs_to_jiffies(timeout));
> 
> If this is going to be provided as a DSA common layer "helper" function,
> at least make an effort to document the expected return code.
> 
> Hint, wait_for_completion_timeout() returns an unsigned long time_left,
> you return an int. What does it mean?!
> 
> At the bare minimum, leave a comment, especially when it's not obvious
> (DSA typically uses negative integer values as error codes, and zero on
> success. Here, zero is an error - timeout. If the amount of time left
> does not matter, do translate this into 0 for success, and -ETIMEDOUT
> for timeout). If you're also feeling generous, do please also update
> Documentation/networking/dsa/dsa.rst with the info about the flow of
> Ethernet-based register access that you wish was available while you
> were figuring out how things worked.

Hi Vladimir

I just posted an alternative to this patch. One of the patches
addresses this point. I convert the return value to the more normal
-ve for error, 0 for success, including -ETIMEOUT if the completion
times out.

My patchset is however missing documentation. Once we get the basic
API agreed on, i can extend Documentation/networking/dsa/dsa.rst.

       Andrew