Re: [PATCH v2 2/2] treewide: Add the __GFP_PACKED flag to several non-DMA kmalloc() allocations

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

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);