On Fri, Feb 10, 2017 at 10:08 PM, Jason Gunthorpe
<jgunthorpe@xxxxxxxxxxxxxxxxxxxx> wrote:
> On Wed, Feb 01, 2017 at 02:39:01PM +0200, Matan Barak wrote:
>
>> +void uverbs_free_qp(struct ib_uobject *uobject)
>
> This should be static...
>
Yeah
> Based on the last email I expect this to be
>
> static int uverbs_destroy_qp(struct ib_uobject *uobject, enum rdma_remove_reason why)
>
Since we don't care about the reason here and it's called from the
destroy callback,
we could leave the deceleration as is.
>> +{
>> + struct ib_qp *qp = uobject->object;
>> + struct ib_uqp_object *uqp =
>> + container_of(uobject, struct ib_uqp_object, uevent.uobject);
>> +
>> + if (qp == qp->real_qp)
>> + ib_uverbs_detach_umcast(qp, uqp);
>> + ib_destroy_qp(qp);
>
> This silently eats the error code from ib_destroy_qp, not OK. See
> discussion in prior email. Lots of cases of this.
>
I can add a WARN_ON, but this is essentially the same as ib_destroy_qp
fails when a context is destroyed (either because of hot unplug or process
termination). You won't get here because of regular destroy handler.
This is the current behavior as well.
>> +const struct uverbs_obj_idr_type uverbs_type_attrs_cq =
>> + UVERBS_TYPE_ALLOC_IDR_SZ(sizeof(struct ib_ucq_object), 0,
>> + uverbs_free_cq);
>>
>
> It would be more readable if the struct decl was near the functions in
> blocks.
>
But then it can't be used by the hardware drivers to declare new types.
> Still don't like these macros. The open coded version would look like this:
>
> const struct uverbs_obj_idr_type uverbs_type_attrs_cq = {
> .type = {
> .obj_size = sizeof(struct ib_ucq_object),
> .ops = &uverbs_idr_ops,
> },
> .destroy = uverbs_destroy_cq,
> };
>
> Whichs is more understandable and typical of the kernel style. If we
> wish to add a new callback function for instance we don't need a new
> macro or big churn.
>
> You could do this I guess:
>
> const struct uverbs_obj_idr_type uverbs_type_attrs_cq = {
> .type = UVERBS_IDR_TYPE(cq, 0),
> .destroy = uverbs_destroy_cq,
> };
>
Yeah, seems reasonable - a good trade off.
>> +extern const struct uverbs_obj_type_ops uverbs_idr_ops;
>> +
>> +#define UVERBS_BUILD_BUG_ON(cond) (sizeof(char[1 - 2 * !!(cond)]) - \
>> + sizeof(char))
>
> This probably doesn't add value, but you could make this a WARN_ON in
> alloc_uobj().
>
Why should we calculate this on runtime if we could know this in compile time?
> Jason
> --
> To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
> the body of a message to majordomo@xxxxxxxxxxxxxxx
> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
