Re: [PATCH net-next RFC V1 5/5] net: mdio: Add a driver for InES time stamping IP core.

Andrew Lunn
2018-03-25T23:02:02Z

> > You can then clean up the code in timestamping.c. Code like:
> >
> > phydev = skb->dev->phydev;
> > if (likely(phydev->drv->txtstamp)) {
> > clone = skb_clone_sk(skb);
> > if (!clone)
> > return;
> > phydev->drv->txtstamp(phydev, clone, type);
> > }
> >
> > violates the layering, and the locking looks broken.
>
> Are you sure the locking is broken? If so, how?

As a general rule of thumb, locking is performed in the core when
possible. Experience has shown that device driver writers get locking
wrong more often than core code writers. So doing it once in the core
results in less bugs.

The phylib core code will take the phydev lock before calling into the
driver. By violating the layering, we are missing on this lock.

Maybe the one driver which currently implements these calls does not
need locking. But after the Marvell DSA work, we have most of the code
needed to implement support for the Marvell PHY PTP. It has pretty
similar registers. That would need the phydev lock to be held, because
we need to swap to different pages, so any other calls happening in
parallel are going to see registers from the wrong page. I don't want
to have to put these locks in the driver, that leads to bugs. The core
should do it.

Andrew