| Message ID | 20221213174650.670767-3-netdev@kapio-technology.com (mailing list archive) |
|---|---|
| State | Deferred |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | mv88e6xxx: Add MAB offload support | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Clearly marked for net-next |
| netdev/fixes_present | success | Fixes tag not required for -next series |
| netdev/subject_prefix | success | Link |
| netdev/cover_letter | success | Series has a cover letter |
| netdev/patch_count | success | Link |
| netdev/header_inline | success | No static functions without inline keyword in header files |
| netdev/build_32bit | success | Errors and warnings before: 0 this patch: 0 |
| netdev/cc_maintainers | success | CCed 8 of 8 maintainers |
| netdev/build_clang | success | Errors and warnings before: 0 this patch: 0 |
| netdev/module_param | success | Was 0 now: 0 |
| netdev/verify_signedoff | success | Signed-off-by tag matches author and committer |
| netdev/check_selftest | success | No net selftest shell script |
| netdev/verify_fixes | success | No Fixes tag |
| netdev/build_allmodconfig_warn | success | Errors and warnings before: 0 this patch: 0 |
| netdev/checkpatch | success | total: 0 errors, 0 warnings, 0 checks, 43 lines checked |
| netdev/kdoc | success | Errors and warnings before: 0 this patch: 0 |
| netdev/source_inline | success | Was 0 now: 0 |
On Tue, 2022-12-13 at 18:46 +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. > > Signed-off-by: Hans J. Schultz <netdev@kapio-technology.com> Just to confirm, in order to see the deadlocks I would imagine you need something in the tracepoints that is taking the netlink lock? If so you might want to reference the commit that switched out the dev_err_ratelimited for the tracepoints 8646384d80f3 ("net: dsa: mv88e6xxx: replace ATU violation prints with trace points") so that this patch can be found an applied to any kernels that pull in those tracepoints. > --- > 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; Reviewed-by: Alexander Duyck <alexanderduyck@fb.com>
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;
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 <netdev@kapio-technology.com> --- drivers/net/dsa/mv88e6xxx/global1_atu.c | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-)