Date:   Wed, 8 Dec 2021 09:21:36 +0800
From:   Kent Gibson <warthog618@gmail.com>
To:     Dipen Patel <dipenp@nvidia.com>
Cc:     thierry.reding@gmail.com, jonathanh@nvidia.com,
        linux-kernel@vger.kernel.org, linux-tegra@vger.kernel.org,
        linux-gpio@vger.kernel.org, linus.walleij@linaro.org,
        brgl@bgdev.pl, devicetree@vger.kernel.org,
        linux-doc@vger.kernel.org, robh+dt@kernel.org
Subject: Re: [RFC v3 02/12] drivers: Add hardware timestamp engine (HTE)
Message-ID: <20211208012136.GA18163@sol>
References: <20211123193039.25154-1-dipenp@nvidia.com>
 <20211123193039.25154-3-dipenp@nvidia.com>
 <20211126013041.GA10380@sol>
 <246d1ff4-ec51-b4bf-a664-4559c45021fb@nvidia.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <246d1ff4-ec51-b4bf-a664-4559c45021fb@nvidia.com>
Precedence: bulk

On Tue, Dec 07, 2021 at 04:36:35PM -0800, Dipen Patel wrote:
> Hi,
> 

[snip]

> >> +/**
> >> + * enum hte_return- HTE subsystem return values used during callback.
> >> + *
> >> + * @HTE_CB_HANDLED: The consumer handled the data successfully.
> >> + * @HTE_RUN_THREADED_CB: The consumer needs further processing, in that case HTE
> >> + * subsystem will invoke kernel thread and call secondary callback provided by
> >> + * the consumer during devm_of_hte_request_ts and hte_req_ts_by_dt_node call.
> >> + * @HTE_CB_TS_DROPPED: The client returns when it can not store ts data.
> >> + * @HTE_CB_ERROR: The client returns error if anything goes wrong.
> >> + */
> >> +enum hte_return {
> >> +	HTE_CB_HANDLED,
> >> +	HTE_RUN_THREADED_CB,
> >> +	HTE_CB_TS_DROPPED,
> >> +	HTE_CB_ERROR,
> >> +};
> >> +typedef enum hte_return hte_return_t;
> >> +
> > Wrt HTE_CB_TS_DROPPED, why is the client dropping data any of hte's
> > business?  It is also confusing in that I would expect the dropped_ts
> > gauge, that you increment when this code is returned, to indicate the
> > events dropped by the hardware, not the client.  But then you have no
> > indication of events dropped by hardware at all, though you could
> > determine that from gaps in the sequence numbers.
> > Anyway, the client can do the math in both cases if they care to, so not
> > sure what its purpose is here.
> 
> It is used for statistical purpose and hte being subsytem it can provide
> 
> standard interface in debugfs (so that clients do not have to) to anyone interested.
> 
> The dropped_ts could represent total dropped ts by both hardware and
> 
> client. I can add debugfs interface to break it down further if it helps in statistics.
> 

Updating stats is not what the return code here is for.

And what if the client discards the event AFTER returning from the
handler, say in the threaded cb?

If you want stats fedback then provide a function for the client to call
to update stats, rather than piggy-backing it on the callback return.
I'm unconvinced that stats are a worthwhile addition, and you certainly
don't need to bake it into your core api.

Cheers,
Kent.