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

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

[Jeremy Kerr]

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

[PATCH net-next 1/3] mctp: serial: cancel tx work on ldisc close

[Jeremy Kerr]

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

[PATCH net-next 2/3] mctp: serial: enforce fixed MTU

[Jeremy Kerr]

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

[PATCH net-next 3/3] mctp: serial: remove unnecessary ldisc data check

[Jeremy Kerr]

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

Re: [PATCH net-next 1/3] mctp: serial: cancel tx work on ldisc close

[Jiri Slaby]

On 23. 11. 21, 13:50, Jeremy Kerr wrote:
> 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?

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

thanks,
-- 
js
suse labs

Re: [PATCH net-next 1/3] mctp: serial: cancel tx work on ldisc close

[Jiri Slaby]

On 24. 11. 21, 6:36, Jiri Slaby wrote:
> On 23. 11. 21, 13:50, Jeremy Kerr wrote:
>> 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()?

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


-- 
js
suse labs

Re: [PATCH net-next 1/3] mctp: serial: cancel tx work on ldisc close

[Jeremy Kerr]

Hi Jiri,

> > +       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

Re: [PATCH net-next 1/3] mctp: serial: cancel tx work on ldisc close

[Jiri Slaby]

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

Re: [PATCH net-next 1/3] mctp: serial: cancel tx work on ldisc close

[Jeremy Kerr]

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