Message ID | 20210801051627.78999-1-xianting.tian@linux.alibaba.com (mailing list archive) |
---|---|
State | Not Applicable |
Headers | show |
Series | [1/2] tty: hvc: pass DMA capable memory to put_chars() | expand |
Related | show |
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
Hi Xianting, Thank you for the patch! Yet something to improve: [auto build test ERROR on tty/tty-testing] [also build test ERROR on char-misc/char-misc-testing soc/for-next linus/master v5.14-rc3 next-20210730] [If your patch is applied to the wrong git tree, kindly drop us a note. And when submitting patch, we suggest to use '--base' as documented in https://git-scm.com/docs/git-format-patch] url: https://github.com/0day-ci/linux/commits/Xianting-Tian/tty-hvc-pass-DMA-capable-memory-to-put_chars/20210801-131800 base: https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/tty.git tty-testing config: sparc64-allyesconfig (attached as .config) compiler: sparc64-linux-gcc (GCC) 10.3.0 reproduce (this is a W=1 build): wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross chmod +x ~/bin/make.cross # https://github.com/0day-ci/linux/commit/b33d1f04db78e1bfa5d798b676bd1062e2d54afc git remote add linux-review https://github.com/0day-ci/linux git fetch --no-tags linux-review Xianting-Tian/tty-hvc-pass-DMA-capable-memory-to-put_chars/20210801-131800 git checkout b33d1f04db78e1bfa5d798b676bd1062e2d54afc # save the attached .config to linux build tree mkdir build_dir COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-10.3.0 make.cross O=build_dir ARCH=sparc64 SHELL=/bin/bash If you fix the issue, kindly add following tag as appropriate Reported-by: kernel test robot <lkp@intel.com> All errors (new ones prefixed by >>): drivers/tty/hvc/hvc_console.c: In function 'hvc_poll_put_char': >> drivers/tty/hvc/hvc_console.c:888:14: error: 'ch' redeclared as different kind of symbol 888 | static char ch = ch; | ^~ drivers/tty/hvc/hvc_console.c:883:73: note: previous definition of 'ch' was here 883 | static void hvc_poll_put_char(struct tty_driver *driver, int line, char ch) | ~~~~~^~ >> drivers/tty/hvc/hvc_console.c:888:19: error: initializer element is not constant 888 | static char ch = ch; | ^~ vim +/ch +888 drivers/tty/hvc/hvc_console.c 882 883 static void hvc_poll_put_char(struct tty_driver *driver, int line, char ch) 884 { 885 struct tty_struct *tty = driver->ttys[0]; 886 struct hvc_struct *hp = tty->driver_data; 887 int n; > 888 static char ch = ch; 889 890 do { 891 n = hp->ops->put_chars(hp->vtermno, &ch, 1); 892 } while (n <= 0); 893 } 894 #endif 895 --- 0-DAY CI Kernel Test Service, Intel Corporation https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
diff --git a/drivers/tty/hvc/hvc_console.c b/drivers/tty/hvc/hvc_console.c index 5bb8c4e44..f7a7fa083 100644 --- a/drivers/tty/hvc/hvc_console.c +++ b/drivers/tty/hvc/hvc_console.c @@ -151,9 +151,10 @@ static uint32_t vtermnos[MAX_NR_HVC_CONSOLES] = static void hvc_console_print(struct console *co, const char *b, unsigned count) { - char c[N_OUTBUF] __ALIGNED__; + char *c; unsigned i = 0, n = 0; int r, donecr = 0, index = co->index; + struct hvc_struct *hp; /* Console access attempt outside of acceptable console range. */ if (index >= MAX_NR_HVC_CONSOLES) @@ -163,8 +164,14 @@ static void hvc_console_print(struct console *co, const char *b, if (vtermnos[index] == -1) return; + list_for_each_entry(hp, &hvc_structs, next) + if (hp->vtermno == vtermnos[index]) + break; + + c = &hp->outbuf[hp->outbuf_size]; + while (count > 0 || i > 0) { - if (count > 0 && i < sizeof(c)) { + if (count > 0 && i < N_OUTBUF) { if (b[n] == '\n' && !donecr) { c[i++] = '\r'; donecr = 1; @@ -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); @@ -922,7 +930,7 @@ struct hvc_struct *hvc_alloc(uint32_t vtermno, int data, return ERR_PTR(err); } - hp = kzalloc(ALIGN(sizeof(*hp), sizeof(long)) + outbuf_size, + hp = kzalloc(ALIGN(sizeof(*hp), sizeof(long)) + outbuf_size + N_OUTBUF, GFP_KERNEL); if (!hp) return ERR_PTR(-ENOMEM);