Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <20210801051627.78999-1-xianting.tian@linux.alibaba.com>
In-Reply-To: <20210801051627.78999-1-xianting.tian@linux.alibaba.com>
From:   Arnd Bergmann <arnd@arndb.de>
Date:   Sun, 1 Aug 2021 11:24:15 +0200
Message-ID: <CAK8P3a2_ip1zGMe=EeAA7Xpkvi8iQGWw6=0sGvLqv02Mj4LrmA@mail.gmail.com>
Subject: Re: [PATCH 1/2] tty: hvc: pass DMA capable memory to put_chars()
To:     Xianting Tian <xianting.tian@linux.alibaba.com>
Cc:     gregkh <gregkh@linuxfoundation.org>,
        Jiri Slaby <jirislaby@kernel.org>, Amit Shah <amit@kernel.org>,
        Arnd Bergmann <arnd@arndb.de>,
        linuxppc-dev <linuxppc-dev@lists.ozlabs.org>,
        "open list:DRM DRIVER FOR QEMU'S CIRRUS DEVICE" 
        <virtualization@lists.linux-foundation.org>,
        Linux Kernel Mailing List <linux-kernel@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: 8BIT
Precedence: bulk

On Sun, Aug 1, 2021 at 7:16 AM Xianting Tian
<xianting.tian@linux.alibaba.com> wrote:

> Considering lock competition of hp->outbuf and the complicated logic in
> hvc_console_print(), I didn’t use hp->outbuf, just allocate additional
> memory(length N_OUTBUF) and append it to hp->outbuf.
> For the issue in hvc_poll_put_char(), I use a static char to replace
> the char in stack.

While this may work, it sounds rather obscure to me, I don't think
it's a good idea
to append the buffer at the back.

If you need a separate field besides hp->outbuf, I would make that part of the
structure itself, and give it the correct alignment constraints to ensure it is
in a cache line by itself. The size of this field is a compile-time
constant, so I
don't see a need to play tricks with pointer arithmetic.

I'm not sure about the locking either: Is it possible for two CPUs to enter
hvc_console_print() at the same time, or is there locking at a higher level
already? It would be good to document this in the structure definition next
to the field.

> @@ -878,6 +885,7 @@ static void hvc_poll_put_char(struct tty_driver *driver, int line, char ch)
>         struct tty_struct *tty = driver->ttys[0];
>         struct hvc_struct *hp = tty->driver_data;
>         int n;
> +       static char ch = ch;
>
>         do {
>                 n = hp->ops->put_chars(hp->vtermno, &ch, 1);

This does not compile, and it's neither thread-safe nor dma safe if you get it
to build by renaming the variable.

        Arnd