From: Jonathan Cameron <jonathan.cameron@huawei.com>
To: Aneesh Kumar K.V <aneesh.kumar@kernel.org>
Cc: <linux-coco@lists.linux.dev>, <kvmarm@lists.linux.dev>,
<linux-pci@vger.kernel.org>, <linux-kernel@vger.kernel.org>,
<dan.j.williams@intel.com>, <aik@amd.com>, <lukas@wunner.de>,
Samuel Ortiz <sameo@rivosinc.com>,
Xu Yilun <yilun.xu@linux.intel.com>,
Jason Gunthorpe <jgg@ziepe.ca>,
Suzuki K Poulose <Suzuki.Poulose@arm.com>,
Steven Price <steven.price@arm.com>,
Bjorn Helgaas <helgaas@kernel.org>,
Catalin Marinas <catalin.marinas@arm.com>,
Marc Zyngier <maz@kernel.org>, Will Deacon <will@kernel.org>,
Oliver Upton <oliver.upton@linux.dev>
Subject: Re: [PATCH RESEND v2 06/12] coco: host: arm64: Add RMM device communication helpers
Date: Fri, 31 Oct 2025 12:07:30 +0000 [thread overview]
Message-ID: <20251031120730.00003758@huawei.com> (raw)
In-Reply-To: <yq5ams57cx9q.fsf@kernel.org>
On Fri, 31 Oct 2025 13:34:33 +0530
"Aneesh Kumar K.V" <aneesh.kumar@kernel.org> wrote:
> Jonathan Cameron <jonathan.cameron@huawei.com> writes:
>
> > 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
> >
>
> 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)
> {
next prev parent reply other threads:[~2025-10-31 12:07 UTC|newest]
Thread overview: 34+ messages / expand[flat|nested] mbox.gz Atom feed top
2025-10-27 9:55 [PATCH RESEND v2 00/12] coc: tsm: Implement ->connect()/->disconnect() callbacks for ARM CCA IDE setup Aneesh Kumar K.V (Arm)
2025-10-27 9:55 ` [PATCH RESEND v2 01/12] KVM: arm64: RMI: Export kvm_has_da_feature Aneesh Kumar K.V (Arm)
2025-10-29 16:39 ` Jonathan Cameron
2025-10-27 9:55 ` [PATCH RESEND v2 02/12] firmware: smccc: coco: Manage arm-smccc platform device and CCA auxiliary drivers Aneesh Kumar K.V (Arm)
2025-10-29 16:52 ` Jonathan Cameron
2026-01-06 12:33 ` Aneesh Kumar K.V
2025-10-27 9:55 ` [PATCH RESEND v2 03/12] coco: guest: arm64: Drop dummy RSI platform device stub Aneesh Kumar K.V (Arm)
2025-10-29 16:54 ` Jonathan Cameron
2025-10-27 9:55 ` [PATCH RESEND v2 04/12] coco: host: arm64: Add host TSM callback and IDE stream allocation support Aneesh Kumar K.V (Arm)
2025-10-29 17:18 ` Jonathan Cameron
2025-10-27 9:55 ` [PATCH RESEND v2 05/12] coco: host: arm64: Build and register RMM pdev descriptors Aneesh Kumar K.V (Arm)
2025-10-29 17:37 ` Jonathan Cameron
2025-10-30 8:44 ` Aneesh Kumar K.V
2025-10-30 10:00 ` Jonathan Cameron
2025-10-27 9:55 ` [PATCH RESEND v2 06/12] coco: host: arm64: Add RMM device communication helpers Aneesh Kumar K.V (Arm)
2025-10-29 18:33 ` Jonathan Cameron
2025-10-30 9:18 ` Aneesh Kumar K.V
2025-10-30 10:00 ` Jonathan Cameron
2025-10-30 14:04 ` Aneesh Kumar K.V
2025-10-30 18:02 ` Jonathan Cameron
2025-10-30 16:20 ` Aneesh Kumar K.V
2025-10-30 18:12 ` Jonathan Cameron
2025-10-31 8:04 ` Aneesh Kumar K.V
2025-10-31 12:07 ` Jonathan Cameron [this message]
2025-10-27 9:55 ` [PATCH RESEND v2 07/12] coco: host: arm64: Add helper to stop and tear down an RMM pdev Aneesh Kumar K.V (Arm)
2025-10-29 18:34 ` Jonathan Cameron
2025-10-27 9:55 ` [PATCH RESEND v2 08/12] coco: host: arm64: Instantiate RMM pdev during device connect Aneesh Kumar K.V (Arm)
2025-10-29 18:38 ` Jonathan Cameron
2025-10-27 9:55 ` [PATCH RESEND v2 09/12] X.509: Make certificate parser public Aneesh Kumar K.V (Arm)
2025-10-27 9:56 ` [PATCH RESEND v2 10/12] X.509: Parse Subject Alternative Name in certificates Aneesh Kumar K.V (Arm)
2025-10-27 9:56 ` [PATCH RESEND v2 11/12] X.509: Move certificate length retrieval into new helper Aneesh Kumar K.V (Arm)
2025-10-27 9:56 ` [PATCH RESEND v2 12/12] coco: host: arm64: Register device public key with RMM Aneesh Kumar K.V (Arm)
2025-10-29 17:19 ` Jason Gunthorpe
2025-10-29 18:53 ` Jonathan Cameron
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20251031120730.00003758@huawei.com \
--to=jonathan.cameron@huawei.com \
--cc=Suzuki.Poulose@arm.com \
--cc=aik@amd.com \
--cc=aneesh.kumar@kernel.org \
--cc=catalin.marinas@arm.com \
--cc=dan.j.williams@intel.com \
--cc=helgaas@kernel.org \
--cc=jgg@ziepe.ca \
--cc=kvmarm@lists.linux.dev \
--cc=linux-coco@lists.linux.dev \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-pci@vger.kernel.org \
--cc=lukas@wunner.de \
--cc=maz@kernel.org \
--cc=oliver.upton@linux.dev \
--cc=sameo@rivosinc.com \
--cc=steven.price@arm.com \
--cc=will@kernel.org \
--cc=yilun.xu@linux.intel.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.