From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=2a00:1450:4864:20::444; helo=mail-wr1-x444.google.com; envelope-from=leif.lindholm@linaro.org; receiver=edk2-devel@lists.01.org Received: from mail-wr1-x444.google.com (mail-wr1-x444.google.com [IPv6:2a00:1450:4864:20::444]) (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 821B321BADAB3 for ; Tue, 14 Aug 2018 08:48:17 -0700 (PDT) Received: by mail-wr1-x444.google.com with SMTP id h9-v6so17650180wro.3 for ; Tue, 14 Aug 2018 08:48:17 -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:content-transfer-encoding:in-reply-to :user-agent; bh=j/bGdPQWo+DqdfIRHZfpoq0pGKv0P5HV0UhHSj/hkTE=; b=LZwFsH0D3tmta3c1ZzADjjn38N8bxnUbgQhsOHGtQp4bOa3OrZ+kGpbL6g+tJFNnye tSyGqu4bY2hJ2LpoCsKMOnDheHaFrQVU1n8dKNjO4WV/GgcgYG4xZFQjPu9IR1r8HcjP uHPxoVb2mHJYZ3Mvu5kz4uXMm0spuA1uxSSUk= 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:content-transfer-encoding :in-reply-to:user-agent; bh=j/bGdPQWo+DqdfIRHZfpoq0pGKv0P5HV0UhHSj/hkTE=; b=LbYOXBEkkcKYtc100x/Jb0nJYqTI2JqOGngSHGrA0fDW6zUJ95ZWQSabiSM13N4Y85 jpOm4FhGIkypQDWdiCEObbMAqJ/tAqQ+EqaIxMdPhzz1AtbzKYcSRG8EgXAHb2586zjs owy08cqi7FSfU+T4cUQC2xW+aQ0Vu4fb/7v8QlA0XDMpom0Li08xLoJP2IGxp6VC5mc6 9fmZ2nB6kDkQYslOkXM7f/5IGFckx+TdjIi9Gf2HWayWh3DUL65Ze4EksdJf04S5O8l7 9Wkbr8gU6cTZukzRE5PA5b5JIDe/ncqgqjQMAGEYbNIaJSmeq4y3W/eNaTToA/f4mf85 yMSw== X-Gm-Message-State: AOUpUlEJa6OP9fzutheYLkqvcH+QIauA1OBdgiG0MoR+xJ6WO/Go3reT YlKoMkWel/v61x+jkfw8VOKgnqVFm0w= X-Google-Smtp-Source: AA+uWPyNvzGNNfqrr4KB6GUGYEt6ervV8Dxt750KDl7BsZZ28tK2O5Vmw55ab5zwhhIfm5Kw6X60hA== X-Received: by 2002:a5d:4a84:: with SMTP id o4-v6mr11197435wrq.82.1534261695909; Tue, 14 Aug 2018 08:48:15 -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-v6sm26374004wmd.39.2018.08.14.08.48.14 (version=TLS1_2 cipher=ECDHE-RSA-CHACHA20-POLY1305 bits=256/256); Tue, 14 Aug 2018 08:48:14 -0700 (PDT) Date: Tue, 14 Aug 2018 16:48:12 +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: <20180814154812.c23f5ys4zobnwb5w@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> <20180809101936.q4cqhe6qncnghgtr@bivouac.eciton.net> <780640eb-44fb-1c44-2cdf-7ea4d38119f8@linaro.org> MIME-Version: 1.0 In-Reply-To: <780640eb-44fb-1c44-2cdf-7ea4d38119f8@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: Tue, 14 Aug 2018 15:48:17 -0000 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: 8bit 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? The alternative would be to ensure the function does not return. But I would not recommend that. / Leif