netdev - RE: [net-next 10/13] igb: Tidy up wrapping for CONFIG_IGB

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <02874ECE860811409154E81DA85FBB5807858A31@ORSMSX105.amr.corp.intel.com>
Date:	Fri, 24 Aug 2012 19:38:38 +0000
From:	"Keller, Jacob E" <jacob.e.keller@...el.com>
To:	Ben Hutchings <bhutchings@...arflare.com>,
	Richard Cochran <richardcochran@...il.com>
CC:	"Vick, Matthew" <matthew.vick@...el.com>,
	"Kirsher, Jeffrey T" <jeffrey.t.kirsher@...el.com>,
	"davem@...emloft.net" <davem@...emloft.net>,
	"netdev@...r.kernel.org" <netdev@...r.kernel.org>,
	"gospo@...hat.com" <gospo@...hat.com>,
	"sassmann@...hat.com" <sassmann@...hat.com>
Subject: RE: [net-next 10/13] igb: Tidy up wrapping for CONFIG_IGB_PTP.

> -----Original Message-----
> From: Ben Hutchings [mailto:bhutchings@...arflare.com]
> Sent: Friday, August 24, 2012 9:51 AM
> To: Richard Cochran
> Cc: Vick, Matthew; Keller, Jacob E; Kirsher, Jeffrey T;
> davem@...emloft.net; netdev@...r.kernel.org; gospo@...hat.com;
> sassmann@...hat.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:
> > On Thu, Aug 23, 2012 at 06:40:25PM +0000, Vick, Matthew wrote:
> > >
> > > 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?
> >
> > 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)
> 
> > 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