Netdev Archive on lore.kernel.org
help / color / mirror / Atom feed
From: Jonathan Lemon <jonathan.lemon@gmail.com>
To: Jakub Kicinski <kuba@kernel.org>
Cc: davem@davemloft.net, richardcochran@gmail.com,
	kernel-team@fb.com, netdev@vger.kernel.org
Subject: Re: [PATCH net-next v4] ptp: ocp: Expose various resources on the timecard.
Date: Wed, 4 Aug 2021 16:52:23 -0700	[thread overview]
Message-ID: <20210804235223.rkyxuvdeowcf7wgl@bsd-mbp.dhcp.thefacebook.com> (raw)
In-Reply-To: <20210804140957.1fd894dc@kicinski-fedora-pc1c0hjn.dhcp.thefacebook.com>

On Wed, Aug 04, 2021 at 02:09:57PM -0700, Jakub Kicinski wrote:
> On Tue,  3 Aug 2021 20:33:27 -0700 Jonathan Lemon wrote:
> > +static const struct devlink_param ptp_ocp_devlink_params[] = {
> > +};
> > +
> > +static void
> > +ptp_ocp_devlink_set_params_init_values(struct devlink *devlink)
> > +{
> > +}
> 
> Why register empty set of params?

I had this filled out at some point, but they got removed.


> > +static int
> > +ptp_ocp_devlink_register(struct devlink *devlink, struct device *dev)
> > +{
> > +	int err;
> > +
> > +	err = devlink_register(devlink, dev);
> > +	if (err)
> > +		return err;
> > +
> > +	err = devlink_params_register(devlink, ptp_ocp_devlink_params,
> > +				      ARRAY_SIZE(ptp_ocp_devlink_params));
> > +	ptp_ocp_devlink_set_params_init_values(devlink);
> > +	if (err)
> > +		goto out;
> > +	devlink_params_publish(devlink);
> > +
> > +	return 0;
> > +
> > +out:
> > +	devlink_unregister(devlink);
> > +	return err;
> > +}
> 
> > +static int
> > +ptp_ocp_devlink_info_get(struct devlink *devlink, struct devlink_info_req *req,
> > +			 struct netlink_ext_ack *extack)
> > +{
> > +	struct ptp_ocp *bp = devlink_priv(devlink);
> > +	char buf[32];
> > +	int err;
> > +
> > +	err = devlink_info_driver_name_put(req, KBUILD_MODNAME);
> > +	if (err)
> > +		return err;
> > +
> > +	if (bp->pending_image) {
> > +		err = devlink_info_version_stored_put(req,
> > +						      "timecard", "pending");
> 
> "pending" is not a version. It seems you're talking to the flash
> directly, why not read the version?

We're not talking to the flash yet.  We're writing a new image, but don't
know the image version, since it's not accessible from the FPGA blob.  So
since we're don't know what the stored image is until we reboot, I've set
it to 'pending' here - aka "pending reboot".  Could also be "unknown".
 

> > +	}
> > +
> > +	if (bp->image) {
> > +		u32 ver = ioread32(&bp->image->version);
> > +
> > +		if (ver & 0xffff) {
> > +			sprintf(buf, "%d", ver);
> > +			err = devlink_info_version_running_put(req,
> > +							       "timecard",
> > +							       buf);
> > +		} else {
> > +			sprintf(buf, "%d", ver >> 16);
> > +			err = devlink_info_version_running_put(req,
> > +							       "golden flash",
> > +							       buf);
> 
> What's the difference between "timecard" and "golden flash"?

There are two images stored in flash: "golden image", an image that
just provides flash functionality, and the actual featured FPGA image.


> Why call firmware for a timecard timecard? We don't call NIC
> firmware "NIC".

I didn't see a standard string to use.  I can call it 'fw.version', just
needed to differentiate it between the 'golden flash' loader and actual
firmware.



> Drivers using devlink should document what they implement and meaning
> of various fields. Please see examples in
> Documentation/networking/devlink/

I'll add things there.


> > +static void
> > +ptp_ocp_devlink_health_register(struct devlink *devlink)
> > +{
> > +	struct ptp_ocp *bp = devlink_priv(devlink);
> > +	struct devlink_health_reporter *r;
> > +
> > +	r = devlink_health_reporter_create(devlink, &ptp_ocp_health_ops, 0, bp);
> > +	if (IS_ERR(r))
> > +		dev_err(&bp->pdev->dev, "Failed to create reporter, err %ld\n",
> > +			PTR_ERR(r));
> > +	bp->health = r;
> > +}
> 
> What made you use devlink health here? Why not just print that "No GPS
> signal" message to the logs? Devlink health is supposed to give us
> meaningful context dumps and remediation, here neither is used.

The initial idea was to use 'devlink monitor' report the immediate
failure of the GNSS signal (rather than going through the kernel logs)
The 'devlink health' also keeps a count of how often the GPS signal
is lost.

Our application guys decided to use a different monitoring method,
so I can rip this out if objectionable. 
-- 
Jonathan

  reply	other threads:[~2021-08-04 23:52 UTC|newest]

Thread overview: 7+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-08-04  3:33 Jonathan Lemon
2021-08-04 14:16 ` Richard Cochran
2021-08-04 21:09 ` Jakub Kicinski
2021-08-04 23:52   ` Jonathan Lemon [this message]
2021-08-05 13:03     ` Jakub Kicinski
2021-08-05 17:26       ` Jonathan Lemon
2021-08-05 18:55         ` Jakub Kicinski

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20210804235223.rkyxuvdeowcf7wgl@bsd-mbp.dhcp.thefacebook.com \
    --to=jonathan.lemon@gmail.com \
    --cc=davem@davemloft.net \
    --cc=kernel-team@fb.com \
    --cc=kuba@kernel.org \
    --cc=netdev@vger.kernel.org \
    --cc=richardcochran@gmail.com \
    --subject='Re: [PATCH net-next v4] ptp: ocp: Expose various resources on the timecard.' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).