Messages in this thread | | | Date | Tue, 2 Nov 2021 07:33:31 +0100 | Subject | Re: [PATCH v12 1/2] tty: hvc: pass DMA capable memory to put_chars() | From | Jiri Slaby <> |
| |
On 28. 10. 21, 17:09, Xianting Tian wrote: > As well known, hvc backend can register its opertions to hvc backend. > the operations contain put_chars(), get_chars() and so on. > > Some hvc backend may do dma in its operations. eg, put_chars() of > virtio-console. But in the code of hvc framework, it may pass DMA > incapable memory to put_chars() under a specific configuration, which > is explained in commit c4baad5029(virtio-console: avoid DMA from stack): > 1, c[] is on stack, > hvc_console_print(): > char c[N_OUTBUF] __ALIGNED__; > cons_ops[index]->put_chars(vtermnos[index], c, i); > 2, ch is on stack, > static void hvc_poll_put_char(,,char ch) > { > struct tty_struct *tty = driver->ttys[0]; > struct hvc_struct *hp = tty->driver_data; > int n; > > do { > n = hp->ops->put_chars(hp->vtermno, &ch, 1); > } while (n <= 0); > } > > Commit c4baad5029 is just the fix to avoid DMA from stack memory, which > is passed to virtio-console by hvc framework in above code. But I think > the fix is aggressive, it directly uses kmemdup() to alloc new buffer > from kmalloc area and do memcpy no matter the memory is in kmalloc area > or not. But most importantly, it should better be fixed in the hvc > framework, by changing it to never pass stack memory to the put_chars() > function in the first place. Otherwise, we still face the same issue if > a new hvc backend using dma added in the furture. > > In this patch, add 'char cons_outbuf[]' as part of 'struct hvc_struct', > so hp->cons_outbuf is no longer the stack memory, we can use it in above > cases safely. We also add lock to protect cons_outbuf instead of using > the global lock of hvc. > > Introduce array cons_hvcs[] for hvc pointers next to the cons_ops[] and > vtermnos[] arrays. With the array, we can easily find hvc's cons_outbuf > and its lock.
Hi,
this is still overly complicated IMO. As I already noted in: https://lore.kernel.org/all/5b728c71-a754-b3ef-4ad3-6e33db1b7647@kernel.org/
this: ============= In fact, you need only a single char for the poll case (hvc_poll_put_char), so hvc_struct needs to contain only c, not an array.
OTOH, you need c[N_OUTBUF] in the console case (hvc_console_print), but not whole hvc_struct. So cons_hvcs should be an array of structs composed of only the lock and the buffer. =============
And I would do it even simpler now. One c[N_OUTBUF] buffer for all consoles and a single lock.
And "char c" in struct hvc_struct.
No need for the complex logic in hvc_console_print.
> Introduce array cons_early_outbuf[] to ensure the mechanism of early > console still work normally.
thanks, -- js suse labs
| |