From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail-wr1-f68.google.com (mail-wr1-f68.google.com [209.85.221.68]) by mx.groups.io with SMTP id smtpd.web09.9109.1582098525348987894 for ; Tue, 18 Feb 2020 23:48:45 -0800 Authentication-Results: mx.groups.io; dkim=pass header.i=@linaro.org header.s=google header.b=zLAv44GD; spf=pass (domain: linaro.org, ip: 209.85.221.68, mailfrom: ard.biesheuvel@linaro.org) Received: by mail-wr1-f68.google.com with SMTP id z7so26932933wrl.13 for ; Tue, 18 Feb 2020 23:48:45 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=/oZqOqdsabufWktZwKWWunal0PkxTbAPFyu9dDHh/z0=; b=zLAv44GDt4EczrS+VznV7mTYHWqoTXHc2gj3OIFk+qpkHeQZ9GSq7oiVpN7OymlNej OBldMWjjM3XJM2AHWDC/VQc+1Df1bmStZCNG/EK0HCX5Z8Z6Ge8ZzdN+/5EmvkZ1m9jI 7P+PpQgjSEEkCXyBva1ooDu4f/owc2FAYgQ/Nf3gHRVioW133ycmMBOfMz7yAP9kxa3g O0PYBmnJuDQXAO8ge6ND6r/zb13nMjEX0zLRBL6Grjq3Pl6jpSHmItylwcPh0lvK4/oh MUADd7XbhicAoHNjxRKOzAyF1Dy9tdECbAKEjLq1krOzToAGOWa5nbQcpMrdojbJ2/6u R81A== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=/oZqOqdsabufWktZwKWWunal0PkxTbAPFyu9dDHh/z0=; b=X5+YyyATlSoRvc/J+LUOwmb5Isos3FQwNL9xHSoQ2kA7gy0SpxlnDT5YfcBWs9P/++ B2+SsDBdnkPddew0fY+YA1LASVRrExpg+6Po+4Xmw1MPBsecnM5dryFEd10l+KdeUQmk nHsdC6Z2JKs7BXC1hExXsYgjb1pGLJ6nn36wvaiuPdbRmHJOYPd2fQexny+JZJjYT64x p18zjE23dKEGlTf1F2ABu9sE0SSQwOWwleCrUN4jp8tRsKNGOP/WYfsZAMcI59u/gKpV 0/65zHOunHsTZcGYVKh/5lqRg2odmZXreYfGcwZEyF9YZHw/td6AddpMERqZ0mh06cXT /QAA== X-Gm-Message-State: APjAAAWfiPs2kqFq1l/0pZBuI9uCVg69QJqHfwpVnaboey/L67VwzgrM HXV2AvpctSj/q6tpoMdmyihUQw9HA87euMlCzB8DOg== X-Google-Smtp-Source: APXvYqwSFVQ2eNXvL+dOW1QhjjSykSo8D3nLcFGV6olu+lf9a3E5FNfwoyR8xeT5bxr7TnYyu3s2dkjv5EYMBZPqPMo= X-Received: by 2002:a5d:65cf:: with SMTP id e15mr33614832wrw.126.1582098523831; Tue, 18 Feb 2020 23:48:43 -0800 (PST) MIME-Version: 1.0 References: <20200130133804.32261-1-gaurav.jain@nxp.com> In-Reply-To: From: "Ard Biesheuvel" Date: Wed, 19 Feb 2020 08:48:33 +0100 Message-ID: Subject: Re: [PATCH 1/1] MdeModulePkg/Pci: Fixed Asserts in SCT PCIIO Protocol Test. To: "Wu, Hao A" Cc: Gaurav Jain , "devel@edk2.groups.io" , "Wang, Jian J" , "Ni, Ray" , Pankaj Bansal Content-Type: text/plain; charset="UTF-8" On Wed, 19 Feb 2020 at 07:34, Wu, Hao A wrote: > > > -----Original Message----- > > From: Gaurav Jain [mailto:gaurav.jain@nxp.com] > > Sent: Thursday, January 30, 2020 4:18 PM > > To: devel@edk2.groups.io > > Cc: Wang, Jian J; Wu, Hao A; Ni, Ray; Pankaj Bansal; Gaurav Jain > > Subject: [PATCH 1/1] MdeModulePkg/Pci: Fixed Asserts in SCT PCIIO Protocol > > Test. > > > > ASSERT in CopyMem_Conf, PollMem_Conf, SetBarAttributes_Conf > > Conformance Test. > > SCT Test expect return as Invalid Parameter. > > So removed ASSERT(). > > > Include Ard (as the driver contributor) to see if he has additional comments. > > A couple of general level comments: > 1. I think the ASSERT can still be added after the checks you add. The ASSERT > may serve as a notification in the DEBUG image that the service is not > implemented. > Agreed. > 2. I found that for functions: > PciIoPollIo() > PciIoIoRead() > PciIoIoWrite() > They also do not have checks that perfectly match with the UEFI spec. Even > though they are not exposed by the SCT tests, could you help to address them > as well? > > Some other inline comments below. > > > > > > Signed-off-by: Gaurav Jain > > --- > > .../NonDiscoverablePciDeviceIo.c | 20 ++++++++++++++++--- > > 1 file changed, 17 insertions(+), 3 deletions(-) > > > > diff --git > > a/MdeModulePkg/Bus/Pci/NonDiscoverablePciDeviceDxe/NonDiscoverableP > > ciDeviceIo.c > > b/MdeModulePkg/Bus/Pci/NonDiscoverablePciDeviceDxe/NonDiscoverableP > > ciDeviceIo.c > > index 2d55c9699322..76cb000602fc 100644 > > --- > > a/MdeModulePkg/Bus/Pci/NonDiscoverablePciDeviceDxe/NonDiscoverableP > > ciDeviceIo.c > > +++ > > b/MdeModulePkg/Bus/Pci/NonDiscoverablePciDeviceDxe/NonDiscoverableP > > ciDeviceIo.c > > @@ -93,7 +93,15 @@ PciIoPollMem ( > > OUT UINT64 *Result > > ) > > { > > - ASSERT (FALSE); > > + if ((UINT32)Width >= EfiPciIoWidthMaximum || > > > Looks to me that the 1st part of the 'if' check is redundant. > Could you help to double confirm? > > > > + Width > EfiPciIoWidthUint64) { > > + return EFI_INVALID_PARAMETER; > > + } > > + > > + if (Result == NULL) { > > + return EFI_INVALID_PARAMETER; > > + } > > + > > return EFI_UNSUPPORTED; > > } > > > > @@ -556,7 +564,10 @@ PciIoCopyMem ( > > IN UINTN Count > > ) > > { > > - ASSERT (FALSE); > > + if ((UINT32)Width >= EfiPciIoWidthMaximum || > > > Looks to me that the 1st part of the 'if' check is redundant. > Could you help to double confirm? > > Best Regards, > Hao Wu > > > > + Width > EfiPciIoWidthUint64) { > > + return EFI_INVALID_PARAMETER; > > + } > > return EFI_UNSUPPORTED; > > } > > > > @@ -1414,7 +1425,10 @@ PciIoSetBarAttributes ( > > IN OUT UINT64 *Length > > ) > > { > > - ASSERT (FALSE); > > + if (Offset == NULL || Length == NULL) { > > + return EFI_INVALID_PARAMETER; > > + } > > + > > return EFI_UNSUPPORTED; > > } > > > > -- > > 2.17.1 >