Re: [RFC PATCH 0/1] Sanity check buffer callback

[Olivier MOYSAN <olivier.moysan@st.com>]

[-- 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