[PATCH net-next 0/3] mctp serial minor fixes

We had a few minor fixes queued for a v4 of the original series, so
they're sent here as separate changes.

Cheers,


Jeremy

Jeremy Kerr (3):
  mctp: serial: cancel tx work on ldisc close
  mctp: serial: enforce fixed MTU
  mctp: serial: remove unnecessary ldisc data check

 drivers/net/mctp/mctp-serial.c | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

-- 
2.33.0

We want to ensure that the tx work has finished before returning from
the ldisc close op, so do a synchronous cancel.

Reported-by: Jiri Slaby <jirislaby@kernel.org>
Signed-off-by: Jeremy Kerr <jk@codeconstruct.com.au>
---
 drivers/net/mctp/mctp-serial.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/net/mctp/mctp-serial.c b/drivers/net/mctp/mctp-serial.c
index 9ac0e187f36e..c958d773a82a 100644
--- a/drivers/net/mctp/mctp-serial.c
+++ b/drivers/net/mctp/mctp-serial.c

@@ -478,6 +478,7 @@ static void mctp_serial_close(struct tty_struct *tty)
 	struct mctp_serial *dev = tty->disc_data;
 	int idx = dev->idx;
 
+	cancel_work_sync(&dev->tx_work);
 	unregister_netdev(dev->netdev);
 	ida_free(&mctp_serial_ida, idx);
 }

-- 
2.33.0

The current serial driver requires a maximum MTU of 68, and it doesn't
make sense to set a MTU below the MCTP-required baseline (of 68) either.

This change sets the min_mtu & max_mtu of the mctp netdev, essentially
disallowing changes. By using these instead of a ndo_change_mtu op, we
get the netlink extacks reported too.

Signed-off-by: Jeremy Kerr <jk@codeconstruct.com.au>
---
 drivers/net/mctp/mctp-serial.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/net/mctp/mctp-serial.c b/drivers/net/mctp/mctp-serial.c
index c958d773a82a..5687ad3220cd 100644
--- a/drivers/net/mctp/mctp-serial.c
+++ b/drivers/net/mctp/mctp-serial.c

@@ -410,7 +410,14 @@ static const struct net_device_ops mctp_serial_netdev_ops = {
 static void mctp_serial_setup(struct net_device *ndev)
 {
 	ndev->type = ARPHRD_MCTP;
+
+	/* we limit at the fixed MTU, which is also the MCTP-standard
+	 * baseline MTU, so is also our minimum
+	 */
 	ndev->mtu = MCTP_SERIAL_MTU;
+	ndev->max_mtu = MCTP_SERIAL_MTU;
+	ndev->min_mtu = MCTP_SERIAL_MTU;
+
 	ndev->hard_header_len = 0;
 	ndev->addr_len = 0;
 	ndev->tx_queue_len = DEFAULT_TX_QUEUE_LEN;

-- 
2.33.0

Jiri assures me that a ldisc->open with tty->disc_data set should never
happen, so this check doesn't do anything.

Reported-by: Jiri Slaby <jirislaby@kernel.org>
Signed-off-by: Jeremy Kerr <jk@codeconstruct.com.au>
---
 drivers/net/mctp/mctp-serial.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/drivers/net/mctp/mctp-serial.c b/drivers/net/mctp/mctp-serial.c
index 5687ad3220cd..9919a4734ed9 100644
--- a/drivers/net/mctp/mctp-serial.c
+++ b/drivers/net/mctp/mctp-serial.c

@@ -439,9 +439,6 @@ static int mctp_serial_open(struct tty_struct *tty)
 	if (!tty->ops->write)
 		return -EOPNOTSUPP;
 
-	if (tty->disc_data)
-		return -EEXIST;
-
 	idx = ida_alloc(&mctp_serial_ida, GFP_KERNEL);
 	if (idx < 0)
 		return idx;

-- 
2.33.0

On 23. 11. 21, 13:50, Jeremy Kerr wrote:

But the work still can be queued after the cancel (and before the 
unregister), right?

  	unregister_netdev(dev->netdev);
  	ida_free(&mctp_serial_ida, idx);
  }

thanks,
-- 
js
suse labs

On 24. 11. 21, 6:36, Jiri Slaby wrote:

On 23. 11. 21, 13:50, Jeremy Kerr wrote:

quoted

We want to ensure that the tx work has finished before returning from
the ldisc close op, so do a synchronous cancel.

Reported-by: Jiri Slaby <jirislaby@kernel.org>
Signed-off-by: Jeremy Kerr <jk@codeconstruct.com.au>
---
  drivers/net/mctp/mctp-serial.c | 1 +
  1 file changed, 1 insertion(+)

diff --git a/drivers/net/mctp/mctp-serial.c 

b/drivers/net/mctp/mctp-serial.c
index 9ac0e187f36e..c958d773a82a 100644

--- a/drivers/net/mctp/mctp-serial.c
+++ b/drivers/net/mctp/mctp-serial.c

@@ -478,6 +478,7 @@ static void mctp_serial_close(struct tty_struct *tty)

      struct mctp_serial *dev = tty->disc_data;
      int idx = dev->idx;
+    cancel_work_sync(&dev->tx_work);

But the work still can be queued after the cancel (and before the 
unregister), right?

Maybe do it in ->ndo_uninit()?

quoted

      unregister_netdev(dev->netdev);
      ida_free(&mctp_serial_ida, idx);
  }

thanks,

-- 
js
suse labs

Hi Jiri,

quoted

+       cancel_work_sync(&dev->tx_work);

But the work still can be queued after the cancel (and before the 
unregister), right?

Yes. Yes it can.

I should be cancelling after the unregister, not before, so this'll need
a v2.

On the ldisc side: is there any case where we'd get a write wakeup
during (or after) the ->close()?

Cheers,


Jeremy

Hi,

On 24. 11. 21, 7:38, Jeremy Kerr wrote:

On the ldisc side: is there any case where we'd get a write wakeup
during (or after) the ->close()?

there should be no invocation of ldisc after close(). If there is, it's 
a bug as this is even documented:

  * @close: [TTY] ``void ()(struct tty_struct *tty)``
  *
  *      This function is called when the line discipline is being shutdown,
  *      either because the @tty is being closed or because the @tty is 
being
  *      changed to use a new line discipline. At the point of execution no
  *      further users will enter the ldisc code for this tty.
  *
  *      Can sleep.


Should be the same also for the "during" case.

regards,
-- 
js

Hi Jiri,

there should be no invocation of ldisc after close(). If there is,
it's a bug as this is even documented:

Excellent thanks for that (and the doc pointer), I'll get a v2 done
now.

Cheers,


Jeremy