Linux-RDMA Archive on lore.kernel.org
 help / color / Atom feed
* New GID query API broke EFA
@ 2020-10-22 10:58 Gal Pressman
  2020-10-22 11:21 ` Leon Romanovsky
  0 siblings, 1 reply; 9+ messages in thread
From: Gal Pressman @ 2020-10-22 10:58 UTC (permalink / raw)
  To: Leon Romanovsky, Avihai Horon, Maor Gottlieb, Jason Gunthorpe
  Cc: linux-rdma, Leybovich, Yossi

Hi all,

The new IOCTL query GID API 9f85cbe50aa0 ("RDMA/uverbs: Expose the new GID query
API to user space") currently breaks EFA, as ibv_query_gid() no longer works.

The problem is that the IOCTL call checks for:
	if (!rdma_ib_or_roce(ib_dev, port_num))
		return -EOPNOTSUPP;

EFA is neither of these, but it uses GIDs.

Any objections to remove the check? Any other solutions come to mind?

Gal

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: New GID query API broke EFA
  2020-10-22 10:58 New GID query API broke EFA Gal Pressman
@ 2020-10-22 11:21 ` Leon Romanovsky
  2020-10-22 11:49   ` Gal Pressman
  2020-10-22 12:10   ` Jason Gunthorpe
  0 siblings, 2 replies; 9+ messages in thread
From: Leon Romanovsky @ 2020-10-22 11:21 UTC (permalink / raw)
  To: Gal Pressman
  Cc: Avihai Horon, Maor Gottlieb, Jason Gunthorpe, linux-rdma,
	Leybovich, Yossi

On Thu, Oct 22, 2020 at 01:58:29PM +0300, Gal Pressman wrote:
> Hi all,
>
> The new IOCTL query GID API 9f85cbe50aa0 ("RDMA/uverbs: Expose the new GID query
> API to user space") currently breaks EFA, as ibv_query_gid() no longer works.
>
> The problem is that the IOCTL call checks for:
> 	if (!rdma_ib_or_roce(ib_dev, port_num))
> 		return -EOPNOTSUPP;
>
> EFA is neither of these, but it uses GIDs.
>
> Any objections to remove the check? Any other solutions come to mind?

We added this check to protect access to rdma_get_gid_attr() for devices
without GID table.
 1234         table = rdma_gid_table(device, port_num);
 1235         if (index < 0 || index >= table->sz)
 1236                 return ERR_PTR(-EINVAL);

So you can extend that function to return for table == NULL an error and
remove rdma_ib_or_roce()

Thanks

>
> Gal

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: New GID query API broke EFA
  2020-10-22 11:21 ` Leon Romanovsky
@ 2020-10-22 11:49   ` Gal Pressman
  2020-10-22 12:10   ` Jason Gunthorpe
  1 sibling, 0 replies; 9+ messages in thread
From: Gal Pressman @ 2020-10-22 11:49 UTC (permalink / raw)
  To: Leon Romanovsky
  Cc: Avihai Horon, Maor Gottlieb, Jason Gunthorpe, linux-rdma,
	Leybovich, Yossi

On 22/10/2020 14:21, Leon Romanovsky wrote:
> On Thu, Oct 22, 2020 at 01:58:29PM +0300, Gal Pressman wrote:
>> Hi all,
>>
>> The new IOCTL query GID API 9f85cbe50aa0 ("RDMA/uverbs: Expose the new GID query
>> API to user space") currently breaks EFA, as ibv_query_gid() no longer works.
>>
>> The problem is that the IOCTL call checks for:
>>       if (!rdma_ib_or_roce(ib_dev, port_num))
>>               return -EOPNOTSUPP;
>>
>> EFA is neither of these, but it uses GIDs.
>>
>> Any objections to remove the check? Any other solutions come to mind?
> 
> We added this check to protect access to rdma_get_gid_attr() for devices
> without GID table.
>  1234         table = rdma_gid_table(device, port_num);
>  1235         if (index < 0 || index >= table->sz)
>  1236                 return ERR_PTR(-EINVAL);
> 
> So you can extend that function to return for table == NULL an error and
> remove rdma_ib_or_roce()

Thanks Leon, I will send a patch.

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: New GID query API broke EFA
  2020-10-22 11:21 ` Leon Romanovsky
  2020-10-22 11:49   ` Gal Pressman
@ 2020-10-22 12:10   ` Jason Gunthorpe
  2020-10-22 12:12     ` Gal Pressman
  2020-10-22 12:23     ` Leon Romanovsky
  1 sibling, 2 replies; 9+ messages in thread
From: Jason Gunthorpe @ 2020-10-22 12:10 UTC (permalink / raw)
  To: Leon Romanovsky
  Cc: Gal Pressman, Avihai Horon, Maor Gottlieb, linux-rdma, Leybovich, Yossi

On Thu, Oct 22, 2020 at 02:21:00PM +0300, Leon Romanovsky wrote:
> On Thu, Oct 22, 2020 at 01:58:29PM +0300, Gal Pressman wrote:
> > Hi all,
> >
> > The new IOCTL query GID API 9f85cbe50aa0 ("RDMA/uverbs: Expose the new GID query
> > API to user space") currently breaks EFA, as ibv_query_gid() no longer works.
> >
> > The problem is that the IOCTL call checks for:
> > 	if (!rdma_ib_or_roce(ib_dev, port_num))
> > 		return -EOPNOTSUPP;
> >
> > EFA is neither of these, but it uses GIDs.
> >
> > Any objections to remove the check? Any other solutions come to mind?
> 
> We added this check to protect access to rdma_get_gid_attr() for devices
> without GID table.
>  1234         table = rdma_gid_table(device, port_num);
>  1235         if (index < 0 || index >= table->sz)
>  1236                 return ERR_PTR(-EINVAL);
>
> So you can extend that function to return for table == NULL an error and
> remove rdma_ib_or_roce()

How does table == NULL ever?

Jason

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: New GID query API broke EFA
  2020-10-22 12:10   ` Jason Gunthorpe
@ 2020-10-22 12:12     ` Gal Pressman
  2020-10-22 12:15       ` Gal Pressman
  2020-10-22 12:23     ` Leon Romanovsky
  1 sibling, 1 reply; 9+ messages in thread
From: Gal Pressman @ 2020-10-22 12:12 UTC (permalink / raw)
  To: Jason Gunthorpe, Leon Romanovsky
  Cc: Avihai Horon, Maor Gottlieb, linux-rdma, Leybovich, Yossi

On 22/10/2020 15:10, Jason Gunthorpe wrote:
> On Thu, Oct 22, 2020 at 02:21:00PM +0300, Leon Romanovsky wrote:
>> On Thu, Oct 22, 2020 at 01:58:29PM +0300, Gal Pressman wrote:
>>> Hi all,
>>>
>>> The new IOCTL query GID API 9f85cbe50aa0 ("RDMA/uverbs: Expose the new GID query
>>> API to user space") currently breaks EFA, as ibv_query_gid() no longer works.
>>>
>>> The problem is that the IOCTL call checks for:
>>>     if (!rdma_ib_or_roce(ib_dev, port_num))
>>>             return -EOPNOTSUPP;
>>>
>>> EFA is neither of these, but it uses GIDs.
>>>
>>> Any objections to remove the check? Any other solutions come to mind?
>>
>> We added this check to protect access to rdma_get_gid_attr() for devices
>> without GID table.
>>  1234         table = rdma_gid_table(device, port_num);
>>  1235         if (index < 0 || index >= table->sz)
>>  1236                 return ERR_PTR(-EINVAL);
>>
>> So you can extend that function to return for table == NULL an error and
>> remove rdma_ib_or_roce()
> 
> How does table == NULL ever?

A driver with gid_tbl_len == 0 would make the allocation return NULL, no?

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: New GID query API broke EFA
  2020-10-22 12:12     ` Gal Pressman
@ 2020-10-22 12:15       ` Gal Pressman
  2020-10-22 12:23         ` Leon Romanovsky
  0 siblings, 1 reply; 9+ messages in thread
From: Gal Pressman @ 2020-10-22 12:15 UTC (permalink / raw)
  To: Jason Gunthorpe, Leon Romanovsky
  Cc: Avihai Horon, Maor Gottlieb, linux-rdma, Leybovich, Yossi

On 22/10/2020 15:12, Gal Pressman wrote:
> On 22/10/2020 15:10, Jason Gunthorpe wrote:
>> On Thu, Oct 22, 2020 at 02:21:00PM +0300, Leon Romanovsky wrote:
>>> On Thu, Oct 22, 2020 at 01:58:29PM +0300, Gal Pressman wrote:
>>>> Hi all,
>>>>
>>>> The new IOCTL query GID API 9f85cbe50aa0 ("RDMA/uverbs: Expose the new GID query
>>>> API to user space") currently breaks EFA, as ibv_query_gid() no longer works.
>>>>
>>>> The problem is that the IOCTL call checks for:
>>>>     if (!rdma_ib_or_roce(ib_dev, port_num))
>>>>             return -EOPNOTSUPP;
>>>>
>>>> EFA is neither of these, but it uses GIDs.
>>>>
>>>> Any objections to remove the check? Any other solutions come to mind?
>>>
>>> We added this check to protect access to rdma_get_gid_attr() for devices
>>> without GID table.
>>>  1234         table = rdma_gid_table(device, port_num);
>>>  1235         if (index < 0 || index >= table->sz)
>>>  1236                 return ERR_PTR(-EINVAL);
>>>
>>> So you can extend that function to return for table == NULL an error and
>>> remove rdma_ib_or_roce()
>>
>> How does table == NULL ever?
> 
> A driver with gid_tbl_len == 0 would make the allocation return NULL, no?

Nevermind, we don't really allocate with gid_tbl_len as the size, and any
allocation failure will fail ib_register_device().
So I guess there is no way for it to be NULL.

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: New GID query API broke EFA
  2020-10-22 12:10   ` Jason Gunthorpe
  2020-10-22 12:12     ` Gal Pressman
@ 2020-10-22 12:23     ` Leon Romanovsky
  2020-10-22 16:37       ` Jason Gunthorpe
  1 sibling, 1 reply; 9+ messages in thread
From: Leon Romanovsky @ 2020-10-22 12:23 UTC (permalink / raw)
  To: Jason Gunthorpe
  Cc: Gal Pressman, Avihai Horon, Maor Gottlieb, linux-rdma, Leybovich, Yossi

On Thu, Oct 22, 2020 at 09:10:35AM -0300, Jason Gunthorpe wrote:
> On Thu, Oct 22, 2020 at 02:21:00PM +0300, Leon Romanovsky wrote:
> > On Thu, Oct 22, 2020 at 01:58:29PM +0300, Gal Pressman wrote:
> > > Hi all,
> > >
> > > The new IOCTL query GID API 9f85cbe50aa0 ("RDMA/uverbs: Expose the new GID query
> > > API to user space") currently breaks EFA, as ibv_query_gid() no longer works.
> > >
> > > The problem is that the IOCTL call checks for:
> > > 	if (!rdma_ib_or_roce(ib_dev, port_num))
> > > 		return -EOPNOTSUPP;
> > >
> > > EFA is neither of these, but it uses GIDs.
> > >
> > > Any objections to remove the check? Any other solutions come to mind?
> >
> > We added this check to protect access to rdma_get_gid_attr() for devices
> > without GID table.
> >  1234         table = rdma_gid_table(device, port_num);
> >  1235         if (index < 0 || index >= table->sz)
> >  1236                 return ERR_PTR(-EINVAL);
> >
> > So you can extend that function to return for table == NULL an error and
> > remove rdma_ib_or_roce()
>
> How does table == NULL ever?

ok, you are right in that regards.
However, mlx5 IB representors don't support GIDs and sets gid_tbl_len to be zero.
Do we want to rely on the assumption that table will always exist?

Thanks

>
> Jason

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: New GID query API broke EFA
  2020-10-22 12:15       ` Gal Pressman
@ 2020-10-22 12:23         ` Leon Romanovsky
  0 siblings, 0 replies; 9+ messages in thread
From: Leon Romanovsky @ 2020-10-22 12:23 UTC (permalink / raw)
  To: Gal Pressman
  Cc: Jason Gunthorpe, Avihai Horon, Maor Gottlieb, linux-rdma,
	Leybovich, Yossi

On Thu, Oct 22, 2020 at 03:15:55PM +0300, Gal Pressman wrote:
> On 22/10/2020 15:12, Gal Pressman wrote:
> > On 22/10/2020 15:10, Jason Gunthorpe wrote:
> >> On Thu, Oct 22, 2020 at 02:21:00PM +0300, Leon Romanovsky wrote:
> >>> On Thu, Oct 22, 2020 at 01:58:29PM +0300, Gal Pressman wrote:
> >>>> Hi all,
> >>>>
> >>>> The new IOCTL query GID API 9f85cbe50aa0 ("RDMA/uverbs: Expose the new GID query
> >>>> API to user space") currently breaks EFA, as ibv_query_gid() no longer works.
> >>>>
> >>>> The problem is that the IOCTL call checks for:
> >>>>     if (!rdma_ib_or_roce(ib_dev, port_num))
> >>>>             return -EOPNOTSUPP;
> >>>>
> >>>> EFA is neither of these, but it uses GIDs.
> >>>>
> >>>> Any objections to remove the check? Any other solutions come to mind?
> >>>
> >>> We added this check to protect access to rdma_get_gid_attr() for devices
> >>> without GID table.
> >>>  1234         table = rdma_gid_table(device, port_num);
> >>>  1235         if (index < 0 || index >= table->sz)
> >>>  1236                 return ERR_PTR(-EINVAL);
> >>>
> >>> So you can extend that function to return for table == NULL an error and
> >>> remove rdma_ib_or_roce()
> >>
> >> How does table == NULL ever?
> >
> > A driver with gid_tbl_len == 0 would make the allocation return NULL, no?
>
> Nevermind, we don't really allocate with gid_tbl_len as the size, and any
> allocation failure will fail ib_register_device().
> So I guess there is no way for it to be NULL.

I think so too.

Thanks

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: New GID query API broke EFA
  2020-10-22 12:23     ` Leon Romanovsky
@ 2020-10-22 16:37       ` Jason Gunthorpe
  0 siblings, 0 replies; 9+ messages in thread
From: Jason Gunthorpe @ 2020-10-22 16:37 UTC (permalink / raw)
  To: Leon Romanovsky
  Cc: Gal Pressman, Avihai Horon, Maor Gottlieb, linux-rdma, Leybovich, Yossi

On Thu, Oct 22, 2020 at 03:23:01PM +0300, Leon Romanovsky wrote:
> On Thu, Oct 22, 2020 at 09:10:35AM -0300, Jason Gunthorpe wrote:
> > On Thu, Oct 22, 2020 at 02:21:00PM +0300, Leon Romanovsky wrote:
> > > On Thu, Oct 22, 2020 at 01:58:29PM +0300, Gal Pressman wrote:
> > > > Hi all,
> > > >
> > > > The new IOCTL query GID API 9f85cbe50aa0 ("RDMA/uverbs: Expose the new GID query
> > > > API to user space") currently breaks EFA, as ibv_query_gid() no longer works.
> > > >
> > > > The problem is that the IOCTL call checks for:
> > > > 	if (!rdma_ib_or_roce(ib_dev, port_num))
> > > > 		return -EOPNOTSUPP;
> > > >
> > > > EFA is neither of these, but it uses GIDs.
> > > >
> > > > Any objections to remove the check? Any other solutions come to mind?
> > >
> > > We added this check to protect access to rdma_get_gid_attr() for devices
> > > without GID table.
> > >  1234         table = rdma_gid_table(device, port_num);
> > >  1235         if (index < 0 || index >= table->sz)
> > >  1236                 return ERR_PTR(-EINVAL);
> > >
> > > So you can extend that function to return for table == NULL an error and
> > > remove rdma_ib_or_roce()
> >
> > How does table == NULL ever?
> 
> ok, you are right in that regards.
> However, mlx5 IB representors don't support GIDs and sets gid_tbl_len to be zero.
> Do we want to rely on the assumption that table will always exist?

We do already, seems like the rdma_ib_or_roce should just be deleted

Jason

^ permalink raw reply	[flat|nested] 9+ messages in thread

end of thread, back to index

Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-10-22 10:58 New GID query API broke EFA Gal Pressman
2020-10-22 11:21 ` Leon Romanovsky
2020-10-22 11:49   ` Gal Pressman
2020-10-22 12:10   ` Jason Gunthorpe
2020-10-22 12:12     ` Gal Pressman
2020-10-22 12:15       ` Gal Pressman
2020-10-22 12:23         ` Leon Romanovsky
2020-10-22 12:23     ` Leon Romanovsky
2020-10-22 16:37       ` Jason Gunthorpe

Linux-RDMA Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-rdma/0 linux-rdma/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-rdma linux-rdma/ https://lore.kernel.org/linux-rdma \
		linux-rdma@vger.kernel.org
	public-inbox-index linux-rdma

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-rdma


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git