Re: [PATCH 3/3] libchip/network/if_fxp.c: do not use rtems_interrupt_disable.
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.
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->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.
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 only,
Re: [PATCH 3/3] libchip/network/if_fxp.c: do not use rtems_interrupt_disable.
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: CBX
Re: [PATCH 3/3] libchip/network/if_fxp.c: do not use rtems_interrupt_disable.
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.
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->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