[PATCH] vfio_pci: Add local source directory as include

[PATCH] vfio_pci: Add local source directory as include

[Laura Abbott]

Commit 7f92891778df ("vfio_pci: Add NVIDIA GV100GL [Tesla V100 SXM2]
subdriver") introduced a trace.h file in the local directory but
missed adding the local include path, resulting in compilation
failures with tracepoints:

In file included from drivers/vfio/pci/trace.h:102,
                 from drivers/vfio/pci/vfio_pci_nvlink2.c:29:
./include/trace/define_trace.h:89:42: fatal error: ./trace.h: No such file or directory
 #include TRACE_INCLUDE(TRACE_INCLUDE_FILE)

Fix this by adjusting the include path.

Fixes: 7f92891778df ("vfio_pci: Add NVIDIA GV100GL [Tesla V100 SXM2] subdriver")
Signed-off-by: Laura Abbott <labbott@redhat.com>
---
I'd still like to echo my sentiment that this should not be a def_bool.
We hit this error on our internal testing and we couldn't even turn
off the driver until we fixed this.
---
 drivers/vfio/pci/Makefile | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/vfio/pci/Makefile b/drivers/vfio/pci/Makefile
index 9662c063a6b1..08d4676a8495 100644
--- a/drivers/vfio/pci/Makefile
+++ b/drivers/vfio/pci/Makefile
@@ -1,3 +1,4 @@
+ccflags-y                               += -I$(src)
 
 vfio-pci-y := vfio_pci.o vfio_pci_intrs.o vfio_pci_rdwr.o vfio_pci_config.o
 vfio-pci-$(CONFIG_VFIO_PCI_IGD) += vfio_pci_igd.o
-- 
2.20.1

Re: [PATCH] vfio_pci: Add local source directory as include

[Alexey Kardashevskiy]

On 05/01/2019 06:57, Laura Abbott wrote:
> Commit 7f92891778df ("vfio_pci: Add NVIDIA GV100GL [Tesla V100 SXM2]
> subdriver") introduced a trace.h file in the local directory but
> missed adding the local include path, resulting in compilation
> failures with tracepoints:
> 
> In file included from drivers/vfio/pci/trace.h:102,
>                  from drivers/vfio/pci/vfio_pci_nvlink2.c:29:
> ./include/trace/define_trace.h:89:42: fatal error: ./trace.h: No such file or directory
>  #include TRACE_INCLUDE(TRACE_INCLUDE_FILE)
> 
> Fix this by adjusting the include path.
> 
> Fixes: 7f92891778df ("vfio_pci: Add NVIDIA GV100GL [Tesla V100 SXM2] subdriver")
> Signed-off-by: Laura Abbott <labbott@redhat.com>



Reviewed-by: Alexey Kardashevskiy <aik@ozlabs.ru>



> ---
> I'd still like to echo my sentiment that this should not be a def_bool.
> We hit this error on our internal testing and we couldn't even turn
> off the driver until we fixed this.
> ---
>  drivers/vfio/pci/Makefile | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/vfio/pci/Makefile b/drivers/vfio/pci/Makefile
> index 9662c063a6b1..08d4676a8495 100644
> --- a/drivers/vfio/pci/Makefile
> +++ b/drivers/vfio/pci/Makefile
> @@ -1,3 +1,4 @@
> +ccflags-y                               += -I$(src)
>  
>  vfio-pci-y := vfio_pci.o vfio_pci_intrs.o vfio_pci_rdwr.o vfio_pci_config.o
>  vfio-pci-$(CONFIG_VFIO_PCI_IGD) += vfio_pci_igd.o
> 

-- 
Alexey

Re: [PATCH] vfio_pci: Add local source directory as include

[Michael Ellerman]

Laura Abbott <labbott@redhat.com> writes:
> Commit 7f92891778df ("vfio_pci: Add NVIDIA GV100GL [Tesla V100 SXM2]
> subdriver") introduced a trace.h file in the local directory but
> missed adding the local include path, resulting in compilation
> failures with tracepoints:
>
> In file included from drivers/vfio/pci/trace.h:102,
>                  from drivers/vfio/pci/vfio_pci_nvlink2.c:29:
> ./include/trace/define_trace.h:89:42: fatal error: ./trace.h: No such file or directory
>  #include TRACE_INCLUDE(TRACE_INCLUDE_FILE)
>
> Fix this by adjusting the include path.
>
> Fixes: 7f92891778df ("vfio_pci: Add NVIDIA GV100GL [Tesla V100 SXM2] subdriver")
> Signed-off-by: Laura Abbott <labbott@redhat.com>
> ---
> I'd still like to echo my sentiment that this should not be a def_bool.
> We hit this error on our internal testing and we couldn't even turn
> off the driver until we fixed this.

I assume there's some reason you can't commit a patch to your tree to
change it to bool, or turn it off entirely? That would change the SHA
which is perhaps reason enough.

In general we have far too many options and most of them never get
turned off (or on), so it just creates testing/bug surface for not much
benefit. This is one that will probably be turned on in all distro
kernels for example.

But I have no real objection to making it user configurable.


Alex I assume you'll merge this fix via the vfio tree?

cheers

> diff --git a/drivers/vfio/pci/Makefile b/drivers/vfio/pci/Makefile
> index 9662c063a6b1..08d4676a8495 100644
> --- a/drivers/vfio/pci/Makefile
> +++ b/drivers/vfio/pci/Makefile
> @@ -1,3 +1,4 @@
> +ccflags-y                               += -I$(src)
>  
>  vfio-pci-y := vfio_pci.o vfio_pci_intrs.o vfio_pci_rdwr.o vfio_pci_config.o
>  vfio-pci-$(CONFIG_VFIO_PCI_IGD) += vfio_pci_igd.o
> -- 
> 2.20.1

Re: [PATCH] vfio_pci: Add local source directory as include

[Masahiro Yamada]

On Mon, Jan 7, 2019 at 6:18 PM Michael Ellerman <mpe@ellerman.id.au> wrote:
>
> Laura Abbott <labbott@redhat.com> writes:
> > Commit 7f92891778df ("vfio_pci: Add NVIDIA GV100GL [Tesla V100 SXM2]
> > subdriver") introduced a trace.h file in the local directory but
> > missed adding the local include path, resulting in compilation
> > failures with tracepoints:
> >
> > In file included from drivers/vfio/pci/trace.h:102,
> >                  from drivers/vfio/pci/vfio_pci_nvlink2.c:29:
> > ./include/trace/define_trace.h:89:42: fatal error: ./trace.h: No such file or directory
> >  #include TRACE_INCLUDE(TRACE_INCLUDE_FILE)
> >
> > Fix this by adjusting the include path.
> >
> > Fixes: 7f92891778df ("vfio_pci: Add NVIDIA GV100GL [Tesla V100 SXM2] subdriver")
> > Signed-off-by: Laura Abbott <labbott@redhat.com>
> > ---
> > I'd still like to echo my sentiment that this should not be a def_bool.
> > We hit this error on our internal testing and we couldn't even turn
> > off the driver until we fixed this.
>
> I assume there's some reason you can't commit a patch to your tree to
> change it to bool, or turn it off entirely? That would change the SHA
> which is perhaps reason enough.
>
> In general we have far too many options and most of them never get
> turned off (or on), so it just creates testing/bug surface for not much
> benefit. This is one that will probably be turned on in all distro
> kernels for example.
>
> But I have no real objection to making it user configurable.
>
>
> Alex I assume you'll merge this fix via the vfio tree?
>
> cheers
>
> > diff --git a/drivers/vfio/pci/Makefile b/drivers/vfio/pci/Makefile
> > index 9662c063a6b1..08d4676a8495 100644
> > --- a/drivers/vfio/pci/Makefile
> > +++ b/drivers/vfio/pci/Makefile
> > @@ -1,3 +1,4 @@
> > +ccflags-y                               += -I$(src)
> >
> >  vfio-pci-y := vfio_pci.o vfio_pci_intrs.o vfio_pci_rdwr.o vfio_pci_config.o
> >  vfio-pci-$(CONFIG_VFIO_PCI_IGD) += vfio_pci_igd.o
> > --
> > 2.20.1


Hi.

If I correctly understand the usage of TRACE_INCLUDE_PATH,
the correct fix should be like follows:


diff --git a/drivers/vfio/pci/trace.h b/drivers/vfio/pci/trace.h
index 228ccdb..4d13e51 100644
--- a/drivers/vfio/pci/trace.h
+++ b/drivers/vfio/pci/trace.h
@@ -94,7 +94,7 @@ TRACE_EVENT(vfio_pci_npu2_mmap,
 #endif /* _TRACE_VFIO_PCI_H */

 #undef TRACE_INCLUDE_PATH
-#define TRACE_INCLUDE_PATH .
+#define TRACE_INCLUDE_PATH ../../drivers/vfio/pci
 #undef TRACE_INCLUDE_FILE
 #define TRACE_INCLUDE_FILE trace









-- 
Best Regards
Masahiro Yamada

Re: [PATCH] vfio_pci: Add local source directory as include

[Cornelia Huck]

On Mon, 7 Jan 2019 19:12:10 +0900
Masahiro Yamada <yamada.masahiro@socionext.com> wrote:

> On Mon, Jan 7, 2019 at 6:18 PM Michael Ellerman <mpe@ellerman.id.au> wrote:
> >
> > Laura Abbott <labbott@redhat.com> writes:  
> > > Commit 7f92891778df ("vfio_pci: Add NVIDIA GV100GL [Tesla V100 SXM2]
> > > subdriver") introduced a trace.h file in the local directory but
> > > missed adding the local include path, resulting in compilation
> > > failures with tracepoints:
> > >
> > > In file included from drivers/vfio/pci/trace.h:102,
> > >                  from drivers/vfio/pci/vfio_pci_nvlink2.c:29:
> > > ./include/trace/define_trace.h:89:42: fatal error: ./trace.h: No such file or directory
> > >  #include TRACE_INCLUDE(TRACE_INCLUDE_FILE)
> > >
> > > Fix this by adjusting the include path.
> > >
> > > Fixes: 7f92891778df ("vfio_pci: Add NVIDIA GV100GL [Tesla V100 SXM2] subdriver")
> > > Signed-off-by: Laura Abbott <labbott@redhat.com>

(...)

> > Alex I assume you'll merge this fix via the vfio tree?
> >
> > cheers
> >  
> > > diff --git a/drivers/vfio/pci/Makefile b/drivers/vfio/pci/Makefile
> > > index 9662c063a6b1..08d4676a8495 100644
> > > --- a/drivers/vfio/pci/Makefile
> > > +++ b/drivers/vfio/pci/Makefile
> > > @@ -1,3 +1,4 @@
> > > +ccflags-y                               += -I$(src)
> > >
> > >  vfio-pci-y := vfio_pci.o vfio_pci_intrs.o vfio_pci_rdwr.o vfio_pci_config.o
> > >  vfio-pci-$(CONFIG_VFIO_PCI_IGD) += vfio_pci_igd.o
> > > --
> > > 2.20.1  
> 
> 
> Hi.
> 
> If I correctly understand the usage of TRACE_INCLUDE_PATH,
> the correct fix should be like follows:
> 
> 
> diff --git a/drivers/vfio/pci/trace.h b/drivers/vfio/pci/trace.h
> index 228ccdb..4d13e51 100644
> --- a/drivers/vfio/pci/trace.h
> +++ b/drivers/vfio/pci/trace.h
> @@ -94,7 +94,7 @@ TRACE_EVENT(vfio_pci_npu2_mmap,
>  #endif /* _TRACE_VFIO_PCI_H */
> 
>  #undef TRACE_INCLUDE_PATH
> -#define TRACE_INCLUDE_PATH .
> +#define TRACE_INCLUDE_PATH ../../drivers/vfio/pci
>  #undef TRACE_INCLUDE_FILE
>  #define TRACE_INCLUDE_FILE trace

Going from the comments in samples/trace_events/trace-events-sample.h,
I think both approaches are possible, and I see both used in various
places.

Personally, I'd prefer Laura's patch, as it doesn't involve hardcoding
a path.

Re: [PATCH] vfio_pci: Add local source directory as include

[Masahiro Yamada]

On Mon, Jan 7, 2019 at 8:09 PM Cornelia Huck <cohuck@redhat.com> wrote:
>
> On Mon, 7 Jan 2019 19:12:10 +0900
> Masahiro Yamada <yamada.masahiro@socionext.com> wrote:
>
> > On Mon, Jan 7, 2019 at 6:18 PM Michael Ellerman <mpe@ellerman.id.au> wrote:
> > >
> > > Laura Abbott <labbott@redhat.com> writes:
> > > > Commit 7f92891778df ("vfio_pci: Add NVIDIA GV100GL [Tesla V100 SXM2]
> > > > subdriver") introduced a trace.h file in the local directory but
> > > > missed adding the local include path, resulting in compilation
> > > > failures with tracepoints:
> > > >
> > > > In file included from drivers/vfio/pci/trace.h:102,
> > > >                  from drivers/vfio/pci/vfio_pci_nvlink2.c:29:
> > > > ./include/trace/define_trace.h:89:42: fatal error: ./trace.h: No such file or directory
> > > >  #include TRACE_INCLUDE(TRACE_INCLUDE_FILE)
> > > >
> > > > Fix this by adjusting the include path.
> > > >
> > > > Fixes: 7f92891778df ("vfio_pci: Add NVIDIA GV100GL [Tesla V100 SXM2] subdriver")
> > > > Signed-off-by: Laura Abbott <labbott@redhat.com>
>
> (...)
>
> > > Alex I assume you'll merge this fix via the vfio tree?
> > >
> > > cheers
> > >
> > > > diff --git a/drivers/vfio/pci/Makefile b/drivers/vfio/pci/Makefile
> > > > index 9662c063a6b1..08d4676a8495 100644
> > > > --- a/drivers/vfio/pci/Makefile
> > > > +++ b/drivers/vfio/pci/Makefile
> > > > @@ -1,3 +1,4 @@
> > > > +ccflags-y                               += -I$(src)
> > > >
> > > >  vfio-pci-y := vfio_pci.o vfio_pci_intrs.o vfio_pci_rdwr.o vfio_pci_config.o
> > > >  vfio-pci-$(CONFIG_VFIO_PCI_IGD) += vfio_pci_igd.o
> > > > --
> > > > 2.20.1
> >
> >
> > Hi.
> >
> > If I correctly understand the usage of TRACE_INCLUDE_PATH,
> > the correct fix should be like follows:
> >
> >
> > diff --git a/drivers/vfio/pci/trace.h b/drivers/vfio/pci/trace.h
> > index 228ccdb..4d13e51 100644
> > --- a/drivers/vfio/pci/trace.h
> > +++ b/drivers/vfio/pci/trace.h
> > @@ -94,7 +94,7 @@ TRACE_EVENT(vfio_pci_npu2_mmap,
> >  #endif /* _TRACE_VFIO_PCI_H */
> >
> >  #undef TRACE_INCLUDE_PATH
> > -#define TRACE_INCLUDE_PATH .
> > +#define TRACE_INCLUDE_PATH ../../drivers/vfio/pci
> >  #undef TRACE_INCLUDE_FILE
> >  #define TRACE_INCLUDE_FILE trace
>
> Going from the comments in samples/trace_events/trace-events-sample.h,
> I think both approaches are possible, and I see both used in various
> places.
>
> Personally, I'd prefer Laura's patch, as it doesn't involve hardcoding
> a path.



ccflags-y += -I$(src)
would add the header search path for all files in drivers/vfio/pci/
whereas only the drivers/vfio/pci/vfio_pci_nvlink2.c needs it.



CFLAGS_vfio_pci_nvlink2.o += -I$(src)
is a bit better.
However, it is not obvious why this extra header search path is needed
until you find vfio_pci_nvlink2.c including trace.h



#define TRACE_INCLUDE_PATH ../../drivers/vfio/pci
clarifies the intention because the related code is all placed in trace.h



From the comment in include/trace/define_trace.h
TRACE_INCLUDE_PATH is relative to include/trace/define_trace.h




-- 
Best Regards
Masahiro Yamada

Re: [PATCH] vfio_pci: Add local source directory as include

[Alex Williamson]

On Mon, 7 Jan 2019 20:39:19 +0900
Masahiro Yamada <yamada.masahiro@socionext.com> wrote:

> On Mon, Jan 7, 2019 at 8:09 PM Cornelia Huck <cohuck@redhat.com> wrote:
> >
> > On Mon, 7 Jan 2019 19:12:10 +0900
> > Masahiro Yamada <yamada.masahiro@socionext.com> wrote:
> >  
> > > On Mon, Jan 7, 2019 at 6:18 PM Michael Ellerman <mpe@ellerman.id.au> wrote:  
> > > >
> > > > Laura Abbott <labbott@redhat.com> writes:  
> > > > > Commit 7f92891778df ("vfio_pci: Add NVIDIA GV100GL [Tesla V100 SXM2]
> > > > > subdriver") introduced a trace.h file in the local directory but
> > > > > missed adding the local include path, resulting in compilation
> > > > > failures with tracepoints:
> > > > >
> > > > > In file included from drivers/vfio/pci/trace.h:102,
> > > > >                  from drivers/vfio/pci/vfio_pci_nvlink2.c:29:
> > > > > ./include/trace/define_trace.h:89:42: fatal error: ./trace.h: No such file or directory
> > > > >  #include TRACE_INCLUDE(TRACE_INCLUDE_FILE)
> > > > >
> > > > > Fix this by adjusting the include path.
> > > > >
> > > > > Fixes: 7f92891778df ("vfio_pci: Add NVIDIA GV100GL [Tesla V100 SXM2] subdriver")
> > > > > Signed-off-by: Laura Abbott <labbott@redhat.com>  
> >
> > (...)
> >  
> > > > Alex I assume you'll merge this fix via the vfio tree?
> > > >
> > > > cheers
> > > >  
> > > > > diff --git a/drivers/vfio/pci/Makefile b/drivers/vfio/pci/Makefile
> > > > > index 9662c063a6b1..08d4676a8495 100644
> > > > > --- a/drivers/vfio/pci/Makefile
> > > > > +++ b/drivers/vfio/pci/Makefile
> > > > > @@ -1,3 +1,4 @@
> > > > > +ccflags-y                               += -I$(src)
> > > > >
> > > > >  vfio-pci-y := vfio_pci.o vfio_pci_intrs.o vfio_pci_rdwr.o vfio_pci_config.o
> > > > >  vfio-pci-$(CONFIG_VFIO_PCI_IGD) += vfio_pci_igd.o
> > > > > --
> > > > > 2.20.1  
> > >
> > >
> > > Hi.
> > >
> > > If I correctly understand the usage of TRACE_INCLUDE_PATH,
> > > the correct fix should be like follows:
> > >
> > >
> > > diff --git a/drivers/vfio/pci/trace.h b/drivers/vfio/pci/trace.h
> > > index 228ccdb..4d13e51 100644
> > > --- a/drivers/vfio/pci/trace.h
> > > +++ b/drivers/vfio/pci/trace.h
> > > @@ -94,7 +94,7 @@ TRACE_EVENT(vfio_pci_npu2_mmap,
> > >  #endif /* _TRACE_VFIO_PCI_H */
> > >
> > >  #undef TRACE_INCLUDE_PATH
> > > -#define TRACE_INCLUDE_PATH .
> > > +#define TRACE_INCLUDE_PATH ../../drivers/vfio/pci
> > >  #undef TRACE_INCLUDE_FILE
> > >  #define TRACE_INCLUDE_FILE trace  
> >
> > Going from the comments in samples/trace_events/trace-events-sample.h,
> > I think both approaches are possible, and I see both used in various
> > places.
> >
> > Personally, I'd prefer Laura's patch, as it doesn't involve hardcoding
> > a path.

Numbering options for clarity:

1)
> ccflags-y += -I$(src)
> would add the header search path for all files in drivers/vfio/pci/
> whereas only the drivers/vfio/pci/vfio_pci_nvlink2.c needs it.
> 

2)
> CFLAGS_vfio_pci_nvlink2.o += -I$(src)
> is a bit better.
> However, it is not obvious why this extra header search path is needed
> until you find vfio_pci_nvlink2.c including trace.h
> 

3)
> #define TRACE_INCLUDE_PATH ../../drivers/vfio/pci
> clarifies the intention because the related code is all placed in trace.h
> 
> 
> 
> From the comment in include/trace/define_trace.h
> TRACE_INCLUDE_PATH is relative to include/trace/define_trace.h

In my scan of the tree, the most common solution seems to be 2) as this
is essentially recommended in the sample file.  3) is well represented,
with much fewer examples of 1), though it might depend how liberally
we grep out or examine the use cases.  Choice 1) also seems to be the
most shotgun approach, adding to the search path for all files.  From a
maintenance perspective I agree that 2) seems more error prone,
especially when the build system only catches the error on in-tree
builds, something I rarely do.  Therefore I'm leaning towards option
3).  The hardcoded path here doesn't seem much of an issue relative to
the negatives of the other approaches (how often do we move these
files?) and it keeps the trace support relatively self-contained.  Are
there further arguments for or against these options?  Otherwise who
wants to formally post the TRACE_INCLUDE_PATH version?  Thanks,

Alex

Re: [PATCH] vfio_pci: Add local source directory as include

[Alexey Kardashevskiy]

On 08/01/2019 07:13, Alex Williamson wrote:
> On Mon, 7 Jan 2019 20:39:19 +0900
> Masahiro Yamada <yamada.masahiro@socionext.com> wrote:
> 
>> On Mon, Jan 7, 2019 at 8:09 PM Cornelia Huck <cohuck@redhat.com> wrote:
>>>
>>> On Mon, 7 Jan 2019 19:12:10 +0900
>>> Masahiro Yamada <yamada.masahiro@socionext.com> wrote:
>>>  
>>>> On Mon, Jan 7, 2019 at 6:18 PM Michael Ellerman <mpe@ellerman.id.au> wrote:  
>>>>>
>>>>> Laura Abbott <labbott@redhat.com> writes:  
>>>>>> Commit 7f92891778df ("vfio_pci: Add NVIDIA GV100GL [Tesla V100 SXM2]
>>>>>> subdriver") introduced a trace.h file in the local directory but
>>>>>> missed adding the local include path, resulting in compilation
>>>>>> failures with tracepoints:
>>>>>>
>>>>>> In file included from drivers/vfio/pci/trace.h:102,
>>>>>>                  from drivers/vfio/pci/vfio_pci_nvlink2.c:29:
>>>>>> ./include/trace/define_trace.h:89:42: fatal error: ./trace.h: No such file or directory
>>>>>>  #include TRACE_INCLUDE(TRACE_INCLUDE_FILE)
>>>>>>
>>>>>> Fix this by adjusting the include path.
>>>>>>
>>>>>> Fixes: 7f92891778df ("vfio_pci: Add NVIDIA GV100GL [Tesla V100 SXM2] subdriver")
>>>>>> Signed-off-by: Laura Abbott <labbott@redhat.com>  
>>>
>>> (...)
>>>  
>>>>> Alex I assume you'll merge this fix via the vfio tree?
>>>>>
>>>>> cheers
>>>>>  
>>>>>> diff --git a/drivers/vfio/pci/Makefile b/drivers/vfio/pci/Makefile
>>>>>> index 9662c063a6b1..08d4676a8495 100644
>>>>>> --- a/drivers/vfio/pci/Makefile
>>>>>> +++ b/drivers/vfio/pci/Makefile
>>>>>> @@ -1,3 +1,4 @@
>>>>>> +ccflags-y                               += -I$(src)
>>>>>>
>>>>>>  vfio-pci-y := vfio_pci.o vfio_pci_intrs.o vfio_pci_rdwr.o vfio_pci_config.o
>>>>>>  vfio-pci-$(CONFIG_VFIO_PCI_IGD) += vfio_pci_igd.o
>>>>>> --
>>>>>> 2.20.1  
>>>>
>>>>
>>>> Hi.
>>>>
>>>> If I correctly understand the usage of TRACE_INCLUDE_PATH,
>>>> the correct fix should be like follows:
>>>>
>>>>
>>>> diff --git a/drivers/vfio/pci/trace.h b/drivers/vfio/pci/trace.h
>>>> index 228ccdb..4d13e51 100644
>>>> --- a/drivers/vfio/pci/trace.h
>>>> +++ b/drivers/vfio/pci/trace.h
>>>> @@ -94,7 +94,7 @@ TRACE_EVENT(vfio_pci_npu2_mmap,
>>>>  #endif /* _TRACE_VFIO_PCI_H */
>>>>
>>>>  #undef TRACE_INCLUDE_PATH
>>>> -#define TRACE_INCLUDE_PATH .
>>>> +#define TRACE_INCLUDE_PATH ../../drivers/vfio/pci
>>>>  #undef TRACE_INCLUDE_FILE
>>>>  #define TRACE_INCLUDE_FILE trace  
>>>
>>> Going from the comments in samples/trace_events/trace-events-sample.h,
>>> I think both approaches are possible, and I see both used in various
>>> places.
>>>
>>> Personally, I'd prefer Laura's patch, as it doesn't involve hardcoding
>>> a path.
> 
> Numbering options for clarity:
> 
> 1)
>> ccflags-y += -I$(src)
>> would add the header search path for all files in drivers/vfio/pci/
>> whereas only the drivers/vfio/pci/vfio_pci_nvlink2.c needs it.
>>
> 
> 2)
>> CFLAGS_vfio_pci_nvlink2.o += -I$(src)
>> is a bit better.
>> However, it is not obvious why this extra header search path is needed
>> until you find vfio_pci_nvlink2.c including trace.h
>>
> 
> 3)
>> #define TRACE_INCLUDE_PATH ../../drivers/vfio/pci
>> clarifies the intention because the related code is all placed in trace.h
>>
>>
>>
>> From the comment in include/trace/define_trace.h
>> TRACE_INCLUDE_PATH is relative to include/trace/define_trace.h
> 
> In my scan of the tree, the most common solution seems to be 2) as this
> is essentially recommended in the sample file.  3) is well represented,
> with much fewer examples of 1), though it might depend how liberally
> we grep out or examine the use cases.  Choice 1) also seems to be the
> most shotgun approach, adding to the search path for all files.


The shotgun approach is always used when compiling out of tree which is
what many people do anyway without realizing that there are additional
-I. Why do not we make in-tree behave the same way? Thanks,


>  From a
> maintenance perspective I agree that 2) seems more error prone,
> especially when the build system only catches the error on in-tree
> builds, something I rarely do.  Therefore I'm leaning towards option
> 3).  The hardcoded path here doesn't seem much of an issue relative to
> the negatives of the other approaches (how often do we move these
> files?) and it keeps the trace support relatively self-contained.  Are
> there further arguments for or against these options?  Otherwise who
> wants to formally post the TRACE_INCLUDE_PATH version?  Thanks,

-- 
Alexey

Re: [PATCH] vfio_pci: Add local source directory as include

[Alex Williamson]

On Tue, 8 Jan 2019 10:52:43 +1100
Alexey Kardashevskiy <aik@ozlabs.ru> wrote:

> On 08/01/2019 07:13, Alex Williamson wrote:
> > On Mon, 7 Jan 2019 20:39:19 +0900
> > Masahiro Yamada <yamada.masahiro@socionext.com> wrote:
> >   
> >> On Mon, Jan 7, 2019 at 8:09 PM Cornelia Huck <cohuck@redhat.com> wrote:  
> >>>
> >>> On Mon, 7 Jan 2019 19:12:10 +0900
> >>> Masahiro Yamada <yamada.masahiro@socionext.com> wrote:
> >>>    
> >>>> On Mon, Jan 7, 2019 at 6:18 PM Michael Ellerman <mpe@ellerman.id.au> wrote:    
> >>>>>
> >>>>> Laura Abbott <labbott@redhat.com> writes:    
> >>>>>> Commit 7f92891778df ("vfio_pci: Add NVIDIA GV100GL [Tesla V100 SXM2]
> >>>>>> subdriver") introduced a trace.h file in the local directory but
> >>>>>> missed adding the local include path, resulting in compilation
> >>>>>> failures with tracepoints:
> >>>>>>
> >>>>>> In file included from drivers/vfio/pci/trace.h:102,
> >>>>>>                  from drivers/vfio/pci/vfio_pci_nvlink2.c:29:
> >>>>>> ./include/trace/define_trace.h:89:42: fatal error: ./trace.h: No such file or directory
> >>>>>>  #include TRACE_INCLUDE(TRACE_INCLUDE_FILE)
> >>>>>>
> >>>>>> Fix this by adjusting the include path.
> >>>>>>
> >>>>>> Fixes: 7f92891778df ("vfio_pci: Add NVIDIA GV100GL [Tesla V100 SXM2] subdriver")
> >>>>>> Signed-off-by: Laura Abbott <labbott@redhat.com>    
> >>>
> >>> (...)
> >>>    
> >>>>> Alex I assume you'll merge this fix via the vfio tree?
> >>>>>
> >>>>> cheers
> >>>>>    
> >>>>>> diff --git a/drivers/vfio/pci/Makefile b/drivers/vfio/pci/Makefile
> >>>>>> index 9662c063a6b1..08d4676a8495 100644
> >>>>>> --- a/drivers/vfio/pci/Makefile
> >>>>>> +++ b/drivers/vfio/pci/Makefile
> >>>>>> @@ -1,3 +1,4 @@
> >>>>>> +ccflags-y                               += -I$(src)
> >>>>>>
> >>>>>>  vfio-pci-y := vfio_pci.o vfio_pci_intrs.o vfio_pci_rdwr.o vfio_pci_config.o
> >>>>>>  vfio-pci-$(CONFIG_VFIO_PCI_IGD) += vfio_pci_igd.o
> >>>>>> --
> >>>>>> 2.20.1    
> >>>>
> >>>>
> >>>> Hi.
> >>>>
> >>>> If I correctly understand the usage of TRACE_INCLUDE_PATH,
> >>>> the correct fix should be like follows:
> >>>>
> >>>>
> >>>> diff --git a/drivers/vfio/pci/trace.h b/drivers/vfio/pci/trace.h
> >>>> index 228ccdb..4d13e51 100644
> >>>> --- a/drivers/vfio/pci/trace.h
> >>>> +++ b/drivers/vfio/pci/trace.h
> >>>> @@ -94,7 +94,7 @@ TRACE_EVENT(vfio_pci_npu2_mmap,
> >>>>  #endif /* _TRACE_VFIO_PCI_H */
> >>>>
> >>>>  #undef TRACE_INCLUDE_PATH
> >>>> -#define TRACE_INCLUDE_PATH .
> >>>> +#define TRACE_INCLUDE_PATH ../../drivers/vfio/pci
> >>>>  #undef TRACE_INCLUDE_FILE
> >>>>  #define TRACE_INCLUDE_FILE trace    
> >>>
> >>> Going from the comments in samples/trace_events/trace-events-sample.h,
> >>> I think both approaches are possible, and I see both used in various
> >>> places.
> >>>
> >>> Personally, I'd prefer Laura's patch, as it doesn't involve hardcoding
> >>> a path.  
> > 
> > Numbering options for clarity:
> > 
> > 1)  
> >> ccflags-y += -I$(src)
> >> would add the header search path for all files in drivers/vfio/pci/
> >> whereas only the drivers/vfio/pci/vfio_pci_nvlink2.c needs it.
> >>  
> > 
> > 2)  
> >> CFLAGS_vfio_pci_nvlink2.o += -I$(src)
> >> is a bit better.
> >> However, it is not obvious why this extra header search path is needed
> >> until you find vfio_pci_nvlink2.c including trace.h
> >>  
> > 
> > 3)  
> >> #define TRACE_INCLUDE_PATH ../../drivers/vfio/pci
> >> clarifies the intention because the related code is all placed in trace.h
> >>
> >>
> >>
> >> From the comment in include/trace/define_trace.h
> >> TRACE_INCLUDE_PATH is relative to include/trace/define_trace.h  
> > 
> > In my scan of the tree, the most common solution seems to be 2) as this
> > is essentially recommended in the sample file.  3) is well represented,
> > with much fewer examples of 1), though it might depend how liberally
> > we grep out or examine the use cases.  Choice 1) also seems to be the
> > most shotgun approach, adding to the search path for all files.  
> 
> 
> The shotgun approach is always used when compiling out of tree which is
> what many people do anyway without realizing that there are additional
> -I. Why do not we make in-tree behave the same way? Thanks,

Are you suggesting bandaging this individual leaf directory, as in
choice 1), because it's no worse than what happens for an out-of-tree
build anyway, or are you suggesting to fix the in-tree build behavior to
be as inefficient as the out-of-tree behavior in general?  It appears
to me that options 2) and 3) are the intended solutions for this issue
while 1) is more of a workaround.  Thanks,

Alex

> >  From a
> > maintenance perspective I agree that 2) seems more error prone,
> > especially when the build system only catches the error on in-tree
> > builds, something I rarely do.  Therefore I'm leaning towards option
> > 3).  The hardcoded path here doesn't seem much of an issue relative to
> > the negatives of the other approaches (how often do we move these
> > files?) and it keeps the trace support relatively self-contained.  Are
> > there further arguments for or against these options?  Otherwise who
> > wants to formally post the TRACE_INCLUDE_PATH version?  Thanks,  
> 

Re: [PATCH] vfio_pci: Add local source directory as include

[Alexey Kardashevskiy]

On 08/01/2019 11:24, Alex Williamson wrote:
> On Tue, 8 Jan 2019 10:52:43 +1100
> Alexey Kardashevskiy <aik@ozlabs.ru> wrote:
> 
>> On 08/01/2019 07:13, Alex Williamson wrote:
>>> On Mon, 7 Jan 2019 20:39:19 +0900
>>> Masahiro Yamada <yamada.masahiro@socionext.com> wrote:
>>>   
>>>> On Mon, Jan 7, 2019 at 8:09 PM Cornelia Huck <cohuck@redhat.com> wrote:  
>>>>>
>>>>> On Mon, 7 Jan 2019 19:12:10 +0900
>>>>> Masahiro Yamada <yamada.masahiro@socionext.com> wrote:
>>>>>    
>>>>>> On Mon, Jan 7, 2019 at 6:18 PM Michael Ellerman <mpe@ellerman.id.au> wrote:    
>>>>>>>
>>>>>>> Laura Abbott <labbott@redhat.com> writes:    
>>>>>>>> Commit 7f92891778df ("vfio_pci: Add NVIDIA GV100GL [Tesla V100 SXM2]
>>>>>>>> subdriver") introduced a trace.h file in the local directory but
>>>>>>>> missed adding the local include path, resulting in compilation
>>>>>>>> failures with tracepoints:
>>>>>>>>
>>>>>>>> In file included from drivers/vfio/pci/trace.h:102,
>>>>>>>>                  from drivers/vfio/pci/vfio_pci_nvlink2.c:29:
>>>>>>>> ./include/trace/define_trace.h:89:42: fatal error: ./trace.h: No such file or directory
>>>>>>>>  #include TRACE_INCLUDE(TRACE_INCLUDE_FILE)
>>>>>>>>
>>>>>>>> Fix this by adjusting the include path.
>>>>>>>>
>>>>>>>> Fixes: 7f92891778df ("vfio_pci: Add NVIDIA GV100GL [Tesla V100 SXM2] subdriver")
>>>>>>>> Signed-off-by: Laura Abbott <labbott@redhat.com>    
>>>>>
>>>>> (...)
>>>>>    
>>>>>>> Alex I assume you'll merge this fix via the vfio tree?
>>>>>>>
>>>>>>> cheers
>>>>>>>    
>>>>>>>> diff --git a/drivers/vfio/pci/Makefile b/drivers/vfio/pci/Makefile
>>>>>>>> index 9662c063a6b1..08d4676a8495 100644
>>>>>>>> --- a/drivers/vfio/pci/Makefile
>>>>>>>> +++ b/drivers/vfio/pci/Makefile
>>>>>>>> @@ -1,3 +1,4 @@
>>>>>>>> +ccflags-y                               += -I$(src)
>>>>>>>>
>>>>>>>>  vfio-pci-y := vfio_pci.o vfio_pci_intrs.o vfio_pci_rdwr.o vfio_pci_config.o
>>>>>>>>  vfio-pci-$(CONFIG_VFIO_PCI_IGD) += vfio_pci_igd.o
>>>>>>>> --
>>>>>>>> 2.20.1    
>>>>>>
>>>>>>
>>>>>> Hi.
>>>>>>
>>>>>> If I correctly understand the usage of TRACE_INCLUDE_PATH,
>>>>>> the correct fix should be like follows:
>>>>>>
>>>>>>
>>>>>> diff --git a/drivers/vfio/pci/trace.h b/drivers/vfio/pci/trace.h
>>>>>> index 228ccdb..4d13e51 100644
>>>>>> --- a/drivers/vfio/pci/trace.h
>>>>>> +++ b/drivers/vfio/pci/trace.h
>>>>>> @@ -94,7 +94,7 @@ TRACE_EVENT(vfio_pci_npu2_mmap,
>>>>>>  #endif /* _TRACE_VFIO_PCI_H */
>>>>>>
>>>>>>  #undef TRACE_INCLUDE_PATH
>>>>>> -#define TRACE_INCLUDE_PATH .
>>>>>> +#define TRACE_INCLUDE_PATH ../../drivers/vfio/pci
>>>>>>  #undef TRACE_INCLUDE_FILE
>>>>>>  #define TRACE_INCLUDE_FILE trace    
>>>>>
>>>>> Going from the comments in samples/trace_events/trace-events-sample.h,
>>>>> I think both approaches are possible, and I see both used in various
>>>>> places.
>>>>>
>>>>> Personally, I'd prefer Laura's patch, as it doesn't involve hardcoding
>>>>> a path.  
>>>
>>> Numbering options for clarity:
>>>
>>> 1)  
>>>> ccflags-y += -I$(src)
>>>> would add the header search path for all files in drivers/vfio/pci/
>>>> whereas only the drivers/vfio/pci/vfio_pci_nvlink2.c needs it.
>>>>  
>>>
>>> 2)  
>>>> CFLAGS_vfio_pci_nvlink2.o += -I$(src)
>>>> is a bit better.
>>>> However, it is not obvious why this extra header search path is needed
>>>> until you find vfio_pci_nvlink2.c including trace.h
>>>>  
>>>
>>> 3)  
>>>> #define TRACE_INCLUDE_PATH ../../drivers/vfio/pci
>>>> clarifies the intention because the related code is all placed in trace.h
>>>>
>>>>
>>>>
>>>> From the comment in include/trace/define_trace.h
>>>> TRACE_INCLUDE_PATH is relative to include/trace/define_trace.h  
>>>
>>> In my scan of the tree, the most common solution seems to be 2) as this
>>> is essentially recommended in the sample file.  3) is well represented,
>>> with much fewer examples of 1), though it might depend how liberally
>>> we grep out or examine the use cases.  Choice 1) also seems to be the
>>> most shotgun approach, adding to the search path for all files.  
>>
>>
>> The shotgun approach is always used when compiling out of tree which is
>> what many people do anyway without realizing that there are additional
>> -I. Why do not we make in-tree behave the same way? Thanks,
> 
> Are you suggesting bandaging this individual leaf directory, as in
> choice 1), because it's no worse than what happens for an out-of-tree
> build anyway, 


I suggest making in-tree and out-of-tree behavior equal - both either
fail or compile. Just makes sense to me.

Since out-of-tree adds extra -I, then there should have been a reason
for that at the time (before 2005 though). Unfortunately git blame does
not say why it was done this way for out-of-tree so imho the safest
thing is to add -I for in-tree as well. Or ditch extra -I and do 2) or
3) - this is fine too and can count as an impressive compile
optimization, although... look below :)


> or are you suggesting to fix the in-tree build behavior to
> be as inefficient as the out-of-tree behavior in general? 

Inefficient you say. Hm.

I tried this:

--- scripts/Makefile.lib.old	2019-01-08 11:39:51.830983393 +1100
+++ scripts/Makefile.lib	2019-01-08 13:09:54.199054981 +1100
@@ -140,11 +140,6 @@
 # If building the kernel in a separate objtree expand all occurrences
 # of -Idir to -I$(srctree)/dir except for absolute paths (starting with
'/').

-ifeq ($(KBUILD_SRC),)
-__c_flags	= $(_c_flags)
-__a_flags	= $(_a_flags)
-__cpp_flags     = $(_cpp_flags)
-else

 # -I$(obj) locates generated .h files
 # $(call addtree,-I$(obj)) locates .h files in srctree, from generated
.c files
@@ -154,7 +149,6 @@
 		  $(call flags,_c_flags)
 __a_flags	= $(call flags,_a_flags)
 __cpp_flags     = $(call flags,_cpp_flags)
-endif


Compiled 3 times with the patch and without, "make clean ; time make
-j200 vmlinux modules".

No patch:

real    4m33.047s       user    481m48.322s     sys     10m15.639s
real    4m29.038s       user    480m22.873s     sys     10m11.394s
real    4m34.373s       user    483m7.570s      sys     10m10.559s

With the patch:

real    4m32.008s       user    479m54.207s     sys     10m13.075s
real    4m30.027s       user    479m46.272s     sys     10m15.886s
real    4m31.548s       user    480m2.897s      sys     10m10.024


which is slightly faster but I guess within accuracy.



> It appears
> to me that options 2) and 3) are the intended solutions for this issue
> while 1) is more of a workaround.  Thanks,
> 
> Alex
> 
>>>  From a
>>> maintenance perspective I agree that 2) seems more error prone,
>>> especially when the build system only catches the error on in-tree
>>> builds, something I rarely do.  Therefore I'm leaning towards option
>>> 3).  The hardcoded path here doesn't seem much of an issue relative to
>>> the negatives of the other approaches (how often do we move these
>>> files?) and it keeps the trace support relatively self-contained.  Are
>>> there further arguments for or against these options?  Otherwise who
>>> wants to formally post the TRACE_INCLUDE_PATH version?  Thanks,  




-- 
Alexey

Re: [PATCH] vfio_pci: Add local source directory as include

[Masahiro Yamada]

On Tue, Jan 8, 2019 at 11:22 AM Alexey Kardashevskiy <aik@ozlabs.ru> wrote:
>
>
>
> On 08/01/2019 11:24, Alex Williamson wrote:
> > On Tue, 8 Jan 2019 10:52:43 +1100
> > Alexey Kardashevskiy <aik@ozlabs.ru> wrote:
> >
> >> On 08/01/2019 07:13, Alex Williamson wrote:
> >>> On Mon, 7 Jan 2019 20:39:19 +0900
> >>> Masahiro Yamada <yamada.masahiro@socionext.com> wrote:
> >>>
> >>>> On Mon, Jan 7, 2019 at 8:09 PM Cornelia Huck <cohuck@redhat.com> wrote:
> >>>>>
> >>>>> On Mon, 7 Jan 2019 19:12:10 +0900
> >>>>> Masahiro Yamada <yamada.masahiro@socionext.com> wrote:
> >>>>>
> >>>>>> On Mon, Jan 7, 2019 at 6:18 PM Michael Ellerman <mpe@ellerman.id.au> wrote:
> >>>>>>>
> >>>>>>> Laura Abbott <labbott@redhat.com> writes:
> >>>>>>>> Commit 7f92891778df ("vfio_pci: Add NVIDIA GV100GL [Tesla V100 SXM2]
> >>>>>>>> subdriver") introduced a trace.h file in the local directory but
> >>>>>>>> missed adding the local include path, resulting in compilation
> >>>>>>>> failures with tracepoints:
> >>>>>>>>
> >>>>>>>> In file included from drivers/vfio/pci/trace.h:102,
> >>>>>>>>                  from drivers/vfio/pci/vfio_pci_nvlink2.c:29:
> >>>>>>>> ./include/trace/define_trace.h:89:42: fatal error: ./trace.h: No such file or directory
> >>>>>>>>  #include TRACE_INCLUDE(TRACE_INCLUDE_FILE)
> >>>>>>>>
> >>>>>>>> Fix this by adjusting the include path.
> >>>>>>>>
> >>>>>>>> Fixes: 7f92891778df ("vfio_pci: Add NVIDIA GV100GL [Tesla V100 SXM2] subdriver")
> >>>>>>>> Signed-off-by: Laura Abbott <labbott@redhat.com>
> >>>>>
> >>>>> (...)
> >>>>>
> >>>>>>> Alex I assume you'll merge this fix via the vfio tree?
> >>>>>>>
> >>>>>>> cheers
> >>>>>>>
> >>>>>>>> diff --git a/drivers/vfio/pci/Makefile b/drivers/vfio/pci/Makefile
> >>>>>>>> index 9662c063a6b1..08d4676a8495 100644
> >>>>>>>> --- a/drivers/vfio/pci/Makefile
> >>>>>>>> +++ b/drivers/vfio/pci/Makefile
> >>>>>>>> @@ -1,3 +1,4 @@
> >>>>>>>> +ccflags-y                               += -I$(src)
> >>>>>>>>
> >>>>>>>>  vfio-pci-y := vfio_pci.o vfio_pci_intrs.o vfio_pci_rdwr.o vfio_pci_config.o
> >>>>>>>>  vfio-pci-$(CONFIG_VFIO_PCI_IGD) += vfio_pci_igd.o
> >>>>>>>> --
> >>>>>>>> 2.20.1
> >>>>>>
> >>>>>>
> >>>>>> Hi.
> >>>>>>
> >>>>>> If I correctly understand the usage of TRACE_INCLUDE_PATH,
> >>>>>> the correct fix should be like follows:
> >>>>>>
> >>>>>>
> >>>>>> diff --git a/drivers/vfio/pci/trace.h b/drivers/vfio/pci/trace.h
> >>>>>> index 228ccdb..4d13e51 100644
> >>>>>> --- a/drivers/vfio/pci/trace.h
> >>>>>> +++ b/drivers/vfio/pci/trace.h
> >>>>>> @@ -94,7 +94,7 @@ TRACE_EVENT(vfio_pci_npu2_mmap,
> >>>>>>  #endif /* _TRACE_VFIO_PCI_H */
> >>>>>>
> >>>>>>  #undef TRACE_INCLUDE_PATH
> >>>>>> -#define TRACE_INCLUDE_PATH .
> >>>>>> +#define TRACE_INCLUDE_PATH ../../drivers/vfio/pci
> >>>>>>  #undef TRACE_INCLUDE_FILE
> >>>>>>  #define TRACE_INCLUDE_FILE trace
> >>>>>
> >>>>> Going from the comments in samples/trace_events/trace-events-sample.h,
> >>>>> I think both approaches are possible, and I see both used in various
> >>>>> places.
> >>>>>
> >>>>> Personally, I'd prefer Laura's patch, as it doesn't involve hardcoding
> >>>>> a path.
> >>>
> >>> Numbering options for clarity:
> >>>
> >>> 1)
> >>>> ccflags-y += -I$(src)
> >>>> would add the header search path for all files in drivers/vfio/pci/
> >>>> whereas only the drivers/vfio/pci/vfio_pci_nvlink2.c needs it.
> >>>>
> >>>
> >>> 2)
> >>>> CFLAGS_vfio_pci_nvlink2.o += -I$(src)
> >>>> is a bit better.
> >>>> However, it is not obvious why this extra header search path is needed
> >>>> until you find vfio_pci_nvlink2.c including trace.h
> >>>>
> >>>
> >>> 3)
> >>>> #define TRACE_INCLUDE_PATH ../../drivers/vfio/pci
> >>>> clarifies the intention because the related code is all placed in trace.h
> >>>>
> >>>>
> >>>>
> >>>> From the comment in include/trace/define_trace.h
> >>>> TRACE_INCLUDE_PATH is relative to include/trace/define_trace.h
> >>>
> >>> In my scan of the tree, the most common solution seems to be 2) as this
> >>> is essentially recommended in the sample file.  3) is well represented,
> >>> with much fewer examples of 1), though it might depend how liberally
> >>> we grep out or examine the use cases.  Choice 1) also seems to be the
> >>> most shotgun approach, adding to the search path for all files.
> >>
> >>
> >> The shotgun approach is always used when compiling out of tree which is
> >> what many people do anyway without realizing that there are additional
> >> -I. Why do not we make in-tree behave the same way? Thanks,
> >
> > Are you suggesting bandaging this individual leaf directory, as in
> > choice 1), because it's no worse than what happens for an out-of-tree
> > build anyway,
>
>
> I suggest making in-tree and out-of-tree behavior equal - both either
> fail or compile. Just makes sense to me.
>
> Since out-of-tree adds extra -I, then there should have been a reason
> for that at the time (before 2005 though). Unfortunately git blame does
> not say why it was done this way for out-of-tree so imho the safest
> thing is to add -I for in-tree as well. Or ditch extra -I and do 2) or
> 3) - this is fine too and can count as an impressive compile
> optimization, although... look below :)


For the out-of-tree building,
scripts/Makefile.lib adds -I$(srctree)/$(src) and -I$(obj).


In my understanding, -I$(srctree)/$(src) is for
including check-in headers from generated C files.

-I$(obj) is for including generated headers from check-in C files.


One example of the correct usage of this is,

  crypto/rsa_helper.c     (check-in file)

includes

  crypto/rsapubkey.asn1.h (generated fle)



Those extra header search paths are unneeded for in-tree building
since generated files and check-in files exist in the same tree.



You just happened to find it works for out-of-tree building.


We are talking about how to make include/trace/define_trace.h
include  drivers/vfio/pci/trace.h

The build system cannot (should not)
cater to such a particular case.






>
> > or are you suggesting to fix the in-tree build behavior to
> > be as inefficient as the out-of-tree behavior in general?
>
> Inefficient you say. Hm.
>
> I tried this:
>
> --- scripts/Makefile.lib.old    2019-01-08 11:39:51.830983393 +1100
> +++ scripts/Makefile.lib        2019-01-08 13:09:54.199054981 +1100
> @@ -140,11 +140,6 @@
>  # If building the kernel in a separate objtree expand all occurrences
>  # of -Idir to -I$(srctree)/dir except for absolute paths (starting with
> '/').
>
> -ifeq ($(KBUILD_SRC),)
> -__c_flags      = $(_c_flags)
> -__a_flags      = $(_a_flags)
> -__cpp_flags     = $(_cpp_flags)
> -else
>
>  # -I$(obj) locates generated .h files
>  # $(call addtree,-I$(obj)) locates .h files in srctree, from generated
> .c files
> @@ -154,7 +149,6 @@
>                   $(call flags,_c_flags)
>  __a_flags      = $(call flags,_a_flags)
>  __cpp_flags     = $(call flags,_cpp_flags)
> -endif
>
>
> Compiled 3 times with the patch and without, "make clean ; time make
> -j200 vmlinux modules".
>
> No patch:
>
> real    4m33.047s       user    481m48.322s     sys     10m15.639s
> real    4m29.038s       user    480m22.873s     sys     10m11.394s
> real    4m34.373s       user    483m7.570s      sys     10m10.559s
>
> With the patch:
>
> real    4m32.008s       user    479m54.207s     sys     10m13.075s
> real    4m30.027s       user    479m46.272s     sys     10m15.886s
> real    4m31.548s       user    480m2.897s      sys     10m10.024
>
>
> which is slightly faster but I guess within accuracy.
>
>
>
> > It appears
> > to me that options 2) and 3) are the intended solutions for this issue
> > while 1) is more of a workaround.  Thanks,
> >
> > Alex
> >
> >>>  From a
> >>> maintenance perspective I agree that 2) seems more error prone,
> >>> especially when the build system only catches the error on in-tree
> >>> builds, something I rarely do.  Therefore I'm leaning towards option
> >>> 3).  The hardcoded path here doesn't seem much of an issue relative to
> >>> the negatives of the other approaches (how often do we move these
> >>> files?) and it keeps the trace support relatively self-contained.  Are
> >>> there further arguments for or against these options?  Otherwise who
> >>> wants to formally post the TRACE_INCLUDE_PATH version?  Thanks,
>
>
>
>
> --
> Alexey



-- 
Best Regards
Masahiro Yamada

Re: [PATCH] vfio_pci: Add local source directory as include

[Alexey Kardashevskiy]

On 08/01/2019 13:38, Masahiro Yamada wrote:
> On Tue, Jan 8, 2019 at 11:22 AM Alexey Kardashevskiy <aik@ozlabs.ru> wrote:
>>
>>
>>
>> On 08/01/2019 11:24, Alex Williamson wrote:
>>> On Tue, 8 Jan 2019 10:52:43 +1100
>>> Alexey Kardashevskiy <aik@ozlabs.ru> wrote:
>>>
>>>> On 08/01/2019 07:13, Alex Williamson wrote:
>>>>> On Mon, 7 Jan 2019 20:39:19 +0900
>>>>> Masahiro Yamada <yamada.masahiro@socionext.com> wrote:
>>>>>
>>>>>> On Mon, Jan 7, 2019 at 8:09 PM Cornelia Huck <cohuck@redhat.com> wrote:
>>>>>>>
>>>>>>> On Mon, 7 Jan 2019 19:12:10 +0900
>>>>>>> Masahiro Yamada <yamada.masahiro@socionext.com> wrote:
>>>>>>>
>>>>>>>> On Mon, Jan 7, 2019 at 6:18 PM Michael Ellerman <mpe@ellerman.id.au> wrote:
>>>>>>>>>
>>>>>>>>> Laura Abbott <labbott@redhat.com> writes:
>>>>>>>>>> Commit 7f92891778df ("vfio_pci: Add NVIDIA GV100GL [Tesla V100 SXM2]
>>>>>>>>>> subdriver") introduced a trace.h file in the local directory but
>>>>>>>>>> missed adding the local include path, resulting in compilation
>>>>>>>>>> failures with tracepoints:
>>>>>>>>>>
>>>>>>>>>> In file included from drivers/vfio/pci/trace.h:102,
>>>>>>>>>>                  from drivers/vfio/pci/vfio_pci_nvlink2.c:29:
>>>>>>>>>> ./include/trace/define_trace.h:89:42: fatal error: ./trace.h: No such file or directory
>>>>>>>>>>  #include TRACE_INCLUDE(TRACE_INCLUDE_FILE)
>>>>>>>>>>
>>>>>>>>>> Fix this by adjusting the include path.
>>>>>>>>>>
>>>>>>>>>> Fixes: 7f92891778df ("vfio_pci: Add NVIDIA GV100GL [Tesla V100 SXM2] subdriver")
>>>>>>>>>> Signed-off-by: Laura Abbott <labbott@redhat.com>
>>>>>>>
>>>>>>> (...)
>>>>>>>
>>>>>>>>> Alex I assume you'll merge this fix via the vfio tree?
>>>>>>>>>
>>>>>>>>> cheers
>>>>>>>>>
>>>>>>>>>> diff --git a/drivers/vfio/pci/Makefile b/drivers/vfio/pci/Makefile
>>>>>>>>>> index 9662c063a6b1..08d4676a8495 100644
>>>>>>>>>> --- a/drivers/vfio/pci/Makefile
>>>>>>>>>> +++ b/drivers/vfio/pci/Makefile
>>>>>>>>>> @@ -1,3 +1,4 @@
>>>>>>>>>> +ccflags-y                               += -I$(src)
>>>>>>>>>>
>>>>>>>>>>  vfio-pci-y := vfio_pci.o vfio_pci_intrs.o vfio_pci_rdwr.o vfio_pci_config.o
>>>>>>>>>>  vfio-pci-$(CONFIG_VFIO_PCI_IGD) += vfio_pci_igd.o
>>>>>>>>>> --
>>>>>>>>>> 2.20.1
>>>>>>>>
>>>>>>>>
>>>>>>>> Hi.
>>>>>>>>
>>>>>>>> If I correctly understand the usage of TRACE_INCLUDE_PATH,
>>>>>>>> the correct fix should be like follows:
>>>>>>>>
>>>>>>>>
>>>>>>>> diff --git a/drivers/vfio/pci/trace.h b/drivers/vfio/pci/trace.h
>>>>>>>> index 228ccdb..4d13e51 100644
>>>>>>>> --- a/drivers/vfio/pci/trace.h
>>>>>>>> +++ b/drivers/vfio/pci/trace.h
>>>>>>>> @@ -94,7 +94,7 @@ TRACE_EVENT(vfio_pci_npu2_mmap,
>>>>>>>>  #endif /* _TRACE_VFIO_PCI_H */
>>>>>>>>
>>>>>>>>  #undef TRACE_INCLUDE_PATH
>>>>>>>> -#define TRACE_INCLUDE_PATH .
>>>>>>>> +#define TRACE_INCLUDE_PATH ../../drivers/vfio/pci
>>>>>>>>  #undef TRACE_INCLUDE_FILE
>>>>>>>>  #define TRACE_INCLUDE_FILE trace
>>>>>>>
>>>>>>> Going from the comments in samples/trace_events/trace-events-sample.h,
>>>>>>> I think both approaches are possible, and I see both used in various
>>>>>>> places.
>>>>>>>
>>>>>>> Personally, I'd prefer Laura's patch, as it doesn't involve hardcoding
>>>>>>> a path.
>>>>>
>>>>> Numbering options for clarity:
>>>>>
>>>>> 1)
>>>>>> ccflags-y += -I$(src)
>>>>>> would add the header search path for all files in drivers/vfio/pci/
>>>>>> whereas only the drivers/vfio/pci/vfio_pci_nvlink2.c needs it.
>>>>>>
>>>>>
>>>>> 2)
>>>>>> CFLAGS_vfio_pci_nvlink2.o += -I$(src)
>>>>>> is a bit better.
>>>>>> However, it is not obvious why this extra header search path is needed
>>>>>> until you find vfio_pci_nvlink2.c including trace.h
>>>>>>
>>>>>
>>>>> 3)
>>>>>> #define TRACE_INCLUDE_PATH ../../drivers/vfio/pci
>>>>>> clarifies the intention because the related code is all placed in trace.h
>>>>>>
>>>>>>
>>>>>>
>>>>>> From the comment in include/trace/define_trace.h
>>>>>> TRACE_INCLUDE_PATH is relative to include/trace/define_trace.h
>>>>>
>>>>> In my scan of the tree, the most common solution seems to be 2) as this
>>>>> is essentially recommended in the sample file.  3) is well represented,
>>>>> with much fewer examples of 1), though it might depend how liberally
>>>>> we grep out or examine the use cases.  Choice 1) also seems to be the
>>>>> most shotgun approach, adding to the search path for all files.
>>>>
>>>>
>>>> The shotgun approach is always used when compiling out of tree which is
>>>> what many people do anyway without realizing that there are additional
>>>> -I. Why do not we make in-tree behave the same way? Thanks,
>>>
>>> Are you suggesting bandaging this individual leaf directory, as in
>>> choice 1), because it's no worse than what happens for an out-of-tree
>>> build anyway,
>>
>>
>> I suggest making in-tree and out-of-tree behavior equal - both either
>> fail or compile. Just makes sense to me.
>>
>> Since out-of-tree adds extra -I, then there should have been a reason
>> for that at the time (before 2005 though). Unfortunately git blame does
>> not say why it was done this way for out-of-tree so imho the safest
>> thing is to add -I for in-tree as well. Or ditch extra -I and do 2) or
>> 3) - this is fine too and can count as an impressive compile
>> optimization, although... look below :)
> 
> 
> For the out-of-tree building,
> scripts/Makefile.lib adds -I$(srctree)/$(src) and -I$(obj).
> 
> 
> In my understanding, -I$(srctree)/$(src) is for
> including check-in headers from generated C files.
> 
> -I$(obj) is for including generated headers from check-in C files.
> 
> 
> One example of the correct usage of this is,
> 
>   crypto/rsa_helper.c     (check-in file)
> 
> includes
> 
>   crypto/rsapubkey.asn1.h (generated fle)
> 
> 
> 
> Those extra header search paths are unneeded for in-tree building
> since generated files and check-in files exist in the same tree.
> 
> 
> 
> You just happened to find it works for out-of-tree building.
> 
> 
> We are talking about how to make include/trace/define_trace.h
> include  drivers/vfio/pci/trace.h
> 
> The build system cannot (should not)
> cater to such a particular case.


Oh well, thanks for the explanation.

Alex, how do we proceed now? I like 2) + comment as in
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/misc/ocxl/Makefile?h=v4.20#n8
better than 3) as relative paths make it dependable on what file
actually includes it and it is not clear what is that and what happens
if that file moves deeper in the tree (unlikely to happen though).




-- 
Alexey

Re: [PATCH] vfio_pci: Add local source directory as include

[Masahiro Yamada]

On Tue, Jan 8, 2019 at 11:59 AM Alexey Kardashevskiy <aik@ozlabs.ru> wrote:
>
>
>
> On 08/01/2019 13:38, Masahiro Yamada wrote:
> > On Tue, Jan 8, 2019 at 11:22 AM Alexey Kardashevskiy <aik@ozlabs.ru> wrote:
> >>
> >>
> >>
> >> On 08/01/2019 11:24, Alex Williamson wrote:
> >>> On Tue, 8 Jan 2019 10:52:43 +1100
> >>> Alexey Kardashevskiy <aik@ozlabs.ru> wrote:
> >>>
> >>>> On 08/01/2019 07:13, Alex Williamson wrote:
> >>>>> On Mon, 7 Jan 2019 20:39:19 +0900
> >>>>> Masahiro Yamada <yamada.masahiro@socionext.com> wrote:
> >>>>>
> >>>>>> On Mon, Jan 7, 2019 at 8:09 PM Cornelia Huck <cohuck@redhat.com> wrote:
> >>>>>>>
> >>>>>>> On Mon, 7 Jan 2019 19:12:10 +0900
> >>>>>>> Masahiro Yamada <yamada.masahiro@socionext.com> wrote:
> >>>>>>>
> >>>>>>>> On Mon, Jan 7, 2019 at 6:18 PM Michael Ellerman <mpe@ellerman.id.au> wrote:
> >>>>>>>>>
> >>>>>>>>> Laura Abbott <labbott@redhat.com> writes:
> >>>>>>>>>> Commit 7f92891778df ("vfio_pci: Add NVIDIA GV100GL [Tesla V100 SXM2]
> >>>>>>>>>> subdriver") introduced a trace.h file in the local directory but
> >>>>>>>>>> missed adding the local include path, resulting in compilation
> >>>>>>>>>> failures with tracepoints:
> >>>>>>>>>>
> >>>>>>>>>> In file included from drivers/vfio/pci/trace.h:102,
> >>>>>>>>>>                  from drivers/vfio/pci/vfio_pci_nvlink2.c:29:
> >>>>>>>>>> ./include/trace/define_trace.h:89:42: fatal error: ./trace.h: No such file or directory
> >>>>>>>>>>  #include TRACE_INCLUDE(TRACE_INCLUDE_FILE)
> >>>>>>>>>>
> >>>>>>>>>> Fix this by adjusting the include path.
> >>>>>>>>>>
> >>>>>>>>>> Fixes: 7f92891778df ("vfio_pci: Add NVIDIA GV100GL [Tesla V100 SXM2] subdriver")
> >>>>>>>>>> Signed-off-by: Laura Abbott <labbott@redhat.com>
> >>>>>>>
> >>>>>>> (...)
> >>>>>>>
> >>>>>>>>> Alex I assume you'll merge this fix via the vfio tree?
> >>>>>>>>>
> >>>>>>>>> cheers
> >>>>>>>>>
> >>>>>>>>>> diff --git a/drivers/vfio/pci/Makefile b/drivers/vfio/pci/Makefile
> >>>>>>>>>> index 9662c063a6b1..08d4676a8495 100644
> >>>>>>>>>> --- a/drivers/vfio/pci/Makefile
> >>>>>>>>>> +++ b/drivers/vfio/pci/Makefile
> >>>>>>>>>> @@ -1,3 +1,4 @@
> >>>>>>>>>> +ccflags-y                               += -I$(src)
> >>>>>>>>>>
> >>>>>>>>>>  vfio-pci-y := vfio_pci.o vfio_pci_intrs.o vfio_pci_rdwr.o vfio_pci_config.o
> >>>>>>>>>>  vfio-pci-$(CONFIG_VFIO_PCI_IGD) += vfio_pci_igd.o
> >>>>>>>>>> --
> >>>>>>>>>> 2.20.1
> >>>>>>>>
> >>>>>>>>
> >>>>>>>> Hi.
> >>>>>>>>
> >>>>>>>> If I correctly understand the usage of TRACE_INCLUDE_PATH,
> >>>>>>>> the correct fix should be like follows:
> >>>>>>>>
> >>>>>>>>
> >>>>>>>> diff --git a/drivers/vfio/pci/trace.h b/drivers/vfio/pci/trace.h
> >>>>>>>> index 228ccdb..4d13e51 100644
> >>>>>>>> --- a/drivers/vfio/pci/trace.h
> >>>>>>>> +++ b/drivers/vfio/pci/trace.h
> >>>>>>>> @@ -94,7 +94,7 @@ TRACE_EVENT(vfio_pci_npu2_mmap,
> >>>>>>>>  #endif /* _TRACE_VFIO_PCI_H */
> >>>>>>>>
> >>>>>>>>  #undef TRACE_INCLUDE_PATH
> >>>>>>>> -#define TRACE_INCLUDE_PATH .
> >>>>>>>> +#define TRACE_INCLUDE_PATH ../../drivers/vfio/pci
> >>>>>>>>  #undef TRACE_INCLUDE_FILE
> >>>>>>>>  #define TRACE_INCLUDE_FILE trace
> >>>>>>>
> >>>>>>> Going from the comments in samples/trace_events/trace-events-sample.h,
> >>>>>>> I think both approaches are possible, and I see both used in various
> >>>>>>> places.
> >>>>>>>
> >>>>>>> Personally, I'd prefer Laura's patch, as it doesn't involve hardcoding
> >>>>>>> a path.
> >>>>>
> >>>>> Numbering options for clarity:
> >>>>>
> >>>>> 1)
> >>>>>> ccflags-y += -I$(src)
> >>>>>> would add the header search path for all files in drivers/vfio/pci/
> >>>>>> whereas only the drivers/vfio/pci/vfio_pci_nvlink2.c needs it.
> >>>>>>
> >>>>>
> >>>>> 2)
> >>>>>> CFLAGS_vfio_pci_nvlink2.o += -I$(src)
> >>>>>> is a bit better.
> >>>>>> However, it is not obvious why this extra header search path is needed
> >>>>>> until you find vfio_pci_nvlink2.c including trace.h
> >>>>>>
> >>>>>
> >>>>> 3)
> >>>>>> #define TRACE_INCLUDE_PATH ../../drivers/vfio/pci
> >>>>>> clarifies the intention because the related code is all placed in trace.h
> >>>>>>
> >>>>>>
> >>>>>>
> >>>>>> From the comment in include/trace/define_trace.h
> >>>>>> TRACE_INCLUDE_PATH is relative to include/trace/define_trace.h
> >>>>>
> >>>>> In my scan of the tree, the most common solution seems to be 2) as this
> >>>>> is essentially recommended in the sample file.  3) is well represented,
> >>>>> with much fewer examples of 1), though it might depend how liberally
> >>>>> we grep out or examine the use cases.  Choice 1) also seems to be the
> >>>>> most shotgun approach, adding to the search path for all files.
> >>>>
> >>>>
> >>>> The shotgun approach is always used when compiling out of tree which is
> >>>> what many people do anyway without realizing that there are additional
> >>>> -I. Why do not we make in-tree behave the same way? Thanks,
> >>>
> >>> Are you suggesting bandaging this individual leaf directory, as in
> >>> choice 1), because it's no worse than what happens for an out-of-tree
> >>> build anyway,
> >>
> >>
> >> I suggest making in-tree and out-of-tree behavior equal - both either
> >> fail or compile. Just makes sense to me.
> >>
> >> Since out-of-tree adds extra -I, then there should have been a reason
> >> for that at the time (before 2005 though). Unfortunately git blame does
> >> not say why it was done this way for out-of-tree so imho the safest
> >> thing is to add -I for in-tree as well. Or ditch extra -I and do 2) or
> >> 3) - this is fine too and can count as an impressive compile
> >> optimization, although... look below :)
> >
> >
> > For the out-of-tree building,
> > scripts/Makefile.lib adds -I$(srctree)/$(src) and -I$(obj).
> >
> >
> > In my understanding, -I$(srctree)/$(src) is for
> > including check-in headers from generated C files.
> >
> > -I$(obj) is for including generated headers from check-in C files.
> >
> >
> > One example of the correct usage of this is,
> >
> >   crypto/rsa_helper.c     (check-in file)
> >
> > includes
> >
> >   crypto/rsapubkey.asn1.h (generated fle)
> >
> >
> >
> > Those extra header search paths are unneeded for in-tree building
> > since generated files and check-in files exist in the same tree.
> >
> >
> >
> > You just happened to find it works for out-of-tree building.
> >
> >
> > We are talking about how to make include/trace/define_trace.h
> > include  drivers/vfio/pci/trace.h
> >
> > The build system cannot (should not)
> > cater to such a particular case.
>
>
> Oh well, thanks for the explanation.
>
> Alex, how do we proceed now? I like 2) + comment as in
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/misc/ocxl/Makefile?h=v4.20#n8

It is clear, but
3) TRACE_INCLUDE_PATH solution
is self-documenting.


> better than 3) as relative paths make it dependable on what file
> actually includes it and it is not clear what is that and what happens
> if that file moves deeper in the tree (unlikely to happen though).


From the build system PoV, I'd like to discourage people
from adding crappy header search path like this.

Both 2) and 3) have pros and cons.

I just sent 3) patch, but it is the maintainer's call after all.



>
>
>
>
> --
> Alexey



-- 
Best Regards
Masahiro Yamada

Re: [PATCH] vfio_pci: Add local source directory as include

[Michael Ellerman]

Alex Williamson <alex.williamson@redhat.com> writes:
...
>
> Numbering options for clarity:
>
> 1)
>> ccflags-y += -I$(src)
>> would add the header search path for all files in drivers/vfio/pci/
>> whereas only the drivers/vfio/pci/vfio_pci_nvlink2.c needs it.
>> 
>
> 2)
>> CFLAGS_vfio_pci_nvlink2.o += -I$(src)
>> is a bit better.
>> However, it is not obvious why this extra header search path is needed
>> until you find vfio_pci_nvlink2.c including trace.h
>> 
>
> 3)
>> #define TRACE_INCLUDE_PATH ../../drivers/vfio/pci
>> clarifies the intention because the related code is all placed in trace.h
 
Good summary.

>> From the comment in include/trace/define_trace.h
>> TRACE_INCLUDE_PATH is relative to include/trace/define_trace.h
>
> In my scan of the tree, the most common solution seems to be 2) as this
> is essentially recommended in the sample file.  3) is well represented,
> with much fewer examples of 1), though it might depend how liberally
> we grep out or examine the use cases.

It seems to me that 1 and 2 is overwhelmingly used:

$ git grep -F "#define TRACE_INCLUDE_PATH" | wc -l
133

That counts all definitions of TRACE_INCLUDE_PATH.

$ git grep -F "#define TRACE_INCLUDE_PATH ." | wc -l
122

That's all files using '.', so only 11 locations use the relative path
method (3).


Which is unsurprising given that the sample uses '.'.

And people often look at existing code for an example, so they're also
going to tend to use '.'.


I agree with Masahiro that adding include paths to the Makefile for
this is a bit gross, and method 3 is much more preferable.

Fixing all the existing code to use method 3 would be a good beginner
project :)

cheers

Re: [PATCH] vfio_pci: Add local source directory as include

[Laura Abbott]

On 1/7/19 12:58 AM, Michael Ellerman wrote:
> Laura Abbott <labbott@redhat.com> writes:
>> Commit 7f92891778df ("vfio_pci: Add NVIDIA GV100GL [Tesla V100 SXM2]
>> subdriver") introduced a trace.h file in the local directory but
>> missed adding the local include path, resulting in compilation
>> failures with tracepoints:
>>
>> In file included from drivers/vfio/pci/trace.h:102,
>>                   from drivers/vfio/pci/vfio_pci_nvlink2.c:29:
>> ./include/trace/define_trace.h:89:42: fatal error: ./trace.h: No such file or directory
>>   #include TRACE_INCLUDE(TRACE_INCLUDE_FILE)
>>
>> Fix this by adjusting the include path.
>>
>> Fixes: 7f92891778df ("vfio_pci: Add NVIDIA GV100GL [Tesla V100 SXM2] subdriver")
>> Signed-off-by: Laura Abbott <labbott@redhat.com>
>> ---
>> I'd still like to echo my sentiment that this should not be a def_bool.
>> We hit this error on our internal testing and we couldn't even turn
>> off the driver until we fixed this.
> 
> I assume there's some reason you can't commit a patch to your tree to
> change it to bool, or turn it off entirely? That would change the SHA
> which is perhaps reason enough.
> 
> In general we have far too many options and most of them never get
> turned off (or on), so it just creates testing/bug surface for not much
> benefit. This is one that will probably be turned on in all distro
> kernels for example.
> 
> But I have no real objection to making it user configurable.
> 
> 
> Alex I assume you'll merge this fix via the vfio tree?
> 
> cheers
> 

Well now that everything is fixed, we will probably keep it on.
My gripe is that when things break like this it's difficult to
work around vs. the configuration option at least makes it possible
to work around in a fast way. This may also just be a strong
preference of mine for working around problems.

Thanks,
Laura

>> diff --git a/drivers/vfio/pci/Makefile b/drivers/vfio/pci/Makefile
>> index 9662c063a6b1..08d4676a8495 100644
>> --- a/drivers/vfio/pci/Makefile
>> +++ b/drivers/vfio/pci/Makefile
>> @@ -1,3 +1,4 @@
>> +ccflags-y                               += -I$(src)
>>   
>>   vfio-pci-y := vfio_pci.o vfio_pci_intrs.o vfio_pci_rdwr.o vfio_pci_config.o
>>   vfio-pci-$(CONFIG_VFIO_PCI_IGD) += vfio_pci_igd.o
>> -- 
>> 2.20.1

Re: [PATCH] vfio_pci: Add local source directory as include

[Cornelia Huck]

On Fri,  4 Jan 2019 11:57:14 -0800
Laura Abbott <labbott@redhat.com> wrote:

> Commit 7f92891778df ("vfio_pci: Add NVIDIA GV100GL [Tesla V100 SXM2]
> subdriver") introduced a trace.h file in the local directory but
> missed adding the local include path, resulting in compilation
> failures with tracepoints:
> 
> In file included from drivers/vfio/pci/trace.h:102,
>                  from drivers/vfio/pci/vfio_pci_nvlink2.c:29:
> ./include/trace/define_trace.h:89:42: fatal error: ./trace.h: No such file or directory
>  #include TRACE_INCLUDE(TRACE_INCLUDE_FILE)
> 
> Fix this by adjusting the include path.
> 
> Fixes: 7f92891778df ("vfio_pci: Add NVIDIA GV100GL [Tesla V100 SXM2] subdriver")
> Signed-off-by: Laura Abbott <labbott@redhat.com>
> ---
> I'd still like to echo my sentiment that this should not be a def_bool.
> We hit this error on our internal testing and we couldn't even turn
> off the driver until we fixed this.
> ---
>  drivers/vfio/pci/Makefile | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/vfio/pci/Makefile b/drivers/vfio/pci/Makefile
> index 9662c063a6b1..08d4676a8495 100644
> --- a/drivers/vfio/pci/Makefile
> +++ b/drivers/vfio/pci/Makefile
> @@ -1,3 +1,4 @@
> +ccflags-y                               += -I$(src)
>  
>  vfio-pci-y := vfio_pci.o vfio_pci_intrs.o vfio_pci_rdwr.o vfio_pci_config.o
>  vfio-pci-$(CONFIG_VFIO_PCI_IGD) += vfio_pci_igd.o

Reviewed-by: Cornelia Huck <cohuck@redhat.com>