From: Alexander Duyck <alexander.duyck@gmail.com> To: Mario Limonciello <mario.limonciello@dell.com> Cc: Jeff Kirsher <jeffrey.t.kirsher@intel.com>, Tony Nguyen <anthony.l.nguyen@intel.com>, intel-wired-lan <intel-wired-lan@lists.osuosl.org>, LKML <linux-kernel@vger.kernel.org>, Linux PM <linux-pm@vger.kernel.org>, Netdev <netdev@vger.kernel.org>, Jakub Kicinski <kuba@kernel.org>, Sasha Netfin <sasha.neftin@intel.com>, Aaron Brown <aaron.f.brown@intel.com>, Stefan Assmann <sassmann@redhat.com>, David Miller <davem@davemloft.net>, David Arcari <darcari@redhat.com>, Yijun Shen <Yijun.Shen@dell.com>, Perry.Yuan@dell.com, anthony.wong@canonical.com Subject: Re: [PATCH v3 2/7] e1000e: Move all S0ix related code into its own source file Date: Fri, 4 Dec 2020 13:25:50 -0800 [thread overview] Message-ID: <CAKgT0Ucz5zDp3fEJFpt1x1e+OcLCxOZVyo5KK5sM_LktbLQH3Q@mail.gmail.com> (raw) In-Reply-To: <20201204200920.133780-3-mario.limonciello@dell.com> On Fri, Dec 4, 2020 at 12:09 PM Mario Limonciello <mario.limonciello@dell.com> wrote: > > Introduce a flag to indicate the device should be using the S0ix > flows and use this flag to run those functions. > > Splitting the code to it's own file will make future heuristics > more self contained. > > Tested-by: Yijun Shen <yijun.shen@dell.com> > Signed-off-by: Mario Limonciello <mario.limonciello@dell.com> One minor issue pointed out below. > --- > drivers/net/ethernet/intel/e1000e/Makefile | 2 +- > drivers/net/ethernet/intel/e1000e/e1000.h | 4 + > drivers/net/ethernet/intel/e1000e/netdev.c | 272 +------------------- > drivers/net/ethernet/intel/e1000e/s0ix.c | 280 +++++++++++++++++++++ > 4 files changed, 290 insertions(+), 268 deletions(-) > create mode 100644 drivers/net/ethernet/intel/e1000e/s0ix.c > > diff --git a/drivers/net/ethernet/intel/e1000e/Makefile b/drivers/net/ethernet/intel/e1000e/Makefile > index 44e58b6e7660..f2332c01f86c 100644 > --- a/drivers/net/ethernet/intel/e1000e/Makefile > +++ b/drivers/net/ethernet/intel/e1000e/Makefile > @@ -9,5 +9,5 @@ obj-$(CONFIG_E1000E) += e1000e.o > > e1000e-objs := 82571.o ich8lan.o 80003es2lan.o \ > mac.o manage.o nvm.o phy.o \ > - param.o ethtool.o netdev.o ptp.o > + param.o ethtool.o netdev.o s0ix.o ptp.o > > diff --git a/drivers/net/ethernet/intel/e1000e/e1000.h b/drivers/net/ethernet/intel/e1000e/e1000.h > index ba7a0f8f6937..b13f956285ae 100644 > --- a/drivers/net/ethernet/intel/e1000e/e1000.h > +++ b/drivers/net/ethernet/intel/e1000e/e1000.h > @@ -436,6 +436,7 @@ s32 e1000e_get_base_timinca(struct e1000_adapter *adapter, u32 *timinca); > #define FLAG2_DFLT_CRC_STRIPPING BIT(12) > #define FLAG2_CHECK_RX_HWTSTAMP BIT(13) > #define FLAG2_CHECK_SYSTIM_OVERFLOW BIT(14) > +#define FLAG2_ENABLE_S0IX_FLOWS BIT(15) > > #define E1000_RX_DESC_PS(R, i) \ > (&(((union e1000_rx_desc_packet_split *)((R).desc))[i])) > @@ -462,6 +463,9 @@ enum latency_range { > extern char e1000e_driver_name[]; > > void e1000e_check_options(struct e1000_adapter *adapter); > +void e1000e_s0ix_entry_flow(struct e1000_adapter *adapter); > +void e1000e_s0ix_exit_flow(struct e1000_adapter *adapter); > +void e1000e_maybe_enable_s0ix(struct e1000_adapter *adapter); > void e1000e_set_ethtool_ops(struct net_device *netdev); > > int e1000e_open(struct net_device *netdev); > diff --git a/drivers/net/ethernet/intel/e1000e/netdev.c b/drivers/net/ethernet/intel/e1000e/netdev.c > index 128ab6898070..cd9839e86615 100644 > --- a/drivers/net/ethernet/intel/e1000e/netdev.c > +++ b/drivers/net/ethernet/intel/e1000e/netdev.c <snip> > static int e1000e_pm_freeze(struct device *dev) > { > struct net_device *netdev = dev_get_drvdata(dev); > @@ -6962,7 +6701,6 @@ static __maybe_unused int e1000e_pm_suspend(struct device *dev) > struct net_device *netdev = pci_get_drvdata(to_pci_dev(dev)); > struct e1000_adapter *adapter = netdev_priv(netdev); > struct pci_dev *pdev = to_pci_dev(dev); > - struct e1000_hw *hw = &adapter->hw; > int rc; > > e1000e_flush_lpic(pdev); > @@ -6974,8 +6712,7 @@ static __maybe_unused int e1000e_pm_suspend(struct device *dev) > e1000e_pm_thaw(dev); > > /* Introduce S0ix implementation */ > - if (hw->mac.type >= e1000_pch_cnp && > - !e1000e_check_me(hw->adapter->pdev->device)) > + if (adapter->flags2 & FLAG2_ENABLE_S0IX_FLOWS) > e1000e_s0ix_entry_flow(adapter); So the placement of this code raises some issues. It isn't a problem with your patch but a bug in the driver that needs to be addressed. I am assuming you only need to perform this flow if you successfully froze the part. However this is doing it in all cases, which is why the e1000e_pm_thaw is being called before you call this code. This is something that should probably be an "else if" rather than a seperate if statement. > > return rc; > @@ -6986,12 +6723,10 @@ static __maybe_unused int e1000e_pm_resume(struct device *dev) > struct net_device *netdev = pci_get_drvdata(to_pci_dev(dev)); > struct e1000_adapter *adapter = netdev_priv(netdev); > struct pci_dev *pdev = to_pci_dev(dev); > - struct e1000_hw *hw = &adapter->hw; > int rc; > > /* Introduce S0ix implementation */ > - if (hw->mac.type >= e1000_pch_cnp && > - !e1000e_check_me(hw->adapter->pdev->device)) > + if (adapter->flags2 & FLAG2_ENABLE_S0IX_FLOWS) > e1000e_s0ix_exit_flow(adapter); > > rc = __e1000_resume(pdev); > @@ -7655,6 +7390,9 @@ static int e1000_probe(struct pci_dev *pdev, const struct pci_device_id *ent) > if (!(adapter->flags & FLAG_HAS_AMT)) > e1000e_get_hw_control(adapter); > > + /* use heuristics to decide whether to enable s0ix flows */ > + e1000e_maybe_enable_s0ix(adapter); > + > strlcpy(netdev->name, "eth%d", sizeof(netdev->name)); > err = register_netdev(netdev); > if (err)
WARNING: multiple messages have this Message-ID (diff)
From: Alexander Duyck <alexander.duyck@gmail.com> To: intel-wired-lan@osuosl.org Subject: [Intel-wired-lan] [PATCH v3 2/7] e1000e: Move all S0ix related code into its own source file Date: Fri, 4 Dec 2020 13:25:50 -0800 [thread overview] Message-ID: <CAKgT0Ucz5zDp3fEJFpt1x1e+OcLCxOZVyo5KK5sM_LktbLQH3Q@mail.gmail.com> (raw) In-Reply-To: <20201204200920.133780-3-mario.limonciello@dell.com> On Fri, Dec 4, 2020 at 12:09 PM Mario Limonciello <mario.limonciello@dell.com> wrote: > > Introduce a flag to indicate the device should be using the S0ix > flows and use this flag to run those functions. > > Splitting the code to it's own file will make future heuristics > more self contained. > > Tested-by: Yijun Shen <yijun.shen@dell.com> > Signed-off-by: Mario Limonciello <mario.limonciello@dell.com> One minor issue pointed out below. > --- > drivers/net/ethernet/intel/e1000e/Makefile | 2 +- > drivers/net/ethernet/intel/e1000e/e1000.h | 4 + > drivers/net/ethernet/intel/e1000e/netdev.c | 272 +------------------- > drivers/net/ethernet/intel/e1000e/s0ix.c | 280 +++++++++++++++++++++ > 4 files changed, 290 insertions(+), 268 deletions(-) > create mode 100644 drivers/net/ethernet/intel/e1000e/s0ix.c > > diff --git a/drivers/net/ethernet/intel/e1000e/Makefile b/drivers/net/ethernet/intel/e1000e/Makefile > index 44e58b6e7660..f2332c01f86c 100644 > --- a/drivers/net/ethernet/intel/e1000e/Makefile > +++ b/drivers/net/ethernet/intel/e1000e/Makefile > @@ -9,5 +9,5 @@ obj-$(CONFIG_E1000E) += e1000e.o > > e1000e-objs := 82571.o ich8lan.o 80003es2lan.o \ > mac.o manage.o nvm.o phy.o \ > - param.o ethtool.o netdev.o ptp.o > + param.o ethtool.o netdev.o s0ix.o ptp.o > > diff --git a/drivers/net/ethernet/intel/e1000e/e1000.h b/drivers/net/ethernet/intel/e1000e/e1000.h > index ba7a0f8f6937..b13f956285ae 100644 > --- a/drivers/net/ethernet/intel/e1000e/e1000.h > +++ b/drivers/net/ethernet/intel/e1000e/e1000.h > @@ -436,6 +436,7 @@ s32 e1000e_get_base_timinca(struct e1000_adapter *adapter, u32 *timinca); > #define FLAG2_DFLT_CRC_STRIPPING BIT(12) > #define FLAG2_CHECK_RX_HWTSTAMP BIT(13) > #define FLAG2_CHECK_SYSTIM_OVERFLOW BIT(14) > +#define FLAG2_ENABLE_S0IX_FLOWS BIT(15) > > #define E1000_RX_DESC_PS(R, i) \ > (&(((union e1000_rx_desc_packet_split *)((R).desc))[i])) > @@ -462,6 +463,9 @@ enum latency_range { > extern char e1000e_driver_name[]; > > void e1000e_check_options(struct e1000_adapter *adapter); > +void e1000e_s0ix_entry_flow(struct e1000_adapter *adapter); > +void e1000e_s0ix_exit_flow(struct e1000_adapter *adapter); > +void e1000e_maybe_enable_s0ix(struct e1000_adapter *adapter); > void e1000e_set_ethtool_ops(struct net_device *netdev); > > int e1000e_open(struct net_device *netdev); > diff --git a/drivers/net/ethernet/intel/e1000e/netdev.c b/drivers/net/ethernet/intel/e1000e/netdev.c > index 128ab6898070..cd9839e86615 100644 > --- a/drivers/net/ethernet/intel/e1000e/netdev.c > +++ b/drivers/net/ethernet/intel/e1000e/netdev.c <snip> > static int e1000e_pm_freeze(struct device *dev) > { > struct net_device *netdev = dev_get_drvdata(dev); > @@ -6962,7 +6701,6 @@ static __maybe_unused int e1000e_pm_suspend(struct device *dev) > struct net_device *netdev = pci_get_drvdata(to_pci_dev(dev)); > struct e1000_adapter *adapter = netdev_priv(netdev); > struct pci_dev *pdev = to_pci_dev(dev); > - struct e1000_hw *hw = &adapter->hw; > int rc; > > e1000e_flush_lpic(pdev); > @@ -6974,8 +6712,7 @@ static __maybe_unused int e1000e_pm_suspend(struct device *dev) > e1000e_pm_thaw(dev); > > /* Introduce S0ix implementation */ > - if (hw->mac.type >= e1000_pch_cnp && > - !e1000e_check_me(hw->adapter->pdev->device)) > + if (adapter->flags2 & FLAG2_ENABLE_S0IX_FLOWS) > e1000e_s0ix_entry_flow(adapter); So the placement of this code raises some issues. It isn't a problem with your patch but a bug in the driver that needs to be addressed. I am assuming you only need to perform this flow if you successfully froze the part. However this is doing it in all cases, which is why the e1000e_pm_thaw is being called before you call this code. This is something that should probably be an "else if" rather than a seperate if statement. > > return rc; > @@ -6986,12 +6723,10 @@ static __maybe_unused int e1000e_pm_resume(struct device *dev) > struct net_device *netdev = pci_get_drvdata(to_pci_dev(dev)); > struct e1000_adapter *adapter = netdev_priv(netdev); > struct pci_dev *pdev = to_pci_dev(dev); > - struct e1000_hw *hw = &adapter->hw; > int rc; > > /* Introduce S0ix implementation */ > - if (hw->mac.type >= e1000_pch_cnp && > - !e1000e_check_me(hw->adapter->pdev->device)) > + if (adapter->flags2 & FLAG2_ENABLE_S0IX_FLOWS) > e1000e_s0ix_exit_flow(adapter); > > rc = __e1000_resume(pdev); > @@ -7655,6 +7390,9 @@ static int e1000_probe(struct pci_dev *pdev, const struct pci_device_id *ent) > if (!(adapter->flags & FLAG_HAS_AMT)) > e1000e_get_hw_control(adapter); > > + /* use heuristics to decide whether to enable s0ix flows */ > + e1000e_maybe_enable_s0ix(adapter); > + > strlcpy(netdev->name, "eth%d", sizeof(netdev->name)); > err = register_netdev(netdev); > if (err)
next prev parent reply other threads:[~2020-12-04 21:26 UTC|newest] Thread overview: 52+ messages / expand[flat|nested] mbox.gz Atom feed top 2020-12-04 20:09 [PATCH v3 0/7] Improve s0ix flows for systems i219LM Mario Limonciello 2020-12-04 20:09 ` [Intel-wired-lan] " Mario Limonciello 2020-12-04 20:09 ` [PATCH v3 1/7] e1000e: fix S0ix flow to allow S0i3.2 subset entry Mario Limonciello 2020-12-04 20:09 ` [Intel-wired-lan] " Mario Limonciello 2020-12-08 17:24 ` Mario Limonciello 2020-12-08 17:24 ` [Intel-wired-lan] " Mario Limonciello 2020-12-08 18:18 ` Jakub Kicinski 2020-12-08 18:18 ` [Intel-wired-lan] " Jakub Kicinski 2020-12-04 20:09 ` [PATCH v3 2/7] e1000e: Move all S0ix related code into its own source file Mario Limonciello 2020-12-04 20:09 ` [Intel-wired-lan] " Mario Limonciello 2020-12-04 21:25 ` Alexander Duyck [this message] 2020-12-04 21:25 ` Alexander Duyck 2020-12-04 20:09 ` [PATCH v3 3/7] e1000e: Export S0ix flags to ethtool Mario Limonciello 2020-12-04 20:09 ` [Intel-wired-lan] " Mario Limonciello 2020-12-04 20:09 ` [PATCH v3 4/7] e1000e: Add Dell's Comet Lake systems into S0ix heuristics Mario Limonciello 2020-12-04 20:09 ` [Intel-wired-lan] " Mario Limonciello 2020-12-04 20:09 ` [PATCH v3 5/7] e1000e: Add more Dell CML " Mario Limonciello 2020-12-04 20:09 ` [Intel-wired-lan] " Mario Limonciello 2020-12-04 20:09 ` [PATCH v3 6/7] e1000e: Add Dell TGL desktop " Mario Limonciello 2020-12-04 20:09 ` [Intel-wired-lan] " Mario Limonciello 2020-12-04 20:09 ` [PATCH v3 7/7] e1000e: Add another Dell TGL notebook system " Mario Limonciello 2020-12-04 20:09 ` [Intel-wired-lan] " Mario Limonciello 2020-12-04 21:27 ` [PATCH v3 0/7] Improve s0ix flows for systems i219LM Alexander Duyck 2020-12-04 21:27 ` [Intel-wired-lan] " Alexander Duyck 2020-12-04 22:28 ` Limonciello, Mario 2020-12-04 22:28 ` [Intel-wired-lan] " Limonciello, Mario 2020-12-04 22:38 ` Alexander Duyck 2020-12-04 22:38 ` [Intel-wired-lan] " Alexander Duyck 2020-12-05 23:49 ` Jakub Kicinski 2020-12-05 23:49 ` [Intel-wired-lan] " Jakub Kicinski 2020-12-06 17:32 ` Alexander Duyck 2020-12-06 17:32 ` [Intel-wired-lan] " Alexander Duyck 2020-12-07 13:28 ` Hans de Goede 2020-12-07 13:28 ` [Intel-wired-lan] " Hans de Goede 2020-12-07 15:41 ` Limonciello, Mario 2020-12-07 15:41 ` [Intel-wired-lan] " Limonciello, Mario 2020-12-08 5:08 ` Neftin, Sasha 2020-12-08 5:08 ` [Intel-wired-lan] " Neftin, Sasha 2020-12-08 9:30 ` Hans de Goede 2020-12-08 9:30 ` [Intel-wired-lan] " Hans de Goede 2020-12-08 16:14 ` Alexander Duyck 2020-12-08 16:14 ` [Intel-wired-lan] " Alexander Duyck 2020-12-08 22:29 ` Limonciello, Mario 2020-12-08 22:29 ` [Intel-wired-lan] " Limonciello, Mario 2020-12-09 14:44 ` Hans de Goede 2020-12-09 14:44 ` [Intel-wired-lan] " Hans de Goede 2020-12-10 2:24 ` Alexander Duyck 2020-12-10 2:24 ` [Intel-wired-lan] " Alexander Duyck 2020-12-10 5:28 ` Neftin, Sasha 2020-12-10 5:28 ` [Intel-wired-lan] " Neftin, Sasha 2020-12-13 8:33 ` Neftin, Sasha 2020-12-13 8:33 ` [Intel-wired-lan] " Neftin, Sasha
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=CAKgT0Ucz5zDp3fEJFpt1x1e+OcLCxOZVyo5KK5sM_LktbLQH3Q@mail.gmail.com \ --to=alexander.duyck@gmail.com \ --cc=Perry.Yuan@dell.com \ --cc=Yijun.Shen@dell.com \ --cc=aaron.f.brown@intel.com \ --cc=anthony.l.nguyen@intel.com \ --cc=anthony.wong@canonical.com \ --cc=darcari@redhat.com \ --cc=davem@davemloft.net \ --cc=intel-wired-lan@lists.osuosl.org \ --cc=jeffrey.t.kirsher@intel.com \ --cc=kuba@kernel.org \ --cc=linux-kernel@vger.kernel.org \ --cc=linux-pm@vger.kernel.org \ --cc=mario.limonciello@dell.com \ --cc=netdev@vger.kernel.org \ --cc=sasha.neftin@intel.com \ --cc=sassmann@redhat.com \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.