Re: [net:master 1/9] pch_gbe_main.c:(.text+0x510370): undefined reference to `pch_ch_control_write'

Re: [net:master 1/9] pch_gbe_main.c:(.text+0x510370): undefined reference to `pch_ch_control_write'

[Haicheng Li]

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 <haicheng.lee@gmail.com>

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 <haicheng.lee@gmail.com>
---
  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

Re: [net:master 1/9] pch_gbe_main.c:(.text+0x510370): undefined reference to `pch_ch_control_write'

[David Miller]

From: Haicheng Li <haicheng.li@linux.intel.com>
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.

Re: [net:master 1/9] pch_gbe_main.c:(.text+0x510370): undefined reference to `pch_ch_control_write'

[Haicheng Li]

On 10/06/2012 09:22 PM, David Miller wrote:
> From: Haicheng Li<haicheng.li@linux.intel.com>
> 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.

Re: [net:master 1/9] pch_gbe_main.c:(.text+0x510370): undefined reference to `pch_ch_control_write'

[David Miller]

From: Haicheng Li <haicheng.li@linux.intel.com>
Date: Sat, 06 Oct 2012 22:07:23 +0800

> On 10/06/2012 09:22 PM, David Miller wrote:
>> From: Haicheng Li<haicheng.li@linux.intel.com>
>> 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?

Re: [net:master 1/9] pch_gbe_main.c:(.text+0x510370): undefined reference to `pch_ch_control_write'

[Haicheng Li]

On 10/06/2012 10:21 PM, David Miller wrote:
> From: Haicheng Li<haicheng.li@linux.intel.com>
> Date: Sat, 06 Oct 2012 22:07:23 +0800
>
>> On 10/06/2012 09:22 PM, David Miller wrote:
>>> From: Haicheng Li<haicheng.li@linux.intel.com>
>>> 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?

Re: [net:master 1/9] pch_gbe_main.c:(.text+0x510370): undefined reference to `pch_ch_control_write'

[David Miller]

From: Haicheng Li <haicheng.li@linux.intel.com>
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.

[PATCH] Fix PTP dependencies: explicitly select all the possible dependencies.

[Haicheng Li]

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 <fengguang.wu@intel.com>
Reviewed-by: David S. Miller <davem@davemloft.net>
Signed-off-by: Haicheng Li <haicheng.lee@gmail.com>
---
  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

Re: [PATCH] Fix PTP dependencies: explicitly select all the possible dependencies.

[David Miller]

From: Haicheng Li <haicheng.li@linux.intel.com>
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.

Re: [net:master 1/9] pch_gbe_main.c:(.text+0x510370): undefined reference to `pch_ch_control_write'

[Richard Cochran]

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.

Re: [PATCH] pch_pge: Fix build error by selecting all the possible dependencies.

[Haicheng Li]

Just realized a typo in the subject, pls. ignore this. will send a new soon. 
sorry.

On 10/08/2012 12:29 PM, Haicheng Li wrote:
> Fengguang reported a kernel build failure as folowing:
> 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<fengguang.wu@intel.com>
> Reviewed-by: David S. Miller<davem@davemloft.net>
> Signed-off-by: Haicheng Li<haicheng.lee@gmail.com>
> ---
>   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

Re: [PATCH] pch_gbe: Fix build error by selecting all the possible dependencies.

[David Miller]

This didn't make it to any mailing list, so nobody can review it.

[PATCH] pch_gbe: Fix build error by selecting all the possible dependencies.

[Haicheng Li]

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 <fengguang.wu@intel.com>
Reviewed-by: David S. Miller <davem@davemloft.net>
Signed-off-by: Haicheng Li <haicheng.lee@gmail.com>
---
 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

Re: [PATCH] pch_gbe: Fix build error by selecting all the possible dependencies.

[David Miller]

From: Haicheng Li <haicheng.lee@gmail.com>
Date: Tue,  9 Oct 2012 17:43:12 +0800

> 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 <fengguang.wu@intel.com>
> Reviewed-by: David S. Miller <davem@davemloft.net>
> Signed-off-by: Haicheng Li <haicheng.lee@gmail.com>

Applied, thanks.