Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S932078AbbDNJNT (ORCPT ); Tue, 14 Apr 2015 05:13:19 -0400 Received: from mail-wi0-f169.google.com ([209.85.212.169]:37675 "EHLO mail-wi0-f169.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752780AbbDNJNH (ORCPT ); Tue, 14 Apr 2015 05:13:07 -0400 Message-ID: <552CDA1F.4050609@profitbricks.com> Date: Tue, 14 Apr 2015 11:13:03 +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: Jason Gunthorpe 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 , Ira Weiny , Tom Talpey , Doug Ledford Subject: Re: [PATCH v3 27/28] IB/Verbs: Clean up rdma_ib_or_iboe() References: <552BB470.4090407@profitbricks.com> <552BB85D.7010400@profitbricks.com> <20150413203350.GA20611@obsidianresearch.com> In-Reply-To: <20150413203350.GA20611@obsidianresearch.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: 2507 Lines: 71 On 04/13/2015 10:33 PM, Jason Gunthorpe wrote: > On Mon, Apr 13, 2015 at 02:36:45PM +0200, Michael Wang wrote: >> We have finished introducing the cap_XX(), and raw helper rdma_ib_or_iboe() >> is no longer necessary, thus clean it up. > > So, the net result is not looking too bad, but I'm confused about the > structure of this series. > > Why introduce query_transport early on? This won't be erased until bitmask introduced, at this moment it's the basic method for helpers to acquire port transport from device. Sure we can still use the legacy method but IMHO this abstraction will be more readable for internal reforming, it's like 'mapping from tech to bits' VS 'mapping from transport and link layer to bits'. > > Why is the patch series going through this progression most lines? > > - if (rdma_port_get_link_layer(device, port_num) == IB_LINK_LAYER_INFINIBAND) { > + if (rdma_tech_ib(device, port_num)) { This is mostly focus on the reforming on logical. > + if (cap_ib_smi(device, port_num)) { This focus on the description and semantic, won't contain logical reform , just replace the helper. I want this way to help us focus on the different main point during the review :-) > > This would be a lot shorter and simpler to look at if the cap's were > introduced first, then their implementation finalized. > > I thought we agreed Doug's bitmask plan should be the final > destination for this series, so this progress seems even stranger? > > I would be very happy to see a patch that adds cap_ib_smi to the > current tree and states 'This patch is tested to have no change on the > binary compilation results' There are too much reform there (per-dev to per-port), I guess the binary will changed more or less anyway... BTW, I may misunderstanding your point on "Re: [PATCH v2 03/17]": > I would prefer to see these changes in control flow as dedicated > patches, at the top of your patch stack. > For this kind of work a patch should be mechanical changes only, it is > easier to review that way. > Same comment applies throughout. I thought you prefer introducing cap_XX() based on the reforming... But anyway, please let me know if this really bothered the review :-) Regards, Michael Wang > > Jason > -- 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/