netdev - Re: [PATCH net-next 4/7] net: phy: marvell: Add support for amplitude graph

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: <20200518044920.GK21714@lion.mk-sys.cz>
Date:   Mon, 18 May 2020 06:49:20 +0200
From:   Michal Kubecek <mkubecek@...e.cz>
To:     netdev@...r.kernel.org
Cc:     Andrew Lunn <andrew@...n.ch>, Chris Healy <cphealy@...il.com>,
        David Miller <davem@...emloft.net>,
        Florian Fainelli <f.fainelli@...il.com>,
        Heiner Kallweit <hkallweit1@...il.com>
Subject: Re: [PATCH net-next 4/7] net: phy: marvell: Add support for
 amplitude graph

On Sun, May 17, 2020 at 10:51:50PM +0200, Andrew Lunn wrote:
> > > +static int marvell_vct5_amplitude_distance(struct phy_device *phydev,
> > > +                                          int meters)
> > > +{
> > > +       int mV_pair0, mV_pair1, mV_pair2, mV_pair3;
> > > +       int distance;
> > > +       u16 reg;
> > > +       int err;
> > > +
> > > +       distance = meters * 1000 / 805;
> > 
> > With this integer based meters representation, it seems to me that we
> > are artificially reducing the resolution of the distance sampling.
> > For a 100 meter cable, the Marvell implementation looks to support 124
> > sample points.  This could result in incorrect data reporting as two
> > adjacent meter numbers would resolve to the same disatance value
> > entered into the register.  (eg - 2 meters = 2 distance  3 meters = 2
> > distance)
> > 
> > Is there a better way of doing this which would allow for userspace to
> > use the full resolution of the hardware?
> 
> Hi Chris
> 
> I don't see a simple solution to this.
> 
> PHYs/vendors seem to disagree about the ratio. Atheros use
> 824. Marvell use 805. I've no idea what Broadcom, aQuantia uses. We
> would need to limit the choice of step to multiples of whatever the
> vendor picks as its ratio. If the user picks a step of 2m, the driver
> needs to return an error and say sorry, please try 2.488 meter steps
> for Marvell, 2.427 meter steps on Atheros, and who knows what for
> Broadcom. And when the user requests data just for 1-25 meters, the
> driver needs to say sorry, try again with 0.824-24.62, or maybe
> 0.805-24.955. That is not a nice user experience.

How about this?

- user would use meters as unit on ethtool command line but non-integer
  values like "2.4" would be allowed
- request message would use e.g. cm (for consistency with existing cable
  test results)
- driver would round requested values to closest supported (you actually
  already round them)
- optionally, actual values used could be returned in request reply or
  start notification

Michal