Re: [PATCH v2] PCI: Add quirk for HiSilicon NP 5896 devices

[Bjorn Helgaas]

On Mon, Dec 30, 2019 at 04:11:50PM +0800, Xiongfeng Wang wrote:
> Hi, Bjorn
> 
> On 2019/12/18 22:28, Bjorn Helgaas wrote:
> > On Wed, Dec 18, 2019 at 05:16:03PM +0800, Xiongfeng Wang wrote:
> >> On 2019/12/11 12:10, Bjorn Helgaas wrote:
> >>> On Tue, Dec 10, 2019 at 9:28 PM Xiongfeng Wang
> >>> <wangxiongfeng2@xxxxxxxxxx> wrote:
> >>>> On 2019/12/7 2:10, Bjorn Helgaas wrote:
> >>>>> On Fri, Dec 06, 2019 at 03:01:45PM +0800, Xiongfeng Wang wrote:
> >>>>>> HiSilicon PCI Network Processor 5896 devices misreport the
> >>>>>> class type as 'NOT_DEFINED', but it is actually a network
> >>>>>> device. Also the size of BAR3 is reported as 265T, but this BAR
> >>>>>> is actually unused.  This patch modify the class type to
> >>>>>> 'CLASS_NETWORK' and disable the unused BAR3.
> > 
> >>>>> The question is not whether the BAR is used by the driver; the
> >>>>> question is whether the device responds to accesses to the
> >>>>> region described by the BAR when PCI_COMMAND_MEMORY is turned
> >>>>> on.
> >>>>
> >>>> I asked the hardware engineer. He said I can not write an address
> >>>> into that BAR.
> >>>
> >>> If the BAR is not writable, I think sizing should fail, so I
> >>> suspect some of the bits are actually writable.
> >>
> >> Sorry for the delayed response. It's not so convenient for me to get
> >> to the hardware guys.  BAR0 BAR1 BAR2 are 32-bit and can be used to
> >> access the registers and memory within 5896 devices. These three
> >> BARs can meet the need for most scenario.  BAR3 is 64-bit and can be
> >> used to access all the registers and memory within 5896 devices.
> >> (BAR3 is writable. Sorry for the non-confirmed information before.)
> >> But BAR3 is not used by the driver and the size is very
> >> largeïlarger than 100G, still didn't get the precise sizeï.  So I
> >> think maybe we can disable this BAR for now, otherwise the
> >> unassigned resource will cause 'pci_enable_device()' returning
> >> failure.
> > 
> > Here's the problem: the proposed patch (below) clears the struct
> > resource corresponding to BAR 3, but that doesn't actually disable the
> > BAR.  It hides the BAR from Linux, so Linux will pretend it doesn't
> > exist, but it's still there in the hardware.
> > 
> > The hardware BAR 3 still contains some value (possibly zero), and if
> > PCI_COMMAND_MEMORY is set (which you need to do if you want to use
> > *any* memory BARs on the device), the device will respond to any
> > transactions in the BAR 3 range.  Depending on the topology and all
> > the other BAR and window assignments, this may cause address
> > conflicts.
> 
> I have checked with the hardware engineer. He said the transactions
> have some bits to indicate whether the address is 32-bit or 64-bit.
> The device will respond only when the 64-bit address transactions is
> in the BAR3 range.
> 
> So I think, if I clear the resource corresponding to BAR3, the
> 64-bit window of the downport is empty. There will be no 64-bit
> address transaction sent to the device.

Sorry to repeat myself, but your patch only clears the struct
resource, which is in memory.  It doesn't touch the BAR (the hardware
register in the device) at all.  It does not disable the BAR.  It does
not guarantee that the memory windows of the Downstream Port leading
to the device will be disabled.  It does not prevent upper level
software from generating transactions that will match BAR 3.

> > + * HiSilicon NP 5896 devices BAR3 size is reported as 256T and causes problem
> > + * when assigning the resources. But this BAR is actually unused by the driver,
> > + * so let's disable it.
> > + */
> > +static void quirk_hisi_fixup_np_bar(struct pci_dev *pdev)
> > +{
> > +       struct resource *r = &pdev->resource[3];
> > +
> > +       r->start = 0;
> > +       r->end = 0;
> > +       r->flags = 0;
> > +
> > +       pci_info(pdev, "Disabling invalid BAR 3\n");
> > 
> > .
> > 
>