[PATCH] kfifo: don't include dma-mapping.h in kfifo.h

[PATCH] kfifo: don't include dma-mapping.h in kfifo.h

[Christoph Hellwig]

Nothing in kfifo.h needs dma-mapping.h.  Drop the include to
reduce include bloat.

Fixes: d52b761e4b1a ("kfifo: add kfifo_dma_out_prepare_mapped()")
Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 drivers/mailbox/omap-mailbox.c | 1 +
 include/linux/kfifo.h          | 1 -
 samples/kfifo/dma-example.c    | 1 +
 3 files changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/mailbox/omap-mailbox.c b/drivers/mailbox/omap-mailbox.c
index 6797770474a55d..680243751d625f 100644
--- a/drivers/mailbox/omap-mailbox.c
+++ b/drivers/mailbox/omap-mailbox.c
@@ -15,6 +15,7 @@
 #include <linux/slab.h>
 #include <linux/kfifo.h>
 #include <linux/err.h>
+#include <linux/io.h>
 #include <linux/module.h>
 #include <linux/of.h>
 #include <linux/platform_device.h>
diff --git a/include/linux/kfifo.h b/include/linux/kfifo.h
index 564868bdce898b..fd743d4c4b4bdc 100644
--- a/include/linux/kfifo.h
+++ b/include/linux/kfifo.h
@@ -37,7 +37,6 @@
  */
 
 #include <linux/array_size.h>
-#include <linux/dma-mapping.h>
 #include <linux/spinlock.h>
 #include <linux/stddef.h>
 #include <linux/types.h>
diff --git a/samples/kfifo/dma-example.c b/samples/kfifo/dma-example.c
index 48df719dac8c6d..8076ac410161a3 100644
--- a/samples/kfifo/dma-example.c
+++ b/samples/kfifo/dma-example.c
@@ -9,6 +9,7 @@
 #include <linux/kfifo.h>
 #include <linux/module.h>
 #include <linux/scatterlist.h>
+#include <linux/dma-mapping.h>
 
 /*
  * This module shows how to handle fifo dma operations.
-- 
2.45.2

Re: [PATCH] kfifo: don't include dma-mapping.h in kfifo.h

[Jiri Slaby]

On 14. 10. 24, 16:46, Christoph Hellwig wrote:
> Nothing in kfifo.h needs dma-mapping.h.  Drop the include to
> reduce include bloat.

Except DMA_MAPPING_ERROR.

The header should stay self-contained.

-- 
js
suse labs

Re: [PATCH] kfifo: don't include dma-mapping.h in kfifo.h

[Christoph Hellwig]

On Tue, Oct 15, 2024 at 09:38:24AM +0200, Jiri Slaby wrote:
> On 14. 10. 24, 16:46, Christoph Hellwig wrote:
>> Nothing in kfifo.h needs dma-mapping.h.  Drop the include to
>> reduce include bloat.
>
> Except DMA_MAPPING_ERROR.

DMA_MAPPING_ERROR is never used by kfifo.h itself.  It is used
by user of the header that instanciate one of the macros that use
it.

> The header should stay self-contained.

It does with this patch.  You can include it as the only header
in a source file and will work fine.  I've actually tried that.

>
> -- 
> js
> suse labs
---end quoted text---

Re: [PATCH] kfifo: don't include dma-mapping.h in kfifo.h

[Jiri Slaby]

On 15. 10. 24, 9:40, Christoph Hellwig wrote:
> On Tue, Oct 15, 2024 at 09:38:24AM +0200, Jiri Slaby wrote:
>> On 14. 10. 24, 16:46, Christoph Hellwig wrote:
>>> Nothing in kfifo.h needs dma-mapping.h.  Drop the include to
>>> reduce include bloat.
>>
>> Except DMA_MAPPING_ERROR.
> 
> DMA_MAPPING_ERROR is never used by kfifo.h itself.  It is used
> by user of the header that instanciate one of the macros that use
> it.

Well, I don't understand. Looking at:
#define kfifo_dma_in_prepare(fifo, sgl, nents, len) \
         kfifo_dma_in_prepare_mapped(fifo, sgl, nents, len, 
DMA_MAPPING_ERROR)

You'd have to include dma-mapping.h if you used this macro. Even though 
you do not explicitly use any other def from the dma header.

I know one will very likely include dma-mapping.h when using the macro. 
But that might not be a rule.

>> The header should stay self-contained.
> 
> It does with this patch.  You can include it as the only header
> in a source file and will work fine.  I've actually tried that.

Well, this is not a definition of self-containment. If you use every 
macro from a header and it does not need any other include, then it is 
self-contained.

-- 
js
suse labs

Re: [PATCH] kfifo: don't include dma-mapping.h in kfifo.h

[Christoph Hellwig]

On Tue, Oct 15, 2024 at 09:53:52AM +0200, Jiri Slaby wrote:
>> DMA_MAPPING_ERROR is never used by kfifo.h itself.  It is used
>> by user of the header that instanciate one of the macros that use
>> it.
>
> Well, I don't understand. Looking at:
> #define kfifo_dma_in_prepare(fifo, sgl, nents, len) \
>         kfifo_dma_in_prepare_mapped(fifo, sgl, nents, len, 
> DMA_MAPPING_ERROR)
>
> You'd have to include dma-mapping.h if you used this macro.

Yes, obviously.

> Even though you 
> do not explicitly use any other def from the dma header.

Sure.

> Well, this is not a definition of self-containment.

Yes, it is the exact definition of it.

> If you use every macro 
> from a header and it does not need any other include, then it is 
> self-contained.

No, that goes way beyond the self containedness.  In fact these days
the main reason to use macros is exactly to avoid these kinds of
dependencies.

Re: [PATCH] kfifo: don't include dma-mapping.h in kfifo.h

[Jiri Slaby]

On 15. 10. 24, 9:56, Christoph Hellwig wrote:
> On Tue, Oct 15, 2024 at 09:53:52AM +0200, Jiri Slaby wrote:
>>> DMA_MAPPING_ERROR is never used by kfifo.h itself.  It is used
>>> by user of the header that instanciate one of the macros that use
>>> it.
>>
>> Well, I don't understand. Looking at:
>> #define kfifo_dma_in_prepare(fifo, sgl, nents, len) \
>>          kfifo_dma_in_prepare_mapped(fifo, sgl, nents, len,
>> DMA_MAPPING_ERROR)
>>
>> You'd have to include dma-mapping.h if you used this macro.
> 
> Yes, obviously.
> 
>> Even though you
>> do not explicitly use any other def from the dma header.
> 
> Sure.
> 
>> Well, this is not a definition of self-containment.
> 
> Yes, it is the exact definition of it.

Then it depends on which one, apparently.

So I disagree, but I don't mind either (meaning: I don't oppose to the 
patch any longer either).

>> If you use every macro
>> from a header and it does not need any other include, then it is
>> self-contained.
> 
> No, that goes way beyond the self containedness.  In fact these days
> the main reason to use macros is exactly to avoid these kinds of
> dependencies.


-- 
js
suse labs

Re: [PATCH] kfifo: don't include dma-mapping.h in kfifo.h

[Jiri Slaby]

On 14. 10. 24, 16:46, Christoph Hellwig wrote:
> Nothing in kfifo.h needs dma-mapping.h.  Drop the include to
> reduce include bloat.
> 
> Fixes: d52b761e4b1a ("kfifo: add kfifo_dma_out_prepare_mapped()")
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>   drivers/mailbox/omap-mailbox.c | 1 +
>   include/linux/kfifo.h          | 1 -
>   samples/kfifo/dma-example.c    | 1 +
>   3 files changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/mailbox/omap-mailbox.c b/drivers/mailbox/omap-mailbox.c
> index 6797770474a55d..680243751d625f 100644
> --- a/drivers/mailbox/omap-mailbox.c
> +++ b/drivers/mailbox/omap-mailbox.c
> @@ -15,6 +15,7 @@
>   #include <linux/slab.h>
>   #include <linux/kfifo.h>
>   #include <linux/err.h>
> +#include <linux/io.h>

Oh, I've just noticed this. Why io.h?

-- 
js
suse labs

Re: [PATCH] kfifo: don't include dma-mapping.h in kfifo.h

[Christoph Hellwig]

On Tue, Oct 15, 2024 at 09:47:31AM +0200, Jiri Slaby wrote:
>> diff --git a/drivers/mailbox/omap-mailbox.c b/drivers/mailbox/omap-mailbox.c
>> index 6797770474a55d..680243751d625f 100644
>> --- a/drivers/mailbox/omap-mailbox.c
>> +++ b/drivers/mailbox/omap-mailbox.c
>> @@ -15,6 +15,7 @@
>>   #include <linux/slab.h>
>>   #include <linux/kfifo.h>
>>   #include <linux/err.h>
>> +#include <linux/io.h>
>
> Oh, I've just noticed this. Why io.h?

Because it would not comple with out it.  To be specific: asm/io.h is
where __raw_readl and __raw_writel are defined, and we generally prefer
the Linux over the asm headers.

>
> -- 
> js
> suse labs
---end quoted text---

Re: [PATCH] kfifo: don't include dma-mapping.h in kfifo.h

[Jiri Slaby]

On 15. 10. 24, 9:50, Christoph Hellwig wrote:
> On Tue, Oct 15, 2024 at 09:47:31AM +0200, Jiri Slaby wrote:
>>> diff --git a/drivers/mailbox/omap-mailbox.c b/drivers/mailbox/omap-mailbox.c
>>> index 6797770474a55d..680243751d625f 100644
>>> --- a/drivers/mailbox/omap-mailbox.c
>>> +++ b/drivers/mailbox/omap-mailbox.c
>>> @@ -15,6 +15,7 @@
>>>    #include <linux/slab.h>
>>>    #include <linux/kfifo.h>
>>>    #include <linux/err.h>
>>> +#include <linux/io.h>
>>
>> Oh, I've just noticed this. Why io.h?
> 
> Because it would not comple with out it.  To be specific: asm/io.h is
> where __raw_readl and __raw_writel are defined, and we generally prefer
> the Linux over the asm headers.

OK, but how does this relate to "kfifo: don't include dma-mapping.h in 
kfifo.h"?

-- 
js
suse labs

Re: [PATCH] kfifo: don't include dma-mapping.h in kfifo.h

[Christoph Hellwig]

On Tue, Oct 15, 2024 at 09:55:06AM +0200, Jiri Slaby wrote:
>> Because it would not comple with out it.  To be specific: asm/io.h is
>> where __raw_readl and __raw_writel are defined, and we generally prefer
>> the Linux over the asm headers.
>
> OK, but how does this relate to "kfifo: don't include dma-mapping.h in 
> kfifo.h"?

Because before that it get io.h pulled in through our batshit crazy
header depency chain. 

Re: [PATCH] kfifo: don't include dma-mapping.h in kfifo.h

[Jiri Slaby]

On 15. 10. 24, 9:57, Christoph Hellwig wrote:
> On Tue, Oct 15, 2024 at 09:55:06AM +0200, Jiri Slaby wrote:
>>> Because it would not comple with out it.  To be specific: asm/io.h is
>>> where __raw_readl and __raw_writel are defined, and we generally prefer
>>> the Linux over the asm headers.
>>
>> OK, but how does this relate to "kfifo: don't include dma-mapping.h in
>> kfifo.h"?
> 
> Because before that it get io.h pulled in through our batshit crazy
> header depency chain.

Good fix then. But you somehow forgot to mention this in the commit message.

-- 
js
suse labs