[PATCH 3/3] libchip/network/if_fxp.c: do not use rtems_interrupt_disable.

[Pavel Pisa]

Hello Gedare,

On Wednesday 12 of October 2016 18:10:19 Gedare Bloom wrote:
> On Wed, Oct 12, 2016 at 4:26 AM,  <pisa at cmp.felk.cvut.cz> wrote:
> > From: Pavel Pisa <pisa at cmp.felk.cvut.cz>
> >
> > The single write to memory or ioport output are mostly
> > atomic operations already. The proper memory synchronization barrier
> > should be used around them to guarantee ordering (sync or eieio
> > on PowerPC for example) but because I have not found settable
> > portable primitive only compiler barrier is used.
> > It should be enough on x86 because the externally visible order
> > should be/is guaranteed to be preserved on x86 architecture.
> > ---
> >  c/src/libchip/network/if_fxp.c | 11 ++++-------
> >  1 file changed, 4 insertions(+), 7 deletions(-)
> >
> > diff --git a/c/src/libchip/network/if_fxp.c
> > b/c/src/libchip/network/if_fxp.c index 4d9d983..c2ca419 100644
> > --- a/c/src/libchip/network/if_fxp.c
> > +++ b/c/src/libchip/network/if_fxp.c
> > @@ -1130,7 +1130,6 @@ fxp_start(struct ifnet *ifp)
> >  {
> >         struct fxp_softc *sc = ifp->if_softc;
> >         struct fxp_cb_tx *txp;
> > -       rtems_interrupt_level level;
> >
> >         DBGLVL_PRINTK(3,"fxp_start called\n");
> >
> > @@ -1279,10 +1278,10 @@ tbdinit:
> >         /*
> >          * reenable interrupts
> >          */
> > -       rtems_interrupt_disable (level);
> > +       RTEMS_COMPILER_MEMORY_BARRIER();
> >         CSR_WRITE_1(sc, FXP_CSR_SCB_INTRCNTL,0);
> >         bsp_interrupt_vector_enable(sc->irq_num);
>
> Here you don't delete bsp_interrupt_vector_enable ...

Good catch. The reason is that there should not be
bsp_interrupt_vector_enable() in neither.
The interrupt is enabled after attach and should stay that
way for whole driver life. The disable at ISR level
and enable in daemon is controlled on the board registers
level  for FXP which is how correct PCI driver with
possibly shared interrupts should work.

But RTEMS i8269 support has been broken to disable
vector for level triggered interrupts in generic
IRQ processing code. So I have introduced reenable
bsp_interrupt_vector_enable to ensure that driver
can work even with that setup.

classic networking: adapt FXP driver to work with actual PCI and IRQ code.

The hack is not required after

bsps/i386: Separate variable for i8259 IRQs disable due to in progress state.

so I have removed unneeded reenable from daemon hot path.
I have left it in the setup to be sure that it is enabled
after some driver stop start cycles.

In theory, this occurrence should be deleted as well.

Generally, I am not sure if/how much I have broken/I am
breaking i386 support by all these changes.

I believe only, hat it is the right direction.
I would be happy to discus them with others.

Thanks for review,

                     Pavel


> > -       rtems_interrupt_enable (level);
> > +       RTEMS_COMPILER_MEMORY_BARRIER();
> >  }
> >
> >  /*
> > @@ -1311,7 +1310,6 @@ static void fxp_daemon(void *xsc)
> >         struct ifnet *ifp = &sc->sc_if;
> >         u_int8_t statack;
> >         rtems_event_set events;
> > -       rtems_interrupt_level level;
> >
> >  #ifdef NOTUSED
> >         if (sc->suspended) {
> > @@ -1458,10 +1456,9 @@ rcvloop:
> >           /*
> >            * reenable interrupts
> >            */
> > -         rtems_interrupt_disable (level);
> > +         RTEMS_COMPILER_MEMORY_BARRIER();
> >           CSR_WRITE_1(sc, FXP_CSR_SCB_INTRCNTL,0);
> > -         bsp_interrupt_vector_enable(sc->irq_num);
>
> ... here you do. Why are the cases different?
>
> > -         rtems_interrupt_enable (level);
> > +         RTEMS_COMPILER_MEMORY_BARRIER();
> >         }
> >  }
> >
> > --
> > 1.9.1
> >