[ofa-general] Re: [PATCH] uverbs: return ENOSYS for unimplemented commands (not EINVAL)

[Jeff Squyres]

+1 for this patch (returning ENOSYS instead of EINVAL): it makes life  
a bunch easier for Open MPI because we can tell the difference between  
"it isn't implemented" and a bug in certain versions of ConnectX  
firmware.

Roland -- do you think you'll apply this patch?


On Dec 2, 2008, at 12:43 PM, Jack Morgenstein wrote:

> uverbs: return ENOSYS for unimplemented commands (not EINVAL)
>
> In the original commit (883a99c7024c5763d6d4f22d9239c133893e8d74)
> (Add a mask of device methods allowed for userspace),
> the driver returned EINVAL for unimplemented commands.
>
> This creates a problem that there is no way to differentiate between
> an unimplemented command and an implemented one which is incorrectly
> invoked (which also returns EINVAL).
>
> The fix is to have unimplemented commands return ENOSYS.
>
> Signed-off-by: Jack Morgenstein <jackm at dev.mellanox.co.il>
>
> ---
>
> Roland,
> We've got a bit of a salad here (d--ned if we do, d--ned if we don't).
>
> In userspace, we have low-level libraries put NULL in the virtual  
> function
> table for unimplemented verbs. libibverbs then returns NULL for those
> unimplemented verbs which expect a pointer return (e.g.,  
> ibv_create_srq)
> (also a problem since this does not differentiate between a missing  
> verb and
> an incorrectly-invoked one),
> and ENOSYS for verbs which expect an int returned (e.g., resize_cq).
>
> This is not consistent with what was done in the kernel for  
> unimplemented verbs
> (where EINVAL is returned).
>
> Additionally, what was done in the kernel (returning EINVAL) is  
> problematic
> in that it makes it impossible to differentiate an unimplemented  
> verb from one
> which simply had errors in the calling parameters.
>
> IMHO, the correct fix is to have unimplemented kernel verbs return  
> ENOSYS.
> **(MPI already checks for ENOSYS when deciding whether or not to use  
> resize-cq)**.
>
> I'm not sure, though, how to handle returns from older kernels --  
> should all user
> apps check for either ENOSYS or EINVAL?  What about cases where the  
> verb actually
> is implemented, but incorrectly called -- in older kernels this is  
> already a problem.
> What about apps which checked for ENOSYS (consistent with userspace  
> usage), and
> then run with a new low-level library over an older kernel, and  
> suddenly start getting
> EINVAL returns (for resize_cq, there is no "activation" bit anywhere  
> which gets passed
> from kernel to the user-level)?
>
> Ouch!
>
> Any ideas?
>
> Index: infiniband/drivers/infiniband/core/uverbs_main.c
> ===================================================================
> --- infiniband.orig/drivers/infiniband/core/uverbs_main.c
> +++ infiniband/drivers/infiniband/core/uverbs_main.c
> @@ -584,10 +584,12 @@ static ssize_t ib_uverbs_write(struct fi
>
>         if (hdr.command < 0                             ||
>             hdr.command >= ARRAY_SIZE(uverbs_cmd_table) ||
> -           !uverbs_cmd_table[hdr.command]              ||
> -           !(file->device->ib_dev->uverbs_cmd_mask & (1ull <<  
> hdr.command)))
> +           !uverbs_cmd_table[hdr.command])
>                 return -EINVAL;
>
> +       if (!(file->device->ib_dev->uverbs_cmd_mask & (1ull <<  
> hdr.command)))
> +               return -ENOSYS;
> +
>         if (!file->ucontext &&
>             hdr.command != IB_USER_VERBS_CMD_GET_CONTEXT)
>                 return -EINVAL;


-- 
Jeff Squyres
Cisco Systems