Re: [PATCH v4 1/2] net: brcm: netXtreme driver

["Marek Behún" <marek.behun@nic.cz>]

Hello Roman,

On Thu, 28 Oct 2021 16:29:28 -0700
Roman Bacik <roman.bacik@broadcom.com> wrote:

> +void bnxt_env_set_ethaddr(struct udevice *dev)
> +{
> +	struct eth_pdata *plat = dev_get_plat(dev);
> +	char cmd[100];
> +	char var[32];
> +	u8 mac_env[ARP_HLEN];
> +
> +	eth_env_get_enetaddr_by_index("eth", dev_seq(dev), mac_env);
> +	if (!memcmp(plat->enetaddr, mac_env, ARP_HLEN))
> +		return;
> +
> +	sprintf(var, dev_seq(dev) ? "%s%daddr" : "%saddr", "eth", dev_seq(dev));
> +	sprintf(cmd, "%s %s %pM", "env set -f", var, plat->enetaddr);
> +	run_command(cmd, CMD_FLAG_ENV);
> +}
> +
> +void bnxt_env_del_ethaddr(struct udevice *dev)
> +{
> +	struct eth_pdata *plat = dev_get_plat(dev);
> +	char cmd[100];
> +	char var[32];
> +
> +	sprintf(var, dev_seq(dev) ? "%s%daddr" : "%saddr", "eth", dev_seq(dev));
> +	sprintf(cmd, "%s %s %pM", "env delete -f", var, plat->enetaddr);
> +	run_command(cmd, CMD_FLAG_ENV);
> +}

And then in bnxt_eth_probe():
> +	eth_env_get_enetaddr_by_index("eth", dev_seq(dev), bp->mac_set);
...
> +	memcpy(plat->enetaddr, bp->mac_set, ETH_ALEN);
> +	bnxt_env_set_ethaddr(dev);

So if I understand this correctly, in bnxt_eth_probe(), you read env
variable ethNaddr into bp->mac_set. Then bnxt_bring_chip() is called,
which calls various functions, including bnxt_hwrm_func_qcaps_req(),
which may overwrite bp->mac_set. Then bp->mac_set is copied into
plat->enetaddr.

Then bnxt_env_set_ethaddr() is called, which reads the env variable
ethNaddr again, and compares it with value in plat->enetaddr, and if
they are different, it overwrites the value in ethNaddr with
plat->enetaddr.

I have this to say:
- could you please explain why this is done so? I mean the logic behind
  this...
- it seems to me that you haven't read the documentation for struct
  env_ops in include/net.h: there are methods read_rom_hwaddr() and
  write_hwaddr(), which you could use, instead of implementing this
  whole mechanism ad-hoc. You should use those or explain your reasons
  why you aren't doing this
- why do you need the plat structure? Why not use bp->mac_set directly,
  without plat->enetaddr?
- the way you set and delete ethNaddr variable, by running U-Boot
  commands, is very weird, when there is a direct function for setting
  the value:
    eth_env_set_enetaddr_by_index()
  As for deleting:
  - why do you need it in the first place? I mean you are removing the
    variable upon driver removal. No other ethernet driver does that...
  - instead of assembling the "env delete" command it would make far
    more sense to include patch that adds env_del() function into
    env/common.c

I have another question: does this driver support adapters with SFP
cages only, or also those with RJ-45 ports?

Thank you.

Marek