netdev - Re: [PATCH net-next 09/11] ptp: ocp: Add debugfs entry for timecard

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]

Message-ID: <20210901170641.0e9c9481@kicinski-fedora-pc1c0hjn.dhcp.thefacebook.com>
Date:   Wed, 1 Sep 2021 17:06:41 -0700
From:   Jakub Kicinski <kuba@...nel.org>
To:     Jonathan Lemon <jonathan.lemon@...il.com>
Cc:     davem@...emloft.net, richardcochran@...il.com,
        netdev@...r.kernel.org, kernel-team@...com, abyagowi@...com
Subject: Re: [PATCH net-next 09/11] ptp: ocp: Add debugfs entry for timecard

On Mon, 30 Aug 2021 16:52:34 -0700 Jonathan Lemon wrote:
> Provide a view into the timecard internals for debugging.
> 
> Signed-off-by: Jonathan Lemon <jonathan.lemon@...il.com>

> +#ifdef CONFIG_DEBUG_FS
> +#define gpio_map(gpio, bit, pri, sec, def) ({			\
> +	char *_ans;						\
> +	if (gpio & (1 << bit))					\
> +		_ans = pri;					\
> +	else if (gpio & (1 << (bit + 16)))			\
> +		_ans = sec;					\
> +	else							\
> +		_ans = def;					\
> +	_ans;							\
> +})
> +
> +#define gpio_multi_map(buf, gpio, bit, pri, sec, def) ({	\
> +		char *_ans;					\
> +		_ans = buf;					\
> +		strcpy(buf, def);				\
> +		if (gpio & (1 << (bit + 16)))			\
> +			_ans += sprintf(_ans, "%s ", pri);	\
> +		if (gpio & (1 << bit))				\
> +			_ans += sprintf(_ans, "%s ", sec);	\
> +})

These can't be static inlines?

> +static int
> +ptp_ocp_summary_show(struct seq_file *s, void *data)
> +{
> +	struct device *dev = s->private;
> +	struct ts_reg __iomem *ts_reg;
> +	u32 sma_in, sma_out, val;
> +	struct timespec64 ts;
> +	struct ptp_ocp *bp;
> +	char *buf, *src;
> +	bool on;
> +
> +	buf = (char *)__get_free_page(GFP_KERNEL);
> +	if (!buf)
> +		return -ENOMEM;
> +
> +	sma1_out_show(dev, NULL, buf);
> +	seq_printf(s, "   sma1: out from %s", buf);
> +
> +	sma2_out_show(dev, NULL, buf);
> +	seq_printf(s, "   sma2: out from %s", buf);
> +
> +	sma3_in_show(dev, NULL, buf);
> +	seq_printf(s, "   sma3: input to %s", buf);
> +
> +	sma4_in_show(dev, NULL, buf);
> +	seq_printf(s, "   sma4: input to %s", buf);

Why duplicate the data already available via sysfs?

> +static int
> +ptp_ocp_debugfs_add_device(struct ptp_ocp *bp)
> +{
> +	struct dentry *d;
> +
> +	d = debugfs_create_dir(dev_name(&bp->dev), ptp_ocp_debugfs_root);
> +	if (IS_ERR(d))
> +		return PTR_ERR(d);

Driver's are not supposed to depend on debugfs, you should be able to
carry on and all debugfs functions you pass an error pointer as a
parent will just return the same error right back.

> +	bp->debug_root = d;
> +
> +	d = debugfs_create_file("summary", 0444, bp->debug_root,
> +				&bp->dev, &ptp_ocp_summary_fops);
> +	if (IS_ERR(d))
> +		goto fail;
> +
> +	return 0;
> +
> +fail:
> +	debugfs_remove_recursive(bp->debug_root);
> +	bp->debug_root = NULL;
> +
> +	return PTR_ERR(d);
> +}
> +
> +static void
> +ptp_ocp_debugfs_remove_device(struct ptp_ocp *bp)
> +{
> +	debugfs_remove_recursive(bp->debug_root);
> +}
> +
> +static int
> +ptp_ocp_debugfs_init(void)
> +{
> +	struct dentry *d;
> +
> +	d = debugfs_create_dir("timecard", NULL);
> +	if (IS_ERR(d))
> +		return PTR_ERR(d);
> +
> +	ptp_ocp_debugfs_root = d;
> +
> +	return 0;
> +}
> +
> +static void
> +ptp_ocp_debugfs_fini(void)
> +{
> +	debugfs_remove_recursive(ptp_ocp_debugfs_root);
> +}
> +#else
> +#define ptp_ocp_debugfs_init()			0
> +#define ptp_ocp_debugfs_fini()
> +#define ptp_ocp_debugfs_add_device(bp)		0
> +#define ptp_ocp_debugfs_remove_device(bp)
> +#endif

This should not be necessary. Compiler should remove all those
functions as dead code when debugfs is not compiled in.

>  static void
>  ptp_ocp_dev_release(struct device *dev)
>  {