| Message ID | 20240807-v6-10-topic-usb-serial-serdev-v1-0-ed2cc5da591f@pengutronix.de |
|---|---|
| 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 > >
Hi Johan, On 24-09-09, Johan Hovold wrote: > On Wed, Aug 07, 2024 at 04:08:47PM +0200, Marco Felsch wrote: > > this patchset is based on Johan's patches [1] but dropped the need of > > the special 'serial' of-node [2]. > > That's great that you found and referenced my proof-of-concept patches, > but it doesn't seem like you tried to understand why this hasn't been > merged yet. I'm glad for your input. > First, as the commit message you refer to below explain, we need some > way to describe multiport controllers. Just dropping the 'serial' node > does not make that issue go away. Sorry for asking but isn't the current OF abstraction [1] enough? As far as I understood we can describe the whole USB tree within OF. I used [1] and the this patchset to describe the following hierarchy: usb-root -> usb-hub port-1 -> usb-serial interface-0 -> serial bt-module [1] Documentation/devicetree/bindings/usb/usb-device.yaml > Second, and more importantly, you do not address the main obstacle for > enabling serdev for USB serial which is that the serdev cannot handle > hotplugging. Hotplugging is a good point but out-of-scope IMHO (at least for now) since the current serdev implementation rely on additional firmware information e.g OF node to be present. E.g. if the above mentioned setup would connect the "serial bt-module" directly to the UART port you still need an OF node to bind the serdev driver. If the node isn't present user-space would need to do the hci handling. So from my POV the serdev abstraction is for manufacturers which make use of "onboard" usb-devices which are always present at the same USB tree location. Serdev is not made for general purpose USB ports (yet) where a user can plug-in all types of USB devices. Regards, Marco > > [1] https://git.kernel.org/pub/scm/linux/kernel/git/johan/usb-serial.git/log/?h=usb-serial-of > > [2] https://git.kernel.org/pub/scm/linux/kernel/git/johan/usb-serial.git/commit/?h=usb-serial-of&id=b19239022c92567a6a9ed40e8522e84972b0997f > > Johan >
Hi, gentle ping as this is series is two months old now. Regards, Marco On 24-08-07, Marco Felsch wrote: > Hi, > > this patchset is based on Johan's patches [1] but dropped the need of > the special 'serial' of-node [2]. > > With the patches in place and the usb hierarchy described in properly we > can use serdev on-top of usb-serial. The below example adds the support > for the following hierarchy: > - host->usb-hub->ftdi-usb-uart->bt/wlan-module: > > &usb_dwc3_1 { > dr_mode = "host"; > status = "okay"; > > hub@1 { > compatible = "usb424,2514"; > reg = <1>; > > vdd-supply = <®>; > reset-gpios = <&gpio4 5 GPIO_ACTIVE_LOW>; > > #address-cells = <1>; > #size-cells = <0>; > > device@1 { > compatible = "usb403,6010"; > reg = <1>; > > #address-cells = <2>; > #size-cells = <0>; > > interface@0 { > compatible = "usbif403,6010.config1.0"; > reg = <0 1>; > > #address-cells = <1>; > #size-cells = <0>; > > bluetooth { > compatible = "nxp,88w8987-bt"; > fw-init-baudrate = <3000000>; > }; > }; > }; > }; > }; > > If no serdev node is found the usb-serial is exposed as usual and can be > accessed via /dev/ttyUSBx. > > Regards, > Marco > > [1] https://git.kernel.org/pub/scm/linux/kernel/git/johan/usb-serial.git/log/?h=usb-serial-of > [2] https://git.kernel.org/pub/scm/linux/kernel/git/johan/usb-serial.git/commit/?h=usb-serial-of&id=b19239022c92567a6a9ed40e8522e84972b0997f > > Signed-off-by: Marco Felsch <m.felsch@pengutronix.de> > --- > Marco Felsch (3): > serdev: ttyport: make use of tty_kopen_exclusive > USB: serial: cosmetic cleanup <space><tab> mix > USB: serial: enable serdev support > > drivers/tty/serdev/serdev-ttyport.c | 9 ++++++--- > drivers/usb/serial/bus.c | 10 ++++++---- > 2 files changed, 12 insertions(+), 7 deletions(-) > --- > base-commit: 0c3836482481200ead7b416ca80c68a29cfdaabd > change-id: 20240807-v6-10-topic-usb-serial-serdev-83a7f8f86432 > > Best regards, > -- > Marco Felsch <m.felsch@pengutronix.de> > >
On Tue, Oct 01, 2024 at 09:24:53AM +0200, Marco Felsch wrote: > Hi, > > gentle ping as this is series is two months old now. And it was rejected as serdev does not support hotplug which of course, usb-serial does. So until serdev is fixed up to handle that correctly, this is not going anywhere, nor should you want it to as then you would be in charge of code that does not work properly :) thanks, greg k-h
On 24-10-01, Greg Kroah-Hartman wrote: > On Tue, Oct 01, 2024 at 09:24:53AM +0200, Marco Felsch wrote: > > Hi, > > > > gentle ping as this is series is two months old now. > > And it was rejected as serdev does not support hotplug which of course, > usb-serial does. I just read concerns which I tried to explain/argue but didn't saw it as rejected. > So until serdev is fixed up to handle that correctly, this is not going > anywhere, nor should you want it to as then you would be in charge of > code that does not work properly :) Sure as always :) Regards, Marco
Hi Greg, On 24-10-01, Greg Kroah-Hartman wrote: > On Tue, Oct 01, 2024 at 09:24:53AM +0200, Marco Felsch wrote: > > Hi, > > > > gentle ping as this is series is two months old now. > > And it was rejected as serdev does not support hotplug which of course, > usb-serial does. I hoped to get some feedback on my answer [1]. Regarding hotplug support: serdev _requires_ some sort of firmware like OF (not sure if it does work with ACPI too). That said, if serdev finds no firmware a fallback is provided to the standard serial handling. The firmware could either be added directly by the platform OF file or via OF-overlays. By making use of overlays we could gain some kind of hotplug: Once a usb devices was detected and the driver has an overlay, the overlay gets applied and the probe continues, like we do it for PCIe devices now [2]. For devices which don't have a registered overlay the standard usb-serial setup is done by exposing the serial interface to the userspace. > So until serdev is fixed up to handle that correctly, this is not going > anywhere, nor should you want it to as then you would be in charge of > code that does not work properly :) Regards, Marco [1] https://lore.kernel.org/all/20240917044948.i2eog4ondf7vna7q@pengutronix.de/ [2] https://lore.kernel.org/all/7512cbb7911b8395d926e9e9e390fbb55ce3aea9.camel@pengutronix.de/ > > thanks, > > greg k-h >
Hi, this patchset is based on Johan's patches [1] but dropped the need of the special 'serial' of-node [2]. With the patches in place and the usb hierarchy described in properly we can use serdev on-top of usb-serial. The below example adds the support for the following hierarchy: - host->usb-hub->ftdi-usb-uart->bt/wlan-module: &usb_dwc3_1 { dr_mode = "host"; status = "okay"; hub@1 { compatible = "usb424,2514"; reg = <1>; vdd-supply = <®>; reset-gpios = <&gpio4 5 GPIO_ACTIVE_LOW>; #address-cells = <1>; #size-cells = <0>; device@1 { compatible = "usb403,6010"; reg = <1>; #address-cells = <2>; #size-cells = <0>; interface@0 { compatible = "usbif403,6010.config1.0"; reg = <0 1>; #address-cells = <1>; #size-cells = <0>; bluetooth { compatible = "nxp,88w8987-bt"; fw-init-baudrate = <3000000>; }; }; }; }; }; If no serdev node is found the usb-serial is exposed as usual and can be accessed via /dev/ttyUSBx. Regards, Marco [1] https://git.kernel.org/pub/scm/linux/kernel/git/johan/usb-serial.git/log/?h=usb-serial-of [2] https://git.kernel.org/pub/scm/linux/kernel/git/johan/usb-serial.git/commit/?h=usb-serial-of&id=b19239022c92567a6a9ed40e8522e84972b0997f Signed-off-by: Marco Felsch <m.felsch@pengutronix.de> --- Marco Felsch (3): serdev: ttyport: make use of tty_kopen_exclusive USB: serial: cosmetic cleanup <space><tab> mix USB: serial: enable serdev support drivers/tty/serdev/serdev-ttyport.c | 9 ++++++--- drivers/usb/serial/bus.c | 10 ++++++---- 2 files changed, 12 insertions(+), 7 deletions(-) --- base-commit: 0c3836482481200ead7b416ca80c68a29cfdaabd change-id: 20240807-v6-10-topic-usb-serial-serdev-83a7f8f86432 Best regards,