> > 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
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
