This patchset adds MAB [1] offload support in mv88e6xxx.
Patch #1: Correct default return value for mv88e6xxx_port_bridge_flags.
Patch #2: Change chip lock handling in ATU interrupt handler.
Patch #3: The MAB implementation for mv88e6xxx.
LOG:
V2: -FID reading patch already applied, so dropped here. [1]
-Patch #2 here as separate patch instead of part of MAB
implementation patch.
-Check if fid is MV88E6XXX_FID_STANDALONE, and not if
fid is zero, as that is the correct check. Do not
report an error.
[1] https://git.kernel.org/netdev/net-next/c/4bf24ad09bc0
Hans J. Schultz (3):
net: dsa: mv88e6xxx: change default return of
mv88e6xxx_port_bridge_flags
net: dsa: mv88e6xxx: disable hold of chip lock for handling
net: dsa: mv88e6xxx: mac-auth/MAB implementation
drivers/net/dsa/mv88e6xxx/Makefile | 1 +
drivers/net/dsa/mv88e6xxx/chip.c | 20 +++---
drivers/net/dsa/mv88e6xxx/chip.h | 15 +++++
drivers/net/dsa/mv88e6xxx/global1_atu.c | 22 +++++--
drivers/net/dsa/mv88e6xxx/switchdev.c | 83 +++++++++++++++++++++++++
drivers/net/dsa/mv88e6xxx/switchdev.h | 19 ++++++
6 files changed, 147 insertions(+), 13 deletions(-)
create mode 100644 drivers/net/dsa/mv88e6xxx/switchdev.c
create mode 100644 drivers/net/dsa/mv88e6xxx/switchdev.h
--
2.34.1
As functions called under the interrupt handler will need to take the
netlink lock, we need to release the chip lock before calling those
functions as otherwise double lock deadlocks will occur as userspace
calls towards the driver often take the netlink lock and then the
chip lock.
The deadlock would look like:
Interrupt handler: chip lock taken, but cannot take netlink lock as
userspace config call has netlink lock.
Userspace config: netlink lock taken, but cannot take chip lock as
the interrupt handler has the chip lock.
Signed-off-by: Hans J. Schultz <[email protected]>
---
drivers/net/dsa/mv88e6xxx/global1_atu.c | 14 ++++++++------
1 file changed, 8 insertions(+), 6 deletions(-)
diff --git a/drivers/net/dsa/mv88e6xxx/global1_atu.c b/drivers/net/dsa/mv88e6xxx/global1_atu.c
index 61ae2d61e25c..34203e112eef 100644
--- a/drivers/net/dsa/mv88e6xxx/global1_atu.c
+++ b/drivers/net/dsa/mv88e6xxx/global1_atu.c
@@ -409,11 +409,11 @@ static irqreturn_t mv88e6xxx_g1_atu_prob_irq_thread_fn(int irq, void *dev_id)
err = mv88e6xxx_g1_read_atu_violation(chip);
if (err)
- goto out;
+ goto out_unlock;
err = mv88e6xxx_g1_read(chip, MV88E6XXX_G1_ATU_OP, &val);
if (err)
- goto out;
+ goto out_unlock;
err = mv88e6xxx_g1_atu_fid_read(chip, &fid);
if (err)
@@ -421,11 +421,13 @@ static irqreturn_t mv88e6xxx_g1_atu_prob_irq_thread_fn(int irq, void *dev_id)
err = mv88e6xxx_g1_atu_data_read(chip, &entry);
if (err)
- goto out;
+ goto out_unlock;
err = mv88e6xxx_g1_atu_mac_read(chip, &entry);
if (err)
- goto out;
+ goto out_unlock;
+
+ mv88e6xxx_reg_unlock(chip);
spid = entry.state;
@@ -449,13 +451,13 @@ static irqreturn_t mv88e6xxx_g1_atu_prob_irq_thread_fn(int irq, void *dev_id)
fid);
chip->ports[spid].atu_full_violation++;
}
- mv88e6xxx_reg_unlock(chip);
return IRQ_HANDLED;
-out:
+out_unlock:
mv88e6xxx_reg_unlock(chip);
+out:
dev_err(chip->dev, "ATU problem: error %d while handling interrupt\n",
err);
return IRQ_HANDLED;
--
2.34.1
On Wed, Jan 04, 2023 at 02:06:02PM +0100, Hans J. Schultz wrote:
> As functions called under the interrupt handler will need to take the
> netlink lock, we need to release the chip lock before calling those
> functions as otherwise double lock deadlocks will occur as userspace
> calls towards the driver often take the netlink lock and then the
> chip lock.
>
> The deadlock would look like:
>
> Interrupt handler: chip lock taken, but cannot take netlink lock as
> userspace config call has netlink lock.
> Userspace config: netlink lock taken, but cannot take chip lock as
> the interrupt handler has the chip lock.
Ultimately, none of this explanation is really relevant, and it requires
too much prior reviewer knowledge. I would phrase the commit title as
"shorten the locked section in mv88e6xxx_g1_atu_prob_irq_thread_fn()"
and say, as an explanation, that only the hardware access functions (up
until the call to mv88e6xxx_g1_atu_mac_read()) require the register lock.
The follow-up code, which processes the ATU violation data, can run
unlocked, and in a future patch will even run from a context which is
incompatible with the register lock being held. If you wish, you can
mention here as a small note that the incompatible context comes from an
AB/BA ordering inversion with rtnl_lock().
>
> Signed-off-by: Hans J. Schultz <[email protected]>
> ---
> drivers/net/dsa/mv88e6xxx/global1_atu.c | 14 ++++++++------
> 1 file changed, 8 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/net/dsa/mv88e6xxx/global1_atu.c b/drivers/net/dsa/mv88e6xxx/global1_atu.c
> index 61ae2d61e25c..34203e112eef 100644
> --- a/drivers/net/dsa/mv88e6xxx/global1_atu.c
> +++ b/drivers/net/dsa/mv88e6xxx/global1_atu.c
> @@ -409,11 +409,11 @@ static irqreturn_t mv88e6xxx_g1_atu_prob_irq_thread_fn(int irq, void *dev_id)
>
> err = mv88e6xxx_g1_read_atu_violation(chip);
> if (err)
> - goto out;
> + goto out_unlock;
>
> err = mv88e6xxx_g1_read(chip, MV88E6XXX_G1_ATU_OP, &val);
> if (err)
> - goto out;
> + goto out_unlock;
>
> err = mv88e6xxx_g1_atu_fid_read(chip, &fid);
If mv88e6xxx_g1_atu_fid_read() fails, it will goto out, which will exit
the IRQ handler with the mv88e6xxx_reg_lock() still held.
Probably not a good idea, since the driver will access the registers
again in the future (errors in IRQ handlers aren't propagated anywhere),
and the user might need a computer which is not deadlocked.
> if (err)
> @@ -421,11 +421,13 @@ static irqreturn_t mv88e6xxx_g1_atu_prob_irq_thread_fn(int irq, void *dev_id)
>
> err = mv88e6xxx_g1_atu_data_read(chip, &entry);
> if (err)
> - goto out;
> + goto out_unlock;
>
> err = mv88e6xxx_g1_atu_mac_read(chip, &entry);
> if (err)
> - goto out;
> + goto out_unlock;
> +
> + mv88e6xxx_reg_unlock(chip);
>
> spid = entry.state;
>
> @@ -449,13 +451,13 @@ static irqreturn_t mv88e6xxx_g1_atu_prob_irq_thread_fn(int irq, void *dev_id)
> fid);
> chip->ports[spid].atu_full_violation++;
> }
> - mv88e6xxx_reg_unlock(chip);
>
> return IRQ_HANDLED;
>
> -out:
> +out_unlock:
> mv88e6xxx_reg_unlock(chip);
>
> +out:
> dev_err(chip->dev, "ATU problem: error %d while handling interrupt\n",
> err);
> return IRQ_HANDLED;
> --
> 2.34.1
>