linux-kernel - Re: [PATCH net-next 4/7] net: hibmcge: Add register dump supported in this module

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: <e1827d48-7aeb-4f40-9332-8ce1efc5c960@lunn.ch>
Date: Thu, 24 Oct 2024 14:22:13 +0200
From: Andrew Lunn <andrew@...n.ch>
To: Jijie Shao <shaojijie@...wei.com>
Cc: davem@...emloft.net, edumazet@...gle.com, kuba@...nel.org,
	pabeni@...hat.com, andrew+netdev@...n.ch, horms@...nel.org,
	shenjian15@...wei.com, wangpeiyang1@...wei.com,
	liuyonglong@...wei.com, chenhao418@...wei.com,
	sudongming1@...wei.com, xujunsheng@...wei.com,
	shiyongbang@...wei.com, libaihan@...wei.com,
	jonathan.cameron@...wei.com, shameerali.kolothum.thodi@...wei.com,
	salil.mehta@...wei.com, netdev@...r.kernel.org,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH net-next 4/7] net: hibmcge: Add register dump supported
 in this module

> We have other considerations:
> 
> If the dump register changes in the future, we hope that
> only the kernel needs to be modified, and the ethtool does not need to be modified.
> In this case, We do not need to consider the mapping between the ethtool and driver versions.
> 
> So in ethtool, we only need to consider basic formatted printing.
> like this(not send yet):
> > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > 
> #define HBG_REG_NAEM_MAX_LEN 32
> struct hbg_reg_info {
> 	char name[HBG_REG_NAEM_MAX_LEN];
> 	u32 offset;
> 	u32 val;
> };
> static void hibmcge_dump_reg_info(struct hbg_reg_info *info)
> {
> 	fprintf(stdout, "%-*s[0x%04x]: 0x%08x\n",
> 		HBG_REG_NAEM_MAX_LEN, info->name, info->offset, info->val);
> }
> int hibmcge_dump_regs(struct ethtool_drvinfo *info __maybe_unused,
> 		      struct ethtool_regs *regs)
> {
> 	struct hbg_reg_info *reg_info;
> 	u32 name_max_len;
> 	u32 offset = 0;
> 	if (regs->len % sizeof(*reg_info) != 0)
> 		return -EINVAL;
> 	while (offset < regs->len) {
> 		reg_info = (struct hbg_reg_info *)(regs->data + offset);
> 		hibmcge_dump_reg_info(reg_info);
> 		offset += sizeof(*reg_info);
> 	}
> 	return 0;
> }
> > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > 
> 
> So, In this patch, pass back hbg_reg_info(name, offset, value)

So this is different to all other drivers doing registers dumps.

1) Please explain this in the commit message, with a justification why
your driver is different.

2) What is actually specific to your driver here? Why not make this
available to all drivers? Maybe check if ethtool_regs.version ==
MAX_U32 is used by any of the other drivers, and if not, make that a
magic value to indicate your special format.

3) Maybe consider that there does not appear to be a netlink version
of this ethtool ioctl. Could this be nicely integrated into a netlink
version, where you have more flexibility with attributes?


	Andrew