* [PATCH for-rc] RDMA/srpt: Fix disabling device management
@ 2020-05-11 22:29 Kamal Heib
2020-05-13 7:22 ` Leon Romanovsky
2020-05-13 12:48 ` Jason Gunthorpe
0 siblings, 2 replies; 17+ messages in thread
From: Kamal Heib @ 2020-05-11 22:29 UTC (permalink / raw)
To: linux-rdma; +Cc: Doug Ledford, Jason Gunthorpe, Bart Van Assche, Kamal Heib
Avoid disabling device management for devices that don't support
Management datagrams (MADs) by checking if the "mad_agent" pointer is
initialized before calling ib_modify_port, also change the error message
to a warning and make it more informative.
Fixes: 09f8a1486dca ("RDMA/srpt: Fix handling of SR-IOV and iWARP ports")
Signed-off-by: Kamal Heib <kamalheib1@gmail.com>
---
drivers/infiniband/ulp/srpt/ib_srpt.c | 8 ++++++--
1 file changed, 6 insertions(+), 2 deletions(-)
diff --git a/drivers/infiniband/ulp/srpt/ib_srpt.c b/drivers/infiniband/ulp/srpt/ib_srpt.c
index 7ed38d1cb997..7b21792ab6f7 100644
--- a/drivers/infiniband/ulp/srpt/ib_srpt.c
+++ b/drivers/infiniband/ulp/srpt/ib_srpt.c
@@ -625,14 +625,18 @@ static void srpt_unregister_mad_agent(struct srpt_device *sdev)
.clr_port_cap_mask = IB_PORT_DEVICE_MGMT_SUP,
};
struct srpt_port *sport;
+ int ret;
int i;
for (i = 1; i <= sdev->device->phys_port_cnt; i++) {
sport = &sdev->port[i - 1];
WARN_ON(sport->port != i);
- if (ib_modify_port(sdev->device, i, 0, &port_modify) < 0)
- pr_err("disabling MAD processing failed.\n");
if (sport->mad_agent) {
+ ret = ib_modify_port(sdev->device, i, 0, &port_modify);
+ if (ret < 0)
+ pr_warn("%s-%d: disabling device management failed (%d). Note: this is expected if SR-IOV is enabled.\n",
+ dev_name(&sport->sdev->device->dev),
+ sport->port, ret);
ib_unregister_mad_agent(sport->mad_agent);
sport->mad_agent = NULL;
}
--
2.25.4
^ permalink raw reply related [flat|nested] 17+ messages in thread* Re: [PATCH for-rc] RDMA/srpt: Fix disabling device management 2020-05-11 22:29 [PATCH for-rc] RDMA/srpt: Fix disabling device management Kamal Heib @ 2020-05-13 7:22 ` Leon Romanovsky 2020-05-13 10:02 ` Kamal Heib 2020-05-13 12:48 ` Jason Gunthorpe 1 sibling, 1 reply; 17+ messages in thread From: Leon Romanovsky @ 2020-05-13 7:22 UTC (permalink / raw) To: Kamal Heib; +Cc: linux-rdma, Doug Ledford, Jason Gunthorpe, Bart Van Assche On Tue, May 12, 2020 at 01:29:18AM +0300, Kamal Heib wrote: > Avoid disabling device management for devices that don't support > Management datagrams (MADs) by checking if the "mad_agent" pointer is > initialized before calling ib_modify_port, also change the error message > to a warning and make it more informative. > > Fixes: 09f8a1486dca ("RDMA/srpt: Fix handling of SR-IOV and iWARP ports") > Signed-off-by: Kamal Heib <kamalheib1@gmail.com> > --- > drivers/infiniband/ulp/srpt/ib_srpt.c | 8 ++++++-- > 1 file changed, 6 insertions(+), 2 deletions(-) > > diff --git a/drivers/infiniband/ulp/srpt/ib_srpt.c b/drivers/infiniband/ulp/srpt/ib_srpt.c > index 7ed38d1cb997..7b21792ab6f7 100644 > --- a/drivers/infiniband/ulp/srpt/ib_srpt.c > +++ b/drivers/infiniband/ulp/srpt/ib_srpt.c > @@ -625,14 +625,18 @@ static void srpt_unregister_mad_agent(struct srpt_device *sdev) > .clr_port_cap_mask = IB_PORT_DEVICE_MGMT_SUP, > }; > struct srpt_port *sport; > + int ret; > int i; > > for (i = 1; i <= sdev->device->phys_port_cnt; i++) { > sport = &sdev->port[i - 1]; > WARN_ON(sport->port != i); > - if (ib_modify_port(sdev->device, i, 0, &port_modify) < 0) > - pr_err("disabling MAD processing failed.\n"); > if (sport->mad_agent) { > + ret = ib_modify_port(sdev->device, i, 0, &port_modify); > + if (ret < 0) > + pr_warn("%s-%d: disabling device management failed (%d). Note: this is expected if SR-IOV is enabled.\n", > + dev_name(&sport->sdev->device->dev), The ib_modify_port() shouldn't be called if it expected to fail. Thanks > + sport->port, ret); > ib_unregister_mad_agent(sport->mad_agent); > sport->mad_agent = NULL; > } > -- > 2.25.4 > ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH for-rc] RDMA/srpt: Fix disabling device management 2020-05-13 7:22 ` Leon Romanovsky @ 2020-05-13 10:02 ` Kamal Heib 2020-05-13 10:21 ` Leon Romanovsky 0 siblings, 1 reply; 17+ messages in thread From: Kamal Heib @ 2020-05-13 10:02 UTC (permalink / raw) To: Leon Romanovsky Cc: linux-rdma, Doug Ledford, Jason Gunthorpe, Bart Van Assche On Wed, May 13, 2020 at 10:22:03AM +0300, Leon Romanovsky wrote: > On Tue, May 12, 2020 at 01:29:18AM +0300, Kamal Heib wrote: > > Avoid disabling device management for devices that don't support > > Management datagrams (MADs) by checking if the "mad_agent" pointer is > > initialized before calling ib_modify_port, also change the error message > > to a warning and make it more informative. > > > > Fixes: 09f8a1486dca ("RDMA/srpt: Fix handling of SR-IOV and iWARP ports") > > Signed-off-by: Kamal Heib <kamalheib1@gmail.com> > > --- > > drivers/infiniband/ulp/srpt/ib_srpt.c | 8 ++++++-- > > 1 file changed, 6 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/infiniband/ulp/srpt/ib_srpt.c b/drivers/infiniband/ulp/srpt/ib_srpt.c > > index 7ed38d1cb997..7b21792ab6f7 100644 > > --- a/drivers/infiniband/ulp/srpt/ib_srpt.c > > +++ b/drivers/infiniband/ulp/srpt/ib_srpt.c > > @@ -625,14 +625,18 @@ static void srpt_unregister_mad_agent(struct srpt_device *sdev) > > .clr_port_cap_mask = IB_PORT_DEVICE_MGMT_SUP, > > }; > > struct srpt_port *sport; > > + int ret; > > int i; > > > > for (i = 1; i <= sdev->device->phys_port_cnt; i++) { > > sport = &sdev->port[i - 1]; > > WARN_ON(sport->port != i); > > - if (ib_modify_port(sdev->device, i, 0, &port_modify) < 0) > > - pr_err("disabling MAD processing failed.\n"); > > if (sport->mad_agent) { > > + ret = ib_modify_port(sdev->device, i, 0, &port_modify); > > + if (ret < 0) > > + pr_warn("%s-%d: disabling device management failed (%d). Note: this is expected if SR-IOV is enabled.\n", > > + dev_name(&sport->sdev->device->dev), > > The ib_modify_port() shouldn't be called if it expected to fail. > > Thanks OK, Do you know if there is a way to check if the created ib device is for VF to avoid calling ib_modify_port()? Thanks, Kamal > > > + sport->port, ret); > > ib_unregister_mad_agent(sport->mad_agent); > > sport->mad_agent = NULL; > > } > > -- > > 2.25.4 > > ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH for-rc] RDMA/srpt: Fix disabling device management 2020-05-13 10:02 ` Kamal Heib @ 2020-05-13 10:21 ` Leon Romanovsky 2020-05-13 10:45 ` Kamal Heib 0 siblings, 1 reply; 17+ messages in thread From: Leon Romanovsky @ 2020-05-13 10:21 UTC (permalink / raw) To: Kamal Heib; +Cc: linux-rdma, Doug Ledford, Jason Gunthorpe, Bart Van Assche On Wed, May 13, 2020 at 01:02:04PM +0300, Kamal Heib wrote: > On Wed, May 13, 2020 at 10:22:03AM +0300, Leon Romanovsky wrote: > > On Tue, May 12, 2020 at 01:29:18AM +0300, Kamal Heib wrote: > > > Avoid disabling device management for devices that don't support > > > Management datagrams (MADs) by checking if the "mad_agent" pointer is > > > initialized before calling ib_modify_port, also change the error message > > > to a warning and make it more informative. > > > > > > Fixes: 09f8a1486dca ("RDMA/srpt: Fix handling of SR-IOV and iWARP ports") > > > Signed-off-by: Kamal Heib <kamalheib1@gmail.com> > > > --- > > > drivers/infiniband/ulp/srpt/ib_srpt.c | 8 ++++++-- > > > 1 file changed, 6 insertions(+), 2 deletions(-) > > > > > > diff --git a/drivers/infiniband/ulp/srpt/ib_srpt.c b/drivers/infiniband/ulp/srpt/ib_srpt.c > > > index 7ed38d1cb997..7b21792ab6f7 100644 > > > --- a/drivers/infiniband/ulp/srpt/ib_srpt.c > > > +++ b/drivers/infiniband/ulp/srpt/ib_srpt.c > > > @@ -625,14 +625,18 @@ static void srpt_unregister_mad_agent(struct srpt_device *sdev) > > > .clr_port_cap_mask = IB_PORT_DEVICE_MGMT_SUP, > > > }; > > > struct srpt_port *sport; > > > + int ret; > > > int i; > > > > > > for (i = 1; i <= sdev->device->phys_port_cnt; i++) { > > > sport = &sdev->port[i - 1]; > > > WARN_ON(sport->port != i); > > > - if (ib_modify_port(sdev->device, i, 0, &port_modify) < 0) > > > - pr_err("disabling MAD processing failed.\n"); > > > if (sport->mad_agent) { > > > + ret = ib_modify_port(sdev->device, i, 0, &port_modify); > > > + if (ret < 0) > > > + pr_warn("%s-%d: disabling device management failed (%d). Note: this is expected if SR-IOV is enabled.\n", > > > + dev_name(&sport->sdev->device->dev), > > > > The ib_modify_port() shouldn't be called if it expected to fail. > > > > Thanks > > OK, Do you know if there is a way to check if the created ib device is > for VF to avoid calling ib_modify_port()? The "is_virtfn" field inside pci_dev will give this information, but I don't know why it is expected to fail here. Thanks > > Thanks, > Kamal > > > > > > + sport->port, ret); > > > ib_unregister_mad_agent(sport->mad_agent); > > > sport->mad_agent = NULL; > > > } > > > -- > > > 2.25.4 > > > ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH for-rc] RDMA/srpt: Fix disabling device management 2020-05-13 10:21 ` Leon Romanovsky @ 2020-05-13 10:45 ` Kamal Heib 2020-05-13 10:50 ` Leon Romanovsky 2020-05-13 11:59 ` Gal Pressman 0 siblings, 2 replies; 17+ messages in thread From: Kamal Heib @ 2020-05-13 10:45 UTC (permalink / raw) To: Leon Romanovsky Cc: linux-rdma, Doug Ledford, Jason Gunthorpe, Bart Van Assche On Wed, May 13, 2020 at 01:21:32PM +0300, Leon Romanovsky wrote: > On Wed, May 13, 2020 at 01:02:04PM +0300, Kamal Heib wrote: > > On Wed, May 13, 2020 at 10:22:03AM +0300, Leon Romanovsky wrote: > > > On Tue, May 12, 2020 at 01:29:18AM +0300, Kamal Heib wrote: > > > > Avoid disabling device management for devices that don't support > > > > Management datagrams (MADs) by checking if the "mad_agent" pointer is > > > > initialized before calling ib_modify_port, also change the error message > > > > to a warning and make it more informative. > > > > > > > > Fixes: 09f8a1486dca ("RDMA/srpt: Fix handling of SR-IOV and iWARP ports") > > > > Signed-off-by: Kamal Heib <kamalheib1@gmail.com> > > > > --- > > > > drivers/infiniband/ulp/srpt/ib_srpt.c | 8 ++++++-- > > > > 1 file changed, 6 insertions(+), 2 deletions(-) > > > > > > > > diff --git a/drivers/infiniband/ulp/srpt/ib_srpt.c b/drivers/infiniband/ulp/srpt/ib_srpt.c > > > > index 7ed38d1cb997..7b21792ab6f7 100644 > > > > --- a/drivers/infiniband/ulp/srpt/ib_srpt.c > > > > +++ b/drivers/infiniband/ulp/srpt/ib_srpt.c > > > > @@ -625,14 +625,18 @@ static void srpt_unregister_mad_agent(struct srpt_device *sdev) > > > > .clr_port_cap_mask = IB_PORT_DEVICE_MGMT_SUP, > > > > }; > > > > struct srpt_port *sport; > > > > + int ret; > > > > int i; > > > > > > > > for (i = 1; i <= sdev->device->phys_port_cnt; i++) { > > > > sport = &sdev->port[i - 1]; > > > > WARN_ON(sport->port != i); > > > > - if (ib_modify_port(sdev->device, i, 0, &port_modify) < 0) > > > > - pr_err("disabling MAD processing failed.\n"); > > > > if (sport->mad_agent) { > > > > + ret = ib_modify_port(sdev->device, i, 0, &port_modify); > > > > + if (ret < 0) > > > > + pr_warn("%s-%d: disabling device management failed (%d). Note: this is expected if SR-IOV is enabled.\n", > > > > + dev_name(&sport->sdev->device->dev), > > > > > > The ib_modify_port() shouldn't be called if it expected to fail. > > > > > > Thanks > > > > OK, Do you know if there is a way to check if the created ib device is > > for VF to avoid calling ib_modify_port()? > > The "is_virtfn" field inside pci_dev will give this information, > but I don't know why it is expected to fail here. > > Thanks > Looks like there a more trivial way, I mean checking if IB_DEVICE_VIRTUAL_FUNCTION cap is set, probably there is a need to make to sure that this cap is set for all providers when the IB device is created for a VF. With regards to why it is expected to fail, as stated in the commit message and the in "Fixes" commit message there is some devices (VF) that don't support MADs. Thanks, Kamal > > > > Thanks, > > Kamal > > > > > > > > > + sport->port, ret); > > > > ib_unregister_mad_agent(sport->mad_agent); > > > > sport->mad_agent = NULL; > > > > } > > > > -- > > > > 2.25.4 > > > > ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH for-rc] RDMA/srpt: Fix disabling device management 2020-05-13 10:45 ` Kamal Heib @ 2020-05-13 10:50 ` Leon Romanovsky 2020-05-13 11:14 ` Kamal Heib 2020-05-13 11:59 ` Gal Pressman 1 sibling, 1 reply; 17+ messages in thread From: Leon Romanovsky @ 2020-05-13 10:50 UTC (permalink / raw) To: Kamal Heib; +Cc: linux-rdma, Doug Ledford, Jason Gunthorpe, Bart Van Assche On Wed, May 13, 2020 at 01:45:36PM +0300, Kamal Heib wrote: > On Wed, May 13, 2020 at 01:21:32PM +0300, Leon Romanovsky wrote: > > On Wed, May 13, 2020 at 01:02:04PM +0300, Kamal Heib wrote: > > > On Wed, May 13, 2020 at 10:22:03AM +0300, Leon Romanovsky wrote: > > > > On Tue, May 12, 2020 at 01:29:18AM +0300, Kamal Heib wrote: > > > > > Avoid disabling device management for devices that don't support > > > > > Management datagrams (MADs) by checking if the "mad_agent" pointer is > > > > > initialized before calling ib_modify_port, also change the error message > > > > > to a warning and make it more informative. > > > > > > > > > > Fixes: 09f8a1486dca ("RDMA/srpt: Fix handling of SR-IOV and iWARP ports") > > > > > Signed-off-by: Kamal Heib <kamalheib1@gmail.com> > > > > > --- > > > > > drivers/infiniband/ulp/srpt/ib_srpt.c | 8 ++++++-- > > > > > 1 file changed, 6 insertions(+), 2 deletions(-) > > > > > > > > > > diff --git a/drivers/infiniband/ulp/srpt/ib_srpt.c b/drivers/infiniband/ulp/srpt/ib_srpt.c > > > > > index 7ed38d1cb997..7b21792ab6f7 100644 > > > > > --- a/drivers/infiniband/ulp/srpt/ib_srpt.c > > > > > +++ b/drivers/infiniband/ulp/srpt/ib_srpt.c > > > > > @@ -625,14 +625,18 @@ static void srpt_unregister_mad_agent(struct srpt_device *sdev) > > > > > .clr_port_cap_mask = IB_PORT_DEVICE_MGMT_SUP, > > > > > }; > > > > > struct srpt_port *sport; > > > > > + int ret; > > > > > int i; > > > > > > > > > > for (i = 1; i <= sdev->device->phys_port_cnt; i++) { > > > > > sport = &sdev->port[i - 1]; > > > > > WARN_ON(sport->port != i); > > > > > - if (ib_modify_port(sdev->device, i, 0, &port_modify) < 0) > > > > > - pr_err("disabling MAD processing failed.\n"); > > > > > if (sport->mad_agent) { > > > > > + ret = ib_modify_port(sdev->device, i, 0, &port_modify); > > > > > + if (ret < 0) > > > > > + pr_warn("%s-%d: disabling device management failed (%d). Note: this is expected if SR-IOV is enabled.\n", > > > > > + dev_name(&sport->sdev->device->dev), > > > > > > > > The ib_modify_port() shouldn't be called if it expected to fail. > > > > > > > > Thanks > > > > > > OK, Do you know if there is a way to check if the created ib device is > > > for VF to avoid calling ib_modify_port()? > > > > The "is_virtfn" field inside pci_dev will give this information, > > but I don't know why it is expected to fail here. > > > > Thanks > > > > Looks like there a more trivial way, I mean checking if > IB_DEVICE_VIRTUAL_FUNCTION cap is set, probably there is a need to make > to sure that this cap is set for all providers when the IB device is > created for a VF. > > With regards to why it is expected to fail, as stated in the commit > message and the in "Fixes" commit message there is some devices (VF) > that don't support MADs. So shouldn't you test that device support MAD instead of testing VF cap? Thanks > > Thanks, > Kamal > > > > > > > Thanks, > > > Kamal > > > > > > > > > > > > + sport->port, ret); > > > > > ib_unregister_mad_agent(sport->mad_agent); > > > > > sport->mad_agent = NULL; > > > > > } > > > > > -- > > > > > 2.25.4 > > > > > ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH for-rc] RDMA/srpt: Fix disabling device management 2020-05-13 10:50 ` Leon Romanovsky @ 2020-05-13 11:14 ` Kamal Heib 2020-05-13 11:31 ` Leon Romanovsky 0 siblings, 1 reply; 17+ messages in thread From: Kamal Heib @ 2020-05-13 11:14 UTC (permalink / raw) To: Leon Romanovsky Cc: linux-rdma, Doug Ledford, Jason Gunthorpe, Bart Van Assche On Wed, May 13, 2020 at 01:50:45PM +0300, Leon Romanovsky wrote: > On Wed, May 13, 2020 at 01:45:36PM +0300, Kamal Heib wrote: > > On Wed, May 13, 2020 at 01:21:32PM +0300, Leon Romanovsky wrote: > > > On Wed, May 13, 2020 at 01:02:04PM +0300, Kamal Heib wrote: > > > > On Wed, May 13, 2020 at 10:22:03AM +0300, Leon Romanovsky wrote: > > > > > On Tue, May 12, 2020 at 01:29:18AM +0300, Kamal Heib wrote: > > > > > > Avoid disabling device management for devices that don't support > > > > > > Management datagrams (MADs) by checking if the "mad_agent" pointer is > > > > > > initialized before calling ib_modify_port, also change the error message > > > > > > to a warning and make it more informative. > > > > > > > > > > > > Fixes: 09f8a1486dca ("RDMA/srpt: Fix handling of SR-IOV and iWARP ports") > > > > > > Signed-off-by: Kamal Heib <kamalheib1@gmail.com> > > > > > > --- > > > > > > drivers/infiniband/ulp/srpt/ib_srpt.c | 8 ++++++-- > > > > > > 1 file changed, 6 insertions(+), 2 deletions(-) > > > > > > > > > > > > diff --git a/drivers/infiniband/ulp/srpt/ib_srpt.c b/drivers/infiniband/ulp/srpt/ib_srpt.c > > > > > > index 7ed38d1cb997..7b21792ab6f7 100644 > > > > > > --- a/drivers/infiniband/ulp/srpt/ib_srpt.c > > > > > > +++ b/drivers/infiniband/ulp/srpt/ib_srpt.c > > > > > > @@ -625,14 +625,18 @@ static void srpt_unregister_mad_agent(struct srpt_device *sdev) > > > > > > .clr_port_cap_mask = IB_PORT_DEVICE_MGMT_SUP, > > > > > > }; > > > > > > struct srpt_port *sport; > > > > > > + int ret; > > > > > > int i; > > > > > > > > > > > > for (i = 1; i <= sdev->device->phys_port_cnt; i++) { > > > > > > sport = &sdev->port[i - 1]; > > > > > > WARN_ON(sport->port != i); > > > > > > - if (ib_modify_port(sdev->device, i, 0, &port_modify) < 0) > > > > > > - pr_err("disabling MAD processing failed.\n"); > > > > > > if (sport->mad_agent) { > > > > > > + ret = ib_modify_port(sdev->device, i, 0, &port_modify); > > > > > > + if (ret < 0) > > > > > > + pr_warn("%s-%d: disabling device management failed (%d). Note: this is expected if SR-IOV is enabled.\n", > > > > > > + dev_name(&sport->sdev->device->dev), > > > > > > > > > > The ib_modify_port() shouldn't be called if it expected to fail. > > > > > > > > > > Thanks > > > > > > > > OK, Do you know if there is a way to check if the created ib device is > > > > for VF to avoid calling ib_modify_port()? > > > > > > The "is_virtfn" field inside pci_dev will give this information, > > > but I don't know why it is expected to fail here. > > > > > > Thanks > > > > > > > Looks like there a more trivial way, I mean checking if > > IB_DEVICE_VIRTUAL_FUNCTION cap is set, probably there is a need to make > > to sure that this cap is set for all providers when the IB device is > > created for a VF. > > > > With regards to why it is expected to fail, as stated in the commit > > message and the in "Fixes" commit message there is some devices (VF) > > that don't support MADs. > > So shouldn't you test that device support MAD instead of testing VF cap? > > Thanks Correct me if I'm wrong, Do you mean check the return value from rdma_cap_ib_mad()? Thanks, Kamal > > > > > Thanks, > > Kamal > > > > > > > > > > Thanks, > > > > Kamal > > > > > > > > > > > > > > > + sport->port, ret); > > > > > > ib_unregister_mad_agent(sport->mad_agent); > > > > > > sport->mad_agent = NULL; > > > > > > } > > > > > > -- > > > > > > 2.25.4 > > > > > > ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH for-rc] RDMA/srpt: Fix disabling device management 2020-05-13 11:14 ` Kamal Heib @ 2020-05-13 11:31 ` Leon Romanovsky 2020-05-13 12:38 ` Kamal Heib 0 siblings, 1 reply; 17+ messages in thread From: Leon Romanovsky @ 2020-05-13 11:31 UTC (permalink / raw) To: Kamal Heib; +Cc: linux-rdma, Doug Ledford, Jason Gunthorpe, Bart Van Assche On Wed, May 13, 2020 at 02:14:35PM +0300, Kamal Heib wrote: > On Wed, May 13, 2020 at 01:50:45PM +0300, Leon Romanovsky wrote: > > On Wed, May 13, 2020 at 01:45:36PM +0300, Kamal Heib wrote: > > > On Wed, May 13, 2020 at 01:21:32PM +0300, Leon Romanovsky wrote: > > > > On Wed, May 13, 2020 at 01:02:04PM +0300, Kamal Heib wrote: > > > > > On Wed, May 13, 2020 at 10:22:03AM +0300, Leon Romanovsky wrote: > > > > > > On Tue, May 12, 2020 at 01:29:18AM +0300, Kamal Heib wrote: > > > > > > > Avoid disabling device management for devices that don't support > > > > > > > Management datagrams (MADs) by checking if the "mad_agent" pointer is > > > > > > > initialized before calling ib_modify_port, also change the error message > > > > > > > to a warning and make it more informative. > > > > > > > > > > > > > > Fixes: 09f8a1486dca ("RDMA/srpt: Fix handling of SR-IOV and iWARP ports") > > > > > > > Signed-off-by: Kamal Heib <kamalheib1@gmail.com> > > > > > > > --- > > > > > > > drivers/infiniband/ulp/srpt/ib_srpt.c | 8 ++++++-- > > > > > > > 1 file changed, 6 insertions(+), 2 deletions(-) > > > > > > > > > > > > > > diff --git a/drivers/infiniband/ulp/srpt/ib_srpt.c b/drivers/infiniband/ulp/srpt/ib_srpt.c > > > > > > > index 7ed38d1cb997..7b21792ab6f7 100644 > > > > > > > --- a/drivers/infiniband/ulp/srpt/ib_srpt.c > > > > > > > +++ b/drivers/infiniband/ulp/srpt/ib_srpt.c > > > > > > > @@ -625,14 +625,18 @@ static void srpt_unregister_mad_agent(struct srpt_device *sdev) > > > > > > > .clr_port_cap_mask = IB_PORT_DEVICE_MGMT_SUP, > > > > > > > }; > > > > > > > struct srpt_port *sport; > > > > > > > + int ret; > > > > > > > int i; > > > > > > > > > > > > > > for (i = 1; i <= sdev->device->phys_port_cnt; i++) { > > > > > > > sport = &sdev->port[i - 1]; > > > > > > > WARN_ON(sport->port != i); > > > > > > > - if (ib_modify_port(sdev->device, i, 0, &port_modify) < 0) > > > > > > > - pr_err("disabling MAD processing failed.\n"); > > > > > > > if (sport->mad_agent) { > > > > > > > + ret = ib_modify_port(sdev->device, i, 0, &port_modify); > > > > > > > + if (ret < 0) > > > > > > > + pr_warn("%s-%d: disabling device management failed (%d). Note: this is expected if SR-IOV is enabled.\n", > > > > > > > + dev_name(&sport->sdev->device->dev), > > > > > > > > > > > > The ib_modify_port() shouldn't be called if it expected to fail. > > > > > > > > > > > > Thanks > > > > > > > > > > OK, Do you know if there is a way to check if the created ib device is > > > > > for VF to avoid calling ib_modify_port()? > > > > > > > > The "is_virtfn" field inside pci_dev will give this information, > > > > but I don't know why it is expected to fail here. > > > > > > > > Thanks > > > > > > > > > > Looks like there a more trivial way, I mean checking if > > > IB_DEVICE_VIRTUAL_FUNCTION cap is set, probably there is a need to make > > > to sure that this cap is set for all providers when the IB device is > > > created for a VF. > > > > > > With regards to why it is expected to fail, as stated in the commit > > > message and the in "Fixes" commit message there is some devices (VF) > > > that don't support MADs. > > > > So shouldn't you test that device support MAD instead of testing VF cap? > > > > Thanks > > Correct me if I'm wrong, Do you mean check the return value from > rdma_cap_ib_mad()? I think so. Thanks > > Thanks, > Kamal > > > > > > > > Thanks, > > > Kamal > > > > > > > > > > > > > Thanks, > > > > > Kamal > > > > > > > > > > > > > > > > > > + sport->port, ret); > > > > > > > ib_unregister_mad_agent(sport->mad_agent); > > > > > > > sport->mad_agent = NULL; > > > > > > > } > > > > > > > -- > > > > > > > 2.25.4 > > > > > > > ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH for-rc] RDMA/srpt: Fix disabling device management 2020-05-13 11:31 ` Leon Romanovsky @ 2020-05-13 12:38 ` Kamal Heib 2020-05-13 12:43 ` Jason Gunthorpe 0 siblings, 1 reply; 17+ messages in thread From: Kamal Heib @ 2020-05-13 12:38 UTC (permalink / raw) To: Leon Romanovsky Cc: linux-rdma, Doug Ledford, Jason Gunthorpe, Bart Van Assche On Wed, May 13, 2020 at 02:31:18PM +0300, Leon Romanovsky wrote: > On Wed, May 13, 2020 at 02:14:35PM +0300, Kamal Heib wrote: > > On Wed, May 13, 2020 at 01:50:45PM +0300, Leon Romanovsky wrote: > > > On Wed, May 13, 2020 at 01:45:36PM +0300, Kamal Heib wrote: > > > > On Wed, May 13, 2020 at 01:21:32PM +0300, Leon Romanovsky wrote: > > > > > On Wed, May 13, 2020 at 01:02:04PM +0300, Kamal Heib wrote: > > > > > > On Wed, May 13, 2020 at 10:22:03AM +0300, Leon Romanovsky wrote: > > > > > > > On Tue, May 12, 2020 at 01:29:18AM +0300, Kamal Heib wrote: > > > > > > > > Avoid disabling device management for devices that don't support > > > > > > > > Management datagrams (MADs) by checking if the "mad_agent" pointer is > > > > > > > > initialized before calling ib_modify_port, also change the error message > > > > > > > > to a warning and make it more informative. > > > > > > > > > > > > > > > > Fixes: 09f8a1486dca ("RDMA/srpt: Fix handling of SR-IOV and iWARP ports") > > > > > > > > Signed-off-by: Kamal Heib <kamalheib1@gmail.com> > > > > > > > > --- > > > > > > > > drivers/infiniband/ulp/srpt/ib_srpt.c | 8 ++++++-- > > > > > > > > 1 file changed, 6 insertions(+), 2 deletions(-) > > > > > > > > > > > > > > > > diff --git a/drivers/infiniband/ulp/srpt/ib_srpt.c b/drivers/infiniband/ulp/srpt/ib_srpt.c > > > > > > > > index 7ed38d1cb997..7b21792ab6f7 100644 > > > > > > > > --- a/drivers/infiniband/ulp/srpt/ib_srpt.c > > > > > > > > +++ b/drivers/infiniband/ulp/srpt/ib_srpt.c > > > > > > > > @@ -625,14 +625,18 @@ static void srpt_unregister_mad_agent(struct srpt_device *sdev) > > > > > > > > .clr_port_cap_mask = IB_PORT_DEVICE_MGMT_SUP, > > > > > > > > }; > > > > > > > > struct srpt_port *sport; > > > > > > > > + int ret; > > > > > > > > int i; > > > > > > > > > > > > > > > > for (i = 1; i <= sdev->device->phys_port_cnt; i++) { > > > > > > > > sport = &sdev->port[i - 1]; > > > > > > > > WARN_ON(sport->port != i); > > > > > > > > - if (ib_modify_port(sdev->device, i, 0, &port_modify) < 0) > > > > > > > > - pr_err("disabling MAD processing failed.\n"); > > > > > > > > if (sport->mad_agent) { > > > > > > > > + ret = ib_modify_port(sdev->device, i, 0, &port_modify); > > > > > > > > + if (ret < 0) > > > > > > > > + pr_warn("%s-%d: disabling device management failed (%d). Note: this is expected if SR-IOV is enabled.\n", > > > > > > > > + dev_name(&sport->sdev->device->dev), > > > > > > > > > > > > > > The ib_modify_port() shouldn't be called if it expected to fail. > > > > > > > > > > > > > > Thanks > > > > > > > > > > > > OK, Do you know if there is a way to check if the created ib device is > > > > > > for VF to avoid calling ib_modify_port()? > > > > > > > > > > The "is_virtfn" field inside pci_dev will give this information, > > > > > but I don't know why it is expected to fail here. > > > > > > > > > > Thanks > > > > > > > > > > > > > Looks like there a more trivial way, I mean checking if > > > > IB_DEVICE_VIRTUAL_FUNCTION cap is set, probably there is a need to make > > > > to sure that this cap is set for all providers when the IB device is > > > > created for a VF. > > > > > > > > With regards to why it is expected to fail, as stated in the commit > > > > message and the in "Fixes" commit message there is some devices (VF) > > > > that don't support MADs. > > > > > > So shouldn't you test that device support MAD instead of testing VF cap? > > > > > > Thanks > > > > Correct me if I'm wrong, Do you mean check the return value from > > rdma_cap_ib_mad()? > > I think so. > > Thanks > Well, this function will not help in the case of VFs, because the flag that is checked in rdma_cap_ib_mad() is RDMA_CORE_CAP_IB_MAD which is set almost for each create IB device as part of RDMA_CORE_PORT_IBA_IB or RDMA_CORE_PORT_IBA_ROCE or RDMA_CORE_PORT_IBA_ROCE_UDP_ENCAP. Thanks, Kamal > > > > Thanks, > > Kamal > > > > > > > > > > > Thanks, > > > > Kamal > > > > > > > > > > > > > > > > Thanks, > > > > > > Kamal > > > > > > > > > > > > > > > > > > > > > + sport->port, ret); > > > > > > > > ib_unregister_mad_agent(sport->mad_agent); > > > > > > > > sport->mad_agent = NULL; > > > > > > > > } > > > > > > > > -- > > > > > > > > 2.25.4 > > > > > > > > ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH for-rc] RDMA/srpt: Fix disabling device management 2020-05-13 12:38 ` Kamal Heib @ 2020-05-13 12:43 ` Jason Gunthorpe 2020-05-13 15:25 ` Bart Van Assche 0 siblings, 1 reply; 17+ messages in thread From: Jason Gunthorpe @ 2020-05-13 12:43 UTC (permalink / raw) To: Kamal Heib; +Cc: Leon Romanovsky, linux-rdma, Doug Ledford, Bart Van Assche On Wed, May 13, 2020 at 03:38:37PM +0300, Kamal Heib wrote: > > > Correct me if I'm wrong, Do you mean check the return value from > > > rdma_cap_ib_mad()? > > > > I think so. > > > > Thanks > > Well, this function will not help in the case of VFs, because the flag > that is checked in rdma_cap_ib_mad() is RDMA_CORE_CAP_IB_MAD which is > set almost for each create IB device as part of RDMA_CORE_PORT_IBA_IB or > RDMA_CORE_PORT_IBA_ROCE or RDMA_CORE_PORT_IBA_ROCE_UDP_ENCAP. AFAIK this case only happens for mlx4 devices that use the GUID table to emulate virtual IB ports. In this case there is no bit to control. I didn't quite understand why srpt has this stuff, does it mean it is broken on mlx4 vports? Why allow the failure? Jason ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH for-rc] RDMA/srpt: Fix disabling device management 2020-05-13 12:43 ` Jason Gunthorpe @ 2020-05-13 15:25 ` Bart Van Assche 2020-05-13 18:02 ` Jason Gunthorpe 0 siblings, 1 reply; 17+ messages in thread From: Bart Van Assche @ 2020-05-13 15:25 UTC (permalink / raw) To: Jason Gunthorpe, Kamal Heib; +Cc: Leon Romanovsky, linux-rdma, Doug Ledford On 2020-05-13 05:43, Jason Gunthorpe wrote: > On Wed, May 13, 2020 at 03:38:37PM +0300, Kamal Heib wrote: >>>> Correct me if I'm wrong, Do you mean check the return value from >>>> rdma_cap_ib_mad()? >>> >>> I think so. >>> >>> Thanks >> >> Well, this function will not help in the case of VFs, because the flag >> that is checked in rdma_cap_ib_mad() is RDMA_CORE_CAP_IB_MAD which is >> set almost for each create IB device as part of RDMA_CORE_PORT_IBA_IB or >> RDMA_CORE_PORT_IBA_ROCE or RDMA_CORE_PORT_IBA_ROCE_UDP_ENCAP. > > AFAIK this case only happens for mlx4 devices that use the GUID table > to emulate virtual IB ports. In this case there is no bit to control. > > I didn't quite understand why srpt has this stuff, does it mean it is > broken on mlx4 vports? Why allow the failure? The commit message of the code that introduced the most recent IB_PORT_DEVICE_MGMT_SUP changes is as follows: ----------------------------------------------------------------------- commit 09f8a1486dcaf69753961a6df6cffdaafc5ccbcb Author: Bart Van Assche <bvanassche@acm.org> Date: Mon Sep 30 16:17:01 2019 -0700 RDMA/srpt: Fix handling of SR-IOV and iWARP ports Management datagrams (MADs) are not supported by SR-IOV VFs nor by iWARP ports. Support SR-IOV VFs and iWARP ports by only logging an error message if MAD handler registration fails. ----------------------------------------------------------------------- In other words, on my setup the ib_srpt driver was working find with SR-IOV. Bart. ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH for-rc] RDMA/srpt: Fix disabling device management 2020-05-13 15:25 ` Bart Van Assche @ 2020-05-13 18:02 ` Jason Gunthorpe 2020-05-13 18:07 ` Bart Van Assche 0 siblings, 1 reply; 17+ messages in thread From: Jason Gunthorpe @ 2020-05-13 18:02 UTC (permalink / raw) To: Bart Van Assche; +Cc: Kamal Heib, Leon Romanovsky, linux-rdma, Doug Ledford On Wed, May 13, 2020 at 08:25:41AM -0700, Bart Van Assche wrote: > On 2020-05-13 05:43, Jason Gunthorpe wrote: > > On Wed, May 13, 2020 at 03:38:37PM +0300, Kamal Heib wrote: > >>>> Correct me if I'm wrong, Do you mean check the return value from > >>>> rdma_cap_ib_mad()? > >>> > >>> I think so. > >>> > >>> Thanks > >> > >> Well, this function will not help in the case of VFs, because the flag > >> that is checked in rdma_cap_ib_mad() is RDMA_CORE_CAP_IB_MAD which is > >> set almost for each create IB device as part of RDMA_CORE_PORT_IBA_IB or > >> RDMA_CORE_PORT_IBA_ROCE or RDMA_CORE_PORT_IBA_ROCE_UDP_ENCAP. > > > > AFAIK this case only happens for mlx4 devices that use the GUID table > > to emulate virtual IB ports. In this case there is no bit to control. > > > > I didn't quite understand why srpt has this stuff, does it mean it is > > broken on mlx4 vports? Why allow the failure? > > The commit message of the code that introduced the most recent > IB_PORT_DEVICE_MGMT_SUP changes is as follows: > > commit 09f8a1486dcaf69753961a6df6cffdaafc5ccbcb > Author: Bart Van Assche <bvanassche@acm.org> > Date: Mon Sep 30 16:17:01 2019 -0700 > > RDMA/srpt: Fix handling of SR-IOV and iWARP ports > > Management datagrams (MADs) are not supported by SR-IOV VFs nor by iWARP > ports. Support SR-IOV VFs and iWARP ports by only logging an error > message if MAD handler registration fails. > > In other words, on my setup the ib_srpt driver was working find with SR-IOV. But it won't be properly discoverable without the IB_PORT_DEVICE_MGMT_SUP flag being set on the physical port? Jason ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH for-rc] RDMA/srpt: Fix disabling device management 2020-05-13 18:02 ` Jason Gunthorpe @ 2020-05-13 18:07 ` Bart Van Assche 2020-05-13 18:08 ` Jason Gunthorpe 0 siblings, 1 reply; 17+ messages in thread From: Bart Van Assche @ 2020-05-13 18:07 UTC (permalink / raw) To: Jason Gunthorpe; +Cc: Kamal Heib, Leon Romanovsky, linux-rdma, Doug Ledford On 2020-05-13 11:02, Jason Gunthorpe wrote: > On Wed, May 13, 2020 at 08:25:41AM -0700, Bart Van Assche wrote: >> In other words, on my setup the ib_srpt driver was working find with SR-IOV. > > But it won't be properly discoverable without the > IB_PORT_DEVICE_MGMT_SUP flag being set on the physical port? That matches my understanding. Even if srp_daemon can't discover an SRP target, it is still possible to log in by writing the proper login parameters into /sys/class/infiniband_srp/*/add_target. Bart. ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH for-rc] RDMA/srpt: Fix disabling device management 2020-05-13 18:07 ` Bart Van Assche @ 2020-05-13 18:08 ` Jason Gunthorpe 0 siblings, 0 replies; 17+ messages in thread From: Jason Gunthorpe @ 2020-05-13 18:08 UTC (permalink / raw) To: Bart Van Assche; +Cc: Kamal Heib, Leon Romanovsky, linux-rdma, Doug Ledford On Wed, May 13, 2020 at 11:07:10AM -0700, Bart Van Assche wrote: > On 2020-05-13 11:02, Jason Gunthorpe wrote: > > On Wed, May 13, 2020 at 08:25:41AM -0700, Bart Van Assche wrote: > >> In other words, on my setup the ib_srpt driver was working find with SR-IOV. > > > > But it won't be properly discoverable without the > > IB_PORT_DEVICE_MGMT_SUP flag being set on the physical port? > > That matches my understanding. Even if srp_daemon can't discover an SRP > target, it is still possible to log in by writing the proper login > parameters into /sys/class/infiniband_srp/*/add_target. It is probably worth revising the warning message to reflect the actual harm Jason ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH for-rc] RDMA/srpt: Fix disabling device management 2020-05-13 10:45 ` Kamal Heib 2020-05-13 10:50 ` Leon Romanovsky @ 2020-05-13 11:59 ` Gal Pressman 2020-05-13 12:54 ` Kamal Heib 1 sibling, 1 reply; 17+ messages in thread From: Gal Pressman @ 2020-05-13 11:59 UTC (permalink / raw) To: Kamal Heib, Leon Romanovsky Cc: linux-rdma, Doug Ledford, Jason Gunthorpe, Bart Van Assche On 13/05/2020 13:45, Kamal Heib wrote: > On Wed, May 13, 2020 at 01:21:32PM +0300, Leon Romanovsky wrote: >> On Wed, May 13, 2020 at 01:02:04PM +0300, Kamal Heib wrote: >>> On Wed, May 13, 2020 at 10:22:03AM +0300, Leon Romanovsky wrote: >>>> On Tue, May 12, 2020 at 01:29:18AM +0300, Kamal Heib wrote: >>>>> Avoid disabling device management for devices that don't support >>>>> Management datagrams (MADs) by checking if the "mad_agent" pointer is >>>>> initialized before calling ib_modify_port, also change the error message >>>>> to a warning and make it more informative. >>>>> >>>>> Fixes: 09f8a1486dca ("RDMA/srpt: Fix handling of SR-IOV and iWARP ports") >>>>> Signed-off-by: Kamal Heib <kamalheib1@gmail.com> >>>>> --- >>>>> drivers/infiniband/ulp/srpt/ib_srpt.c | 8 ++++++-- >>>>> 1 file changed, 6 insertions(+), 2 deletions(-) >>>>> >>>>> diff --git a/drivers/infiniband/ulp/srpt/ib_srpt.c b/drivers/infiniband/ulp/srpt/ib_srpt.c >>>>> index 7ed38d1cb997..7b21792ab6f7 100644 >>>>> --- a/drivers/infiniband/ulp/srpt/ib_srpt.c >>>>> +++ b/drivers/infiniband/ulp/srpt/ib_srpt.c >>>>> @@ -625,14 +625,18 @@ static void srpt_unregister_mad_agent(struct srpt_device *sdev) >>>>> .clr_port_cap_mask = IB_PORT_DEVICE_MGMT_SUP, >>>>> }; >>>>> struct srpt_port *sport; >>>>> + int ret; >>>>> int i; >>>>> >>>>> for (i = 1; i <= sdev->device->phys_port_cnt; i++) { >>>>> sport = &sdev->port[i - 1]; >>>>> WARN_ON(sport->port != i); >>>>> - if (ib_modify_port(sdev->device, i, 0, &port_modify) < 0) >>>>> - pr_err("disabling MAD processing failed.\n"); >>>>> if (sport->mad_agent) { >>>>> + ret = ib_modify_port(sdev->device, i, 0, &port_modify); >>>>> + if (ret < 0) >>>>> + pr_warn("%s-%d: disabling device management failed (%d). Note: this is expected if SR-IOV is enabled.\n", >>>>> + dev_name(&sport->sdev->device->dev), >>>> >>>> The ib_modify_port() shouldn't be called if it expected to fail. >>>> >>>> Thanks >>> >>> OK, Do you know if there is a way to check if the created ib device is >>> for VF to avoid calling ib_modify_port()? >> >> The "is_virtfn" field inside pci_dev will give this information, >> but I don't know why it is expected to fail here. >> >> Thanks >> > > Looks like there a more trivial way, I mean checking if > IB_DEVICE_VIRTUAL_FUNCTION cap is set, probably there is a need to make > to sure that this cap is set for all providers when the IB device is > created for a VF. It's not, I think this bit is used for ipoib stuff only or something? ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH for-rc] RDMA/srpt: Fix disabling device management 2020-05-13 11:59 ` Gal Pressman @ 2020-05-13 12:54 ` Kamal Heib 0 siblings, 0 replies; 17+ messages in thread From: Kamal Heib @ 2020-05-13 12:54 UTC (permalink / raw) To: Gal Pressman Cc: Leon Romanovsky, linux-rdma, Doug Ledford, Jason Gunthorpe, Bart Van Assche On Wed, May 13, 2020 at 02:59:21PM +0300, Gal Pressman wrote: > On 13/05/2020 13:45, Kamal Heib wrote: > > On Wed, May 13, 2020 at 01:21:32PM +0300, Leon Romanovsky wrote: > >> On Wed, May 13, 2020 at 01:02:04PM +0300, Kamal Heib wrote: > >>> On Wed, May 13, 2020 at 10:22:03AM +0300, Leon Romanovsky wrote: > >>>> On Tue, May 12, 2020 at 01:29:18AM +0300, Kamal Heib wrote: > >>>>> Avoid disabling device management for devices that don't support > >>>>> Management datagrams (MADs) by checking if the "mad_agent" pointer is > >>>>> initialized before calling ib_modify_port, also change the error message > >>>>> to a warning and make it more informative. > >>>>> > >>>>> Fixes: 09f8a1486dca ("RDMA/srpt: Fix handling of SR-IOV and iWARP ports") > >>>>> Signed-off-by: Kamal Heib <kamalheib1@gmail.com> > >>>>> --- > >>>>> drivers/infiniband/ulp/srpt/ib_srpt.c | 8 ++++++-- > >>>>> 1 file changed, 6 insertions(+), 2 deletions(-) > >>>>> > >>>>> diff --git a/drivers/infiniband/ulp/srpt/ib_srpt.c b/drivers/infiniband/ulp/srpt/ib_srpt.c > >>>>> index 7ed38d1cb997..7b21792ab6f7 100644 > >>>>> --- a/drivers/infiniband/ulp/srpt/ib_srpt.c > >>>>> +++ b/drivers/infiniband/ulp/srpt/ib_srpt.c > >>>>> @@ -625,14 +625,18 @@ static void srpt_unregister_mad_agent(struct srpt_device *sdev) > >>>>> .clr_port_cap_mask = IB_PORT_DEVICE_MGMT_SUP, > >>>>> }; > >>>>> struct srpt_port *sport; > >>>>> + int ret; > >>>>> int i; > >>>>> > >>>>> for (i = 1; i <= sdev->device->phys_port_cnt; i++) { > >>>>> sport = &sdev->port[i - 1]; > >>>>> WARN_ON(sport->port != i); > >>>>> - if (ib_modify_port(sdev->device, i, 0, &port_modify) < 0) > >>>>> - pr_err("disabling MAD processing failed.\n"); > >>>>> if (sport->mad_agent) { > >>>>> + ret = ib_modify_port(sdev->device, i, 0, &port_modify); > >>>>> + if (ret < 0) > >>>>> + pr_warn("%s-%d: disabling device management failed (%d). Note: this is expected if SR-IOV is enabled.\n", > >>>>> + dev_name(&sport->sdev->device->dev), > >>>> > >>>> The ib_modify_port() shouldn't be called if it expected to fail. > >>>> > >>>> Thanks > >>> > >>> OK, Do you know if there is a way to check if the created ib device is > >>> for VF to avoid calling ib_modify_port()? > >> > >> The "is_virtfn" field inside pci_dev will give this information, > >> but I don't know why it is expected to fail here. > >> > >> Thanks > >> > > > > Looks like there a more trivial way, I mean checking if > > IB_DEVICE_VIRTUAL_FUNCTION cap is set, probably there is a need to make > > to sure that this cap is set for all providers when the IB device is > > created for a VF. > > It's not, I think this bit is used for ipoib stuff only or something? Yes, this cap is set only by the mlx5_ib driver for the IPoIB enhanced mode. Thanks, Kamal ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH for-rc] RDMA/srpt: Fix disabling device management 2020-05-11 22:29 [PATCH for-rc] RDMA/srpt: Fix disabling device management Kamal Heib 2020-05-13 7:22 ` Leon Romanovsky @ 2020-05-13 12:48 ` Jason Gunthorpe 1 sibling, 0 replies; 17+ messages in thread From: Jason Gunthorpe @ 2020-05-13 12:48 UTC (permalink / raw) To: Kamal Heib; +Cc: linux-rdma, Doug Ledford, Bart Van Assche On Tue, May 12, 2020 at 01:29:18AM +0300, Kamal Heib wrote: > Avoid disabling device management for devices that don't support > Management datagrams (MADs) by checking if the "mad_agent" pointer is > initialized before calling ib_modify_port, also change the error message > to a warning and make it more informative. > > Fixes: 09f8a1486dca ("RDMA/srpt: Fix handling of SR-IOV and iWARP ports") > Signed-off-by: Kamal Heib <kamalheib1@gmail.com> > drivers/infiniband/ulp/srpt/ib_srpt.c | 8 ++++++-- > 1 file changed, 6 insertions(+), 2 deletions(-) > > diff --git a/drivers/infiniband/ulp/srpt/ib_srpt.c b/drivers/infiniband/ulp/srpt/ib_srpt.c > index 7ed38d1cb997..7b21792ab6f7 100644 > +++ b/drivers/infiniband/ulp/srpt/ib_srpt.c > @@ -625,14 +625,18 @@ static void srpt_unregister_mad_agent(struct srpt_device *sdev) > .clr_port_cap_mask = IB_PORT_DEVICE_MGMT_SUP, > }; > struct srpt_port *sport; > + int ret; > int i; > > for (i = 1; i <= sdev->device->phys_port_cnt; i++) { > sport = &sdev->port[i - 1]; > WARN_ON(sport->port != i); > - if (ib_modify_port(sdev->device, i, 0, &port_modify) < 0) > - pr_err("disabling MAD processing failed.\n"); > if (sport->mad_agent) { > + ret = ib_modify_port(sdev->device, i, 0, &port_modify); > + if (ret < 0) > + pr_warn("%s-%d: disabling device management failed (%d). Note: this is expected if SR-IOV is enabled.\n", > + dev_name(&sport->sdev->device->dev), > + sport->port, ret); This ib_modify_port needs to be strictly paired with the ib_modify_port that turns on IB_PORT_DEVICE_MGMT_SUP. If the call during register fails then we should not try to do it during unregister. So this is sort of the right approach, but the error unwind in srpt_refresh_port() needs to be fixed too so that sport->mad_agent indicates if modify_port is needed on unregister. And the print is wrong, if something fails here it means the driver is busted up as we should always be able to undo setting IB_PORT_DEVICE_MGMT_SUP if it was successfully set. Jason ^ permalink raw reply [flat|nested] 17+ messages in thread
end of thread, other threads:[~2020-05-13 18:08 UTC | newest] Thread overview: 17+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-05-11 22:29 [PATCH for-rc] RDMA/srpt: Fix disabling device management Kamal Heib 2020-05-13 7:22 ` Leon Romanovsky 2020-05-13 10:02 ` Kamal Heib 2020-05-13 10:21 ` Leon Romanovsky 2020-05-13 10:45 ` Kamal Heib 2020-05-13 10:50 ` Leon Romanovsky 2020-05-13 11:14 ` Kamal Heib 2020-05-13 11:31 ` Leon Romanovsky 2020-05-13 12:38 ` Kamal Heib 2020-05-13 12:43 ` Jason Gunthorpe 2020-05-13 15:25 ` Bart Van Assche 2020-05-13 18:02 ` Jason Gunthorpe 2020-05-13 18:07 ` Bart Van Assche 2020-05-13 18:08 ` Jason Gunthorpe 2020-05-13 11:59 ` Gal Pressman 2020-05-13 12:54 ` Kamal Heib 2020-05-13 12:48 ` Jason Gunthorpe
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.