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

[Chris Johns]

On 13/10/2016 20:26, Pavel Pisa wrote:

I have tuneled to school an manager remote PC test system
to run the build on actual HW. So some following results have
been captured over serial port from PC box controlled by
project written by my colleague

https://github.com/wentasah/novaboot

to report real HW results.



I have built and tested these patches with no issues and 0 spurious 
interrupts.


The changes look fine to me.

Thanks
Chris
___
devel mailing list
devel@rtems.org
http://lists.rtems.org/mailman/listinfo/devel

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

[Gedare Bloom]

On Wed, Oct 12, 2016 at 4:26 AM,   wrote:
> From: Pavel Pisa 
>
> 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 ...

> -   rtems_interrupt_enable (level);
> +   RTEMS_COMPILER_MEMORY_BARRIER();
>  }
>
>  /*
> @@ -1311,7 +1310,6 @@ static void fxp_daemon(void *xsc)
> struct ifnet *ifp = >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
>
> ___
> devel mailing list
> devel@rtems.org
> http://lists.rtems.org/mailman/listinfo/devel
___
devel mailing list
devel@rtems.org
http://lists.rtems.org/mailman/listinfo/devel

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

[Pavel Pisa]

Some typo corrections of e-mail written when I have returned
late in night from meeting with friends.

And some more clarification as well.

On Thursday 13 of October 2016 01:55:30 Pavel Pisa wrote:
> Hello Chris,
>
> On Wednesday 12 of October 2016 23:05:30 Chris Johns wrote:
> > On 13/10/2016 03:22, Pavel Pisa wrote:
> > > But RTEMS i8269 support has been broken to disable
> > > vector for level triggered interrupts in generic
> > > IRQ processing code.
> >
> > I am not sure where the blame should be placed. We need to disable at
> > the PIC when using libbsd with shared PCI interrupts because an
> > interrupt server is used that is common to a few architectures. Some
> > legacy drivers like this one assume processing inside the interrupt
> > context. It is not clear to me shared interrupts were ever supported
> > with these drivers. I would assume it means some type of per driver
> > interrupt chaining.
> >
> > > So I have introduced reenable
> > > bsp_interrupt_vector_enable to ensure that driver
> > > can work even with that setup.
> >
> > I am not sure we can mix both models without some changes.
>
> I hope that interrupt server should work after the committed change.
> At least, I have feeling that it has been outcome of previous debate.
>
> The IRQ server bsp_interrupt_server_trigger() disables given IRQ
> vector on PIC level in hardware IRQ context by
> bsp_interrupt_vector_disable()
>
> See
>
>  
> https://git.rtems.org/rtems/tree/c/src/lib/libbsp/shared/src/irq-server.c#n
>64
>
> I would not push the changes if it has not be a case.
>
> > > classic networking: adapt FXP driver to work with actual PCI and IRQ
> > > code.
> > >
> > > The hack is not required after
> >
> > Which hack?
>
> The reenabling of PIC level ISR in driver code. Generally I consider
> the functions bsp_interrupt_vector_disable() and
> bsp_interrupt_vector_enable() should be used as paired and use should allow
> to use them even
> if implemented as counting disable clock> . 

spellchecker ...

s/clock/lock/

The counting of disable calls is required if vector is shared
because if multiple hard IRQ handlers needs to disable
source at controller level (generally bad practice, better is handling
on device level if possible) then if source is enabled on controller level
when the first worker theread finishes processing and cause of the shared
level triggered IRQ is other device for which threaded handler
dis not finish yet then there is complete system dead/livelock.

Linux provides next functions to maintain controller side
interrupt disable and enable

https://www.kernel.org/doc/htmldocs/kernel-api/hardware.html#idp11592384

 disable_irq - this function guarantees that after it finishes
   corresponding IRQ source handlers are not invoked
   and not running in parallel (wait for their finish)
   This function cannot be called in the handler itself
   (deadlock).

 disable_irq_nosync - disables vector at controller level, does not guarantee
   that the last actually running instances of each of shared
   handlers is finished before call return. This can be called
   for source from its own hard context handler.

 enable_irq - undoes effect of the corresponding disable_irq, it is necessary
   to call it as many times as disable_irq has been called before.
   Only when all calls are balanced then controller enables
   source.

I this that when all possible HW and SW constellations are possible then
this is only usable API.

And yes, there are strange thing in the world.

I have debugged over e-mail my CAN driver at another university when I have 
found that PCI card has level triggered IRQ output but multiple CAN 
controllers connected to local bus behind card PCI bridge has shared
interrupts and bridge has asserted interrupt only at shared signal
raising edge. So if PCI interrupt processing finished without beeing
sure that all chips behind bridge has their output inactive then
the device and CAN control/monitoring inside some intelligent van
on the street has been lost. Fortunately not without driver at that time.

> That is implementation where bsp_interrupt_vector_enable() enables vector
> only after same number of calls as was the number of calls
> bsp_interrupt_vector_enable()
>
> > > 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 have not testing the i386 with libbsd with your recent changes. I will
> > see what I can do. I did not notice the enables/disables had been
> > changed.
> >
> > > I believe 

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

[Pavel Pisa]

Hello Chris,

On Wednesday 12 of October 2016 23:05:30 Chris Johns wrote:
> On 13/10/2016 03:22, Pavel Pisa wrote:
> > But RTEMS i8269 support has been broken to disable
> > vector for level triggered interrupts in generic
> > IRQ processing code.
>
> I am not sure where the blame should be placed. We need to disable at
> the PIC when using libbsd with shared PCI interrupts because an
> interrupt server is used that is common to a few architectures. Some
> legacy drivers like this one assume processing inside the interrupt
> context. It is not clear to me shared interrupts were ever supported
> with these drivers. I would assume it means some type of per driver
> interrupt chaining.
>
> > So I have introduced reenable
> > bsp_interrupt_vector_enable to ensure that driver
> > can work even with that setup.
>
> I am not sure we can mix both models without some changes.

I hope that interrupt server should work after the committed change.
At least, I have feeling that it has been outcome of previous debate.

The IRQ server bsp_interrupt_server_trigger() disables given IRQ
vector on PIC level in hardware IRQ context by bsp_interrupt_vector_disable()

See

  https://git.rtems.org/rtems/tree/c/src/lib/libbsp/shared/src/irq-server.c#n64

I would not push the changes if it has not be a case.

> > classic networking: adapt FXP driver to work with actual PCI and IRQ
> > code.
> >
> > The hack is not required after
>
> Which hack?

The reenabling of PIC level ISR in driver code. Generally I consider
the functions bsp_interrupt_vector_disable() and bsp_interrupt_vector_enable()
should be used as paired and use should allow to use them even
if implemented as counting disable clock.
That is implementation where bsp_interrupt_vector_enable() enables vector
only after same number of calls as was the number of calls
bsp_interrupt_vector_enable()

> > 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 have not testing the i386 with libbsd with your recent changes. I will
> see what I can do. I did not notice the enables/disables had been changed.
>
> > I believe only, hat it is the right direction.
>
> I am sorry it is not clear to me what direction this is.

I have on mind changes required for i386 BSP SMP build and generally
that all RTEMS code (not only x86) should eliminate constructs based
on UP assumptions or directly incompatible with SMP. That is
rtems_interrupt_lock_acquire should be used to protect low level operations
which has to be serialized (short system level/hardware critical sections).

i386 support needs to move forward. basic SMP support and LAPIC seems
to be included. So it needs to enable its testing. The proposed changes
allows the build. I have tested UP and SMP i386 builds only under QEMU.
My test of networking, RTL and some others seems to work for both
builds. UP and SMP build single core applications seems to run
on real HW.

SMP build with 1 CPU output on real HW

-
novaboot: Serial line interaction (press Ctrl-C to interrupt)...
Scanning from 0x9D400 for 1024 bytes
Scanning from 0xF for 65536 bytes
Found MP Floating Structure Pointer at FDA40
Intel MultiProcessor Spec 1.4 BIOS support detected
APIC config: "Virtual Wire mode"Local APIC address: 0xFEE0
  OEM id: CBX3 Product id: DELL
  Bus id 0 is PCI
  Bus id 1 is PCI
  Bus id 2 is PCI
  Bus id 3 is PCI
  Bus id 4 is ISA
  I/O APIC id 2 ver 32, address: 0xFEC0
WARNING!! Found more CPUs (4) than configured for (1)!!


RTEMS v 4.11.99
Starting application appfoo v 0.1.0
*** Starting up Task_1 ***
Task_1 woken

RTEMS Shell on /dev/console. Use 'help' to list commands.
[/] # Task_1 woken
Task_1 woken
-

When I try to enable two CPUs and try to run SMP build on QEMU
and real HW then it breaks. REAL HW output

-
i386: isr=0 irr=1
Scanning from 0x9D400 for 1024 bytes
Scanning from 0xF for 65536 bytes
Found MP Floating Structure Pointer at FDA40
Intel MultiProcessor Spec 1.4 BIOS support detected
APIC config: "Virtual Wire mode"Local APIC address: 0xFEE0
  OEM id: CBX3 Product id: DELL
  Processor [APIC id 0 ver 21]: #0  BootStrap Processor (BSP)
  Processor [APIC id 2 ver 21]:

i386: isr=0 irr=1
Scanning from 0x9D400 for 1024 bytes
Scanning from 0xF for 65536 bytes
Found MP Floating Structure Pointer at FDA40
Intel MultiProcessor Spec 1.4 BIOS support detected
APIC config: "Virtual Wire mode"Local APIC address: 0xFEE0
  OEM id: 

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

[Chris Johns]

On 13/10/2016 03:22, Pavel Pisa wrote:

But RTEMS i8269 support has been broken to disable
vector for level triggered interrupts in generic
IRQ processing code.


I am not sure where the blame should be placed. We need to disable at 
the PIC when using libbsd with shared PCI interrupts because an 
interrupt server is used that is common to a few architectures. Some 
legacy drivers like this one assume processing inside the interrupt 
context. It is not clear to me shared interrupts were ever supported 
with these drivers. I would assume it means some type of per driver 
interrupt chaining.



So I have introduced reenable
bsp_interrupt_vector_enable to ensure that driver
can work even with that setup.


I am not sure we can mix both models without some changes.



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

The hack is not required after



Which hack?


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 have not testing the i386 with libbsd with your recent changes. I will 
see what I can do. I did not notice the enables/disables had been changed.




I believe only, hat it is the right direction.


I am sorry it is not clear to me what direction this is.

Chris
___
devel mailing list
devel@rtems.org
http://lists.rtems.org/mailman/listinfo/devel

Re: [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,   wrote:
> > From: Pavel Pisa 
> >
> > 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_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
> >
___
devel mailing list
devel@rtems.org
http://lists.rtems.org/mailman/listinfo/devel

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

[pisa]

From: Pavel Pisa 

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);
-   rtems_interrupt_enable (level);
+   RTEMS_COMPILER_MEMORY_BARRIER();
 }
 
 /*
@@ -1311,7 +1310,6 @@ static void fxp_daemon(void *xsc)
struct ifnet *ifp = >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);
- rtems_interrupt_enable (level);
+ RTEMS_COMPILER_MEMORY_BARRIER();
}
 }
 
-- 
1.9.1

___
devel mailing list
devel@rtems.org
http://lists.rtems.org/mailman/listinfo/devel