Re: [PATCH 3/3] dma-mapping: introduce DMA range map, supplanting dma_pfn_offset

[Greg KH]

On Thu, Sep 10, 2020 at 11:13:51AM +0200, Christoph Hellwig wrote:
> On Thu, Sep 10, 2020 at 09:53:51AM +0200, Greg KH wrote:
> > >  		/*
> > >  		 * Please refer to usb_alloc_dev() to see why we set
> > > -		 * dma_mask and dma_pfn_offset.
> > > +		 * dma_mask and dma_range_map.
> > >  		 */
> > >  		intf->dev.dma_mask = dev->dev.dma_mask;
> > > -		intf->dev.dma_pfn_offset = dev->dev.dma_pfn_offset;
> > > +		if (dma_direct_copy_range_map(&intf->dev, &dev->dev))
> > > +			dev_err(&dev->dev, "failed to copy DMA map\n");
> > 
> > We tell the user, but then just keep on running?  Is there anything that
> > we can do here?
> > 
> > If not, why not have dma_direct_copy_range_map() print out the error?
> 
> At least for USB I'm pretty sure this isn't required at all.  I've been
> running with the patch below on my desktop for two days now trying all
> the usb toys I have (in addition to grepping for obvious abuses in
> the drivers).  remoteproc is a different story, but the DMA handling
> seems there is sketchy to start with..
> 
> ---
> >From 8bae3e6833f2ca431dcfcbc8f9cced7d5e972a01 Mon Sep 17 00:00:00 2001
> From: Christoph Hellwig <hch@xxxxxx>
> Date: Wed, 9 Sep 2020 08:28:59 +0200
> Subject: usb: don't inherity DMA properties for USB devices
> 
> As the comment in usb_alloc_dev correctly states, drivers can't use
> the DMA API on usb device, and at least calling dma_set_mask on them
> is highly dangerous.  Unlike what the comment states upper level drivers
> also can't really use the presence of a dma mask to check for DMA
> support, as the dma_mask is set by default for most busses.
> 
> Remove the copying over of DMA information, and remove the now unused
> dma_direct_copy_range_map export.
> 
> Signed-off-by: Christoph Hellwig <hch@xxxxxx>
> ---
>  drivers/usb/core/message.c |  7 -------
>  drivers/usb/core/usb.c     | 13 -------------
>  kernel/dma/direct.c        |  1 -
>  3 files changed, 21 deletions(-)
> 
> diff --git a/drivers/usb/core/message.c b/drivers/usb/core/message.c
> index 935ee98e049f65..9e45732dc1d1d1 100644
> --- a/drivers/usb/core/message.c
> +++ b/drivers/usb/core/message.c
> @@ -1954,13 +1954,6 @@ int usb_set_configuration(struct usb_device *dev, int configuration)
>  		intf->dev.bus = &usb_bus_type;
>  		intf->dev.type = &usb_if_device_type;
>  		intf->dev.groups = usb_interface_groups;
> -		/*
> -		 * Please refer to usb_alloc_dev() to see why we set
> -		 * dma_mask and dma_range_map.
> -		 */
> -		intf->dev.dma_mask = dev->dev.dma_mask;
> -		if (dma_direct_copy_range_map(&intf->dev, &dev->dev))
> -			dev_err(&dev->dev, "failed to copy DMA map\n");
>  		INIT_WORK(&intf->reset_ws, __usb_queue_reset_device);
>  		intf->minor = -1;
>  		device_initialize(&intf->dev);
> diff --git a/drivers/usb/core/usb.c b/drivers/usb/core/usb.c
> index 23d451f6894d70..9b4ac4415f1a47 100644
> --- a/drivers/usb/core/usb.c
> +++ b/drivers/usb/core/usb.c
> @@ -599,19 +599,6 @@ struct usb_device *usb_alloc_dev(struct usb_device *parent,
>  	dev->dev.bus = &usb_bus_type;
>  	dev->dev.type = &usb_device_type;
>  	dev->dev.groups = usb_device_groups;
> -	/*
> -	 * Fake a dma_mask/offset for the USB device:
> -	 * We cannot really use the dma-mapping API (dma_alloc_* and
> -	 * dma_map_*) for USB devices but instead need to use
> -	 * usb_alloc_coherent and pass data in 'urb's, but some subsystems
> -	 * manually look into the mask/offset pair to determine whether
> -	 * they need bounce buffers.
> -	 * Note: calling dma_set_mask() on a USB device would set the
> -	 * mask for the entire HCD, so don't do that.
> -	 */
> -	dev->dev.dma_mask = bus->sysdev->dma_mask;
> -	if (dma_direct_copy_range_map(&dev->dev, bus->sysdev))
> -		dev_err(&dev->dev, "failed to copy DMA map\n");
>  	set_dev_node(&dev->dev, dev_to_node(bus->sysdev));
>  	dev->state = USB_STATE_ATTACHED;
>  	dev->lpm_disable_count = 1;
> diff --git a/kernel/dma/direct.c b/kernel/dma/direct.c
> index fc815f7375e282..3af257571a3b42 100644
> --- a/kernel/dma/direct.c
> +++ b/kernel/dma/direct.c
> @@ -552,4 +552,3 @@ int dma_direct_copy_range_map(struct device *to, struct device *from)
>  	to->dma_range_map = new_map;
>  	return 0;
>  }
> -EXPORT_SYMBOL_GPL(dma_direct_copy_range_map);

If you think this is safe to do, great, but for some reason I thought
host controllers wanted this information, and that the scsi layer was
the offending layer that also wanted this type of thing.  But it's been
a really long time so I don't remember for sure, sorry.

thanks,

greg k-h