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

[Jijie Shao]

on 2024/10/24 20:22, Andrew Lunn wrote:
> > 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.

In fact, we don't have anything different with other drivers.

In the customer environment, the ethtool version may not be the latest.

If the driver adds a register to the register dump, the register is unknown
when the ethtool is used to query the register.
Therefore, we want to separate the ethtool from the driver.
No matter how the driver is modified, the ethtool can display all information perfectly.

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

In fact, it would be best if ethtool could provide a unified framework to
elegantly display all register information.
The existing framework prints the information in hexadecimal format,
which is not intuitive enough.

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

Okay, I'll analyze it.

Thanks a lot.
Jijie Shao