netdev - Re: [PATCH net-next 2/3] dt-bindings: net: micrel: Configure latency values and timestamping check for LAN8814 phy

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]

Date:   Mon, 7 Mar 2022 14:08:42 +0100
From:   Andrew Lunn <andrew@...n.ch>
To:     Divya.Koppera@...rochip.com
Cc:     netdev@...r.kernel.org, hkallweit1@...il.com,
        linux@...linux.org.uk, davem@...emloft.net, kuba@...nel.org,
        robh+dt@...nel.org, devicetree@...r.kernel.org,
        richardcochran@...il.com, linux-kernel@...r.kernel.org,
        UNGLinuxDriver@...rochip.com, Madhuri.Sripada@...rochip.com,
        Manohar.Puri@...rochip.com
Subject: Re: [PATCH net-next 2/3] dt-bindings: net: micrel: Configure latency
 values and timestamping check for LAN8814 phy

> > > +
> > > + - lan8814,ignore-ts: If present the PHY will not support timestamping.
> > > +
> > > +     This option acts as check whether Timestamping is supported by
> > > +     hardware or not. LAN8814 phy support hardware tmestamping.
> > 
> > Does this mean the hardware itself cannot tell you it is missing the needed
> > hardware? What happens when you forget to add this flag? Does the driver
> > timeout waiting for hardware which does not exists?
> > 
> 
> If forgot to add this flag, driver will try to register ptp_clock that needs
> access to clock related registers, which in turn fails if those registers doesn't exists.

Thanks for the reply, but you did not answer my question:

  Does this mean the hardware itself cannot tell you it is missing the
  needed hardware?

Don't you have different IDs in register 2 and 3 for those devices
with clock register and those without?

> > > + - lan8814,latency_rx_10: Configures Latency value of phy in ingress at 10
> > Mbps.
> > > +
> > > + - lan8814,latency_tx_10: Configures Latency value of phy in egress at 10
> > Mbps.
> > > +
> > > + - lan8814,latency_rx_100: Configures Latency value of phy in ingress at 100
> > Mbps.
> > > +
> > > + - lan8814,latency_tx_100: Configures Latency value of phy in egress at 100
> > Mbps.
> > > +
> > > + - lan8814,latency_rx_1000: Configures Latency value of phy in ingress at
> > 1000 Mbps.
> > > +
> > > + - lan8814,latency_tx_1000: Configures Latency value of phy in egress at
> > 1000 Mbps.
> > 
> > Why does this need to be configured, rather than hard coded? Why would the
> > latency for a given speed change? I would of thought though you would take
> > the average length of a PTP packet and divide is by the link speed.
> > 
> 
> This latency values could be different for different phy's. So hardcoding will not work here.

But you do actually have hard coded defaults. Those odd hex values i
pointed out.

By different PHYs do you mean different PHY versions? So you can look
at register 2 and 3, determine what PHY it is, and so from that what
defaults should be used? Or do you mean different boards with the same
PHY?

In general, the less tunables you have, the better. If the driver can
figure it out, it is better to not have DT properties. The PHY will
then also work with ACPI and USB etc, where there is no
DT. Implementing the user space API Richard pointed out will also
allow your PHY to work with none DP systems.

> Yes in our case latency values depends on port speed. It is delay between network medium and 
> PTP timestamp point.

What are the units. You generally have the units in the property
name. So e.g. lan8814,latency_tx_1000_ns. If need be, the driver then
converts to whatever value you place into the register.

If you do keep them, please make it clear that these values are
optional, and state what value will be used when the property is not
present.

	Andrew