| Message ID | 20230118234612.272916-3-helgaas@kernel.org (mailing list archive) |
|---|---|
| State | Handled Elsewhere |
| Headers | show |
| Series | PCI/AER: Remove redundant Device Control Error Reporting Enable | expand |
On 1/18/2023 3:46 PM, Bjorn Helgaas wrote: > From: Bjorn Helgaas <bhelgaas@google.com> > > pci_enable_pcie_error_reporting() enables the device to send ERR_* > Messages. Since f26e58bf6f54 ("PCI/AER: Enable error reporting when AER is > native"), the PCI core does this for all devices during enumeration. > > Remove the redundant pci_enable_pcie_error_reporting() call from the > driver. Also remove the corresponding pci_disable_pcie_error_reporting() > from the driver .remove() path. > > Note that this doesn't control interrupt generation by the Root Port; that > is controlled by the AER Root Error Command register, which is managed by > the AER service driver. > > Signed-off-by: Bjorn Helgaas <bhelgaas@google.com> > Cc: Jesse Brandeburg <jesse.brandeburg@intel.com> > Cc: Tony Nguyen <anthony.l.nguyen@intel.com> > Cc: intel-wired-lan@lists.osuosl.org > Cc: netdev@vger.kernel.org > --- > drivers/net/ethernet/intel/e1000e/netdev.c | 7 ------- > 1 file changed, 7 deletions(-) Reviewed-by: Tony Nguyen <anthony.l.nguyen@intel.com> > diff --git a/drivers/net/ethernet/intel/e1000e/netdev.c b/drivers/net/ethernet/intel/e1000e/netdev.c > index 04acd1a992fa..e1eb1de88bf9 100644 > --- a/drivers/net/ethernet/intel/e1000e/netdev.c > +++ b/drivers/net/ethernet/intel/e1000e/netdev.c > @@ -7418,9 +7418,6 @@ static int e1000_probe(struct pci_dev *pdev, const struct pci_device_id *ent) > if (err) > goto err_pci_reg; > > - /* AER (Advanced Error Reporting) hooks */ > - pci_enable_pcie_error_reporting(pdev); > - > pci_set_master(pdev); > /* PCI config space info */ > err = pci_save_state(pdev); > @@ -7708,7 +7705,6 @@ static int e1000_probe(struct pci_dev *pdev, const struct pci_device_id *ent) > err_ioremap: > free_netdev(netdev); > err_alloc_etherdev: > - pci_disable_pcie_error_reporting(pdev); > pci_release_mem_regions(pdev); > err_pci_reg: > err_dma: > @@ -7775,9 +7771,6 @@ static void e1000_remove(struct pci_dev *pdev) > > free_netdev(netdev); > > - /* AER disable */ > - pci_disable_pcie_error_reporting(pdev); > - > pci_disable_device(pdev); > } >
[+cc Sathy] On Thu, Jan 19, 2023 at 10:28:16AM -0800, Tony Nguyen wrote: > On 1/18/2023 3:46 PM, Bjorn Helgaas wrote: > > From: Bjorn Helgaas <bhelgaas@google.com> > > > > pci_enable_pcie_error_reporting() enables the device to send ERR_* > > Messages. Since f26e58bf6f54 ("PCI/AER: Enable error reporting when AER is > > native"), the PCI core does this for all devices during enumeration. > > > > Remove the redundant pci_enable_pcie_error_reporting() call from the > > driver. Also remove the corresponding pci_disable_pcie_error_reporting() > > from the driver .remove() path. > > > > Note that this doesn't control interrupt generation by the Root Port; that > > is controlled by the AER Root Error Command register, which is managed by > > the AER service driver. > > > > Signed-off-by: Bjorn Helgaas <bhelgaas@google.com> > > Cc: Jesse Brandeburg <jesse.brandeburg@intel.com> > > Cc: Tony Nguyen <anthony.l.nguyen@intel.com> > > Cc: intel-wired-lan@lists.osuosl.org > > Cc: netdev@vger.kernel.org > > --- > > drivers/net/ethernet/intel/e1000e/netdev.c | 7 ------- > > 1 file changed, 7 deletions(-) > > Reviewed-by: Tony Nguyen <anthony.l.nguyen@intel.com> Thanks a million for taking a look at these, Tony! These driver patches are all independent and have no dependency on the 1/9 PCI/AER patch. What's your opinion on merging these? Should they go via netdev? Should they be squashed into a single patch that does all the Intel drivers at once? I'm happy to squash them and/or merge them via the PCI tree, whatever is easiest. Bjorn
On Wed, 18 Jan 2023 17:46:05 -0600 Bjorn Helgaas wrote: > From: Bjorn Helgaas <bhelgaas@google.com> > > pci_enable_pcie_error_reporting() enables the device to send ERR_* > Messages. Since f26e58bf6f54 ("PCI/AER: Enable error reporting when AER is > native"), the PCI core does this for all devices during enumeration. > > Remove the redundant pci_enable_pcie_error_reporting() call from the > driver. Also remove the corresponding pci_disable_pcie_error_reporting() > from the driver .remove() path. > > Note that this doesn't control interrupt generation by the Root Port; that > is controlled by the AER Root Error Command register, which is managed by > the AER service driver. > > Signed-off-by: Bjorn Helgaas <bhelgaas@google.com> How would you like to route these? Looks like there's no dependency so we can pick them up?
On Thu, 19 Jan 2023 13:31:39 -0800 Tony Nguyen wrote: > > Thanks a million for taking a look at these, Tony! > > > > These driver patches are all independent and have no dependency on the > > 1/9 PCI/AER patch. What's your opinion on merging these? Should they > > go via netdev? Should they be squashed into a single patch that does > > all the Intel drivers at once? > > > > I'm happy to squash them and/or merge them via the PCI tree, whatever > > is easiest. > > Since there's no dependency, IMO, it'd make sense to go through > Intel-wired-lan/netdev. Keeping them per driver is fine. Ah, damn, I spammed Bjorn with the same question because email was pooped most of the day :/ Reportedly not vger, email in general but fool me once... Tony, if you could take these via your tree that'd be best.
On Thu, Jan 19, 2023 at 07:17:35PM -0800, Jakub Kicinski wrote: > On Wed, 18 Jan 2023 17:46:05 -0600 Bjorn Helgaas wrote: > > From: Bjorn Helgaas <bhelgaas@google.com> > > > > pci_enable_pcie_error_reporting() enables the device to send ERR_* > > Messages. Since f26e58bf6f54 ("PCI/AER: Enable error reporting when AER is > > native"), the PCI core does this for all devices during enumeration. > > > > Remove the redundant pci_enable_pcie_error_reporting() call from the > > driver. Also remove the corresponding pci_disable_pcie_error_reporting() > > from the driver .remove() path. > > > > Note that this doesn't control interrupt generation by the Root Port; that > > is controlled by the AER Root Error Command register, which is managed by > > the AER service driver. > > > > Signed-off-by: Bjorn Helgaas <bhelgaas@google.com> > > How would you like to route these? Looks like there's no dependency > so we can pick them up? Right, no dependencies, so you can pick them up. Sounds like you and Tony have it worked out. Thanks! Bjorn
diff --git a/drivers/net/ethernet/intel/e1000e/netdev.c b/drivers/net/ethernet/intel/e1000e/netdev.c index 04acd1a992fa..e1eb1de88bf9 100644 --- a/drivers/net/ethernet/intel/e1000e/netdev.c +++ b/drivers/net/ethernet/intel/e1000e/netdev.c @@ -7418,9 +7418,6 @@ static int e1000_probe(struct pci_dev *pdev, const struct pci_device_id *ent) if (err) goto err_pci_reg; - /* AER (Advanced Error Reporting) hooks */ - pci_enable_pcie_error_reporting(pdev); - pci_set_master(pdev); /* PCI config space info */ err = pci_save_state(pdev); @@ -7708,7 +7705,6 @@ static int e1000_probe(struct pci_dev *pdev, const struct pci_device_id *ent) err_ioremap: free_netdev(netdev); err_alloc_etherdev: - pci_disable_pcie_error_reporting(pdev); pci_release_mem_regions(pdev); err_pci_reg: err_dma: @@ -7775,9 +7771,6 @@ static void e1000_remove(struct pci_dev *pdev) free_netdev(netdev); - /* AER disable */ - pci_disable_pcie_error_reporting(pdev); - pci_disable_device(pdev); }