netdev - Re: [PATCH net-next 3/4] igb: enable internal PPS for the i210.

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]

Date:	Thu, 20 Nov 2014 21:28:26 +0000
From:	"Keller, Jacob E" <jacob.e.keller@...el.com>
To:	"richardcochran@...il.com" <richardcochran@...il.com>
CC:	"netdev@...r.kernel.org" <netdev@...r.kernel.org>,
	"davem@...emloft.net" <davem@...emloft.net>,
	"Allan, Bruce W" <bruce.w.allan@...el.com>,
	"Ronciak, John" <john.ronciak@...el.com>,
	"Kirsher, Jeffrey T" <jeffrey.t.kirsher@...el.com>,
	"Vick, Matthew" <matthew.vick@...el.com>
Subject: Re: [PATCH net-next 3/4] igb: enable internal PPS for the i210.

On Thu, 2014-11-20 at 10:18 +0100, Richard Cochran wrote:
> On Wed, Nov 19, 2014 at 09:06:19PM +0000, Keller, Jacob E wrote:
> > On Wed, 2014-11-19 at 21:26 +0100, Richard Cochran wrote:
> > > On Wed, Nov 19, 2014 at 07:32:33PM +0000, Keller, Jacob E wrote:
> > > > Good catch :)
> 
> I have not been able to reproduce the crash, and so the cause is not
> what I thought it was. Maybe it was my patch that preserved the
> enabled interrupts in igb_ptp_reset(). I didn't notice that the driver
> frees and reallocates the ptp_clock. I would never do that, myself.
> 

Yea, I think that was a design I tried in the ixgbe driver, but it
really isn't good.

> > I think you need something here, but it should be clearing that register
> > after a MAC reset, so it needs to be re-initialized. I'm not sure if
> > that reset path was used in the same place in the past.
> 
> Okay, lets figure this out. Why is there a PTP reset function at all?
> I don't know, lets see who calls it...
> 
>   Finding functions calling: igb_ptp_reset
>   ----------------------------------------
>   *** drivers/net/ethernet/intel/igb/igb_main.c:
>   igb_reset[2033]                igb_ptp_reset(adapter);
> 
> Easy enough to understand. But who is calling igb_reset?
> 
>   Finding functions calling: igb_reset
>   ------------------------------------
>   *** drivers/net/ethernet/intel/igb/igb_ethtool.c:
>   igb_set_settings[345]          igb_reset(adapter);
>   igb_set_pauseparam[409]        igb_reset(adapter);
>   igb_diag_test[2016]            igb_reset(adapter);
>   igb_set_eee[2729]              igb_reset(adapter);
>   *** drivers/net/ethernet/intel/igb/igb_main.c:
>   igb_down[1814]                 igb_reset(adapter);
>   igb_set_features[2069]         igb_reset(adapter);
>   igb_probe[2526]                igb_reset(adapter);
>   __igb_open[3110]               igb_reset(adapter);
>   igb_watchdog_task[4231]        igb_reset(adapter);
>   igb_change_mtu[5189]           igb_reset(adapter);
>   igb_resume[7460]               igb_reset(adapter);
>   igb_sriov_reinit[7545]         igb_reset(adapter);
>   igb_io_slot_reset[7678]        igb_reset(adapter);
> 
> Wow, that is quite much. So, whenever any random parameter is changed,
> we reset the PTP clock. Great.

Here is the problem. In igb_reset, we call e1000_reset_hw() which will
eventually perform a hardware MAC reset. Yes maybe we shouldn't be
calling igb_reset everywhere.. I cannot answer that, however...

> 
> Really, wouldn't better to reset the clock functions only when
> absolutely necessary?
> 

We *are*. If e1000_reset_hw(hw) is called, the MAC registers will be
reset to their initial values, which includes not having SYSTIME setup,
and not having the outputs configured. There is not much that can be
done to avoid that besides major refactoring of the igb driver to avoid
resetting as much as it does. In most of those cases, I think the reset
is fine, and we actually aren't going to reset that many times during
nominal operation.

Some of the ethtool settings maybe could avoid resets, but I am not
certain during exactly which path they end up calling resets.

Regards,
Jake

> Thanks,
> Richard