Re: [PATCH for-next v5 4/6] RDMA-rxe: Isolate mr code from atomic_write_reply()

[Jason Gunthorpe <jgg@nvidia.com>]

On Tue, Jan 17, 2023 at 02:41:53PM -0600, Bob Pearson wrote:
> On 1/17/23 12:05, Jason Gunthorpe wrote:
> > On Tue, Jan 17, 2023 at 11:04:29AM -0600, Bob Pearson wrote:
> >> On 1/17/23 10:59, Jason Gunthorpe wrote:
> >>> On Tue, Jan 17, 2023 at 10:57:31AM -0600, Bob Pearson wrote:
> >>>
> >>>>>> -	dst = iova_to_vaddr(mr, qp->resp.va + qp->resp.offset, payload);
> >>>>>> -	/* check vaddr is 8 bytes aligned. */
> >>>>>> -	if (!dst || (uintptr_t)dst & 7)
> >>>>>> -		return RESPST_ERR_MISALIGNED_ATOMIC;
> >>>>>> +	if (res->replay)
> >>>>>> +		return RESPST_ACKNOWLEDGE;
> >>>>>>  
> >>>>>> -	/* Do atomic write after all prior operations have completed */
> >>>>>> -	smp_store_release(dst, src);
> >>>>>> +	mr = qp->resp.mr;
> >>>>>> +	value = *(u64 *)payload_addr(pkt);
> >>>>>> +	iova = qp->resp.va + qp->resp.offset;
> >>>>>>  
> >>>>>> -	/* decrease resp.resid to zero */
> >>>>>> -	qp->resp.resid -= sizeof(payload);
> >>>>>> +#if defined CONFIG_64BIT
> >>>>>
> >>>>> Shouldn't need a #ifdef here
> >>>>
> >>>> This avoids a new special error (i.e. NOT_64_bit) and makes it clear we
> >>>> won't call the code in mr.
> >>>
> >>> ? That doesn't seem right
> >>
> >> that was the -3 of the -1, -2, -3 that we just fixed. there are three error paths out
> >> of this state and we need a way to get to them. The #ifdef provides
> >> that third path.
> > 
> > I feel like it should be solvable without this ifdef though
> > 
> > Jason
> 
> You could get rid of the ifdef in the atomic_write_reply() routine but then the rxe_mr_do_atomic_write() routine would have to have a second version in the #else case
> that would have to return something different so that the third exit could be taken i.e.
> whatever replaces the original -3. I really think this is simpler.

You really should just return the RESPST_* from these functions.

Jason