Re: [PATCH pci] PCI: don't skip probing entire device if first fn OF node has status = "disabled"

[Vladimir Oltean]

Hi Rob,

On Fri, Jun 16, 2023 at 11:57:43AM -0600, Rob Herring wrote:
> On Sun, Jun 4, 2023 at 2:55 AM Vladimir Oltean <vladimir.oltean@xxxxxxx> wrote:
> >
> 
> Sorry, just now seeing this as I've been out the last month.
> 
> > On Sat, Jun 03, 2023 at 10:35:50AM +0800, Jianmin Lv wrote:
> > > > How about 3. handle of_device_is_available() in the probe function of
> > > > the "loongson, pci-gmac" driver? Would that not work?
> > > >
> > > This way does work only for the specified device. There are other devices,
> > > such as HDA, I2S, etc, which have shared pins. Then we have to add
> > > of_device_is_available() checking to those drivers one by one. And we are
> > > not sure if there are other devices in new generation chips in future. So
> > > I'm afraid that the way you mentioned is not suitable for us.
> 
> If we decided that disabled devices should probe, then that is exactly
> what will have to be done. The restriction (of shared pins) is in the
> devices and is potentially per device, so it makes more sense for the
> device's drivers to handle than the host bridge IMO. (Assuming the
> core doesn't handle a per device property.)
> 
> 
> > Got it, so you have more on-chip PCIe devices than the ones listed in
> > loongson64-2k1000.dtsi, and you don't want to describe them in the
> > device tree just to put status = "disabled" for those devices/functions
> > that you don't want Linux to use - although you could, and it wouldn't
> > be that hard or have unintended side effects.
> >
> > Though you need to admit, in case you had an on-chip multi-function PCIe
> > device like the NXP ENETC, and you wanted Linux to not use function 0,
> > the strategy you're suggesting here that is acceptable for Loongson
> > would not have worked.
> >
> > I believe we need a bit of coordination from PCIe and device tree
> > maintainers, to suggest what would be the encouraged best practices and
> > ways to solve this regression for the ENETC.
> 
> I think we need to define what behavior is correct for 'status =
> "disabled"'. For almost everywhere in DT, it is equivalent to the
> device is not present. A not present device doesn't probe. There are
> unfortunately cases where status got ignored/forgotten and PCI was one
> of those. PCI is a bit different since there are 2 sources of
> information about a device being present. The intent with PCI is DT
> overrides what's discovered. For example, 'vendor-id' overrides what's
> read from the h/w.
> 
> I think we can fix making the status per function simply by making
> 'match_driver' be set based on the status. This would move the check
> later to just before probing. That would not work for a case where
> accessing the config registers is a problem. It doesn't sound like
> that's a problem for Loongson based on the above response, but their
> original solution did prevent that. This change would also mean the
> PCI quirks would run. Perhaps the func0 memory clearing you need could
> be run as a quirk instead?
> 
> Rob

Sorry to return to this thread very late. I had lots of other stuff to
take care of, and somehow *this* breakage had less priority :)

So, first off, there's a confusion regarding the "func0 memory clearing"
that could be run as a quirk instead. It's not memory clearing for fn 0,
but memory clearing for all ENETC functions, regardless or not whether
they have status = "disabled" or not in the device tree.

That being said, I've implemented the workaround below in a quirk as
you've said, and the quirks only get applied for those PCI functions
which don't have status = "disabled" in the device tree. So, as things
stand, it won't work.

Also, the original patch on which we're commenting ("PCI: don't skip
probing entire device if first fn OF node has status = "disabled"") is
needed in any case, because of the other issue: the PCI core thinks that
when fn 0 has status = "disabled", fn 1 .. 6 are also unavailable. False.