[PATCH] staging: comedi: comedi_isadma: Use a non-NULL device for DMA API

[PATCH] staging: comedi: comedi_isadma: Use a non-NULL device for DMA API

[Ian Abbott]

The "comedi_isadma" module calls `dma_alloc_coherent()` and
`dma_free_coherent()` with a NULL device pointer which is no longer
allowed.  If the `hw_dev` member of the `struct comedi_device` has been
set to a valid device, that can be used instead.  Unfortunately, all the
current users of the "comedi_isadma" module leave the `hw_dev` member
set to NULL.  In that case, use a static dummy fallback device structure
with the coherent DMA mask set to the ISA bus limit of 16MB.

Signed-off-by: Ian Abbott <abbotti@mev.co.uk>
---
 drivers/staging/comedi/drivers/comedi_isadma.c | 15 +++++++++++++--
 drivers/staging/comedi/drivers/comedi_isadma.h |  3 +++
 2 files changed, 16 insertions(+), 2 deletions(-)

diff --git a/drivers/staging/comedi/drivers/comedi_isadma.c b/drivers/staging/comedi/drivers/comedi_isadma.c
index b77dc8d5d3ff..8929952516a1 100644
--- a/drivers/staging/comedi/drivers/comedi_isadma.c
+++ b/drivers/staging/comedi/drivers/comedi_isadma.c
@@ -14,6 +14,16 @@
 
 #include "comedi_isadma.h"
 
+/*
+ * Fallback device used when hardware device is NULL.
+ * This can be removed after drivers have been converted to use isa_driver.
+ */
+static struct device fallback_dev = {
+	.init_name = "comedi_isadma fallback device",
+	.coherent_dma_mask = DMA_BIT_MASK(24),
+	.dma_mask = &fallback_dev.coherent_dma_mask,
+};
+
 /**
  * comedi_isadma_program - program and enable an ISA DMA transfer
  * @desc:	the ISA DMA cookie to program and enable
@@ -172,6 +182,7 @@ struct comedi_isadma *comedi_isadma_alloc(struct comedi_device *dev,
 		goto no_dma;
 	dma->desc = desc;
 	dma->n_desc = n_desc;
+	dma->dev = dev->hw_dev ? dev->hw_dev : &fallback_dev;
 
 	dma_chans[0] = dma_chan1;
 	if (dma_chan2 == 0 || dma_chan2 == dma_chan1)
@@ -192,7 +203,7 @@ struct comedi_isadma *comedi_isadma_alloc(struct comedi_device *dev,
 		desc = &dma->desc[i];
 		desc->chan = dma_chans[i];
 		desc->maxsize = maxsize;
-		desc->virt_addr = dma_alloc_coherent(NULL, desc->maxsize,
+		desc->virt_addr = dma_alloc_coherent(dma->dev, desc->maxsize,
 						     &desc->hw_addr,
 						     GFP_KERNEL);
 		if (!desc->virt_addr)
@@ -224,7 +235,7 @@ void comedi_isadma_free(struct comedi_isadma *dma)
 		for (i = 0; i < dma->n_desc; i++) {
 			desc = &dma->desc[i];
 			if (desc->virt_addr)
-				dma_free_coherent(NULL, desc->maxsize,
+				dma_free_coherent(dma->dev, desc->maxsize,
 						  desc->virt_addr,
 						  desc->hw_addr);
 		}
diff --git a/drivers/staging/comedi/drivers/comedi_isadma.h b/drivers/staging/comedi/drivers/comedi_isadma.h
index 2bd1329d727f..9d2b12db7e6e 100644
--- a/drivers/staging/comedi/drivers/comedi_isadma.h
+++ b/drivers/staging/comedi/drivers/comedi_isadma.h
@@ -10,6 +10,7 @@
 #include <linux/types.h>
 
 struct comedi_device;
+struct device;
 
 /*
  * These are used to avoid issues when <asm/dma.h> and the DMA_MODE_
@@ -38,6 +39,7 @@ struct comedi_isadma_desc {
 
 /**
  * struct comedi_isadma - ISA DMA data
+ * @dev:	device to allocate non-coherent memory for
  * @desc:	cookie for each DMA buffer
  * @n_desc:	the number of cookies
  * @cur_dma:	the current cookie in use
@@ -45,6 +47,7 @@ struct comedi_isadma_desc {
  * @chan2:	the second DMA channel requested
  */
 struct comedi_isadma {
+	struct device *dev;
 	struct comedi_isadma_desc *desc;
 	int n_desc;
 	int cur_dma;
-- 
2.20.1

Re: [PATCH] staging: comedi: comedi_isadma: Use a non-NULL device for DMA API

[Greg Kroah-Hartman]

On Thu, Apr 25, 2019 at 05:26:44PM +0100, Ian Abbott wrote:
> The "comedi_isadma" module calls `dma_alloc_coherent()` and
> `dma_free_coherent()` with a NULL device pointer which is no longer
> allowed.  If the `hw_dev` member of the `struct comedi_device` has been
> set to a valid device, that can be used instead.  Unfortunately, all the
> current users of the "comedi_isadma" module leave the `hw_dev` member
> set to NULL.  In that case, use a static dummy fallback device structure
> with the coherent DMA mask set to the ISA bus limit of 16MB.
> 
> Signed-off-by: Ian Abbott <abbotti@mev.co.uk>
> ---
>  drivers/staging/comedi/drivers/comedi_isadma.c | 15 +++++++++++++--
>  drivers/staging/comedi/drivers/comedi_isadma.h |  3 +++
>  2 files changed, 16 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/staging/comedi/drivers/comedi_isadma.c b/drivers/staging/comedi/drivers/comedi_isadma.c
> index b77dc8d5d3ff..8929952516a1 100644
> --- a/drivers/staging/comedi/drivers/comedi_isadma.c
> +++ b/drivers/staging/comedi/drivers/comedi_isadma.c
> @@ -14,6 +14,16 @@
>  
>  #include "comedi_isadma.h"
>  
> +/*
> + * Fallback device used when hardware device is NULL.
> + * This can be removed after drivers have been converted to use isa_driver.
> + */
> +static struct device fallback_dev = {
> +	.init_name = "comedi_isadma fallback device",
> +	.coherent_dma_mask = DMA_BIT_MASK(24),
> +	.dma_mask = &fallback_dev.coherent_dma_mask,
> +};

Ick, no, static struct device are a very bad idea as this is a reference
counted structure and making it static can cause odd problems.

Why not just create a "real" one?  Or better yet, use the real device
for the comedi device as all of these drivers should have one now.

thanks,

greg k-h

Re: [PATCH] staging: comedi: comedi_isadma: Use a non-NULL device for DMA API

[Ian Abbott]

On 25/04/2019 18:13, Greg Kroah-Hartman wrote:
> On Thu, Apr 25, 2019 at 05:26:44PM +0100, Ian Abbott wrote:
>> The "comedi_isadma" module calls `dma_alloc_coherent()` and
>> `dma_free_coherent()` with a NULL device pointer which is no longer
>> allowed.  If the `hw_dev` member of the `struct comedi_device` has been
>> set to a valid device, that can be used instead.  Unfortunately, all the
>> current users of the "comedi_isadma" module leave the `hw_dev` member
>> set to NULL.  In that case, use a static dummy fallback device structure
>> with the coherent DMA mask set to the ISA bus limit of 16MB.
>>
>> Signed-off-by: Ian Abbott <abbotti@mev.co.uk>
>> ---
>>   drivers/staging/comedi/drivers/comedi_isadma.c | 15 +++++++++++++--
>>   drivers/staging/comedi/drivers/comedi_isadma.h |  3 +++
>>   2 files changed, 16 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/staging/comedi/drivers/comedi_isadma.c b/drivers/staging/comedi/drivers/comedi_isadma.c
>> index b77dc8d5d3ff..8929952516a1 100644
>> --- a/drivers/staging/comedi/drivers/comedi_isadma.c
>> +++ b/drivers/staging/comedi/drivers/comedi_isadma.c
>> @@ -14,6 +14,16 @@
>>   
>>   #include "comedi_isadma.h"
>>   
>> +/*
>> + * Fallback device used when hardware device is NULL.
>> + * This can be removed after drivers have been converted to use isa_driver.
>> + */
>> +static struct device fallback_dev = {
>> +	.init_name = "comedi_isadma fallback device",
>> +	.coherent_dma_mask = DMA_BIT_MASK(24),
>> +	.dma_mask = &fallback_dev.coherent_dma_mask,
>> +};
> 
> Ick, no, static struct device are a very bad idea as this is a reference
> counted structure and making it static can cause odd problems.

This was based on the use of `struct device x86_dma_fallback_dev` in 
"arch/x86/kernel/pci-dma.c", and `static struct device isa_dma_dev` in 
"arch/arm/kernel/dma-isa.c", but perhaps it is not appropriate in 
non-arch code.

> Why not just create a "real" one?  Or better yet, use the real device
> for the comedi device as all of these drivers should have one now.

I suppose I could use the comedi class device pointed to by the 
`class_dev` member of `struct comedi_device` (although that could also 
be NULL because the comedi core does not currently treat 
`device_create()` failures as fatal).

> 
> thanks,
> 
> greg k-h

Thanks for the review,

Ian Abbott.

-- 
-=( Ian Abbott <abbotti@mev.co.uk> || Web: www.mev.co.uk )=-
-=( MEV Ltd. is a company registered in England & Wales. )=-
-=( Registered number: 02862268.  Registered address:    )=-
-=( 15 West Park Road, Bramhall, STOCKPORT, SK7 3JZ, UK. )=-

[PATCH v2] staging: comedi: comedi_isadma: Use a non-NULL device for DMA API

[Ian Abbott]

The "comedi_isadma" module calls `dma_alloc_coherent()` and
`dma_free_coherent()` with a NULL device pointer which is no longer
allowed.  If the `hw_dev` member of the `struct comedi_device` has been
set to a valid device, that can be used instead.  Unfortunately, all the
current users of the "comedi_isadma" module leave the `hw_dev` member
set to NULL.  In that case, fall back to using the comedi "class" device
pointed to by the `class_dev` member if that is non-NULL.  In that case,
make it "DMA-capable" with a coherent DMA mask set to the ISA bus limit
of 16MB (24 bits).

Signed-off-by: Ian Abbott <abbotti@mev.co.uk>
---
v2: Use the comedi "class" device instead of a static fallback device.
---
 drivers/staging/comedi/drivers/comedi_isadma.c | 17 +++++++++++++++--
 drivers/staging/comedi/drivers/comedi_isadma.h |  3 +++
 2 files changed, 18 insertions(+), 2 deletions(-)

diff --git a/drivers/staging/comedi/drivers/comedi_isadma.c b/drivers/staging/comedi/drivers/comedi_isadma.c
index b77dc8d5d3ff..c729094298c2 100644
--- a/drivers/staging/comedi/drivers/comedi_isadma.c
+++ b/drivers/staging/comedi/drivers/comedi_isadma.c
@@ -172,6 +172,19 @@ struct comedi_isadma *comedi_isadma_alloc(struct comedi_device *dev,
 		goto no_dma;
 	dma->desc = desc;
 	dma->n_desc = n_desc;
+	if (dev->hw_dev) {
+		dma->dev = dev->hw_dev;
+	} else {
+		/* Fall back to using the "class" device. */
+		if (!dev->class_dev)
+			goto no_dma;
+		/* Need 24-bit mask for ISA DMA. */
+		if (dma_coerce_mask_and_coherent(dev->class_dev,
+						 DMA_BIT_MASK(24))) {
+			goto no_dma;
+		}
+		dma->dev = dev->class_dev;
+	}
 
 	dma_chans[0] = dma_chan1;
 	if (dma_chan2 == 0 || dma_chan2 == dma_chan1)
@@ -192,7 +205,7 @@ struct comedi_isadma *comedi_isadma_alloc(struct comedi_device *dev,
 		desc = &dma->desc[i];
 		desc->chan = dma_chans[i];
 		desc->maxsize = maxsize;
-		desc->virt_addr = dma_alloc_coherent(NULL, desc->maxsize,
+		desc->virt_addr = dma_alloc_coherent(dma->dev, desc->maxsize,
 						     &desc->hw_addr,
 						     GFP_KERNEL);
 		if (!desc->virt_addr)
@@ -224,7 +237,7 @@ void comedi_isadma_free(struct comedi_isadma *dma)
 		for (i = 0; i < dma->n_desc; i++) {
 			desc = &dma->desc[i];
 			if (desc->virt_addr)
-				dma_free_coherent(NULL, desc->maxsize,
+				dma_free_coherent(dma->dev, desc->maxsize,
 						  desc->virt_addr,
 						  desc->hw_addr);
 		}
diff --git a/drivers/staging/comedi/drivers/comedi_isadma.h b/drivers/staging/comedi/drivers/comedi_isadma.h
index 2bd1329d727f..9d2b12db7e6e 100644
--- a/drivers/staging/comedi/drivers/comedi_isadma.h
+++ b/drivers/staging/comedi/drivers/comedi_isadma.h
@@ -10,6 +10,7 @@
 #include <linux/types.h>
 
 struct comedi_device;
+struct device;
 
 /*
  * These are used to avoid issues when <asm/dma.h> and the DMA_MODE_
@@ -38,6 +39,7 @@ struct comedi_isadma_desc {
 
 /**
  * struct comedi_isadma - ISA DMA data
+ * @dev:	device to allocate non-coherent memory for
  * @desc:	cookie for each DMA buffer
  * @n_desc:	the number of cookies
  * @cur_dma:	the current cookie in use
@@ -45,6 +47,7 @@ struct comedi_isadma_desc {
  * @chan2:	the second DMA channel requested
  */
 struct comedi_isadma {
+	struct device *dev;
 	struct comedi_isadma_desc *desc;
 	int n_desc;
 	int cur_dma;
-- 
2.20.1

Re: [PATCH] staging: comedi: comedi_isadma: Use a non-NULL device for DMA API

[Greg Kroah-Hartman]

On Fri, Apr 26, 2019 at 10:41:20AM +0100, Ian Abbott wrote:
> On 25/04/2019 18:13, Greg Kroah-Hartman wrote:
> > On Thu, Apr 25, 2019 at 05:26:44PM +0100, Ian Abbott wrote:
> > > The "comedi_isadma" module calls `dma_alloc_coherent()` and
> > > `dma_free_coherent()` with a NULL device pointer which is no longer
> > > allowed.  If the `hw_dev` member of the `struct comedi_device` has been
> > > set to a valid device, that can be used instead.  Unfortunately, all the
> > > current users of the "comedi_isadma" module leave the `hw_dev` member
> > > set to NULL.  In that case, use a static dummy fallback device structure
> > > with the coherent DMA mask set to the ISA bus limit of 16MB.
> > > 
> > > Signed-off-by: Ian Abbott <abbotti@mev.co.uk>
> > > ---
> > >   drivers/staging/comedi/drivers/comedi_isadma.c | 15 +++++++++++++--
> > >   drivers/staging/comedi/drivers/comedi_isadma.h |  3 +++
> > >   2 files changed, 16 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/drivers/staging/comedi/drivers/comedi_isadma.c b/drivers/staging/comedi/drivers/comedi_isadma.c
> > > index b77dc8d5d3ff..8929952516a1 100644
> > > --- a/drivers/staging/comedi/drivers/comedi_isadma.c
> > > +++ b/drivers/staging/comedi/drivers/comedi_isadma.c
> > > @@ -14,6 +14,16 @@
> > >   #include "comedi_isadma.h"
> > > +/*
> > > + * Fallback device used when hardware device is NULL.
> > > + * This can be removed after drivers have been converted to use isa_driver.
> > > + */
> > > +static struct device fallback_dev = {
> > > +	.init_name = "comedi_isadma fallback device",
> > > +	.coherent_dma_mask = DMA_BIT_MASK(24),
> > > +	.dma_mask = &fallback_dev.coherent_dma_mask,
> > > +};
> > 
> > Ick, no, static struct device are a very bad idea as this is a reference
> > counted structure and making it static can cause odd problems.
> 
> This was based on the use of `struct device x86_dma_fallback_dev` in
> "arch/x86/kernel/pci-dma.c", and `static struct device isa_dma_dev` in
> "arch/arm/kernel/dma-isa.c", but perhaps it is not appropriate in non-arch
> code.

No, those are probably broken as well :)

Ah, I see why, ugh, ISA.

> > Why not just create a "real" one?  Or better yet, use the real device
> > for the comedi device as all of these drivers should have one now.
> 
> I suppose I could use the comedi class device pointed to by the `class_dev`
> member of `struct comedi_device` (although that could also be NULL because
> the comedi core does not currently treat `device_create()` failures as
> fatal).

Why would device_create() fail?  If it does, the driver should not have
been bound to anything.

And no, this shouldn't be the class device, but the "real" hardware
device that does dma, which is what is needed anyway here to allow the
DMA to happen properly.

I guess the problem is with the ISA drivers, right?  And I doubt that is
ever going to get fixed up, hopefully just deleted :)

Your second patch is "good enough", I'll go queue that up now.

thanks,

greg k-h