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] [day] [month] [year] [list]

Message-ID: <94a63b04-a3ca-48c5-bae9-bf01032fb260@lunn.ch>
Date: Tue, 24 Dec 2024 03:27:39 +0100
From: Andrew Lunn <andrew@...n.ch>
To: tianx <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

On Mon, Dec 23, 2024 at 11:20:20AM +0800, tianx wrote:
> 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|.
 
Did you look at the age of these drivers? starfire has been around
from before git was adopted, 2005. Chelsio is of a similar age. You
cannot expect anything so old to be a good reference of today's best
practices.

Please just use netdev_dbg(), netdev_info(), netdev_warn(),
netdev_err() etc for the ethernet driver, combined with msglevel. It
is an ethernet driver, so the usual ethernet driver APIs should be
used.

For the PCI driver, pci_dbg(), pci_info(), pci_notices() etc.

I don't know rdma, is there rdma_dbg(), rdma_info() etc.

	Andrew