Re: [PATCH net-next v14 2/7] net: dsa: Add convenience functions for frame handling
flat view
From: Vladimir Oltean <olteanv@gmail.com>
Date: 2022-09-19 22:14:54
On Mon, Sep 19, 2022 at 01:08:42PM +0200, Mattias Forsblad wrote:
quoted hunk ↗ jump to hunk
+int dsa_switch_inband_tx(struct dsa_switch *ds, struct sk_buff *skb, + struct completion *completion, unsigned long timeout) +{ + struct completion *com; + + /* Custom completion? */ + com = completion ? : &ds->inband_done; + + reinit_completion(com); + + if (skb) + dev_queue_xmit(skb);
Does it make sense from an API perspective to call dsa_switch_inband_tx() with a NULL skb? If yes, please add a comment explaining why. If not, please delete this check.
+ + 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.
+} +EXPORT_SYMBOL_GPL(dsa_switch_inband_tx);