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

On 2024/12/19 2:20, Andrew Lunn wrote:

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

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.

quoted

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

quoted

+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