Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider: - [Medium] ALSA: virtio: DMA cacheline sharing in virtsnd_pcm_capture_send() - [Low] ALSA: virtio: 64-bit division and modulo on u64 capture counters break 32-bit builds --
commit b45632282ebc50eaee6059eb5173344a95c32ede Author: Joonho Lee <[email protected]> ALSA: virtio: stage VZ capture and restart paused streams This patch adds opt-in capture staging using request-owned buffers with response validation and ring accounting. It also introduces a pause release and reprepare state machine for staged capture streams. > diff --git a/sound/virtio/virtio_pcm.c b/sound/virtio/virtio_pcm.c > index be3893de40a55..a3478fc8848db 100644 > --- a/sound/virtio/virtio_pcm.c > +++ b/sound/virtio/virtio_pcm.c > @@ -310,8 +310,94 @@ static void virtsnd_pcm_period_elapsed(struct > work_struct *work) > { [ ... ] > + period_bytes = snd_pcm_lib_period_bytes(substream); > + if (vss->xfer_xrun) { > + notify = true; > + } else if (!period_bytes || period_bytes >= vss->buffer_bytes || > + !virtsnd_pcm_capture_counters_valid(vss) || > + vss->capture_notified_periods > > + vss->capture_copied_total / period_bytes) { > + vss->xfer_xrun = true; > + notify = true; > + } else { > + period_floor = vss->capture_copied_total / period_bytes; > + if (period_floor > vss->capture_notified_periods) { > + /* Reserve crossed boundaries before the > callback. */ > + vss->capture_notified_periods = period_floor; > + notify = true; > + } > + } [ ... ] > diff --git a/sound/virtio/virtio_pcm.h b/sound/virtio/virtio_pcm.h > index 61faf7b13cbc1..e143a15592f05 100644 > --- a/sound/virtio/virtio_pcm.h > +++ b/sound/virtio/virtio_pcm.h [ ... ] > +static inline bool > +virtsnd_pcm_capture_counters_valid(const struct virtio_pcm_substream *vss) > +{ > + return vss->buffer_bytes && > + vss->capture_copied_total >= vss->capture_acked_total && > + vss->capture_copied_total - vss->capture_acked_total == > + (u64)vss->capture_ready && > + vss->capture_ready < vss->buffer_bytes && > + vss->capture_pointer_seen_total <= vss->capture_copied_total && > + vss->hw_ptr == vss->capture_copied_total % vss->buffer_bytes && > + (!vss->capture_callback_snapshot_active || > + (vss->capture_callback_snapshot >= > + vss->capture_pointer_seen_total && > + vss->capture_callback_snapshot <= > + vss->capture_copied_total)); > +} [Severity: Low] Will these raw 64-bit divisions and modulo operations break 32-bit builds with undefined references to __udivdi3 and __umoddi3? Here in virtsnd_pcm_capture_counters_valid() (sound/virtio/virtio_pcm.h) and virtsnd_pcm_period_elapsed() (sound/virtio/virtio_pcm.c), as well as in virtsnd_pcm_capture_complete() (sound/virtio/virtio_pcm_msg.c): else if (vss->hw_ptr != vss->capture_copied_total % vss->buffer_bytes) ... size_t ring_ptr = vss->capture_copied_total % vss->buffer_bytes; ... vss->hw_ptr = next_copied_total % vss->buffer_bytes; and in virtsnd_pcm_cp_pointer() (sound/virtio/virtio_pcm_ops.c): hw_ptr = total % vss->buffer_bytes; the u64 counters (capture_copied_total, next_copied_total, and total) are divided by or taken modulo a size_t value (vss->buffer_bytes or period_bytes). On 32-bit architectures, usual arithmetic conversions promote the size_t operand to u64, causing the compiler to emit __udivdi3 and __umoddi3 calls. Should these use helpers from linux/math64.h such as div64_ul() or div_u64_rem() instead? > diff --git a/sound/virtio/virtio_pcm_msg.c b/sound/virtio/virtio_pcm_msg.c > index b54642a9521db..795e52e09564b 100644 > --- a/sound/virtio/virtio_pcm_msg.c > +++ b/sound/virtio/virtio_pcm_msg.c > @@ -16,6 +17,7 @@ > * @xfer: Request header payload. > * @status: Response header payload. > * @length: Data length in bytes. > + * @data: Request-owned payload for staged capture. > * @data_offset: Offset in the driver-owned PCM buffer on NOMMU systems. > * @sgs: Payload scatter-gather table. > */ > @@ -24,6 +26,7 @@ struct virtio_pcm_msg { > struct virtio_snd_pcm_xfer xfer; > struct virtio_snd_pcm_status status; > size_t length; > + u8 *data; > #ifndef CONFIG_MMU > size_t data_offset; > #endif [ ... ] > + for (i = 0; i < vss->nmsgs && vss->msg_count < request_limit; ++i) { > + struct virtio_pcm_msg *msg = vss->msgs[i]; > + struct scatterlist *sgs[] = { > + &msg->sgs[PCM_MSG_SG_XFER], > + &msg->sgs[PCM_MSG_SG_DATA], > + &msg->sgs[PCM_MSG_SG_STATUS] > + }; > + int rc; > + > + if (msg->length) > + continue; > + > + msg->xfer.stream_id = cpu_to_le32(vss->sid); > + /* Do not let an absent packed trailer reuse old PCM bytes. */ > + memset(msg->data, 0, period_bytes); > + memset(&msg->status, 0, sizeof(msg->status)); > + msg->status.status = cpu_to_le32(VIRTIO_SND_S_BAD_MSG); > + rc = virtqueue_add_sgs(vqueue, sgs, 1, 2, msg, GFP_ATOMIC); > + if (rc) { > + if (added && !(vss->features & > + (1U << VIRTIO_SND_PCM_F_MSG_POLLING)) && > + virtqueue_kick_prepare(vqueue)) > + virtqueue_notify(vqueue); > + return rc; > + } > + > + msg->length = period_bytes; > + vss->msg_count++; > + added = true; > + } [Severity: Medium] Can touching msg->length after virtqueue_add_sgs() in virtsnd_pcm_capture_send() corrupt DMA transfers on non-coherent platforms? In struct virtio_pcm_msg, status, length, and data are adjacent fields in the same slab allocation and share a CPU cacheline. When virtsnd_pcm_capture_send() calls virtqueue_add_sgs(), msg->status is mapped for device-to-driver DMA. Immediately after virtqueue_add_sgs() returns, virtsnd_pcm_capture_send() writes msg->length = period_bytes, and subsequent calls to virtsnd_pcm_capture_send() read msg->length while the descriptor is still queued. On non-coherent DMA systems where virtio uses the DMA API, dirtying the cacheline containing msg->length after msg->status is mapped can cause a later cacheline writeback to overwrite the device's DMA write to msg->status, or cache invalidation during DMA unmap in virtqueue_get_buf() can discard the write to msg->length so virtsnd_pcm_capture_complete() sees msg->length == 0 and triggers a false xfer_xrun. -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=7
