* [PATCH 1/2] iio: at91-sama5d2: Fix incorrect cast to platform_device
@ 2021-10-19 8:29 Lars-Peter Clausen
2021-10-19 8:29 ` [PATCH 2/2] iio: at91-sama5d2: Use dev_to_iio_dev() in sysfs callbacks Lars-Peter Clausen
2021-10-28 14:34 ` [PATCH 1/2] iio: at91-sama5d2: Fix incorrect cast to platform_device Jonathan Cameron
0 siblings, 2 replies; 6+ messages in thread
From: Lars-Peter Clausen @ 2021-10-19 8:29 UTC (permalink / raw)
To: Jonathan Cameron; +Cc: Eugen Hristev, linux-iio, Lars-Peter Clausen
The at91-sama5d2 driver calls `to_platform_device()` on a struct device
that is part of a IIO device. This is incorrect since
`to_platform_device()` must only be called on a struct device that is part
of a platform device.
The code still works by accident because non of the struct platform_device
specific fields are accessed.
Refactor the code a bit so that it behaves identically, but does not use
the incorrect cast. This avoids accidentally adding undefined behavior in
the future by assuming the `struct platform_device` is actually valid.
Signed-off-by: Lars-Peter Clausen <lars@metafoo.de>
---
The code is equivalent to before, but I'm a bit confused how this works.
We call dma_request_chan() on the IIO device's struct device. Which should
not yield any results.
Eugen can you check if/why this works and see if a follow up patch using
the right struct device (the platform_device's) to request the DMA channel
makes sense?
---
drivers/iio/adc/at91-sama5d2_adc.c | 34 ++++++++++++++----------------
1 file changed, 16 insertions(+), 18 deletions(-)
diff --git a/drivers/iio/adc/at91-sama5d2_adc.c b/drivers/iio/adc/at91-sama5d2_adc.c
index 4c922ef634f8..3841e7b6c81d 100644
--- a/drivers/iio/adc/at91-sama5d2_adc.c
+++ b/drivers/iio/adc/at91-sama5d2_adc.c
@@ -1661,10 +1661,9 @@ static int at91_adc_write_raw(struct iio_dev *indio_dev,
}
}
-static void at91_adc_dma_init(struct platform_device *pdev)
+static void at91_adc_dma_init(struct at91_adc_state *st)
{
- struct iio_dev *indio_dev = platform_get_drvdata(pdev);
- struct at91_adc_state *st = iio_priv(indio_dev);
+ struct device *dev = &st->indio_dev->dev;
struct dma_slave_config config = {0};
/* we have 2 bytes for each channel */
unsigned int sample_size = st->soc_info.platform->nr_channels * 2;
@@ -1679,9 +1678,9 @@ static void at91_adc_dma_init(struct platform_device *pdev)
if (st->dma_st.dma_chan)
return;
- st->dma_st.dma_chan = dma_request_chan(&pdev->dev, "rx");
+ st->dma_st.dma_chan = dma_request_chan(dev, "rx");
if (IS_ERR(st->dma_st.dma_chan)) {
- dev_info(&pdev->dev, "can't get DMA channel\n");
+ dev_info(dev, "can't get DMA channel\n");
st->dma_st.dma_chan = NULL;
goto dma_exit;
}
@@ -1691,7 +1690,7 @@ static void at91_adc_dma_init(struct platform_device *pdev)
&st->dma_st.rx_dma_buf,
GFP_KERNEL);
if (!st->dma_st.rx_buf) {
- dev_info(&pdev->dev, "can't allocate coherent DMA area\n");
+ dev_info(dev, "can't allocate coherent DMA area\n");
goto dma_chan_disable;
}
@@ -1704,11 +1703,11 @@ static void at91_adc_dma_init(struct platform_device *pdev)
config.dst_maxburst = 1;
if (dmaengine_slave_config(st->dma_st.dma_chan, &config)) {
- dev_info(&pdev->dev, "can't configure DMA slave\n");
+ dev_info(dev, "can't configure DMA slave\n");
goto dma_free_area;
}
- dev_info(&pdev->dev, "using %s for rx DMA transfers\n",
+ dev_info(dev, "using %s for rx DMA transfers\n",
dma_chan_name(st->dma_st.dma_chan));
return;
@@ -1720,13 +1719,12 @@ static void at91_adc_dma_init(struct platform_device *pdev)
dma_release_channel(st->dma_st.dma_chan);
st->dma_st.dma_chan = NULL;
dma_exit:
- dev_info(&pdev->dev, "continuing without DMA support\n");
+ dev_info(dev, "continuing without DMA support\n");
}
-static void at91_adc_dma_disable(struct platform_device *pdev)
+static void at91_adc_dma_disable(struct at91_adc_state *st)
{
- struct iio_dev *indio_dev = platform_get_drvdata(pdev);
- struct at91_adc_state *st = iio_priv(indio_dev);
+ struct device *dev = &st->indio_dev->dev;
/* we have 2 bytes for each channel */
unsigned int sample_size = st->soc_info.platform->nr_channels * 2;
unsigned int pages = DIV_ROUND_UP(AT91_HWFIFO_MAX_SIZE *
@@ -1744,7 +1742,7 @@ static void at91_adc_dma_disable(struct platform_device *pdev)
dma_release_channel(st->dma_st.dma_chan);
st->dma_st.dma_chan = NULL;
- dev_info(&pdev->dev, "continuing without DMA support\n");
+ dev_info(dev, "continuing without DMA support\n");
}
static int at91_adc_set_watermark(struct iio_dev *indio_dev, unsigned int val)
@@ -1770,9 +1768,9 @@ static int at91_adc_set_watermark(struct iio_dev *indio_dev, unsigned int val)
*/
if (val == 1)
- at91_adc_dma_disable(to_platform_device(&indio_dev->dev));
+ at91_adc_dma_disable(st);
else if (val > 1)
- at91_adc_dma_init(to_platform_device(&indio_dev->dev));
+ at91_adc_dma_init(st);
/*
* We can start the DMA only after setting the watermark and
@@ -1780,7 +1778,7 @@ static int at91_adc_set_watermark(struct iio_dev *indio_dev, unsigned int val)
*/
ret = at91_adc_buffer_prepare(indio_dev);
if (ret)
- at91_adc_dma_disable(to_platform_device(&indio_dev->dev));
+ at91_adc_dma_disable(st);
return ret;
}
@@ -2077,7 +2075,7 @@ static int at91_adc_probe(struct platform_device *pdev)
return 0;
dma_disable:
- at91_adc_dma_disable(pdev);
+ at91_adc_dma_disable(st);
per_clk_disable_unprepare:
clk_disable_unprepare(st->per_clk);
vref_disable:
@@ -2094,7 +2092,7 @@ static int at91_adc_remove(struct platform_device *pdev)
iio_device_unregister(indio_dev);
- at91_adc_dma_disable(pdev);
+ at91_adc_dma_disable(st);
clk_disable_unprepare(st->per_clk);
--
2.20.1
^ permalink raw reply related [flat|nested] 6+ messages in thread
* [PATCH 2/2] iio: at91-sama5d2: Use dev_to_iio_dev() in sysfs callbacks
2021-10-19 8:29 [PATCH 1/2] iio: at91-sama5d2: Fix incorrect cast to platform_device Lars-Peter Clausen
@ 2021-10-19 8:29 ` Lars-Peter Clausen
2021-10-28 14:34 ` [PATCH 1/2] iio: at91-sama5d2: Fix incorrect cast to platform_device Jonathan Cameron
1 sibling, 0 replies; 6+ messages in thread
From: Lars-Peter Clausen @ 2021-10-19 8:29 UTC (permalink / raw)
To: Jonathan Cameron; +Cc: Eugen Hristev, linux-iio, Lars-Peter Clausen
Using `dev_get_drvdata()` in IIO sysfs callbacks to get a pointer to the
IIO device is a relic from the very early days of IIO. The IIO core as well
as most other drivers have switched over to using `dev_to_iio_dev()`
instead.
This driver is one of the last few drivers remaining that uses the outdated
idiom, update it. This will allow to eventually update the IIO core to no
longer set the drvdata for the IIO device and free it up for driver usage.
Signed-off-by: Lars-Peter Clausen <lars@metafoo.de>
---
drivers/iio/adc/at91-sama5d2_adc.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/iio/adc/at91-sama5d2_adc.c b/drivers/iio/adc/at91-sama5d2_adc.c
index 3841e7b6c81d..a2c406276329 100644
--- a/drivers/iio/adc/at91-sama5d2_adc.c
+++ b/drivers/iio/adc/at91-sama5d2_adc.c
@@ -1825,7 +1825,7 @@ static void at91_adc_hw_init(struct iio_dev *indio_dev)
static ssize_t at91_adc_get_fifo_state(struct device *dev,
struct device_attribute *attr, char *buf)
{
- struct iio_dev *indio_dev = dev_get_drvdata(dev);
+ struct iio_dev *indio_dev = dev_to_iio_dev(dev);
struct at91_adc_state *st = iio_priv(indio_dev);
return scnprintf(buf, PAGE_SIZE, "%d\n", !!st->dma_st.dma_chan);
@@ -1834,7 +1834,7 @@ static ssize_t at91_adc_get_fifo_state(struct device *dev,
static ssize_t at91_adc_get_watermark(struct device *dev,
struct device_attribute *attr, char *buf)
{
- struct iio_dev *indio_dev = dev_get_drvdata(dev);
+ struct iio_dev *indio_dev = dev_to_iio_dev(dev);
struct at91_adc_state *st = iio_priv(indio_dev);
return scnprintf(buf, PAGE_SIZE, "%d\n", st->dma_st.watermark);
--
2.20.1
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH 1/2] iio: at91-sama5d2: Fix incorrect cast to platform_device
2021-10-19 8:29 [PATCH 1/2] iio: at91-sama5d2: Fix incorrect cast to platform_device Lars-Peter Clausen
2021-10-19 8:29 ` [PATCH 2/2] iio: at91-sama5d2: Use dev_to_iio_dev() in sysfs callbacks Lars-Peter Clausen
@ 2021-10-28 14:34 ` Jonathan Cameron
2021-10-28 14:39 ` Eugen.Hristev
1 sibling, 1 reply; 6+ messages in thread
From: Jonathan Cameron @ 2021-10-28 14:34 UTC (permalink / raw)
To: Lars-Peter Clausen; +Cc: Eugen Hristev, linux-iio
On Tue, 19 Oct 2021 10:29:28 +0200
Lars-Peter Clausen <lars@metafoo.de> wrote:
> The at91-sama5d2 driver calls `to_platform_device()` on a struct device
> that is part of a IIO device. This is incorrect since
> `to_platform_device()` must only be called on a struct device that is part
> of a platform device.
>
> The code still works by accident because non of the struct platform_device
> specific fields are accessed.
>
> Refactor the code a bit so that it behaves identically, but does not use
> the incorrect cast. This avoids accidentally adding undefined behavior in
> the future by assuming the `struct platform_device` is actually valid.
>
> Signed-off-by: Lars-Peter Clausen <lars@metafoo.de>
This makes me nervous for the reason you give below.
Looking for a response from Eugen or someone else with access to the device
before I apply this one.
Thanks,
Jonathan
> ---
> The code is equivalent to before, but I'm a bit confused how this works.
> We call dma_request_chan() on the IIO device's struct device. Which should
> not yield any results.
>
> Eugen can you check if/why this works and see if a follow up patch using
> the right struct device (the platform_device's) to request the DMA channel
> makes sense?
> ---
> drivers/iio/adc/at91-sama5d2_adc.c | 34 ++++++++++++++----------------
> 1 file changed, 16 insertions(+), 18 deletions(-)
>
> diff --git a/drivers/iio/adc/at91-sama5d2_adc.c b/drivers/iio/adc/at91-sama5d2_adc.c
> index 4c922ef634f8..3841e7b6c81d 100644
> --- a/drivers/iio/adc/at91-sama5d2_adc.c
> +++ b/drivers/iio/adc/at91-sama5d2_adc.c
> @@ -1661,10 +1661,9 @@ static int at91_adc_write_raw(struct iio_dev *indio_dev,
> }
> }
>
> -static void at91_adc_dma_init(struct platform_device *pdev)
> +static void at91_adc_dma_init(struct at91_adc_state *st)
> {
> - struct iio_dev *indio_dev = platform_get_drvdata(pdev);
> - struct at91_adc_state *st = iio_priv(indio_dev);
> + struct device *dev = &st->indio_dev->dev;
> struct dma_slave_config config = {0};
> /* we have 2 bytes for each channel */
> unsigned int sample_size = st->soc_info.platform->nr_channels * 2;
> @@ -1679,9 +1678,9 @@ static void at91_adc_dma_init(struct platform_device *pdev)
> if (st->dma_st.dma_chan)
> return;
>
> - st->dma_st.dma_chan = dma_request_chan(&pdev->dev, "rx");
> + st->dma_st.dma_chan = dma_request_chan(dev, "rx");
> if (IS_ERR(st->dma_st.dma_chan)) {
> - dev_info(&pdev->dev, "can't get DMA channel\n");
> + dev_info(dev, "can't get DMA channel\n");
> st->dma_st.dma_chan = NULL;
> goto dma_exit;
> }
> @@ -1691,7 +1690,7 @@ static void at91_adc_dma_init(struct platform_device *pdev)
> &st->dma_st.rx_dma_buf,
> GFP_KERNEL);
> if (!st->dma_st.rx_buf) {
> - dev_info(&pdev->dev, "can't allocate coherent DMA area\n");
> + dev_info(dev, "can't allocate coherent DMA area\n");
> goto dma_chan_disable;
> }
>
> @@ -1704,11 +1703,11 @@ static void at91_adc_dma_init(struct platform_device *pdev)
> config.dst_maxburst = 1;
>
> if (dmaengine_slave_config(st->dma_st.dma_chan, &config)) {
> - dev_info(&pdev->dev, "can't configure DMA slave\n");
> + dev_info(dev, "can't configure DMA slave\n");
> goto dma_free_area;
> }
>
> - dev_info(&pdev->dev, "using %s for rx DMA transfers\n",
> + dev_info(dev, "using %s for rx DMA transfers\n",
> dma_chan_name(st->dma_st.dma_chan));
>
> return;
> @@ -1720,13 +1719,12 @@ static void at91_adc_dma_init(struct platform_device *pdev)
> dma_release_channel(st->dma_st.dma_chan);
> st->dma_st.dma_chan = NULL;
> dma_exit:
> - dev_info(&pdev->dev, "continuing without DMA support\n");
> + dev_info(dev, "continuing without DMA support\n");
> }
>
> -static void at91_adc_dma_disable(struct platform_device *pdev)
> +static void at91_adc_dma_disable(struct at91_adc_state *st)
> {
> - struct iio_dev *indio_dev = platform_get_drvdata(pdev);
> - struct at91_adc_state *st = iio_priv(indio_dev);
> + struct device *dev = &st->indio_dev->dev;
> /* we have 2 bytes for each channel */
> unsigned int sample_size = st->soc_info.platform->nr_channels * 2;
> unsigned int pages = DIV_ROUND_UP(AT91_HWFIFO_MAX_SIZE *
> @@ -1744,7 +1742,7 @@ static void at91_adc_dma_disable(struct platform_device *pdev)
> dma_release_channel(st->dma_st.dma_chan);
> st->dma_st.dma_chan = NULL;
>
> - dev_info(&pdev->dev, "continuing without DMA support\n");
> + dev_info(dev, "continuing without DMA support\n");
> }
>
> static int at91_adc_set_watermark(struct iio_dev *indio_dev, unsigned int val)
> @@ -1770,9 +1768,9 @@ static int at91_adc_set_watermark(struct iio_dev *indio_dev, unsigned int val)
> */
>
> if (val == 1)
> - at91_adc_dma_disable(to_platform_device(&indio_dev->dev));
> + at91_adc_dma_disable(st);
> else if (val > 1)
> - at91_adc_dma_init(to_platform_device(&indio_dev->dev));
> + at91_adc_dma_init(st);
>
> /*
> * We can start the DMA only after setting the watermark and
> @@ -1780,7 +1778,7 @@ static int at91_adc_set_watermark(struct iio_dev *indio_dev, unsigned int val)
> */
> ret = at91_adc_buffer_prepare(indio_dev);
> if (ret)
> - at91_adc_dma_disable(to_platform_device(&indio_dev->dev));
> + at91_adc_dma_disable(st);
>
> return ret;
> }
> @@ -2077,7 +2075,7 @@ static int at91_adc_probe(struct platform_device *pdev)
> return 0;
>
> dma_disable:
> - at91_adc_dma_disable(pdev);
> + at91_adc_dma_disable(st);
> per_clk_disable_unprepare:
> clk_disable_unprepare(st->per_clk);
> vref_disable:
> @@ -2094,7 +2092,7 @@ static int at91_adc_remove(struct platform_device *pdev)
>
> iio_device_unregister(indio_dev);
>
> - at91_adc_dma_disable(pdev);
> + at91_adc_dma_disable(st);
>
> clk_disable_unprepare(st->per_clk);
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 1/2] iio: at91-sama5d2: Fix incorrect cast to platform_device
2021-10-28 14:34 ` [PATCH 1/2] iio: at91-sama5d2: Fix incorrect cast to platform_device Jonathan Cameron
@ 2021-10-28 14:39 ` Eugen.Hristev
2021-11-04 12:30 ` Eugen.Hristev
0 siblings, 1 reply; 6+ messages in thread
From: Eugen.Hristev @ 2021-10-28 14:39 UTC (permalink / raw)
To: jic23, lars; +Cc: linux-iio
On 10/28/21 5:34 PM, Jonathan Cameron wrote:
> On Tue, 19 Oct 2021 10:29:28 +0200
> Lars-Peter Clausen <lars@metafoo.de> wrote:
>
>> The at91-sama5d2 driver calls `to_platform_device()` on a struct device
>> that is part of a IIO device. This is incorrect since
>> `to_platform_device()` must only be called on a struct device that is part
>> of a platform device.
>>
>> The code still works by accident because non of the struct platform_device
>> specific fields are accessed.
>>
>> Refactor the code a bit so that it behaves identically, but does not use
>> the incorrect cast. This avoids accidentally adding undefined behavior in
>> the future by assuming the `struct platform_device` is actually valid.
>>
>> Signed-off-by: Lars-Peter Clausen <lars@metafoo.de>
>
> This makes me nervous for the reason you give below.
> Looking for a response from Eugen or someone else with access to the device
> before I apply this one.
Hi,
I will take some time to test this on my setup. Thanks for the patch,
and sorry for the delays !
Eugen
>
> Thanks,
>
> Jonathan
>
>> ---
>> The code is equivalent to before, but I'm a bit confused how this works.
>> We call dma_request_chan() on the IIO device's struct device. Which should
>> not yield any results.
>>
>> Eugen can you check if/why this works and see if a follow up patch using
>> the right struct device (the platform_device's) to request the DMA channel
>> makes sense?
>> ---
>> drivers/iio/adc/at91-sama5d2_adc.c | 34 ++++++++++++++----------------
>> 1 file changed, 16 insertions(+), 18 deletions(-)
>>
>> diff --git a/drivers/iio/adc/at91-sama5d2_adc.c b/drivers/iio/adc/at91-sama5d2_adc.c
>> index 4c922ef634f8..3841e7b6c81d 100644
>> --- a/drivers/iio/adc/at91-sama5d2_adc.c
>> +++ b/drivers/iio/adc/at91-sama5d2_adc.c
>> @@ -1661,10 +1661,9 @@ static int at91_adc_write_raw(struct iio_dev *indio_dev,
>> }
>> }
>>
>> -static void at91_adc_dma_init(struct platform_device *pdev)
>> +static void at91_adc_dma_init(struct at91_adc_state *st)
>> {
>> - struct iio_dev *indio_dev = platform_get_drvdata(pdev);
>> - struct at91_adc_state *st = iio_priv(indio_dev);
>> + struct device *dev = &st->indio_dev->dev;
>> struct dma_slave_config config = {0};
>> /* we have 2 bytes for each channel */
>> unsigned int sample_size = st->soc_info.platform->nr_channels * 2;
>> @@ -1679,9 +1678,9 @@ static void at91_adc_dma_init(struct platform_device *pdev)
>> if (st->dma_st.dma_chan)
>> return;
>>
>> - st->dma_st.dma_chan = dma_request_chan(&pdev->dev, "rx");
>> + st->dma_st.dma_chan = dma_request_chan(dev, "rx");
>> if (IS_ERR(st->dma_st.dma_chan)) {
>> - dev_info(&pdev->dev, "can't get DMA channel\n");
>> + dev_info(dev, "can't get DMA channel\n");
>> st->dma_st.dma_chan = NULL;
>> goto dma_exit;
>> }
>> @@ -1691,7 +1690,7 @@ static void at91_adc_dma_init(struct platform_device *pdev)
>> &st->dma_st.rx_dma_buf,
>> GFP_KERNEL);
>> if (!st->dma_st.rx_buf) {
>> - dev_info(&pdev->dev, "can't allocate coherent DMA area\n");
>> + dev_info(dev, "can't allocate coherent DMA area\n");
>> goto dma_chan_disable;
>> }
>>
>> @@ -1704,11 +1703,11 @@ static void at91_adc_dma_init(struct platform_device *pdev)
>> config.dst_maxburst = 1;
>>
>> if (dmaengine_slave_config(st->dma_st.dma_chan, &config)) {
>> - dev_info(&pdev->dev, "can't configure DMA slave\n");
>> + dev_info(dev, "can't configure DMA slave\n");
>> goto dma_free_area;
>> }
>>
>> - dev_info(&pdev->dev, "using %s for rx DMA transfers\n",
>> + dev_info(dev, "using %s for rx DMA transfers\n",
>> dma_chan_name(st->dma_st.dma_chan));
>>
>> return;
>> @@ -1720,13 +1719,12 @@ static void at91_adc_dma_init(struct platform_device *pdev)
>> dma_release_channel(st->dma_st.dma_chan);
>> st->dma_st.dma_chan = NULL;
>> dma_exit:
>> - dev_info(&pdev->dev, "continuing without DMA support\n");
>> + dev_info(dev, "continuing without DMA support\n");
>> }
>>
>> -static void at91_adc_dma_disable(struct platform_device *pdev)
>> +static void at91_adc_dma_disable(struct at91_adc_state *st)
>> {
>> - struct iio_dev *indio_dev = platform_get_drvdata(pdev);
>> - struct at91_adc_state *st = iio_priv(indio_dev);
>> + struct device *dev = &st->indio_dev->dev;
>> /* we have 2 bytes for each channel */
>> unsigned int sample_size = st->soc_info.platform->nr_channels * 2;
>> unsigned int pages = DIV_ROUND_UP(AT91_HWFIFO_MAX_SIZE *
>> @@ -1744,7 +1742,7 @@ static void at91_adc_dma_disable(struct platform_device *pdev)
>> dma_release_channel(st->dma_st.dma_chan);
>> st->dma_st.dma_chan = NULL;
>>
>> - dev_info(&pdev->dev, "continuing without DMA support\n");
>> + dev_info(dev, "continuing without DMA support\n");
>> }
>>
>> static int at91_adc_set_watermark(struct iio_dev *indio_dev, unsigned int val)
>> @@ -1770,9 +1768,9 @@ static int at91_adc_set_watermark(struct iio_dev *indio_dev, unsigned int val)
>> */
>>
>> if (val == 1)
>> - at91_adc_dma_disable(to_platform_device(&indio_dev->dev));
>> + at91_adc_dma_disable(st);
>> else if (val > 1)
>> - at91_adc_dma_init(to_platform_device(&indio_dev->dev));
>> + at91_adc_dma_init(st);
>>
>> /*
>> * We can start the DMA only after setting the watermark and
>> @@ -1780,7 +1778,7 @@ static int at91_adc_set_watermark(struct iio_dev *indio_dev, unsigned int val)
>> */
>> ret = at91_adc_buffer_prepare(indio_dev);
>> if (ret)
>> - at91_adc_dma_disable(to_platform_device(&indio_dev->dev));
>> + at91_adc_dma_disable(st);
>>
>> return ret;
>> }
>> @@ -2077,7 +2075,7 @@ static int at91_adc_probe(struct platform_device *pdev)
>> return 0;
>>
>> dma_disable:
>> - at91_adc_dma_disable(pdev);
>> + at91_adc_dma_disable(st);
>> per_clk_disable_unprepare:
>> clk_disable_unprepare(st->per_clk);
>> vref_disable:
>> @@ -2094,7 +2092,7 @@ static int at91_adc_remove(struct platform_device *pdev)
>>
>> iio_device_unregister(indio_dev);
>>
>> - at91_adc_dma_disable(pdev);
>> + at91_adc_dma_disable(st);
>>
>> clk_disable_unprepare(st->per_clk);
>>
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 1/2] iio: at91-sama5d2: Fix incorrect cast to platform_device
2021-10-28 14:39 ` Eugen.Hristev
@ 2021-11-04 12:30 ` Eugen.Hristev
2021-11-04 18:06 ` Jonathan Cameron
0 siblings, 1 reply; 6+ messages in thread
From: Eugen.Hristev @ 2021-11-04 12:30 UTC (permalink / raw)
To: jic23, lars; +Cc: linux-iio
On 10/28/21 5:39 PM, Eugen Hristev - M18282 wrote:
> On 10/28/21 5:34 PM, Jonathan Cameron wrote:
>> On Tue, 19 Oct 2021 10:29:28 +0200
>> Lars-Peter Clausen <lars@metafoo.de> wrote:
>>
>>> The at91-sama5d2 driver calls `to_platform_device()` on a struct device
>>> that is part of a IIO device. This is incorrect since
>>> `to_platform_device()` must only be called on a struct device that is part
>>> of a platform device.
>>>
>>> The code still works by accident because non of the struct platform_device
>>> specific fields are accessed.
>>>
>>> Refactor the code a bit so that it behaves identically, but does not use
>>> the incorrect cast. This avoids accidentally adding undefined behavior in
>>> the future by assuming the `struct platform_device` is actually valid.
>>>
>>> Signed-off-by: Lars-Peter Clausen <lars@metafoo.de>
>>
>> This makes me nervous for the reason you give below.
>> Looking for a response from Eugen or someone else with access to the device
>> before I apply this one.
>
> Hi,
>
> I will take some time to test this on my setup. Thanks for the patch,
> and sorry for the delays !
>
> Eugen
Hello Jonathan and Lars,
Sorry for the delay,
I have applied the patches in my tree and tested basic DMA transfers
(sanity checks) on my board.
It looks to be fine. Have not found any weird or non functioning behavior.
You can add my
Tested-by: Eugen Hristev <eugen.hristev@microchip.com>
About the query below,
>
>>
>> Thanks,
>>
>> Jonathan
>>
>>> ---
>>> The code is equivalent to before, but I'm a bit confused how this works.
>>> We call dma_request_chan() on the IIO device's struct device. Which should
>>> not yield any results.
>>>
>>> Eugen can you check if/why this works and see if a follow up patch using
>>> the right struct device (the platform_device's) to request the DMA channel
>>> makes sense?
I do not fully understand the problem yet. Why should dma_request_chan
on the iio dev should not yield any results? the dma channel is
allocated and it worked so far.
I tried to following: save the pdev pointer in the state struct, and
then use dma_request_chan on the pdev->dev . It still works and I could
not find any difference in functionality in basic sanity checks.
I believe that the dma_request_chan wants a valid struct device. If it's
iio's dev or platform device's dev, it doesn't seem to matter at the
first glance.
Could you please detail a bit how it should be used ?
Thanks,
Eugen
>>> ---
>>> drivers/iio/adc/at91-sama5d2_adc.c | 34 ++++++++++++++----------------
>>> 1 file changed, 16 insertions(+), 18 deletions(-)
>>>
>>> diff --git a/drivers/iio/adc/at91-sama5d2_adc.c b/drivers/iio/adc/at91-sama5d2_adc.c
>>> index 4c922ef634f8..3841e7b6c81d 100644
>>> --- a/drivers/iio/adc/at91-sama5d2_adc.c
>>> +++ b/drivers/iio/adc/at91-sama5d2_adc.c
>>> @@ -1661,10 +1661,9 @@ static int at91_adc_write_raw(struct iio_dev *indio_dev,
>>> }
>>> }
>>>
>>> -static void at91_adc_dma_init(struct platform_device *pdev)
>>> +static void at91_adc_dma_init(struct at91_adc_state *st)
>>> {
>>> - struct iio_dev *indio_dev = platform_get_drvdata(pdev);
>>> - struct at91_adc_state *st = iio_priv(indio_dev);
>>> + struct device *dev = &st->indio_dev->dev;
>>> struct dma_slave_config config = {0};
>>> /* we have 2 bytes for each channel */
>>> unsigned int sample_size = st->soc_info.platform->nr_channels * 2;
>>> @@ -1679,9 +1678,9 @@ static void at91_adc_dma_init(struct platform_device *pdev)
>>> if (st->dma_st.dma_chan)
>>> return;
>>>
>>> - st->dma_st.dma_chan = dma_request_chan(&pdev->dev, "rx");
>>> + st->dma_st.dma_chan = dma_request_chan(dev, "rx");
>>> if (IS_ERR(st->dma_st.dma_chan)) {
>>> - dev_info(&pdev->dev, "can't get DMA channel\n");
>>> + dev_info(dev, "can't get DMA channel\n");
>>> st->dma_st.dma_chan = NULL;
>>> goto dma_exit;
>>> }
>>> @@ -1691,7 +1690,7 @@ static void at91_adc_dma_init(struct platform_device *pdev)
>>> &st->dma_st.rx_dma_buf,
>>> GFP_KERNEL);
>>> if (!st->dma_st.rx_buf) {
>>> - dev_info(&pdev->dev, "can't allocate coherent DMA area\n");
>>> + dev_info(dev, "can't allocate coherent DMA area\n");
>>> goto dma_chan_disable;
>>> }
>>>
>>> @@ -1704,11 +1703,11 @@ static void at91_adc_dma_init(struct platform_device *pdev)
>>> config.dst_maxburst = 1;
>>>
>>> if (dmaengine_slave_config(st->dma_st.dma_chan, &config)) {
>>> - dev_info(&pdev->dev, "can't configure DMA slave\n");
>>> + dev_info(dev, "can't configure DMA slave\n");
>>> goto dma_free_area;
>>> }
>>>
>>> - dev_info(&pdev->dev, "using %s for rx DMA transfers\n",
>>> + dev_info(dev, "using %s for rx DMA transfers\n",
>>> dma_chan_name(st->dma_st.dma_chan));
>>>
>>> return;
>>> @@ -1720,13 +1719,12 @@ static void at91_adc_dma_init(struct platform_device *pdev)
>>> dma_release_channel(st->dma_st.dma_chan);
>>> st->dma_st.dma_chan = NULL;
>>> dma_exit:
>>> - dev_info(&pdev->dev, "continuing without DMA support\n");
>>> + dev_info(dev, "continuing without DMA support\n");
>>> }
>>>
>>> -static void at91_adc_dma_disable(struct platform_device *pdev)
>>> +static void at91_adc_dma_disable(struct at91_adc_state *st)
>>> {
>>> - struct iio_dev *indio_dev = platform_get_drvdata(pdev);
>>> - struct at91_adc_state *st = iio_priv(indio_dev);
>>> + struct device *dev = &st->indio_dev->dev;
>>> /* we have 2 bytes for each channel */
>>> unsigned int sample_size = st->soc_info.platform->nr_channels * 2;
>>> unsigned int pages = DIV_ROUND_UP(AT91_HWFIFO_MAX_SIZE *
>>> @@ -1744,7 +1742,7 @@ static void at91_adc_dma_disable(struct platform_device *pdev)
>>> dma_release_channel(st->dma_st.dma_chan);
>>> st->dma_st.dma_chan = NULL;
>>>
>>> - dev_info(&pdev->dev, "continuing without DMA support\n");
>>> + dev_info(dev, "continuing without DMA support\n");
>>> }
>>>
>>> static int at91_adc_set_watermark(struct iio_dev *indio_dev, unsigned int val)
>>> @@ -1770,9 +1768,9 @@ static int at91_adc_set_watermark(struct iio_dev *indio_dev, unsigned int val)
>>> */
>>>
>>> if (val == 1)
>>> - at91_adc_dma_disable(to_platform_device(&indio_dev->dev));
>>> + at91_adc_dma_disable(st);
>>> else if (val > 1)
>>> - at91_adc_dma_init(to_platform_device(&indio_dev->dev));
>>> + at91_adc_dma_init(st);
>>>
>>> /*
>>> * We can start the DMA only after setting the watermark and
>>> @@ -1780,7 +1778,7 @@ static int at91_adc_set_watermark(struct iio_dev *indio_dev, unsigned int val)
>>> */
>>> ret = at91_adc_buffer_prepare(indio_dev);
>>> if (ret)
>>> - at91_adc_dma_disable(to_platform_device(&indio_dev->dev));
>>> + at91_adc_dma_disable(st);
>>>
>>> return ret;
>>> }
>>> @@ -2077,7 +2075,7 @@ static int at91_adc_probe(struct platform_device *pdev)
>>> return 0;
>>>
>>> dma_disable:
>>> - at91_adc_dma_disable(pdev);
>>> + at91_adc_dma_disable(st);
>>> per_clk_disable_unprepare:
>>> clk_disable_unprepare(st->per_clk);
>>> vref_disable:
>>> @@ -2094,7 +2092,7 @@ static int at91_adc_remove(struct platform_device *pdev)
>>>
>>> iio_device_unregister(indio_dev);
>>>
>>> - at91_adc_dma_disable(pdev);
>>> + at91_adc_dma_disable(st);
>>>
>>> clk_disable_unprepare(st->per_clk);
>>>
>>
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 1/2] iio: at91-sama5d2: Fix incorrect cast to platform_device
2021-11-04 12:30 ` Eugen.Hristev
@ 2021-11-04 18:06 ` Jonathan Cameron
0 siblings, 0 replies; 6+ messages in thread
From: Jonathan Cameron @ 2021-11-04 18:06 UTC (permalink / raw)
To: Eugen.Hristev; +Cc: lars, linux-iio
On Thu, 4 Nov 2021 12:30:57 +0000
<Eugen.Hristev@microchip.com> wrote:
> On 10/28/21 5:39 PM, Eugen Hristev - M18282 wrote:
> > On 10/28/21 5:34 PM, Jonathan Cameron wrote:
> >> On Tue, 19 Oct 2021 10:29:28 +0200
> >> Lars-Peter Clausen <lars@metafoo.de> wrote:
> >>
> >>> The at91-sama5d2 driver calls `to_platform_device()` on a struct device
> >>> that is part of a IIO device. This is incorrect since
> >>> `to_platform_device()` must only be called on a struct device that is part
> >>> of a platform device.
> >>>
> >>> The code still works by accident because non of the struct platform_device
> >>> specific fields are accessed.
> >>>
> >>> Refactor the code a bit so that it behaves identically, but does not use
> >>> the incorrect cast. This avoids accidentally adding undefined behavior in
> >>> the future by assuming the `struct platform_device` is actually valid.
> >>>
> >>> Signed-off-by: Lars-Peter Clausen <lars@metafoo.de>
> >>
> >> This makes me nervous for the reason you give below.
> >> Looking for a response from Eugen or someone else with access to the device
> >> before I apply this one.
> >
> > Hi,
> >
> > I will take some time to test this on my setup. Thanks for the patch,
> > and sorry for the delays !
> >
> > Eugen
>
> Hello Jonathan and Lars,
>
> Sorry for the delay,
>
> I have applied the patches in my tree and tested basic DMA transfers
> (sanity checks) on my board.
> It looks to be fine. Have not found any weird or non functioning behavior.
>
> You can add my
> Tested-by: Eugen Hristev <eugen.hristev@microchip.com>
I've queued this up now, but hope Lars can address the questions below.
Thanks,
Jonathan
>
> About the query below,
> >
> >>
> >> Thanks,
> >>
> >> Jonathan
> >>
> >>> ---
> >>> The code is equivalent to before, but I'm a bit confused how this works.
> >>> We call dma_request_chan() on the IIO device's struct device. Which should
> >>> not yield any results.
> >>>
> >>> Eugen can you check if/why this works and see if a follow up patch using
> >>> the right struct device (the platform_device's) to request the DMA channel
> >>> makes sense?
>
> I do not fully understand the problem yet. Why should dma_request_chan
> on the iio dev should not yield any results? the dma channel is
> allocated and it worked so far.
>
> I tried to following: save the pdev pointer in the state struct, and
> then use dma_request_chan on the pdev->dev . It still works and I could
> not find any difference in functionality in basic sanity checks.
>
> I believe that the dma_request_chan wants a valid struct device. If it's
> iio's dev or platform device's dev, it doesn't seem to matter at the
> first glance.
>
> Could you please detail a bit how it should be used ?
>
> Thanks,
> Eugen
>
> >>> ---
> >>> drivers/iio/adc/at91-sama5d2_adc.c | 34 ++++++++++++++----------------
> >>> 1 file changed, 16 insertions(+), 18 deletions(-)
> >>>
> >>> diff --git a/drivers/iio/adc/at91-sama5d2_adc.c b/drivers/iio/adc/at91-sama5d2_adc.c
> >>> index 4c922ef634f8..3841e7b6c81d 100644
> >>> --- a/drivers/iio/adc/at91-sama5d2_adc.c
> >>> +++ b/drivers/iio/adc/at91-sama5d2_adc.c
> >>> @@ -1661,10 +1661,9 @@ static int at91_adc_write_raw(struct iio_dev *indio_dev,
> >>> }
> >>> }
> >>>
> >>> -static void at91_adc_dma_init(struct platform_device *pdev)
> >>> +static void at91_adc_dma_init(struct at91_adc_state *st)
> >>> {
> >>> - struct iio_dev *indio_dev = platform_get_drvdata(pdev);
> >>> - struct at91_adc_state *st = iio_priv(indio_dev);
> >>> + struct device *dev = &st->indio_dev->dev;
> >>> struct dma_slave_config config = {0};
> >>> /* we have 2 bytes for each channel */
> >>> unsigned int sample_size = st->soc_info.platform->nr_channels * 2;
> >>> @@ -1679,9 +1678,9 @@ static void at91_adc_dma_init(struct platform_device *pdev)
> >>> if (st->dma_st.dma_chan)
> >>> return;
> >>>
> >>> - st->dma_st.dma_chan = dma_request_chan(&pdev->dev, "rx");
> >>> + st->dma_st.dma_chan = dma_request_chan(dev, "rx");
> >>> if (IS_ERR(st->dma_st.dma_chan)) {
> >>> - dev_info(&pdev->dev, "can't get DMA channel\n");
> >>> + dev_info(dev, "can't get DMA channel\n");
> >>> st->dma_st.dma_chan = NULL;
> >>> goto dma_exit;
> >>> }
> >>> @@ -1691,7 +1690,7 @@ static void at91_adc_dma_init(struct platform_device *pdev)
> >>> &st->dma_st.rx_dma_buf,
> >>> GFP_KERNEL);
> >>> if (!st->dma_st.rx_buf) {
> >>> - dev_info(&pdev->dev, "can't allocate coherent DMA area\n");
> >>> + dev_info(dev, "can't allocate coherent DMA area\n");
> >>> goto dma_chan_disable;
> >>> }
> >>>
> >>> @@ -1704,11 +1703,11 @@ static void at91_adc_dma_init(struct platform_device *pdev)
> >>> config.dst_maxburst = 1;
> >>>
> >>> if (dmaengine_slave_config(st->dma_st.dma_chan, &config)) {
> >>> - dev_info(&pdev->dev, "can't configure DMA slave\n");
> >>> + dev_info(dev, "can't configure DMA slave\n");
> >>> goto dma_free_area;
> >>> }
> >>>
> >>> - dev_info(&pdev->dev, "using %s for rx DMA transfers\n",
> >>> + dev_info(dev, "using %s for rx DMA transfers\n",
> >>> dma_chan_name(st->dma_st.dma_chan));
> >>>
> >>> return;
> >>> @@ -1720,13 +1719,12 @@ static void at91_adc_dma_init(struct platform_device *pdev)
> >>> dma_release_channel(st->dma_st.dma_chan);
> >>> st->dma_st.dma_chan = NULL;
> >>> dma_exit:
> >>> - dev_info(&pdev->dev, "continuing without DMA support\n");
> >>> + dev_info(dev, "continuing without DMA support\n");
> >>> }
> >>>
> >>> -static void at91_adc_dma_disable(struct platform_device *pdev)
> >>> +static void at91_adc_dma_disable(struct at91_adc_state *st)
> >>> {
> >>> - struct iio_dev *indio_dev = platform_get_drvdata(pdev);
> >>> - struct at91_adc_state *st = iio_priv(indio_dev);
> >>> + struct device *dev = &st->indio_dev->dev;
> >>> /* we have 2 bytes for each channel */
> >>> unsigned int sample_size = st->soc_info.platform->nr_channels * 2;
> >>> unsigned int pages = DIV_ROUND_UP(AT91_HWFIFO_MAX_SIZE *
> >>> @@ -1744,7 +1742,7 @@ static void at91_adc_dma_disable(struct platform_device *pdev)
> >>> dma_release_channel(st->dma_st.dma_chan);
> >>> st->dma_st.dma_chan = NULL;
> >>>
> >>> - dev_info(&pdev->dev, "continuing without DMA support\n");
> >>> + dev_info(dev, "continuing without DMA support\n");
> >>> }
> >>>
> >>> static int at91_adc_set_watermark(struct iio_dev *indio_dev, unsigned int val)
> >>> @@ -1770,9 +1768,9 @@ static int at91_adc_set_watermark(struct iio_dev *indio_dev, unsigned int val)
> >>> */
> >>>
> >>> if (val == 1)
> >>> - at91_adc_dma_disable(to_platform_device(&indio_dev->dev));
> >>> + at91_adc_dma_disable(st);
> >>> else if (val > 1)
> >>> - at91_adc_dma_init(to_platform_device(&indio_dev->dev));
> >>> + at91_adc_dma_init(st);
> >>>
> >>> /*
> >>> * We can start the DMA only after setting the watermark and
> >>> @@ -1780,7 +1778,7 @@ static int at91_adc_set_watermark(struct iio_dev *indio_dev, unsigned int val)
> >>> */
> >>> ret = at91_adc_buffer_prepare(indio_dev);
> >>> if (ret)
> >>> - at91_adc_dma_disable(to_platform_device(&indio_dev->dev));
> >>> + at91_adc_dma_disable(st);
> >>>
> >>> return ret;
> >>> }
> >>> @@ -2077,7 +2075,7 @@ static int at91_adc_probe(struct platform_device *pdev)
> >>> return 0;
> >>>
> >>> dma_disable:
> >>> - at91_adc_dma_disable(pdev);
> >>> + at91_adc_dma_disable(st);
> >>> per_clk_disable_unprepare:
> >>> clk_disable_unprepare(st->per_clk);
> >>> vref_disable:
> >>> @@ -2094,7 +2092,7 @@ static int at91_adc_remove(struct platform_device *pdev)
> >>>
> >>> iio_device_unregister(indio_dev);
> >>>
> >>> - at91_adc_dma_disable(pdev);
> >>> + at91_adc_dma_disable(st);
> >>>
> >>> clk_disable_unprepare(st->per_clk);
> >>>
> >>
> >
>
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2021-11-04 18:02 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-10-19 8:29 [PATCH 1/2] iio: at91-sama5d2: Fix incorrect cast to platform_device Lars-Peter Clausen
2021-10-19 8:29 ` [PATCH 2/2] iio: at91-sama5d2: Use dev_to_iio_dev() in sysfs callbacks Lars-Peter Clausen
2021-10-28 14:34 ` [PATCH 1/2] iio: at91-sama5d2: Fix incorrect cast to platform_device Jonathan Cameron
2021-10-28 14:39 ` Eugen.Hristev
2021-11-04 12:30 ` Eugen.Hristev
2021-11-04 18:06 ` Jonathan Cameron
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.