* [RFC PATCH 0/1] Sanity check buffer callback @ 2020-11-12 15:13 Nuno Sá 2020-11-12 15:13 ` [RFC PATCH 1/1] iio: buffer: " Nuno Sá 2020-11-14 16:33 ` [RFC PATCH 0/1] " Jonathan Cameron 0 siblings, 2 replies; 6+ messages in thread From: Nuno Sá @ 2020-11-12 15:13 UTC (permalink / raw) To: linux-iio; +Cc: Jonathan Cameron, Lars-Peter Clausen, Peter Meerwald-Stadler When going through the code of the buffer-cb interface and all it's users, I realized that the stm32_adfsdm driver is calling `iio_channel_get_all_cb()` with NULL for the cb. After going a bit trough the stm drivers, it looks like this is actually intentional. However, it is clear that we have a clear/direct route here for a NULL pointer dereference. I'm being lazy in this RFC and just doing a sanity check in the `iio_buffer_cb_store_to()` so that we don't have to change the stm driver... The point is just to bring this up and see if we want to do something about this. To be clear, the way I think this should go is just to return -EINVAL in `iio_channel_get_all_cb()` if a NULL ptr is passed. Whats the point of a buffer-cb if cb is NULL, right? This would naturally break the stm driver, but I guess we could just define a dummy handler there that would not be used (or could the HW consumer be an option here?)... Thoughts? Nuno Sá (1): iio: buffer: Sanity check buffer callback drivers/iio/buffer/industrialio-buffer-cb.c | 4 ++++ 1 file changed, 4 insertions(+) -- 2.29.2 ^ permalink raw reply [flat|nested] 6+ messages in thread
* [RFC PATCH 1/1] iio: buffer: Sanity check buffer callback 2020-11-12 15:13 [RFC PATCH 0/1] Sanity check buffer callback Nuno Sá @ 2020-11-12 15:13 ` Nuno Sá 2020-11-12 16:15 ` Lars-Peter Clausen 2020-11-14 16:33 ` [RFC PATCH 0/1] " Jonathan Cameron 1 sibling, 1 reply; 6+ messages in thread From: Nuno Sá @ 2020-11-12 15:13 UTC (permalink / raw) To: linux-iio; +Cc: Jonathan Cameron, Lars-Peter Clausen, Peter Meerwald-Stadler Make sure that a callback is actually present before trying to call it... Signed-off-by: Nuno Sá <nuno.sa@analog.com> --- drivers/iio/buffer/industrialio-buffer-cb.c | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/drivers/iio/buffer/industrialio-buffer-cb.c b/drivers/iio/buffer/industrialio-buffer-cb.c index 47c96f7f4976..b4385f59399d 100644 --- a/drivers/iio/buffer/industrialio-buffer-cb.c +++ b/drivers/iio/buffer/industrialio-buffer-cb.c @@ -27,6 +27,10 @@ static struct iio_cb_buffer *buffer_to_cb_buffer(struct iio_buffer *buffer) static int iio_buffer_cb_store_to(struct iio_buffer *buffer, const void *data) { struct iio_cb_buffer *cb_buff = buffer_to_cb_buffer(buffer); + + if (!cb_buff->cb) + return 0; + return cb_buff->cb(data, cb_buff->private); } -- 2.29.2 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [RFC PATCH 1/1] iio: buffer: Sanity check buffer callback 2020-11-12 15:13 ` [RFC PATCH 1/1] iio: buffer: " Nuno Sá @ 2020-11-12 16:15 ` Lars-Peter Clausen 2020-11-12 16:20 ` Lars-Peter Clausen 0 siblings, 1 reply; 6+ messages in thread From: Lars-Peter Clausen @ 2020-11-12 16:15 UTC (permalink / raw) To: Nuno Sá, linux-iio; +Cc: Jonathan Cameron, Peter Meerwald-Stadler On 11/12/20 4:13 PM, Nuno Sá wrote: > Make sure that a callback is actually present before trying to call > it... > > Signed-off-by: Nuno Sá <nuno.sa@analog.com> > --- > drivers/iio/buffer/industrialio-buffer-cb.c | 4 ++++ > 1 file changed, 4 insertions(+) > > diff --git a/drivers/iio/buffer/industrialio-buffer-cb.c b/drivers/iio/buffer/industrialio-buffer-cb.c > index 47c96f7f4976..b4385f59399d 100644 > --- a/drivers/iio/buffer/industrialio-buffer-cb.c > +++ b/drivers/iio/buffer/industrialio-buffer-cb.c > @@ -27,6 +27,10 @@ static struct iio_cb_buffer *buffer_to_cb_buffer(struct iio_buffer *buffer) > static int iio_buffer_cb_store_to(struct iio_buffer *buffer, const void *data) > { > struct iio_cb_buffer *cb_buff = buffer_to_cb_buffer(buffer); > + > + if (!cb_buff->cb) > + return 0; > + I think it makes more sense to check this once when the buffer is registered instead of every time we want to call the function. ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [RFC PATCH 1/1] iio: buffer: Sanity check buffer callback 2020-11-12 16:15 ` Lars-Peter Clausen @ 2020-11-12 16:20 ` Lars-Peter Clausen 0 siblings, 0 replies; 6+ messages in thread From: Lars-Peter Clausen @ 2020-11-12 16:20 UTC (permalink / raw) To: Nuno Sá, linux-iio; +Cc: Jonathan Cameron, Peter Meerwald-Stadler On 11/12/20 5:15 PM, Lars-Peter Clausen wrote: > On 11/12/20 4:13 PM, Nuno Sá wrote: >> Make sure that a callback is actually present before trying to call >> it... >> >> Signed-off-by: Nuno Sá <nuno.sa@analog.com> >> --- >> drivers/iio/buffer/industrialio-buffer-cb.c | 4 ++++ >> 1 file changed, 4 insertions(+) >> >> diff --git a/drivers/iio/buffer/industrialio-buffer-cb.c >> b/drivers/iio/buffer/industrialio-buffer-cb.c >> index 47c96f7f4976..b4385f59399d 100644 >> --- a/drivers/iio/buffer/industrialio-buffer-cb.c >> +++ b/drivers/iio/buffer/industrialio-buffer-cb.c >> @@ -27,6 +27,10 @@ static struct iio_cb_buffer >> *buffer_to_cb_buffer(struct iio_buffer *buffer) >> static int iio_buffer_cb_store_to(struct iio_buffer *buffer, const >> void *data) >> { >> struct iio_cb_buffer *cb_buff = buffer_to_cb_buffer(buffer); >> + >> + if (!cb_buff->cb) >> + return 0; >> + > I think it makes more sense to check this once when the buffer is > registered instead of every time we want to call the function. Sorry, should have read the cover letter more carefully. What you write there makes sense. ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [RFC PATCH 0/1] Sanity check buffer callback 2020-11-12 15:13 [RFC PATCH 0/1] Sanity check buffer callback Nuno Sá 2020-11-12 15:13 ` [RFC PATCH 1/1] iio: buffer: " Nuno Sá @ 2020-11-14 16:33 ` Jonathan Cameron 2020-11-16 16:49 ` Olivier MOYSAN 1 sibling, 1 reply; 6+ messages in thread From: Jonathan Cameron @ 2020-11-14 16:33 UTC (permalink / raw) To: Nuno Sá Cc: linux-iio, Lars-Peter Clausen, Peter Meerwald-Stadler, Arnaud Pouliquen, Olivier Moysan On Thu, 12 Nov 2020 16:13:33 +0100 Nuno Sá <nuno.sa@analog.com> wrote: > When going through the code of the buffer-cb interface and all it's > users, I realized that the stm32_adfsdm driver is calling > `iio_channel_get_all_cb()` with NULL for the cb. After going a bit > trough the stm drivers, it looks like this is actually intentional. > However, it is clear that we have a clear/direct route here for a NULL > pointer dereference. I'm being lazy in this RFC and just doing a > sanity check in the `iio_buffer_cb_store_to()` so that we don't have to > change the stm driver... The point is just to bring this up and see if > we want to do something about this. > > To be clear, the way I think this should go is just to return -EINVAL in > `iio_channel_get_all_cb()` if a NULL ptr is passed. Whats the point of a > buffer-cb if cb is NULL, right? This would naturally break the stm > driver, but I guess we could just define a dummy handler there that > would not be used (or could the HW consumer be an option here?)... > > Thoughts? Good description thanks. I think you are right and better option is to return -EINVAL in iio_channel_get_all_cb() and add a dummy handler to the stm driver. cc Arnaud and Olivier to see if they are fine with the dummy handler. (with appropriate comment on why it is there). Jonathan > > Nuno Sá (1): > iio: buffer: Sanity check buffer callback > > drivers/iio/buffer/industrialio-buffer-cb.c | 4 ++++ > 1 file changed, 4 insertions(+) > ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [RFC PATCH 0/1] Sanity check buffer callback 2020-11-14 16:33 ` [RFC PATCH 0/1] " Jonathan Cameron @ 2020-11-16 16:49 ` Olivier MOYSAN 0 siblings, 0 replies; 6+ messages in thread From: Olivier MOYSAN @ 2020-11-16 16:49 UTC (permalink / raw) To: Jonathan Cameron, Nuno Sá Cc: linux-iio, Lars-Peter Clausen, Peter Meerwald-Stadler, Arnaud POULIQUEN [-- Attachment #1: Type: text/plain, Size: 1885 bytes --] On 11/14/20 5:33 PM, Jonathan Cameron wrote: > On Thu, 12 Nov 2020 16:13:33 +0100 > Nuno Sá <nuno.sa@analog.com> wrote: > >> When going through the code of the buffer-cb interface and all it's >> users, I realized that the stm32_adfsdm driver is calling >> `iio_channel_get_all_cb()` with NULL for the cb. After going a bit >> trough the stm drivers, it looks like this is actually intentional. >> However, it is clear that we have a clear/direct route here for a NULL >> pointer dereference. I'm being lazy in this RFC and just doing a >> sanity check in the `iio_buffer_cb_store_to()` so that we don't have to >> change the stm driver... The point is just to bring this up and see if >> we want to do something about this. >> >> To be clear, the way I think this should go is just to return -EINVAL in >> `iio_channel_get_all_cb()` if a NULL ptr is passed. Whats the point of a >> buffer-cb if cb is NULL, right? This would naturally break the stm >> driver, but I guess we could just define a dummy handler there that >> would not be used (or could the HW consumer be an option here?)... >> >> Thoughts? > > Good description thanks. I think you are right and better option is > to return -EINVAL in iio_channel_get_all_cb() and add a dummy handler to the > stm driver. > > cc Arnaud and Olivier to see if they are fine with the dummy handler. > (with appropriate comment on why it is there). > > Jonathan > > Hi Jonathan, Thanks for your notice. Arnaud and myself agree with this solution. I attach to this mail a patch which implements this dummy handler and can be added to Nuno's patchset, to avoid a regression on stm driver. Regards, Olivier >> >> Nuno Sá (1): >> iio: buffer: Sanity check buffer callback >> >> drivers/iio/buffer/industrialio-buffer-cb.c | 4 ++++ >> 1 file changed, 4 insertions(+) >> > [-- Warning: decoded text below may be mangled, UTF-8 assumed --] [-- Attachment #2: 0001-ASoC-stm32-dfsdm-add-stm32_adfsdm_dummy_cb-callback.patch --] [-- Type: text/x-patch; name="0001-ASoC-stm32-dfsdm-add-stm32_adfsdm_dummy_cb-callback.patch", Size: 1787 bytes --] From 98d3008bb2cdee75b2efb42a7d3a5af9bed47e9c Mon Sep 17 00:00:00 2001 From: Olivier Moysan <olivier.moysan@st.com> Date: Mon, 16 Nov 2020 17:23:43 +0100 Subject: [PATCH] ASoC: stm32: dfsdm: add stm32_adfsdm_dummy_cb() callback Adapt STM32 DFSDM driver to a change in iio_channel_get_all_cb() API. The callback pointer becomes a requested parameter of this API, so add a dummy callback to be given as parameter of this function. However, the stm32_dfsdm_get_buff_cb() API is still used instead, to optimize DMA transfers. Signed-off-by: Olivier Moysan <olivier.moysan@st.com> --- sound/soc/stm/stm32_adfsdm.c | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-) diff --git a/sound/soc/stm/stm32_adfsdm.c b/sound/soc/stm/stm32_adfsdm.c index c4031988f981..47fae8dd20b4 100644 --- a/sound/soc/stm/stm32_adfsdm.c +++ b/sound/soc/stm/stm32_adfsdm.c @@ -293,6 +293,16 @@ static int stm32_adfsdm_pcm_new(struct snd_soc_component *component, return 0; } +static int stm32_adfsdm_dummy_cb(const void *data, void *private) +{ + /* + * This dummmy callback is requested by iio_channel_get_all_cb() API, + * but the stm32_dfsdm_get_buff_cb() API is used instead, to optimize + * DMA transfers. + */ + return 0; +} + static struct snd_soc_component_driver stm32_adfsdm_soc_platform = { .open = stm32_adfsdm_pcm_open, .close = stm32_adfsdm_pcm_close, @@ -335,7 +345,7 @@ static int stm32_adfsdm_probe(struct platform_device *pdev) if (IS_ERR(priv->iio_ch)) return PTR_ERR(priv->iio_ch); - priv->iio_cb = iio_channel_get_all_cb(&pdev->dev, NULL, NULL); + priv->iio_cb = iio_channel_get_all_cb(&pdev->dev, &stm32_adfsdm_dummy_cb, NULL); if (IS_ERR(priv->iio_cb)) return PTR_ERR(priv->iio_cb); -- 2.17.1 ^ permalink raw reply related [flat|nested] 6+ messages in thread
end of thread, other threads:[~2020-11-16 16:50 UTC | newest] Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-11-12 15:13 [RFC PATCH 0/1] Sanity check buffer callback Nuno Sá 2020-11-12 15:13 ` [RFC PATCH 1/1] iio: buffer: " Nuno Sá 2020-11-12 16:15 ` Lars-Peter Clausen 2020-11-12 16:20 ` Lars-Peter Clausen 2020-11-14 16:33 ` [RFC PATCH 0/1] " Jonathan Cameron 2020-11-16 16:49 ` Olivier MOYSAN
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.