linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: "Pali Rohár" <pali@kernel.org>
To: Bjorn Helgaas <helgaas@kernel.org>
Cc: "Aaron Ma" <aaron.ma@canonical.com>,
	jesse.brandeburg@intel.com, anthony.l.nguyen@intel.com,
	davem@davemloft.net, kuba@kernel.org,
	intel-wired-lan@lists.osuosl.org, netdev@vger.kernel.org,
	linux-kernel@vger.kernel.org,
	"Krzysztof Wilczyński" <kw@linux.com>,
	linux-pci@vger.kernel.org
Subject: Re: [PATCH 1/2] igc: don't rd/wr iomem when PCI is removed
Date: Wed, 7 Jul 2021 23:53:37 +0200	[thread overview]
Message-ID: <20210707215337.lwbgvb6lxs3gmsbb@pali> (raw)
In-Reply-To: <20210706201241.GA820992@bjorn-Precision-5520>

On Tuesday 06 July 2021 15:12:41 Bjorn Helgaas wrote:
> On Fri, Jul 02, 2021 at 12:51:19PM +0800, Aaron Ma wrote:
> > Check PCI state when rd/wr iomem.
> > Implement wr32 function as rd32 too.
> > 
> > When unplug TBT dock with i225, rd/wr PCI iomem will cause error log:
> > Trace:
> > BUG: unable to handle page fault for address: 000000000000b604
> > Oops: 0000 [#1] SMP NOPTI
> > RIP: 0010:igc_rd32+0x1c/0x90 [igc]
> > Call Trace:
> > igc_ptp_suspend+0x6c/0xa0 [igc]
> > igc_ptp_stop+0x12/0x50 [igc]
> > igc_remove+0x7f/0x1c0 [igc]
> > pci_device_remove+0x3e/0xb0
> > __device_release_driver+0x181/0x240
> > 
> > Signed-off-by: Aaron Ma <aaron.ma@canonical.com>
> > ---
> >  drivers/net/ethernet/intel/igc/igc_main.c | 16 ++++++++++++++++
> >  drivers/net/ethernet/intel/igc/igc_regs.h |  7 ++-----
> >  2 files changed, 18 insertions(+), 5 deletions(-)
> > 
> > diff --git a/drivers/net/ethernet/intel/igc/igc_main.c b/drivers/net/ethernet/intel/igc/igc_main.c
> > index f1adf154ec4a..606b72cb6193 100644
> > --- a/drivers/net/ethernet/intel/igc/igc_main.c
> > +++ b/drivers/net/ethernet/intel/igc/igc_main.c
> > @@ -5292,6 +5292,10 @@ u32 igc_rd32(struct igc_hw *hw, u32 reg)
> >  	u8 __iomem *hw_addr = READ_ONCE(hw->hw_addr);
> >  	u32 value = 0;
> >  
> > +	if (igc->pdev &&
> > +		igc->pdev->error_state == pci_channel_io_perm_failure)
> > +		return 0;
> 
> I don't think this solves the problem.
> 
>   - Driver calls igc_rd32().
> 
>   - "if (pci_channel_io_perm_failure)" evaluates to false (error_state
>     does not indicate an error).
> 
>   - Device is unplugged.
> 
>   - igc_rd32() calls readl(), which performs MMIO read, which fails
>     because the device is no longer present.  readl() returns ~0 on
>     most platforms.
> 
>   - Same page fault occurs.

Hi Bjorn! I think that backtrace show that this error happens when PCIe
hotplug get interrupt that device was unplugged and PCIe hotplug code
calls remove/unbind procedure to stop unplugged driver.

And in this case really does not make sense to try issuing MMIO read,
device is already unplugged.

I looked that PCIe hotplug driver calls pci_dev_set_disconnected() when
this unplug interrupt happens and pci_dev_set_disconnected() just sets
pci_channel_io_perm_failure flag.

drivers/pci/pci.h provides function pci_dev_is_disconnected() which
checks if that flag pci_channel_io_perm_failure is set.

So I think that pci_dev_is_disconnected() is useful and could be
exported also to drivers (like this one) so they can check if
pci_dev_set_disconnected() was called in past and PCI driver is now in
unbind/cleanup/remove state because PCIe device is already disconnected
and not accessible anymore.

But maybe this check should be on other place in driver unbound
procedure and not in general MMIO read function?

> The only way is to check *after* the MMIO read to see whether an error
> occurred.  On most platforms that means checking for ~0 data.  If you
> see that, a PCI error *may* have occurred.
> 
> If you know that ~0 can never be valid, e.g., if you're reading a
> register where ~0 is not a valid value, you know for sure that an
> error has occurred.
> 
> If ~0 might be a valid value, e.g., if you're reading a buffer that
> contains arbitrary data, you have to look harder.   You might read a
> register than cannot contain ~0, and see if you get the data you
> expect.  Or you might read the Vendor ID or something from config
> space.
> 
> >  	value = readl(&hw_addr[reg]);
> >  
> >  	/* reads should not return all F's */
> > @@ -5308,6 +5312,18 @@ u32 igc_rd32(struct igc_hw *hw, u32 reg)
> >  	return value;
> >  }
> >  
> > +void igc_wr32(struct igc_hw *hw, u32 reg, u32 val)
> > +{
> > +	struct igc_adapter *igc = container_of(hw, struct igc_adapter, hw);
> > +	u8 __iomem *hw_addr = READ_ONCE(hw->hw_addr);
> > +
> > +	if (igc->pdev &&
> > +		igc->pdev->error_state == pci_channel_io_perm_failure)
> > +		return;
> > +
> > +	writel((val), &hw_addr[(reg)]);
> > +}
> > +
> >  int igc_set_spd_dplx(struct igc_adapter *adapter, u32 spd, u8 dplx)
> >  {
> >  	struct igc_mac_info *mac = &adapter->hw.mac;
> > diff --git a/drivers/net/ethernet/intel/igc/igc_regs.h b/drivers/net/ethernet/intel/igc/igc_regs.h
> > index cc174853554b..eb4be87d0e8b 100644
> > --- a/drivers/net/ethernet/intel/igc/igc_regs.h
> > +++ b/drivers/net/ethernet/intel/igc/igc_regs.h
> > @@ -260,13 +260,10 @@ struct igc_hw;
> >  u32 igc_rd32(struct igc_hw *hw, u32 reg);
> >  
> >  /* write operations, indexed using DWORDS */
> > -#define wr32(reg, val) \
> > -do { \
> > -	u8 __iomem *hw_addr = READ_ONCE((hw)->hw_addr); \
> > -	writel((val), &hw_addr[(reg)]); \
> > -} while (0)
> > +void igc_wr32(struct igc_hw *hw, u32 reg, u32 val);
> >  
> >  #define rd32(reg) (igc_rd32(hw, reg))
> > +#define wr32(reg, val) (igc_wr32(hw, reg, val))
> >  
> >  #define wrfl() ((void)rd32(IGC_STATUS))
> >  
> > -- 
> > 2.30.2
> > 

  reply	other threads:[~2021-07-07 21:53 UTC|newest]

Thread overview: 27+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-07-02  4:51 Aaron Ma
2021-07-02  4:51 ` [PATCH 2/2] igc: wait for the MAC copy when enabled MAC passthrough Aaron Ma
2021-07-04  5:36   ` [Intel-wired-lan] " Neftin, Sasha
2021-07-05  7:38     ` Aaron Ma
2021-07-05 11:54       ` Neftin, Sasha
2021-07-06  6:46         ` Aaron Ma
2021-07-08  4:24           ` Neftin, Sasha
2021-07-13 13:45             ` Aaron Ma
2021-07-14  9:13               ` Ruinskiy, Dima
2021-07-04 14:28 ` [PATCH 1/2] igc: don't rd/wr iomem when PCI is removed Pali Rohár
2021-07-05  7:23   ` Aaron Ma
2021-07-05 23:02   ` Krzysztof Wilczyński
2021-07-06 14:23     ` Pali Rohár
2021-07-05  7:47 ` Dave Airlie
2021-07-06  6:42   ` Aaron Ma
2021-07-06 20:12 ` Bjorn Helgaas
2021-07-07 21:53   ` Pali Rohár [this message]
2021-07-07 22:10     ` Bjorn Helgaas
2021-07-08  2:04       ` Oliver O'Halloran
2021-07-08 15:45         ` Bjorn Helgaas
2021-07-18 16:31           ` Oliver O'Halloran
2021-07-18 22:50             ` Pali Rohár
2021-07-19  2:49               ` Oliver O'Halloran
2021-07-19  8:13                 ` Pali Rohár
2021-07-20  0:17                 ` Bjorn Helgaas
2021-07-13 13:00 ` [PATCH v2] igc: fix page fault when thunderbolt is unplugged Aaron Ma
2021-08-04 12:06   ` [Intel-wired-lan] " Fuxbrumer, Dvora

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=20210707215337.lwbgvb6lxs3gmsbb@pali \
    --to=pali@kernel.org \
    --cc=aaron.ma@canonical.com \
    --cc=anthony.l.nguyen@intel.com \
    --cc=davem@davemloft.net \
    --cc=helgaas@kernel.org \
    --cc=intel-wired-lan@lists.osuosl.org \
    --cc=jesse.brandeburg@intel.com \
    --cc=kuba@kernel.org \
    --cc=kw@linux.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-pci@vger.kernel.org \
    --cc=netdev@vger.kernel.org \
    --subject='Re: [PATCH 1/2] igc: don'\''t rd/wr iomem when PCI is removed' \
    /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: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).