From: Greg KH <gregkh@linuxfoundation.org>
To: "Ilpo Järvinen" <ilpo.jarvinen@linux.intel.com>
Cc: linux-serial@vger.kernel.org, Jiri Slaby <jirislaby@kernel.org>,
Andy Shevchenko <andriy.shevchenko@linux.intel.com>,
linux-kernel@vger.kernel.org,
Gilles Buloz <gilles.buloz@kontron.com>,
Johan Hovold <johan@kernel.org>
Subject: Re: [PATCH v3 3/5] tty: Add lookahead param to receive_buf
Date: Fri, 22 Apr 2022 16:34:04 +0200 [thread overview]
Message-ID: <YmK83NfVqEvGg8DW@kroah.com> (raw)
In-Reply-To: <20220411094859.10894-4-ilpo.jarvinen@linux.intel.com>
On Mon, Apr 11, 2022 at 12:48:57PM +0300, Ilpo Järvinen wrote:
> After lookahead for XON/XOFF characters is added by the next
> patch,
There is no "next" in a series. This commit needs to be justified here,
right?
> the receive side needs to ensure the flow-control
> actions are not retaken later on when those same characters
> get received by TTY.
>
> Thus, pass lookahead count to receive_buf and skip
> flow-control character actions if already taken for the
> character in question. Lookahead count will become live after
> the next patch.
You have all 72 columns, please use them.
>
> The logic in n_tty_receive_char_closing is bit tricky.
> The lookahead_done checks cannot be combined with the main
> conditions because doing so would give chance for the part
> after || in the else if condition to execute when lookahead
> done is set (rather than just skipping the flow-control
> character like it should).
>
> Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
> ---
> drivers/accessibility/speakup/spk_ttyio.c | 2 +-
> drivers/bluetooth/hci_ldisc.c | 2 +-
> drivers/char/pcmcia/synclink_cs.c | 2 +-
> drivers/input/serio/serport.c | 4 +-
> drivers/isdn/capi/capi.c | 2 +-
> drivers/misc/ti-st/st_core.c | 2 +-
> drivers/net/caif/caif_serial.c | 4 +-
> drivers/net/can/slcan.c | 2 +-
> drivers/net/hamradio/6pack.c | 4 +-
> drivers/net/hamradio/mkiss.c | 2 +-
> drivers/net/mctp/mctp-serial.c | 2 +-
> drivers/net/ppp/ppp_async.c | 2 +-
> drivers/net/ppp/ppp_synctty.c | 2 +-
> drivers/net/slip/slip.c | 2 +-
> drivers/tty/n_gsm.c | 2 +-
> drivers/tty/n_hdlc.c | 2 +-
> drivers/tty/n_null.c | 2 +-
> drivers/tty/n_tty.c | 53 ++++++++++++++---------
> drivers/tty/serdev/serdev-ttyport.c | 3 +-
> drivers/tty/synclink_gt.c | 2 +-
> drivers/tty/tty_buffer.c | 8 ++--
> drivers/tty/tty_io.c | 2 +-
> drivers/tty/tty_port.c | 5 ++-
> drivers/tty/vt/selection.c | 2 +-
> include/linux/tty_flip.h | 2 +-
> include/linux/tty_ldisc.h | 20 ++++++---
> include/linux/tty_port.h | 3 +-
> net/nfc/nci/uart.c | 2 +-
> sound/soc/codecs/cx20442.c | 2 +-
> sound/soc/ti/ams-delta.c | 2 +-
> 30 files changed, 83 insertions(+), 63 deletions(-)
>
> diff --git a/drivers/accessibility/speakup/spk_ttyio.c b/drivers/accessibility/speakup/spk_ttyio.c
> index 08cf8a17754b..b33536eea1d3 100644
> --- a/drivers/accessibility/speakup/spk_ttyio.c
> +++ b/drivers/accessibility/speakup/spk_ttyio.c
> @@ -73,7 +73,7 @@ static void spk_ttyio_ldisc_close(struct tty_struct *tty)
>
> static int spk_ttyio_receive_buf2(struct tty_struct *tty,
> const unsigned char *cp,
> - const char *fp, int count)
> + const char *fp, int count, unsigned int lookahead_count)
Ick, adding yet-another-parameter to a function is a mess as it's hard
to know what to do with this and what it means just by looking at when
it is called.
> {
> struct spk_ldisc_data *ldisc_data = tty->disc_data;
> struct spk_synth *synth = ldisc_data->synth;
> diff --git a/drivers/bluetooth/hci_ldisc.c b/drivers/bluetooth/hci_ldisc.c
> index f537673ede17..08c329a4cdcc 100644
> --- a/drivers/bluetooth/hci_ldisc.c
> +++ b/drivers/bluetooth/hci_ldisc.c
> @@ -596,7 +596,7 @@ static void hci_uart_tty_wakeup(struct tty_struct *tty)
> * Return Value: None
> */
> static void hci_uart_tty_receive(struct tty_struct *tty, const u8 *data,
> - const char *flags, int count)
> + const char *flags, int count, unsigned int lookahead_count)
> {
> struct hci_uart *hu = tty->disc_data;
>
> diff --git a/drivers/char/pcmcia/synclink_cs.c b/drivers/char/pcmcia/synclink_cs.c
> index 78baba55a8b5..de9c151cfb12 100644
> --- a/drivers/char/pcmcia/synclink_cs.c
> +++ b/drivers/char/pcmcia/synclink_cs.c
> @@ -501,7 +501,7 @@ static void ldisc_receive_buf(struct tty_struct *tty,
> ld = tty_ldisc_ref(tty);
> if (ld) {
> if (ld->ops->receive_buf)
> - ld->ops->receive_buf(tty, data, flags, count);
> + ld->ops->receive_buf(tty, data, flags, count, 0);
> tty_ldisc_deref(ld);
> }
> }
> diff --git a/drivers/input/serio/serport.c b/drivers/input/serio/serport.c
> index 669a728095b8..f0f19b5a2059 100644
> --- a/drivers/input/serio/serport.c
> +++ b/drivers/input/serio/serport.c
> @@ -114,8 +114,8 @@ static void serport_ldisc_close(struct tty_struct *tty)
> * 'interrupt' routine.
> */
>
> -static void serport_ldisc_receive(struct tty_struct *tty,
> - const unsigned char *cp, const char *fp, int count)
> +static void serport_ldisc_receive(struct tty_struct *tty, const unsigned char *cp,
> + const char *fp, int count, unsigned int lookahead_count)
> {
> struct serport *serport = (struct serport*) tty->disc_data;
> unsigned long flags;
> diff --git a/drivers/isdn/capi/capi.c b/drivers/isdn/capi/capi.c
> index 0f00be62438d..beb4c78a7219 100644
> --- a/drivers/isdn/capi/capi.c
> +++ b/drivers/isdn/capi/capi.c
> @@ -454,7 +454,7 @@ static int handle_recv_skb(struct capiminor *mp, struct sk_buff *skb)
> skb_pull(skb, CAPIMSG_LEN(skb->data));
> pr_debug("capi: DATA_B3_RESP %u len=%d => ldisc\n",
> datahandle, skb->len);
> - ld->ops->receive_buf(tty, skb->data, NULL, skb->len);
> + ld->ops->receive_buf(tty, skb->data, NULL, skb->len, 0);
> } else {
> printk(KERN_ERR "capi: send DATA_B3_RESP failed=%x\n",
> errcode);
> diff --git a/drivers/misc/ti-st/st_core.c b/drivers/misc/ti-st/st_core.c
> index 7f6976a9f508..b2c96d72c0e3 100644
> --- a/drivers/misc/ti-st/st_core.c
> +++ b/drivers/misc/ti-st/st_core.c
> @@ -797,7 +797,7 @@ static void st_tty_close(struct tty_struct *tty)
> }
>
> static void st_tty_receive(struct tty_struct *tty, const unsigned char *data,
> - const char *tty_flags, int count)
> + const char *tty_flags, int count, unsigned int lookahead_count)
> {
> #ifdef VERBOSE
> print_hex_dump(KERN_DEBUG, ">in>", DUMP_PREFIX_NONE,
> diff --git a/drivers/net/caif/caif_serial.c b/drivers/net/caif/caif_serial.c
> index 688075859ae4..cc97c436ec88 100644
> --- a/drivers/net/caif/caif_serial.c
> +++ b/drivers/net/caif/caif_serial.c
> @@ -159,7 +159,7 @@ static inline void debugfs_tx(struct ser_device *ser, const u8 *data, int size)
> #endif
>
> static void ldisc_receive(struct tty_struct *tty, const u8 *data,
> - const char *flags, int count)
> + const char *flags, int count, unsigned int lookahead_count)
> {
> struct sk_buff *skb = NULL;
> struct ser_device *ser;
> @@ -237,7 +237,7 @@ static int handle_tx(struct ser_device *ser)
> update_tty_status(ser);
> } else {
> tty_wr = len;
> - ldisc_receive(tty, skb->data, NULL, len);
> + ldisc_receive(tty, skb->data, NULL, len, 0);
See, what does 0 here mean? Who knows? Not a good api :(
> }
> ser->dev->stats.tx_packets++;
> ser->dev->stats.tx_bytes += tty_wr;
> diff --git a/drivers/net/can/slcan.c b/drivers/net/can/slcan.c
> index ec294d0c5722..5e03e14c2d99 100644
> --- a/drivers/net/can/slcan.c
> +++ b/drivers/net/can/slcan.c
> @@ -471,7 +471,7 @@ static void slc_setup(struct net_device *dev)
>
> static void slcan_receive_buf(struct tty_struct *tty,
> const unsigned char *cp, const char *fp,
> - int count)
> + int count, unsigned int lookahead_count)
> {
> struct slcan *sl = (struct slcan *) tty->disc_data;
>
> diff --git a/drivers/net/hamradio/6pack.c b/drivers/net/hamradio/6pack.c
> index 45c3c4a1101b..6ba4b05dff78 100644
> --- a/drivers/net/hamradio/6pack.c
> +++ b/drivers/net/hamradio/6pack.c
> @@ -426,8 +426,8 @@ static void sixpack_write_wakeup(struct tty_struct *tty)
> * a block of 6pack data has been received, which can now be decapsulated
> * and sent on to some IP layer for further processing.
> */
> -static void sixpack_receive_buf(struct tty_struct *tty,
> - const unsigned char *cp, const char *fp, int count)
> +static void sixpack_receive_buf(struct tty_struct *tty, const unsigned char *cp,
> + const char *fp, int count, unsigned int lookahead_count)
> {
> struct sixpack *sp;
> int count1;
> diff --git a/drivers/net/hamradio/mkiss.c b/drivers/net/hamradio/mkiss.c
> index c251e04ae047..498997ef429a 100644
> --- a/drivers/net/hamradio/mkiss.c
> +++ b/drivers/net/hamradio/mkiss.c
> @@ -875,7 +875,7 @@ static int mkiss_ioctl(struct tty_struct *tty, unsigned int cmd,
> * and sent on to the AX.25 layer for further processing.
> */
> static void mkiss_receive_buf(struct tty_struct *tty, const unsigned char *cp,
> - const char *fp, int count)
> + const char *fp, int count, unsigned int lookahead_count)
> {
> struct mkiss *ax = mkiss_get(tty);
>
> diff --git a/drivers/net/mctp/mctp-serial.c b/drivers/net/mctp/mctp-serial.c
> index 7cd103fd34ef..f1dfab4c54cb 100644
> --- a/drivers/net/mctp/mctp-serial.c
> +++ b/drivers/net/mctp/mctp-serial.c
> @@ -390,7 +390,7 @@ static void mctp_serial_push(struct mctp_serial *dev, unsigned char c)
>
> static void mctp_serial_tty_receive_buf(struct tty_struct *tty,
> const unsigned char *c,
> - const char *f, int len)
> + const char *f, int len, unsigned int lookahead_count)
> {
> struct mctp_serial *dev = tty->disc_data;
> int i;
> diff --git a/drivers/net/ppp/ppp_async.c b/drivers/net/ppp/ppp_async.c
> index 15a179631903..87c205a02984 100644
> --- a/drivers/net/ppp/ppp_async.c
> +++ b/drivers/net/ppp/ppp_async.c
> @@ -338,7 +338,7 @@ ppp_asynctty_poll(struct tty_struct *tty, struct file *file, poll_table *wait)
> /* May sleep, don't call from interrupt level or with interrupts disabled */
> static void
> ppp_asynctty_receive(struct tty_struct *tty, const unsigned char *buf,
> - const char *cflags, int count)
> + const char *cflags, int count, unsigned int lookahead_count)
> {
> struct asyncppp *ap = ap_get(tty);
> unsigned long flags;
> diff --git a/drivers/net/ppp/ppp_synctty.c b/drivers/net/ppp/ppp_synctty.c
> index 18283b7b94bc..b82f4bd82b21 100644
> --- a/drivers/net/ppp/ppp_synctty.c
> +++ b/drivers/net/ppp/ppp_synctty.c
> @@ -331,7 +331,7 @@ ppp_sync_poll(struct tty_struct *tty, struct file *file, poll_table *wait)
> /* May sleep, don't call from interrupt level or with interrupts disabled */
> static void
> ppp_sync_receive(struct tty_struct *tty, const unsigned char *buf,
> - const char *cflags, int count)
> + const char *cflags, int count, unsigned int lookahead_count)
> {
> struct syncppp *ap = sp_get(tty);
> unsigned long flags;
> diff --git a/drivers/net/slip/slip.c b/drivers/net/slip/slip.c
> index 88396ff99f03..7fccf28256a8 100644
> --- a/drivers/net/slip/slip.c
> +++ b/drivers/net/slip/slip.c
> @@ -686,7 +686,7 @@ static void sl_setup(struct net_device *dev)
> */
>
> static void slip_receive_buf(struct tty_struct *tty, const unsigned char *cp,
> - const char *fp, int count)
> + const char *fp, int count, unsigned int lookahead_count)
> {
> struct slip *sl = tty->disc_data;
>
> diff --git a/drivers/tty/n_gsm.c b/drivers/tty/n_gsm.c
> index fa92f727fdf8..45f022892b28 100644
> --- a/drivers/tty/n_gsm.c
> +++ b/drivers/tty/n_gsm.c
> @@ -2500,7 +2500,7 @@ static void gsmld_detach_gsm(struct tty_struct *tty, struct gsm_mux *gsm)
> }
>
> static void gsmld_receive_buf(struct tty_struct *tty, const unsigned char *cp,
> - const char *fp, int count)
> + const char *fp, int count, unsigned int lookahead_count)
> {
> struct gsm_mux *gsm = tty->disc_data;
> char flags = TTY_NORMAL;
> diff --git a/drivers/tty/n_hdlc.c b/drivers/tty/n_hdlc.c
> index 94c1ec2dd754..2d7c94a38a92 100644
> --- a/drivers/tty/n_hdlc.c
> +++ b/drivers/tty/n_hdlc.c
> @@ -379,7 +379,7 @@ static void n_hdlc_tty_wakeup(struct tty_struct *tty)
> * interpreted as one HDLC frame.
> */
> static void n_hdlc_tty_receive(struct tty_struct *tty, const __u8 *data,
> - const char *flags, int count)
> + const char *flags, int count, unsigned int lookahead_count)
> {
> register struct n_hdlc *n_hdlc = tty->disc_data;
> register struct n_hdlc_buf *buf;
> diff --git a/drivers/tty/n_null.c b/drivers/tty/n_null.c
> index f913b665af72..6bce76b5bb1c 100644
> --- a/drivers/tty/n_null.c
> +++ b/drivers/tty/n_null.c
> @@ -34,7 +34,7 @@ static ssize_t n_null_write(struct tty_struct *tty, struct file *file,
>
> static void n_null_receivebuf(struct tty_struct *tty,
> const unsigned char *cp, const char *fp,
> - int cnt)
> + int cnt, unsigned int lookahead_count)
> {
> }
>
> diff --git a/drivers/tty/n_tty.c b/drivers/tty/n_tty.c
> index 90b3e06cbeb1..708cc85ac43d 100644
> --- a/drivers/tty/n_tty.c
> +++ b/drivers/tty/n_tty.c
> @@ -1226,11 +1226,15 @@ static bool n_tty_is_char_flow_ctrl(struct tty_struct *tty, unsigned char c)
> }
>
> /* Returns true if c is consumed as flow-control character */
> -static bool n_tty_receive_char_flow_ctrl(struct tty_struct *tty, unsigned char c)
> +static bool n_tty_receive_char_flow_ctrl(struct tty_struct *tty, unsigned char c,
> + bool lookahead_done)
> {
> if (!n_tty_is_char_flow_ctrl(tty, c))
> return false;
>
> + if (lookahead_done)
> + return true;
Why would this function be called if this option was true?
> +
> if (c == START_CHAR(tty)) {
> start_tty(tty);
> process_echoes(tty);
> @@ -1241,11 +1245,12 @@ static bool n_tty_receive_char_flow_ctrl(struct tty_struct *tty, unsigned char c
> return true;
> }
>
> -static void n_tty_receive_char_special(struct tty_struct *tty, unsigned char c)
> +static void n_tty_receive_char_special(struct tty_struct *tty, unsigned char c,
> + bool lookahead_done)
> {
> struct n_tty_data *ldata = tty->disc_data;
>
> - if (I_IXON(tty) && n_tty_receive_char_flow_ctrl(tty, c))
> + if (I_IXON(tty) && n_tty_receive_char_flow_ctrl(tty, c, lookahead_done))
> return;
>
> if (L_ISIG(tty)) {
> @@ -1400,7 +1405,8 @@ static void n_tty_receive_char(struct tty_struct *tty, unsigned char c)
> put_tty_queue(c, ldata);
> }
>
> -static void n_tty_receive_char_closing(struct tty_struct *tty, unsigned char c)
> +static void n_tty_receive_char_closing(struct tty_struct *tty, unsigned char c,
> + bool lookahead_done)
bool here, yet characters elsewhere? That's messy :(
the overall idea is good, this implementation isn't quite there yet.
I did take the first patch in the series, it made sense.
thanks,
greg k-h
next prev parent reply other threads:[~2022-04-22 14:34 UTC|newest]
Thread overview: 12+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-04-11 9:48 [PATCH v3 0/5] tty/serial: Process XON/XOFF robustly Ilpo Järvinen
2022-04-11 9:48 ` [PATCH v3 1/5] tty: Add function for handling flow control chars Ilpo Järvinen
2022-04-11 9:48 ` [PATCH v3 2/5] tty: Simplify receive flow control char logic Ilpo Järvinen
2022-04-22 14:30 ` Greg KH
2022-04-11 9:48 ` [PATCH v3 3/5] tty: Add lookahead param to receive_buf Ilpo Järvinen
2022-04-22 14:34 ` Greg KH [this message]
2022-04-22 20:09 ` Ilpo Järvinen
2022-04-26 7:48 ` Ilpo Järvinen
2022-04-26 7:59 ` Greg KH
2022-04-11 9:48 ` [PATCH v3 4/5] tty: Implement lookahead to process XON/XOFF timely Ilpo Järvinen
2022-04-11 9:48 ` [PATCH v3 5/5] tty: Use flow-control char function on closing path Ilpo Järvinen
2022-04-11 11:28 ` Andy Shevchenko
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=YmK83NfVqEvGg8DW@kroah.com \
--to=gregkh@linuxfoundation.org \
--cc=andriy.shevchenko@linux.intel.com \
--cc=gilles.buloz@kontron.com \
--cc=ilpo.jarvinen@linux.intel.com \
--cc=jirislaby@kernel.org \
--cc=johan@kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-serial@vger.kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).