The failure is due to the CONFIG_PPS is not set there, consequently
CONFIG_PTP_1588_CLOCK can not be set as =y anyway.
So David's patch of "da1586461e53a4dd045738cce309ab488970f0ef [1/9] pch_gbe:
Fix PTP dependencies" is buggy. Furthermore, I think using "selects" to
resolve such dependency issue is not good idea as it won't visit the dependencies.
David, I would still suggest to take my original patch:
https://lkml.org/lkml/2012/9/28/70
+ depends on PTP_1588_CLOCK_PCH && (PCH_GBE=m || PTP_1588_CLOCK_PCH=y)
or simply like:
---
From: Haicheng Li <[email protected]>
Fix build error caused by broken PCH_PTP module dependency.
The .config is:
CONFIG_PCH_GBE=y
CONFIG_PCH_PTP=y
CONFIG_PTP_1588_CLOCK=m
The build error:
drivers/built-in.o: In function `pch_tx_timestamp':
.../pch_gbe_main.c:215: undefined reference to `pch_ch_event_read'
.../pch_gbe_main.c:225: undefined reference to `pch_tx_snap_read'
.../pch_gbe_main.c:231: undefined reference to `pch_ch_event_write'
.../pch_gbe_main.c:170: undefined reference to `pch_ch_event_read'
.../pch_gbe_main.c:175: undefined reference to `pch_src_uuid_lo_read'
.../pch_gbe_main.c:176: undefined reference to `pch_src_uuid_hi_read'
.../pch_gbe_main.c:190: undefined reference to `pch_ch_event_write'
.../pch_gbe_main.c:184: undefined reference to `pch_rx_snap_read'
.../pch_gbe_main.c:267: undefined reference to `pch_ch_control_write'
.../pch_gbe_main.c:271: undefined reference to `pch_ch_control_write'
.../pch_gbe_main.c:275: undefined reference to `pch_ch_control_write'
.../pch_gbe_main.c:281: undefined reference to `pch_ch_control_write'
.../pch_gbe_main.c:283: undefined reference to `pch_set_station_address'
.../pch_gbe_main.c:290: undefined reference to `pch_ch_event_write'
Signed-off-by: Haicheng Li <[email protected]>
---
drivers/net/ethernet/oki-semi/pch_gbe/Kconfig | 4 ++--
1 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/net/ethernet/oki-semi/pch_gbe/Kconfig
b/drivers/net/ethernet/oki-semi/pch_gbe/Kconfig
index bce0164..df1e649 100644
--- a/drivers/net/ethernet/oki-semi/pch_gbe/Kconfig
+++ b/drivers/net/ethernet/oki-semi/pch_gbe/Kconfig
@@ -21,12 +21,12 @@ config PCH_GBE
ML7223/ML7831 is companion chip for Intel Atom E6xx series.
ML7223/ML7831 is completely compatible for Intel EG20T PCH.
-if PCH_GBE
+if PTP_1588_CLOCK_PCH
config PCH_PTP
bool "PCH PTP clock support"
default n
- depends on PTP_1588_CLOCK_PCH
+ depends on PTP_1588_CLOCK_PCH=y || PCH_GBE=m
---help---
Say Y here if you want to use Precision Time Protocol (PTP) in the
driver. PTP is a method to precisely synchronize distributed clocks
On 10/06/2012 03:59 PM, Fengguang Wu wrote:
> Hi David,
>
> FYI, kernel build failed on
>
> tree: git://git.kernel.org/pub/scm/linux/kernel/git/davem/net.git master
> head: c0b8b99287235626a5850ef7e5bfc842d1ebcecd
> commit: da1586461e53a4dd045738cce309ab488970f0ef [1/9] pch_gbe: Fix PTP dependencies.
> config: x86_64-randconfig-s052 (attached as .config)
>
> All error/warnings:
>
> drivers/built-in.o: In function `pch_gbe_ioctl':
> pch_gbe_main.c:(.text+0x510370): undefined reference to `pch_ch_control_write'
> pch_gbe_main.c:(.text+0x510393): undefined reference to `pch_ch_control_write'
> pch_gbe_main.c:(.text+0x5103b3): undefined reference to `pch_ch_control_write'
> pch_gbe_main.c:(.text+0x5103e1): undefined reference to `pch_set_station_address'
> pch_gbe_main.c:(.text+0x510403): undefined reference to `pch_ch_control_write'
> pch_gbe_main.c:(.text+0x510431): undefined reference to `pch_set_station_address'
> pch_gbe_main.c:(.text+0x51043e): undefined reference to `pch_ch_event_write'
> drivers/built-in.o: In function `pch_gbe_xmit_frame':
> pch_gbe_main.c:(.text+0x510fe4): undefined reference to `pch_ch_event_read'
> pch_gbe_main.c:(.text+0x511049): undefined reference to `pch_tx_snap_read'
> pch_gbe_main.c:(.text+0x51106f): undefined reference to `pch_ch_event_write'
> drivers/built-in.o: In function `pch_gbe_napi_poll':
> pch_gbe_main.c:(.text+0x511537): undefined reference to `pch_ch_event_read'
> pch_gbe_main.c:(.text+0x511562): undefined reference to `pch_src_uuid_lo_read'
> pch_gbe_main.c:(.text+0x51156d): undefined reference to `pch_src_uuid_hi_read'
> pch_gbe_main.c:(.text+0x511659): undefined reference to `pch_ch_event_write'
> pch_gbe_main.c:(.text+0x511dc1): undefined reference to `pch_rx_snap_read'
>
> ---
> 0-DAY kernel build testing backend Open Source Technology Center
> Fengguang Wu, Yuanhan Liu Intel Corporation
From: Haicheng Li <[email protected]>
Date: Sat, 06 Oct 2012 20:07:08 +0800
> The failure is due to the CONFIG_PPS is not set there, consequently
> CONFIG_PTP_1588_CLOCK can not be set as =y anyway.
>
> So David's patch of "da1586461e53a4dd045738cce309ab488970f0ef [1/9]
> pch_gbe: Fix PTP dependencies" is buggy. Furthermore, I think using
> "selects" to resolve such dependency issue is not good idea as it
> won't visit the dependencies.
>
> David, I would still suggest to take my original patch:
> https://lkml.org/lkml/2012/9/28/70
>
> + depends on PTP_1588_CLOCK_PCH && (PCH_GBE=m || PTP_1588_CLOCK_PCH=y)
>
> or simply like:
This is all very rediculous if you ask me.
Why should the user have to know a detail like the underlying
PTP chip type just to enable PTP on his networking card?
Because that is what you are making him do with your change.
Select removed the necessity of the user having to know these
things.
On 10/06/2012 09:22 PM, David Miller wrote:
> From: Haicheng Li<[email protected]>
> Date: Sat, 06 Oct 2012 20:07:08 +0800
>
>> The failure is due to the CONFIG_PPS is not set there, consequently
>> CONFIG_PTP_1588_CLOCK can not be set as =y anyway.
>>
>> So David's patch of "da1586461e53a4dd045738cce309ab488970f0ef [1/9]
>> pch_gbe: Fix PTP dependencies" is buggy. Furthermore, I think using
>> "selects" to resolve such dependency issue is not good idea as it
>> won't visit the dependencies.
>>
>> David, I would still suggest to take my original patch:
>> https://lkml.org/lkml/2012/9/28/70
>>
>> + depends on PTP_1588_CLOCK_PCH&& (PCH_GBE=m || PTP_1588_CLOCK_PCH=y)
>>
>> or simply like:
>
> This is all very rediculous if you ask me.
>
> Why should the user have to know a detail like the underlying
> PTP chip type just to enable PTP on his networking card?
>
> Because that is what you are making him do with your change.
>
> Select removed the necessity of the user having to know these
> things.
However it possibly breaks the build...
IMHO, the reason why the dependency of PCH_PTP becomes so tricky is that the
code of these two modules call the functions of each other (bad code
structure?). To fix it neatly, either we restructure the code or just simply
make it:
+ depends on PTP_1588_CLOCK_PCH=y
For PCH_GBE=m case, it does be able to pass the build test, but I'm afraid it
won't be smoothly workable via "insmod" due to the codependency of these two
when PCH_PTP is enabled.
From: Haicheng Li <[email protected]>
Date: Sat, 06 Oct 2012 22:07:23 +0800
> On 10/06/2012 09:22 PM, David Miller wrote:
>> From: Haicheng Li<[email protected]>
>> Date: Sat, 06 Oct 2012 20:07:08 +0800
>>
>>> The failure is due to the CONFIG_PPS is not set there, consequently
>>> CONFIG_PTP_1588_CLOCK can not be set as =y anyway.
>>>
>>> So David's patch of "da1586461e53a4dd045738cce309ab488970f0ef [1/9]
>>> pch_gbe: Fix PTP dependencies" is buggy. Furthermore, I think using
>>> "selects" to resolve such dependency issue is not good idea as it
>>> won't visit the dependencies.
>>>
>>> David, I would still suggest to take my original patch:
>>> https://lkml.org/lkml/2012/9/28/70
>>>
>>> + depends on PTP_1588_CLOCK_PCH&& (PCH_GBE=m || PTP_1588_CLOCK_PCH=y)
>>>
>>> or simply like:
>>
>> This is all very rediculous if you ask me.
>>
>> Why should the user have to know a detail like the underlying
>> PTP chip type just to enable PTP on his networking card?
>>
>> Because that is what you are making him do with your change.
>>
>> Select removed the necessity of the user having to know these
>> things.
> However it possibly breaks the build...
>
> IMHO, the reason why the dependency of PCH_PTP becomes so tricky is
> that the code of these two modules call the functions of each other
> (bad code structure?). To fix it neatly, either we restructure the
> code or just simply make it:
> + depends on PTP_1588_CLOCK_PCH=y
>
> For PCH_GBE=m case, it does be able to pass the build test, but I'm
> afraid it won't be smoothly workable via "insmod" due to the
> codependency of these two when PCH_PTP is enabled.
Then why does it work for IXGBE and others who use select?
On Sat, Oct 06, 2012 at 10:07:23PM +0800, Haicheng Li wrote:
>
> IMHO, the reason why the dependency of PCH_PTP becomes so tricky is
> that the code of these two modules call the functions of each other
> (bad code structure?).
Yes, that is the source of the problem. I don't recall how this habit
of having the PTP option as a module got started (and I hope it wasn't
me), but I think the right way to handle this option is with a simple
boolean connected to ifdefs for the MAC driver.
Come to think of it, it may have all started with the gianfar ptp
module. In principle, the time stamping function of a MAC driver can
be completely separate from the clock function, and indeed that is how
the pair of gianfar drivers work.
But for other hardware, it might not be practical to keep the
functions separate, and in that case I would say, just keep it as one
driver.
Thanks,
Richard
PS It looks like no one is really looking after this PCH thing, so
does anyone want to send me a board so I can take care of it? Let me
know off list.
On 10/06/2012 10:21 PM, David Miller wrote:
> From: Haicheng Li<[email protected]>
> Date: Sat, 06 Oct 2012 22:07:23 +0800
>
>> On 10/06/2012 09:22 PM, David Miller wrote:
>>> From: Haicheng Li<[email protected]>
>>> Date: Sat, 06 Oct 2012 20:07:08 +0800
>>>
>>>> The failure is due to the CONFIG_PPS is not set there, consequently
>>>> CONFIG_PTP_1588_CLOCK can not be set as =y anyway.
>>>>
>>>> So David's patch of "da1586461e53a4dd045738cce309ab488970f0ef [1/9]
>>>> pch_gbe: Fix PTP dependencies" is buggy. Furthermore, I think using
>>>> "selects" to resolve such dependency issue is not good idea as it
>>>> won't visit the dependencies.
>>>>
>>>> David, I would still suggest to take my original patch:
>>>> https://lkml.org/lkml/2012/9/28/70
>>>>
>>>> + depends on PTP_1588_CLOCK_PCH&& (PCH_GBE=m || PTP_1588_CLOCK_PCH=y)
>>>>
>>>> or simply like:
>>>
>>> This is all very rediculous if you ask me.
>>>
>>> Why should the user have to know a detail like the underlying
>>> PTP chip type just to enable PTP on his networking card?
>>>
>>> Because that is what you are making him do with your change.
>>>
>>> Select removed the necessity of the user having to know these
>>> things.
>> However it possibly breaks the build...
>>
>> IMHO, the reason why the dependency of PCH_PTP becomes so tricky is
>> that the code of these two modules call the functions of each other
>> (bad code structure?). To fix it neatly, either we restructure the
>> code or just simply make it:
>> + depends on PTP_1588_CLOCK_PCH=y
>>
>> For PCH_GBE=m case, it does be able to pass the build test, but I'm
>> afraid it won't be smoothly workable via "insmod" due to the
>> codependency of these two when PCH_PTP is enabled.
>
> Then why does it work for IXGBE and others who use select?
They explicitly select all the possible dependencies if they are bug-free (I
didn't strictly check them).
Take IXGBE_PTP as example, it explicitly selects PPS, and also depends on
EXPERIMENTAL:
config IXGBE_PTP
bool "PTP Clock Support"
default n
depends on IXGBE && EXPERIMENTAL
select PPS
select PTP_1588_CLOCK
So if you stick to use "select" as the convention of such build issue fixing,
fengguang's build failure would be fixed by:
+ depends on EXPERIMENTAL
+ select PPS
+ select PTP_1588_CLOCK
would you prefer this way?
From: Haicheng Li <[email protected]>
Date: Sun, 07 Oct 2012 01:21:09 +0800
> Take IXGBE_PTP as example, it explicitly selects PPS, and also depends
> on EXPERIMENTAL:
> config IXGBE_PTP
> bool "PTP Clock Support"
> default n
> depends on IXGBE && EXPERIMENTAL
> select PPS
> select PTP_1588_CLOCK
>
> So if you stick to use "select" as the convention of such build issue
> fixing, fengguang's build failure would be fixed by:
> + depends on EXPERIMENTAL
> + select PPS
> + select PTP_1588_CLOCK
>
> would you prefer this way?
Yes, I would.
Thanks.
Fengguang reported a kernel build failure as following:
drivers/built-in.o: In function `pch_gbe_ioctl':
pch_gbe_main.c:(.text+0x510370): undefined reference to `pch_ch_control_write'
pch_gbe_main.c:(.text+0x510393): undefined reference to `pch_ch_control_write'
pch_gbe_main.c:(.text+0x5103b3): undefined reference to `pch_ch_control_write'
...
It's a regression by commit da1586461. The root cause is that
the CONFIG_PPS is not set there, consequently CONFIG_PTP_1588_CLOCK
can not be set anyway, which finally causes ptp_pch and pch_gbe_main
build failures.
As David prefers to use *select* to fix such module co-dependency issues,
this patch explicitly selects all the possible dependencies of PCH_PTP.
Reported-by: Fengguang Wu <[email protected]>
Reviewed-by: David S. Miller <[email protected]>
Signed-off-by: Haicheng Li <[email protected]>
---
drivers/net/ethernet/oki-semi/pch_gbe/Kconfig | 3 +++
1 files changed, 3 insertions(+), 0 deletions(-)
diff --git a/drivers/net/ethernet/oki-semi/pch_gbe/Kconfig
b/drivers/net/ethernet/oki-semi/pch_gbe/Kconfig
index 9730241..5296cc8 100644
--- a/drivers/net/ethernet/oki-semi/pch_gbe/Kconfig
+++ b/drivers/net/ethernet/oki-semi/pch_gbe/Kconfig
@@ -26,6 +26,9 @@ if PCH_GBE
config PCH_PTP
bool "PCH PTP clock support"
default n
+ depends on EXPERIMENTAL
+ select PPS
+ select PTP_1588_CLOCK
select PTP_1588_CLOCK_PCH
---help---
Say Y here if you want to use Precision Time Protocol (PTP) in the
--
1.7.1
From: Haicheng Li <[email protected]>
Date: Sun, 07 Oct 2012 22:14:29 +0800
Use "pch_gbe: " as the prefix in your patch Subject line,
also:
> @@ -26,6 +26,9 @@ if PCH_GBE
> config PCH_PTP
> bool "PCH PTP clock support"
> default n
> + depends on EXPERIMENTAL
> + select PPS
> + select PTP_1588_CLOCK
This patch is corrupted by your email client.
DO NOT resend this patch without first emailing it to yourself
and verifying that you can in fact apply what you receive in
the email cleanly.