netdev - Re: [PATCH net-next 1/2] ixgbe: register a mdiobus

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: <ae364876-d63d-e3f4-71aa-ad81ab0114f1@silicom-usa.com>
Date:   Fri, 30 Nov 2018 15:22:04 +0000
From:   Steve Douthit <stephend@...icom-usa.com>
To:     Andrew Lunn <andrew@...n.ch>
CC:     Jeff Kirsher <jeffrey.t.kirsher@...el.com>,
        "David S. Miller" <davem@...emloft.net>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>
Subject: Re: [PATCH net-next 1/2] ixgbe: register a mdiobus

On 11/30/18 8:21 AM, Andrew Lunn wrote:
> Hi Steve
> 
> Cool to see another interface being made DSA capable.
> 
>> +/**
>> + *  ixgbe_msca - Write the command register and poll for completion/timeout
>> + *  @hw: pointer to hardware structure
>> + *  @cmd: command register value to write
>> + **/
>> +static s32 ixgbe_msca_cmd(struct ixgbe_hw *hw, u32 cmd)
>> +{
>> +	u32 i;
>> +
>> +	IXGBE_WRITE_REG(hw, IXGBE_MSCA, cmd);
>> +
>> +	for (i = 0; i < IXGBE_MDIO_COMMAND_TIMEOUT; i++) {
>> +		udelay(10);
>> +		cmd = IXGBE_READ_REG(hw, IXGBE_MSCA);
>> +		if (!(cmd & IXGBE_MSCA_MDI_COMMAND))
>> +			return 0;
>> +	}
>> +
>> +	return -ETIMEDOUT;
> 
> Please consider using readx_poll_timeout()

OK

>> +}
>> +
>> +/**
>> + *  ixgbe_msca - Use device_id to figure out if MDIO bus is shared between MACs.
>> + *  The embedded x550(s) in the C3000 line of SoCs only have a single mii_bus
>> + *  shared between all MACs, and that introduces some new mask flags that need
>> + *  to be passed to the *_swfw_sync callbacks.
>> + *  @hw: pointer to hardware structure
>> + **/
>> +static bool ixgbe_is_shared_mii(struct ixgbe_hw *hw)
>> +{
>> +	switch (hw->device_id) {
>> +	case IXGBE_DEV_ID_X550EM_A_KR:
>> +	case IXGBE_DEV_ID_X550EM_A_KR_L:
>> +	case IXGBE_DEV_ID_X550EM_A_SFP_N:
>> +	case IXGBE_DEV_ID_X550EM_A_SGMII:
>> +	case IXGBE_DEV_ID_X550EM_A_SGMII_L:
>> +	case IXGBE_DEV_ID_X550EM_A_10G_T:
>> +	case IXGBE_DEV_ID_X550EM_A_SFP:
>> +	case IXGBE_DEV_ID_X550EM_A_1G_T:
>> +	case IXGBE_DEV_ID_X550EM_A_1G_T_L:
>> +		return true;
>> +	}
>> +
>> +	return false;
>> +}
>> +
>> +/**
>> + *  ixgbe_mii_bus_read - Read a clause 22/45 register
>> + *  @hw: pointer to hardware structure
>> + *  @addr: address
>> + *  @regnum: register number
>> + **/
>> +static s32 ixgbe_mii_bus_read(struct mii_bus *bus, int addr, int regnum)
>> +{
>> +	struct ixgbe_adapter *adapter = (struct ixgbe_adapter *)bus->priv;
>> +	struct ixgbe_hw *hw = &adapter->hw;
>> +	u32 hwaddr, cmd, gssr = hw->phy.phy_semaphore_mask;
>> +	s32 data;
>> +
>> +	if (ixgbe_is_shared_mii(hw)) {
>> +		gssr |= IXGBE_GSSR_TOKEN_SM;
>> +		if (hw->bus.lan_id)
>> +			gssr |= IXGBE_GSSR_PHY1_SM;
>> +		else
>> +			gssr |= IXGBE_GSSR_PHY0_SM;
>> +	}
> 
> Could you explain this shared bus a bit more.  If there is only one
> physical MDIO bus, you should only register one bus with Linux. So i
> would not normally expect to see ixgbe_is_shared_mii() sort of
> statements into the read/write functions. That tends to happen at the
> point you are registering the MDIO bus, and when looking for the MACs
> PHY on the bus.

Yep, registering multiple interfaces is wrong.  The first board I tested
against only had a single MAC enabled (they can be disabled/hidden via
straps) so it just happened to work.  Then I moved to a board with all
four interfaces enabled and found this.

The Intel C3xxx family of SoCs have up to four ixgbe MACs.  These are
structured as two devices of two functions each on fixed internal root
ports.

from lspci:
<snip>
            +-16.0-[05]--+-00.0
            |            \-00.1
            +-17.0-[06]--+-00.0
            |            \-00.1
<snip>

Each of those MACs exposes the same set of MDIO control registers as if
there are four interface, but there's really only a single bus in the
hardware for these SoCs.

How about I change:
1) ixgbe_is_shared_mii() -> ixgbe_is_x550em_a()
2) ixgbe_mii_bus_(read|write) -> ixgbe_x550em_a_mii_bus_(read|write) and
    update the phy_semaphore_mask unconditionally
3) Only have ixgbe_mii_bus_init() register something for the x550em_a
    devices for now, and leave handling other platforms for the future.
4) Have ixgbe_mii_bus_init() register only a single mdiobus for func 0
    of the device downstream of the 00:16.0 root port, or if that's been
    disabled then func 0 downstream of the 00:17.0 root port.  All of the
    devices on bus 0 of the SoC are at fixed device/function locations.

Would that be acceptable?