On 11/18/22 10:42, Vladimir Oltean wrote:

On Fri, Nov 18, 2022 at 10:11:06AM -0500, Sean Anderson wrote:

quoted

quoted

+enum phy_an_inband {
+	PHY_AN_INBAND_UNKNOWN		= BIT(0),

Shouldn't this be something like

	PHY_AN_INBAND_UNKNOWN		= 0,

?

Could be 0 as well. The code explicitly tests against PHY_AN_INBAND_UNKNOWN
everywhere, so the precise value doesn't matter too much.

quoted

What does it mean if a phy returns e.g. 0b101?

You mean PHY_AN_INBAND_ON | PHY_AN_INBAND_UNKNOWN. Well, it doesn't mean
anything, it's not a valid return code. I didn't make the code too defensive
in this regard, because I didn't see a reason for making some pieces of
code defend themselves against other pieces of code. It's a bit mask of
3 bits where not all combinations are valid. Even if PHY_AN_INBAND_UNKNOWN
was defined as 0 instead of BIT(0), it would still be just as logically
invalid to return PHY_AN_INBAND_ON | PHY_AN_INBAND_UNKNOWN, but this
would be indistinguishable in machine code from just PHY_AN_INBAND_ON.

I don't know, I don't see a practical reason to make a change here.

If we have the opportunity, we should try to make invalid return codes
inexpressible. If we remove the extra bit, then all the combinations we
would like to have:

- I don't know what I support
- In-band must be enabled
- In-band must be disabled
- I can support either

are exactly the combinations supported by the underlying data.

--Sean