| 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
| ||
|
Message-ID: <20150918135746.GG21084@n2100.arm.linux.org.uk>
Date: Fri, 18 Sep 2015 14:57:46 +0100
From: Russell King - ARM Linux <linux@....linux.org.uk>
To: Stas Sergeev <stsp@...t.ru>
Cc: David Miller <davem@...emloft.net>, andrew@...n.ch,
linux-arm-kernel@...ts.infradead.org, netdev@...r.kernel.org
Subject: Re: mvneta: SGMII fixed-link not so fixed
On Fri, Sep 18, 2015 at 04:43:59PM +0300, Stas Sergeev wrote:
> 18.09.2015 16:12, Russell King - ARM Linux пишет:
> > On Fri, Sep 18, 2015 at 03:43:54PM +0300, Stas Sergeev wrote:
> >> 18.09.2015 15:13, Russell King - ARM Linux пишет:
> >>> On Fri, Sep 18, 2015 at 02:29:34PM +0300, Stas Sergeev wrote:
> >>>> 18.09.2015 02:14, Russell King - ARM Linux пишет:
> >>>>> _But_ using the in-band status
> >>>>> property fundamentally requires this for mvneta to behave correctly:
> >>>>>
> >>>>> phy-mode = "sgmii";
> >>>>> managed = "in-band-status";
> >>>>> fixed-link {
> >>>>> };
> >>>>>
> >>>>> with _no_ phy node.
> >>>> I don't understand this one.
> >>>> At least for me it works without empty fixed-link.
> >>>> There may be some bug.
> >>>
> >>> if (cause_rx_tx & MVNETA_MISCINTR_INTR_MASK) {
> >>> u32 cause_misc = mvreg_read(pp, MVNETA_INTR_MISC_CAUSE);
> >>>
> >>> mvreg_write(pp, MVNETA_INTR_MISC_CAUSE, 0);
> >>> if (pp->use_inband_status && (cause_misc &
> >>> (MVNETA_CAUSE_PHY_STATUS_CHANGE |
> >>> MVNETA_CAUSE_LINK_CHANGE |
> >>> MVNETA_CAUSE_PSC_SYNC_CHANGE))) {
> >>> mvneta_fixed_link_update(pp, pp->phy_dev);
> >>> }
> >>>
> >>> pp->use_inband_status is set when managed = "in-band-status" is set.
> >>> We detect changes in the in-band status, and call mvneta_fixed_link_update():
> >>>
> >>> mvneta_fixed_link_update() reads the status, and communicates that into
> >>> the fixed-link phy:
> >>>
> >>> u32 gmac_stat = mvreg_read(pp, MVNETA_GMAC_STATUS);
> >>>
> >>> ... code setting status.* values from gmac_stat ...
> >>> changed.link = 1;
> >>> changed.speed = 1;
> >>> changed.duplex = 1;
> >>> fixed_phy_update_state(phy, &status, &changed);
> >>>
> >>> fixed_phy_update_state() then looks up the phy in its list, comparing only
> >>> the address:
> >>>
> >>> if (!phydev || !phydev->bus)
> >>> return -EINVAL;
> >>>
> >>> list_for_each_entry(fp, &fmb->phys, node) {
> >>> if (fp->addr == phydev->addr) {
> >>>
> >>> updating fp->* with the new state, calling fixed_phy_update_regs(). This
> >>> updates the fixed-link phy emulated registers, and phylib then notices
> >>> the change in link status, and notifies the netdevice attached to the
> >>> PHY it found of the change.
> >>>
> >>> Now, one of two things happens as a result of this:
> >>>
> >>> 1. If pp->phy_dev is a fixed-link phy, this finds the correct fixed-link
> >>> phy to update its "fixed-link" properties, and the "not so fixed" phy
> >>> changes its parameters according to the new status.
> >>>
> >>> 2. If pp->phy_dev is a MDIO phy which matches the address of a fixed-link
> >>> phy,
> >> Doesn't the above loop iterates only "fixed_mdio_bus platform_fmb"?
> >> I don't think MDIO PHYs can appear there. If they can - the bug is
> >> very nasty. Have you seen exactly when/why that happens?
> >
> > I think I explained it fully - please follow the code paths I've detailed
> > above.
> I try. What I don't understand is why both PHYs get the
> same address on the "Fixed MDIO bus".
They aren't both on the fixed MDIO bus - that's the whole point I'm
making. They get the same phydev->addr but on _different_ buses.
> > Specifically, look at this code:
> >
> > if (!phydev || !phydev->bus)
> > return -EINVAL;
> >
> > list_for_each_entry(fp, &fmb->phys, node) {
> > if (fp->addr == phydev->addr) {
Look at this closely - at what point is there any validation that "phydev"
is actually a fixed-link phy? There is no validation done. The only
criteria there are:
- phydev is not NULL
- phydev->bus is not NULL (which is true of any registered phy)
- phydev->addr matches _any_ fixed-link phy.
I've already sent a patch earlier today to address this issue.
If you place a WARN_ON(fp->phydev != phydev) then that'll show you
when it incorrectly matches.
> > Consider what the effect is if you have a MDIO phy at address 0 on eth0
> > which has in-band-status enabled.
> So as I understand, you have MDIO phy with DT looking like this:
> ethernet@...00 {
> status = "okay";
> phy-mode = "sgmii";
> managed = "in-band-status";
> }
> W/O either "phy" of "fixed-link" nodes. Correct?
> mvneta calls of_phy_register_fixed_link(dn) on it after not
> finding the "phy" node. And it will do the same with the second
> non-MDIO phy. What I don't see is how do they get the same addr
> on the same bus, could you please clarify that a bit?
mdio@...04 {
phy_dedicated: ethernet-phy@0 {
reg = <0>;
};
};
eth1: ethernet@...00 {
phy = <&phy_dedicated>;
phy-mode = "sgmii";
managed = "in-band-status";
};
eth0: ethernet@...00 {
phy-mode = "sgmii";
fixed-link {
speed = 1000;
full-duplex;
};
};
Bring eth0 up first, everything works. Then, bring eth1 up, and eth0
goes down.
This happens because when eth1 is brought up, eth1's mvneta calls into
fixed_phy_update_state() with a pointer to the "phy_dedicated" PHY at
address 0. fixed_phy_update_state() scans the fixed-link phys for one
at address 0, and finds the fixed-link phy associated with eth0.
This causes the fixed link code to change the settings for eth0.
As I have already shown in my previous setup diagrams, it is _entirely_
reasonable to use in-band status with SGMII with a phy attached.
--
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Powered by blists - more mailing lists