From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail-pj1-f41.google.com (mail-pj1-f41.google.com [209.85.216.41]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id D5AE6595758 for ; Tue, 8 Sep 2026 18:02:12 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=209.85.216.41 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1788890535; cv=none; b=IsHMioAHG9CM4FZhooKpJIGMhX6ASBX1ha4+VyryOmBMHh+I558vGAeNVdghx7dRr74d8JAhHHwvF4d0phYBvZbZ0LuExN9vp9eQ7aH2ibEni7qou3Q3s/c2YCTyUcDmGkT91TvqTnijWCoCLSrjxcVZc+EAxrK4DowWC2BBNlw= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1788890535; c=relaxed/simple; bh=vkByFvx/xSRJDZge4ztLdHppOqU3iMImUjgSHZMGaQs=; h=Date:From:To:Cc:Subject:Message-ID:References:MIME-Version: Content-Type:Content-Disposition:In-Reply-To; b=Vo0WdM0ZwyknPFkS2uApHDWxdQ8LpQPixxmG4fUQFsSg21o0ljAqhuSAr1DsiZ2HcFdCCYJDcD7dzBHTushTtfB0buJ2LgTgyoaE7MwMwJE/0eO2SvtnGvOGL4zz55IA6INItZEUyRiFW9qFg6PgIbUPdgNpBcveNDWUG7zJmXQ= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=linaro.org; spf=pass smtp.mailfrom=linaro.org; dkim=pass (2048-bit key) header.d=linaro.org header.i=@linaro.org header.b=IGUCJFNB; arc=none smtp.client-ip=209.85.216.41 Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=linaro.org Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=linaro.org Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=linaro.org header.i=@linaro.org header.b="IGUCJFNB" Received: by mail-pj1-f41.google.com with SMTP id 98e67ed59e1d1-398b3c37877so4606327a91.0 for ; Tue, 08 Sep 2026 11:02:12 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; t=1788890532; x=1789495332; darn=vger.kernel.org; h=in-reply-to:content-disposition:content-type:mime-version :references:message-id:subject:cc:to:from:date:from:to:cc:subject :date:message-id:reply-to:content-type; bh=/bkSweG2AjV1rpphqtQ+x4kgo6UUQ9p9UEyix7QWjvg=; b=IGUCJFNBJHXQQ1x7Rzmmw5FXirYqHPAj/uaIvCEvzf9ZGqq61SOLe7mSKHNrsSA4EH nUh8BvmvgJySHMnZ+8s8K85ly0g6qP+Ip8C594vpSA/MZ9SHpMJb4JqBPVFU/Fu0mNxb eb0Xss+Vq22eFvz03R86iv/DdYJAN58IfI18IjbvydagtzbD/6hgMOF8RF36pSOaWKvX pAB4bgV7h4YETGmHza9YASc+LOlAa3PvhJz9m4AMJmtBnjL9atqeOMHZrgNGC+6GVuDD NwOgiqcYWjMQJ7lsei07CSaYttQ2JKTzdC7+JbLsjkWLbvWKV3Nlo6BCoFynAwi17IaQ SDdQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20251104; t=1788890532; x=1789495332; h=in-reply-to:content-disposition:content-type:mime-version :references:message-id:subject:cc:to:from:date:x-gm-gg :x-gm-message-state:from:to:cc:subject:date:message-id:reply-to :content-type; bh=/bkSweG2AjV1rpphqtQ+x4kgo6UUQ9p9UEyix7QWjvg=; b=H8qUFg3l819o+oPr0h3VlJWa59+DvgYW2RTalcozTmKxwPHZkkKlJEHGzD3B3oQZB6 EwOikHW5G2vl8P9XLyzoE5CFCoIGoRU/4ZAukwHlo3tfwIHIOxCV2DmkiGw7rpZ/4gIU layIpP94NAMmJQ1u+AIA0fG6tjN+levhLfXWWPZ9zmZk6KKkJ4QWuau+dDTSAHd3HQ8X OVaApHgZUHx7X76uhQCkPXAoCGGT1KQI7tv1scenJfeGsvnBaXIC2VAMZ05C5DWtX4ff aZ5hxLN89+4yQ4ZSJERGUBG0JQv0gZi5+DMmw96QNBxQ9SyzSE3N7tKNKHyP9o8N3SfK peeQ== X-Forwarded-Encrypted: i=1; AKwUvBzpTKnWtPuKpkgCChbJatAXPKOnw6or/7Gdv8dfyTEuoI9W5u80kuB0IQ8wD+fGpl6CprSMDID5cUwdRIM=@vger.kernel.org X-Gm-Message-State: AFuF++lwnm5h8qNHV6cdD8EXm4p0rY4WEQbvETyE4pBvwbtHQkgrb9IR 7LEPH/zsoIGd/tPa9mMmEY72h3zVP4mkSQj9bry+jvhLbOV6DlCnfoxQy6+SPoaPGn+L/NSVUVb Iw23u X-Gm-Gg: AYBFou2CVGsxIMeI4YF1toSTtukhCqnhjSivTsjMGS7VTlf15Ofz1Hp2JlrrDZTD2JP YrUkASfFrqy5KRBAogsXwH/7z/DbcwjsrTMf3bdA4RpuQtAixHNrzvPz2XQGnzcVwBZu9vjxQUE 4+vyyxXwQk40dPgg9Rz8UQK9yPOJ1bJ84P9WANbEp7xrDHZqPY5Ky8eIHOJnSL4W/L/fya2SVOh Pvd9K5WiwiSh0+oHGnCTFYqZiOEdOjrHVo9R8583W2nVO1g9Ybve13WznV+D8p7eznI1tKYa5wd FsP+AkTWf6czWdWDqK8JsetSBT5iXuw5uDqeAEn4Ln2GWSWNigYmujD2SNmZwyNDT30UkL1Jyy6 jnU3nf05F3wRyiUzB23QgD8w5C/1yHzUTFH36G7mDwbQUcW82nP1nJAOk+cMbUY0X7xLOjaBTJz BJhX5Epl5+Rdgm8w3JKfAEf+yVn17z4f5a77HiCpl3P0Nwghk1ZB9kgK9onBRVgaIcwcxBSaIeV mw= X-Received: by 2002:a17:90b:3b8d:b0:398:b426:ca4b with SMTP id 98e67ed59e1d1-39b2624f10amr44409664a91.22.1788890531955; Tue, 08 Sep 2026 11:02:11 -0700 (PDT) Received: from p14s ([2604:3d09:148c:c800:14af:4aef:7bc8:f5bc]) by smtp.gmail.com with ESMTPSA id 98e67ed59e1d1-39b25f8d2c5sm28055519a91.2.2026.09.08.11.02.10 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Tue, 08 Sep 2026 11:02:11 -0700 (PDT) Date: Tue, 8 Sep 2026 12:02:09 -0600 From: Mathieu Poirier To: Tanmay Shah Cc: andersson@kernel.org, linux-remoteproc@vger.kernel.org, linux-kernel@vger.kernel.org Subject: Re: [PATCH] remoteproc: remoteproc_virtio: add acknowledged vdev reset Message-ID: References: <20260902214453.634339-2-tanmay.shah@amd.com> Precedence: bulk X-Mailing-List: linux-kernel@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20260902214453.634339-2-tanmay.shah@amd.com> Good day, On Wed, Sep 02, 2026 at 02:44:54PM -0700, Tanmay Shah wrote: > The existing remoteproc virtio reset path clears the vdev status locally > without notifying the remote processor. As a result, the host cannot tell > whether the remote side has observed the reset request or completed its > cleanup. > > Add a new resource type, RSC_VDEV_V2, for virtio vdevs that support an > acknowledged reset protocol. For these resources, encode a reset request > in the virtio status byte, kick the remote processor using the vdev notify > ID, and wait for the remote side to clear the status back to 0. > > Keep the existing RSC_VDEV behavior for backwards compatibility by > clearing the status locally. Also reset remoteproc-created virtio > devices before unregistering them, and expose RSC_VDEV_V2 reset state > in debugfs. > > Assisted-by: Codex:GPT-5 > Signed-off-by: Tanmay Shah > --- > drivers/remoteproc/remoteproc_core.c | 3 +- > drivers/remoteproc/remoteproc_debugfs.c | 29 +++++++++++++++- > drivers/remoteproc/remoteproc_internal.h | 21 +++++++++++ > drivers/remoteproc/remoteproc_virtio.c | 44 ++++++++++++++++++++++-- > include/linux/rsc_table.h | 5 ++- > 5 files changed, 97 insertions(+), 5 deletions(-) > > diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c > index 1ed406714849..31d79684977c 100644 > --- a/drivers/remoteproc/remoteproc_core.c > +++ b/drivers/remoteproc/remoteproc_core.c > @@ -471,6 +471,7 @@ void rproc_remove_rvdev(struct rproc_vdev *rvdev) > static int rproc_handle_vdev(struct rproc *rproc, void *ptr, > int offset, int avail) > { > + struct fw_rsc_hdr *hdr = ptr - sizeof(*hdr); Spurious change. > struct fw_rsc_vdev *rsc = ptr; > struct device *dev = &rproc->dev; > struct rproc_vdev *rvdev; > @@ -485,7 +486,6 @@ static int rproc_handle_vdev(struct rproc *rproc, void *ptr, > return -EINVAL; > } > > - /* make sure reserved bytes are zeroes */ Same > if (rsc->reserved[0] || rsc->reserved[1]) { > dev_err(dev, "vdev rsc has non zero reserved bytes\n"); > return -EINVAL; > @@ -1009,6 +1009,7 @@ static rproc_handle_resource_t rproc_loading_handlers[RSC_LAST] = { > [RSC_DEVMEM] = rproc_handle_devmem, > [RSC_TRACE] = rproc_handle_trace, > [RSC_VDEV] = rproc_handle_vdev, > + [RSC_VDEV_V2] = rproc_handle_vdev, > }; > > struct rproc_rsc_cb_data { > diff --git a/drivers/remoteproc/remoteproc_debugfs.c b/drivers/remoteproc/remoteproc_debugfs.c > index b86c1d09c70c..1fe99749f5b4 100644 > --- a/drivers/remoteproc/remoteproc_debugfs.c > +++ b/drivers/remoteproc/remoteproc_debugfs.c > @@ -274,7 +274,7 @@ static const struct file_operations rproc_crash_ops = { > /* Expose resource table content via debugfs */ > static int rproc_rsc_table_show(struct seq_file *seq, void *p) > { > - static const char * const types[] = {"carveout", "devmem", "trace", "vdev"}; > + static const char * const types[] = {"carveout", "devmem", "trace", "vdev", "vdev_v2"}; > struct rproc *rproc = seq->private; > struct resource_table *table = rproc->table_ptr; > struct fw_rsc_carveout *c; > @@ -336,6 +336,33 @@ static int rproc_rsc_table_show(struct seq_file *seq, void *p) > seq_printf(seq, " Reserved (should be zero) [%d][%d]\n\n", > v->reserved[0], v->reserved[1]); > > + for (j = 0; j < v->num_of_vrings; j++) { > + seq_printf(seq, " Vring %d\n", j); > + seq_printf(seq, " Device Address 0x%x\n", v->vring[j].da); > + seq_printf(seq, " Alignment %d\n", v->vring[j].align); > + seq_printf(seq, " Number of buffers %d\n", v->vring[j].num); > + seq_printf(seq, " Notify ID %d\n", v->vring[j].notifyid); > + seq_printf(seq, " Physical Address 0x%x\n\n", > + v->vring[j].pa); > + } > + break; > + case RSC_VDEV_V2: > + v = rsc; > + seq_printf(seq, "Entry %d is of type %s\n", i, types[hdr->type]); > + > + seq_printf(seq, " ID %d\n", v->id); > + seq_printf(seq, " Notify ID %d\n", v->notifyid); > + seq_printf(seq, " Device features 0x%x\n", v->dfeatures); > + seq_printf(seq, " Guest features 0x%x\n", v->gfeatures); > + seq_printf(seq, " Config length 0x%x\n", v->config_len); > + seq_printf(seq, " Status 0x%x\n", v->status); > + seq_printf(seq, " Number of vrings %d\n", v->num_of_vrings); > + seq_printf(seq, " Reset request pending %s\n", > + rproc_rsc_vdev_reset_requested(v->status) ? > + "yes" : "no"); > + seq_printf(seq, " Reserved (should be zero) [%d][%d]\n\n", > + v->reserved[0], v->reserved[1]); > + > for (j = 0; j < v->num_of_vrings; j++) { > seq_printf(seq, " Vring %d\n", j); > seq_printf(seq, " Device Address 0x%x\n", v->vring[j].da); > diff --git a/drivers/remoteproc/remoteproc_internal.h b/drivers/remoteproc/remoteproc_internal.h > index 3a742ef6ef60..f07a96ff82a4 100644 > --- a/drivers/remoteproc/remoteproc_internal.h > +++ b/drivers/remoteproc/remoteproc_internal.h > @@ -14,6 +14,7 @@ > > #include > #include > +#include > #ifdef CONFIG_HAS_IOMEM > #include > #endif > @@ -42,6 +43,26 @@ struct rproc_vdev_data { > struct fw_rsc_vdev *rsc; > }; > > +/* > + * RSC_VDEV_V2 requests an acknowledged reset by writing an otherwise > + * impossible virtio status pattern: DRIVER and FAILED set while > + * ACKNOWLEDGE is clear. Other status bits are left unchanged. > + */ > +static inline u8 rproc_rsc_vdev_reset_status(u8 status) > +{ > + status |= VIRTIO_CONFIG_S_DRIVER | VIRTIO_CONFIG_S_FAILED; > + status &= ~VIRTIO_CONFIG_S_ACKNOWLEDGE; > + > + return status; > +} > + > +static inline bool rproc_rsc_vdev_reset_requested(u8 status) > +{ > + return !(status & VIRTIO_CONFIG_S_ACKNOWLEDGE) && > + (status & VIRTIO_CONFIG_S_DRIVER) && > + (status & VIRTIO_CONFIG_S_FAILED); > +} > + > static inline bool rproc_has_feature(struct rproc *rproc, unsigned int feature) > { > return test_bit(feature, rproc->features); > diff --git a/drivers/remoteproc/remoteproc_virtio.c b/drivers/remoteproc/remoteproc_virtio.c > index d5e9ff045a28..e682caa546b2 100644 > --- a/drivers/remoteproc/remoteproc_virtio.c > +++ b/drivers/remoteproc/remoteproc_virtio.c > @@ -13,6 +13,7 @@ > #include > #include > #include > +#include > #include > #include > #include > @@ -234,12 +235,48 @@ static void rproc_virtio_set_status(struct virtio_device *vdev, u8 status) > static void rproc_virtio_reset(struct virtio_device *vdev) > { > struct rproc_vdev *rvdev = vdev_to_rvdev(vdev); > + struct rproc *rproc = rvdev->rproc; > struct fw_rsc_vdev *rsc; > + struct fw_rsc_hdr *hdr; > + int ret; > + u8 val; > + > + /* > + * During crash recovery, vdev can be stopped. But the driver can't reset > + * the device, as device is already crashed. In this case, reset becomes > + * no op. > + */ > + if (rproc->state == RPROC_CRASHED) > + return; > > rsc = (void *)rvdev->rproc->table_ptr + rvdev->rsc_offset; > + hdr = (void *)rsc - sizeof(*hdr); > + > + if (hdr->type == RSC_VDEV_V2) { > + /* > + * RSC_VDEV_V2 encodes an acknowledged reset request in the > + * status byte. The remote is expected to complete the reset > + * and then clear status back to 0. > + */ > + rsc->status = rproc_rsc_vdev_reset_status(rsc->status); > + > + /* after setting reset request, kick the device */ > + rproc->ops->kick(rproc, rsc->notifyid); > > - rsc->status = 0; > - dev_dbg(&vdev->dev, "reset !\n"); > + /* > + * When device completes reset, it is expected to set status > + * to 0. > + */ > + ret = readb_poll_timeout(&rsc->status, val, val == 0, > + 1000, /* 1ms between reads */ > + 3000000); /* 3s total timeout */ > + if (ret) > + dev_warn(&vdev->dev, "vdev reset timed out\n"); The problem here is that we are introducing behavior that is not compliant with the virtio specifications. One way to acheive the same behavior could be for the remote processor to check rsc->status before sending a interrupt of using the virtqueues. > + } else { > + /* back compatible for RSC_VDEV type of rsc vdev */ > + rsc->status = 0; > + } > + dev_info(&vdev->dev, "reset !\n"); > } > > /* provide the vdev features as retrieved from the firmware */ > @@ -469,6 +506,9 @@ static int rproc_remove_virtio_dev(struct device *dev, void *data) > { > struct virtio_device *vdev = dev_to_virtio(dev); > > + /* reset virtio device before unregister */ > + virtio_reset_device(vdev); > + Regardless of this feature, I think it is wise to reset the device before unregistering with the virtio subsystem. Thanks, Mathieu > unregister_virtio_device(vdev); > return 0; > } > diff --git a/include/linux/rsc_table.h b/include/linux/rsc_table.h > index 71b60125310e..2398a6d7033e 100644 > --- a/include/linux/rsc_table.h > +++ b/include/linux/rsc_table.h > @@ -66,6 +66,8 @@ struct fw_rsc_hdr { > * the remote processor will be writing logs. > * @RSC_VDEV: declare support for a virtio device, and serve as its > * virtio header. > + * @RSC_VDEV_V2: declare support for a virtio device whose reset request is > + * encoded in the virtio status byte. > * @RSC_LAST: just keep this one at the end of standard resources > * @RSC_VENDOR_START: start of the vendor specific resource types range > * @RSC_VENDOR_END: end of the vendor specific resource types range > @@ -83,7 +85,8 @@ enum fw_resource_type { > RSC_DEVMEM = 1, > RSC_TRACE = 2, > RSC_VDEV = 3, > - RSC_LAST = 4, > + RSC_VDEV_V2 = 4, > + RSC_LAST = 5, > RSC_VENDOR_START = 128, > RSC_VENDOR_END = 512, > }; > > base-commit: d4d61a4b0a52e8f3cdb3e1578602850a3452ec3e > -- > 2.43.0 >