Received-SPF: pass (google.com: domain of linux-kernel+bounces-17542-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249;
Message-ID: <e08d7240-6b28-43c4-9ce4-95a041bfad25@ti.com>
Date: Fri, 5 Jan 2024 12:49:14 +0530
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH] net: can: Add support for aliases in CAN
Content-Language: en-US
To: Simon Horman <horms@kernel.org>
CC: <linux-kernel@vger.kernel.org>, <netdev@vger.kernel.org>,
        <linux-can@vger.kernel.org>, <mailhol.vincent@wanadoo.fr>,
        <rcsekar@samsung.com>, <pabeni@redhat.com>, <kuba@kernel.org>,
        <edumazet@google.com>, <davem@davemloft.net>, <mkl@pengutronix.de>,
        <wg@grandegger.com>, <vigneshr@ti.com>, <u-kumar1@ti.com>
References: <20240102102949.138607-1-b-kapoor@ti.com>
 <20240104171940.GI31813@kernel.org>
From: Bhavya Kapoor <b-kapoor@ti.com>
In-Reply-To: <20240104171940.GI31813@kernel.org>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: 8bit


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