From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by smtp.lore.kernel.org (Postfix) with ESMTP id 8A52DC83F10 for ; Thu, 31 Aug 2023 06:52:08 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S240457AbjHaGwJ (ORCPT ); Thu, 31 Aug 2023 02:52:09 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:40634 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S234006AbjHaGwI (ORCPT ); Thu, 31 Aug 2023 02:52:08 -0400 Received: from smtp-out2.suse.de (smtp-out2.suse.de [195.135.220.29]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 810BFCDD for ; Wed, 30 Aug 2023 23:52:01 -0700 (PDT) Received: from imap2.suse-dmz.suse.de (imap2.suse-dmz.suse.de [192.168.254.74]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature ECDSA (P-521) server-digest SHA512) (No client certificate requested) by smtp-out2.suse.de (Postfix) with ESMTPS id 374BE1F747; Thu, 31 Aug 2023 06:52:00 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=suse.de; s=susede2_rsa; t=1693464720; h=from:from:reply-to:date:date:message-id:message-id:to:to:cc:cc: mime-version:mime-version:content-type:content-type: in-reply-to:in-reply-to:references:references; bh=wT7p9xQfpwS081tUsfVqMhTHOZAl/BZREkaQZK/c9U4=; b=CI0E0wajg+ePG3uJcnIxuRfC8ePvfK3hz6lMEYXKZR16tueJQxDqvlwtpLtQ3bL/CVxG0l J7SeQYb8x0vXkIpj89bJL4LL6FxgKfMCIzFi20BEm5KB2dF2EY0wbFiDwwKqhHIi6vD1gr h6d7GtI04bNHK5zJaAWrGMeW0K7yOSQ= DKIM-Signature: v=1; a=ed25519-sha256; c=relaxed/relaxed; d=suse.de; s=susede2_ed25519; t=1693464720; h=from:from:reply-to:date:date:message-id:message-id:to:to:cc:cc: mime-version:mime-version:content-type:content-type: in-reply-to:in-reply-to:references:references; bh=wT7p9xQfpwS081tUsfVqMhTHOZAl/BZREkaQZK/c9U4=; b=0QEVwAkoF2ulHn/UHRifOa8FekNWiTG+EAnLgvW/I1k31nT8Y1vZhHas0PN29cXsr/JEsf BBINpwewg5C0ujAg== Received: from imap2.suse-dmz.suse.de (imap2.suse-dmz.suse.de [192.168.254.74]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature ECDSA (P-521) server-digest SHA512) (No client certificate requested) by imap2.suse-dmz.suse.de (Postfix) with ESMTPS id 07F0B13583; Thu, 31 Aug 2023 06:52:00 +0000 (UTC) Received: from dovecot-director2.suse.de ([192.168.254.65]) by imap2.suse-dmz.suse.de with ESMTPSA id VkzxAJA48GSgQwAAMHmgww (envelope-from ); Thu, 31 Aug 2023 06:52:00 +0000 Date: Thu, 31 Aug 2023 08:51:59 +0200 Message-ID: <878r9rsnzk.wl-tiwai@suse.de> From: Takashi Iwai To: Linus Torvalds Cc: Takashi Iwai , Mark Brown , Liam Girdwood , Linux Kernel Mailing List Subject: Re: [GIT PULL] sound updates for 6.6-rc1 In-Reply-To: References: <87msy8hicb.wl-tiwai@suse.de> User-Agent: Wanderlust/2.15.9 (Almost Unreal) Emacs/27.2 Mule/6.0 MIME-Version: 1.0 (generated by SEMI-EPG 1.14.7 - "Harue") Content-Type: text/plain; charset=US-ASCII Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Wed, 30 Aug 2023 23:17:36 +0200, Linus Torvalds wrote: > > On Wed, 30 Aug 2023 at 04:37, Takashi Iwai wrote: > > > > - Unified PCM copy ops with iov_iter > > So I know I suggested this, but I think some of it is seriously buggy. > > In particular, look at dmaengine_copy(). > > It was *really* completely and unacceptbly broken at one point, when it did that > > void *ptr = (void __force *)iter_iov_addr(buf); > > which is complete garbage in so many ways. That was removed by commit > 9bebd65443c1 ("ASoC: dmaengine: Use iov_iter for process callback, > too"), and the end result looks superficially much better. > > But the key word there is "superficially". The end result is still > completely broken as far as I can see. > > Why? Because the code does > > if (is_playback) > if (copy_from_iter(dma_ptr, bytes, buf) != bytes) > return -EFAULT; > > if (process) { > int ret = process(substream, channel, hwoff, buf, bytes); > if (ret < 0) > ... > > and notice how the "is_playback" has already *used* the iter in 'buf', > and has advanced the iterator. > > So that operation is completely nonsensical. > > Now, the commit message says "(although both atmel and stm drivers > don't use the given buffer address at all for now)", which may be the > only thing that saves the code from being broken. > > Or rather, it's completely broken, but it is not broken in actual > *noticeable* ways. > > Please just remove that useless iter argument. You simply cannot do > what that code tries to do. OK, makes sense. > There are alternatives, which involve either "dup_iter()" or > "iov_iter_save_state() / iov_iter_restore()". So using an iter twice > can be made to work, but not the way you do it. > > Can I also please ask you to not use a name like "buf" for an > iterator. It's not a buffer. You must not think of it as a buffer. > Thinking of it as a buffer is what made the above nonsensical code > happen. > > It's literally an _iterator_. There's a buffer somewhere behind it, > but that thing itself does *not* act as a buffer. It acts as a > descriptor of where in the buyffer you are, which is exactly why you > can't then re-use it twice as if it was some "buffer". > > So please - when you change a buffer interface to use an iterator, > change the variable name. Don't make mistakes like the above. OK, will cover in the follow up patch. thanks, Takashi