Re: [PATCH v3 00/32] PCI: fix config and I/O Address space memory mappings

[Benjamin Herrenschmidt <benh@kernel.crashing.org>]

On Wed, 2017-04-12 at 15:41 +0100, Lorenzo Pieralisi wrote:
> > > At least we make the failure explicit, and the driver can take
> > > alternate (possibly sub-optimal) action if it chooses to do so.
> > 
> > The same points apply to things like pgprot_writecombine(),
> > pgprot_noncached(), pgprot_device().  Then there's also pgprot_nonposted()
> > that this series also introduces.

No. pgprot_writecombine() silently falling back to simply non-cached is
ok as we aren't "weakening" the ordering rules silently here. Something
that is correct with writecombine will also work without. It's just an
optimisation. Not correctness.

Things like noncached() must of course be honored, and I don't think
we have a case anywhere where it isn't.

My point with nopost() is that it's never ok to silently downgrade it.
Code written with the assumption that there is no posting will be
*incorrect* if posting happens. We do live with that "bug" today indeed
but once we have that accessors we might start growing more code that
relies on the specific attribute that things aren't posted and will be
wrong on all the archs providing the default implementation.

This is why I insist that pgprot_nopost() if it exists globally, should
return NULL when the semantic cannot be provided. That way there is a
clear line in the sand. If the driver choses to operate with posted
non-cached anyway, then make it an explicit driver choice.

> > If ioremap_nopost() is not possible on an architecture, then
> > pgprot_nonposted() won't be possible either - but you've made no
> > mention of that so far.

Right. It's not on most in fact.

> > Just like the proposed ioremap_nopost(), pgprot_nonposted() is given a
> > default implementation that uses pgprot_noncached().  Maybe we should
> > also make pci_remap_iospace() fail if pgprot_nonposted() is not defined
> > by the architecture?

Or we *document* that mmap of IO space can result in something that is
partially non-posted.

> Yes, I was about to mention that and you are right, I should deal with
> that too unfortunately. BTW, I have not posted the drivers to make the
> review easier (ie it would add 20 more patches to an already massive
> patch series - that will be trimmed when the asm-generic include is
> removed from arches according to this discussion).
> 
> Thanks,
> Lorenzo

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel