netdev - Re: [RFC] net: dsa: mv88e6xxx: ptp: improve phc2sys precision for mv88e6xxx switch in combination with imx6-fec

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, 5 Aug 2019 12:36:52 +0200
From:   Miroslav Lichvar <mlichvar@...hat.com>
To:     Vladimir Oltean <olteanv@...il.com>
Cc:     Hubert Feurstein <h.feurstein@...il.com>,
        Richard Cochran <richardcochran@...il.com>,
        Andrew Lunn <andrew@...n.ch>, netdev <netdev@...r.kernel.org>,
        lkml <linux-kernel@...r.kernel.org>,
        Vivien Didelot <vivien.didelot@...il.com>,
        Florian Fainelli <f.fainelli@...il.com>,
        "David S. Miller" <davem@...emloft.net>
Subject: Re: [RFC] net: dsa: mv88e6xxx: ptp: improve phc2sys precision for
 mv88e6xxx switch in combination with imx6-fec

On Mon, Aug 05, 2019 at 12:54:49PM +0300, Vladimir Oltean wrote:
> - Along the lines of the above, I wonder how badly would the
> maintainers shout at the proposal of adding a struct
> ptp_system_timestamp pointer and its associated lock in struct device.
> That way at least you don't have to change the API, like you did with
> mdiobus_write_nested_ptp. Relatively speaking, this is the least
> amount of intrusion (although, again, far from beautiful).

That would make sense to me, if there are other drivers that could use
it.

> I also added Miroslav Lichvar, who originally created the
> PTP_SYS_OFFSET_EXTENDED ioctl, maybe he can share some ideas on how it
> is best served.

The idea behind that ioctl was to allow drivers to take the system
timestamps as close to the reading of the HW clock as possible,
excluding delays (and jitter) due to other operations that are
necessary to get that timestamp. In the ethernet drivers that was a
single PCI read. If in this case it's a "write" operation that
triggers the reading of the HW clock, then I think the patch is
using is the ptp_read_system_*ts() calls correctly.

> > --- a/drivers/net/ethernet/freescale/fec_main.c
> > +++ b/drivers/net/ethernet/freescale/fec_main.c
> > @@ -1814,11 +1814,25 @@ static int fec_enet_mdio_write(struct mii_bus *bus, int mii_id, int regnum,
> >
> >         reinit_completion(&fep->mdio_done);
> >
> > -       /* start a write op */
> > -       writel(FEC_MMFR_ST | FEC_MMFR_OP_WRITE |
> > -               FEC_MMFR_PA(mii_id) | FEC_MMFR_RA(regnum) |
> > -               FEC_MMFR_TA | FEC_MMFR_DATA(value),
> > -               fep->hwp + FEC_MII_DATA);
> > +       if (bus->ptp_sts) {
> > +               unsigned long flags = 0;
> > +               local_irq_save(flags);
> > +               __iowmb();
> > +               /* >Take the timestamp *after* the memory barrier */
> > +               ptp_read_system_prets(bus->ptp_sts);
> > +               writel_relaxed(FEC_MMFR_ST | FEC_MMFR_OP_WRITE |
> > +                       FEC_MMFR_PA(mii_id) | FEC_MMFR_RA(regnum) |
> > +                       FEC_MMFR_TA | FEC_MMFR_DATA(value),
> > +                       fep->hwp + FEC_MII_DATA);
> > +               ptp_read_system_postts(bus->ptp_sts);
> > +               local_irq_restore(flags);
> > +       } else {
> > +               /* start a write op */
> > +               writel(FEC_MMFR_ST | FEC_MMFR_OP_WRITE |
> > +                       FEC_MMFR_PA(mii_id) | FEC_MMFR_RA(regnum) |
> > +                       FEC_MMFR_TA | FEC_MMFR_DATA(value),
> > +                       fep->hwp + FEC_MII_DATA);
> > +       }

-- 
Miroslav Lichvar