On Thu, Jan 29, 2015 at 06:59:58PM +0100, Yann Droneaud wrote:
> As specified in "Extending Verbs API" presentation [1] by Tzahi Oved
> during OFA International Developer Workshop 2013, the request's
> comp_mask should describe the request data: it's describe the
> availability of extended fields in the request.
> Conversely, the response's comp_mask should describe the presence
> of extended fields in the response.

Roland: I agree with Yann, these patches need to go in, or the ODP
patches reverted.

>  #ifdef CONFIG_INFINIBAND_ON_DEMAND_PAGING
> -     if (cmd.comp_mask & IB_USER_VERBS_EX_QUERY_DEVICE_ODP) {

Absolutely, a query output should never depend on the input comp_mask.

> -             resp.odp_caps.general_caps = attr.odp_caps.general_caps;
> -             resp.odp_caps.per_transport_caps.rc_odp_caps =
> -                     attr.odp_caps.per_transport_caps.rc_odp_caps;
> -             resp.odp_caps.per_transport_caps.uc_odp_caps =
> -                     attr.odp_caps.per_transport_caps.uc_odp_caps;
> -             resp.odp_caps.per_transport_caps.ud_odp_caps =
> -                     attr.odp_caps.per_transport_caps.ud_odp_caps;
> -             resp.comp_mask |= IB_USER_VERBS_EX_QUERY_DEVICE_ODP;
> -     }
> +     resp.odp_caps.general_caps = attr.odp_caps.general_caps;
> +     resp.odp_caps.per_transport_caps.rc_odp_caps =
> +             attr.odp_caps.per_transport_caps.rc_odp_caps;
> +     resp.odp_caps.per_transport_caps.uc_odp_caps =
> +             attr.odp_caps.per_transport_caps.uc_odp_caps;
> +     resp.odp_caps.per_transport_caps.ud_odp_caps =
> +             attr.odp_caps.per_transport_caps.ud_odp_caps;
>  #endif
> +     resp.comp_mask |= IB_USER_VERBS_EX_QUERY_DEVICE_ODP;

Not sure about this - if 0 is a valid null answer for all the _caps
then it is fine, and the comp_mask bit should just be removed as the
size alone should be enough.

This looks wrong however:

>       err = ib_copy_to_udata(ucore, &resp, sizeof(resp));
>       if (err)
>           return err;
>        return 0;

Why isn't this returning the filled structure size to userspace?  This
would seem to be very urgently wrong to me? Am I missing something?

Patch 3 probably should gain:

- return 0;
+ return resp_len;

This patch looks like an improvement to me:

Reviewed-By: Jason Gunthorpe <[email protected]>

Jason
--
To unsubscribe from this list: send the line "unsubscribe linux-api" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Reply via email to