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

Jonathan Cameron
2025-10-30T18:13:01Z

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
>