Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1482012 > unrolled thread

Re: [PATCH 3/3] PCI: Xilinx NWL PCIe: Fix Error for multi function device for legacy interrupts.

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH 3/3] PCI: Xilinx NWL PCIe: Fix Error for multi function  device for legacy interrupts. Bjorn Helgaas <helgaas@kernel.org> - 2016-09-13 00:10 +0200
    Re: [PATCH 3/3] PCI: Xilinx NWL PCIe: Fix Error for multi function  device for legacy interrupts. Marc Zyngier <marc.zyngier@arm.com> - 2016-09-13 09:50 +0200
      Re: [PATCH 3/3] PCI: Xilinx NWL PCIe: Fix Error for multi function  device for legacy interrupts. Bjorn Helgaas <helgaas@kernel.org> - 2016-09-13 17:10 +0200
        Re: [PATCH 3/3] PCI: Xilinx NWL PCIe: Fix Error for multi function  device for legacy interrupts. Bjorn Helgaas <helgaas@kernel.org> - 2016-09-13 17:40 +0200
          Re: [PATCH 3/3] PCI: Xilinx NWL PCIe: Fix Error for multi function  device for legacy interrupts. Thomas Petazzoni <thomas.petazzoni@free-electrons.com> - 2016-09-13 18:00 +0200
          Re: [PATCH 3/3] PCI: Xilinx NWL PCIe: Fix Error for multi function  device for legacy interrupts. Kishon Vijay Abraham I <kishon@ti.com> - 2016-09-14 12:00 +0200

#1482012 — Re: [PATCH 3/3] PCI: Xilinx NWL PCIe: Fix Error for multi function device for legacy interrupts.

On Thu, Sep 01, 2016 at 05:19:55AM +0000, Bharat Kumar Gogada wrote:
> > >>>> Hi Bharat,
> > >>>>> @@ -561,7 +561,7 @@ static int nwl_pcie_init_irq_domain(struct
> > >>>>> nwl_pcie
> > >>>> *pcie)
> > >>>>>     }
> > >>>>>
> > >>>>>     pcie->legacy_irq_domain = irq_domain_add_linear(legacy_intc_node,
> > >>>>> -                                                   INTX_NUM,
> > >>>>> +                                                   INTX_NUM + 1,
> > >>>>>                                                     &legacy_domain_ops,
> > >>>>>                                                     pcie);
> > >>>>
> > >>>> This feels like the wrong thing to do. You have INTX_NUM irqs, so
> > >>>> the domain allocation should reflect this. On the other hand, the
> > >>>> way the driver currently deals with mappings is quite broken
> > >>>> (consistently adding 1 to
> > >> the HW interrupt).
> > >>>>
> > >>> Hi Marc,
> > >>>
> > >>> Without above change I get following crash in kernel while booting.
> > >>>
> > >>> [    2.441684] error: hwirq 0x4 is too large for dummy
> > >>>
> > >>> [    2.441694] ------------[ cut here ]------------
> > >>>
> > >>> [    2.441698] WARNING: at kernel/irq/irqdomain.c:344
> > >>>
> > >>> [    2.441702] Modules linked in:
> > >>>
> > >>> [    2.441706]
> > >>>
> > >>> [    2.441714] CPU: 1 PID: 1 Comm: swapper/0 Not tainted 4.4.0 #8
> > >>>
> > >>> [    2.441718] Hardware name: xlnx,zynqmp (DT)
> > >>>
> > >>> [    2.441723] task: ffffffc071886b80 ti: ffffffc071888000 task.ti:
> > >> ffffffc071888000
> > >>>
> > >>> [    2.441732] PC is at irq_domain_associate+0x138/0x1c0
> > >>>
> > >>> [    2.441738] LR is at irq_domain_associate+0x138/0x1c0
> > >>>
> > >>> In kernel/irq/irqdomain.c function irq_domain_associate
> > >>>
> > >>> if (WARN(hwirq >= domain->hwirq_max,
> > >>>                  "error: hwirq 0x%x is too large for %s\n", (int)hwirq, domain-
> > >name))
> > >>>                 return -EINVAL;
> > >>>
> > >>> Here the hwirq and hwirq_max are equal to 4 without the above
> > >>> condition
> > >> (INTX_NUM + 1) due to which crash is coming.
> > >>> This is happening as the legacy interrupts are starting from 1 (INTA).
> > >>
> > >> I understood that. I'm still persisting in saying that you have the wrong fix.
> > >>
> > >> Your domain should always allocate many interrupts as you have
> > >> interrupt sources. These interrupts (hwirq) should be numbered from 0 to (n-
> > 1).
> > >
> > > Agreed, but here comes the problem the hwirq for legacy interrupts
> > > will start at 0x1 to 0x4 (INTA to INTD) and these values are as per
> > > PCIe specification for legacy interrupts. So these cannot be numbered
> > > from 0. So when 0x4 (INTD) for a multi-function device comes the crash
> > > occurs.
> > 
> > So who provides this hwirq? Who calls irq_domain_associate() with hwirq set to
> > 4?
> > 
> PCIe subsystem invokes pcibios_add_device function in arch/arm64/kernel/pci.c for every pci device.
> The purpose of this function is to assign dev->irq using of_irq_parse_and_map_pci.
> of_irq_parse_and_map_pci invokes of_irq_parse_pci where it reads PCI_INTERRUPT_PIN from configuration space and saves it
> in parameter of struct of_phandle_args.
> This structure is passed to irq_create_of_mapping where it invokes irq_create_fwspec_mapping.
> irq_create_fwspec_mapping invokes irq_domain_translate and gets hwirq, here the above saved PCI_INTERRUPT_PIN value is assigned 
> to hwirq (*hwirq = fwspec->param[0]).
> And then using this hwirq irq_create_mapping -> irq_domain_associate were invoked and mapping is created for virtual irq with this hwirq.
> So for any end point PCI_INTERRUPT_PIN value starts from 0x1 to 0x4 and so hwirq starts from 0x1 to 0x4.
> 
> So the values are more generic w.r.t to protocol, that's why hwirq will range from 0x1 to 0x4. 
> And then if you check pcie-altera.c they are doing this adding one in their handler and while creating legacy domain.

Is this resolved yet?  Marc, are you happy, or should we iterate on this
again?

[toc] | [next] | [standalone]

#1482213

On 12/09/16 23:02, Bjorn Helgaas wrote:
> On Thu, Sep 01, 2016 at 05:19:55AM +0000, Bharat Kumar Gogada wrote:
>>>>>>> Hi Bharat,
>>>>>>>> @@ -561,7 +561,7 @@ static int nwl_pcie_init_irq_domain(struct
>>>>>>>> nwl_pcie
>>>>>>> *pcie)
>>>>>>>>     }
>>>>>>>>
>>>>>>>>     pcie->legacy_irq_domain = irq_domain_add_linear(legacy_intc_node,
>>>>>>>> -                                                   INTX_NUM,
>>>>>>>> +                                                   INTX_NUM + 1,
>>>>>>>>                                                     &legacy_domain_ops,
>>>>>>>>                                                     pcie);
>>>>>>>
>>>>>>> This feels like the wrong thing to do. You have INTX_NUM irqs, so
>>>>>>> the domain allocation should reflect this. On the other hand, the
>>>>>>> way the driver currently deals with mappings is quite broken
>>>>>>> (consistently adding 1 to
>>>>> the HW interrupt).
>>>>>>>
>>>>>> Hi Marc,
>>>>>>
>>>>>> Without above change I get following crash in kernel while booting.
>>>>>>
>>>>>> [    2.441684] error: hwirq 0x4 is too large for dummy
>>>>>>
>>>>>> [    2.441694] ------------[ cut here ]------------
>>>>>>
>>>>>> [    2.441698] WARNING: at kernel/irq/irqdomain.c:344
>>>>>>
>>>>>> [    2.441702] Modules linked in:
>>>>>>
>>>>>> [    2.441706]
>>>>>>
>>>>>> [    2.441714] CPU: 1 PID: 1 Comm: swapper/0 Not tainted 4.4.0 #8
>>>>>>
>>>>>> [    2.441718] Hardware name: xlnx,zynqmp (DT)
>>>>>>
>>>>>> [    2.441723] task: ffffffc071886b80 ti: ffffffc071888000 task.ti:
>>>>> ffffffc071888000
>>>>>>
>>>>>> [    2.441732] PC is at irq_domain_associate+0x138/0x1c0
>>>>>>
>>>>>> [    2.441738] LR is at irq_domain_associate+0x138/0x1c0
>>>>>>
>>>>>> In kernel/irq/irqdomain.c function irq_domain_associate
>>>>>>
>>>>>> if (WARN(hwirq >= domain->hwirq_max,
>>>>>>                  "error: hwirq 0x%x is too large for %s\n", (int)hwirq, domain-
>>>> name))
>>>>>>                 return -EINVAL;
>>>>>>
>>>>>> Here the hwirq and hwirq_max are equal to 4 without the above
>>>>>> condition
>>>>> (INTX_NUM + 1) due to which crash is coming.
>>>>>> This is happening as the legacy interrupts are starting from 1 (INTA).
>>>>>
>>>>> I understood that. I'm still persisting in saying that you have the wrong fix.
>>>>>
>>>>> Your domain should always allocate many interrupts as you have
>>>>> interrupt sources. These interrupts (hwirq) should be numbered from 0 to (n-
>>> 1).
>>>>
>>>> Agreed, but here comes the problem the hwirq for legacy interrupts
>>>> will start at 0x1 to 0x4 (INTA to INTD) and these values are as per
>>>> PCIe specification for legacy interrupts. So these cannot be numbered
>>>> from 0. So when 0x4 (INTD) for a multi-function device comes the crash
>>>> occurs.
>>>
>>> So who provides this hwirq? Who calls irq_domain_associate() with hwirq set to
>>> 4?
>>>
>> PCIe subsystem invokes pcibios_add_device function in arch/arm64/kernel/pci.c for every pci device.
>> The purpose of this function is to assign dev->irq using of_irq_parse_and_map_pci.
>> of_irq_parse_and_map_pci invokes of_irq_parse_pci where it reads PCI_INTERRUPT_PIN from configuration space and saves it
>> in parameter of struct of_phandle_args.
>> This structure is passed to irq_create_of_mapping where it invokes irq_create_fwspec_mapping.
>> irq_create_fwspec_mapping invokes irq_domain_translate and gets hwirq, here the above saved PCI_INTERRUPT_PIN value is assigned 
>> to hwirq (*hwirq = fwspec->param[0]).
>> And then using this hwirq irq_create_mapping -> irq_domain_associate were invoked and mapping is created for virtual irq with this hwirq.
>> So for any end point PCI_INTERRUPT_PIN value starts from 0x1 to 0x4 and so hwirq starts from 0x1 to 0x4.
>>
>> So the values are more generic w.r.t to protocol, that's why hwirq will range from 0x1 to 0x4. 
>> And then if you check pcie-altera.c they are doing this adding one in their handler and while creating legacy domain.
> 
> Is this resolved yet?  Marc, are you happy, or should we iterate on this
> again?


Ah, sorry to have dropped the ball on this patch.

I guess that given that the infrastructure imposes the hwirq range on
the host drivers, Bharat's approach is the only way (and a number of
other host drivers are already slightly broken). I'll try and have a
look at solving this at the generic level. In the meantime:

Acked-by: Marc Zyngier <marc.zyngier@arm.com>

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny...

[toc] | [prev] | [next] | [standalone]

#1482570

On Tue, Sep 13, 2016 at 08:41:28AM +0100, Marc Zyngier wrote:
> On 12/09/16 23:02, Bjorn Helgaas wrote:
> > On Thu, Sep 01, 2016 at 05:19:55AM +0000, Bharat Kumar Gogada wrote:
> >>>>>>> Hi Bharat,
> >>>>>>>> @@ -561,7 +561,7 @@ static int nwl_pcie_init_irq_domain(struct
> >>>>>>>> nwl_pcie
> >>>>>>> *pcie)
> >>>>>>>>     }
> >>>>>>>>
> >>>>>>>>     pcie->legacy_irq_domain = irq_domain_add_linear(legacy_intc_node,
> >>>>>>>> -                                                   INTX_NUM,
> >>>>>>>> +                                                   INTX_NUM + 1,
> >>>>>>>>                                                     &legacy_domain_ops,
> >>>>>>>>                                                     pcie);
> >>>>>>>
> >>>>>>> This feels like the wrong thing to do. You have INTX_NUM irqs, so
> >>>>>>> the domain allocation should reflect this. On the other hand, the
> >>>>>>> way the driver currently deals with mappings is quite broken
> >>>>>>> (consistently adding 1 to
> >>>>> the HW interrupt).
> >>>>>>>
> >>>>>> Hi Marc,
> >>>>>>
> >>>>>> Without above change I get following crash in kernel while booting.
> >>>>>>
> >>>>>> [    2.441684] error: hwirq 0x4 is too large for dummy
> >>>>>>
> >>>>>> [    2.441694] ------------[ cut here ]------------
> >>>>>>
> >>>>>> [    2.441698] WARNING: at kernel/irq/irqdomain.c:344
> >>>>>>
> >>>>>> [    2.441702] Modules linked in:
> >>>>>>
> >>>>>> [    2.441706]
> >>>>>>
> >>>>>> [    2.441714] CPU: 1 PID: 1 Comm: swapper/0 Not tainted 4.4.0 #8
> >>>>>>
> >>>>>> [    2.441718] Hardware name: xlnx,zynqmp (DT)
> >>>>>>
> >>>>>> [    2.441723] task: ffffffc071886b80 ti: ffffffc071888000 task.ti:
> >>>>> ffffffc071888000
> >>>>>>
> >>>>>> [    2.441732] PC is at irq_domain_associate+0x138/0x1c0
> >>>>>>
> >>>>>> [    2.441738] LR is at irq_domain_associate+0x138/0x1c0
> >>>>>>
> >>>>>> In kernel/irq/irqdomain.c function irq_domain_associate
> >>>>>>
> >>>>>> if (WARN(hwirq >= domain->hwirq_max,
> >>>>>>                  "error: hwirq 0x%x is too large for %s\n", (int)hwirq, domain-
> >>>> name))
> >>>>>>                 return -EINVAL;
> >>>>>>
> >>>>>> Here the hwirq and hwirq_max are equal to 4 without the above
> >>>>>> condition
> >>>>> (INTX_NUM + 1) due to which crash is coming.
> >>>>>> This is happening as the legacy interrupts are starting from 1 (INTA).
> >>>>>
> >>>>> I understood that. I'm still persisting in saying that you have the wrong fix.
> >>>>>
> >>>>> Your domain should always allocate many interrupts as you have
> >>>>> interrupt sources. These interrupts (hwirq) should be numbered from 0 to (n-
> >>> 1).
> >>>>
> >>>> Agreed, but here comes the problem the hwirq for legacy interrupts
> >>>> will start at 0x1 to 0x4 (INTA to INTD) and these values are as per
> >>>> PCIe specification for legacy interrupts. So these cannot be numbered
> >>>> from 0. So when 0x4 (INTD) for a multi-function device comes the crash
> >>>> occurs.
> >>>
> >>> So who provides this hwirq? Who calls irq_domain_associate() with hwirq set to
> >>> 4?
> >>>
> >> PCIe subsystem invokes pcibios_add_device function in arch/arm64/kernel/pci.c for every pci device.
> >> The purpose of this function is to assign dev->irq using of_irq_parse_and_map_pci.
> >> of_irq_parse_and_map_pci invokes of_irq_parse_pci where it reads PCI_INTERRUPT_PIN from configuration space and saves it
> >> in parameter of struct of_phandle_args.
> >> This structure is passed to irq_create_of_mapping where it invokes irq_create_fwspec_mapping.
> >> irq_create_fwspec_mapping invokes irq_domain_translate and gets hwirq, here the above saved PCI_INTERRUPT_PIN value is assigned 
> >> to hwirq (*hwirq = fwspec->param[0]).
> >> And then using this hwirq irq_create_mapping -> irq_domain_associate were invoked and mapping is created for virtual irq with this hwirq.
> >> So for any end point PCI_INTERRUPT_PIN value starts from 0x1 to 0x4 and so hwirq starts from 0x1 to 0x4.
> >>
> >> So the values are more generic w.r.t to protocol, that's why hwirq will range from 0x1 to 0x4. 
> >> And then if you check pcie-altera.c they are doing this adding one in their handler and while creating legacy domain.
> > 
> > Is this resolved yet?  Marc, are you happy, or should we iterate on this
> > again?
> 
> Ah, sorry to have dropped the ball on this patch.

No problem, I wasn't making forward progress anyway.

> I guess that given that the infrastructure imposes the hwirq range on
> the host drivers, Bharat's approach is the only way (and a number of
> other host drivers are already slightly broken). I'll try and have a
> look at solving this at the generic level. In the meantime:
> 
> Acked-by: Marc Zyngier <marc.zyngier@arm.com>

After looking at this myself, I'm not happy with this either.  It feels
like there are bugs lurking here and we're just hiding one of them.

Here are the callers of irq_domain_add_linear() for legacy INTx in
drivers/pci/host:

  advk_pcie_init_irq_domain    LEGACY_IRQ_NUM   (4)
  dra7xx_pcie_init_irq_domain  4
  ks_dw_pcie_host_init         MAX_LEGACY_IRQS  (4)
  altera_pcie_init_irq_domain  INTX_NUM + 1     (5)
  nwl_pcie_init_irq_domain     INTX_NUM + 1     (5)
  xilinx_pcie_init_irq_domain  4

I think all of these use the of_irq_parse_and_map_pci() path you
mentioned, so if the problem is in the way that path works, I would
think these should *all* be requesting the same number of interrupts
in the domain.

I agree with Marc that we should request 4 IRQs, because that's what
we need.  If we can't do that for some reason, we ought to at least
make all these callers the same.

Bjorn

[toc] | [prev] | [next] | [standalone]

#1482596

[+cc Ley Foon (altera), Thomas (aardvark), Kishon (dra7xx), Murali (keystone)]

On Tue, Sep 13, 2016 at 10:05:11AM -0500, Bjorn Helgaas wrote:
> On Tue, Sep 13, 2016 at 08:41:28AM +0100, Marc Zyngier wrote:
> > On 12/09/16 23:02, Bjorn Helgaas wrote:
> > > On Thu, Sep 01, 2016 at 05:19:55AM +0000, Bharat Kumar Gogada wrote:
> > >>>>>>> Hi Bharat,
> > >>>>>>>> @@ -561,7 +561,7 @@ static int nwl_pcie_init_irq_domain(struct
> > >>>>>>>> nwl_pcie
> > >>>>>>> *pcie)
> > >>>>>>>>     }
> > >>>>>>>>
> > >>>>>>>>     pcie->legacy_irq_domain = irq_domain_add_linear(legacy_intc_node,
> > >>>>>>>> -                                                   INTX_NUM,
> > >>>>>>>> +                                                   INTX_NUM + 1,
> > >>>>>>>>                                                     &legacy_domain_ops,
> > >>>>>>>>                                                     pcie);
> > >>>>>>>
> > >>>>>>> This feels like the wrong thing to do. You have INTX_NUM irqs, so
> > >>>>>>> the domain allocation should reflect this. On the other hand, the
> > >>>>>>> way the driver currently deals with mappings is quite broken
> > >>>>>>> (consistently adding 1 to
> > >>>>> the HW interrupt).
> > >>>>>>>
> > >>>>>> Hi Marc,
> > >>>>>>
> > >>>>>> Without above change I get following crash in kernel while booting.
> > >>>>>>
> > >>>>>> [    2.441684] error: hwirq 0x4 is too large for dummy
> > >>>>>>
> > >>>>>> [    2.441694] ------------[ cut here ]------------
> > >>>>>>
> > >>>>>> [    2.441698] WARNING: at kernel/irq/irqdomain.c:344
> > >>>>>>
> > >>>>>> [    2.441702] Modules linked in:
> > >>>>>>
> > >>>>>> [    2.441706]
> > >>>>>>
> > >>>>>> [    2.441714] CPU: 1 PID: 1 Comm: swapper/0 Not tainted 4.4.0 #8
> > >>>>>>
> > >>>>>> [    2.441718] Hardware name: xlnx,zynqmp (DT)
> > >>>>>>
> > >>>>>> [    2.441723] task: ffffffc071886b80 ti: ffffffc071888000 task.ti:
> > >>>>> ffffffc071888000
> > >>>>>>
> > >>>>>> [    2.441732] PC is at irq_domain_associate+0x138/0x1c0
> > >>>>>>
> > >>>>>> [    2.441738] LR is at irq_domain_associate+0x138/0x1c0
> > >>>>>>
> > >>>>>> In kernel/irq/irqdomain.c function irq_domain_associate
> > >>>>>>
> > >>>>>> if (WARN(hwirq >= domain->hwirq_max,
> > >>>>>>                  "error: hwirq 0x%x is too large for %s\n", (int)hwirq, domain-
> > >>>> name))
> > >>>>>>                 return -EINVAL;
> > >>>>>>
> > >>>>>> Here the hwirq and hwirq_max are equal to 4 without the above
> > >>>>>> condition
> > >>>>> (INTX_NUM + 1) due to which crash is coming.
> > >>>>>> This is happening as the legacy interrupts are starting from 1 (INTA).
> > >>>>>
> > >>>>> I understood that. I'm still persisting in saying that you have the wrong fix.
> > >>>>>
> > >>>>> Your domain should always allocate many interrupts as you have
> > >>>>> interrupt sources. These interrupts (hwirq) should be numbered from 0 to (n-
> > >>> 1).
> > >>>>
> > >>>> Agreed, but here comes the problem the hwirq for legacy interrupts
> > >>>> will start at 0x1 to 0x4 (INTA to INTD) and these values are as per
> > >>>> PCIe specification for legacy interrupts. So these cannot be numbered
> > >>>> from 0. So when 0x4 (INTD) for a multi-function device comes the crash
> > >>>> occurs.
> > >>>
> > >>> So who provides this hwirq? Who calls irq_domain_associate() with hwirq set to
> > >>> 4?
> > >>>
> > >> PCIe subsystem invokes pcibios_add_device function in arch/arm64/kernel/pci.c for every pci device.
> > >> The purpose of this function is to assign dev->irq using of_irq_parse_and_map_pci.
> > >> of_irq_parse_and_map_pci invokes of_irq_parse_pci where it reads PCI_INTERRUPT_PIN from configuration space and saves it
> > >> in parameter of struct of_phandle_args.
> > >> This structure is passed to irq_create_of_mapping where it invokes irq_create_fwspec_mapping.
> > >> irq_create_fwspec_mapping invokes irq_domain_translate and gets hwirq, here the above saved PCI_INTERRUPT_PIN value is assigned 
> > >> to hwirq (*hwirq = fwspec->param[0]).
> > >> And then using this hwirq irq_create_mapping -> irq_domain_associate were invoked and mapping is created for virtual irq with this hwirq.
> > >> So for any end point PCI_INTERRUPT_PIN value starts from 0x1 to 0x4 and so hwirq starts from 0x1 to 0x4.
> > >>
> > >> So the values are more generic w.r.t to protocol, that's why hwirq will range from 0x1 to 0x4. 
> > >> And then if you check pcie-altera.c they are doing this adding one in their handler and while creating legacy domain.
> > > 
> > > Is this resolved yet?  Marc, are you happy, or should we iterate on this
> > > again?
> > 
> > Ah, sorry to have dropped the ball on this patch.
> 
> No problem, I wasn't making forward progress anyway.
> 
> > I guess that given that the infrastructure imposes the hwirq range on
> > the host drivers, Bharat's approach is the only way (and a number of
> > other host drivers are already slightly broken). I'll try and have a
> > look at solving this at the generic level. In the meantime:
> > 
> > Acked-by: Marc Zyngier <marc.zyngier@arm.com>
> 
> After looking at this myself, I'm not happy with this either.  It feels
> like there are bugs lurking here and we're just hiding one of them.
> 
> Here are the callers of irq_domain_add_linear() for legacy INTx in
> drivers/pci/host:
> 
>   advk_pcie_init_irq_domain    LEGACY_IRQ_NUM   (4)
>   dra7xx_pcie_init_irq_domain  4
>   ks_dw_pcie_host_init         MAX_LEGACY_IRQS  (4)
>   altera_pcie_init_irq_domain  INTX_NUM + 1     (5)
>   nwl_pcie_init_irq_domain     INTX_NUM + 1     (5)
>   xilinx_pcie_init_irq_domain  4

The altera change corresponding to this was 99496bd2971f ("PCI: altera: Fix
error when INTx is 4").  I should have noticed this inconsistency back
then.

Are aardvark, dra7xx, keystone, and xilinx (non-NWL) broken because they
only request 4 IRQs and only INTA, INTB, and INTC work?

> I think all of these use the of_irq_parse_and_map_pci() path you
> mentioned, so if the problem is in the way that path works, I would
> think these should *all* be requesting the same number of interrupts
> in the domain.
> 
> I agree with Marc that we should request 4 IRQs, because that's what
> we need.  If we can't do that for some reason, we ought to at least
> make all these callers the same.
> 
> 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

[toc] | [prev] | [next] | [standalone]

#1482609

Hello,

On Tue, 13 Sep 2016 10:34:02 -0500, Bjorn Helgaas wrote:

> > After looking at this myself, I'm not happy with this either.  It feels
> > like there are bugs lurking here and we're just hiding one of them.
> > 
> > Here are the callers of irq_domain_add_linear() for legacy INTx in
> > drivers/pci/host:
> > 
> >   advk_pcie_init_irq_domain    LEGACY_IRQ_NUM   (4)
> >   dra7xx_pcie_init_irq_domain  4
> >   ks_dw_pcie_host_init         MAX_LEGACY_IRQS  (4)
> >   altera_pcie_init_irq_domain  INTX_NUM + 1     (5)
> >   nwl_pcie_init_irq_domain     INTX_NUM + 1     (5)
> >   xilinx_pcie_init_irq_domain  4  
> 
> The altera change corresponding to this was 99496bd2971f ("PCI: altera: Fix
> error when INTx is 4").  I should have noticed this inconsistency back
> then.
> 
> Are aardvark, dra7xx, keystone, and xilinx (non-NWL) broken because they
> only request 4 IRQs and only INTA, INTB, and INTC work?
> 
> > I think all of these use the of_irq_parse_and_map_pci() path you
> > mentioned, so if the problem is in the way that path works, I would
> > think these should *all* be requesting the same number of interrupts
> > in the domain.
> > 
> > I agree with Marc that we should request 4 IRQs, because that's what
> > we need.  If we can't do that for some reason, we ought to at least
> > make all these callers the same.

Thanks for Cc'ing about this issue. Indeed, the Aardvark driver
supports all of INT{A,B,C,D}, so the current situation doesn't work. As
suggested, the simplest solution is to just allocate an irq domain with
5 IRQs, like is done in the Altera driver.

However, my feeling is that a more correct solution would be to have a
translation between the PCI_INTERRUPT_PIN value (in the range 0x1 to
0x4) to the hwirq value (in the range 0x0 to 0x3).

Best regards,

Thomas
-- 
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

[toc] | [prev] | [next] | [standalone]

#1483140

Hi,

On Tuesday 13 September 2016 09:04 PM, Bjorn Helgaas wrote:
> [+cc Ley Foon (altera), Thomas (aardvark), Kishon (dra7xx), Murali (keystone)]
> 
> On Tue, Sep 13, 2016 at 10:05:11AM -0500, Bjorn Helgaas wrote:
>> On Tue, Sep 13, 2016 at 08:41:28AM +0100, Marc Zyngier wrote:
>>> On 12/09/16 23:02, Bjorn Helgaas wrote:
>>>> On Thu, Sep 01, 2016 at 05:19:55AM +0000, Bharat Kumar Gogada wrote:
>>>>>>>>>> Hi Bharat,
>>>>>>>>>>> @@ -561,7 +561,7 @@ static int nwl_pcie_init_irq_domain(struct
>>>>>>>>>>> nwl_pcie
>>>>>>>>>> *pcie)
>>>>>>>>>>>     }
>>>>>>>>>>>
>>>>>>>>>>>     pcie->legacy_irq_domain = irq_domain_add_linear(legacy_intc_node,
>>>>>>>>>>> -                                                   INTX_NUM,
>>>>>>>>>>> +                                                   INTX_NUM + 1,
>>>>>>>>>>>                                                     &legacy_domain_ops,
>>>>>>>>>>>                                                     pcie);
>>>>>>>>>>
>>>>>>>>>> This feels like the wrong thing to do. You have INTX_NUM irqs, so
>>>>>>>>>> the domain allocation should reflect this. On the other hand, the
>>>>>>>>>> way the driver currently deals with mappings is quite broken
>>>>>>>>>> (consistently adding 1 to
>>>>>>>> the HW interrupt).
>>>>>>>>>>
>>>>>>>>> Hi Marc,
>>>>>>>>>
>>>>>>>>> Without above change I get following crash in kernel while booting.
>>>>>>>>>
>>>>>>>>> [    2.441684] error: hwirq 0x4 is too large for dummy
>>>>>>>>>
>>>>>>>>> [    2.441694] ------------[ cut here ]------------
>>>>>>>>>
>>>>>>>>> [    2.441698] WARNING: at kernel/irq/irqdomain.c:344
>>>>>>>>>
>>>>>>>>> [    2.441702] Modules linked in:
>>>>>>>>>
>>>>>>>>> [    2.441706]
>>>>>>>>>
>>>>>>>>> [    2.441714] CPU: 1 PID: 1 Comm: swapper/0 Not tainted 4.4.0 #8
>>>>>>>>>
>>>>>>>>> [    2.441718] Hardware name: xlnx,zynqmp (DT)
>>>>>>>>>
>>>>>>>>> [    2.441723] task: ffffffc071886b80 ti: ffffffc071888000 task.ti:
>>>>>>>> ffffffc071888000
>>>>>>>>>
>>>>>>>>> [    2.441732] PC is at irq_domain_associate+0x138/0x1c0
>>>>>>>>>
>>>>>>>>> [    2.441738] LR is at irq_domain_associate+0x138/0x1c0
>>>>>>>>>
>>>>>>>>> In kernel/irq/irqdomain.c function irq_domain_associate
>>>>>>>>>
>>>>>>>>> if (WARN(hwirq >= domain->hwirq_max,
>>>>>>>>>                  "error: hwirq 0x%x is too large for %s\n", (int)hwirq, domain-
>>>>>>> name))
>>>>>>>>>                 return -EINVAL;
>>>>>>>>>
>>>>>>>>> Here the hwirq and hwirq_max are equal to 4 without the above
>>>>>>>>> condition
>>>>>>>> (INTX_NUM + 1) due to which crash is coming.
>>>>>>>>> This is happening as the legacy interrupts are starting from 1 (INTA).
>>>>>>>>
>>>>>>>> I understood that. I'm still persisting in saying that you have the wrong fix.
>>>>>>>>
>>>>>>>> Your domain should always allocate many interrupts as you have
>>>>>>>> interrupt sources. These interrupts (hwirq) should be numbered from 0 to (n-
>>>>>> 1).
>>>>>>>
>>>>>>> Agreed, but here comes the problem the hwirq for legacy interrupts
>>>>>>> will start at 0x1 to 0x4 (INTA to INTD) and these values are as per
>>>>>>> PCIe specification for legacy interrupts. So these cannot be numbered
>>>>>>> from 0. So when 0x4 (INTD) for a multi-function device comes the crash
>>>>>>> occurs.
>>>>>>
>>>>>> So who provides this hwirq? Who calls irq_domain_associate() with hwirq set to
>>>>>> 4?
>>>>>>
>>>>> PCIe subsystem invokes pcibios_add_device function in arch/arm64/kernel/pci.c for every pci device.
>>>>> The purpose of this function is to assign dev->irq using of_irq_parse_and_map_pci.
>>>>> of_irq_parse_and_map_pci invokes of_irq_parse_pci where it reads PCI_INTERRUPT_PIN from configuration space and saves it
>>>>> in parameter of struct of_phandle_args.
>>>>> This structure is passed to irq_create_of_mapping where it invokes irq_create_fwspec_mapping.
>>>>> irq_create_fwspec_mapping invokes irq_domain_translate and gets hwirq, here the above saved PCI_INTERRUPT_PIN value is assigned 
>>>>> to hwirq (*hwirq = fwspec->param[0]).
>>>>> And then using this hwirq irq_create_mapping -> irq_domain_associate were invoked and mapping is created for virtual irq with this hwirq.
>>>>> So for any end point PCI_INTERRUPT_PIN value starts from 0x1 to 0x4 and so hwirq starts from 0x1 to 0x4.
>>>>>
>>>>> So the values are more generic w.r.t to protocol, that's why hwirq will range from 0x1 to 0x4. 
>>>>> And then if you check pcie-altera.c they are doing this adding one in their handler and while creating legacy domain.
>>>>
>>>> Is this resolved yet?  Marc, are you happy, or should we iterate on this
>>>> again?
>>>
>>> Ah, sorry to have dropped the ball on this patch.
>>
>> No problem, I wasn't making forward progress anyway.
>>
>>> I guess that given that the infrastructure imposes the hwirq range on
>>> the host drivers, Bharat's approach is the only way (and a number of
>>> other host drivers are already slightly broken). I'll try and have a
>>> look at solving this at the generic level. In the meantime:
>>>
>>> Acked-by: Marc Zyngier <marc.zyngier@arm.com>
>>
>> After looking at this myself, I'm not happy with this either.  It feels
>> like there are bugs lurking here and we're just hiding one of them.
>>
>> Here are the callers of irq_domain_add_linear() for legacy INTx in
>> drivers/pci/host:
>>
>>   advk_pcie_init_irq_domain    LEGACY_IRQ_NUM   (4)
>>   dra7xx_pcie_init_irq_domain  4
>>   ks_dw_pcie_host_init         MAX_LEGACY_IRQS  (4)
>>   altera_pcie_init_irq_domain  INTX_NUM + 1     (5)
>>   nwl_pcie_init_irq_domain     INTX_NUM + 1     (5)
>>   xilinx_pcie_init_irq_domain  4
> 
> The altera change corresponding to this was 99496bd2971f ("PCI: altera: Fix
> error when INTx is 4").  I should have noticed this inconsistency back
> then.
> 
> Are aardvark, dra7xx, keystone, and xilinx (non-NWL) broken because they
> only request 4 IRQs and only INTA, INTB, and INTC work?

yeah.. it's broken in dra7xx. I get [1] when I configure the pci endpoint to
use INTD.

Thanks
Kishon

[1] -> http://pastebin.ubuntu.com/23177268/
> 
>> I think all of these use the of_irq_parse_and_map_pci() path you
>> mentioned, so if the problem is in the way that path works, I would
>> think these should *all* be requesting the same number of interrupts
>> in the domain.
>>
>> I agree with Marc that we should request 4 IRQs, because that's what
>> we need.  If we can't do that for some reason, we ought to at least
>> make all these callers the same.
>>
>> 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

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web