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