| Message ID | 1382056042-7299-1-git-send-email-tharvey@gateworks.com (mailing list archive) |
|---|---|
| State | New, archived |
| Delegated to: | Bjorn Helgaas |
| Headers | show |
Dear Tim Harvey, > An imprecise abort is triggered when a port behind a switch is accessed > and no device is present. At enumeration, imprecise aborts are not enabled > thus this ends up getting deferred until the kernel has completed init. At > that point we must not adjust PC - the handler must do nothing, but a > handler must exist. > > This fixes random crashes that occur right after freeing init. > This is against linux-pci/host-imx6. > > Acked-by: Marek Vasut <marex@denx.de> > Tested-by: Marek Vasut <marex@denx.de> > Signed-off-by: Tim Harvey <tharvey@gateworks.com> Expanding CC a bit, let's have more eyes on this. > --- > drivers/pci/host/pci-imx6.c | 6 ------ > 1 file changed, 6 deletions(-) > > diff --git a/drivers/pci/host/pci-imx6.c b/drivers/pci/host/pci-imx6.c > index 966bac6..90fce05 100644 > --- a/drivers/pci/host/pci-imx6.c > +++ b/drivers/pci/host/pci-imx6.c > @@ -200,12 +200,6 @@ static int pcie_phy_write(void __iomem *dbi_base, int > addr, int data) static int imx6q_pcie_abort_handler(unsigned long addr, > unsigned int fsr, struct pt_regs *regs) > { > - /* > - * If it was an imprecise abort, then we need to correct the > - * return address to be _after_ the instruction. > - */ > - if (fsr & (1 << 10)) > - regs->ARM_pc += 4; > return 0; > } Best regards, Marek Vasut -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Sat, Oct 19, 2013 at 03:33:52AM +0200, Marek Vasut wrote: > Dear Tim Harvey, > > > An imprecise abort is triggered when a port behind a switch is accessed > > and no device is present. At enumeration, imprecise aborts are not enabled > > thus this ends up getting deferred until the kernel has completed init. At > > that point we must not adjust PC - the handler must do nothing, but a > > handler must exist. > > > > This fixes random crashes that occur right after freeing init. > > This is against linux-pci/host-imx6. > > > > Acked-by: Marek Vasut <marex@denx.de> > > Tested-by: Marek Vasut <marex@denx.de> > > Signed-off-by: Tim Harvey <tharvey@gateworks.com> > > Expanding CC a bit, let's have more eyes on this. > Acked-by: Shawn Guo <shawn.guo@linaro.org> -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Oct 21, 2013 at 2:16 AM, Pratyush Anand <pratyush.anand@gmail.com> wrote: > Hi, > > On Sat, Oct 19, 2013 at 7:03 AM, Marek Vasut <marex@denx.de> wrote: >> >> Dear Tim Harvey, >> >> > An imprecise abort is triggered when a port behind a switch is accessed >> > and no device is present. At enumeration, imprecise aborts are not >> > enabled > > > > I do not know the complete history. But I do not understand, why > imprecise aborts are not enabled during kernel enumeration. > What happens when PCIe switch is already connected before boot? > pci_common_init will try to read vendor/product ID for a non-existing > function during kernel enumeration itself, which should result in imprecise > external abort. In fact, I had observed with my system that if a switch is > connected to PCIe RC, then abort occurs during kernel initialization itself. > So a proper handling was needed. We called hook_fault_code from > a subsys_init pcie_init (or similar) function, which insured that abort > handler is called whenever, software tries to read vendor/product id of > a non-existing function > >> >> > thus this ends up getting deferred until the kernel has completed init. >> > At >> > that point we must not adjust PC - the handler must do nothing, but a >> > handler must exist. >> > >> > This fixes random crashes that occur right after freeing init. >> > This is against linux-pci/host-imx6. >> > >> > Acked-by: Marek Vasut <marex@denx.de> >> > Tested-by: Marek Vasut <marex@denx.de> >> > Signed-off-by: Tim Harvey <tharvey@gateworks.com> >> >> Expanding CC a bit, let's have more eyes on this. >> >> > --- >> > drivers/pci/host/pci-imx6.c | 6 ------ >> > 1 file changed, 6 deletions(-) >> > >> > diff --git a/drivers/pci/host/pci-imx6.c b/drivers/pci/host/pci-imx6.c >> > index 966bac6..90fce05 100644 >> > --- a/drivers/pci/host/pci-imx6.c >> > +++ b/drivers/pci/host/pci-imx6.c >> > @@ -200,12 +200,6 @@ static int pcie_phy_write(void __iomem *dbi_base, >> > int >> > addr, int data) static int imx6q_pcie_abort_handler(unsigned long addr, >> > unsigned int fsr, struct pt_regs *regs) >> > { >> > - /* >> > - * If it was an imprecise abort, then we need to correct the >> > - * return address to be _after_ the instruction. >> > - */ >> > - if (fsr & (1 << 10)) >> > - regs->ARM_pc += 4; > > > Just for knowledge, is there any case other than read of IDs which might > cause imprecise abort? If not, then this handler can be modified a bit to > insure > that it does not handle any unintended imprecise abort. > > Regards > Pratyush > >> >> > return 0; >> > } I'm sort of dubious about imx6q_pcie_abort_handler() to begin with -- it seems like it assumes that imprecise aborts are either enabled or disabled. Isn't there some way it can *check* whether that's the case, so it won't break if those aborts are enabled at a different point in the future? For now, I'll apply this to my pci/host-imx6 branch for v3.13, but let me know if you want to tweak it somehow. Bjorn -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Dear Bjorn Helgaas, > On Mon, Oct 21, 2013 at 2:16 AM, Pratyush Anand > > <pratyush.anand@gmail.com> wrote: > > Hi, > > > > On Sat, Oct 19, 2013 at 7:03 AM, Marek Vasut <marex@denx.de> wrote: > >> Dear Tim Harvey, > >> > >> > An imprecise abort is triggered when a port behind a switch is > >> > accessed and no device is present. At enumeration, imprecise aborts > >> > are not enabled > > > > I do not know the complete history. But I do not understand, why > > imprecise aborts are not enabled during kernel enumeration. > > What happens when PCIe switch is already connected before boot? > > pci_common_init will try to read vendor/product ID for a non-existing > > function during kernel enumeration itself, which should result in > > imprecise external abort. In fact, I had observed with my system that if > > a switch is connected to PCIe RC, then abort occurs during kernel > > initialization itself. So a proper handling was needed. We called > > hook_fault_code from > > a subsys_init pcie_init (or similar) function, which insured that abort > > handler is called whenever, software tries to read vendor/product id of > > a non-existing function > > > >> > thus this ends up getting deferred until the kernel has completed > >> > init. At > >> > that point we must not adjust PC - the handler must do nothing, but a > >> > handler must exist. > >> > > >> > This fixes random crashes that occur right after freeing init. > >> > This is against linux-pci/host-imx6. > >> > > >> > Acked-by: Marek Vasut <marex@denx.de> > >> > Tested-by: Marek Vasut <marex@denx.de> > >> > Signed-off-by: Tim Harvey <tharvey@gateworks.com> > >> > >> Expanding CC a bit, let's have more eyes on this. > >> > >> > --- > >> > > >> > drivers/pci/host/pci-imx6.c | 6 ------ > >> > 1 file changed, 6 deletions(-) > >> > > >> > diff --git a/drivers/pci/host/pci-imx6.c b/drivers/pci/host/pci-imx6.c > >> > index 966bac6..90fce05 100644 > >> > --- a/drivers/pci/host/pci-imx6.c > >> > +++ b/drivers/pci/host/pci-imx6.c > >> > @@ -200,12 +200,6 @@ static int pcie_phy_write(void __iomem *dbi_base, > >> > int > >> > addr, int data) static int imx6q_pcie_abort_handler(unsigned long > >> > addr, > >> > > >> > unsigned int fsr, struct pt_regs *regs) > >> > > >> > { > >> > > >> > - /* > >> > - * If it was an imprecise abort, then we need to correct the > >> > - * return address to be _after_ the instruction. > >> > - */ > >> > - if (fsr & (1 << 10)) > >> > - regs->ARM_pc += 4; > > > > Just for knowledge, is there any case other than read of IDs which might > > cause imprecise abort? If not, then this handler can be modified a bit to > > insure > > that it does not handle any unintended imprecise abort. > > > > Regards > > Pratyush > > > >> > return 0; > >> > > >> > } > > I'm sort of dubious about imx6q_pcie_abort_handler() to begin with -- > it seems like it assumes that imprecise aborts are either enabled or > disabled. Isn't there some way it can *check* whether that's the > case, so it won't break if those aborts are enabled at a different > point in the future? The best way here would be to find a way to prevent the DWC controller from generating data abort on missing device _at_all_ and just make the read from that portion of config space be 0xffffffff or zeroes . I didn't find any configuration bit that would be able to do it though :( Richard, can you maybe check with FSL if such an option exists on the controller? > For now, I'll apply this to my pci/host-imx6 branch for v3.13, but let > me know if you want to tweak it somehow. > > Bjorn -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Marek: > -----Original Message----- > From: Marek Vasut [mailto:marex@denx.de] > Sent: Wednesday, October 30, 2013 10:55 PM > To: Bjorn Helgaas > Cc: Pratyush Anand; Tim Harvey; linux-pci@vger.kernel.org; linux-arm- > kernel@lists.infradead.org; Sascha Hauer; Zhu Richard-R65037; Shawn Guo; > Jingoo Han; Pratyush Anand; Troy Kisky > Subject: Re: [PATCH] PCI: imx6: fix imprecise abort handler > > Dear Bjorn Helgaas, > > > On Mon, Oct 21, 2013 at 2:16 AM, Pratyush Anand > > > > <pratyush.anand@gmail.com> wrote: > > > Hi, > > > > > > On Sat, Oct 19, 2013 at 7:03 AM, Marek Vasut <marex@denx.de> wrote: > > >> Dear Tim Harvey, > > >> > > >> > An imprecise abort is triggered when a port behind a switch is > > >> > accessed and no device is present. At enumeration, imprecise > > >> > aborts are not enabled > > > > > > I do not know the complete history. But I do not understand, why > > > imprecise aborts are not enabled during kernel enumeration. > > > What happens when PCIe switch is already connected before boot? > > > pci_common_init will try to read vendor/product ID for a > > > non-existing function during kernel enumeration itself, which should > > > result in imprecise external abort. In fact, I had observed with my > > > system that if a switch is connected to PCIe RC, then abort occurs > > > during kernel initialization itself. So a proper handling was > > > needed. We called hook_fault_code from a subsys_init pcie_init (or > > > similar) function, which insured that abort handler is called > > > whenever, software tries to read vendor/product id of a non-existing > > > function > > > > > >> > thus this ends up getting deferred until the kernel has completed > > >> > init. At that point we must not adjust PC - the handler must do > > >> > nothing, but a handler must exist. > > >> > > > >> > This fixes random crashes that occur right after freeing init. > > >> > This is against linux-pci/host-imx6. > > >> > > > >> > Acked-by: Marek Vasut <marex@denx.de> > > >> > Tested-by: Marek Vasut <marex@denx.de> > > >> > Signed-off-by: Tim Harvey <tharvey@gateworks.com> > > >> > > >> Expanding CC a bit, let's have more eyes on this. > > >> > > >> > --- > > >> > > > >> > drivers/pci/host/pci-imx6.c | 6 ------ > > >> > 1 file changed, 6 deletions(-) > > >> > > > >> > diff --git a/drivers/pci/host/pci-imx6.c > > >> > b/drivers/pci/host/pci-imx6.c index 966bac6..90fce05 100644 > > >> > --- a/drivers/pci/host/pci-imx6.c > > >> > +++ b/drivers/pci/host/pci-imx6.c > > >> > @@ -200,12 +200,6 @@ static int pcie_phy_write(void __iomem > > >> > *dbi_base, int addr, int data) static int > > >> > imx6q_pcie_abort_handler(unsigned long addr, > > >> > > > >> > unsigned int fsr, struct pt_regs *regs) > > >> > > > >> > { > > >> > > > >> > - /* > > >> > - * If it was an imprecise abort, then we need to correct the > > >> > - * return address to be _after_ the instruction. > > >> > - */ > > >> > - if (fsr & (1 << 10)) > > >> > - regs->ARM_pc += 4; > > > > > > Just for knowledge, is there any case other than read of IDs which > > > might cause imprecise abort? If not, then this handler can be > > > modified a bit to insure that it does not handle any unintended > > > imprecise abort. > > > > > > Regards > > > Pratyush > > > > > >> > return 0; > > >> > > > >> > } > > > > I'm sort of dubious about imx6q_pcie_abort_handler() to begin with -- > > it seems like it assumes that imprecise aborts are either enabled or > > disabled. Isn't there some way it can *check* whether that's the > > case, so it won't break if those aborts are enabled at a different > > point in the future? > > The best way here would be to find a way to prevent the DWC controller from > generating data abort on missing device _at_all_ and just make the read from > that portion of config space be 0xffffffff or zeroes . I didn't find any > configuration bit that would be able to do it though :( Richard, can you maybe > check with FSL if such an option exists on the controller? > [Richard] Unfortunately, discussed with IC guys, there is no such kind of mechanism. > > For now, I'll apply this to my pci/host-imx6 branch for v3.13, but let > > me know if you want to tweak it somehow. > > > > Bjorn -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Richard, [...] > > > I'm sort of dubious about imx6q_pcie_abort_handler() to begin with -- > > > it seems like it assumes that imprecise aborts are either enabled or > > > disabled. Isn't there some way it can *check* whether that's the > > > case, so it won't break if those aborts are enabled at a different > > > point in the future? > > > > The best way here would be to find a way to prevent the DWC controller > > from generating data abort on missing device _at_all_ and just make the > > read from that portion of config space be 0xffffffff or zeroes . I > > didn't find any configuration bit that would be able to do it though :( > > Richard, can you maybe check with FSL if such an option exists on the > > controller? > > [Richard] Unfortunately, discussed with IC guys, there is no such kind of > mechanism. Do you know or can you please check if there's at least some mechanism to detect whether the data abort was generated by the DWC PCIe or something else ? Best regards, Marek Vasut -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/pci/host/pci-imx6.c b/drivers/pci/host/pci-imx6.c index 966bac6..90fce05 100644 --- a/drivers/pci/host/pci-imx6.c +++ b/drivers/pci/host/pci-imx6.c @@ -200,12 +200,6 @@ static int pcie_phy_write(void __iomem *dbi_base, int addr, int data) static int imx6q_pcie_abort_handler(unsigned long addr, unsigned int fsr, struct pt_regs *regs) { - /* - * If it was an imprecise abort, then we need to correct the - * return address to be _after_ the instruction. - */ - if (fsr & (1 << 10)) - regs->ARM_pc += 4; return 0; }