| 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:
<DM4PR11MB623907EC9F1C76CC7F14D3588B632@DM4PR11MB6239.namprd11.prod.outlook.com>
Date: Thu, 19 Sep 2024 12:04:38 +0000
From: <Tarun.Alle@...rochip.com>
To: <linux@...linux.org.uk>
CC: <Arun.Ramadoss@...rochip.com>, <UNGLinuxDriver@...rochip.com>,
<andrew@...n.ch>, <hkallweit1@...il.com>, <davem@...emloft.net>,
<edumazet@...gle.com>, <kuba@...nel.org>, <pabeni@...hat.com>,
<netdev@...r.kernel.org>, <linux-kernel@...r.kernel.org>
Subject: RE: [PATCH net-next V4] net: phy: microchip_t1: SQI support for
LAN887x
Hi Russell,
Thanks for your review comments. Will fix in the next version.
> -----Original Message-----
> From: Russell King <linux@...linux.org.uk>
> Sent: Tuesday, September 17, 2024 7:59 PM
> To: Tarun Alle - I68638 <Tarun.Alle@...rochip.com>
> Cc: Arun Ramadoss - I17769 <Arun.Ramadoss@...rochip.com>;
> UNGLinuxDriver <UNGLinuxDriver@...rochip.com>; andrew@...n.ch;
> hkallweit1@...il.com; davem@...emloft.net; edumazet@...gle.com;
> kuba@...nel.org; pabeni@...hat.com; netdev@...r.kernel.org; linux-
> kernel@...r.kernel.org
> Subject: Re: [PATCH net-next V4] net: phy: microchip_t1: SQI support for
> LAN887x
>
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the
> content is safe
>
> On Tue, Sep 17, 2024 at 05:26:57PM +0530, Tarun Alle wrote:
> > From: Tarun Alle <Tarun.Alle@...rochip.com>
> >
> > Add support for measuring Signal Quality Index for LAN887x T1 PHY.
> > Signal Quality Index (SQI) is measure of Link Channel Quality from
> > 0 to 7, with 7 as the best. By default, a link loss event shall
> > indicate an SQI of 0.
> >
> > Signed-off-by: Tarun Alle <Tarun.Alle@...rochip.com>
>
> Please note that the merge window is open, which means that net-next is
> currently closed. Thus, patches should be submitted as RFC.
>
> > ---
> > v3 -> v4
> > - Added check to handle invalid samples.
> > - Added macro for ARRAY_SIZE(rawtable).
> >
> > v2 -> v3
> > - Replaced hard-coded values with ARRAY_SIZE(rawtable).
> >
> > v1 -> v2
> > - Replaced hard-coded 200 with ARRAY_SIZE(rawtable).
>
> Hmm. We've been through several iterations trying to clean this up into
> something more easily readable, but I fear there'll be another iteration.
>
> Maybe the following would be nicer:
>
> enum {
> SQI_SAMPLES = 200,
> /* Look at samples of the middle 60% */
> SQI_INLIERS_NUM = SQI_SAMPLES * 60 / 100,
> SQI_INLIERS_START = (SQI_SAMPLES - SQI_INLIERS_NUM) / 2,
> SQI_INLIERS_END = SQI_INLIERS_START + SQI_INLIERS_NUM, };
>
> > +static int lan887x_get_sqi_100M(struct phy_device *phydev) {
> > + u16 rawtable[200];
>
> u16 rawtable[SQI_SAMPLES];
>
> > + u32 sqiavg = 0;
> > + u8 sqinum = 0;
> > + int rc;
>
> Since you use "i" multiple times, declare it at the beginning of the function
> rather than in each for loop.
>
> int i;
>
> > +
> > + /* Configuration of SQI 100M */
> > + rc = phy_write_mmd(phydev, MDIO_MMD_VEND1,
> > + LAN887X_COEFF_PWR_DN_CONFIG_100,
> > + LAN887X_COEFF_PWR_DN_CONFIG_100_V);
> > + if (rc < 0)
> > + return rc;
> > +
> > + rc = phy_write_mmd(phydev, MDIO_MMD_VEND1,
> LAN887X_SQI_CONFIG_100,
> > + LAN887X_SQI_CONFIG_100_V);
> > + if (rc < 0)
> > + return rc;
> > +
> > + rc = phy_read_mmd(phydev, MDIO_MMD_VEND1,
> LAN887X_SQI_CONFIG_100);
> > + if (rc != LAN887X_SQI_CONFIG_100_V)
> > + return -EINVAL;
> > +
> > + rc = phy_modify_mmd(phydev, MDIO_MMD_VEND1,
> LAN887X_POKE_PEEK_100,
> > + LAN887X_POKE_PEEK_100_EN,
> > + LAN887X_POKE_PEEK_100_EN);
> > + if (rc < 0)
> > + return rc;
> > +
> > + /* Required before reading register
> > + * otherwise it will return high value
> > + */
> > + msleep(50);
> > +
> > + /* Link check before raw readings */
> > + rc = genphy_c45_read_link(phydev);
> > + if (rc < 0)
> > + return rc;
> > +
> > + if (!phydev->link)
> > + return -ENETDOWN;
> > +
> > + /* Get 200 SQI raw readings */
> > + for (int i = 0; i < ARRAY_SIZE(rawtable); i++) {
>
> for (i = 0; i < SQI_SAMPLES; i++) {
>
> > + rc = phy_write_mmd(phydev, MDIO_MMD_VEND1,
> > + LAN887X_POKE_PEEK_100,
> > + LAN887X_POKE_PEEK_100_EN);
> > + if (rc < 0)
> > + return rc;
> > +
> > + rc = phy_read_mmd(phydev, MDIO_MMD_VEND1,
> > + LAN887X_SQI_MSE_100);
> > + if (rc < 0)
> > + return rc;
> > +
> > + rawtable[i] = (u16)rc;
> > + }
> > +
> > + /* Link check after raw readings */
> > + rc = genphy_c45_read_link(phydev);
> > + if (rc < 0)
> > + return rc;
> > +
> > + if (!phydev->link)
> > + return -ENETDOWN;
> > +
> > + /* Sort SQI raw readings in ascending order */
> > + sort(rawtable, ARRAY_SIZE(rawtable), sizeof(u16), data_compare,
> > + NULL);
>
> sort(rawtable, SQI_SAMPLES, sizeof(u16), data_compare, NULL);
>
> Although renaming data_compare to sqi_compare would be even more
> descriptive of what it's doing.
>
> > +
> > + /* Keep inliers and discard outliers */
> > + for (int i = SQI100M_SAMPLE_INIT(5, rawtable);
> > + i < SQI100M_SAMPLE_INIT(5, rawtable) * 4; i++)
>
> for (i = SQI_INLIERS_START; i < SQI_INLIERS_END; i++)
>
> > + sqiavg += rawtable[i];
> > +
> > + /* Handle invalid samples */
> > + if (sqiavg != 0) {
> > + /* Get SQI average */
> > + sqiavg /= SQI100M_SAMPLE_INIT(5, rawtable) * 4 -
> > + SQI100M_SAMPLE_INIT(5, rawtable);
>
> sqiavg /= SQI_INLIERS_NUM;
>
> Overall, I think this is better rather than the SQI100M_SAMPLE_INIT() macro...
> for which I'm not sure what the _INIT() bit actually means.
>
> I think my suggestion has the advantage that it makes it clear what these
> various calculations are doing, because the result of the calculations is
> described in the enum name.
>
> Thanks.
>
> --
> RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
> FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!
Thanks,
Tarun Alle.
Powered by blists - more mailing lists