* [PATCH] PCI: imx6: fix imprecise abort handler
[not found] <1382056042-7299-1-git-send-email-tharvey@gateworks.com>
@ 2013-10-19 1:33 ` Marek Vasut
2013-10-21 0:56 ` Shawn Guo
[not found] ` <CAHM4w1nyXCuTya+TR7Rxiw0ojmwZHwz9iUmz=cG8z_8h2wKhSw@mail.gmail.com>
0 siblings, 2 replies; 6+ messages in thread
From: Marek Vasut @ 2013-10-19 1:33 UTC (permalink / raw)
To: linux-arm-kernel
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH] PCI: imx6: fix imprecise abort handler
2013-10-19 1:33 ` [PATCH] PCI: imx6: fix imprecise abort handler Marek Vasut
@ 2013-10-21 0:56 ` Shawn Guo
[not found] ` <CAHM4w1nyXCuTya+TR7Rxiw0ojmwZHwz9iUmz=cG8z_8h2wKhSw@mail.gmail.com>
1 sibling, 0 replies; 6+ messages in thread
From: Shawn Guo @ 2013-10-21 0:56 UTC (permalink / raw)
To: linux-arm-kernel
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>
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH] PCI: imx6: fix imprecise abort handler
[not found] ` <CAHM4w1nyXCuTya+TR7Rxiw0ojmwZHwz9iUmz=cG8z_8h2wKhSw@mail.gmail.com>
@ 2013-10-29 19:46 ` Bjorn Helgaas
2013-10-30 14:55 ` Marek Vasut
0 siblings, 1 reply; 6+ messages in thread
From: Bjorn Helgaas @ 2013-10-29 19:46 UTC (permalink / raw)
To: linux-arm-kernel
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH] PCI: imx6: fix imprecise abort handler
2013-10-29 19:46 ` Bjorn Helgaas
@ 2013-10-30 14:55 ` Marek Vasut
2013-11-01 3:31 ` Richard Zhu
0 siblings, 1 reply; 6+ messages in thread
From: Marek Vasut @ 2013-10-30 14:55 UTC (permalink / raw)
To: linux-arm-kernel
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH] PCI: imx6: fix imprecise abort handler
2013-10-30 14:55 ` Marek Vasut
@ 2013-11-01 3:31 ` Richard Zhu
2013-11-01 12:18 ` Marek Vasut
0 siblings, 1 reply; 6+ messages in thread
From: Richard Zhu @ 2013-11-01 3:31 UTC (permalink / raw)
To: linux-arm-kernel
Hi Marek:
> -----Original Message-----
> From: Marek Vasut [mailto:marex at denx.de]
> Sent: Wednesday, October 30, 2013 10:55 PM
> To: Bjorn Helgaas
> Cc: Pratyush Anand; Tim Harvey; linux-pci at vger.kernel.org; linux-arm-
> kernel at 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
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH] PCI: imx6: fix imprecise abort handler
2013-11-01 3:31 ` Richard Zhu
@ 2013-11-01 12:18 ` Marek Vasut
0 siblings, 0 replies; 6+ messages in thread
From: Marek Vasut @ 2013-11-01 12:18 UTC (permalink / raw)
To: linux-arm-kernel
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
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2013-11-01 12:18 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <1382056042-7299-1-git-send-email-tharvey@gateworks.com>
2013-10-19 1:33 ` [PATCH] PCI: imx6: fix imprecise abort handler Marek Vasut
2013-10-21 0:56 ` Shawn Guo
[not found] ` <CAHM4w1nyXCuTya+TR7Rxiw0ojmwZHwz9iUmz=cG8z_8h2wKhSw@mail.gmail.com>
2013-10-29 19:46 ` Bjorn Helgaas
2013-10-30 14:55 ` Marek Vasut
2013-11-01 3:31 ` Richard Zhu
2013-11-01 12:18 ` Marek Vasut
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).