| Message ID | 20211019082929.30503-1-lars@metafoo.de (mailing list archive) |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [1/2] iio: at91-sama5d2: Fix incorrect cast to platform_device | expand |
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); >
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); >> >
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); >>> >> >
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); > >>> > >> > > >
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);
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(-)