Hello Jiri, On 12/14/21 7:43 AM, Jiri Slaby wrote: > Hi, > > On 13. 12. 21, 20:53, Arnaud Pouliquen wrote: >> In current implementation the tty_port struct is part of the >> rpmsg_tty_port structure.The issue is that the rpmsg_tty_port structure is >> freed on rpmsg_tty_remove but also referenced in the tty_struct. >> Its release is not predictable due to workqueues. >> >> For instance following ftrace shows that rpmsg_tty_close is called after >> rpmsg_tty_release_cport: >> >> nr_test.sh-389 [000] ..... 212.093752: rpmsg_tty_remove <-rpmsg_dev_ >> remove >> cat-1191 [001] ..... 212.095697: tty_release <-__fput >> nr_test.sh-389 [000] ..... 212.099166: rpmsg_tty_release_cport <-rpm >> sg_tty_remove >> cat-1191 [001] ..... 212.115352: rpmsg_tty_close <-tty_release >> cat-1191 [001] ..... 212.115371: release_tty <-tty_release_str >> >> As consequence, the port must be free only when user has released the TTY >> interface. >> >> This path (inspired from vcc.c): >> - moves the management of the port in the install and clean-up tty ops, >> - allocates the tty_port struct independently of the rpmsg_tty_port structure, > > This looks rather wrong. Why not to use tty_port refcounting? Please could you detail what seems rather wrong for you? Everything or do the tty_port port struct independently of the rpmsg_tty_port structure? Concerning the tty_port refcounting: Yes it also an option that I have already tried without success, before implementing this patch. That said, as you pointed it out, I reimplemented it today in another way, and this time it seems that it works without any runtime warning or error. I need to perform more test to confirm, then I will propose a V2 based on tty_port refcountingt and the .destruct tty_port_operations. > >> - uses tty_vhangup and tty_port_hangup. > > OK, but don't store a tty pointer as it looks racy. You should use > tty_port_tty_get instead. > > Hm, we look we need tty_port_tty_vhangup (aside from tty_port_tty_hangup). There > are plenty of drivers doing: > tty = tty_port_tty_get(port); > if (tty) { > tty_vhangup(port->tty); > tty_kref_put(tty); I would like to first fix the issue in rpmsg_tty.c in separate thread. But yes this should not take me too much time to propose this helper next. Thanks, Arnaud > > >> Fixes: 7c0408d80579 ("tty: add rpmsg driver") >> Signed-off-by: Arnaud Pouliquen <arnaud.pouliquen@xxxxxxxxxxx> > > thanks,