Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Message-ID: <cb9dedaf242264f76eca18e94934703300be5a7e.camel@yadro.com>
Subject: Re:  [PATCH 1/1] net/ncsi: add get MAC address command to get Intel
 i210 MAC address
From:   Ivan Mikhaylov <i.mikhaylov@yadro.com>
To:     Milton Miller II <miltonm@us.ibm.com>
CC:     "David S . Miller" <davem@davemloft.net>,
        Jakub Kicinski <kuba@kernel.org>,
        Samuel Mendoza-Jonas <sam@mendozajonas.com>,
        Brad Ho <Brad_Ho@phoenix.com>,
        Paul Fertser <fercerpav@gmail.com>, <openbmc@lists.ozlabs.org>,
        <linux-kernel@vger.kernel.org>, <netdev@vger.kernel.org>,
        <devicetree@vger.kernel.org>
Date:   Thu, 2 Sep 2021 18:17:30 +0300
In-Reply-To: <OF2487FB9E.ECED277D-ON00258741.006BEF89-00258744.001FE4C0@ibm.com>
References: <20210830171806.119857-2-i.mikhaylov@yadro.com>
         <OF2487FB9E.ECED277D-ON00258741.006BEF89-00258744.001FE4C0@ibm.com>
Organization: YADRO
Content-Type: text/plain; charset="UTF-8"
User-Agent: Evolution 3.38.4 (3.38.4-1.fc33) 
MIME-Version: 1.0
Content-Transfer-Encoding: 8bit
Precedence: bulk

On Thu, 2021-09-02 at 05:48 +0000, Milton Miller II wrote:
> On August 30, 2021, Ivan Mikhaylov" <i.mikhaylov@yadro.com> 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.