Hi all,
I'm doing some refactoring of the dpaa/fman drivers, and I'm a bit
confused by the way IRQs are handled. To contrast, in GAM/MACB, one of
the first things IRQ handler does is grab a spinlock guarding register
access. This lets it do read/modify/writes all it wants. However, I
don't see anything like that in the FMan code. I'd like to use two
examples to illustrate.
First, consider call_mac_isr. It will race with both fman_register_intr:
CPU0 (call_mac_isr) CPU1 (fman_register_intr)
isr_cb = foo
isr_cb(src_handle)
src_handle = bar
and with fman_unregister_intr
CPU0 (call_mac_isr) CPU1 (fman_unregister_intr)
if (isr_cb)
isr_cb = NULL
src_handle = NULL
isr_cb(src_handle)
This is probably not too critical (since hopefully there are no
interrupts before/after the handler is registered), but it certainly
looks very strange.
Second, consider dtsec_isr. It will race with (for example) dtsec_set_allmulti:
CPU0 (dtsec_isr) CPU1 (dtsec_set_allmulti)
<XFUNEN interrupt>
ioread32be(rctrl) ioread32be(rctrl)
iowrite32be(rctrl | MPROM)
iowrite32be(rctrl | GRS)
and suddenly the MPROM write is dropped. (Actually, the whole
FM_TX_LOCKUP_ERRATA_DTSEC6 errata code seems funky, since after calling
fman_reset_mac it seems like everything would need to be reinitialized).
So what's going on here? Is there actually no locking, or am I missing
something?
--Sean
> -----Original Message-----
> From: Sean Anderson <[email protected]>
> Sent: Tuesday, May 31, 2022 22:09
> To: Madalin Bucur <[email protected]>; netdev
> <[email protected]>
> Cc: Linux Kernel Mailing List <[email protected]>; Florinel
> Iordache <[email protected]>
> Subject: net: fman: IRQ races
>
> Hi all,
>
> I'm doing some refactoring of the dpaa/fman drivers, and I'm a bit
> confused by the way IRQs are handled. To contrast, in GAM/MACB, one of
> the first things IRQ handler does is grab a spinlock guarding register
> access. This lets it do read/modify/writes all it wants. However, I
> don't see anything like that in the FMan code. I'd like to use two
> examples to illustrate.
>
> First, consider call_mac_isr. It will race with both fman_register_intr:
>
> CPU0 (call_mac_isr) CPU1 (fman_register_intr)
> isr_cb = foo
> isr_cb(src_handle)
> src_handle = bar
>
> and with fman_unregister_intr
>
> CPU0 (call_mac_isr) CPU1 (fman_unregister_intr)
> if (isr_cb)
> isr_cb = NULL
> src_handle = NULL
> isr_cb(src_handle)
>
> This is probably not too critical (since hopefully there are no
> interrupts before/after the handler is registered), but it certainly
> looks very strange.
>
> Second, consider dtsec_isr. It will race with (for example) dtsec_set_allmulti:
>
> CPU0 (dtsec_isr) CPU1 (dtsec_set_allmulti)
> <XFUNEN interrupt>
> ioread32be(rctrl) ioread32be(rctrl)
> iowrite32be(rctrl | MPROM)
> iowrite32be(rctrl | GRS)
>
> and suddenly the MPROM write is dropped. (Actually, the whole
> FM_TX_LOCKUP_ERRATA_DTSEC6 errata code seems funky, since after
> calling
> fman_reset_mac it seems like everything would need to be reinitialized).
>
> So what's going on here? Is there actually no locking, or am I missing
> something?
Hi
You are right, there is no locking. The original FMan driver design didn't intend
on supporting runtime register changes. Clearly this was a mistake as you
pointed out.
Camelia