| Message ID | 20240807-v6-10-topic-usb-serial-serdev-v1-1-ed2cc5da591f@pengutronix.de |
|---|---|
| State | New |
| Headers | show |
| Series | USB-Serial serdev support | expand |
On 07. 08. 24, 16:08, Marco Felsch wrote: > The purpose of serdev is to provide kernel drivers for particular serial > device, serdev-ttyport is no exception here. Make use of the > tty_kopen_exclusive() funciton to mark this tty device as kernel > internal device. > > Signed-off-by: Marco Felsch <m.felsch@pengutronix.de> > --- > drivers/tty/serdev/serdev-ttyport.c | 9 ++++++--- > 1 file changed, 6 insertions(+), 3 deletions(-) > > diff --git a/drivers/tty/serdev/serdev-ttyport.c b/drivers/tty/serdev/serdev-ttyport.c > index 3d7ae7fa5018..94c43d25ddbe 100644 > --- a/drivers/tty/serdev/serdev-ttyport.c > +++ b/drivers/tty/serdev/serdev-ttyport.c > @@ -103,11 +103,14 @@ static int ttyport_write_room(struct serdev_controller *ctrl) > static int ttyport_open(struct serdev_controller *ctrl) > { > struct serport *serport = serdev_controller_get_drvdata(ctrl); > + struct tty_driver *tty_drv = serport->tty_drv; > struct tty_struct *tty; > struct ktermios ktermios; > + dev_t dev; > int ret; > > - tty = tty_init_dev(serport->tty_drv, serport->tty_idx); > + dev = MKDEV(tty_drv->major, tty_drv->minor_start + serport->tty_idx); > + tty = tty_kopen_exclusive(dev); I believe that the now added tty_lookup_driver() has negligible impact in this anyway slow path, right? thanks,
Hi, sorry for not replying earlier. On 24-08-08, Jiri Slaby wrote: > On 07. 08. 24, 16:08, Marco Felsch wrote: > > The purpose of serdev is to provide kernel drivers for particular serial > > device, serdev-ttyport is no exception here. Make use of the > > tty_kopen_exclusive() funciton to mark this tty device as kernel > > internal device. > > > > Signed-off-by: Marco Felsch <m.felsch@pengutronix.de> > > --- > > drivers/tty/serdev/serdev-ttyport.c | 9 ++++++--- > > 1 file changed, 6 insertions(+), 3 deletions(-) > > > > diff --git a/drivers/tty/serdev/serdev-ttyport.c b/drivers/tty/serdev/serdev-ttyport.c > > index 3d7ae7fa5018..94c43d25ddbe 100644 > > --- a/drivers/tty/serdev/serdev-ttyport.c > > +++ b/drivers/tty/serdev/serdev-ttyport.c > > @@ -103,11 +103,14 @@ static int ttyport_write_room(struct serdev_controller *ctrl) > > static int ttyport_open(struct serdev_controller *ctrl) > > { > > struct serport *serport = serdev_controller_get_drvdata(ctrl); > > + struct tty_driver *tty_drv = serport->tty_drv; > > struct tty_struct *tty; > > struct ktermios ktermios; > > + dev_t dev; > > int ret; > > - tty = tty_init_dev(serport->tty_drv, serport->tty_idx); > > + dev = MKDEV(tty_drv->major, tty_drv->minor_start + serport->tty_idx); > > + tty = tty_kopen_exclusive(dev); > > I believe that the now added tty_lookup_driver() has negligible impact in > this anyway slow path, right? Can you please elaborate a bit more? I don't see how the tty_lookup_driver() is involved in the serdev-ctrl open path anyway. Regards, Marco
On 19. 08. 24, 12:19, Marco Felsch wrote: > Hi, > > sorry for not replying earlier. > > On 24-08-08, Jiri Slaby wrote: >> On 07. 08. 24, 16:08, Marco Felsch wrote: >>> The purpose of serdev is to provide kernel drivers for particular serial >>> device, serdev-ttyport is no exception here. Make use of the >>> tty_kopen_exclusive() funciton to mark this tty device as kernel >>> internal device. >>> >>> Signed-off-by: Marco Felsch <m.felsch@pengutronix.de> >>> --- >>> drivers/tty/serdev/serdev-ttyport.c | 9 ++++++--- >>> 1 file changed, 6 insertions(+), 3 deletions(-) >>> >>> diff --git a/drivers/tty/serdev/serdev-ttyport.c b/drivers/tty/serdev/serdev-ttyport.c >>> index 3d7ae7fa5018..94c43d25ddbe 100644 >>> --- a/drivers/tty/serdev/serdev-ttyport.c >>> +++ b/drivers/tty/serdev/serdev-ttyport.c >>> @@ -103,11 +103,14 @@ static int ttyport_write_room(struct serdev_controller *ctrl) >>> static int ttyport_open(struct serdev_controller *ctrl) >>> { >>> struct serport *serport = serdev_controller_get_drvdata(ctrl); >>> + struct tty_driver *tty_drv = serport->tty_drv; >>> struct tty_struct *tty; >>> struct ktermios ktermios; >>> + dev_t dev; >>> int ret; >>> - tty = tty_init_dev(serport->tty_drv, serport->tty_idx); >>> + dev = MKDEV(tty_drv->major, tty_drv->minor_start + serport->tty_idx); >>> + tty = tty_kopen_exclusive(dev); >> >> I believe that the now added tty_lookup_driver() has negligible impact in >> this anyway slow path, right? > > Can you please elaborate a bit more? I don't see how the > tty_lookup_driver() is involved in the serdev-ctrl open path anyway. It's called now in of tty_kopen_exclusive()->tty_kopen(). (tty_lookup_driver() is the major difference between the raw tty_init_dev() and tty_kopen_exclusive().)
On 24-08-19, Jiri Slaby wrote: > On 19. 08. 24, 12:19, Marco Felsch wrote: > > Hi, > > > > sorry for not replying earlier. > > > > On 24-08-08, Jiri Slaby wrote: > > > On 07. 08. 24, 16:08, Marco Felsch wrote: > > > > The purpose of serdev is to provide kernel drivers for particular serial > > > > device, serdev-ttyport is no exception here. Make use of the > > > > tty_kopen_exclusive() funciton to mark this tty device as kernel > > > > internal device. > > > > > > > > Signed-off-by: Marco Felsch <m.felsch@pengutronix.de> > > > > --- > > > > drivers/tty/serdev/serdev-ttyport.c | 9 ++++++--- > > > > 1 file changed, 6 insertions(+), 3 deletions(-) > > > > > > > > diff --git a/drivers/tty/serdev/serdev-ttyport.c b/drivers/tty/serdev/serdev-ttyport.c > > > > index 3d7ae7fa5018..94c43d25ddbe 100644 > > > > --- a/drivers/tty/serdev/serdev-ttyport.c > > > > +++ b/drivers/tty/serdev/serdev-ttyport.c > > > > @@ -103,11 +103,14 @@ static int ttyport_write_room(struct serdev_controller *ctrl) > > > > static int ttyport_open(struct serdev_controller *ctrl) > > > > { > > > > struct serport *serport = serdev_controller_get_drvdata(ctrl); > > > > + struct tty_driver *tty_drv = serport->tty_drv; > > > > struct tty_struct *tty; > > > > struct ktermios ktermios; > > > > + dev_t dev; > > > > int ret; > > > > - tty = tty_init_dev(serport->tty_drv, serport->tty_idx); > > > > + dev = MKDEV(tty_drv->major, tty_drv->minor_start + serport->tty_idx); > > > > + tty = tty_kopen_exclusive(dev); > > > > > > I believe that the now added tty_lookup_driver() has negligible impact in > > > this anyway slow path, right? > > > > Can you please elaborate a bit more? I don't see how the > > tty_lookup_driver() is involved in the serdev-ctrl open path anyway. > > It's called now in of tty_kopen_exclusive()->tty_kopen(). > (tty_lookup_driver() is the major difference between the raw tty_init_dev() > and tty_kopen_exclusive().) Okay now I get the "now added tty_lookup_driver()" statement, sorry. Yes, I believe that this is negligible. The main difference for me was that the tty_port_set_kopened() is set accordingly which which is important to not trigger warnings during the release path. Regards, Marco > > -- > js > suse labs > >
On 24-08-08, Jiri Slaby wrote: > On 07. 08. 24, 16:08, Marco Felsch wrote: > > The purpose of serdev is to provide kernel drivers for particular serial > > device, serdev-ttyport is no exception here. Make use of the > > tty_kopen_exclusive() funciton to mark this tty device as kernel > > internal device. > > > > Signed-off-by: Marco Felsch <m.felsch@pengutronix.de> > > --- > > drivers/tty/serdev/serdev-ttyport.c | 9 ++++++--- > > 1 file changed, 6 insertions(+), 3 deletions(-) > > > > diff --git a/drivers/tty/serdev/serdev-ttyport.c b/drivers/tty/serdev/serdev-ttyport.c > > index 3d7ae7fa5018..94c43d25ddbe 100644 > > --- a/drivers/tty/serdev/serdev-ttyport.c > > +++ b/drivers/tty/serdev/serdev-ttyport.c > > @@ -103,11 +103,14 @@ static int ttyport_write_room(struct serdev_controller *ctrl) > > static int ttyport_open(struct serdev_controller *ctrl) > > { > > struct serport *serport = serdev_controller_get_drvdata(ctrl); > > + struct tty_driver *tty_drv = serport->tty_drv; > > struct tty_struct *tty; > > struct ktermios ktermios; > > + dev_t dev; > > int ret; > > - tty = tty_init_dev(serport->tty_drv, serport->tty_idx); > > + dev = MKDEV(tty_drv->major, tty_drv->minor_start + serport->tty_idx); > > + tty = tty_kopen_exclusive(dev); > > I believe that the now added tty_lookup_driver() has negligible impact in > this anyway slow path, right? Any other comments, suggestions apart this one? Regards, Marco > > thanks, > -- > js > suse labs > >
diff --git a/drivers/tty/serdev/serdev-ttyport.c b/drivers/tty/serdev/serdev-ttyport.c index 3d7ae7fa5018..94c43d25ddbe 100644 --- a/drivers/tty/serdev/serdev-ttyport.c +++ b/drivers/tty/serdev/serdev-ttyport.c @@ -103,11 +103,14 @@ static int ttyport_write_room(struct serdev_controller *ctrl) static int ttyport_open(struct serdev_controller *ctrl) { struct serport *serport = serdev_controller_get_drvdata(ctrl); + struct tty_driver *tty_drv = serport->tty_drv; struct tty_struct *tty; struct ktermios ktermios; + dev_t dev; int ret; - tty = tty_init_dev(serport->tty_drv, serport->tty_idx); + dev = MKDEV(tty_drv->major, tty_drv->minor_start + serport->tty_idx); + tty = tty_kopen_exclusive(dev); if (IS_ERR(tty)) return PTR_ERR(tty); serport->tty = tty; @@ -144,7 +147,7 @@ static int ttyport_open(struct serdev_controller *ctrl) tty->ops->close(tty, NULL); err_unlock: tty_unlock(tty); - tty_release_struct(tty, serport->tty_idx); + tty_kclose(tty); return ret; } @@ -161,7 +164,7 @@ static void ttyport_close(struct serdev_controller *ctrl) tty->ops->close(tty, NULL); tty_unlock(tty); - tty_release_struct(tty, serport->tty_idx); + tty_kclose(tty); } static unsigned int ttyport_set_baudrate(struct serdev_controller *ctrl, unsigned int speed)
The purpose of serdev is to provide kernel drivers for particular serial device, serdev-ttyport is no exception here. Make use of the tty_kopen_exclusive() funciton to mark this tty device as kernel internal device. Signed-off-by: Marco Felsch <m.felsch@pengutronix.de> --- drivers/tty/serdev/serdev-ttyport.c | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-)