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

[Greg Kroah-Hartman <gregkh@linuxfoundation.org>]

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