Re: [PATCH 4/4] PCI: Add a REBAR size quirk for Sapphire RX 5600 XT Pulse

[Bjorn Helgaas <helgaas@kernel.org>]

On Thu, Jan 07, 2021 at 06:50:17PM +0100, Nirmoy Das wrote:
> RX 5600 XT Pulse advertises support for BAR0 being 256MB, 512MB,
> or 1GB, but it also supports 2GB, 4GB, and 8GB. Add a rebar
> size quirk so that CPU can fully access the BAR0.

This isn't quite accurate.  The CPU can fully access BAR 0 no matter
what.  I think the problem you're solving is that without this quirk,
BAR 0 isn't big enough to cover the VRAM.

> Signed-off-by: Christian König <christian.koenig@amd.com>
> Reported-by: kernel test robot <lkp@intel.com>
> Reported-by: Dan Carpenter <dan.carpenter@oracle.com>

IIRC, these Reported-by lines are from the "cap == 0x3f0" problem.  It
would make sense to include these if this patch fixed that problem in
something that had already been merged.  But this *hasn't* been
merged, so these lines only make sense to someone who trawls through
the mailing list to find the previous version.

I don't really think it's worthwhile to include them.  It's the same
as giving credit to reviewers, which we typically don't do except via
a Reviewed-by tag (which I think is too strong for this case) or a
"v2" changes note after the "---" line.  That doesn't get included in
the git history, but is easily findable via the Link: tags as below.

If you merge these via a non-PCI tree, please include the "Link:"
lines in the PCI patches, e.g.,

  Link: https://lore.kernel.org/r/20210107175017.15893-5-nirmoy.das@amd.com

for this one.  Obviously the link is different for each patch and will
change if you repost the series.

I'm not sure why you put the amd patch in the middle of the series.
Seems like it would be slightly prettier and just as safe to put it at
the end.

> Signed-off-by: Nirmoy Das <nirmoy.das@amd.com>

Acked-by: Bjorn Helgaas <bhelgaas@google.com>

Let me know if you want me to take the series.

> ---
>  drivers/pci/pci.c | 9 ++++++++-
>  1 file changed, 8 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index 16216186b51c..b061bbd4afb1 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -3577,7 +3577,14 @@ u32 pci_rebar_get_possible_sizes(struct pci_dev *pdev, int bar)
>  		return 0;
>  
>  	pci_read_config_dword(pdev, pos + PCI_REBAR_CAP, &cap);
> -	return (cap & PCI_REBAR_CAP_SIZES) >> 4;
> +	cap = (cap & PCI_REBAR_CAP_SIZES) >> 4;
> +
> +	/* Sapphire RX 5600 XT Pulse has an invalid cap dword for BAR 0 */
> +	if (pdev->vendor == PCI_VENDOR_ID_ATI && pdev->device == 0x731f &&
> +	    bar == 0 && cap == 0x700)
> +		cap = 0x3f00;

I think this is structured wrong.  It should be like this so it's
easier to match with the spec:

  cap &= PCI_REBAR_CAP_SIZES;

  if (... && cap == 0x7000)
    cap = 0x3f000;

  return cap >> 4;

> +
> +	return cap;
>  }
>  EXPORT_SYMBOL(pci_rebar_get_possible_sizes);
>  
> -- 
> 2.29.2
>