RE: [net-next 10/13] igb: Tidy up wrapping for CONFIG_IGB_PTP.

-----Original Message-----
From: Ben Hutchings [mailto:bhutchings@solarflare.com]
Sent: Friday, August 24, 2012 9:51 AM
To: Richard Cochran
Cc: Vick, Matthew; Keller, Jacob E; Kirsher, Jeffrey T;
davem@davemloft.net; netdev@vger.kernel.org; gospo@redhat.com;
sassmann@redhat.com
Subject: Re: [net-next 10/13] igb: Tidy up wrapping for CONFIG_IGB_PTP.

On Fri, 2012-08-24 at 12:10 +0200, Richard Cochran wrote:

quoted

On Thu, Aug 23, 2012 at 06:40:25PM +0000, Vick, Matthew wrote:

quoted

I tend to agree with Jake here--I like having the information. I'm

fine removing them, but I'd like to do it for all CONFIG_IGB_PTP wrapping
if we're going to do it. What do you think, Richard?

quoted

Come to think of it, I never liked the CONFIG_IGB_PTP very much in the
first place. These were added after the fact by Jeff Kirsher. He had
said off list that there was some issue with CONFIG_PTP_1588_CLOCK and
igb as a module, or something like that. At that time I said, just go
ahead and fix it up.

If there is some feature of a driver that depends on an optional
modularisable subsystem, and you want the driver to be built without that
feature when the subsystem is missing, then the feature has to be disabled
when the driver is built-in and the subsystem is a module.  So in general
you need:

config DRIVER_FEATURE
	bool "This is a really neat feature for the driver"
	depends on DRIVER && SUBSYSTEM && !(DRIVER=y && SUBYSTEM=m)

quoted

I think it would be better if the "time stamp all Rx packets" of the
82580 were always available, and that the PHC feature always be
compiled when CONFIG_PTP_1588_CLOCK is selected.

Maybe you could ask Jeff what the issue was, and then see if there is
a way to remove CONFIG_IGB_PTP altogether.

Even if they are to be enabled automatically, CONFIG_IGB_PTP and similar
config symbols are important as shorthand.  So I think what you want is:

config IGB_PTP
	def_bool y
	depends on IGB && PTP_1588_CLOCK && !(IGB=y && PTP_1588_CLOCK=m)

(But currently it's actually *selecting* PTP_1588_CLOCK.  We should
probably have drivers consistently select or depend on it, not a
mixture.)

Ben.

--
Ben Hutchings, Staff Engineer, Solarflare Not speaking for my employer;
that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.

The IGP_PTP is necessary otherwise you have to do something like #if defined(CONFIG_PTP_1588_CLOCK), or #ifdef CONFIG_PTP_1588_CLOCK \ #ifdef CONFIG_PTP_1588_CLOCK_MODULE

The main reason that I wouldn't want to un-wrap the timestamping is because the hwtstamp ioctl is somewhat problematic because it is almost all ptp only. Also, some of the parts for igb driver don't support timestamp all, they only support ptp only packets, and this would be a lot more confusing since I would say still only allow that if ptp is on.. (since those values are useless except with the PHC clock)

- Jake