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
| ||
|
Date: Tue, 14 Apr 2015 09:50:55 +0200 From: Michael Wang <yun.wang@...fitbricks.com> To: "ira.weiny" <ira.weiny@...el.com> CC: Roland Dreier <roland@...nel.org>, Sean Hefty <sean.hefty@...el.com>, Hal Rosenstock <hal.rosenstock@...il.com>, linux-rdma@...r.kernel.org, linux-kernel@...r.kernel.org, Tom Tucker <tom@...ngridcomputing.com>, Steve Wise <swise@...ngridcomputing.com>, Hoang-Nam Nguyen <hnguyen@...ibm.com>, Christoph Raisch <raisch@...ibm.com>, Mike Marciniszyn <infinipath@...el.com>, Eli Cohen <eli@...lanox.com>, Faisal Latif <faisal.latif@...el.com>, Jack Morgenstein <jackm@....mellanox.co.il>, Or Gerlitz <ogerlitz@...lanox.com>, Haggai Eran <haggaie@...lanox.com>, Tom Talpey <tom@...pey.com>, Jason Gunthorpe <jgunthorpe@...idianresearch.com>, Doug Ledford <dledford@...hat.com> Subject: Re: [PATCH v3 04/28] IB/Verbs: Reform IB-core cm On 04/13/2015 08:12 PM, ira.weiny wrote: [snip] >> - >> - if (rdma_node_get_transport(ib_device->node_type) != RDMA_TRANSPORT_IB) >> - return; >> + int count = 0; > > I'm ok with this as an intermediate patch but going forward if we are going to > have calls like > > static inline int cap_ib_cm_dev(struct ib_device *device) Actually I really don't want to introduce this kind of helper, it's slow, ugly and break the consistency, but I can't find a good way to avoid that... For example the check inside cma_listen_on_dev(), how could we do per-port check while don't even know which port will be used later... > > Then I think we should have similar calls like > > cap_ib_mad_dev(device) > > Which eliminates the clean up below... I'd like to avoid using such helper as long as possible :-P > >> >> cm_dev = kzalloc(sizeof(*cm_dev) + sizeof(*port) * >> ib_device->phys_port_cnt, GFP_KERNEL); >> @@ -3783,6 +3781,9 @@ static void cm_add_one(struct ib_device *ib_device) >> >> set_bit(IB_MGMT_METHOD_SEND, reg_req.method_mask); >> for (i = 1; i <= ib_device->phys_port_cnt; i++) { >> + if (!rdma_ib_or_iboe(ib_device, i)) >> + continue; >> + >> port = kzalloc(sizeof *port, GFP_KERNEL); >> if (!port) >> goto error1; >> @@ -3809,7 +3810,16 @@ static void cm_add_one(struct ib_device *ib_device) >> ret = ib_modify_port(ib_device, i, 0, &port_modify); >> if (ret) >> goto error3; >> + >> + count++; >> } >> + >> + if (!count) { >> + device_unregister(cm_dev->device); >> + kfree(cm_dev); >> + return; > > Here. > > I worry about mistakes being made when we loop through only to find that none > of the ports support the feature and then we have to clean up. As this is > initialization code I don't see any issue with looping through the ports 2 > times and making the code cleaner. This style of logical could be found in other core module too, may be keep consistent is not a bad idea? After all, it's just initialization code which relatively rarely used :-) Regards, Michael Wang > > This applies to the SA and CM modules as well. > > However, in the ib_cm module you already have cap_ib_cm_dev(device) so you > should use it at the start of cm_add_one. > > Ira > -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@...r.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists