Re: [PATCH v18 4/7] firmware: arm_rmm: Add support for SRO

[Jonathan Cameron <jonathan.cameron@oss.qualcomm.com>]

> >> diff --git a/drivers/firmware/arm_rmm/rmi.c b/drivers/firmware/arm_rmm/rmi.c
> >> index 5b0e342ce3d5..4f9898ece754 100644
> >> --- a/drivers/firmware/arm_rmm/rmi.c
> >> +++ b/drivers/firmware/arm_rmm/rmi.c
> >> @@ -15,6 +15,59 @@
> >>   #define RMI_FEAT_REG_COUNT	2
> >>   static unsigned long rmi_feat_reg_cache[RMI_FEAT_REG_COUNT] __ro_after_init;  
> >   
> 
> Was there a question here you missed ? FWIW:

I think inaccurate cropping only!  Have been using b4 review tui
(which is excellent btw!) Not sure I've gotten the hang of it fully yet.

> 
> I have expanded this to cover all 5 RmiFeatureRegisters exposed by RMM, 
> and it looks something like this now, with the use of ARRAY_SIZE() to
> limit accesses.
...


> 
> >> +
> >> +	/* Keep just the blocks the RMM didn't use in addr_list */
> >> +	for (int i = consumed_blocks; i < count; i++)
> >> +		sro->addr_list[addr_list_start + i - consumed_blocks] =
> >> +			sro->addr_list[addr_list_start + i];  
> > 
> > Maybe memmove to make it clear you are shifting a chunk of the array
> > down.  This might be worth some asci art showing each step in brief.
> > Anyhow, something along these lines:
> > 
> >        memmove(&sro->addr_list[addr_list_start],
> > 	      &sro->addr_list[addr_list_start + consumed_blocks],
> > 	      (count - consumed_blocks) * sizeof(*sro->addr_list));
> > 
> > Obviously doesn't save code, but maybe simpler to understand than
> > the loop.
> > 		  
> 
> Does the following hunk look better with the comment ?
> 
>          /*
>           * Keep just the blocks the RMM didn't use in addr_list.
>           * RMM claimed consumed_blocks entries from addr_list_start.
>           * Move the entries left out at the end i.e.
>           * [ addr_list_start + consumed_blocks, addr_list_start + found)
>           * to the rest of the valid entries and adjust the addr_count to
>           * reflect the available entries.
>           */
>          for (int i = 0, src = addr_list_start + consumed_blocks;
>                  i < found - consumed_blocks; i++)
>                  sro->addr_list[addr_list_start + i] = 
> sro->addr_list[src + i];
> 
>          sro->addr_count -= consumed_blocks;

Good enough.  Not sure there is a perfect answer for this one!

> 
> >   
> >> +}
> >> +
> >> +static int rmi_sro_reclaim(struct rmi_sro_state *sro,
> >> +			   unsigned long sro_handle,
> >> +			   struct arm_smccc_1_2_regs *out_regs)
> >> +{
> >> +	unsigned long capacity;
> >> +
> >> +	if (rmi_sro_ensure_capacity(sro, 1))
> >> +		rmi_sro_free(sro);  
> > 
> > Maybe a comment on why you free the whole lot rather than a smaller
> > amount to make some room.  
> 
> We don't do partial free of the list today, and having that is
> unnecssarily complicating the logic/code. We are going to do a reclaim 
> anyways. Sure, I could add a comment.

Might be useful as a breadcrumb in future but indeed not vital.
That was what I was assuming was the case.

> 
> 
> >   
> >> +
> >> +	capacity = RMI_MAX_ADDR_LIST - sro->addr_count;
> >> +
> >> +	rmi_op_mem_reclaim(sro_handle,
> >> +			   virt_to_phys(&sro->addr_list[sro->addr_count]),
> >> +			   capacity, out_regs);
> >> +
> >> +	/*
> >> +	  * RMI_OP_MEM_RECLAIM always return RMI_INCOMPLETE, except when the
> >> +	  * input parameters were invalid.
> >> +	  */
> >> +	if (WARN_ON_ONCE(RMI_RETURN_STATUS(out_regs->a0) != RMI_INCOMPLETE))
> >> +		return -EINVAL;
> >> +	if (WARN_ON_ONCE(out_regs->a1 > capacity))
> >> +		out_regs->a1 = capacity;  
> > I guess this should never happen, but bit ugly to fixup the register
> > return rather than perhaps using a local variable instead.  
> 
> We pass/get the out_regs, so local variable doesn't work, unless
> I missed what you were hinting at..

Ahh. Indeed, I wasn't thinking ahead to it being visible after. 

> >   
> >> +long rmi_sro_execute(struct arm_smccc_1_2_regs *regs)
> >> +{
> >> +	bool cancelled = false;
> >> +	unsigned long sro_handle = regs->a1;
> >> +
> >> +	rmi_smccc_invoke(regs);
> >> +
> >> +	sro_handle = regs->a1;
> >> +	while (RMI_RETURN_STATUS(regs->a0) == RMI_INCOMPLETE) {
> >> +		bool can_cancel = RMI_RETURN_CAN_CANCEL(regs->a0) == RMI_OP_CAN_CANCEL;
> >> +
> >> +		switch (RMI_RETURN_MEMREQ(regs->a0)) {
> >> +		case RMI_OP_MEM_REQ_NONE:
> >> +			rmi_op_continue(sro_handle, RMI_CONTINUE_KEEP_GOING,
> >> +					regs);
> >> +			break;
> >> +		default:
> >> +			WARN_ON_ONCE(1);
> >> +			if (!can_cancel)
> >> +				return regs->a0;
> >> +			/* If we have already cancelled, don't retry this */  
> > 
> > Maybe expand that comment a little to say if we can get here under a
> > valid sequence and if so are we guaranteed that cancel is complete
> > given that is what caller will probably expect if it gets -ECANCELED.
> > 
> > My reading of spec would suggest we can't hit this path for the
> > calls that are not memory tranferring. Then again we can't hit the
> > first cancel either so all bets are off.  So maybe this is the
> > bet we can do.  Also can_cancel shouldn't be true after
> > rmi_op_cancel() as I assume you can't cancel a cancel.  
> 
> Ack.  This was to handle a buggy RMM (and soothing an LLM bot)

Modern job titles: LLM Soother!

I aggressively cropped previous email. For avoidance of doubt
the rest all looked good to me.

> 
> Thank you for the review.
You are welcome. Thanks for pushing this forwards

Jonathan

> 
> Cheers
> Suzuki
> 
> 
> >   
>