Re: [PATCH v3 27/28] IB/Verbs: Clean up rdma_ib_or_iboe()

[Michael Wang]

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@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/