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: <e219e106-72ef-4045-9fc0-db2639720aa6@yunsilicon.com>
Date: Mon, 23 Dec 2024 11:20:20 +0800
From: "tianx" <tianx@...silicon.com>
To: "Andrew Lunn" <andrew@...n.ch>
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

On 2024/12/19 2:20, Andrew Lunn wrote:
>> +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.

Apologies for the delayed reply. Thank you for the feedback.

Our driver suite consists of three modules: xsc_pci (which manages 
hardware resources and provides common services for the other two 
modules), xsc_eth (providing Ethernet functionality), and xsc_ib 
(offering RDMA functionality). The patch set we are submitting currently 
includes xsc_pci and xsc_eth.

To ensure consistent and fine-grained log control for all modules, we 
have wrapped these logging functions for ease of use. The use of these 
interfaces is strictly limited to our driver and does not impact other 
parts of the kernel. I believe this can be considered a small feature 
within our code. I’ve also observed similar implementations in other 
drivers, such as in |drivers/net/ethernet/chelsio/common.h| and 
|drivers/net/ethernet/adaptec/starfire.c|.

The |get_msglevel| and |set_msglevel| can only be used in the Ethernet 
driver, whereas we need to define a shared |log_level| in the PCI driver.

Please let me know the main concerns of the community, and we will be 
happy to make any necessary adjustments.

>> +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.
got it.
>> +EXPORT_SYMBOL(xsc_log_level);
> I've not looked at your overall structure yet, but why export this?
> Are there multiple modules involved?
The two modules we are currently submitting (xsc_pci and xsc_eth) both 
need access to |xsc_log_level|, so it is being exported.
>
> 	Andrew

Thank you, Andrew. Looking forward to your reply.

Best regards,

Xin Tian

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ