On Fri, 5 Apr 2019, Mikulas Patocka wrote:
> > > I did some more testing and it turns out that mb() is not entirely
> > > sufficient to prevent the boot hang. mb()'s latecy varies, sometimes it is
> > > sufficient, sometimes not.
> > >
> > > So, I submit this patch that adds 1us delay between any I/O accesses
> > > directed at the ISA bus. This patch makes my machine boot. 1us seems to be
> > > minimal acceptable value, with 800ns I still get hangs.
> >
> > Why wasn't the delay needed then before commit cd0e00c10672 ("alpha: io:
> > reorder barriers to guarantee writeX() and iowriteX() ordering"), which
> > only moved `mb' around?
> >
> > Maciej
>
> Suppose that someone does
> outl(123, INDEX); x = inl(DATA);
>
> Before the patch cd0e00c10672, the kernel would do
> __iowrite(123, INDEX);
> mb();
> x = __ioread(DATA);
> mb();
>
> After the patch cd0e00c10672, the kernel would do
> mb();
> __iowrite(123, INDEX);
> x = __ioread(DATA);
> mb();
>
> The patch changes the timing between the write and the read and the
> hardware doesn't like it.
Obviously you do need that `mb' before `__ioread' in the second case,
just like in the first one, because otherwise the read bus access issued
by `__ioread' can be reordered ahead of the write bus access issued by the
preceding `__iowrite'. A delay inserted by executing a loop out of cache
will usually prevent such reordering as with the external bus being idle
the write bus access issued by `__iowrite' will eventually have retired,
but without an intervening `mb' it is not actually guaranteed that the
retirement will happen ahead of `__ioread'.
I hope I explained it clearly, however please do let me know if you think
you are still missing something here.
Also please mind Linus's note on a possible need to do a read back to
make the write bus access retire right away (as Alpha does not have an
explicit completion barrier instruction) in case a driver expects strict
x86 semantics, e.g.:
/* Beginning of `outl'. */
mb();
__iowrite(123, INDEX);
mb();
__ioread(INDEX);
/* End of `outl'. */
/* Beginning of `inl'. */
mb();
x = __ioread(DATA);
mb(); /* Or `dma_rmb();'. */
/* End of `inl'. */
(though in reality it may actually be overly aggressive in terms of
synchronisation, and also issuing `__ioread' to INDEX may not be safe in
all cases, as it may cause side effects with some device registers, so
another location may have to be used instead; e.g. for PCI port I/O 0x80
might be a reasonable choice, at least for systems that have a south
bridge).
Maciej
