Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Date:   Fri, 15 Nov 2019 15:52:14 -0700
From:   Alex Williamson <alex.williamson@redhat.com>
To:     Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Cc:     linux-kernel@vger.kernel.org, Christoph Hellwig <hch@lst.de>,
        Dan Williams <dan.j.williams@intel.com>,
        Cornelia Huck <cohuck@redhat.com>
Subject: Re: [PATCH v1 5/5] vfio/pci: Drop duplicate check for size
 parameter of memremap()
Message-ID: <20191115155214.55e949cc@x1.home>
In-Reply-To: <20191115180044.83659-5-andriy.shevchenko@linux.intel.com>
References: <20191115180044.83659-1-andriy.shevchenko@linux.intel.com>
        <20191115180044.83659-5-andriy.shevchenko@linux.intel.com>
Organization: Red Hat
MIME-Version: 1.0
Content-Type: text/plain; charset=WINDOWS-1252
Content-Transfer-Encoding: quoted-printable
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Fri, 15 Nov 2019 20:00:44 +0200
Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:

> Since memremap() returns NULL pointer for size =3D 0, there is no need
> to duplicate this check in the callers.
>=20
> Cc: Christoph Hellwig <hch@lst.de>
> Cc: Dan Williams <dan.j.williams@intel.com>
> Cc: Alex Williamson <alex.williamson@redhat.com>
> Cc: Cornelia Huck <cohuck@redhat.com>
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
>  drivers/vfio/pci/vfio_pci_igd.c | 8 +-------
>  1 file changed, 1 insertion(+), 7 deletions(-)
>=20
> diff --git a/drivers/vfio/pci/vfio_pci_igd.c b/drivers/vfio/pci/vfio_pci_=
igd.c
> index 53d97f459252..3088a33af271 100644
> --- a/drivers/vfio/pci/vfio_pci_igd.c
> +++ b/drivers/vfio/pci/vfio_pci_igd.c
> @@ -75,13 +75,7 @@ static int vfio_pci_igd_opregion_init(struct vfio_pci_=
device *vdev)
>  =09=09return -EINVAL;
>  =09}
> =20
> -=09size =3D le32_to_cpu(*(__le32 *)(base + 16));
> -=09if (!size) {
> -=09=09memunmap(base);
> -=09=09return -EINVAL;
> -=09}
> -
> -=09size *=3D 1024; /* In KB */
> +=09size =3D le32_to_cpu(*(__le32 *)(base + 16)) * 1024; /* In KB */
> =20
>  =09if (size !=3D OPREGION_SIZE) {
>  =09=09memunmap(base);

This seems convoluted, patch 1/5 states "[t]here is no use of memremap()
to be called with size =3D 0", which we weren't doing thanks to the check
removed above.  So now we are potentially calling it with zero,
apparently only to take advantage of this new behavior, and we lose the
error granularity that previously such a condition failed with an
-EINVAL and now we fail with an -ENONMEM and cannot distinguish whether
the OpRegion table size was empty or we just weren't able to memremap()
it.  I don't see how this is an improvement.  Thanks,

Alex