Messages in this thread |  | | Date | Tue, 14 Apr 2015 09:50:55 +0200 | From | Michael Wang <> | 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 >
|  |