Re: [PATCH net-next v5 2/2] net: phy: broadcom: Add PTP support for some Broadcom PHYs.

[Jonathan Lemon <jonathan.lemon@gmail.com>]

On Sun, May 22, 2022 at 07:49:20PM -0700, Florian Fainelli wrote:
> 
> 
> On 5/20/2022 7:04 PM, Jonathan Lemon wrote:
> > On Fri, May 20, 2022 at 10:24:25AM -0700, Florian Fainelli wrote:
> > > 
> > > 
> > > On 5/18/2022 3:39 PM, Jonathan Lemon wrote:
> > > > This adds PTP support for BCM54210E Broadcom PHYs, in particular,
> > > > the BCM54213PE, as used in the Rasperry PI CM4.  It has only been
> > > > tested on that hardware.
> > > > 
> > > > Signed-off-by: Jonathan Lemon <jonathan.lemon@gmail.com>
> > > > ---
> > > [snip]
> > > 
> > > Looks good to me, just one question below:
> > > 
> > > > +static void bcm_ptp_init(struct bcm_ptp_private *priv)
> > > > +{
> > > > +	priv->nse_ctrl = NSE_GMODE_EN;
> > > > +
> > > > +	mutex_init(&priv->mutex);
> > > > +	skb_queue_head_init(&priv->tx_queue);
> > > > +
> > > > +	priv->mii_ts.rxtstamp = bcm_ptp_rxtstamp;
> > > > +	priv->mii_ts.txtstamp = bcm_ptp_txtstamp;
> > > > +	priv->mii_ts.hwtstamp = bcm_ptp_hwtstamp;
> > > > +	priv->mii_ts.ts_info = bcm_ptp_ts_info;
> > > > +
> > > > +	priv->phydev->mii_ts = &priv->mii_ts;
> > > > +
> > > > +	INIT_DELAYED_WORK(&priv->out_work, bcm_ptp_fsync_work);
> > > 
> > > Do we need to make sure that we cancel the workqueue in an bcm_ptp_exit()
> > > function?
> > > 
> > > I would imagine that the Ethernet MAC attached to that PHY device having
> > > stopped its receiver and transmitter should ensure no more packets coming in
> > > or out, however since this is a delayed/asynchronous work, do not we need to
> > > protect against use after free?
> > 
> > The workqueue is just mamually creatimg a 1PPS pulse on the SYNC_OUT
> > pin, no packet activity.  Arguably, the .suspend hook could stop all work,
> > but that seems out of scope here? (and this phy does not suspend/resume)
> 
> The BCM54210E entry does have a suspend/resume entry so it seems to me that
> we do need to cancel the workqueue as the PHY library will not do that on
> our behalf. What I imagine could happen is that this workqueue generates
> spurious MDIO accesses *after* both the PHY and the bus have been suspended
> (and their driver's clock possibly gated already).

Yes, you're right.  I was looking at the rpi-5.15.y tree, which doesn't
have these hooks yet.  I'll add a call to stop the workqueue.

Actually, in the next series, I'll break out the extts/perout into
separate patch.

Thanks for pointing this out!
-- 
Jonathan