Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753403AbbDNHvP (ORCPT ); Tue, 14 Apr 2015 03:51:15 -0400 Received: from mail-wi0-f182.google.com ([209.85.212.182]:38134 "EHLO mail-wi0-f182.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752447AbbDNHvE (ORCPT ); Tue, 14 Apr 2015 03:51:04 -0400 Message-ID: <552CC6DF.8070704@profitbricks.com> Date: Tue, 14 Apr 2015 09:50:55 +0200 From: Michael Wang User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Thunderbird/31.6.0 MIME-Version: 1.0 To: "ira.weiny" CC: Roland Dreier , Sean Hefty , Hal Rosenstock , linux-rdma@vger.kernel.org, linux-kernel@vger.kernel.org, Tom Tucker , Steve Wise , Hoang-Nam Nguyen , Christoph Raisch , Mike Marciniszyn , Eli Cohen , Faisal Latif , Jack Morgenstein , Or Gerlitz , Haggai Eran , Tom Talpey , Jason Gunthorpe , Doug Ledford Subject: Re: [PATCH v3 04/28] IB/Verbs: Reform IB-core cm References: <552BB470.4090407@profitbricks.com> <552BB552.1030905@profitbricks.com> <20150413181248.GA2464@phlsvsds.ph.intel.com> In-Reply-To: <20150413181248.GA2464@phlsvsds.ph.intel.com> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 2483 Lines: 84 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@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/