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

[Kamal Heib <kamalheib1@gmail.com>]

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