Re: [RFC PATCH 2/2] PCI: apple: Add driver for the Apple M1

[Marc Zyngier <maz@kernel.org>]

On Mon, 16 Aug 2021 02:31:40 +0100,
Alyssa Rosenzweig <alyssa@rosenzweig.io> wrote:
> 
> Hi Marc,
> 
> Thank you for the review.

I wouldn't call that a review. Only a cursory look and a quick mention
of what I found really odd. Without specs, this thing is impossible to
properly review.

[...]

> > Please use relaxed accessors. If the barriers are actually needed,
> > please document what you are ordering against. This applies throughout
> > the patch.
> 
> Relaxed accessors are used throughout in v2... it Works For Me™ but no
> guarantees I didn't introduce a race...

That's not exactly what I wanted to read... You really need to make an
informed decision on the need of barriers. If the MMIO write needs to
be ordered after a main memory write (i.e. a memory write that is
consumed by the device you are subsequently writing to), you then need
a barrier. If, as I suspect, the device isn't DMA capable and doesn't
require ordering with the rest of the memory accesses, then no
barriers are required.

> 
> > This also begs the question: can this be called concurrently?
> 
> I'm not sure. Sven, any idea how Apple devices are usually
> structured here?

Nothing here is Apple specific. If you can get two CPUs to issue a RMW
on the same register, this piece of code is broken. This code has an
undocumented assumption of being single threaded, and it is pretty
unclear whether this assumption holds or not.

[...]

> > > +	writel(0xfb512fff, port + PORT_INTMSKSET);
> > 
> > Magic. What is this for?
> 
> Sven's explanation is the most likely. This magic value comes from
> Corellium via Mark; I assume it's written by macOS.

I really wish there was no magic values whatsoever, and I've resisted
posting the PCIe support patch myself for this very reason. Frankly,
this stuff doesn't give me the warm feeling that we know what we're
doing.

> 
> > > +	writel(PORT_INT_LINK_UP | PORT_INT_LINK_DOWN | PORT_INT_AF_TIMEOUT |
> > > +	       PORT_INT_REQADDR_GT32 | PORT_INT_MSI_ERR |
> > > +	       PORT_INT_MSI_BAD_DATA | PORT_INT_CPL_ABORT |
> > > +	       PORT_INT_CPL_TIMEOUT | (1 << 26), port + PORT_INTSTAT);
> > > +
> > > +	usleep_range(5000, 10000);
> > > +
> > > +	rmwl(0, PORT_LTSSMCTL_START, port + PORT_LTSSMCTL);
> > > +
> > > +	ret = readl_poll_timeout(port + PORT_LINKSTS, stat,
> > > +				 stat & PORT_LINKSTS_UP, 100, 500000);
> > > +	if (ret < 0) {
> > > +		dev_err(pcie->dev, "port %u link up wait timeout\n", i);
> > > +		return ret;
> > > +	}
> > 
> > I have the strong feeling that a lot of things in the above is to get
> > an interrupt when the port reports an event. Why the polling then?
> 
> I reordered the code to have the configuration after this happen
> before the START command as suggested (this works), and then removed
> the poll entirely (this also works?). It's possible the poll here
> was just a debug leftover in the original code.

What happens if the core PCI code probes the ports without the link
being up yet?

> It's possible it's needed in the original but not needed in the
> interrupt-driven common code (if the link doesn't come up yet,
> nothing happens, so we don't have to block on it ourselves..)
> 
> It's also possible I've introduced a race that we happen to win every time.
> 
> Without specs, it's exceedingly hard to know which it is...

Indeed, and I hate this "finger in the air" approach. Specially when
you need to trust your data to it.

> The poll isn't what we want at any rate, so I've removed the poll in
> v2. But we may want extra interrupt handling code for v3.

Indeed. I need to rework the MSI patch anyway after the discussion
with Rob, and I'll see what I can do for the rest of the event stuff.

Thanks,

	M.

-- 
Without deviation from the norm, progress is not possible.