netdev - Re: [PATCH net-next v3] net: qualcomm: rmnet: Add side band flow control support

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date: Thu, 5 Oct 2023 16:52:22 -0600
From: "Subash Abhinov Kasiviswanathan (KS)" <quic_subashab@...cinc.com>
To: Simon Horman <horms@...nel.org>
CC: <davem@...emloft.net>, <edumazet@...gle.com>, <kuba@...nel.org>,
        <pabeni@...hat.com>, <netdev@...r.kernel.org>,
        <vadim.fedorenko@...ux.dev>, <lkp@...el.com>,
        Sean Tranchetti <quic_stranche@...cinc.com>
Subject: Re: [PATCH net-next v3] net: qualcomm: rmnet: Add side band flow
 control support


On 10/5/2023 6:44 AM, Simon Horman wrote:
> On Wed, Oct 04, 2023 at 01:43:20PM -0700, Subash Abhinov Kasiviswanathan wrote:
>> Individual rmnet devices map to specific network types such as internet,
>> multimedia messaging services, IP multimedia subsystem etc. Each of
>> these network types may support varying quality of service for different
>> bearers or traffic types.
>>
> 
> Hi Subash and Sean,
> 
> a few comments on error handling from my side.
> 
> ...
> 
>> +	default:
>> +		NL_SET_ERR_MSG_MOD(extack, "unsupported operation");
>> +		return -EINVAL;
> 
> I'm wondering if EOPNOTSUPP is appropriate here.

Hi Simon

Sure, I can update this error code here and return the appropriate value 
through the caller functions.

> 
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>>   static void rmnet_unregister_bridge(struct rmnet_port *port)
>>   {
>>   	struct net_device *bridge_dev, *real_dev, *rmnet_dev;
>> @@ -175,8 +237,24 @@ static int rmnet_newlink(struct net *src_net, struct net_device *dev,
>>   	netdev_dbg(dev, "data format [0x%08X]\n", data_format);
>>   	port->data_format = data_format;
>>   
>> +	if (data[IFLA_RMNET_QUEUE]) {
>> +		struct rmnet_queue_mapping *queue_map;
>> +
>> +		queue_map = nla_data(data[IFLA_RMNET_QUEUE]);
>> +		if (rmnet_update_queue_map(dev, queue_map->operation,
>> +					   queue_map->txqueue, queue_map->mark,
>> +					   extack))
> 
> Should the return value of rmnet_update_queue_map() be stored in err
> so that it is also the return value of this function?
> 
>> +			goto err3;
>> +
>> +		netdev_dbg(dev, "op %02x txq %02x mark %08x\n",
>> +			   queue_map->operation, queue_map->txqueue,
>> +			   queue_map->mark);
>> +	}
>> +
>>   	return 0;
>>   
>> +err3:
>> +	hlist_del_init_rcu(&ep->hlnode);
> 
> Is a call to netdev_upper_dev_unlink() needed here?

I'll update this missing API call in the cleanup.

> 
>>   err2:
>>   	unregister_netdevice(dev);
>>   	rmnet_vnd_dellink(mux_id, port, ep);
>> @@ -352,6 +430,20 @@ static int rmnet_changelink(struct net_device *dev, struct nlattr *tb[],
>>   		}
>>   	}
>>   
>> +	if (data[IFLA_RMNET_QUEUE]) {
>> +		struct rmnet_queue_mapping *queue_map;
>> +
>> +		queue_map = nla_data(data[IFLA_RMNET_QUEUE]);
>> +		if (rmnet_update_queue_map(dev, queue_map->operation,
>> +					   queue_map->txqueue, queue_map->mark,
>> +					   extack))
>> +			return -EINVAL;
> 
> I guess that with the current implementation of rmnet_update_queue_map()
> it makes no difference, but perhaps it would be better to return
> the return value of rmnet_update_queue_map() rather than hard coding
> -EINVAL here.
> 
>> +
>> +		netdev_dbg(dev, "op %02x txq %02x mark %08x\n",
>> +			   queue_map->operation, queue_map->txqueue,
>> +			   queue_map->mark);
>> +	}
>> +
>>   	return 0;
>>   }
>>   
> 
> ...