[PATCH] net: can: Add support for aliases in CAN

[PATCH] net: can: Add support for aliases in CAN

[Bhavya Kapoor]

When multiple CAN's are present, then names that are getting assigned
changes after every boot even after providing alias in the device tree.
Thus, Add support for implementing CAN aliasing so that names or
alias for CAN will now be provided from device tree.

Signed-off-by: Bhavya Kapoor <b-kapoor@ti.com>
---
 drivers/net/can/dev/dev.c     | 15 ++++++++++++---
 drivers/net/can/m_can/m_can.c |  2 +-
 include/linux/can/dev.h       |  8 +++++---
 3 files changed, 18 insertions(+), 7 deletions(-)

diff --git a/drivers/net/can/dev/dev.c b/drivers/net/can/dev/dev.c
index 3a3be5cdfc1f..ed483c23ec79 100644
--- a/drivers/net/can/dev/dev.c
+++ b/drivers/net/can/dev/dev.c
@@ -247,12 +247,14 @@ void can_setup(struct net_device *dev)
 
 /* Allocate and setup space for the CAN network device */
 struct net_device *alloc_candev_mqs(int sizeof_priv, unsigned int echo_skb_max,
-				    unsigned int txqs, unsigned int rxqs)
+					unsigned int txqs, unsigned int rxqs,
+					struct device *candev)
 {
 	struct can_ml_priv *can_ml;
 	struct net_device *dev;
 	struct can_priv *priv;
-	int size;
+	int size, aliasid;
+	char devname[6] = "can%d";
 
 	/* We put the driver's priv, the CAN mid layer priv and the
 	 * echo skb into the netdevice's priv. The memory layout for
@@ -273,7 +275,14 @@ struct net_device *alloc_candev_mqs(int sizeof_priv, unsigned int echo_skb_max,
 		size = ALIGN(size, sizeof(struct sk_buff *)) +
 			echo_skb_max * sizeof(struct sk_buff *);
 
-	dev = alloc_netdev_mqs(size, "can%d", NET_NAME_UNKNOWN, can_setup,
+	if (candev) {
+		aliasid = of_alias_get_id(candev->of_node, "can");
+		if (aliasid >= 0)
+			snprintf(devname, sizeof(devname), "%s%d", "can", aliasid);
+	}
+	dev_dbg(candev, "Name of CAN assigned is : %s\n", devname);
+
+	dev = alloc_netdev_mqs(size, devname, NET_NAME_UNKNOWN, can_setup,
 			       txqs, rxqs);
 	if (!dev)
 		return NULL;
diff --git a/drivers/net/can/m_can/m_can.c b/drivers/net/can/m_can/m_can.c
index 16ecc11c7f62..c91a5c7b3ae5 100644
--- a/drivers/net/can/m_can/m_can.c
+++ b/drivers/net/can/m_can/m_can.c
@@ -2029,7 +2029,7 @@ struct m_can_classdev *m_can_class_allocate_dev(struct device *dev,
 	tx_fifo_size = mram_config_vals[7];
 
 	/* allocate the m_can device */
-	net_dev = alloc_candev(sizeof_priv, tx_fifo_size);
+	net_dev = alloc_candev_with_dev(sizeof_priv, tx_fifo_size, dev);
 	if (!net_dev) {
 		dev_err(dev, "Failed to allocate CAN device");
 		goto out;
diff --git a/include/linux/can/dev.h b/include/linux/can/dev.h
index 1b92aed49363..b59142c16e59 100644
--- a/include/linux/can/dev.h
+++ b/include/linux/can/dev.h
@@ -171,11 +171,13 @@ static inline bool can_dev_dropped_skb(struct net_device *dev, struct sk_buff *s
 void can_setup(struct net_device *dev);
 
 struct net_device *alloc_candev_mqs(int sizeof_priv, unsigned int echo_skb_max,
-				    unsigned int txqs, unsigned int rxqs);
+					unsigned int txqs, unsigned int rxqs, struct device *dev);
 #define alloc_candev(sizeof_priv, echo_skb_max) \
-	alloc_candev_mqs(sizeof_priv, echo_skb_max, 1, 1)
+	alloc_candev_mqs(sizeof_priv, echo_skb_max, 1, 1, NULL)
+#define alloc_candev_with_dev(sizeof_priv, echo_skb_max, dev) \
+	alloc_candev_mqs(sizeof_priv, echo_skb_max, 1, 1, dev)
 #define alloc_candev_mq(sizeof_priv, echo_skb_max, count) \
-	alloc_candev_mqs(sizeof_priv, echo_skb_max, count, count)
+	alloc_candev_mqs(sizeof_priv, echo_skb_max, count, count, NULL)
 void free_candev(struct net_device *dev);
 
 /* a candev safe wrapper around netdev_priv */
-- 
2.40.1

Re: [PATCH] net: can: Add support for aliases in CAN

[Marc Kleine-Budde]

[-- Attachment #1: Type: text/plain, Size: 694 bytes --]

On 02.01.2024 15:59:49, Bhavya Kapoor wrote:
> When multiple CAN's are present, then names that are getting assigned
> changes after every boot even after providing alias in the device tree.
> Thus, Add support for implementing CAN aliasing so that names or
> alias for CAN will now be provided from device tree.

NACK, please use udev or systemd-networkd to provide stable names for
CAN interfaces.

regards,
Marc

-- 
Pengutronix e.K.                 | Marc Kleine-Budde          |
Embedded Linux                   | https://www.pengutronix.de |
Vertretung Nürnberg              | Phone: +49-5121-206917-129 |
Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-9   |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH] net: can: Add support for aliases in CAN

[Simon Horman]

On Tue, Jan 02, 2024 at 03:59:49PM +0530, Bhavya Kapoor wrote:
> When multiple CAN's are present, then names that are getting assigned
> changes after every boot even after providing alias in the device tree.
> Thus, Add support for implementing CAN aliasing so that names or
> alias for CAN will now be provided from device tree.
> 
> Signed-off-by: Bhavya Kapoor <b-kapoor@ti.com>

Hi Bhavya,

some minor feedback from my side.

...

> diff --git a/drivers/net/can/dev/dev.c b/drivers/net/can/dev/dev.c
> index 3a3be5cdfc1f..ed483c23ec79 100644
> --- a/drivers/net/can/dev/dev.c
> +++ b/drivers/net/can/dev/dev.c
> @@ -247,12 +247,14 @@ void can_setup(struct net_device *dev)
>  
>  /* Allocate and setup space for the CAN network device */
>  struct net_device *alloc_candev_mqs(int sizeof_priv, unsigned int echo_skb_max,
> -				    unsigned int txqs, unsigned int rxqs)
> +					unsigned int txqs, unsigned int rxqs,
> +					struct device *candev)
>  {
>  	struct can_ml_priv *can_ml;
>  	struct net_device *dev;
>  	struct can_priv *priv;
> -	int size;
> +	int size, aliasid;
> +	char devname[6] = "can%d";

nit: Please consider arranging local variables in Networking code
     in reverse xmas tree order - longest line to shortest.

>  
>  	/* We put the driver's priv, the CAN mid layer priv and the
>  	 * echo skb into the netdevice's priv. The memory layout for
> @@ -273,7 +275,14 @@ struct net_device *alloc_candev_mqs(int sizeof_priv, unsigned int echo_skb_max,
>  		size = ALIGN(size, sizeof(struct sk_buff *)) +
>  			echo_skb_max * sizeof(struct sk_buff *);
>  
> -	dev = alloc_netdev_mqs(size, "can%d", NET_NAME_UNKNOWN, can_setup,
> +	if (candev) {
> +		aliasid = of_alias_get_id(candev->of_node, "can");
> +		if (aliasid >= 0)
> +			snprintf(devname, sizeof(devname), "%s%d", "can", aliasid);

The size of devname is 6 bytes (can%d\0).
This means that snprintf() will truncate devname if alias is greater than 99.
Is this a concern?

If so, perhaps devname could be declared to be IFNAMSIZ bytes long?

Flagged by gcc-13 -Wformat-truncation

> +	}
> +	dev_dbg(candev, "Name of CAN assigned is : %s\n", devname);
> +
> +	dev = alloc_netdev_mqs(size, devname, NET_NAME_UNKNOWN, can_setup,
>  			       txqs, rxqs);
>  	if (!dev)
>  		return NULL;

...

Re: [PATCH] net: can: Add support for aliases in CAN

[Bhavya Kapoor]

On 04/01/24 22:49, Simon Horman wrote:
> On Tue, Jan 02, 2024 at 03:59:49PM +0530, Bhavya Kapoor wrote:
>> When multiple CAN's are present, then names that are getting assigned
>> changes after every boot even after providing alias in the device tree.
>> Thus, Add support for implementing CAN aliasing so that names or
>> alias for CAN will now be provided from device tree.
>>
>> Signed-off-by: Bhavya Kapoor <b-kapoor@ti.com>
> Hi Bhavya,
>
> some minor feedback from my side.
>
> ...
>
>> diff --git a/drivers/net/can/dev/dev.c b/drivers/net/can/dev/dev.c
>> index 3a3be5cdfc1f..ed483c23ec79 100644
>> --- a/drivers/net/can/dev/dev.c
>> +++ b/drivers/net/can/dev/dev.c
>> @@ -247,12 +247,14 @@ void can_setup(struct net_device *dev)
>>  
>>  /* Allocate and setup space for the CAN network device */
>>  struct net_device *alloc_candev_mqs(int sizeof_priv, unsigned int echo_skb_max,
>> -				    unsigned int txqs, unsigned int rxqs)
>> +					unsigned int txqs, unsigned int rxqs,
>> +					struct device *candev)
>>  {
>>  	struct can_ml_priv *can_ml;
>>  	struct net_device *dev;
>>  	struct can_priv *priv;
>> -	int size;
>> +	int size, aliasid;
>> +	char devname[6] = "can%d";
> nit: Please consider arranging local variables in Networking code
>      in reverse xmas tree order - longest line to shortest.
Okay, i will keep this in mind from next time.
>
>>  
>>  	/* We put the driver's priv, the CAN mid layer priv and the
>>  	 * echo skb into the netdevice's priv. The memory layout for
>> @@ -273,7 +275,14 @@ struct net_device *alloc_candev_mqs(int sizeof_priv, unsigned int echo_skb_max,
>>  		size = ALIGN(size, sizeof(struct sk_buff *)) +
>>  			echo_skb_max * sizeof(struct sk_buff *);
>>  
>> -	dev = alloc_netdev_mqs(size, "can%d", NET_NAME_UNKNOWN, can_setup,
>> +	if (candev) {
>> +		aliasid = of_alias_get_id(candev->of_node, "can");
>> +		if (aliasid >= 0)
>> +			snprintf(devname, sizeof(devname), "%s%d", "can", aliasid);
> The size of devname is 6 bytes (can%d\0).
> This means that snprintf() will truncate devname if alias is greater than 99.
> Is this a concern?

When sequential naming will be done from can0 in aliases for can, 

considering that 99 is still a very large number and so 6 bytes for

devname should suffice.

Regards

> If so, perhaps devname could be declared to be IFNAMSIZ bytes long?
>
> Flagged by gcc-13 -Wformat-truncation
>
>> +	}
>> +	dev_dbg(candev, "Name of CAN assigned is : %s\n", devname);
>> +
>> +	dev = alloc_netdev_mqs(size, devname, NET_NAME_UNKNOWN, can_setup,
>>  			       txqs, rxqs);
>>  	if (!dev)
>>  		return NULL;
> ...

Re: [PATCH] net: can: Add support for aliases in CAN

[Kumar, Udit]

Hi Marc

On 1/2/2024 4:43 PM, Marc Kleine-Budde wrote:
> On 02.01.2024 15:59:49, Bhavya Kapoor wrote:
>> When multiple CAN's are present, then names that are getting assigned
>> changes after every boot even after providing alias in the device tree.
>> Thus, Add support for implementing CAN aliasing so that names or
>> alias for CAN will now be provided from device tree.
> NACK, please use udev or systemd-networkd to provide stable names for
> CAN interfaces.

Would you like to re-consider this NACK.

 From kernel side,

IMO if aliasing is set in device tree then kernel should provide 
consistent baseline names.

However, distributions may choose different or other stable naming,

Also, if some distribution want to rely on kernel naming they still can do.

Thanks

>
> regards,
> Marc
>

Re: [PATCH] net: can: Add support for aliases in CAN

[Marc Kleine-Budde]

[-- Attachment #1: Type: text/plain, Size: 1271 bytes --]

On 12.01.2024 20:53:32, Kumar, Udit wrote:
> Hi Marc
> 
> On 1/2/2024 4:43 PM, Marc Kleine-Budde wrote:
> > On 02.01.2024 15:59:49, Bhavya Kapoor wrote:
> > > When multiple CAN's are present, then names that are getting assigned
> > > changes after every boot even after providing alias in the device tree.
> > > Thus, Add support for implementing CAN aliasing so that names or
> > > alias for CAN will now be provided from device tree.
> > NACK, please use udev or systemd-networkd to provide stable names for
> > CAN interfaces.
> 
> Would you like to re-consider this NACK.

This is not a CAN device specific problem. If you want to change this,
talk/convince the networking people.

> From kernel side,
> 
> IMO if aliasing is set in device tree then kernel should provide consistent
> baseline names.
> 
> However, distributions may choose different or other stable naming,
> 
> Also, if some distribution want to rely on kernel naming they still can do.

regards,
Marc

-- 
Pengutronix e.K.                 | Marc Kleine-Budde          |
Embedded Linux                   | https://www.pengutronix.de |
Vertretung Nürnberg              | Phone: +49-5121-206917-129 |
Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-9   |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]