[ofa-general] Re: [PATCH] core: check optional verbs before using them

[Dotan Barak]

I would like to protect buggy SW as well, but you are right - kernel 
coding is for people who knows
what they are doing...

thanks
Dotan


Roland Dreier wrote:
>  > Check that all optional verbs are implemented in the device
>  > before using them.
>
> Some parts make sense, eg:
>
>  > @@ -248,7 +248,9 @@ int ib_modify_srq(struct ib_srq *srq,
>  >  		  struct ib_srq_attr *srq_attr,
>  >  		  enum ib_srq_attr_mask srq_attr_mask)
>  >  {
>  > -	return srq->device->modify_srq(srq, srq_attr, srq_attr_mask, NULL);
>  > +	return srq->device->modify_srq ?
>  > +		srq->device->modify_srq(srq, srq_attr, srq_attr_mask, NULL) :
>  > +		-ENOSYS;
>
> on the other hand:
>
>  > @@ -265,6 +267,9 @@ int ib_destroy_srq(struct ib_srq *srq)
>  >  	struct ib_pd *pd;
>  >  	int ret;
>  >  
>  > +	if (!srq->device->destroy_srq)
>  > +		return -ENOSYS;
>  > +
>
> I think it's safe to assume that a driver that allows SRQs to be created
> will allow them to be destroyed, and code that destroys a non-existent
> SRQ is buggy.  So I don't think this is worth it.  Same for dealloc MW
> and dealloc FMR.
>
> The reg_phys_mr change is sane too.  So I applied this:
>
> commit 3926318b1e52568b10a9275b34e0a1fdef6c10e8
> Author: Dotan Barak <dotanb at dev.mellanox.co.il>
> Date:   Mon Mar 31 17:50:02 2008 +0300
>
>     IB/core: Check optional verbs before using them
>     
>     Make sure that a device implements the modify_srq and reg_phys_mr
>     optional methods before calling them.
>     
>     Signed-off-by: Dotan Barak <dotanb at dev.mellanox.co.il>
>     Signed-off-by: Roland Dreier <rolandd at cisco.com>
>
> diff --git a/drivers/infiniband/core/verbs.c b/drivers/infiniband/core/verbs.c
> index 86ed8af..8ffb5f2 100644
> --- a/drivers/infiniband/core/verbs.c
> +++ b/drivers/infiniband/core/verbs.c
> @@ -248,7 +248,9 @@ int ib_modify_srq(struct ib_srq *srq,
>  		  struct ib_srq_attr *srq_attr,
>  		  enum ib_srq_attr_mask srq_attr_mask)
>  {
> -	return srq->device->modify_srq(srq, srq_attr, srq_attr_mask, NULL);
> +	return srq->device->modify_srq ?
> +		srq->device->modify_srq(srq, srq_attr, srq_attr_mask, NULL) :
> +		-ENOSYS;
>  }
>  EXPORT_SYMBOL(ib_modify_srq);
>  
> @@ -672,6 +674,9 @@ struct ib_mr *ib_reg_phys_mr(struct ib_pd *pd,
>  {
>  	struct ib_mr *mr;
>  
> +	if (!pd->device->reg_phys_mr)
> +		return ERR_PTR(-ENOSYS);
> +
>  	mr = pd->device->reg_phys_mr(pd, phys_buf_array, num_phys_buf,
>  				     mr_access_flags, iova_start);
>  
>
>