question on correct error return from break_ctl()

question on correct error return from break_ctl()

[Oliver Neukum]

Hi,

it seems inconsistent. The tty layer
in drivers/tty/tty_io.c::send_break()

static int send_break(struct tty_struct *tty, unsigned int duration)
{
         int retval;

         if (tty->ops->break_ctl == NULL)
                 return 0;

not supporting break_ctl() is treated as the operation
succeeding. Yet in drivers/usb/serial/usb-serial.c::serial_break()

static int serial_break(struct tty_struct *tty, int break_state)
{
         struct usb_serial_port *port = tty->driver_data;

         dev_dbg(&port->dev, "%s\n", __func__);

         if (port->serial->type->break_ctl)
                 return port->serial->type->break_ctl(tty, break_state);

         return -ENOTTY;
}

we are seeing that not supporting break_ctl() leads to returning
-ENOTTY, which drivers/tty/tty_io.c::send_break() will return to user space.
These reactions are at odds with each other. What is a driver supposed
to do?

	Regards
		Oliver

Re: question on correct error return from break_ctl()

[Greg Kroah-Hartman]

On Thu, Nov 30, 2023 at 02:09:48PM +0100, Oliver Neukum wrote:
> Hi,
> 
> it seems inconsistent. The tty layer
> in drivers/tty/tty_io.c::send_break()
> 
> static int send_break(struct tty_struct *tty, unsigned int duration)
> {
>         int retval;
> 
>         if (tty->ops->break_ctl == NULL)
>                 return 0;
> 
> not supporting break_ctl() is treated as the operation
> succeeding. Yet in drivers/usb/serial/usb-serial.c::serial_break()
> 
> static int serial_break(struct tty_struct *tty, int break_state)
> {
>         struct usb_serial_port *port = tty->driver_data;
> 
>         dev_dbg(&port->dev, "%s\n", __func__);
> 
>         if (port->serial->type->break_ctl)
>                 return port->serial->type->break_ctl(tty, break_state);
> 
>         return -ENOTTY;
> }
> 
> we are seeing that not supporting break_ctl() leads to returning
> -ENOTTY, which drivers/tty/tty_io.c::send_break() will return to user space.
> These reactions are at odds with each other. What is a driver supposed
> to do?

usb-serial should probably change, but given the fact that no one has
noticed this in the 20+ years it has been like this, is it really
needed?  :)

thanks,

greg k-h

Re: question on correct error return from break_ctl()

[Oliver Neukum]

On 30.11.23 14:48, Greg Kroah-Hartman wrote:
Hi,

> usb-serial should probably change, but given the fact that no one has
> noticed this in the 20+ years it has been like this, is it really
> needed?  :)

I am afraid I need to point out that usb-serial has been changed _this_ _year_
to return -ENOTTY. CDC-ACM being also in this situation unfortunately
I need to decide between one of the alternatives.

	Regards
		Oliver

Re: question on correct error return from break_ctl()

[Greg Kroah-Hartman]

On Thu, Nov 30, 2023 at 03:36:21PM +0100, Oliver Neukum wrote:
> On 30.11.23 14:48, Greg Kroah-Hartman wrote:
> Hi,
> 
> > usb-serial should probably change, but given the fact that no one has
> > noticed this in the 20+ years it has been like this, is it really
> > needed?  :)
> 
> I am afraid I need to point out that usb-serial has been changed _this_ _year_
> to return -ENOTTY. CDC-ACM being also in this situation unfortunately
> I need to decide between one of the alternatives.

Ah, oops, then it should probably be changed back.  Unless Johan, any
specific reason this was changed?

thanks,

greg k-h

Re: question on correct error return from break_ctl()

[Johan Hovold]

On Thu, Nov 30, 2023 at 02:42:20PM +0000, Greg Kroah-Hartman wrote:
> On Thu, Nov 30, 2023 at 03:36:21PM +0100, Oliver Neukum wrote:
> > On 30.11.23 14:48, Greg Kroah-Hartman wrote:

> > I am afraid I need to point out that usb-serial has been changed _this_ _year_
> > to return -ENOTTY. CDC-ACM being also in this situation unfortunately
> > I need to decide between one of the alternatives.
> 
> Ah, oops, then it should probably be changed back.  Unless Johan, any
> specific reason this was changed?

Had to go back to lore to see why I changed this:

	https://lore.kernel.org/all/20230604123505.4661-1-johan@kernel.org/

IIRC we had a user that was not expecting break signalling to only work
with one of the two ports of a Silabs device without user space being
notified about it.

In the cover letter I mention that the intent of "commit 9e98966c7bb9
("tty: rework break handling") from 2008 appears to be to allow missing
support to be reported to user space".

Reporting back an error also avoids waiting for the break period when a
subdriver does not support break (i.e. as when a tty driver does not
support break).

The inconsistency was discussed and if this turns out to be an issue we
can have the tty layer turn that -ENOTTY into 0 before returning:

	https://lore.kernel.org/all/ZH8a12ZYtA2RzEK_@hovoldconsulting.com/

I haven't heard from anyone noticing any issues so far though.

Johan