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: 33+ 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
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 a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).