| 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: <20160712155739.GB31432@lunn.ch>
Date: Tue, 12 Jul 2016 17:57:39 +0200
From: Andrew Lunn <andrew@...n.ch>
To: Charles-Antoine Couret <charles-antoine.couret@...vision.fr>
Cc: netdev <netdev@...r.kernel.org>,
Florian Fainelli <f.fainelli@...il.com>
Subject: Re: [PATCH v3] Marvell phy: add fiber status check and configuration
for some phys
> +#define LPA_FIBER_1000HALF 0x40
> +#define LPA_FIBER_1000FULL 0x20
> +
> +#define LPA_PAUSE_FIBER 0x180
> +#define LPA_PAUSE_ASYM_FIBER 0x100
> +
> +#define ADVERTISE_FIBER_1000HALF 0x40
> +#define ADVERTISE_FIBER_1000FULL 0x20
> +
> +#define ADVERTISE_PAUSE_FIBER 0x180
> +#define ADVERTISE_PAUSE_ASYM_FIBER 0x100
Are these standardised anywhere? If they are following a standard,
they should be put into include/uapi/linux/mii.h.
> +static inline u32 ethtool_adv_to_fiber_adv_t(u32 ethadv)
> +{
> + u32 result = 0;
> +
> + if (ethadv & ADVERTISED_1000baseT_Half)
> + result |= ADVERTISE_FIBER_1000HALF;
Dumb question: Does 1000baseT_Half even make sense for fibre? Can you
do half duplex? Would that not mean you have a single fibre, both
ends are using the same laser frequency, and you are doing some form
of CSMA/CD?
> + if (ethadv & ADVERTISED_1000baseT_Full)
> + result |= ADVERTISE_FIBER_1000FULL;
> +
> + if ((ethadv & ADVERTISE_PAUSE_ASYM) && (ethadv & ADVERTISE_PAUSE_CAP))
> + result |= LPA_PAUSE_ASYM_FIBER;
> + else if (ethadv & ADVERTISE_PAUSE_CAP)
> + result |= (ADVERTISE_PAUSE_FIBER
> + & (~ADVERTISE_PAUSE_ASYM_FIBER));
> +
> + return result;
> +}
If these values are standardised, i think this function should be
moved into the generic code. If however, this is Marvell specific,
keep it here.
> *
> * Generic status code does not detect Fiber correctly!
> @@ -906,12 +1070,17 @@ static int marvell_read_status(struct phy_device *phydev)
> int lpa;
> int lpagb;
> int status = 0;
> + int page, fiber;
>
> - /* Update the link, but return if there
> + /* Detect and update the link, but return if there
> * was an error */
> - err = genphy_update_link(phydev);
> - if (err)
> - return err;
> + page = phy_read(phydev, MII_MARVELL_PHY_PAGE);
> + if (page == MII_M1111_FIBER)
> + fiber = 1;
> + else
> + fiber = 0;
This read is expensive, since the MDIO bus is slow. It would be better
just to pass fibre as a parameter.
> +/* marvell_read_fiber_status
> + *
> + * Some Marvell's phys have two modes: fiber and copper.
> + * Both need status checked.
> + * Description:
> + * First, check the fiber link and status.
> + * If the fiber link is down, check the copper link and status which
> + * will be the default value if both link are down.
> + */
> +static int marvell_read_fiber_status(struct phy_device *phydev)
The name is a bit confusing. I would probably use
marvell_read_copper_fiber_status() making it clear it reads both.
> +{
> + int err;
> +
> + /* Check the fiber mode first */
> + err = phy_write(phydev, MII_MARVELL_PHY_PAGE, MII_M1111_FIBER);
> + if (err < 0)
> + goto error;
> +
> + err = marvell_read_status(phydev);
> + if (err < 0)
> + goto error;
> +
> + if (phydev->link) {
> + phy_write(phydev, MII_MARVELL_PHY_PAGE, MII_M1111_COPPER);
> + return 0;
> + }
> +
> + /* If fiber link is down, check and save copper mode state */
> + err = phy_write(phydev, MII_MARVELL_PHY_PAGE, MII_M1111_COPPER);
> + if (err < 0)
> + goto error;
There should be a big fat comment somewhere that after this function
the copper or fibre page is left selected, depending on which has
link. There is a danger somebody misses this assumption and breaks
the code by unconditionally restoring to copper.
> +
> + return marvell_read_status(phydev);
> +
> +error:
> + phy_write(phydev, MII_MARVELL_PHY_PAGE, MII_M1111_COPPER);
> + return err;
> +}
> +
> +/* marvell_suspend_fiber
> + *
> + * Some Marvell's phys have two modes: fiber and copper.
> + * Both need to be suspended
> + */
> +static int marvell_suspend_fiber(struct phy_device *phydev)
> +{
> + int err;
> +
> + /* Suspend the fiber mode first */
> + err = phy_write(phydev, MII_MARVELL_PHY_PAGE, MII_M1111_FIBER);
> + if (err < 0)
> + goto error;
> +
> + err = genphy_suspend(phydev);
> + if (err < 0)
> + goto error;
> +
> + /* Then, the copper link */
> + err = phy_write(phydev, MII_MARVELL_PHY_PAGE, MII_M1111_COPPER);
> + if (err < 0)
> + goto error;
> +
> + return genphy_suspend(phydev);
> +
> +error:
> + phy_write(phydev, MII_MARVELL_PHY_PAGE, MII_M1111_COPPER);
> + return err;
> +}
I think it would be better to look for SUPPORTED_FIBRE in
drv->features, rather than have two different functions.
In fact, i would do that in general, rather than add your _fibre()
functions.
> +
> +/* marvell_resume_fiber
> + *
> + * Some Marvell's phys have two modes: fiber and copper.
> + * Both need to be resumed
> + */
> +static int marvell_resume_fiber(struct phy_device *phydev)
> +{
> + int err;
> +
> + /* Resume the fiber mode first */
> + err = phy_write(phydev, MII_MARVELL_PHY_PAGE, MII_M1111_FIBER);
> + if (err < 0)
> + goto error;
> +
> + err = genphy_resume(phydev);
> + if (err < 0)
> + goto error;
> +
> + /* Then, the copper link */
> + err = phy_write(phydev, MII_MARVELL_PHY_PAGE, MII_M1111_COPPER);
> + if (err < 0)
> + goto error;
> +
> + return genphy_resume(phydev);
Should it be resumed twice? Or just once at the end? Same question
for suspend.
> @@ -1130,6 +1416,11 @@ static u64 marvell_get_stat(struct phy_device *phydev, int i)
> int err, oldpage, val;
> u64 ret;
>
> + if (!(phydev->supported & SUPPORTED_FIBRE)) {
> + if (strstr(marvell_hw_stats[i].string, "fiber"))
> + return 0;
I think a better solution is for marvell_get_sset_count() to return 2
or 3 depending on phydev->supported & SUPPORTED_FIBRE.
Andrew
Powered by blists - more mailing lists