Re: [PATCH for-next 06/10] rdma: Set physical MTU for query_port function

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 





On 4/20/2021 8:31 AM, Leon Romanovsky wrote:
On Tue, Apr 20, 2021 at 08:17:00AM -0400, Dennis Dalessandro wrote:
On 4/20/2021 1:11 AM, Leon Romanovsky wrote:
On Mon, Apr 19, 2021 at 09:08:55AM -0400, Dennis Dalessandro wrote:
On 4/19/2021 8:29 AM, Leon Romanovsky wrote:
On Mon, Apr 19, 2021 at 12:20:33PM +0000, Wan, Kaike wrote:


-----Original Message-----
From: Dennis Dalessandro <dennis.dalessandro@xxxxxxxxxxxxxxxxxxxx>
Sent: Thursday, April 08, 2021 8:31 AM
To: Leon Romanovsky <leon@xxxxxxxxxx>
Cc: dledford@xxxxxxxxxx; jgg@xxxxxxxx; linux-rdma@xxxxxxxxxxxxxxx; Wan,
Kaike <kaike.wan@xxxxxxxxx>
Subject: Re: [PATCH for-next 06/10] rdma: Set physical MTU for query_port
function

On 4/8/2021 8:14 AM, Leon Romanovsky wrote:
On Thu, Apr 08, 2021 at 08:06:46AM -0400, Dennis Dalessandro wrote:
On 4/1/2021 4:42 AM, Leon Romanovsky wrote:
On Mon, Mar 29, 2021 at 09:54:12AM -0400,
dennis.dalessandro@xxxxxxxxxxxxxxxxxxxx wrote:
From: Kaike Wan <kaike.wan@xxxxxxxxx>

This is a follow on patch to add a phys_mtu field to the
ib_port_attr structure to indicate the maximum physical MTU the
underlying device supports.

Extends the following:
commit 6d72344cf6c4 ("IB/ipoib: Increase ipoib Datagram mode MTU's
upper limit")

Reviewed-by: Mike Marciniszyn
<mike.marciniszyn@xxxxxxxxxxxxxxxxxxxx>
Signed-off-by: Kaike Wan <kaike.wan@xxxxxxxxx>
Signed-off-by: Dennis Dalessandro
<dennis.dalessandro@xxxxxxxxxxxxxxxxxxxx>
---
      drivers/infiniband/hw/bnxt_re/ib_verbs.c        |  1 +
      drivers/infiniband/hw/cxgb4/provider.c          |  1 +
      drivers/infiniband/hw/efa/efa_verbs.c           |  1 +
      drivers/infiniband/hw/hns/hns_roce_main.c       |  1 +
      drivers/infiniband/hw/i40iw/i40iw_verbs.c       |  1 +
      drivers/infiniband/hw/mlx4/main.c               |  1 +
      drivers/infiniband/hw/mlx5/mad.c                |  1 +
      drivers/infiniband/hw/mlx5/main.c               |  2 ++
      drivers/infiniband/hw/mthca/mthca_provider.c    |  1 +
      drivers/infiniband/hw/ocrdma/ocrdma_verbs.c     |  1 +
      drivers/infiniband/hw/qib/qib_verbs.c           |  1 +
      drivers/infiniband/hw/usnic/usnic_ib_verbs.c    |  1 +
      drivers/infiniband/hw/vmw_pvrdma/pvrdma_verbs.c |  1 +
      drivers/infiniband/sw/siw/siw_verbs.c           |  1 +
      drivers/infiniband/ulp/ipoib/ipoib_main.c       |  2 +-
      include/rdma/ib_verbs.h                         | 17 -----------------
      16 files changed, 16 insertions(+), 18 deletions(-)

But why? What will it give us that almost all drivers have same
props->phys_mtu = ib_mtu_enum_to_int(props->max_mtu); line?


Almost is not all. Alternative idea to convey this? Seemed like a
sensible thing to at least have support for but open to other approaches.

What about leave it as is?

I'm struggling to get the rationale behind this patch., the code
already works and set the phys_mtu correctly, isn't it?

I see what you are saying now. Kaike, correct me if I'm wrong, but the intent
of this patch is just to make everything behave the same in the sense that a
device could have a different physical MTU. The field got added to the
ib_port_attr previously so this is giving it an initial value vs leaving it unset.
[Wan, Kaike]  Correct.

No one is using this "phys_mtu" field, except one place in ipoib.

Today. I think it would be better to formalize the idea though and have a
cleaner interface. Does this cause some problem?

Not directly, but yes.

Before your change, drivers don't need to care about this field because
it is not in use at all, after your change all drivers need to carry same
line. This is prone to errors.

Perhaps a more common place to set this in the core is appropriate.

This is basically what I suggested to Kaike.

Yep, just seen that after I hit send.



[Index of Archives]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Photo]     [Yosemite News]     [Yosemite Photos]     [Linux Kernel]     [Linux SCSI]     [XFree86]

  Powered by Linux