Re: [PATCH RESEND v2 06/12] coco: host: arm64: Add RMM device communication helpers

[Jonathan Cameron <jonathan.cameron@huawei.com>]

On Thu, 30 Oct 2025 21:50:22 +0530
"Aneesh Kumar K.V" <aneesh.kumar@kernel.org> wrote:

> Jonathan Cameron <jonathan.cameron@huawei.com> writes:
> 
> > On Mon, 27 Oct 2025 15:25:56 +0530
> > "Aneesh Kumar K.V (Arm)" <aneesh.kumar@kernel.org> wrote:
> >  
> 
> ...
> 
> >> +static int __do_dev_communicate(enum dev_comm_type type,
> >> +				struct pci_tsm *tsm, unsigned long error_state)
> >> +{
> >> +	int ret;
> >> +	int state;
> >> +	struct rmi_dev_comm_enter *io_enter;
> >> +	struct cca_host_pf0_dsc *pf0_dsc = to_cca_pf0_dsc(tsm->dsm_dev);
> >> +
> >> +	io_enter = &pf0_dsc->comm_data.io_params->enter;
> >> +	io_enter->resp_len = 0;
> >> +	io_enter->status = RMI_DEV_COMM_NONE;
> >> +
> >> +	ret = ___do_dev_communicate(type, tsm);  
> >
> > Think up a more meaningful name.  Counting _ doesn't make for readable code.
> >  
> 
> I am not sure about this. What do you think?
> 
> modified   drivers/virt/coco/arm-cca-host/rmi-da.c
> @@ -188,7 +188,7 @@ static inline gfp_t cache_obj_id_to_gfp_flags(u8 cache_obj_id)
>  	return GFP_KERNEL_ACCOUNT;
>  }
>  
> -static int ___do_dev_communicate(enum dev_comm_type type, struct pci_tsm *tsm)
> +static int __do_dev_communicate(enum dev_comm_type type, struct pci_tsm *tsm)
>  {
>  	gfp_t cache_alloc_flags;
>  	int ret, nbytes, cp_len;
> @@ -319,7 +319,7 @@ static int ___do_dev_communicate(enum dev_comm_type type, struct pci_tsm *tsm)
>  	return 0;
>  }
>  
> -static int __do_dev_communicate(enum dev_comm_type type,
> +static int do_dev_communicate(enum dev_comm_type type,
>  				struct pci_tsm *tsm, unsigned long error_state)
>  {
>  	int ret;
> @@ -331,7 +331,7 @@ static int __do_dev_communicate(enum dev_comm_type type,
>  	io_enter->resp_len = 0;
>  	io_enter->status = RMI_DEV_COMM_NONE;
>  
> -	ret = ___do_dev_communicate(type, tsm);
> +	ret = __do_dev_communicate(type, tsm);
>  	if (ret) {
>  		if (type == PDEV_COMMUNICATE)
>  			rmi_pdev_abort(virt_to_phys(pf0_dsc->rmm_pdev));
> @@ -355,14 +355,14 @@ static int __do_dev_communicate(enum dev_comm_type type,
>  	return state;
>  }
>  
> -static int do_dev_communicate(enum dev_comm_type type, struct pci_tsm *tsm,
> -			      unsigned long target_state,
> -			      unsigned long error_state)
> +static int move_dev_to_state(enum dev_comm_type type, struct pci_tsm *tsm,

Naming is always tricky.  Not sure why this name is appropriate given it's definitely
still related to dev_communicate.

Maybe just squash do_dev_communicate and __do_dev_coummnicate.
Slightly long lines will be the result but not too bad.
I haven't checked what it ends up as after the whole series though
so maybe it doesn't work out.

static int do_dev_communicate(enum dev_comm_type type, struct pci_tsm *tsm,
			      unsigned long target_state,
			      unsigned long error_state)
{
	

	do {
		int state, ret;
		struct rmi_dev_comm_enter *io_enter;
		struct cca_host_pf0_dsc *pf0_dsc = to_cca_pf0_dsc(tsm->dsm_dev);

		io_enter = &pf0_dsc->comm_data.io_params->enter;
		io_enter->resp_len = 0;
		io_enter->status = RMI_DEV_COMM_NONE;

		ret = ___do_dev_communicate(type, tsm);
//renamed

		if (ret) {
			if (type == PDEV_COMMUNICATE)
				rmi_pdev_abort(virt_to_phys(pf0_dsc->rmm_pdev));

			state = error_state;
		} else {
			/*
			 * Some device communication error will transition the
			 * device to error state. Report that.
			 */
			if (type == PDEV_COMMUNICATE)
				ret = rmi_pdev_get_state(virt_to_phys(pf0_dsc->rmm_pdev),
							 (enum rmi_pdev_state *)&state);
			if (ret)
				state = error_state;
		}
	
		if (state == error_state) {
			pci_err(tsm->pdev, "device communication error\n");
			return state;
		}
		if (state == target_state)
			return state;
	} while (1);
}
Jonathan

> +			     unsigned long target_state,
> +			     unsigned long error_state)
>  {
>  	int state;
>  
>  	do {
> -		state = __do_dev_communicate(type, tsm, error_state);
> +		state = do_dev_communicate(type, tsm, error_state);
>  
>  		if (state == target_state || state == error_state)
>  			return state;
> @@ -374,7 +374,7 @@ static int do_dev_communicate(enum dev_comm_type type, struct pci_tsm *tsm,
>  
>  static int do_pdev_communicate(struct pci_tsm *tsm, enum rmi_pdev_state target_state)
>  {
> -	return do_dev_communicate(PDEV_COMMUNICATE, tsm, target_state, RMI_PDEV_ERROR);
> +	return move_dev_to_state(PDEV_COMMUNICATE, tsm, target_state, RMI_PDEV_ERROR);
>  }
>  
>  static int parse_certificate_chain(struct pci_tsm *tsm)
> 
> 
> 
> 
> -aneesh
>