| Message ID | 20200511222918.62576-1-kamalheib1@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Jason Gunthorpe |
| Headers | show |
| Series | [for-rc] RDMA/srpt: Fix disabling device management | expand |
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 >
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 > >
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 > > >
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 > > > >
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 > > > > >
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 > > > > > >
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 > > > > > > >
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?
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 > > > > > > > >
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
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
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
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.
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
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.
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
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; }
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(-)