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@xxxxxx> > 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; > ...