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: Wed, 4 Aug 2021 14:09:57 -0700	[thread overview]
Message-ID: <20210804140957.1fd894dc@kicinski-fedora-pc1c0hjn.dhcp.thefacebook.com> (raw)
In-Reply-To: <20210804033327.345759-1-jonathan.lemon@gmail.com>

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?

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

> +	}
> +
> +	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"?
Why call firmware for a timecard timecard? We don't call NIC
firmware "NIC".

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

> +		}
> +		if (err)
> +			return err;
> +	}

> +static int
> +ptp_ocp_health_diagnose(struct devlink_health_reporter *reporter,
> +			struct devlink_fmsg *fmsg,
> +			struct netlink_ext_ack *extack)
> +{
> +	struct ptp_ocp *bp = devlink_health_reporter_priv(reporter);
> +	char buf[32];
> +	int err;
> +
> +	if (!bp->gps_lost)
> +		return 0;
> +
> +	sprintf(buf, "%ptT", &bp->gps_lost);
> +	err = devlink_fmsg_string_pair_put(fmsg, "Lost sync at", buf);
> +	if (err)
> +		return err;
> +
> +	return 0;
> +}
> +
> +static void
> +ptp_ocp_health_update(struct ptp_ocp *bp)
> +{
> +	int state;
> +
> +	state = bp->gps_lost ? DEVLINK_HEALTH_REPORTER_STATE_ERROR
> +			     : DEVLINK_HEALTH_REPORTER_STATE_HEALTHY;
> +
> +	if (bp->gps_lost)
> +		devlink_health_report(bp->health, "No GPS signal", NULL);
> +
> +	devlink_health_reporter_state_update(bp->health, state);
> +}
> +
> +static const struct devlink_health_reporter_ops ptp_ocp_health_ops = {
> +	.name = "gps_sync",
> +	.diagnose = ptp_ocp_health_diagnose,
> +};
> +
> +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.

  parent reply	other threads:[~2021-08-04 21:10 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 [this message]
2021-08-04 23:52   ` Jonathan Lemon
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=20210804140957.1fd894dc@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).