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

[Andrew Lunn]

> 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