diff mbox series

[net,3/3] can: isotp: add SF_BROADCAST support for functional addressing

Message ID 20201204133508.742120-4-mkl@pengutronix.de
State New
Headers show
Series [net,1/3] can: softing: softing_netdev_open(): fix error handling | expand

Commit Message

Marc Kleine-Budde Dec. 4, 2020, 1:35 p.m. UTC 
From: Oliver Hartkopp <socketcan@hartkopp.net>

When CAN_ISOTP_SF_BROADCAST is set in the CAN_ISOTP_OPTS flags the CAN_ISOTP
socket is switched into functional addressing mode, where only single frame
(SF) protocol data units can be send on the specified CAN interface and the
given tp.tx_id after bind().

In opposite to normal and extended addressing this socket does not register a
CAN-ID for reception which would be needed for a 1-to-1 ISOTP connection with a
segmented bi-directional data transfer.

Sending SFs on this socket is therefore a TX-only 'broadcast' operation.

Suggested-by: Thomas Wagner <thwa1@web.de>
Signed-off-by: Oliver Hartkopp <socketcan@hartkopp.net>
Tested-by: Thomas Wagner <thwa1@web.de>
Link: https://lore.kernel.org/r/20201203140604.25488-3-socketcan@hartkopp.net
Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
---
 include/uapi/linux/can/isotp.h |  2 +-
 net/can/isotp.c                | 29 ++++++++++++++++++++---------
 2 files changed, 21 insertions(+), 10 deletions(-)

Comments

Jakub Kicinski Dec. 5, 2020, 3:44 a.m. UTC | #1 
On Fri,  4 Dec 2020 14:35:08 +0100 Marc Kleine-Budde wrote:
> From: Oliver Hartkopp <socketcan@hartkopp.net>
> 
> When CAN_ISOTP_SF_BROADCAST is set in the CAN_ISOTP_OPTS flags the CAN_ISOTP
> socket is switched into functional addressing mode, where only single frame
> (SF) protocol data units can be send on the specified CAN interface and the
> given tp.tx_id after bind().
> 
> In opposite to normal and extended addressing this socket does not register a
> CAN-ID for reception which would be needed for a 1-to-1 ISOTP connection with a
> segmented bi-directional data transfer.
> 
> Sending SFs on this socket is therefore a TX-only 'broadcast' operation.

Unclear from this patch what is getting fixed. Looks a little bit like
a feature which could be added in a backward compatible way, no?
Is it only added for completeness of the ISOTP implementation?
Marc Kleine-Budde Dec. 5, 2020, 9:20 p.m. UTC | #2 
On 12/5/20 10:09 PM, Jakub Kicinski wrote:
> On Sat, 5 Dec 2020 21:56:33 +0100 Marc Kleine-Budde wrote:
>> On 12/5/20 9:33 PM, Jakub Kicinski wrote:
>>>> What about the (incremental?) change that Thomas Wagner posted?
>>>>
>>>> https://lore.kernel.org/r/20201204135557.55599-1-thwa1@web.de  
>>>
>>> That settles it :) This change needs to got into -next and 5.11.  
>>
>> Ok. Can you take patch 1, which is a real fix:
>>
>> https://lore.kernel.org/linux-can/20201204133508.742120-2-mkl@pengutronix.de/
> 
> Sure! Applied that one from the ML (I assumed that's what you meant).

Thanks, exactly that one.

regards,
Marc
Oliver Hartkopp Dec. 8, 2020, 12:54 p.m. UTC | #3 
On 05.12.20 22:09, Jakub Kicinski wrote:
> On Sat, 5 Dec 2020 21:56:33 +0100 Marc Kleine-Budde wrote:

>> On 12/5/20 9:33 PM, Jakub Kicinski wrote:

>>>> What about the (incremental?) change that Thomas Wagner posted?

>>>>

>>>> https://lore.kernel.org/r/20201204135557.55599-1-thwa1@web.de

>>>

>>> That settles it :) This change needs to got into -next and 5.11.

>>

>> Ok. Can you take patch 1, which is a real fix:

>>

>> https://lore.kernel.org/linux-can/20201204133508.742120-2-mkl@pengutronix.de/

> 

> Sure! Applied that one from the ML (I assumed that's what you meant).

> 


I just double-checked this mail and in fact the second patch from Marc's 
pull request was a real fix too:

https://lore.kernel.org/linux-can/20201204133508.742120-3-mkl@pengutronix.de/

Btw. the missing feature which was added for completeness of the ISOTP 
implementation has now also integrated the improvement suggested by 
Thomas Wagner:

https://lore.kernel.org/linux-can/20201206144731.4609-1-socketcan@hartkopp.net/T/#u

Would be cool if it could go into the initial iso-tp contribution as 
5.10 becomes a long-term kernel.

But I don't want to be pushy - treat it as your like.

Many thanks,
Oliver
Jakub Kicinski Dec. 8, 2020, 6:07 p.m. UTC | #4 
On Tue, 8 Dec 2020 13:54:28 +0100 Oliver Hartkopp wrote:
> On 05.12.20 22:09, Jakub Kicinski wrote:

> > On Sat, 5 Dec 2020 21:56:33 +0100 Marc Kleine-Budde wrote:  

> >> On 12/5/20 9:33 PM, Jakub Kicinski wrote:  

> >>>> What about the (incremental?) change that Thomas Wagner posted?

> >>>>

> >>>> https://lore.kernel.org/r/20201204135557.55599-1-thwa1@web.de  

> >>>

> >>> That settles it :) This change needs to got into -next and 5.11.  

> >>

> >> Ok. Can you take patch 1, which is a real fix:

> >>

> >> https://lore.kernel.org/linux-can/20201204133508.742120-2-mkl@pengutronix.de/  

> > 

> > Sure! Applied that one from the ML (I assumed that's what you meant).

> 

> I just double-checked this mail and in fact the second patch from Marc's 

> pull request was a real fix too:

> 

> https://lore.kernel.org/linux-can/20201204133508.742120-3-mkl@pengutronix.de/


Ack, I thought it was a fix to some existing code but it's a fix to
ISO-TP so we should probably get it in before someone start depending
on existing behavior - Marc should I apply that one directly, too?

> Btw. the missing feature which was added for completeness of the ISOTP 

> implementation has now also integrated the improvement suggested by 

> Thomas Wagner:

> 

> https://lore.kernel.org/linux-can/20201206144731.4609-1-socketcan@hartkopp.net/T/#u

> 

> Would be cool if it could go into the initial iso-tp contribution as 

> 5.10 becomes a long-term kernel.

> 

> But I don't want to be pushy - treat it as your like.


I think Linus wants to release 5.10 so that the merge window doesn't
overlap with Christmas too much. Let's not push our luck.
Marc Kleine-Budde Dec. 9, 2020, 7:45 a.m. UTC | #5 
On 12/8/20 7:07 PM, Jakub Kicinski wrote:
> On Tue, 8 Dec 2020 13:54:28 +0100 Oliver Hartkopp wrote:

>> On 05.12.20 22:09, Jakub Kicinski wrote:

>>> On Sat, 5 Dec 2020 21:56:33 +0100 Marc Kleine-Budde wrote:  

>>>> On 12/5/20 9:33 PM, Jakub Kicinski wrote:  

>>>>>> What about the (incremental?) change that Thomas Wagner posted?

>>>>>>

>>>>>> https://lore.kernel.org/r/20201204135557.55599-1-thwa1@web.de  

>>>>>

>>>>> That settles it :) This change needs to got into -next and 5.11.  

>>>>

>>>> Ok. Can you take patch 1, which is a real fix:

>>>>

>>>> https://lore.kernel.org/linux-can/20201204133508.742120-2-mkl@pengutronix.de/  

>>>

>>> Sure! Applied that one from the ML (I assumed that's what you meant).

>>

>> I just double-checked this mail and in fact the second patch from Marc's 

>> pull request was a real fix too:

>>

>> https://lore.kernel.org/linux-can/20201204133508.742120-3-mkl@pengutronix.de/

> 

> Ack, I thought it was a fix to some existing code but it's a fix to

> ISO-TP so we should probably get it in before someone start depending

> on existing behavior - Marc should I apply that one directly, too?


Yes, please take that patch directly.

Marc

-- 
Pengutronix e.K.                 | Marc Kleine-Budde           |
Embedded Linux                   | https://www.pengutronix.de  |
Vertretung West/Dortmund         | Phone: +49-231-2826-924     |
Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-5555 |
diff mbox series

Patch

diff --git a/include/uapi/linux/can/isotp.h b/include/uapi/linux/can/isotp.h
index 7793b26aa154..c55935b64ccc 100644
--- a/include/uapi/linux/can/isotp.h
+++ b/include/uapi/linux/can/isotp.h
@@ -135,7 +135,7 @@  struct can_isotp_ll_options {
 #define CAN_ISOTP_FORCE_RXSTMIN	0x100	/* ignore CFs depending on rx stmin */
 #define CAN_ISOTP_RX_EXT_ADDR	0x200	/* different rx extended addressing */
 #define CAN_ISOTP_WAIT_TX_DONE	0x400	/* wait for tx completion */
-
+#define CAN_ISOTP_SF_BROADCAST	0x800	/* 1-to-N functional addressing */
 
 /* default values */
 
diff --git a/net/can/isotp.c b/net/can/isotp.c
index 26bdc3c20b7e..5ce26e568f16 100644
--- a/net/can/isotp.c
+++ b/net/can/isotp.c
@@ -865,6 +865,14 @@  static int isotp_sendmsg(struct socket *sock, struct msghdr *msg, size_t size)
 	if (!size || size > MAX_MSG_LENGTH)
 		return -EINVAL;
 
+	/* take care of a potential SF_DL ESC offset for TX_DL > 8 */
+	off = (so->tx.ll_dl > CAN_MAX_DLEN) ? 1 : 0;
+
+	/* does the given data fit into a single frame for SF_BROADCAST? */
+	if ((so->opt.flags & CAN_ISOTP_SF_BROADCAST) &&
+	    (size > so->tx.ll_dl - SF_PCI_SZ4 - ae - off))
+		return -EINVAL;
+
 	err = memcpy_from_msg(so->tx.buf, msg, size);
 	if (err < 0)
 		return err;
@@ -891,9 +899,6 @@  static int isotp_sendmsg(struct socket *sock, struct msghdr *msg, size_t size)
 	cf = (struct canfd_frame *)skb->data;
 	skb_put(skb, so->ll.mtu);
 
-	/* take care of a potential SF_DL ESC offset for TX_DL > 8 */
-	off = (so->tx.ll_dl > CAN_MAX_DLEN) ? 1 : 0;
-
 	/* check for single frame transmission depending on TX_DL */
 	if (size <= so->tx.ll_dl - SF_PCI_SZ4 - ae - off) {
 		/* The message size generally fits into a SingleFrame - good.
@@ -1016,7 +1021,7 @@  static int isotp_release(struct socket *sock)
 	hrtimer_cancel(&so->rxtimer);
 
 	/* remove current filters & unregister */
-	if (so->bound) {
+	if (so->bound && (!(so->opt.flags & CAN_ISOTP_SF_BROADCAST))) {
 		if (so->ifindex) {
 			struct net_device *dev;
 
@@ -1052,6 +1057,7 @@  static int isotp_bind(struct socket *sock, struct sockaddr *uaddr, int len)
 	struct net_device *dev;
 	int err = 0;
 	int notify_enetdown = 0;
+	int do_rx_reg = 1;
 
 	if (len < CAN_REQUIRED_SIZE(struct sockaddr_can, can_addr.tp))
 		return -EINVAL;
@@ -1066,6 +1072,10 @@  static int isotp_bind(struct socket *sock, struct sockaddr *uaddr, int len)
 	if (!addr->can_ifindex)
 		return -ENODEV;
 
+	/* do not register frame reception for functional addressing */
+	if (so->opt.flags & CAN_ISOTP_SF_BROADCAST)
+		do_rx_reg = 0;
+
 	lock_sock(sk);
 
 	if (so->bound && addr->can_ifindex == so->ifindex &&
@@ -1093,13 +1103,14 @@  static int isotp_bind(struct socket *sock, struct sockaddr *uaddr, int len)
 
 	ifindex = dev->ifindex;
 
-	can_rx_register(net, dev, addr->can_addr.tp.rx_id,
-			SINGLE_MASK(addr->can_addr.tp.rx_id), isotp_rcv, sk,
-			"isotp", sk);
+	if (do_rx_reg)
+		can_rx_register(net, dev, addr->can_addr.tp.rx_id,
+				SINGLE_MASK(addr->can_addr.tp.rx_id),
+				isotp_rcv, sk, "isotp", sk);
 
 	dev_put(dev);
 
-	if (so->bound) {
+	if (so->bound && do_rx_reg) {
 		/* unregister old filter */
 		if (so->ifindex) {
 			dev = dev_get_by_index(net, so->ifindex);
@@ -1302,7 +1313,7 @@  static int isotp_notifier(struct notifier_block *nb, unsigned long msg,
 	case NETDEV_UNREGISTER:
 		lock_sock(sk);
 		/* remove current filters & unregister */
-		if (so->bound)
+		if (so->bound && (!(so->opt.flags & CAN_ISOTP_SF_BROADCAST)))
 			can_rx_unregister(dev_net(dev), dev, so->rxid,
 					  SINGLE_MASK(so->rxid),
 					  isotp_rcv, sk);