Re: [Potential Spoof] Re: [PATCH net-next v4] net/ncsi: Add NCSI Broadcom OEM command
From: Samuel Mendoza-Jonas
Date: Tue Oct 16 2018 - 01:46:18 EST
On Mon, 2018-10-15 at 17:38 +0000, Vijay Khemka wrote:
> ïOn 10/15/18, 10:27 AM, "Linux-aspeed on behalf of Vijay Khemka" <linux-aspeed-bounces+vijaykhemka=fb.com@xxxxxxxxxxxxxxxx on behalf of vijaykhemka@xxxxxx> wrote:
> On 10/14/18, 8:51 PM, "Samuel Mendoza-Jonas" <sam@xxxxxxxxxxxxxxxx> wrote:
> 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?
> I agree with you setting it only once. I gave a thought about config option and realize that
> we should allow user to configure it. If user wants to set mac address through device tree
> and not through ROM then we must not override mac set by device tree. So my proposal is
> setting of mac address in response should be hidden under config option. Getting mac address
> can still go without config option. Your thought?
> or simply guard following block under config and no other function declaration guard required.
> And set static variable flag in function " ncsi_oem_handler" for calling this only once.
> #if IS_ENABLED(CONFIG_NCSI_OEM_CMD_GET_MAC)
> nca.type = NCSI_PKT_CMD_OEM;
> nca.package = np->id;
> nca.channel = nc->id;
> ndp->pending_req_num = 1;
> ret = ncsi_oem_handler(&nca, nc->version.mf_id);
> #endif /* CONFIG_NCSI_OEM_CMD_GET_MAC */
Either way is likely fine; although you might need to take care you don't
get an unused function warning depending on how you guard
ncsi_oem_handler(). Also a flag in the ncsi_dev_priv struct could be
easier to keep track of than having a static variable in the handler