As well known, hvc backend driver(eg, virtio-console) can register its
operations to hvc framework. The operations can 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 future.
In this patch, we make 'char out_buf[N_OUTBUF]' and 'chat out_ch' part
of 'struct hvc_struct', so both two buf are no longer the stack memory.
we can use it in above two cases separately.
Introduce another array(cons_outbufs[]) for buffer pointers next to
the cons_ops[] and vtermnos[] arrays. With the array, we can easily find
the buffer, instead of traversing hp list.
With the patch, we can remove the fix c4baad5029.
Signed-off-by: Xianting Tian <[email protected]>
Reviewed-by: Shile Zhang <[email protected]>
---
drivers/tty/hvc/hvc_console.c | 27 ++++++++++++---------------
drivers/tty/hvc/hvc_console.h | 16 ++++++++++++++--
2 files changed, 26 insertions(+), 17 deletions(-)
diff --git a/drivers/tty/hvc/hvc_console.c b/drivers/tty/hvc/hvc_console.c
index 5bb8c4e44..300e9c037 100644
--- a/drivers/tty/hvc/hvc_console.c
+++ b/drivers/tty/hvc/hvc_console.c
@@ -41,16 +41,6 @@
*/
#define HVC_CLOSE_WAIT (HZ/100) /* 1/10 of a second */
-/*
- * These sizes are most efficient for vio, because they are the
- * native transfer size. We could make them selectable in the
- * future to better deal with backends that want other buffer sizes.
- */
-#define N_OUTBUF 16
-#define N_INBUF 16
-
-#define __ALIGNED__ __attribute__((__aligned__(L1_CACHE_BYTES)))
-
static struct tty_driver *hvc_driver;
static struct task_struct *hvc_task;
@@ -142,6 +132,7 @@ static int hvc_flush(struct hvc_struct *hp)
static const struct hv_ops *cons_ops[MAX_NR_HVC_CONSOLES];
static uint32_t vtermnos[MAX_NR_HVC_CONSOLES] =
{[0 ... MAX_NR_HVC_CONSOLES - 1] = -1};
+static char *cons_outbufs[MAX_NR_HVC_CONSOLES];
/*
* Console APIs, NOT TTY. These APIs are available immediately when
@@ -151,7 +142,7 @@ 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;
@@ -163,6 +154,10 @@ static void hvc_console_print(struct console *co, const char *b,
if (vtermnos[index] == -1)
return;
+ c = cons_outbufs[index];
+ if (!c)
+ return;
+
while (count > 0 || i > 0) {
if (count > 0 && i < sizeof(c)) {
if (b[n] == '\n' && !donecr) {
@@ -879,8 +874,10 @@ static void hvc_poll_put_char(struct tty_driver *driver, int line, char ch)
struct hvc_struct *hp = tty->driver_data;
int n;
+ hp->out_ch = ch;
+
do {
- n = hp->ops->put_chars(hp->vtermno, &ch, 1);
+ n = hp->ops->put_chars(hp->vtermno, hp->out_ch, 1);
} while (n <= 0);
}
#endif
@@ -922,8 +919,7 @@ struct hvc_struct *hvc_alloc(uint32_t vtermno, int data,
return ERR_PTR(err);
}
- hp = kzalloc(ALIGN(sizeof(*hp), sizeof(long)) + outbuf_size,
- GFP_KERNEL);
+ hp = kzalloc(struct_size(hp, outbuf, outbuf_size), GFP_KERNEL);
if (!hp)
return ERR_PTR(-ENOMEM);
@@ -931,7 +927,6 @@ struct hvc_struct *hvc_alloc(uint32_t vtermno, int data,
hp->data = data;
hp->ops = ops;
hp->outbuf_size = outbuf_size;
- hp->outbuf = &((char *)hp)[ALIGN(sizeof(*hp), sizeof(long))];
tty_port_init(&hp->port);
hp->port.ops = &hvc_port_ops;
@@ -964,6 +959,7 @@ struct hvc_struct *hvc_alloc(uint32_t vtermno, int data,
if (i < MAX_NR_HVC_CONSOLES) {
cons_ops[i] = ops;
vtermnos[i] = vtermno;
+ cons_outbufs[i] = hp->out_buf;
}
list_add_tail(&(hp->next), &hvc_structs);
@@ -988,6 +984,7 @@ int hvc_remove(struct hvc_struct *hp)
if (hp->index < MAX_NR_HVC_CONSOLES) {
vtermnos[hp->index] = -1;
cons_ops[hp->index] = NULL;
+ cons_outbufs[hp->index] = NULL;
}
/* Don't whack hp->irq because tty_hangup() will need to free the irq. */
diff --git a/drivers/tty/hvc/hvc_console.h b/drivers/tty/hvc/hvc_console.h
index 18d005814..b94576d55 100644
--- a/drivers/tty/hvc/hvc_console.h
+++ b/drivers/tty/hvc/hvc_console.h
@@ -32,13 +32,21 @@
*/
#define HVC_ALLOC_TTY_ADAPTERS 8
+/*
+ * These sizes are most efficient for vio, because they are the
+ * native transfer size. We could make them selectable in the
+ * future to better deal with backends that want other buffer sizes.
+ */
+#define N_OUTBUF 16
+#define N_INBUF 16
+
+#define __ALIGNED__ __attribute__((__aligned__(L1_CACHE_BYTES)))
+
struct hvc_struct {
struct tty_port port;
spinlock_t lock;
int index;
int do_wakeup;
- char *outbuf;
- int outbuf_size;
int n_outbuf;
uint32_t vtermno;
const struct hv_ops *ops;
@@ -48,6 +56,10 @@ struct hvc_struct {
struct work_struct tty_resize;
struct list_head next;
unsigned long flags;
+ char out_ch;
+ char out_buf[N_OUTBUF] __ALIGNED__;
+ int outbuf_size;
+ char outbuf[0] __ALIGNED__;
};
/* implemented by a low level driver */
--
2.17.1
Hi Xianting,
Thank you for the patch! Perhaps something to improve:
[auto build test WARNING on tty/tty-testing]
[also build test WARNING on char-misc/char-misc-testing soc/for-next v5.14-rc6 next-20210818]
[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/make-hvc-pass-dma-capable-memory-to-its-backend/20210818-162408
base: https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/tty.git tty-testing
config: sh-allmodconfig (attached as .config)
compiler: sh4-linux-gcc (GCC) 11.2.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/e1b7662dafceb07a6905b64da2f1be27498c4a46
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review Xianting-Tian/make-hvc-pass-dma-capable-memory-to-its-backend/20210818-162408
git checkout e1b7662dafceb07a6905b64da2f1be27498c4a46
# save the attached .config to linux build tree
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-11.2.0 make.cross ARCH=sh
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <[email protected]>
All warnings (new ones prefixed by >>):
drivers/tty/hvc/hvc_console.c: In function 'hvc_poll_put_char':
>> drivers/tty/hvc/hvc_console.c:880:55: warning: passing argument 2 of 'hp->ops->put_chars' makes pointer from integer without a cast [-Wint-conversion]
880 | n = hp->ops->put_chars(hp->vtermno, hp->out_ch, 1);
| ~~^~~~~~~~
| |
| char
drivers/tty/hvc/hvc_console.c:880:55: note: expected 'const char *' but argument is of type 'char'
Kconfig warnings: (for reference only)
WARNING: unmet direct dependencies detected for SND_ATMEL_SOC_PDC
Depends on SOUND && !UML && SND && SND_SOC && SND_ATMEL_SOC && HAS_DMA
Selected by
- SND_ATMEL_SOC_SSC && SOUND && !UML && SND && SND_SOC && SND_ATMEL_SOC
- SND_ATMEL_SOC_SSC_PDC && SOUND && !UML && SND && SND_SOC && SND_ATMEL_SOC && ATMEL_SSC
vim +880 drivers/tty/hvc/hvc_console.c
870
871 static void hvc_poll_put_char(struct tty_driver *driver, int line, char ch)
872 {
873 struct tty_struct *tty = driver->ttys[0];
874 struct hvc_struct *hp = tty->driver_data;
875 int n;
876
877 hp->out_ch = ch;
878
879 do {
> 880 n = hp->ops->put_chars(hp->vtermno, hp->out_ch, 1);
881 } while (n <= 0);
882 }
883 #endif
884
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/[email protected]
Hi Xianting,
Thank you for the patch! Perhaps something to improve:
[auto build test WARNING on tty/tty-testing]
[also build test WARNING on char-misc/char-misc-testing soc/for-next v5.14-rc6 next-20210818]
[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/make-hvc-pass-dma-capable-memory-to-its-backend/20210818-162408
base: https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/tty.git tty-testing
config: arm64-randconfig-r025-20210818 (attached as .config)
compiler: clang version 14.0.0 (https://github.com/llvm/llvm-project d2b574a4dea5b718e4386bf2e26af0126e5978ce)
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
# install arm64 cross compiling tool for clang build
# apt-get install binutils-aarch64-linux-gnu
# https://github.com/0day-ci/linux/commit/e1b7662dafceb07a6905b64da2f1be27498c4a46
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review Xianting-Tian/make-hvc-pass-dma-capable-memory-to-its-backend/20210818-162408
git checkout e1b7662dafceb07a6905b64da2f1be27498c4a46
# save the attached .config to linux build tree
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross ARCH=arm64
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <[email protected]>
All warnings (new ones prefixed by >>):
>> drivers/tty/hvc/hvc_console.c:880:39: warning: incompatible integer to pointer conversion passing 'char' to parameter of type 'const char *'; take the address with & [-Wint-conversion]
n = hp->ops->put_chars(hp->vtermno, hp->out_ch, 1);
^~~~~~~~~~
&
1 warning generated.
vim +880 drivers/tty/hvc/hvc_console.c
870
871 static void hvc_poll_put_char(struct tty_driver *driver, int line, char ch)
872 {
873 struct tty_struct *tty = driver->ttys[0];
874 struct hvc_struct *hp = tty->driver_data;
875 int n;
876
877 hp->out_ch = ch;
878
879 do {
> 880 n = hp->ops->put_chars(hp->vtermno, hp->out_ch, 1);
881 } while (n <= 0);
882 }
883 #endif
884
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/[email protected]
Xianting Tian <[email protected]> writes:
> As well known, hvc backend driver(eg, virtio-console) can register its
> operations to hvc framework. The operations can 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):
We could also run into issues on powerpc where Andrew is working on
adding vmap-stack but the opal hvc driver assumes that it is passed a
buffer which is not in vmalloc space but in the linear mapping. So it
would be good to fix this (or more clearly document what drivers can
expect).
> 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 future.
>
> In this patch, we make 'char out_buf[N_OUTBUF]' and 'chat out_ch' part
> of 'struct hvc_struct', so both two buf are no longer the stack memory.
> we can use it in above two cases separately.
>
> Introduce another array(cons_outbufs[]) for buffer pointers next to
> the cons_ops[] and vtermnos[] arrays. With the array, we can easily find
> the buffer, instead of traversing hp list.
>
> With the patch, we can remove the fix c4baad5029.
>
> Signed-off-by: Xianting Tian <[email protected]>
> Reviewed-by: Shile Zhang <[email protected]>
> struct hvc_struct {
> struct tty_port port;
> spinlock_t lock;
> int index;
> int do_wakeup;
> - char *outbuf;
> - int outbuf_size;
> int n_outbuf;
> uint32_t vtermno;
> const struct hv_ops *ops;
> @@ -48,6 +56,10 @@ struct hvc_struct {
> struct work_struct tty_resize;
> struct list_head next;
> unsigned long flags;
> + char out_ch;
> + char out_buf[N_OUTBUF] __ALIGNED__;
> + int outbuf_size;
> + char outbuf[0] __ALIGNED__;
I'm trying to understand this patch but I am finding it very difficult
to understand what the difference between `out_buf` and `outbuf`
(without the underscore) is supposed to be. `out_buf` is statically
sized and the size of `outbuf` is supposed to depend on the arguments to
hvc_alloc(), but I can't quite figure out what the roles of each one are
and their names are confusingly similiar!
I looked briefly at the older revisions of the series but it didn't make
things much clearer.
Could you give them clearer names?
Also, looking at Documentation/process/deprecated.rst, it looks like
maybe we want to use a 'flexible array member' instead:
.. note:: If you are using struct_size() on a structure containing a zero-length
or a one-element array as a trailing array member, please refactor such
array usage and switch to a `flexible array member
<#zero-length-and-one-element-arrays>`_ instead.
I think we want:
> + char outbuf[] __ALIGNED__;
Kind regards,
Daniel
?? 2021/8/20 ????2:49, Daniel Axtens д??:
> Xianting Tian <[email protected]> writes:
>
>> As well known, hvc backend driver(eg, virtio-console) can register its
>> operations to hvc framework. The operations can 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):
> We could also run into issues on powerpc where Andrew is working on
> adding vmap-stack but the opal hvc driver assumes that it is passed a
> buffer which is not in vmalloc space but in the linear mapping. So it
> would be good to fix this (or more clearly document what drivers can
> expect).
>
>> 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 future.
>>
>> In this patch, we make 'char out_buf[N_OUTBUF]' and 'chat out_ch' part
>> of 'struct hvc_struct', so both two buf are no longer the stack memory.
>> we can use it in above two cases separately.
>>
>> Introduce another array(cons_outbufs[]) for buffer pointers next to
>> the cons_ops[] and vtermnos[] arrays. With the array, we can easily find
>> the buffer, instead of traversing hp list.
>>
>> With the patch, we can remove the fix c4baad5029.
>>
>> Signed-off-by: Xianting Tian <[email protected]>
>> Reviewed-by: Shile Zhang <[email protected]>
>> struct hvc_struct {
>> struct tty_port port;
>> spinlock_t lock;
>> int index;
>> int do_wakeup;
>> - char *outbuf;
>> - int outbuf_size;
>> int n_outbuf;
>> uint32_t vtermno;
>> const struct hv_ops *ops;
>> @@ -48,6 +56,10 @@ struct hvc_struct {
>> struct work_struct tty_resize;
>> struct list_head next;
>> unsigned long flags;
>> + char out_ch;
>> + char out_buf[N_OUTBUF] __ALIGNED__;
>> + int outbuf_size;
>> + char outbuf[0] __ALIGNED__;
> I'm trying to understand this patch but I am finding it very difficult
> to understand what the difference between `out_buf` and `outbuf`
> (without the underscore) is supposed to be. `out_buf` is statically
> sized and the size of `outbuf` is supposed to depend on the arguments to
> hvc_alloc(), but I can't quite figure out what the roles of each one are
> and their names are confusingly similiar!
>
> I looked briefly at the older revisions of the series but it didn't make
> things much clearer.
>
> Could you give them clearer names?
thanks for the comments,
It is indeed not easy to understand by the name. I will change it to a
proper name if we have next version patch.
Jiri Slaby is worring about the performance, because we need add two
locks to protect 'out_ch' and 'out_buf' separately, the origin on-stack
buffer is lockless.
I don't know whether this solution can be accepted, just waiting for
Jiri's further commtents.
>
> Also, looking at Documentation/process/deprecated.rst, it looks like
> maybe we want to use a 'flexible array member' instead:
>
> .. note:: If you are using struct_size() on a structure containing a zero-length
> or a one-element array as a trailing array member, please refactor such
> array usage and switch to a `flexible array member
> <#zero-length-and-one-element-arrays>`_ instead.
>
> I think we want:
thanks, we should use [], not [0].
>
>> + char outbuf[] __ALIGNED__;
> Kind regards,
> Daniel
Daniel Axtens <[email protected]> writes:
> Xianting Tian <[email protected]> writes:
>
>> As well known, hvc backend driver(eg, virtio-console) can register its
>> operations to hvc framework. The operations can 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):
>
> We could also run into issues on powerpc where Andrew is working on
> adding vmap-stack but the opal hvc driver assumes that it is passed a
> buffer which is not in vmalloc space but in the linear mapping.
The right fix for that is our code that calls opal has to be careful
that it's not passing vmalloc addresses.
We have many cases where we pass stack variables to opal, they'll all
have to be fixed to pass the underlying phyiscal/linear map address. The
opal hvc code will just be one more case of that.
cheers
hi
Will you consider to continue the disscussion of this patch? thanks
在 2021/8/20 下午4:43, Xianting TIan 写道:
>
> 在 2021/8/20 下午2:49, Daniel Axtens 写道:
>> Xianting Tian <[email protected]> writes:
>>
>>> As well known, hvc backend driver(eg, virtio-console) can register its
>>> operations to hvc framework. The operations can 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):
>> We could also run into issues on powerpc where Andrew is working on
>> adding vmap-stack but the opal hvc driver assumes that it is passed a
>> buffer which is not in vmalloc space but in the linear mapping. So it
>> would be good to fix this (or more clearly document what drivers can
>> expect).
>>
>>> 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 future.
>>>
>>> In this patch, we make 'char out_buf[N_OUTBUF]' and 'chat out_ch' part
>>> of 'struct hvc_struct', so both two buf are no longer the stack memory.
>>> we can use it in above two cases separately.
>>>
>>> Introduce another array(cons_outbufs[]) for buffer pointers next to
>>> the cons_ops[] and vtermnos[] arrays. With the array, we can easily
>>> find
>>> the buffer, instead of traversing hp list.
>>>
>>> With the patch, we can remove the fix c4baad5029.
>>>
>>> Signed-off-by: Xianting Tian <[email protected]>
>>> Reviewed-by: Shile Zhang <[email protected]>
>>> struct hvc_struct {
>>> struct tty_port port;
>>> spinlock_t lock;
>>> int index;
>>> int do_wakeup;
>>> - char *outbuf;
>>> - int outbuf_size;
>>> int n_outbuf;
>>> uint32_t vtermno;
>>> const struct hv_ops *ops;
>>> @@ -48,6 +56,10 @@ struct hvc_struct {
>>> struct work_struct tty_resize;
>>> struct list_head next;
>>> unsigned long flags;
>>> + char out_ch;
>>> + char out_buf[N_OUTBUF] __ALIGNED__;
>>> + int outbuf_size;
>>> + char outbuf[0] __ALIGNED__;
>> I'm trying to understand this patch but I am finding it very difficult
>> to understand what the difference between `out_buf` and `outbuf`
>> (without the underscore) is supposed to be. `out_buf` is statically
>> sized and the size of `outbuf` is supposed to depend on the arguments to
>> hvc_alloc(), but I can't quite figure out what the roles of each one are
>> and their names are confusingly similiar!
>>
>> I looked briefly at the older revisions of the series but it didn't make
>> things much clearer.
>>
>> Could you give them clearer names?
>
> thanks for the comments,
>
> It is indeed not easy to understand by the name. I will change it to a
> proper name if we have next version patch.
>
> Jiri Slaby is worring about the performance, because we need add two
> locks to protect 'out_ch' and 'out_buf' separately, the origin
> on-stack buffer is lockless.
>
> I don't know whether this solution can be accepted, just waiting for
> Jiri's further commtents.
>
>>
>> Also, looking at Documentation/process/deprecated.rst, it looks like
>> maybe we want to use a 'flexible array member' instead:
>>
>> .. note:: If you are using struct_size() on a structure containing a
>> zero-length
>> or a one-element array as a trailing array member, please
>> refactor such
>> array usage and switch to a `flexible array member
>> <#zero-length-and-one-element-arrays>`_ instead.
>>
>> I think we want:
> thanks, we should use [], not [0].
>>
>>> + char outbuf[] __ALIGNED__;
>> Kind regards,
>> Daniel
On Sat, Sep 18, 2021 at 08:32:01PM +0800, Xianting Tian wrote:
> hi
>
> Will you consider to continue the disscussion of this patch? thanks
I do not see a newer version of this series.
thanks,
greg k-h
thanks Greg, I will submit v9 patch for reviewing.
Before, I was waiting for a new reply:(
?? 2021/9/18 ????8:40, Greg KH д??:
> On Sat, Sep 18, 2021 at 08:32:01PM +0800, Xianting Tian wrote:
>> hi
>>
>> Will you consider to continue the disscussion of this patch? thanks
> I do not see a newer version of this series.
>
> thanks,
>
> greg k-h