[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <e877c99a-072e-76d5-3c83-d552caf0c1f6@quicinc.com>
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;
>> }
>>
>
> ...
Powered by blists - more mailing lists