tty_set_termios() should be called with slave side of pty driver. So, If
tty driver is pty master, it needs to be switched to ->link.
Reported-by: [email protected]
Signed-off-by: Myungho Jung <[email protected]>
---
drivers/bluetooth/hci_ldisc.c | 20 +++++++++++++++-----
1 file changed, 15 insertions(+), 5 deletions(-)
diff --git a/drivers/bluetooth/hci_ldisc.c b/drivers/bluetooth/hci_ldisc.c
index fbf7b4df23ab..90c5ea8c399b 100644
--- a/drivers/bluetooth/hci_ldisc.c
+++ b/drivers/bluetooth/hci_ldisc.c
@@ -299,10 +299,18 @@ static int hci_uart_send_frame(struct hci_dev *hdev, struct sk_buff *skb)
return 0;
}
+/* If driver is pty master, return slave side */
+static struct tty_struct *hci_uart_get_real_tty(struct tty_struct *tty)
+{
+ return (tty->driver->type == TTY_DRIVER_TYPE_PTY &&
+ tty->driver->subtype == PTY_TYPE_MASTER) ? tty->link : tty;
+}
+
/* Flow control or un-flow control the device */
void hci_uart_set_flow_control(struct hci_uart *hu, bool enable)
{
struct tty_struct *tty = hu->tty;
+ struct tty_struct *real_tty;
struct ktermios ktermios;
int status;
unsigned int set = 0;
@@ -314,11 +322,12 @@ void hci_uart_set_flow_control(struct hci_uart *hu, bool enable)
return;
}
+ real_tty = hci_uart_get_real_tty(tty);
if (enable) {
/* Disable hardware flow control */
- ktermios = tty->termios;
+ ktermios = real_tty->termios;
ktermios.c_cflag &= ~CRTSCTS;
- status = tty_set_termios(tty, &ktermios);
+ status = tty_set_termios(real_tty, &ktermios);
BT_DBG("Disabling hardware flow control: %s",
status ? "failed" : "success");
@@ -350,9 +359,9 @@ void hci_uart_set_flow_control(struct hci_uart *hu, bool enable)
BT_DBG("Setting RTS: %s", status ? "failed" : "success");
/* Re-enable hardware flow control */
- ktermios = tty->termios;
+ ktermios = real_tty->termios;
ktermios.c_cflag |= CRTSCTS;
- status = tty_set_termios(tty, &ktermios);
+ status = tty_set_termios(real_tty, &ktermios);
BT_DBG("Enabling hardware flow control: %s",
status ? "failed" : "success");
}
@@ -367,9 +376,10 @@ void hci_uart_set_speeds(struct hci_uart *hu, unsigned int init_speed,
void hci_uart_set_baudrate(struct hci_uart *hu, unsigned int speed)
{
- struct tty_struct *tty = hu->tty;
+ struct tty_struct *tty;
struct ktermios ktermios;
+ tty = hci_uart_get_real_tty(hu->tty);
ktermios = tty->termios;
ktermios.c_cflag &= ~CBAUD;
tty_termios_encode_baud_rate(&ktermios, speed, speed);
--
2.17.1
Hi Myungho,
> tty_set_termios() should be called with slave side of pty driver. So, If
> tty driver is pty master, it needs to be switched to ->link.
>
> Reported-by: [email protected]
> Signed-off-by: Myungho Jung <[email protected]>
> ---
> drivers/bluetooth/hci_ldisc.c | 20 +++++++++++++++-----
> 1 file changed, 15 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/bluetooth/hci_ldisc.c b/drivers/bluetooth/hci_ldisc.c
> index fbf7b4df23ab..90c5ea8c399b 100644
> --- a/drivers/bluetooth/hci_ldisc.c
> +++ b/drivers/bluetooth/hci_ldisc.c
> @@ -299,10 +299,18 @@ static int hci_uart_send_frame(struct hci_dev *hdev, struct sk_buff *skb)
> return 0;
> }
>
> +/* If driver is pty master, return slave side */
> +static struct tty_struct *hci_uart_get_real_tty(struct tty_struct *tty)
> +{
> + return (tty->driver->type == TTY_DRIVER_TYPE_PTY &&
> + tty->driver->subtype == PTY_TYPE_MASTER) ? tty->link : tty;
> +}
> +
if (tty->driver->type == TTY_DRIVER_PTY && tty->driver->subtype = PTY_TYPE_MASTER)
return tty->link;
return tty;
We can spare the extra two lines to make this readable.
> /* Flow control or un-flow control the device */
> void hci_uart_set_flow_control(struct hci_uart *hu, bool enable)
> {
> struct tty_struct *tty = hu->tty;
> + struct tty_struct *real_tty;
> struct ktermios ktermios;
> int status;
> unsigned int set = 0;
> @@ -314,11 +322,12 @@ void hci_uart_set_flow_control(struct hci_uart *hu, bool enable)
> return;
> }
>
> + real_tty = hci_uart_get_real_tty(tty);
I would add a comment above this call why this is done and then also add an empty line after it. The if clause is not related to that call at all.
> if (enable) {
> /* Disable hardware flow control */
> - ktermios = tty->termios;
> + ktermios = real_tty->termios;
> ktermios.c_cflag &= ~CRTSCTS;
> - status = tty_set_termios(tty, &ktermios);
> + status = tty_set_termios(real_tty, &ktermios);
> BT_DBG("Disabling hardware flow control: %s",
> status ? "failed" : "success");
>
> @@ -350,9 +359,9 @@ void hci_uart_set_flow_control(struct hci_uart *hu, bool enable)
> BT_DBG("Setting RTS: %s", status ? "failed" : "success");
>
> /* Re-enable hardware flow control */
> - ktermios = tty->termios;
> + ktermios = real_tty->termios;
> ktermios.c_cflag |= CRTSCTS;
> - status = tty_set_termios(tty, &ktermios);
> + status = tty_set_termios(real_tty, &ktermios);
> BT_DBG("Enabling hardware flow control: %s",
> status ? "failed" : "success");
> }
> @@ -367,9 +376,10 @@ void hci_uart_set_speeds(struct hci_uart *hu, unsigned int init_speed,
>
> void hci_uart_set_baudrate(struct hci_uart *hu, unsigned int speed)
> {
> - struct tty_struct *tty = hu->tty;
> + struct tty_struct *tty;
> struct ktermios ktermios;
>
> + tty = hci_uart_get_real_tty(hu->tty);
Same here. Add comment above and the also an extra empty line.
> ktermios = tty->termios;
> ktermios.c_cflag &= ~CBAUD;
> tty_termios_encode_baud_rate(&ktermios, speed, speed);
Regards
Marcel
On Sun, Jan 27, 2019 at 10:53:02PM -0800, Myungho Jung wrote:
> tty_set_termios() should be called with slave side of pty driver. So, If
> tty driver is pty master, it needs to be switched to ->link.
I'm not sure that's the right solution. PTYs are virtual devices used
for IPC and neither end (master or slave) have support for modem
control or baud rates.
> Reported-by: [email protected]
> Signed-off-by: Myungho Jung <[email protected]>
> ---
> drivers/bluetooth/hci_ldisc.c | 20 +++++++++++++++-----
> 1 file changed, 15 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/bluetooth/hci_ldisc.c b/drivers/bluetooth/hci_ldisc.c
> index fbf7b4df23ab..90c5ea8c399b 100644
> --- a/drivers/bluetooth/hci_ldisc.c
> +++ b/drivers/bluetooth/hci_ldisc.c
> @@ -299,10 +299,18 @@ static int hci_uart_send_frame(struct hci_dev *hdev, struct sk_buff *skb)
> return 0;
> }
>
> +/* If driver is pty master, return slave side */
> +static struct tty_struct *hci_uart_get_real_tty(struct tty_struct *tty)
> +{
> + return (tty->driver->type == TTY_DRIVER_TYPE_PTY &&
> + tty->driver->subtype == PTY_TYPE_MASTER) ? tty->link : tty;
> +}
> +
> /* Flow control or un-flow control the device */
> void hci_uart_set_flow_control(struct hci_uart *hu, bool enable)
> {
> struct tty_struct *tty = hu->tty;
> + struct tty_struct *real_tty;
> struct ktermios ktermios;
> int status;
> unsigned int set = 0;
> @@ -314,11 +322,12 @@ void hci_uart_set_flow_control(struct hci_uart *hu, bool enable)
> return;
> }
>
> + real_tty = hci_uart_get_real_tty(tty);
> if (enable) {
> /* Disable hardware flow control */
> - ktermios = tty->termios;
> + ktermios = real_tty->termios;
> ktermios.c_cflag &= ~CRTSCTS;
> - status = tty_set_termios(tty, &ktermios);
> + status = tty_set_termios(real_tty, &ktermios);
> BT_DBG("Disabling hardware flow control: %s",
> status ? "failed" : "success");
So instead of these pointless calls to set the slave termios and
modem-control state, you might as well bail out early above (and
similarly in set_baudrate()).
Using n_hci for a master pty really makes no sense at all, so we could
even bail out at ldisc open, but perhaps that can be discussed and
addressed later.
Johan
On Wed, Jan 30, 2019 at 11:07:38AM +0100, Johan Hovold wrote:
> On Sun, Jan 27, 2019 at 10:53:02PM -0800, Myungho Jung wrote:
> > tty_set_termios() should be called with slave side of pty driver. So, If
> > tty driver is pty master, it needs to be switched to ->link.
>
> I'm not sure that's the right solution. PTYs are virtual devices used
> for IPC and neither end (master or slave) have support for modem
> control or baud rates.
>
> > Reported-by: [email protected]
> > Signed-off-by: Myungho Jung <[email protected]>
> > ---
> > drivers/bluetooth/hci_ldisc.c | 20 +++++++++++++++-----
> > 1 file changed, 15 insertions(+), 5 deletions(-)
> >
> > diff --git a/drivers/bluetooth/hci_ldisc.c b/drivers/bluetooth/hci_ldisc.c
> > index fbf7b4df23ab..90c5ea8c399b 100644
> > --- a/drivers/bluetooth/hci_ldisc.c
> > +++ b/drivers/bluetooth/hci_ldisc.c
> > @@ -299,10 +299,18 @@ static int hci_uart_send_frame(struct hci_dev *hdev, struct sk_buff *skb)
> > return 0;
> > }
> >
> > +/* If driver is pty master, return slave side */
> > +static struct tty_struct *hci_uart_get_real_tty(struct tty_struct *tty)
> > +{
> > + return (tty->driver->type == TTY_DRIVER_TYPE_PTY &&
> > + tty->driver->subtype == PTY_TYPE_MASTER) ? tty->link : tty;
> > +}
> > +
> > /* Flow control or un-flow control the device */
> > void hci_uart_set_flow_control(struct hci_uart *hu, bool enable)
> > {
> > struct tty_struct *tty = hu->tty;
> > + struct tty_struct *real_tty;
> > struct ktermios ktermios;
> > int status;
> > unsigned int set = 0;
> > @@ -314,11 +322,12 @@ void hci_uart_set_flow_control(struct hci_uart *hu, bool enable)
> > return;
> > }
> >
> > + real_tty = hci_uart_get_real_tty(tty);
> > if (enable) {
> > /* Disable hardware flow control */
> > - ktermios = tty->termios;
> > + ktermios = real_tty->termios;
> > ktermios.c_cflag &= ~CRTSCTS;
> > - status = tty_set_termios(tty, &ktermios);
> > + status = tty_set_termios(real_tty, &ktermios);
> > BT_DBG("Disabling hardware flow control: %s",
> > status ? "failed" : "success");
>
> So instead of these pointless calls to set the slave termios and
> modem-control state, you might as well bail out early above (and
> similarly in set_baudrate()).
>
> Using n_hci for a master pty really makes no sense at all, so we could
> even bail out at ldisc open, but perhaps that can be discussed and
> addressed later.
>
> Johan
Hi Johan,
I fixed it to just return -EOPNOTSUPP if NULL in ath_setup().
Thanks,
Myungho
On Wed, Jan 30, 2019 at 09:13:53PM -0800, Myungho Jung wrote:
> On Wed, Jan 30, 2019 at 11:07:38AM +0100, Johan Hovold wrote:
> > On Sun, Jan 27, 2019 at 10:53:02PM -0800, Myungho Jung wrote:
> > > tty_set_termios() should be called with slave side of pty driver. So, If
> > > tty driver is pty master, it needs to be switched to ->link.
> >
> > I'm not sure that's the right solution. PTYs are virtual devices used
> > for IPC and neither end (master or slave) have support for modem
> > control or baud rates.
> >
> > > Reported-by: [email protected]
> > > Signed-off-by: Myungho Jung <[email protected]>
> > > ---
> > > drivers/bluetooth/hci_ldisc.c | 20 +++++++++++++++-----
> > > 1 file changed, 15 insertions(+), 5 deletions(-)
> > >
> > > diff --git a/drivers/bluetooth/hci_ldisc.c b/drivers/bluetooth/hci_ldisc.c
> > > index fbf7b4df23ab..90c5ea8c399b 100644
> > > --- a/drivers/bluetooth/hci_ldisc.c
> > > +++ b/drivers/bluetooth/hci_ldisc.c
> > > @@ -299,10 +299,18 @@ static int hci_uart_send_frame(struct hci_dev *hdev, struct sk_buff *skb)
> > > return 0;
> > > }
> > >
> > > +/* If driver is pty master, return slave side */
> > > +static struct tty_struct *hci_uart_get_real_tty(struct tty_struct *tty)
> > > +{
> > > + return (tty->driver->type == TTY_DRIVER_TYPE_PTY &&
> > > + tty->driver->subtype == PTY_TYPE_MASTER) ? tty->link : tty;
> > > +}
> > > +
> > > /* Flow control or un-flow control the device */
> > > void hci_uart_set_flow_control(struct hci_uart *hu, bool enable)
> > > {
> > > struct tty_struct *tty = hu->tty;
> > > + struct tty_struct *real_tty;
> > > struct ktermios ktermios;
> > > int status;
> > > unsigned int set = 0;
> > > @@ -314,11 +322,12 @@ void hci_uart_set_flow_control(struct hci_uart *hu, bool enable)
> > > return;
> > > }
> > >
> > > + real_tty = hci_uart_get_real_tty(tty);
> > > if (enable) {
> > > /* Disable hardware flow control */
> > > - ktermios = tty->termios;
> > > + ktermios = real_tty->termios;
> > > ktermios.c_cflag &= ~CRTSCTS;
> > > - status = tty_set_termios(tty, &ktermios);
> > > + status = tty_set_termios(real_tty, &ktermios);
> > > BT_DBG("Disabling hardware flow control: %s",
> > > status ? "failed" : "success");
> >
> > So instead of these pointless calls to set the slave termios and
> > modem-control state, you might as well bail out early above (and
> > similarly in set_baudrate()).
> >
> > Using n_hci for a master pty really makes no sense at all, so we could
> > even bail out at ldisc open, but perhaps that can be discussed and
> > addressed later.
> I fixed it to just return -EOPNOTSUPP if NULL in ath_setup().
That was the other fix. For this one you need to bail out if the tty is
a pty master so that tty_set_termios never gets called here or in
set_baudrate(). Possibly you can do that already at open().
This is also being discussed here:
https://lkml.kernel.org/r/20190130103227.GR3691@localhost
Johan