netdev - Re: [PATCH net-next] dpaa2-eth: add ethtool MAC counters

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Thu, 7 Nov 2019 02:47:58 +0100
From:   Andrew Lunn <andrew@...n.ch>
To:     Ioana Ciornei <ioana.ciornei@....com>
Cc:     davem@...emloft.net, netdev@...r.kernel.org
Subject: Re: [PATCH net-next] dpaa2-eth: add ethtool MAC counters

On Thu, Nov 07, 2019 at 02:49:08AM +0200, Ioana Ciornei wrote:
> +void dpaa2_mac_get_ethtool_stats(struct dpaa2_mac *mac, u64 *data)
> +{
> +	struct fsl_mc_device *dpmac_dev = mac->mc_dev;
> +	int i, err;
> +	u64 value;
> +
> +	for (i = 0; i < DPAA2_MAC_NUM_STATS; i++) {
> +		err = dpmac_get_counter(mac->mc_io, 0, dpmac_dev->mc_handle,
> +					i, &value);
> +		if (err) {
> +			netdev_err(mac->net_dev,
> +				   "dpmac_get_counter error %d\n", err);
> +			return;

Hi Ioana

I've seen quite a few drivers set *data to U64_MAX when there is an
error. A value like that should stand out. The kernel message might
not be seen.

> +/**
> + * dpmac_get_counter() - Read a specific DPMAC counter
> + * @mc_io:	Pointer to opaque I/O object
> + * @cmd_flags:	Command flags; one or more of 'MC_CMD_FLAG_'
> + * @token:	Token of DPMAC object
> + * @id:		The requested counter ID
> + * @value:	Returned counter value
> + *
> + * Return:	The requested counter; '0' otherwise.
> + */
> +int dpmac_get_counter(struct fsl_mc_io *mc_io, u32 cmd_flags, u16 token,
> +		      enum dpmac_counter_id id, u64 *value)
> +{
> +	struct dpmac_cmd_get_counter *dpmac_cmd;
> +	struct dpmac_rsp_get_counter *dpmac_rsp;
> +	struct fsl_mc_command cmd = { 0 };
> +	int err = 0;
> +
> +	cmd.header = mc_encode_cmd_header(DPMAC_CMDID_GET_COUNTER,
> +					  cmd_flags,
> +					  token);
> +	dpmac_cmd = (struct dpmac_cmd_get_counter *)cmd.params;
> +	dpmac_cmd->id = id;
> +
> +	err = mc_send_command(mc_io, &cmd);
> +	if (err)
> +		return err;
> +
> +	dpmac_rsp = (struct dpmac_rsp_get_counter *)cmd.params;
> +	*value = le64_to_cpu(dpmac_rsp->counter);
> +
> +	return 0;
> +}

How expensive is getting a single value? Is there a way to just get
them all in a single command? The ethtool API always returns them all,
so maybe you can optimise the firmware API to do the same?

   Andrew