netdev - Re: [PATCH v1 01/16] net-next/yunsilicon: Add xsc driver basic framework

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: <2792da0b-a1f8-4998-a7ea-f1978f97fc4a@lunn.ch>
Date: Wed, 18 Dec 2024 19:20:06 +0100
From: Andrew Lunn <andrew@...n.ch>
To: Xin Tian <tianx@...silicon.com>
Cc: netdev@...r.kernel.org, andrew+netdev@...n.ch, kuba@...nel.org,
	pabeni@...hat.com, edumazet@...gle.com, davem@...emloft.net,
	jeff.johnson@....qualcomm.com, przemyslaw.kitszel@...el.com,
	weihg@...silicon.com, wanry@...silicon.com
Subject: Re: [PATCH v1 01/16] net-next/yunsilicon: Add xsc driver basic
 framework

> +enum {
> +	XSC_LOG_LEVEL_DBG	= 0,
> +	XSC_LOG_LEVEL_INFO	= 1,
> +	XSC_LOG_LEVEL_WARN	= 2,
> +	XSC_LOG_LEVEL_ERR	= 3,
> +};
> +
> +#define xsc_dev_log(condition, level, dev, fmt, ...)			\
> +do {									\
> +	if (condition)							\
> +		dev_printk(level, dev, dev_fmt(fmt), ##__VA_ARGS__);	\
> +} while (0)
> +
> +#define xsc_core_dbg(__dev, format, ...)				\
> +	xsc_dev_log(xsc_log_level <= XSC_LOG_LEVEL_DBG, KERN_DEBUG,	\
> +		&(__dev)->pdev->dev, "%s:%d:(pid %d): " format,		\
> +		__func__, __LINE__, current->pid, ##__VA_ARGS__)
> +
> +#define xsc_core_dbg_once(__dev, format, ...)				\
> +	dev_dbg_once(&(__dev)->pdev->dev, "%s:%d:(pid %d): " format,	\
> +		     __func__, __LINE__, current->pid,			\
> +		     ##__VA_ARGS__)
> +
> +#define xsc_core_dbg_mask(__dev, mask, format, ...)			\
> +do {									\
> +	if ((mask) & xsc_debug_mask)					\
> +		xsc_core_dbg(__dev, format, ##__VA_ARGS__);		\
> +} while (0)

You where asked to throw all these away and just use the existing
methods. 

If you disagree with a comment, please reply and ask for more details,
understand the reason behind the comment, or maybe try to justify your
solution over what already exists.

Maybe look at the ethtool .get_msglevel & .set_msglevel if you are not
already using them.

> +unsigned int xsc_log_level = XSC_LOG_LEVEL_WARN;
> +module_param_named(log_level, xsc_log_level, uint, 0644);
> +MODULE_PARM_DESC(log_level,
> +		 "lowest log level to print: 0=debug, 1=info, 2=warning, 3=error. Default=1");

Module parameters are not liked. You will however find quite a few
drivers with something like:

MODULE_PARM_DESC(debug, "Debug level (0=none,...,16=all)");

which is used to set the initial msglevel. That will probably be
accepted.

> +EXPORT_SYMBOL(xsc_log_level);

I've not looked at your overall structure yet, but why export this?
Are there multiple modules involved?

	Andrew