Re: [PATCH 1/1] net/ncsi: add get MAC address command to get Intel i210 MAC address

[Ivan Mikhaylov]

On Thu, 2021-09-02 at 05:48 +0000, Milton Miller II wrote:
> On August 30, 2021, Ivan Mikhaylov" <i.mikhaylov@xxxxxxxxx> wrote:
> > This patch adds OEM Intel GMA command and response handler for it.
> > 
> > /* Get Link Status */
> > struct ncsi_rsp_gls_pkt {
> >         struct ncsi_rsp_pkt_hdr rsp;        /* Response header   */
> > diff --git a/net/ncsi/ncsi-rsp.c b/net/ncsi/ncsi-rsp.c
> > index d48374894817..6447a09932f5 100644
> > --- a/net/ncsi/ncsi-rsp.c
> > +++ b/net/ncsi/ncsi-rsp.c
> > @@ -699,9 +699,51 @@ static int ncsi_rsp_handler_oem_bcm(struct
> > ncsi_request *nr)
> >         return 0;
> > }
> > 
> > +/* Response handler for Intel command Get Mac Address */
> > +static int ncsi_rsp_handler_oem_intel_gma(struct ncsi_request *nr)
> > +{
> > +       struct ncsi_dev_priv *ndp = nr->ndp;
> > +       struct net_device *ndev = ndp->ndev.dev;
> > +       const struct net_device_ops *ops = ndev->netdev_ops;
> > +       struct ncsi_rsp_oem_pkt *rsp;
> > +       struct sockaddr saddr;
> > +       int ret = 0;
> > +
> > +       /* 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[INTEL_MAC_ADDR_OFFSET], ETH_ALEN);
> > +       /* Increase mac address by 1 for BMC's address */
> > +       eth_addr_inc((u8 *)saddr.sa_data);
> > +       if (!is_valid_ether_addr((const u8 *)saddr.sa_data))
> > +               return -ENXIO;
> 
> The Intel GMA retireves the MAC address of the host, and the datasheet
> anticipates the BMC will "share" the MAC by stealing specific TCP and 
> UDP port traffic destined to the host.
> 
> This "add one" allocation of the MAC is therefore a policy, and one that 
> is beyond the data sheet.
> 
> While this +1 policy may work for some OEM boards, there are other boards 
> where the MAC address assigned to the BMC does not follow this pattern, 
> but instead the MAC is stored in some platform dependent location obtained 
> in a platform specific manner.
> 
> I suggest this BMC = ether_addr_inc(GMA) be opt in via a device tree
> property.  
> 
> as it appears it would be generic to more than one vendor.
> 
> Unfortunately, we missed this when we added the broadcom and mellanox
> handlers.
> 
> 
> 

Milton,

maybe something like "mac_addr_inc" or "ncsi,mac_addr_inc"? Also those 3(intel,
mellanox, broadcom) functions even with handlers similar to each other, they
could be unified on idea, difference in addresses, payload lengths, ids only as
I see. Joel proposed in the past about dts option for Intel OEM keep_phy option,
maybe that's the right time to reorganize all OEM related parts to fit in one
direction with dts options for ethernet interface without Kconfig options?

Thanks.