Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Message-ID: <69cae9d44cbebb2cd4f468dc710d6a97210af835.camel@mendozajonas.com>
Subject: Re: [PATCH net-next v4] net/ncsi: Add NCSI Broadcom OEM command
From:   Samuel Mendoza-Jonas <sam@mendozajonas.com>
To:     Vijay Khemka <vijaykhemka@fb.com>,
        "David S. Miller" <davem@davemloft.net>, netdev@vger.kernel.org,
        linux-kernel@vger.kernel.org
Cc:     linux-aspeed@lists.ozlabs.org, openbmc@lists.ozlabs.org
Date:   Mon, 15 Oct 2018 13:08:46 +1100
In-Reply-To: <20181012182008.1665690-1-vijaykhemka@fb.com>
References: <20181012182008.1665690-1-vijaykhemka@fb.com>
Content-Type: text/plain; charset="UTF-8"
User-Agent: Evolution 3.30.1 
Mime-Version: 1.0
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

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@fb.com>
> ---
>  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.

> +#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