Re: [PATCH net-next v4] net/ncsi: Add NCSI Broadcom OEM command

[Samuel Mendoza-Jonas]

On Mon, 2018-10-15 at 13:08 +1100, Samuel Mendoza-Jonas wrote:
> On Fri, 2018-10-12 at 11:20 -0700, Vijay Khemka wrote:
> > This patch adds OEM Broadcom commands and response handling. It also
> > defines OEM Get MAC Address handler to get and configure the device.
> > 
> > ncsi_oem_gma_handler_bcm: This handler send NCSI broadcom command for
> > getting mac address.
> > ncsi_rsp_handler_oem_bcm: This handles response received for all
> > broadcom OEM commands.
> > ncsi_rsp_handler_oem_bcm_gma: This handles get mac address response and
> > set it to device.
> > 
> > Signed-off-by: Vijay Khemka <vijaykhemka@xxxxxx>
> > ---
> >  v4: updated as per comment from Sam, I was just wondering if I can remove
> >  NCSI_OEM_CMD_GET_MAC config option and let this code be valid always and
> >  it will configure mac address if there is get mac address handler for given 
> >  manufacture id.
> 
> Hi Vijay,
> 
> We can look at handling this a different way, but I don't think we want
> to unconditionally set the system's MAC address based on the OEM GMA
> command. If the user wants to set a custom MAC address, or in the case of
> OpenBMC for example who have their MAC address saved in flash, this will
> override that value with whatever the Network Controller has saved. In
> particular as it is set up it will override any MAC address every time a
> channel is configured, such as during a failover event.
> 
> We *could* always send the GMA command if it is available and move the
> decision whether to use the resulting address or not into the response
> handler. That would simplify the ncsi_configure_channel() logic a bit.
> Another idea may be to have a Netlink command to tell NCSI to ignore the
> GMA result; then we could drop the config option and the system can
> safely change the address if desired.
> 
> Any thoughts? I'll also ping some of the OpenBMC people and see what
> their expectations are.

After a bit of a think and an ask around, to quote a colleague:
> I think we'd want it handled (overall) like any other net device; the MAC
> address in the device's ROM provides a default, and is overridden by anything
> specified by userspace 

Which describes what I was thinking pretty well.
So if we can have it such that the NCSI driver only sets the MAC address
_once_, and then after then does not update it again, we should be able to call
the OEM GMA command without hiding it behind a config option. So the first time
a channel was configured we store and set the MAC address given, but then on
later configure events we don't continue to update it. What do you think?

Cheers,
Sam

> 
> > +#if IS_ENABLED(CONFIG_NCSI_OEM_CMD_GET_MAC)
> > +
> > +/* NCSI OEM Command APIs */
> > +static void ncsi_oem_gma_handler_bcm(struct ncsi_cmd_arg *nca)
> > +{
> > +	unsigned char data[NCSI_OEM_BCM_CMD_GMA_LEN];
> > +	int ret = 0;
> > +
> > +	nca->payload = NCSI_OEM_BCM_CMD_GMA_LEN;
> > +
> > +	memset(data, 0, NCSI_OEM_BCM_CMD_GMA_LEN);
> > +	*(unsigned int *)data = ntohl(NCSI_OEM_MFR_BCM_ID);
> > +	data[5] = NCSI_OEM_BCM_CMD_GMA;
> > +
> > +	nca->data = data;
> > +
> > +	ret = ncsi_xmit_cmd(nca);
> > +	if (ret)
> > +		netdev_err(nca->ndp->ndev.dev,
> > +			   "NCSI: Failed to transmit cmd 0x%x during configure\n",
> > +			   nca->type);
> > +}
> 
> As a side note while unlikely we probably want to propagate the return
> value of ncsi_xmit_cmd() from here; otherwise we'll miss a failure and
> the configure process will stall.
> 
> Regards,
> Sam
>