| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <20251031120730.00003758@huawei.com>
Date: Fri, 31 Oct 2025 12:07:30 +0000
From: Jonathan Cameron <jonathan.cameron@...wei.com>
To: Aneesh Kumar K.V <aneesh.kumar@...nel.org>
CC: <linux-coco@...ts.linux.dev>, <kvmarm@...ts.linux.dev>,
<linux-pci@...r.kernel.org>, <linux-kernel@...r.kernel.org>,
<dan.j.williams@...el.com>, <aik@....com>, <lukas@...ner.de>, Samuel Ortiz
<sameo@...osinc.com>, Xu Yilun <yilun.xu@...ux.intel.com>, Jason Gunthorpe
<jgg@...pe.ca>, Suzuki K Poulose <Suzuki.Poulose@....com>, Steven Price
<steven.price@....com>, Bjorn Helgaas <helgaas@...nel.org>, Catalin Marinas
<catalin.marinas@....com>, Marc Zyngier <maz@...nel.org>, Will Deacon
<will@...nel.org>, Oliver Upton <oliver.upton@...ux.dev>
Subject: Re: [PATCH RESEND v2 06/12] coco: host: arm64: Add RMM device
communication helpers
On Fri, 31 Oct 2025 13:34:33 +0530
"Aneesh Kumar K.V" <aneesh.kumar@...nel.org> wrote:
> Jonathan Cameron <jonathan.cameron@...wei.com> writes:
>
> > On Thu, 30 Oct 2025 21:50:22 +0530
> > "Aneesh Kumar K.V" <aneesh.kumar@...nel.org> wrote:
> >
> >> Jonathan Cameron <jonathan.cameron@...wei.com> writes:
> >>
> >> > On Mon, 27 Oct 2025 15:25:56 +0530
> >> > "Aneesh Kumar K.V (Arm)" <aneesh.kumar@...nel.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
> >
>
> I need the existing __do_dev_communicate for a followup patch where the
> device communication won't loop till state transition.
Ah. I'd missed that. Fair enough.
> -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,
> +static int wait_for_dev_state(enum dev_comm_type type, struct pci_tsm *tsm,
This name conveys what the wrapper adds to the inner call but neglects that inner bit.
do_dev_communicate_and_wait_for_xxx or
do_dev_communicate_synchronous() // maybe - it's approximately a synchronous wrapper of async operation.
Or something along those lines perhaps?
> unsigned long target_state,
> unsigned long error_state)
> {
Powered by blists - more mailing lists