| Message ID | 1600601186-7420-2-git-send-email-Julia.Lawall@inria.fr |
|---|---|
| State | New |
| Headers | show |
| Series | drop double zeroing | expand |
On Sun, Sep 20, 2020 at 01:26:13PM +0200, Julia Lawall wrote: > sg_init_table zeroes its first argument, so the allocation of that argument > doesn't have to. > > the semantic patch that makes this change is as follows: > (http://coccinelle.lip6.fr/) > > // <smpl> > @@ > expression x,n,flags; > @@ > > x = > - kcalloc > + kmalloc_array > (n,sizeof(struct scatterlist),flags) > ... > sg_init_table(x,n) > // </smpl> > > Signed-off-by: Julia Lawall <Julia.Lawall@inria.fr> It inits the first entry in the array, but what about all of the other ones? Is that "safe" to have uninitialized data in them like your change causes to happen? thanks, greg k-h
On Sun, 20 Sep 2020, Greg Kroah-Hartman wrote: > On Sun, Sep 20, 2020 at 01:26:13PM +0200, Julia Lawall wrote: > > sg_init_table zeroes its first argument, so the allocation of that argument > > doesn't have to. > > > > the semantic patch that makes this change is as follows: > > (http://coccinelle.lip6.fr/) > > > > // <smpl> > > @@ > > expression x,n,flags; > > @@ > > > > x = > > - kcalloc > > + kmalloc_array > > (n,sizeof(struct scatterlist),flags) > > ... > > sg_init_table(x,n) > > // </smpl> > > > > Signed-off-by: Julia Lawall <Julia.Lawall@inria.fr> > > It inits the first entry in the array, but what about all of the other > ones? Is that "safe" to have uninitialized data in them like your > change causes to happen? Sorry, I don't follow. The complete code is: priv->sg_tx_p = kcalloc(num, sizeof(struct scatterlist), GFP_ATOMIC); if (!priv->sg_tx_p) { dev_err(priv->port.dev, "%s:kzalloc Failed\n", __func__); return 0; } sg_init_table(priv->sg_tx_p, num); /* Initialize SG table */ and the definition of sg_init_table is: void sg_init_table(struct scatterlist *sgl, unsigned int nents) { memset(sgl, 0, sizeof(*sgl) * nents); sg_init_marker(sgl, nents); } It looks to me like it zeroes all of the elements? The same file does contain a call: sg_init_table(&priv->sg_rx, 1); But that's not the one associated with the patch. julia
On Sun, Sep 20, 2020 at 02:47:11PM +0200, Julia Lawall wrote: > > > On Sun, 20 Sep 2020, Greg Kroah-Hartman wrote: > > > On Sun, Sep 20, 2020 at 01:26:13PM +0200, Julia Lawall wrote: > > > sg_init_table zeroes its first argument, so the allocation of that argument > > > doesn't have to. > > > > > > the semantic patch that makes this change is as follows: > > > (http://coccinelle.lip6.fr/) > > > > > > // <smpl> > > > @@ > > > expression x,n,flags; > > > @@ > > > > > > x = > > > - kcalloc > > > + kmalloc_array > > > (n,sizeof(struct scatterlist),flags) > > > ... > > > sg_init_table(x,n) > > > // </smpl> > > > > > > Signed-off-by: Julia Lawall <Julia.Lawall@inria.fr> > > > > It inits the first entry in the array, but what about all of the other > > ones? Is that "safe" to have uninitialized data in them like your > > change causes to happen? > > Sorry, I don't follow. The complete code is: > > priv->sg_tx_p = kcalloc(num, sizeof(struct scatterlist), GFP_ATOMIC); > if (!priv->sg_tx_p) { > dev_err(priv->port.dev, "%s:kzalloc Failed\n", __func__); > return 0; > } > > sg_init_table(priv->sg_tx_p, num); /* Initialize SG table */ > > and the definition of sg_init_table is: > > void sg_init_table(struct scatterlist *sgl, unsigned int nents) > { > memset(sgl, 0, sizeof(*sgl) * nents); > sg_init_marker(sgl, nents); > } Ah, missed the "* nents" thing there, sorry, my fault. greg k-h
diff -u -p a/drivers/tty/serial/pch_uart.c b/drivers/tty/serial/pch_uart.c --- a/drivers/tty/serial/pch_uart.c +++ b/drivers/tty/serial/pch_uart.c @@ -981,7 +981,7 @@ static unsigned int dma_handle_tx(struct priv->tx_dma_use = 1; - priv->sg_tx_p = kcalloc(num, sizeof(struct scatterlist), GFP_ATOMIC); + priv->sg_tx_p = kmalloc_array(num, sizeof(struct scatterlist), GFP_ATOMIC); if (!priv->sg_tx_p) { dev_err(priv->port.dev, "%s:kzalloc Failed\n", __func__); return 0;
sg_init_table zeroes its first argument, so the allocation of that argument doesn't have to. the semantic patch that makes this change is as follows: (http://coccinelle.lip6.fr/) // <smpl> @@ expression x,n,flags; @@ x = - kcalloc + kmalloc_array (n,sizeof(struct scatterlist),flags) ... sg_init_table(x,n) // </smpl> Signed-off-by: Julia Lawall <Julia.Lawall@inria.fr> --- drivers/tty/serial/pch_uart.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)