From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753573AbcGYR5K (ORCPT ); Mon, 25 Jul 2016 13:57:10 -0400 Received: from mga11.intel.com ([192.55.52.93]:30670 "EHLO mga11.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752843AbcGYR5G (ORCPT ); Mon, 25 Jul 2016 13:57:06 -0400 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.28,420,1464678000"; d="scan'208";a="1013563519" Date: Mon, 25 Jul 2016 10:56:48 -0700 From: Jesse Brandeburg To: Jarod Wilson Cc: , , , jesse.brandeburg@intel.com, raanan.avargil@intel.com Subject: Re: [Intel-wired-lan] [PATCH v2 net-next 0/2] e1000e: fix PTP on e1000_pch_variants Message-ID: <20160725105648.00000dda@unknown> In-Reply-To: <1469292274-59237-1-git-send-email-jarod@redhat.com> References: <1468959902-25071-1-git-send-email-jarod@redhat.com> <1469292274-59237-1-git-send-email-jarod@redhat.com> X-Mailer: Claws Mail 3.7.10cvs7 (GTK+ 2.16.6; i586-pc-mingw32msvc) Mime-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Sat, 23 Jul 2016 12:44:32 -0400 Jarod Wilson wrote: > This little series factors out the systim sanitization code first, then > adds e1000_pch_lpt as a new case in the switch that calls the sanitize > function, fixing PTP clock issues I've had reported against an Intel > I-218V NIC in an Intel NUC5ik5RYH system. > > Jarod Wilson (2): > e1000e: factor out systim sanitization > e1000e: fix PTP on e1000_pch_lpt variants > Thanks for your patch Jarod, the refactor itself is fine and a good idea, and thanks for working on the fix! This code should have been using a feature flag, and the alert that you're having to add more device IDs to the switch statement makes it even more obvious. Please see line 406 of e1000e/e1000.h, where the flags are declared, add a flag for this workaround (to flags2), and and add some code in the e1000_info_tbl entry to set the flag for the appropriate mac(s) Then the runtime code should only check the flag, and if any further devices require the workaround we just add the flag to that device, or if this is init code, just always call the workaround funtion and have the function itself make sure the right flags2 is set or return. The code has somehow gotten away from this model in some places and any new code we add should be doing it the right way. Thanks, Jesse