Skip to contentSkip to conversation

Question about settings array in phy.c

DISCUSSION Tom Lendacky 2 participants 2 messages Jul 11, 2014

Subsystems (2)
lore
Share & tools
lore
#0 lore raw

In drivers/net/phy/phy.c there is an array of struct phy_setting named settings that is used when auto negotiation is disabled to sanitize the phy settings. This array uses only *baseT* features for validating the various speed/duplex combinations. This may result in not finding the correct array entry in regards to speed and duplex if the *baseT* feature is not supported.

For example, a 10GbE phy that supports KR mode would end up not matching an entry and defaulting to the last entry of the array (SPEED_10 and DUPLEX_HALF) which is not what is desired.

Is it appropriate to be able to extend this array? Either by adding additional array entries or extending the "setting" to include other features? Or is there a specific reason that only the *baseT* values are used?

Examples of changing the array:

Add a new entry

	{
		.speed = SPEED_10000,
		.duplex = DUPLEX_FULL,
		.setting = SUPPORTED_10000baseKR_Full,
	},

or extend current entry

	{
		.speed = SPEED_10000,
		.duplex = DUPLEX_FULL,
		.setting = SUPPORTED_10000baseT_Full |
			   SUPPORTED_10000baseKR_Full,
	},

Thanks,
Tom

#1 lore raw

Hi Tom,

2014-07-11 8:50 GMT-07:00 Tom Lendacky <[email protected]>:

Quoted 6 lines

In drivers/net/phy/phy.c there is an array of struct phy_setting named settings that is used when auto negotiation is disabled to sanitize the phy settings. This array uses only *baseT* features for validating the various speed/duplex combinations. This may result in not finding the correct array entry in regards to speed and duplex if the *baseT* feature is not supported.

Part of the reason is that libphy was designed around 2006-2007 and initially only supported 10/100/1000 PHY devices, and until recently it did not support 10GbE PHYs at all.

Quoted 9 lines

For example, a 10GbE phy that supports KR mode would end up not matching an entry and defaulting to the last entry of the array (SPEED_10 and DUPLEX_HALF) which is not what is desired.

Is it appropriate to be able to extend this array? Either by adding additional array entries or extending the "setting" to include other features? Or is there a specific reason that only the *baseT* values are used?

It seems to me like it will be easier to refine/extend the matching logic by adding a new entries which contain a single 'setting' to be matched against, such as the one you show below:

Quoted 20 lines

Examples of changing the array:

  Add a new entry
        {
                .speed = SPEED_10000,
                .duplex = DUPLEX_FULL,
                .setting = SUPPORTED_10000baseKR_Full,
        },

  or extend current entry
        {
                .speed = SPEED_10000,
                .duplex = DUPLEX_FULL,
                .setting = SUPPORTED_10000baseT_Full |
                           SUPPORTED_10000baseKR_Full,
        },

Thanks,
Tom

Signature · 1 line
Florian