[PATCH for-rc] RDMA/srpt: Fix disabling device management

[PATCH for-rc] RDMA/srpt: Fix disabling device management

[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

Re: [PATCH for-rc] RDMA/srpt: Fix disabling device management

[Leon Romanovsky]

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
>

Re: [PATCH for-rc] RDMA/srpt: Fix disabling device management

[Kamal Heib]

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
> >

Re: [PATCH for-rc] RDMA/srpt: Fix disabling device management

[Leon Romanovsky]

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
> > >

Re: [PATCH for-rc] RDMA/srpt: Fix disabling device management

[Kamal Heib]

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
> > > >

Re: [PATCH for-rc] RDMA/srpt: Fix disabling device management

[Leon Romanovsky]

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
> > > > >

Re: [PATCH for-rc] RDMA/srpt: Fix disabling device management

[Kamal Heib]

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
> > > > > >

Re: [PATCH for-rc] RDMA/srpt: Fix disabling device management

[Leon Romanovsky]

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
> > > > > > >

Re: [PATCH for-rc] RDMA/srpt: Fix disabling device management

[Gal Pressman]

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?

Re: [PATCH for-rc] RDMA/srpt: Fix disabling device management

[Kamal Heib]

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
> > > > > > > >

Re: [PATCH for-rc] RDMA/srpt: Fix disabling device management

[Jason Gunthorpe]

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

Re: [PATCH for-rc] RDMA/srpt: Fix disabling device management

[Jason Gunthorpe]

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

Re: [PATCH for-rc] RDMA/srpt: Fix disabling device management

[Kamal Heib]

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

Re: [PATCH for-rc] RDMA/srpt: Fix disabling device management

[Bart Van Assche]

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.

Re: [PATCH for-rc] RDMA/srpt: Fix disabling device management

[Jason Gunthorpe]

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

Re: [PATCH for-rc] RDMA/srpt: Fix disabling device management

[Bart Van Assche]

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.

Re: [PATCH for-rc] RDMA/srpt: Fix disabling device management

[Jason Gunthorpe]

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