Messages in this thread | | | From | Vijay Khemka <> | Subject | Re: [PATCH net-next 1/2] net/ncsi: Add NCSI Broadcom OEM command | Date | Mon, 8 Oct 2018 19:18:04 +0000 |
| |
On 10/7/18, 5:58 PM, "Samuel Mendoza-Jonas" <sam@mendozajonas.com> wrote:
On Fri, 2018-10-05 at 12:01 -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> Hi Vijay, Looks good, and I've tested this on a BMC with a Broadcom network controller and it properly sets a MAC address. A question below about the response handler:
Hi Sam, Thanks for feedback, I will update code for response handler. Please see my response below.
> +/* Response handler for Broadcom command Get Mac Address */ > +static int ncsi_rsp_handler_oem_bcm_gma(struct ncsi_request *nr) > +{ > + struct ncsi_rsp_oem_pkt *rsp; > + struct ncsi_dev_priv *ndp = nr->ndp; > + struct net_device *ndev = ndp->ndev.dev; > + int ret = 0; > + const struct net_device_ops *ops = ndev->netdev_ops; > + struct sockaddr saddr; > + > + /* Get the response header */ > + rsp = (struct ncsi_rsp_oem_pkt *)skb_network_header(nr->rsp); > + > + saddr.sa_family = ndev->type; > + ndev->priv_flags |= IFF_LIVE_ADDR_CHANGE; > + memcpy(saddr.sa_data, &rsp->data[BCM_MAC_ADDR_OFFSET], ETH_ALEN); > + /* Increase mac address by 1 for BMC's address */ > + saddr.sa_data[ETH_ALEN - 1]++; Is this convention documented somewhere, and could you provide a link to it if so? Also what happens here if the final byte of the address is 0xff, this will overflow and not carry right?
This is a Facebook requirement to increment MAC address by 1 as base address is defined for server followed by BMC address. I will handle this Facebook specific code differently in new patch. For generic Broadcom card, I will just get base mac address and not add any convention. > + ret = ops->ndo_set_mac_address(ndev, &saddr); > + if (ret < 0) > + netdev_warn(ndev, "NCSI: 'Writing mac address to device failed\n"); Also a minor nitpick, there's a bonus "'" in this message. Will be taken care in next version.
| |