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:

quoted

quoted

+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