| Message ID | 20201026133609.24262-3-brgl@bgdev.pl (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | iio: adc: xilinx: use even more devres | expand |
On Mon, Oct 26, 2020 at 4:03 PM Bartosz Golaszewski <brgl@bgdev.pl> wrote: > > From: Bartosz Golaszewski <bgolaszewski@baylibre.com> > > We now have devm_krealloc() in the kernel Use it indstead of calling > kfree() and kcalloc() separately. Which is completely lawful when size > previous_size (I mean, the additional patch you sent previously seems not related to this). > - kfree(xadc->data); > - xadc->data = kcalloc(n, sizeof(*xadc->data), GFP_KERNEL); > + xadc->data = devm_krealloc(indio_dev->dev.parent, xadc->data, > + n * sizeof(*xadc->data), I think you need to use something from overflow.h instead of explicit multiplication here. > + GFP_KERNEL | __GFP_ZERO);
On Tue, Oct 27, 2020 at 10:33 AM Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > > On Mon, Oct 26, 2020 at 4:03 PM Bartosz Golaszewski <brgl@bgdev.pl> wrote: > > > > From: Bartosz Golaszewski <bgolaszewski@baylibre.com> > > > > We now have devm_krealloc() in the kernel Use it indstead of calling > > kfree() and kcalloc() separately. > > Which is completely lawful when size > previous_size (I mean, the > additional patch you sent previously seems not related to this). > Sure but devm_krealloc() is cleaner and adds the benefit of resource management. > > - kfree(xadc->data); > > - xadc->data = kcalloc(n, sizeof(*xadc->data), GFP_KERNEL); > > + xadc->data = devm_krealloc(indio_dev->dev.parent, xadc->data, > > + n * sizeof(*xadc->data), > > I think you need to use something from overflow.h instead of explicit > multiplication here. > Or maybe add devm_krealloc_array() which would perform the checks behind the scenes? > > + GFP_KERNEL | __GFP_ZERO); > > -- > With Best Regards, > Andy Shevchenko Bartosz
On Tue, Oct 27, 2020 at 12:04 PM Bartosz Golaszewski <brgl@bgdev.pl> wrote: > On Tue, Oct 27, 2020 at 10:33 AM Andy Shevchenko > <andy.shevchenko@gmail.com> wrote: > > > > On Mon, Oct 26, 2020 at 4:03 PM Bartosz Golaszewski <brgl@bgdev.pl> wrote: > > > > > > From: Bartosz Golaszewski <bgolaszewski@baylibre.com> > > > > > > We now have devm_krealloc() in the kernel Use it indstead of calling > > > kfree() and kcalloc() separately. > > > > Which is completely lawful when size > previous_size (I mean, the > > additional patch you sent previously seems not related to this). > > > > Sure but devm_krealloc() is cleaner and adds the benefit of resource management. I meant devm_krealloc(). It should work in this case without your additional "fix" patch. > > > - kfree(xadc->data); > > > - xadc->data = kcalloc(n, sizeof(*xadc->data), GFP_KERNEL); > > > + xadc->data = devm_krealloc(indio_dev->dev.parent, xadc->data, > > > + n * sizeof(*xadc->data), > > > > I think you need to use something from overflow.h instead of explicit > > multiplication here. > > > > Or maybe add devm_krealloc_array() which would perform the checks > behind the scenes? Maybe. But what to do in the cases when you have struct with flexible arrays, like struct foo { ... type bar[]; }; ? And you do kzalloc(sizeof(foo)) followed by krealloc(). The above name (krealloc_array) may be a bit ambiguous. > > > + GFP_KERNEL | __GFP_ZERO);
On Tue, Oct 27, 2020 at 11:29 AM Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > > On Tue, Oct 27, 2020 at 12:04 PM Bartosz Golaszewski <brgl@bgdev.pl> wrote: > > On Tue, Oct 27, 2020 at 10:33 AM Andy Shevchenko > > <andy.shevchenko@gmail.com> wrote: > > > > > > On Mon, Oct 26, 2020 at 4:03 PM Bartosz Golaszewski <brgl@bgdev.pl> wrote: > > > > > > > > From: Bartosz Golaszewski <bgolaszewski@baylibre.com> > > > > > > > > We now have devm_krealloc() in the kernel Use it indstead of calling > > > > kfree() and kcalloc() separately. > > > > > > Which is completely lawful when size > previous_size (I mean, the > > > additional patch you sent previously seems not related to this). > > > > > > > Sure but devm_krealloc() is cleaner and adds the benefit of resource management. > > I meant devm_krealloc(). It should work in this case without your > additional "fix" patch. > I know, this is why I sent the fix separately. The fix is still correct on its own. > > > > - kfree(xadc->data); > > > > - xadc->data = kcalloc(n, sizeof(*xadc->data), GFP_KERNEL); > > > > + xadc->data = devm_krealloc(indio_dev->dev.parent, xadc->data, > > > > + n * sizeof(*xadc->data), > > > > > > I think you need to use something from overflow.h instead of explicit > > > multiplication here. > > > > > > > Or maybe add devm_krealloc_array() which would perform the checks > > behind the scenes? > > Maybe. But what to do in the cases when you have struct with flexible > arrays, like > struct foo { > ... > type bar[]; > }; > > ? Just use regular devm_krealloc() with struct_size()? > > And you do kzalloc(sizeof(foo)) followed by krealloc(). The above name > (krealloc_array) may be a bit ambiguous. But devm_krealloc_array() would only be useful for memory allocated by kmalloc_array() or kcalloc(). I don't see what's your point. Bartosz
On Tue, Oct 27, 2020 at 12:40 PM Bartosz Golaszewski <bgolaszewski@baylibre.com> wrote: > On Tue, Oct 27, 2020 at 11:29 AM Andy Shevchenko > <andy.shevchenko@gmail.com> wrote: > > On Tue, Oct 27, 2020 at 12:04 PM Bartosz Golaszewski <brgl@bgdev.pl> wrote: > > > On Tue, Oct 27, 2020 at 10:33 AM Andy Shevchenko > > > <andy.shevchenko@gmail.com> wrote: ... > > I meant devm_krealloc(). It should work in this case without your > > additional "fix" patch. > I know, this is why I sent the fix separately. The fix is still > correct on its own. My point is it's not needed. At all. It will actually make a regression. But this is for discussion in that thread. ... > > > Or maybe add devm_krealloc_array() which would perform the checks > > > behind the scenes? > > > > Maybe. But what to do in the cases when you have struct with flexible > > arrays, like > > struct foo { > > ... > > type bar[]; > > }; > > > > ? > > Just use regular devm_krealloc() with struct_size()? > > > > > And you do kzalloc(sizeof(foo)) followed by krealloc(). The above name > > (krealloc_array) may be a bit ambiguous. > > But devm_krealloc_array() would only be useful for memory allocated by > kmalloc_array() or kcalloc(). I don't see what's your point. Naming ambiguity. Here I'm not against it. If you think it's a good idea, go for it!
diff --git a/drivers/iio/adc/xilinx-xadc-core.c b/drivers/iio/adc/xilinx-xadc-core.c index 8494eb424b33..b516280ccbd4 100644 --- a/drivers/iio/adc/xilinx-xadc-core.c +++ b/drivers/iio/adc/xilinx-xadc-core.c @@ -589,8 +589,9 @@ static int xadc_update_scan_mode(struct iio_dev *indio_dev, n = bitmap_weight(mask, indio_dev->masklength); - kfree(xadc->data); - xadc->data = kcalloc(n, sizeof(*xadc->data), GFP_KERNEL); + xadc->data = devm_krealloc(indio_dev->dev.parent, xadc->data, + n * sizeof(*xadc->data), + GFP_KERNEL | __GFP_ZERO); if (!xadc->data) return -ENOMEM; @@ -1372,7 +1373,6 @@ static int xadc_remove(struct platform_device *pdev) free_irq(xadc->irq, indio_dev); cancel_delayed_work_sync(&xadc->zynq_unmask_work); clk_disable_unprepare(xadc->clk); - kfree(xadc->data); return 0; }