On Fri, Oct 28, 2022 at 11:37:18AM +0200, Greg Kroah-Hartman wrote:
> On Thu, Oct 27, 2022 at 11:29:48PM +0100, Catalin Marinas wrote:
> > On Wed, Oct 26, 2022 at 10:46:46AM -0700, Linus Torvalds wrote:
> > > I think we should just stop bending over backwards over this, and say
> > > "if your DMA isn't coherent, it's on your driver to mark its
> > > allocations".
> > [...]
> > > That hardware may then be one of the one-off strange cases, but those
> > > people with their masochistic tendencies can take the pain of "oh, now
> > > I need to mark my broken driver with dma_alloc()".
> >
> > The driver is not necessarily broken. The same small kmalloc() in a USB
> > driver can work fine on a fully coherent platform but if that chip ends
> > up on a SoC that doesn't support coherent DMA, it needs bigger kmalloc()
> > alignment. The driver could check if it's coherent but that's more of an
> > arch detail that the driver shouldn't care about. If we define a new API
> > like dma_alloc() and drivers don't use it, that's when we can claim they
> > are broken.
> >
> > A further optimisation would be for dma_alloc() to take a struct device
> > pointer and check dev_is_dma_coherent() before deciding to align the
> > size, though this doesn't work when the allocation place cannot tell the
> > destination device (e.g. alloc_skb(), though these buffers are
> > cacheline-aligned already).
> >
> > Reading up on coccinelle to see if I can make this transition easier. If
> > not, I'll probably go back to bouncing.
>
> bouncing?
>
> sparse is your friend here, here's a tiny patch that if you apply and
> then build the kernel with sparse will show up all the USB driver
> changes that are needed. (note, sample code only, does not fully work
> yet as there are no .c changes made).
>
> I suggest we add something like this now, work on fixing up all of the
> drivers for 6.2-rc1, and then you can add the backend allocator changes
> after that. A few rounds of 'make allmodconfig' will show us the places
> needing to be fixed up and 0-day will help out with that as well.
>
> Yes it's a lot, but it gives us a fighting chance to do the right thing
> going forward with regards to knowing what "type" of memory needs to be
> allocated.
And here's actually the patch...
diff --git a/include/linux/compiler_types.h b/include/linux/compiler_types.h
index eb0466236661..dbc8e013cdaf 100644
--- a/include/linux/compiler_types.h
+++ b/include/linux/compiler_types.h
@@ -23,6 +23,7 @@
# define __iomem __attribute__((noderef, address_space(__iomem)))
# define __percpu __attribute__((noderef, address_space(__percpu)))
# define __rcu __attribute__((noderef, address_space(__rcu)))
+# define __dma __attribute__((noderef, address_space(__dma)))
static inline void __chk_user_ptr(const volatile void __user *ptr) { }
static inline void __chk_io_ptr(const volatile void __iomem *ptr) { }
/* context/locking */
@@ -50,6 +51,7 @@ static inline void __chk_io_ptr(const volatile void __iomem *ptr) { }
# define __iomem
# define __percpu BTF_TYPE_TAG(percpu)
# define __rcu
+# define __dma
# define __chk_user_ptr(x) (void)0
# define __chk_io_ptr(x) (void)0
/* context/locking */
diff --git a/include/linux/usb.h b/include/linux/usb.h
index 9ff1ad4dfad1..5f847c921802 100644
--- a/include/linux/usb.h
+++ b/include/linux/usb.h
@@ -1576,7 +1576,7 @@ struct urb {
unsigned int stream_id; /* (in) stream ID */
int status; /* (return) non-ISO status */
unsigned int transfer_flags; /* (in) URB_SHORT_NOT_OK | ...*/
- void *transfer_buffer; /* (in) associated data buffer */
+ void __dma *transfer_buffer; /* (in) associated data buffer */
dma_addr_t transfer_dma; /* (in) dma addr for transfer_buffer */
struct scatterlist *sg; /* (in) scatter gather buffer list */
int num_mapped_sgs; /* (internal) mapped sg entries */
@@ -1616,7 +1616,7 @@ static inline void usb_fill_control_urb(struct urb *urb,
struct usb_device *dev,
unsigned int pipe,
unsigned char *setup_packet,
- void *transfer_buffer,
+ void __dma *transfer_buffer,
int buffer_length,
usb_complete_t complete_fn,
void *context)
@@ -1646,7 +1646,7 @@ static inline void usb_fill_control_urb(struct urb *urb,
static inline void usb_fill_bulk_urb(struct urb *urb,
struct usb_device *dev,
unsigned int pipe,
- void *transfer_buffer,
+ void __dma *transfer_buffer,
int buffer_length,
usb_complete_t complete_fn,
void *context)
@@ -1687,7 +1687,7 @@ static inline void usb_fill_bulk_urb(struct urb *urb,
static inline void usb_fill_int_urb(struct urb *urb,
struct usb_device *dev,
unsigned int pipe,
- void *transfer_buffer,
+ void __dma *transfer_buffer,
int buffer_length,
usb_complete_t complete_fn,
void *context,
@@ -1766,10 +1766,10 @@ static inline int usb_urb_dir_out(struct urb *urb)
int usb_pipe_type_check(struct usb_device *dev, unsigned int pipe);
int usb_urb_ep_type_check(const struct urb *urb);
-void *usb_alloc_coherent(struct usb_device *dev, size_t size,
+void __dma *usb_alloc_coherent(struct usb_device *dev, size_t size,
gfp_t mem_flags, dma_addr_t *dma);
void usb_free_coherent(struct usb_device *dev, size_t size,
- void *addr, dma_addr_t dma);
+ void __dma *addr, dma_addr_t dma);
#if 0
struct urb *usb_buffer_map(struct urb *urb);
