From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=2a00:1450:400c:c09::242; helo=mail-wm0-x242.google.com; envelope-from=leif.lindholm@linaro.org; receiver=edk2-devel@lists.01.org Received: from mail-wm0-x242.google.com (mail-wm0-x242.google.com [IPv6:2a00:1450:400c:c09::242]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id B610B210E38D6 for ; Thu, 9 Aug 2018 03:19:41 -0700 (PDT) Received: by mail-wm0-x242.google.com with SMTP id o11-v6so5762426wmh.2 for ; Thu, 09 Aug 2018 03:19:41 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:in-reply-to:user-agent; bh=L5LhGIMJk8t5ZhkXjEfqAD47lNS6M4zcrbaLVGPjLo8=; b=UQYosDNh1JSFTEz73o2DaaZR3uW3tWQpTOpP0ZdnIZe5oOtPFKALQ+AAqAfPR3gK20 7UxZ2fWiyjMqwvX2uISiLAgbbosKmSLJJwTUlYv8eh7thjyvrXkmsB7Ku1hsd1n/etZY wjWNSV24EvglkIWRNnPRQnvSYTUG5F4DecUSo= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:in-reply-to:user-agent; bh=L5LhGIMJk8t5ZhkXjEfqAD47lNS6M4zcrbaLVGPjLo8=; b=U3+x9cU4hExw8l5VqwRhBN93cEKT8mj185L0svAycCA+7rmX0gJ6Ig4iloqpFm6afC kQLQxapfxEeAdHA1HfntHDmirCE0BnnD5c9pPE3vNYE/LIH9OLM2uDwFzcxn20rfpVi1 /ZstTM1Ln4Wp4aefcy1/mKda7W0RtTBiJZZVacSFtYfZCj2J4qgdwkTLbswWlwL+S5mv V1+wWutO/tewWW0vuosY38epSUtee3508IBUxCXDZmtgw5BnCAAWM0Gd9gii0SbieIJ5 SxmAz0dLkt4HFlWLrcr0t3fH+RFGg4LuFSDRRXbqt3GuE0G+t2EWzvxaLurhzbEl3vQ/ vuQg== X-Gm-Message-State: AOUpUlHgHmXs0IjNUM7KgWo8lK69Iay2nRiHRYw65P6bHCf7H9ILfLQI W2csbWgFnGErlOQQuUpY+E+QkA== X-Google-Smtp-Source: AA+uWPy33xmSYlsX/OvsEwCOhM5o1Fu2hLSKwHvOc233nffYML5nLUcb8j0KZjL7wfeKChCZEqg4vA== X-Received: by 2002:a1c:b286:: with SMTP id b128-v6mr1097020wmf.121.1533809979575; Thu, 09 Aug 2018 03:19:39 -0700 (PDT) Received: from bivouac.eciton.net (bivouac.eciton.net. [2a00:1098:0:86:1000:23:0:2]) by smtp.gmail.com with ESMTPSA id q70-v6sm11326504wmd.39.2018.08.09.03.19.38 (version=TLS1_2 cipher=ECDHE-RSA-CHACHA20-POLY1305 bits=256/256); Thu, 09 Aug 2018 03:19:38 -0700 (PDT) Date: Thu, 9 Aug 2018 11:19:36 +0100 From: Leif Lindholm To: Ming Cc: linaro-uefi@lists.linaro.org, edk2-devel@lists.01.org, graeme.gregory@linaro.org, ard.biesheuvel@linaro.org, guoheyi@huawei.com, wanghuiqiang@huawei.com, huangming23@huawei.com, zhangjinsong2@huawei.com, huangdaode@hisilicon.com, john.garry@huawei.com, xinliang.liu@linaro.org, Heyi Guo Message-ID: <20180809101936.q4cqhe6qncnghgtr@bivouac.eciton.net> References: <20180724070922.63362-1-ming.huang@linaro.org> <20180724070922.63362-23-ming.huang@linaro.org> <20180803143604.brqho43khtiwbs5i@bivouac.eciton.net> <9300682f-c86e-e9f2-4d25-4455c492a3ca@linaro.org> MIME-Version: 1.0 In-Reply-To: <9300682f-c86e-e9f2-4d25-4455c492a3ca@linaro.org> User-Agent: NeoMutt/20170113 (1.7.2) Subject: Re: [PATCH edk2-platforms v1 22/38] Platform/Hisilicon/D06: Add OemNicLib X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.27 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 09 Aug 2018 10:19:42 -0000 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline On Thu, Aug 09, 2018 at 02:16:49PM +0800, Ming wrote: > >> +UINT16 crc_tab[256] = { > > > > CrcTable. > > Modify it in v2. > > > Hmm. > > This might be useful to add to a core library/driver/protocol. We > > don't appear to have it yet. This is another note to the universe. > > Do you suggest to move the CRC16 function to MdePkg/Library/BaseLib/CheckSum.c ? > I think it's not enouth time to do this before Linaro 18.08 maybe. Not needed for 18.08, but I would like to see the improvement made afterwards. > >> +EFI_STATUS > >> +EFIAPI > >> +OemGetMac ( > >> + IN OUT EFI_MAC_ADDRESS *Mac, > >> + IN UINTN Port > >> + ) > >> +{ > >> + EFI_STATUS Status; > >> + > >> + if (NULL == Mac) { > > > > No jeopardy tests. Turn the comparison around to the logical order. > > > >> + DEBUG ((DEBUG_ERROR, "[%a]:[%dL] Mac buffer is null!\n", > >> + __FUNCTION__, __LINE__)); > >> + return EFI_INVALID_PARAMETER; > >> + } > >> + > >> + Status = OemGetMacE2prom (Port, Mac->Addr); > >> + if ((EFI_ERROR (Status))) { > > > > Parantheses around EFI_ERROR are not needed. > > > >> + DEBUG ((DEBUG_ERROR, > >> + "[%a]:[%dL] Cannot get MAC from EEPROM, Status: %r; using default MAC.\n", > >> + __FUNCTION__, __LINE__, Status)); > >> + > >> + Mac->Addr[0] = 0x00; > >> + Mac->Addr[1] = 0x18; > >> + Mac->Addr[2] = 0x82; > >> + Mac->Addr[3] = 0x2F; > >> + Mac->Addr[4] = 0x02; > >> + Mac->Addr[5] = Port; > > > > I'm not super happy about this. This would wreak havoc on any real > > network. > > Arguably, a server platform should just fail hard at this point. > > I would certainly appreciate a Pcd making that an option. > > > > Otherwise, some sort of proper scheme would need to be implemented: > > using the 'locally administered range' of MAC addresses, and ensuring > > addresses are only allocated after checking for possible duplicates on > > the network. > > Do you suggest we should return EFI_NOT_FOUND here? Yes please. It would be good if we could have some (common) code to handle the fluke situation where you end up without your own MAC address. (So that the node can boot up and report that it is broken.) But it needs to be done in a reliable way, and that's too big a task for 18.08. / Leif