linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Linus Torvalds <torvalds@linux-foundation.org>
To: Takashi Iwai <tiwai@suse.de>
Cc: Mark Brown <broonie@kernel.org>,
	Liam Girdwood <lgirdwood@gmail.com>,
	Linux Kernel Mailing List <linux-kernel@vger.kernel.org>
Subject: Re: [GIT PULL] sound updates for 6.6-rc1
Date: Wed, 30 Aug 2023 14:17:36 -0700	[thread overview]
Message-ID: <CAHk-=wje+VkXjjfVTmK-uJdG_M5=ar14QxAwK+XDiq07k_pzBg@mail.gmail.com> (raw)
In-Reply-To: <87msy8hicb.wl-tiwai@suse.de>

On Wed, 30 Aug 2023 at 04:37, Takashi Iwai <tiwai@suse.de> 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.

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.

                 Linus

  reply	other threads:[~2023-08-30 21:19 UTC|newest]

Thread overview: 4+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2023-08-30 11:37 [GIT PULL] sound updates for 6.6-rc1 Takashi Iwai
2023-08-30 21:17 ` Linus Torvalds [this message]
2023-08-31  6:51   ` Takashi Iwai
2023-08-30 21:40 ` pr-tracker-bot

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='CAHk-=wje+VkXjjfVTmK-uJdG_M5=ar14QxAwK+XDiq07k_pzBg@mail.gmail.com' \
    --to=torvalds@linux-foundation.org \
    --cc=broonie@kernel.org \
    --cc=lgirdwood@gmail.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=tiwai@suse.de \
    /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).