Re: git: 68cbe189fdd3 - main - pci: Don't try to read cfg registers of non-existing devices

On Thu, Nov 25, 2021 at 1:55 AM Wojciech Macek <wma@freebsd.org> wrote:

> The branch main has been updated by wma:
>
> URL:
> https://cgit.FreeBSD.org/src/commit/?id=68cbe189fdd3c572476f8af9219a5d335f05b51a
>
> commit 68cbe189fdd3c572476f8af9219a5d335f05b51a
> Author:     Kornel Duleba <mindal@semihalf.com>
> AuthorDate: 2021-11-23 08:13:56 +0000
> Commit:     Wojciech Macek <wma@FreeBSD.org>
> CommitDate: 2021-11-25 08:53:40 +0000
>
>     pci: Don't try to read cfg registers of non-existing devices
>
>     Instead of returning 0xffs some controllers, such as Layerscape
> generate
>     an external exception when someone attempts to read any register
>     of config space of a non-existing device other than PCIR_VENDOR.
>     This causes a kernel panic.
>     Fix it by bailing during device enumeration if a device vendor register
>     returns invalid value. (0xffff)
>     Use this opportunity to replace some hardcoded values with a macro.
>
>     I believe that this change won't have any unintended side-effects since
>     it is safe to assume that vendor == 0xffff -> hdr_type == 0xffff.
>

We didn't use to bail out if function 0 was not present due to
odd balls (I think on sparc64) which have long ago since become
irrelevant, so this should be OK.

Warner


>     Sponsored by:           Alstom
>     Obtained from:          Semihalf
>     Reviewed by:            jhb
>     MFC after:              2 weeks
>     Differential revision:  https://reviews.freebsd.org/D33059
> ---
>  sys/dev/pci/pci.c | 15 ++++++++++-----
>  1 file changed, 10 insertions(+), 5 deletions(-)
>
> diff --git a/sys/dev/pci/pci.c b/sys/dev/pci/pci.c
> index 7493ae1727fb..ecef65477137 100644
> --- a/sys/dev/pci/pci.c
> +++ b/sys/dev/pci/pci.c
> @@ -683,11 +683,12 @@ pci_read_device(device_t pcib, device_t bus, int d,
> int b, int s, int f)
>         uint16_t vid, did;
>
>         vid = REG(PCIR_VENDOR, 2);
> +       if (vid == PCIV_INVALID)
> +               return (NULL);
> +
>         did = REG(PCIR_DEVICE, 2);
> -       if (vid != 0xffff)
> -               return (pci_fill_devinfo(pcib, bus, d, b, s, f, vid, did));
>
> -       return (NULL);
> +       return (pci_fill_devinfo(pcib, bus, d, b, s, f, vid, did));
>  }
>
>  struct pci_devinfo *
> @@ -4141,6 +4142,10 @@ pci_add_children(device_t dev, int domain, int
> busno)
>                 pcifunchigh = 0;
>                 f = 0;
>                 DELAY(1);
> +
> +               /* If function 0 is not present, skip to the next slot. */
> +               if (REG(PCIR_VENDOR, 2) == PCIV_INVALID)
> +                       continue;
>                 hdrtype = REG(PCIR_HDRTYPE, 1);
>                 if ((hdrtype & PCIM_HDRTYPE) > PCI_MAXHDRTYPE)
>                         continue;
> @@ -4182,7 +4187,7 @@ pci_rescan_method(device_t dev)
>         for (s = 0; s <= maxslots; s++) {
>                 /* If function 0 is not present, skip to the next slot. */
>                 f = 0;
> -               if (REG(PCIR_VENDOR, 2) == 0xffff)
> +               if (REG(PCIR_VENDOR, 2) == PCIV_INVALID)
>                         continue;
>                 pcifunchigh = 0;
>                 hdrtype = REG(PCIR_HDRTYPE, 1);
> @@ -4191,7 +4196,7 @@ pci_rescan_method(device_t dev)
>                 if (hdrtype & PCIM_MFDEV)
>                         pcifunchigh = PCIB_MAXFUNCS(pcib);
>                 for (f = 0; f <= pcifunchigh; f++) {
> -                       if (REG(PCIR_VENDOR, 2) == 0xffff)
> +                       if (REG(PCIR_VENDOR, 2) == PCIV_INVALID)
>                                 continue;
>
>                         /*
>