From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=2607:f8b0:4864:20::443; helo=mail-pf1-x443.google.com; envelope-from=ming.huang@linaro.org; receiver=edk2-devel@lists.01.org Received: from mail-pf1-x443.google.com (mail-pf1-x443.google.com [IPv6:2607:f8b0:4864:20::443]) (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 EC47821B02822 for ; Wed, 15 Aug 2018 04:08:48 -0700 (PDT) Received: by mail-pf1-x443.google.com with SMTP id y10-v6so381939pfn.8 for ; Wed, 15 Aug 2018 04:08:48 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=subject:to:cc:references:from:message-id:date:user-agent :mime-version:in-reply-to:content-transfer-encoding; bh=0jV/zzZehBp/SRCFyQNlfYtVuY2mBFnJEOjbcuwNkWA=; b=AHW0VOdvw9KYBWRE+nDzgHhBbfm8kbiqEyXNo4NeDEC4x9TjZ22OvxSvVKVCE3T8dn iP4S7ZBv1RplrtdMewmEUvweHm6GyZgFLa4a4/r4ElgZPn0Ohue6OmP9AIFoLbLBjskd rwl2G3uXLodb/NcEsYSfTvtc7iP5av6TrKXgE= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:subject:to:cc:references:from:message-id:date :user-agent:mime-version:in-reply-to:content-transfer-encoding; bh=0jV/zzZehBp/SRCFyQNlfYtVuY2mBFnJEOjbcuwNkWA=; b=rDpUHSDyJPqRoDVlJLRFsyo2w4dYaMHocvS+l+NUufH+gqwVXdTjBmDpK2lmUvkoHr IQkn53caeZHhvSz8TxJbqys6/xi57TmlGXXm5c+V1ub94f2v/0b6ym0YCaxkYHWx0Bm1 v04630A2P258YuZGe1Hunw5yedtFwxWxY9X3QyldCxRP2L5jDLuNIRf1cbc+KX0b3UB2 VWLqKmkaBdcVeaU0O+1+GIcEnkp+mGDktie/Yq9ODPso5CIPx+KUpN+A2GWGdZ+745kU iNyoe0cEh0rf68y1p0DtZpTIyXKQgyQQ5AYdLuhcXo1hTOPOGIyewf3gVV+D9PTFZRF2 Wrxw== X-Gm-Message-State: AOUpUlHk4gYGTIrQZDHfk4omklJS7Dzmt7NTOocQOFum1ekUUVGkUPc6 XrhSZle6oKijPVVDfBOdhRqyjg== X-Google-Smtp-Source: AA+uWPywtJacGgQSC01azdF71P9fzd6pBwCSrMwdMVZqHQ/YX35RleHc07MTgF7rXcR3rTLyVPOzaA== X-Received: by 2002:a62:1647:: with SMTP id 68-v6mr27233454pfw.6.1534331328120; Wed, 15 Aug 2018 04:08:48 -0700 (PDT) Received: from [10.174.0.166] ([64.64.108.240]) by smtp.gmail.com with ESMTPSA id z11-v6sm45234323pff.162.2018.08.15.04.08.39 (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Wed, 15 Aug 2018 04:08:47 -0700 (PDT) To: Leif Lindholm 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 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> <20180809101936.q4cqhe6qncnghgtr@bivouac.eciton.net> <780640eb-44fb-1c44-2cdf-7ea4d38119f8@linaro.org> <20180814154812.c23f5ys4zobnwb5w@bivouac.eciton.net> From: Ming Message-ID: Date: Wed, 15 Aug 2018 19:08:33 +0800 User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:52.0) Gecko/20100101 Thunderbird/52.7.0 MIME-Version: 1.0 In-Reply-To: <20180814154812.c23f5ys4zobnwb5w@bivouac.eciton.net> 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: Wed, 15 Aug 2018 11:08:49 -0000 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 8bit 在 8/14/2018 11:48 PM, Leif Lindholm 写道: > On Tue, Aug 14, 2018 at 10:38:14AM +0800, Ming wrote: >> >> >> 在 8/9/2018 6:19 PM, Leif Lindholm 写道: >>> 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. >> >> I found some modules which invoke OemGetMac() don't judge the Status of >> OemGetMac, so it may cause some issue now if changing to EFI_NOT_FOUND. >> How about change it while we handle the fluke situation after 18.08 ? > > We cannot release 18.08 with known bugs. > And not checking return value is a bug. > > I presume you mean that these calling functions are inside HwPkg? Yes. All D06 board will burn a Mac to eeprom before delivery and there is a command (SetMac) to write a Mac. For handling the fluke situation, we think there are several ways: 1 Initialize Mac to 0xFF; Kernel seems will create a random Mac while the Mac is 0xFF. 2 Make a Mac from ArmReadCntPct() and gTR->GetTime(); 3 Make a locally administered Mac from ArmReadCntPct() and gTR->GetTime(); The 2nd is the way our product project use to handle the fluke situation. What is your suggestion? > > The alternative would be to ensure the function does not return. > But I would not recommend that. > > / > Leif >