Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Wed, 7 Jun 2023 22:51:52 -0700
From:   Richard Cochran <richardcochran@gmail.com>
To:     Horatiu Vultur <horatiu.vultur@microchip.com>
Cc:     netdev@vger.kernel.org, linux-kernel@vger.kernel.org,
        andrew@lunn.ch, hkallweit1@gmail.com, linux@armlinux.org.uk,
        davem@davemloft.net, edumazet@google.com, kuba@kernel.org,
        pabeni@redhat.com
Subject: Re: [PATCH net-next] net: micrel: Change to receive timestamp in the
 frame for lan8841
Message-ID: <ZIFseH84Cv1KSOtj@hoboy.vegasvil.org>
References: <20230607070948.1746768-1-horatiu.vultur@microchip.com>
 <ZIDCpPbCFCxKBV2k@hoboy.vegasvil.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <ZIDCpPbCFCxKBV2k@hoboy.vegasvil.org>
Precedence: bulk

On Wed, Jun 07, 2023 at 10:47:16AM -0700, Richard Cochran wrote:
> On Wed, Jun 07, 2023 at 09:09:48AM +0200, Horatiu Vultur wrote:
> > +static long lan8841_ptp_do_aux_work(struct ptp_clock_info *ptp)
> > +{
> > +	struct kszphy_ptp_priv *ptp_priv = container_of(ptp, struct kszphy_ptp_priv,
> > +							ptp_clock_info);
> > +	struct skb_shared_hwtstamps *shhwtstamps;
> > +	struct timespec64 ts;
> > +	struct sk_buff *skb;
> > +	u32 ts_header;
> > +
> > +	while ((skb = skb_dequeue(&ptp_priv->rx_queue)) != NULL) {
> > +		lan8841_ptp_getseconds(ptp, &ts);
> 
> No need to call this once per frame.  It would be sufficent to call it
> once every 2 seconds and cache the result.

Okay, this is tricky.

- If you call lan8841_ptp_getseconds() after gathering the received
  frames, then the frame timestamps are clearly in the past WRT the
  call to getseconds.  That makes the wrap check simpler.  But the
  getseconds call really should be placed before the 'while' loop.

- If the Rx frame rate exceeds 1/second, then it would be more
  efficient to call getseconds every second, and cache the result.
  But then the wrap around check needs to account for the fact that
  the cached value may have occurred either before or after the frame
  timestamp.

I'll explain that second point when my brain wakes again...

Thanks,
Richard