Netdev Archive on lore.kernel.org
help / color / mirror / Atom feed
From: Jakub Kicinski <kuba@kernel.org>
To: Jonathan Lemon <jonathan.lemon@gmail.com>
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: Thu, 5 Aug 2021 06:03:26 -0700	[thread overview]
Message-ID: <20210805060326.4c5fbef9@kicinski-fedora-pc1c0hjn.dhcp.thefacebook.com> (raw)
In-Reply-To: <20210804235223.rkyxuvdeowcf7wgl@bsd-mbp.dhcp.thefacebook.com>

On Wed, 4 Aug 2021 16:52:23 -0700 Jonathan Lemon wrote:
> > > +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".

Having the driver remember that the device was flashed is not a solid
indication that the image is actually different. It may be that user
flashed the same version, driver may get reloaded and lose the
indication.. Let's not make a precedent for (ab) use of the version
field to indicate reset required.

> > > +	}
> > > +
> > > +	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.

Is the 'golden flash' a backup in case full featured image does not
work or 'first stage' image/'loader'? IIUC it's the latter so maybe
we can just use "loader"? "fw" for the actual image would be better 
than "fw.version" the entire string is a version after all.

> > > +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. 

Great, thanks!

  reply	other threads:[~2021-08-05 13:03 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
2021-08-05 13:03     ` Jakub Kicinski [this message]
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=20210805060326.4c5fbef9@kicinski-fedora-pc1c0hjn.dhcp.thefacebook.com \
    --to=kuba@kernel.org \
    --cc=davem@davemloft.net \
    --cc=jonathan.lemon@gmail.com \
    --cc=kernel-team@fb.com \
    --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).