| Message ID | 4ed40da8c93766ac05ee8d3507ae1d9fdc0a68a7.1388040447.git.baruch@tkos.co.il (mailing list archive) |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
On Thu, Dec 26, 2013 at 09:00:14AM +0200, Baruch Siach wrote: > @@ -669,6 +671,11 @@ static int dw_spi_setup(struct spi_device *spi) > > spi_set_ctldata(spi, chip); > return 0; > + > +err_kfree: > + kfree(chip); > + > + return ret; > } A better fix would be to convert to devm_kzalloc() so there is no possibility of paths that don't free (and move the ctldata set earlier so we don't forget about it on creation). Indeed this patch will introduce a bug - on the second call to this function if we hit an error then the struct will be freed but ctldata won't be cleared. This will mean that if there is a further call then ctldata will still point at the old struct and the function will try to use it rather than allocating a new one.
Hi Mark, On Mon, Dec 30, 2013 at 01:08:14PM +0000, Mark Brown wrote: > On Thu, Dec 26, 2013 at 09:00:14AM +0200, Baruch Siach wrote: > > @@ -669,6 +671,11 @@ static int dw_spi_setup(struct spi_device *spi) > > > > spi_set_ctldata(spi, chip); > > return 0; > > + > > +err_kfree: > > + kfree(chip); > > + > > + return ret; > > } > > A better fix would be to convert to devm_kzalloc() so there is no > possibility of paths that don't free (and move the ctldata set earlier > so we don't forget about it on creation). Good idea. Will do. > Indeed this patch will introduce a bug - on the second call to this > function if we hit an error then the struct will be freed but ctldata > won't be cleared. This will mean that if there is a further call then > ctldata will still point at the old struct and the function will try to > use it rather than allocating a new one. Thanks for reviewing. baruch
diff --git a/drivers/spi/spi-dw.c b/drivers/spi/spi-dw.c index e37dfc1..c3354e8 100644 --- a/drivers/spi/spi-dw.c +++ b/drivers/spi/spi-dw.c @@ -616,6 +616,7 @@ static int dw_spi_setup(struct spi_device *spi) { struct dw_spi_chip *chip_info = NULL; struct chip_data *chip; + int ret; /* Only alloc on first setup */ chip = spi_get_ctldata(spi); @@ -656,7 +657,8 @@ static int dw_spi_setup(struct spi_device *spi) if (!spi->max_speed_hz) { dev_err(&spi->dev, "No max speed HZ parameter\n"); - return -EINVAL; + ret = -EINVAL; + goto err_kfree; } chip->speed_hz = spi->max_speed_hz; @@ -669,6 +671,11 @@ static int dw_spi_setup(struct spi_device *spi) spi_set_ctldata(spi, chip); return 0; + +err_kfree: + kfree(chip); + + return ret; } static void dw_spi_cleanup(struct spi_device *spi)
Cc: Feng Tang <feng.tang@intel.com> Signed-off-by: Baruch Siach <baruch@tkos.co.il> --- drivers/spi/spi-dw.c | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-)