| Message ID | 20211123125042.2564114-2-jk@codeconstruct.com.au (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | mctp serial minor fixes | 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 5 of 5 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/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, 7 lines checked |
| netdev/kdoc | success | Errors and warnings before: 0 this patch: 0 |
| netdev/source_inline | success | Was 0 now: 0 |
On 23. 11. 21, 13:50, Jeremy Kerr wrote: > We want to ensure that the tx work has finished before returning from > the ldisc close op, so do a synchronous cancel. > > Reported-by: Jiri Slaby <jirislaby@kernel.org> > Signed-off-by: Jeremy Kerr <jk@codeconstruct.com.au> > --- > drivers/net/mctp/mctp-serial.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/drivers/net/mctp/mctp-serial.c b/drivers/net/mctp/mctp-serial.c > index 9ac0e187f36e..c958d773a82a 100644 > --- a/drivers/net/mctp/mctp-serial.c > +++ b/drivers/net/mctp/mctp-serial.c > @@ -478,6 +478,7 @@ static void mctp_serial_close(struct tty_struct *tty) > struct mctp_serial *dev = tty->disc_data; > int idx = dev->idx; > > + cancel_work_sync(&dev->tx_work); But the work still can be queued after the cancel (and before the unregister), right? > unregister_netdev(dev->netdev); > ida_free(&mctp_serial_ida, idx); > } > thanks,
On 24. 11. 21, 6:36, Jiri Slaby wrote: > On 23. 11. 21, 13:50, Jeremy Kerr wrote: >> We want to ensure that the tx work has finished before returning from >> the ldisc close op, so do a synchronous cancel. >> >> Reported-by: Jiri Slaby <jirislaby@kernel.org> >> Signed-off-by: Jeremy Kerr <jk@codeconstruct.com.au> >> --- >> drivers/net/mctp/mctp-serial.c | 1 + >> 1 file changed, 1 insertion(+) >> >> diff --git a/drivers/net/mctp/mctp-serial.c >> b/drivers/net/mctp/mctp-serial.c >> index 9ac0e187f36e..c958d773a82a 100644 >> --- a/drivers/net/mctp/mctp-serial.c >> +++ b/drivers/net/mctp/mctp-serial.c >> @@ -478,6 +478,7 @@ static void mctp_serial_close(struct tty_struct *tty) >> struct mctp_serial *dev = tty->disc_data; >> int idx = dev->idx; >> + cancel_work_sync(&dev->tx_work); > > But the work still can be queued after the cancel (and before the > unregister), right? Maybe do it in ->ndo_uninit()? >> unregister_netdev(dev->netdev); >> ida_free(&mctp_serial_ida, idx); >> } >> > > thanks,
Hi Jiri, > > + cancel_work_sync(&dev->tx_work); > > But the work still can be queued after the cancel (and before the > unregister), right? Yes. Yes it can. I should be cancelling after the unregister, not before, so this'll need a v2. On the ldisc side: is there any case where we'd get a write wakeup during (or after) the ->close()? Cheers, Jeremy
Hi, On 24. 11. 21, 7:38, Jeremy Kerr wrote: > On the ldisc side: is there any case where we'd get a write wakeup > during (or after) the ->close()? there should be no invocation of ldisc after close(). If there is, it's a bug as this is even documented: * @close: [TTY] ``void ()(struct tty_struct *tty)`` * * This function is called when the line discipline is being shutdown, * either because the @tty is being closed or because the @tty is being * changed to use a new line discipline. At the point of execution no * further users will enter the ldisc code for this tty. * * Can sleep. Should be the same also for the "during" case. regards,
Hi Jiri, > there should be no invocation of ldisc after close(). If there is, > it's a bug as this is even documented: Excellent thanks for that (and the doc pointer), I'll get a v2 done now. Cheers, Jeremy
diff --git a/drivers/net/mctp/mctp-serial.c b/drivers/net/mctp/mctp-serial.c index 9ac0e187f36e..c958d773a82a 100644 --- a/drivers/net/mctp/mctp-serial.c +++ b/drivers/net/mctp/mctp-serial.c @@ -478,6 +478,7 @@ static void mctp_serial_close(struct tty_struct *tty) struct mctp_serial *dev = tty->disc_data; int idx = dev->idx; + cancel_work_sync(&dev->tx_work); unregister_netdev(dev->netdev); ida_free(&mctp_serial_ida, idx); }
We want to ensure that the tx work has finished before returning from the ldisc close op, so do a synchronous cancel. Reported-by: Jiri Slaby <jirislaby@kernel.org> Signed-off-by: Jeremy Kerr <jk@codeconstruct.com.au> --- drivers/net/mctp/mctp-serial.c | 1 + 1 file changed, 1 insertion(+)