| 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: <e64fdb4a-f05d-4a99-aabd-221ed4376580@intel.com>
Date: Thu, 3 Oct 2024 09:36:54 +0200
From: Mateusz Polchlopek <mateusz.polchlopek@...el.com>
To: Qingtao Cao <qingtao.cao.au@...il.com>
CC: Qingtao Cao <qingtao.cao@...i.com>, Andrew Lunn <andrew@...n.ch>, "Heiner
Kallweit" <hkallweit1@...il.com>, Russell King <linux@...linux.org.uk>,
"David S. Miller" <davem@...emloft.net>, Eric Dumazet <edumazet@...gle.com>,
"Jakub Kicinski" <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>,
<netdev@...r.kernel.org>, <linux-kernel@...r.kernel.org>
Subject: Re: v2 [PATCH 1/1] net: phy: marvell: avoid bringing down fibre link
when autoneg is bypassed
On 10/3/2024 9:14 AM, Qingtao Cao wrote:
> Hi Mateusz,
>
> Please see my inline relies.
>
> On Thu, Oct 3, 2024 at 4:54 PM Mateusz Polchlopek
> <mateusz.polchlopek@...el.com <mailto:mateusz.polchlopek@...el.com>> wrote:
>
>
> On 10/3/2024 6:45 AM, Qingtao Cao wrote:
> > On 88E151x the SGMII autoneg bypass mode defaults to be enabled.
> When it is
> > activated, the device assumes a link-up status with existing
> configuration
> > in BMCR, avoid bringing down the fibre link in this case
> >
> > Test case:
> > 1. Two 88E151x connected with SFP, both enable autoneg, link is
> up with speed
> > 1000M
>
> checkpatch.pl <http://checkpatch.pl> complains about this line, it
> exceeds 75 chars allowed for
> commit msg. Please adjust.
>
> > 2. Disable autoneg on one device and explicitly set its speed to
> 1000M
> > 3. The fibre link can still up with this change, otherwise not.
> >
> > Signed-off-by: Qingtao Cao <qingtao.cao@...i.com
> <mailto:qingtao.cao@...i.com>>
> > ---
> > drivers/net/phy/marvell.c | 23 ++++++++++++++++++++++-
> > 1 file changed, 22 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/net/phy/marvell.c b/drivers/net/phy/marvell.c
> > index 9964bf3dea2f..e3a8ad8b08dd 100644
> > --- a/drivers/net/phy/marvell.c
> > +++ b/drivers/net/phy/marvell.c
> > @@ -195,6 +195,10 @@
> >
> > #define MII_88E1510_MSCR_2 0x15
> >
> > +#define MII_88E1510_FSCR2 0x1a
>
> Please use GENMASK_ULL for creating mask.
>
>
> The serial interface autoneg bypass mode and active status are just two
> separate bit in the FSCR2 register, instead of complicated masks, so I
> think BIT() serves them right.
Ach, sorry, the GENMASK_ULL usage is not applicable here.
>
> > +#define MII_88E1510_FSCR2_BYPASS_ENABLE BIT(6)
> > +#define MII_88E1510_FSCR2_BYPASS_STATUS BIT(5)
> > +
> > #define MII_VCT5_TX_RX_MDI0_COUPLING 0x10
> > #define MII_VCT5_TX_RX_MDI1_COUPLING 0x11
> > #define MII_VCT5_TX_RX_MDI2_COUPLING 0x12
> > @@ -1623,11 +1627,28 @@ static void
> fiber_lpa_mod_linkmode_lpa_t(unsigned long *advertising, u32 lpa)
> > static int marvell_read_status_page_an(struct phy_device *phydev,
> > int fiber, int status)
> > {
> > + int fscr2;
> > int lpa;
> > int err;
> >
> > if (!(status & MII_M1011_PHY_STATUS_RESOLVED)) {
> > - phydev->link = 0;
> > + if (!fiber) {
> > + phydev->link = 0;
> > + } else {
> > + fscr2 = phy_read(phydev, MII_88E1510_FSCR2);
> > + if (fscr2 > 0) {
> > + if ((fscr2 &
> MII_88E1510_FSCR2_BYPASS_ENABLE) &&
> > + (fscr2 &
> MII_88E1510_FSCR2_BYPASS_STATUS)) {
> > + if
> (genphy_read_status_fixed(phydev) < 0)
> > + phydev->link = 0;
> > + } else {
> > + phydev->link = 0;
> > + }
> > + } else {
> > + phydev->link = 0;
> > + }
> > + }
> > +
> > return 0;
> > }
> >
>
> So many levels of indentation... Couldn't it be merged somehow? I do not
> know, maybe create local variable, store the current state of
> phydev->link, then set phydev->link = 0 and restore it from local
> variable only if (fiber && fscr2 > 0 && (fscr2 &
> MII_88E1510_FSCR2_BYPASS_ENABLE) && (fscr2 &
> MII_88E1510_FSCR2_BYPASS_STATUS) && genphy_read_status_fixed(phydev)
> >=0
> ) ...
>
> or other way? Now you have 5 (!) levels of indentation and almost
> everywhere you just set phydev->link to 0 depends on the condition.
>
>
> Thanks for the feedback, please check the v3 patch just sent.
>
I will take a look but a general rule is to wait a little bit longer (at
least two working days?) before sending next version of patch. It is
mainly to give a chance for taking a look by others. Thanks
>
> BTW. We put "v2" inside the tag in the topic and specify the tree, so
> instead of:
> v2 [PATCH 1/1] net: phy: marvell: avoid bringing down fibre link when
> autoneg is bypassed
>
> it should be:
> [PATCH net-next v2 1/1] net: phy: marvell: avoid bringing down fibre
> link when autoneg is bypassed
>
>
> Thanks again, also improved in the v3 patch header.
>
> Warm regards,
> Qingtao
Powered by blists - more mailing lists