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 for Android: free password hash cracker in your pocket

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

Message-ID: <20210902170034.424a5r5vmrdevoo7@bsd-mbp.dhcp.thefacebook.com>
Date:   Thu, 2 Sep 2021 10:00:34 -0700
From:   Jonathan Lemon <jonathan.lemon@...il.com>
To:     Jakub Kicinski <kuba@...nel.org>
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 Wed, Sep 01, 2021 at 05:06:41PM -0700, Jakub Kicinski wrote:
> 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?

Fixed - old habit of writing macros.


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

It is convenient to have all the information on 
one page when trying to understand where the signals
are being steered.


[ snip ]

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

Ack.

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

Removed #ifdef - now the _summary function is left in 
and the compiler removes it in the dead code pass.
-- 
Jonathan