Re: [PATCH for-rc] RDMA/vmw_pvrdma: Return the correct opcode when creating WR

[Adit Ranadive <aditr@vmware.com>]

On 1/7/19 10:59 AM, Jason Gunthorpe wrote:
> On Mon, Jan 07, 2019 at 06:53:24PM +0000, Adit Ranadive wrote:
>> From: Adit Ranadive <aditr@vmware.com>
>>
>> Since the IB_WR_REG_MR opcode value changed, set some of the PVRDMA device
>> opcodes explicitly.
>>
>> Reported-by: Ruishuang Wang <ruishuangw@vmware.com>
>> Fixes: 9a59739bd01f ("IB/rxe: Revise the ib_wr_opcode enum")
>> Cc: stable@vger.kernel.org
>> Reviewed-by: Bryan Tan <bryantan@vmware.com>
>> Reviewed-by: Ruishuang Wang <ruishuangw@vmware.com>
>> Reviewed-by: Vishnu Dasa <vdasa@vmware.com>
>> Signed-off-by: Adit Ranadive <aditr@vmware.com>
>> ---
>>  drivers/infiniband/hw/vmw_pvrdma/pvrdma.h | 21 ++++++++++++++++++++-
>>  1 file changed, 20 insertions(+), 1 deletion(-)
> 
> So naughty!! How many other places in this driver are assuming the
> intenal IB constants are stable?
> 
> Did you audit for other cases?

Lots of places. Though its the values shared with userspace, like QP types,
IB MTUs, etc. Unless you foresee changes to those .. 

>  
>> diff --git a/drivers/infiniband/hw/vmw_pvrdma/pvrdma.h b/drivers/infiniband/hw/vmw_pvrdma/pvrdma.h
>> index 42b8685c997e..c2ed09e66d2b 100644
>> --- a/drivers/infiniband/hw/vmw_pvrdma/pvrdma.h
>> +++ b/drivers/infiniband/hw/vmw_pvrdma/pvrdma.h
>> @@ -427,7 +427,26 @@ static inline enum ib_qp_state pvrdma_qp_state_to_ib(enum pvrdma_qp_state state)
>>  
>>  static inline enum pvrdma_wr_opcode ib_wr_opcode_to_pvrdma(enum ib_wr_opcode op)
>>  {
>> -	return (enum pvrdma_wr_opcode)op;
>> +	switch (op) {
>> +	case IB_WR_LSO:
>> +		return PVRDMA_WR_LSO;
>> +	case IB_WR_SEND_WITH_INV:
>> +		return PVRDMA_WR_SEND_WITH_INV;
>> +	case IB_WR_RDMA_READ_WITH_INV:
>> +		return PVRDMA_WR_RDMA_READ_WITH_INV;
>> +	case IB_WR_LOCAL_INV:
>> +		return PVRDMA_WR_LOCAL_INV;
>> +	case IB_WR_REG_MR:
>> +		return PVRDMA_WR_FAST_REG_MR;
>> +	case IB_WR_MASKED_ATOMIC_CMP_AND_SWP:
>> +		return PVRDMA_WR_MASKED_ATOMIC_CMP_AND_SWP;
>> +	case IB_WR_MASKED_ATOMIC_FETCH_AND_ADD:
>> +		return PVRDMA_WR_MASKED_ATOMIC_FETCH_AND_ADD;
>> +	case IB_WR_REG_SIG_MR:
>> +		return PVRDMA_WR_REG_SIG_MR;
>> +	default:
>> +		return (enum pvrdma_wr_opcode)op;
> 
> No default, lets write them all out please.

Fair enough.

> 
> Jason
>