[10/14] PCI: aardvark: Enable MSI-X support

Commit Message

Marek Behún Oct. 12, 2021, 4:41 p.m. UTC

From: Pali Rohár <[e-mail]>

According to PCI 3.0 specification, sending both MSI and MSI-X interrupts
is done by DWORD memory write operation to doorbell message address. The
write operation for MSI has zero upper 16 bits and the MSI interrupt number
in the lower 16 bits, while the write operation for MSI-X contains a 32-bit
value from MSI-X table.

Since the driver only uses interrupt numbers from range 0..31, the upper
16 bits of the DWORD memory write operation to doorbell message address
are zero even for MSI-X interrupts. Thus we can enable MSI-X interrupts.

Testing proves that kernel can correctly receive MSI-X interrupts from PCIe
cards which supports both MSI and MSI-X interrupts.

Signed-off-by: Pali Rohár <[e-mail]>
Reviewed-by: Marek Behún <[e-mail]>
Signed-off-by: Marek Behún <[e-mail]>
---
 drivers/pci/controller/pci-aardvark.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Lorenzo Pieralisi Oct. 27, 2021, 2:12 p.m. UTC | #1

On Tue, Oct 12, 2021 at 06:41:41PM +0200, Marek Behún wrote:
> From: Pali Rohár <[e-mail]>
> 
> According to PCI 3.0 specification, sending both MSI and MSI-X interrupts
> is done by DWORD memory write operation to doorbell message address. The
> write operation for MSI has zero upper 16 bits and the MSI interrupt number
> in the lower 16 bits, while the write operation for MSI-X contains a 32-bit
> value from MSI-X table.
> 
> Since the driver only uses interrupt numbers from range 0..31, the upper
> 16 bits of the DWORD memory write operation to doorbell message address
> are zero even for MSI-X interrupts. Thus we can enable MSI-X interrupts.

It is the controller driver that defines the MSI-X data field yes, what
I don't get is why we have to add this comment in the commit log.

Basically Aardvark can support MSI-X up to 32 MSI-X vectors and you
are enabling them with this patch.

Is there anything *else* I am missing wrt 16-bit/32-bit data fields
that we need to know ?

> Testing proves that kernel can correctly receive MSI-X interrupts from
> PCIe cards which supports both MSI and MSI-X interrupts.

I don't understand what you want to convey with this commit log.

To me, the whole comment does not add anything (if I understood it),
please let me know what you want to express with it.

To me this patch enables MSI-X support because the HW can support them,
that's it.

Lorenzo

> Signed-off-by: Pali Rohár <[e-mail]>
> Reviewed-by: Marek Behún <[e-mail]>
> Signed-off-by: Marek Behún <[e-mail]>
> ---
>  drivers/pci/controller/pci-aardvark.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/pci/controller/pci-aardvark.c b/drivers/pci/controller/pci-aardvark.c
> index b703b271c6b1..337b61508799 100644
> --- a/drivers/pci/controller/pci-aardvark.c
> +++ b/drivers/pci/controller/pci-aardvark.c
> @@ -1274,7 +1274,7 @@ static int advk_pcie_init_msi_irq_domain(struct advk_pcie *pcie)
>  
>  	msi_di = &pcie->msi_domain_info;
>  	msi_di->flags = MSI_FLAG_USE_DEF_DOM_OPS | MSI_FLAG_USE_DEF_CHIP_OPS |
> -		MSI_FLAG_MULTI_PCI_MSI;
> +			MSI_FLAG_MULTI_PCI_MSI | MSI_FLAG_PCI_MSIX;
>  	msi_di->chip = msi_ic;
>  
>  	pcie->msi_inner_domain =
> -- 
> 2.32.0
>

Pali Rohár Oct. 27, 2021, 2:23 p.m. UTC | #2

On Wednesday 27 October 2021 15:12:46 Lorenzo Pieralisi wrote:
> On Tue, Oct 12, 2021 at 06:41:41PM +0200, Marek Behún wrote:
> > From: Pali Rohár <[e-mail]>
> > 
> > According to PCI 3.0 specification, sending both MSI and MSI-X interrupts
> > is done by DWORD memory write operation to doorbell message address. The
> > write operation for MSI has zero upper 16 bits and the MSI interrupt number
> > in the lower 16 bits, while the write operation for MSI-X contains a 32-bit
> > value from MSI-X table.
> > 
> > Since the driver only uses interrupt numbers from range 0..31, the upper
> > 16 bits of the DWORD memory write operation to doorbell message address
> > are zero even for MSI-X interrupts. Thus we can enable MSI-X interrupts.
> 
> It is the controller driver that defines the MSI-X data field yes, what
> I don't get is why we have to add this comment in the commit log.
> 
> Basically Aardvark can support MSI-X up to 32 MSI-X vectors and you
> are enabling them with this patch.
> 
> Is there anything *else* I am missing wrt 16-bit/32-bit data fields
> that we need to know ?
> 
> > Testing proves that kernel can correctly receive MSI-X interrupts from
> > PCIe cards which supports both MSI and MSI-X interrupts.
> 
> I don't understand what you want to convey with this commit log.
> 
> To me, the whole comment does not add anything (if I understood it),
> please let me know what you want to express with it.
> 
> To me this patch enables MSI-X support because the HW can support them,
> that's it.

My understanding is that MSI-X by definition uses 32-bit write
operations to doorbell address and so, HW needs to support catching of
32-bit write operations.

Aardvark hw seems to support only 16-bit write operation to doorbell
address. But our testing proved that hw can catch also lower 16-bits of
32-bit write operation to doorbell address.

So if driver enforces that every 32-bit write operation to doorbell
address would have upper 16-bit zeroed then MSI-X should work.

In commit message I originally tried to explain it that after applying
all previous patches which are fixing MSI and Multi-MSI support (part of
them is enforcement to use only MSI numbers 0..31), it makes driver
compatible with also MSI-X interrupts.

If you want to rewrite commit message, let us know, there is no problem.

> Lorenzo
> 
> > Signed-off-by: Pali Rohár <[e-mail]>
> > Reviewed-by: Marek Behún <[e-mail]>
> > Signed-off-by: Marek Behún <[e-mail]>
> > ---
> >  drivers/pci/controller/pci-aardvark.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/drivers/pci/controller/pci-aardvark.c b/drivers/pci/controller/pci-aardvark.c
> > index b703b271c6b1..337b61508799 100644
> > --- a/drivers/pci/controller/pci-aardvark.c
> > +++ b/drivers/pci/controller/pci-aardvark.c
> > @@ -1274,7 +1274,7 @@ static int advk_pcie_init_msi_irq_domain(struct advk_pcie *pcie)
> >  
> >  	msi_di = &pcie->msi_domain_info;
> >  	msi_di->flags = MSI_FLAG_USE_DEF_DOM_OPS | MSI_FLAG_USE_DEF_CHIP_OPS |
> > -		MSI_FLAG_MULTI_PCI_MSI;
> > +			MSI_FLAG_MULTI_PCI_MSI | MSI_FLAG_PCI_MSIX;
> >  	msi_di->chip = msi_ic;
> >  
> >  	pcie->msi_inner_domain =
> > -- 
> > 2.32.0
> >

Lorenzo Pieralisi Oct. 28, 2021, 11:08 a.m. UTC | #3

On Wed, Oct 27, 2021 at 04:23:07PM +0200, Pali Rohár wrote:
> On Wednesday 27 October 2021 15:12:46 Lorenzo Pieralisi wrote:
> > On Tue, Oct 12, 2021 at 06:41:41PM +0200, Marek Behún wrote:
> > > From: Pali Rohár <[e-mail]>
> > > 
> > > According to PCI 3.0 specification, sending both MSI and MSI-X interrupts
> > > is done by DWORD memory write operation to doorbell message address. The
> > > write operation for MSI has zero upper 16 bits and the MSI interrupt number
> > > in the lower 16 bits, while the write operation for MSI-X contains a 32-bit
> > > value from MSI-X table.
> > > 
> > > Since the driver only uses interrupt numbers from range 0..31, the upper
> > > 16 bits of the DWORD memory write operation to doorbell message address
> > > are zero even for MSI-X interrupts. Thus we can enable MSI-X interrupts.
> > 
> > It is the controller driver that defines the MSI-X data field yes, what
> > I don't get is why we have to add this comment in the commit log.
> > 
> > Basically Aardvark can support MSI-X up to 32 MSI-X vectors and you
> > are enabling them with this patch.
> > 
> > Is there anything *else* I am missing wrt 16-bit/32-bit data fields
> > that we need to know ?
> > 
> > > Testing proves that kernel can correctly receive MSI-X interrupts from
> > > PCIe cards which supports both MSI and MSI-X interrupts.
> > 
> > I don't understand what you want to convey with this commit log.
> > 
> > To me, the whole comment does not add anything (if I understood it),
> > please let me know what you want to express with it.
> > 
> > To me this patch enables MSI-X support because the HW can support them,
> > that's it.
> 
> My understanding is that MSI-X by definition uses 32-bit write
> operations to doorbell address and so, HW needs to support catching of
> 32-bit write operations.
> 
> Aardvark hw seems to support only 16-bit write operation to doorbell
> address. But our testing proved that hw can catch also lower 16-bits of
> 32-bit write operation to doorbell address.
> 
> So if driver enforces that every 32-bit write operation to doorbell
> address would have upper 16-bit zeroed then MSI-X should work.

That's clearer than the current commit log.

> In commit message I originally tried to explain it that after applying
> all previous patches which are fixing MSI and Multi-MSI support (part of
> them is enforcement to use only MSI numbers 0..31), it makes driver
> compatible with also MSI-X interrupts.
> 
> If you want to rewrite commit message, let us know, there is no problem.

I think we should.

> > > Signed-off-by: Pali Rohár <[e-mail]>
> > > Reviewed-by: Marek Behún <[e-mail]>

By the way, this tag should be removed. Marek signed it off, that
applies to other patches in this series as well.

Lorenzo

> > > Signed-off-by: Marek Behún <[e-mail]>
> > > ---
> > >  drivers/pci/controller/pci-aardvark.c | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > 
> > > diff --git a/drivers/pci/controller/pci-aardvark.c b/drivers/pci/controller/pci-aardvark.c
> > > index b703b271c6b1..337b61508799 100644
> > > --- a/drivers/pci/controller/pci-aardvark.c
> > > +++ b/drivers/pci/controller/pci-aardvark.c
> > > @@ -1274,7 +1274,7 @@ static int advk_pcie_init_msi_irq_domain(struct advk_pcie *pcie)
> > >  
> > >  	msi_di = &pcie->msi_domain_info;
> > >  	msi_di->flags = MSI_FLAG_USE_DEF_DOM_OPS | MSI_FLAG_USE_DEF_CHIP_OPS |
> > > -		MSI_FLAG_MULTI_PCI_MSI;
> > > +			MSI_FLAG_MULTI_PCI_MSI | MSI_FLAG_PCI_MSIX;
> > >  	msi_di->chip = msi_ic;
> > >  
> > >  	pcie->msi_inner_domain =
> > > -- 
> > > 2.32.0
> > >

Pali Rohár Oct. 28, 2021, 11:13 a.m. UTC | #4

On Thursday 28 October 2021 12:08:35 Lorenzo Pieralisi wrote:
> On Wed, Oct 27, 2021 at 04:23:07PM +0200, Pali Rohár wrote:
> > On Wednesday 27 October 2021 15:12:46 Lorenzo Pieralisi wrote:
> > > On Tue, Oct 12, 2021 at 06:41:41PM +0200, Marek Behún wrote:
> > > > From: Pali Rohár <[e-mail]>
> > > > 
> > > > According to PCI 3.0 specification, sending both MSI and MSI-X interrupts
> > > > is done by DWORD memory write operation to doorbell message address. The
> > > > write operation for MSI has zero upper 16 bits and the MSI interrupt number
> > > > in the lower 16 bits, while the write operation for MSI-X contains a 32-bit
> > > > value from MSI-X table.
> > > > 
> > > > Since the driver only uses interrupt numbers from range 0..31, the upper
> > > > 16 bits of the DWORD memory write operation to doorbell message address
> > > > are zero even for MSI-X interrupts. Thus we can enable MSI-X interrupts.
> > > 
> > > It is the controller driver that defines the MSI-X data field yes, what
> > > I don't get is why we have to add this comment in the commit log.
> > > 
> > > Basically Aardvark can support MSI-X up to 32 MSI-X vectors and you
> > > are enabling them with this patch.
> > > 
> > > Is there anything *else* I am missing wrt 16-bit/32-bit data fields
> > > that we need to know ?
> > > 
> > > > Testing proves that kernel can correctly receive MSI-X interrupts from
> > > > PCIe cards which supports both MSI and MSI-X interrupts.
> > > 
> > > I don't understand what you want to convey with this commit log.
> > > 
> > > To me, the whole comment does not add anything (if I understood it),
> > > please let me know what you want to express with it.
> > > 
> > > To me this patch enables MSI-X support because the HW can support them,
> > > that's it.
> > 
> > My understanding is that MSI-X by definition uses 32-bit write
> > operations to doorbell address and so, HW needs to support catching of
> > 32-bit write operations.
> > 
> > Aardvark hw seems to support only 16-bit write operation to doorbell
> > address. But our testing proved that hw can catch also lower 16-bits of
> > 32-bit write operation to doorbell address.
> > 
> > So if driver enforces that every 32-bit write operation to doorbell
> > address would have upper 16-bit zeroed then MSI-X should work.
> 
> That's clearer than the current commit log.
> 
> > In commit message I originally tried to explain it that after applying
> > all previous patches which are fixing MSI and Multi-MSI support (part of
> > them is enforcement to use only MSI numbers 0..31), it makes driver
> > compatible with also MSI-X interrupts.
> > 
> > If you want to rewrite commit message, let us know, there is no problem.
> 
> I think we should.
> 
> > > > Signed-off-by: Pali Rohár <[e-mail]>
> > > > Reviewed-by: Marek Behún <[e-mail]>
> 
> By the way, this tag should be removed. Marek signed it off, that
> applies to other patches in this series as well.

Ok! Is this the only issue with this patch series? Or something other
needs to be fixed?

> Lorenzo
> 
> > > > Signed-off-by: Marek Behún <[e-mail]>
> > > > ---
> > > >  drivers/pci/controller/pci-aardvark.c | 2 +-
> > > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > > 
> > > > diff --git a/drivers/pci/controller/pci-aardvark.c b/drivers/pci/controller/pci-aardvark.c
> > > > index b703b271c6b1..337b61508799 100644
> > > > --- a/drivers/pci/controller/pci-aardvark.c
> > > > +++ b/drivers/pci/controller/pci-aardvark.c
> > > > @@ -1274,7 +1274,7 @@ static int advk_pcie_init_msi_irq_domain(struct advk_pcie *pcie)
> > > >  
> > > >  	msi_di = &pcie->msi_domain_info;
> > > >  	msi_di->flags = MSI_FLAG_USE_DEF_DOM_OPS | MSI_FLAG_USE_DEF_CHIP_OPS |
> > > > -		MSI_FLAG_MULTI_PCI_MSI;
> > > > +			MSI_FLAG_MULTI_PCI_MSI | MSI_FLAG_PCI_MSIX;
> > > >  	msi_di->chip = msi_ic;
> > > >  
> > > >  	pcie->msi_inner_domain =
> > > > -- 
> > > > 2.32.0
> > > >

Lorenzo Pieralisi Oct. 28, 2021, 11:30 a.m. UTC | #5

On Thu, Oct 28, 2021 at 01:13:02PM +0200, Pali Rohár wrote:

[...]

> > > In commit message I originally tried to explain it that after applying
> > > all previous patches which are fixing MSI and Multi-MSI support (part of
> > > them is enforcement to use only MSI numbers 0..31), it makes driver
> > > compatible with also MSI-X interrupts.
> > > 
> > > If you want to rewrite commit message, let us know, there is no problem.
> > 
> > I think we should.
> > 
> > > > > Signed-off-by: Pali Rohár <[e-mail]>
> > > > > Reviewed-by: Marek Behún <[e-mail]>
> > 
> > By the way, this tag should be removed. Marek signed it off, that
> > applies to other patches in this series as well.
> 
> Ok! Is this the only issue with this patch series? Or something other
> needs to be fixed?

The series looks fine to me - only thing for patch[4-10] I'd like
to have evidence MarcZ is happy with the approach (you have an
ACK on patch 9, for other patches if they were discussed on
ML add a Link: tag to the relevant discussion please).

I am trying to keep the IRQ domain management in PCI controller
drivers in check, that's why I am asking.

I can definitely pull [1-3] and [11-14] since they look good to me.

Thanks,
Lorenzo

> 
> > Lorenzo
> > 
> > > > > Signed-off-by: Marek Behún <[e-mail]>
> > > > > ---
> > > > >  drivers/pci/controller/pci-aardvark.c | 2 +-
> > > > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > > > 
> > > > > diff --git a/drivers/pci/controller/pci-aardvark.c b/drivers/pci/controller/pci-aardvark.c
> > > > > index b703b271c6b1..337b61508799 100644
> > > > > --- a/drivers/pci/controller/pci-aardvark.c
> > > > > +++ b/drivers/pci/controller/pci-aardvark.c
> > > > > @@ -1274,7 +1274,7 @@ static int advk_pcie_init_msi_irq_domain(struct advk_pcie *pcie)
> > > > >  
> > > > >  	msi_di = &pcie->msi_domain_info;
> > > > >  	msi_di->flags = MSI_FLAG_USE_DEF_DOM_OPS | MSI_FLAG_USE_DEF_CHIP_OPS |
> > > > > -		MSI_FLAG_MULTI_PCI_MSI;
> > > > > +			MSI_FLAG_MULTI_PCI_MSI | MSI_FLAG_PCI_MSIX;
> > > > >  	msi_di->chip = msi_ic;
> > > > >  
> > > > >  	pcie->msi_inner_domain =
> > > > > -- 
> > > > > 2.32.0
> > > > >

Pali Rohár Oct. 28, 2021, 11:37 a.m. UTC | #6

On Thursday 28 October 2021 12:30:30 Lorenzo Pieralisi wrote:
> On Thu, Oct 28, 2021 at 01:13:02PM +0200, Pali Rohár wrote:
> 
> [...]
> 
> > > > In commit message I originally tried to explain it that after applying
> > > > all previous patches which are fixing MSI and Multi-MSI support (part of
> > > > them is enforcement to use only MSI numbers 0..31), it makes driver
> > > > compatible with also MSI-X interrupts.
> > > > 
> > > > If you want to rewrite commit message, let us know, there is no problem.
> > > 
> > > I think we should.
> > > 
> > > > > > Signed-off-by: Pali Rohár <[e-mail]>
> > > > > > Reviewed-by: Marek Behún <[e-mail]>
> > > 
> > > By the way, this tag should be removed. Marek signed it off, that
> > > applies to other patches in this series as well.
> > 
> > Ok! Is this the only issue with this patch series? Or something other
> > needs to be fixed?
> 
> The series looks fine to me - only thing for patch[4-10] I'd like
> to have evidence MarcZ is happy with the approach

Marc, could you look at patches 4-10 if you are happy with them? Link:
https://lore.kernel.org/linux-pci/[e-mail]/

> (you have an
> ACK on patch 9, for other patches if they were discussed on
> ML add a Link: tag to the relevant discussion please).

Only that one patch was acked in past:
https://lore.kernel.org/linux-pci/[e-mail]/

> I am trying to keep the IRQ domain management in PCI controller
> drivers in check, that's why I am asking.
> 
> I can definitely pull [1-3] and [11-14] since they look good to me.

Ok!

> Thanks,
> Lorenzo
> 
> > 
> > > Lorenzo
> > > 
> > > > > > Signed-off-by: Marek Behún <[e-mail]>
> > > > > > ---
> > > > > >  drivers/pci/controller/pci-aardvark.c | 2 +-
> > > > > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > > > > 
> > > > > > diff --git a/drivers/pci/controller/pci-aardvark.c b/drivers/pci/controller/pci-aardvark.c
> > > > > > index b703b271c6b1..337b61508799 100644
> > > > > > --- a/drivers/pci/controller/pci-aardvark.c
> > > > > > +++ b/drivers/pci/controller/pci-aardvark.c
> > > > > > @@ -1274,7 +1274,7 @@ static int advk_pcie_init_msi_irq_domain(struct advk_pcie *pcie)
> > > > > >  
> > > > > >  	msi_di = &pcie->msi_domain_info;
> > > > > >  	msi_di->flags = MSI_FLAG_USE_DEF_DOM_OPS | MSI_FLAG_USE_DEF_CHIP_OPS |
> > > > > > -		MSI_FLAG_MULTI_PCI_MSI;
> > > > > > +			MSI_FLAG_MULTI_PCI_MSI | MSI_FLAG_PCI_MSIX;
> > > > > >  	msi_di->chip = msi_ic;
> > > > > >  
> > > > > >  	pcie->msi_inner_domain =
> > > > > > -- 
> > > > > > 2.32.0
> > > > > >

Marc Zyngier Oct. 28, 2021, 3:24 p.m. UTC | #7

On Thu, 28 Oct 2021 12:37:24 +0100,
Pali Rohár <[e-mail]> wrote:
> 
> On Thursday 28 October 2021 12:30:30 Lorenzo Pieralisi wrote:
> > On Thu, Oct 28, 2021 at 01:13:02PM +0200, Pali Rohár wrote:
> > 
> > [...]
> > 
> > > > > In commit message I originally tried to explain it that after applying
> > > > > all previous patches which are fixing MSI and Multi-MSI support (part of
> > > > > them is enforcement to use only MSI numbers 0..31), it makes driver
> > > > > compatible with also MSI-X interrupts.
> > > > > 
> > > > > If you want to rewrite commit message, let us know, there is no problem.
> > > > 
> > > > I think we should.
> > > > 
> > > > > > > Signed-off-by: Pali Rohár <[e-mail]>
> > > > > > > Reviewed-by: Marek Behún <[e-mail]>
> > > > 
> > > > By the way, this tag should be removed. Marek signed it off, that
> > > > applies to other patches in this series as well.
> > > 
> > > Ok! Is this the only issue with this patch series? Or something other
> > > needs to be fixed?
> > 
> > The series looks fine to me - only thing for patch[4-10] I'd like
> > to have evidence MarcZ is happy with the approach
> 
> Marc, could you look at patches 4-10 if you are happy with them? Link:
> https://lore.kernel.org/linux-pci/[e-mail]/

Started with patch #4, and saw that you are still using
irq_find_mapping + generic_handle_irq which I objected to every time I
looked at this patch ([1], [2]).

My NAK still stands, and I haven't looked any further, because you
obviously don't really care about review comments.

	M.

[1] https://lore.kernel.org/r/[e-mail]
[2] https://lore.kernel.org/r/[e-mail]

Pali Rohár Oct. 28, 2021, 3:29 p.m. UTC | #8

On Thursday 28 October 2021 16:24:08 Marc Zyngier wrote:
> On Thu, 28 Oct 2021 12:37:24 +0100,
> Pali Rohár <[e-mail]> wrote:
> > 
> > On Thursday 28 October 2021 12:30:30 Lorenzo Pieralisi wrote:
> > > On Thu, Oct 28, 2021 at 01:13:02PM +0200, Pali Rohár wrote:
> > > 
> > > [...]
> > > 
> > > > > > In commit message I originally tried to explain it that after applying
> > > > > > all previous patches which are fixing MSI and Multi-MSI support (part of
> > > > > > them is enforcement to use only MSI numbers 0..31), it makes driver
> > > > > > compatible with also MSI-X interrupts.
> > > > > > 
> > > > > > If you want to rewrite commit message, let us know, there is no problem.
> > > > > 
> > > > > I think we should.
> > > > > 
> > > > > > > > Signed-off-by: Pali Rohár <[e-mail]>
> > > > > > > > Reviewed-by: Marek Behún <[e-mail]>
> > > > > 
> > > > > By the way, this tag should be removed. Marek signed it off, that
> > > > > applies to other patches in this series as well.
> > > > 
> > > > Ok! Is this the only issue with this patch series? Or something other
> > > > needs to be fixed?
> > > 
> > > The series looks fine to me - only thing for patch[4-10] I'd like
> > > to have evidence MarcZ is happy with the approach
> > 
> > Marc, could you look at patches 4-10 if you are happy with them? Link:
> > https://lore.kernel.org/linux-pci/[e-mail]/
> 
> Started with patch #4, and saw that you are still using
> irq_find_mapping + generic_handle_irq which I objected to every time I
> looked at this patch ([1], [2]).
> 
> My NAK still stands, and I haven't looked any further, because you
> obviously don't really care about review comments.

I passed to Marek all patches including handling and fixing these
issues. But because Lorenzo wanted smaller patch series, Marek probably
has not included them in this batch 2.

> 	M.
> 
> [1] https://lore.kernel.org/r/[e-mail]
> [2] https://lore.kernel.org/r/[e-mail]
> 
> -- 
> Without deviation from the norm, progress is not possible.

Marek Behún Oct. 28, 2021, 3:51 p.m. UTC | #9

On Thu, 28 Oct 2021 16:24:08 +0100
Marc Zyngier <[e-mail]> wrote:

> On Thu, 28 Oct 2021 12:37:24 +0100,
> Pali Rohár <[e-mail]> wrote:
> > 
> > On Thursday 28 October 2021 12:30:30 Lorenzo Pieralisi wrote:  
> > > On Thu, Oct 28, 2021 at 01:13:02PM +0200, Pali Rohár wrote:
> > > 
> > > [...]
> > >   
> > > > > > In commit message I originally tried to explain it that after applying
> > > > > > all previous patches which are fixing MSI and Multi-MSI support (part of
> > > > > > them is enforcement to use only MSI numbers 0..31), it makes driver
> > > > > > compatible with also MSI-X interrupts.
> > > > > > 
> > > > > > If you want to rewrite commit message, let us know, there is no problem.  
> > > > > 
> > > > > I think we should.
> > > > >   
> > > > > > > > Signed-off-by: Pali Rohár <[e-mail]>
> > > > > > > > Reviewed-by: Marek Behún <[e-mail]>  
> > > > > 
> > > > > By the way, this tag should be removed. Marek signed it off, that
> > > > > applies to other patches in this series as well.  
> > > > 
> > > > Ok! Is this the only issue with this patch series? Or something other
> > > > needs to be fixed?  
> > > 
> > > The series looks fine to me - only thing for patch[4-10] I'd like
> > > to have evidence MarcZ is happy with the approach  
> > 
> > Marc, could you look at patches 4-10 if you are happy with them? Link:
> > https://lore.kernel.org/linux-pci/[e-mail]/  
> 
> Started with patch #4, and saw that you are still using
> irq_find_mapping + generic_handle_irq which I objected to every time I
> looked at this patch ([1], [2]).
> 
> My NAK still stands, and I haven't looked any further, because you
> obviously don't really care about review comments.
> 
> 	M.
> 
> [1] https://lore.kernel.org/r/[e-mail]
> [2] https://lore.kernel.org/r/[e-mail]
> 

Marc, we have ~70 patches ready for the aardvark controller driver.

It is patch 53 [1] that converts the old irq_find_mapping() +
generic_handle_irq() API to the new API, so it isn't that Pali did
not address your comments, it is that, due to convenience, he addressed
them in a later patch.

The last time Pali sent a larger number of paches (in a previous
version, which was 42 patches [1]), it was requested that we split the
series into smaller sets, so that it is easier to merge.

Since then some more changes accumulated, resulting in the current ~70
patches, which I have been sending in smaller batches.

I could rebase the entire thing so that the patch changing the usage of
the old irq_find_mapping() + generic_handle_irq() API is first. But
that would require rebasing and testing all the patches one by one,
since the patches in-between touch everything almost everything else.

If it is really that problematic to review the changes while they use
the old API, please let me know and I will rebase it. But if you could
find it in yourself to review the patches with old API usage, it would
really save a lot of time and the result will be the same, to your
satisfaction.

Marek

[1]
https://git.kernel.org/pub/scm/linux/kernel/git/pali/linux.git/commit/?h=pci-aardvark&id=c77d04754fbe85ed37fd7517cee253022f8428fe

[2]
https://patchwork.kernel.org/project/linux-pci/cover/[e-mail]/

Marc Zyngier Oct. 28, 2021, 4:22 p.m. UTC | #10

On Thu, 28 Oct 2021 16:51:50 +0100,
Marek Behún <[e-mail]> wrote:
> 
> On Thu, 28 Oct 2021 16:24:08 +0100
> Marc Zyngier <[e-mail]> wrote:
> 
> > On Thu, 28 Oct 2021 12:37:24 +0100,
> > Pali Rohár <[e-mail]> wrote:
> > > 
> > > On Thursday 28 October 2021 12:30:30 Lorenzo Pieralisi wrote:  
> > > > On Thu, Oct 28, 2021 at 01:13:02PM +0200, Pali Rohár wrote:
> > > > 
> > > > [...]
> > > >   
> > > > > > > In commit message I originally tried to explain it that after applying
> > > > > > > all previous patches which are fixing MSI and Multi-MSI support (part of
> > > > > > > them is enforcement to use only MSI numbers 0..31), it makes driver
> > > > > > > compatible with also MSI-X interrupts.
> > > > > > > 
> > > > > > > If you want to rewrite commit message, let us know, there is no problem.  
> > > > > > 
> > > > > > I think we should.
> > > > > >   
> > > > > > > > > Signed-off-by: Pali Rohár <[e-mail]>
> > > > > > > > > Reviewed-by: Marek Behún <[e-mail]>  
> > > > > > 
> > > > > > By the way, this tag should be removed. Marek signed it off, that
> > > > > > applies to other patches in this series as well.  
> > > > > 
> > > > > Ok! Is this the only issue with this patch series? Or something other
> > > > > needs to be fixed?  
> > > > 
> > > > The series looks fine to me - only thing for patch[4-10] I'd like
> > > > to have evidence MarcZ is happy with the approach  
> > > 
> > > Marc, could you look at patches 4-10 if you are happy with them? Link:
> > > https://lore.kernel.org/linux-pci/[e-mail]/  
> > 
> > Started with patch #4, and saw that you are still using
> > irq_find_mapping + generic_handle_irq which I objected to every time I
> > looked at this patch ([1], [2]).
> > 
> > My NAK still stands, and I haven't looked any further, because you
> > obviously don't really care about review comments.
> > 
> > 	M.
> > 
> > [1] https://lore.kernel.org/r/[e-mail]
> > [2] https://lore.kernel.org/r/[e-mail]
> > 
> 
> Marc, we have ~70 patches ready for the aardvark controller driver.
> 
> It is patch 53 [1] that converts the old irq_find_mapping() +
> generic_handle_irq() API to the new API, so it isn't that Pali did
> not address your comments, it is that, due to convenience, he addressed
> them in a later patch.

I don't see the point in merging patches that use an API that we are
actively removing. You have known that since August. I didn't say
'Just add a patch on top'. I said 'Please send a patch that makes
sense for upstream'. This makes *zero* sense for upstream. At the
time, Pali argued for your approach on the grounds of "but stable!",
and I said no to this. Since then, my opinion hasn't changed.

At the end of the day, the fate of these patches are in your own
hands. You can carry on piling useless changes on top of each others,
leading to a number of patches that is larger than a full new Linux
port. Or you can do something that is actually realistic by dropping
all the pointless intermediate states.

> The last time Pali sent a larger number of paches (in a previous
> version, which was 42 patches [1]), it was requested that we split the
> series into smaller sets, so that it is easier to merge.
> 
> Since then some more changes accumulated, resulting in the current ~70
> patches, which I have been sending in smaller batches.
> 
> I could rebase the entire thing so that the patch changing the usage of
> the old irq_find_mapping() + generic_handle_irq() API is first. But
> that would require rebasing and testing all the patches one by one,
> since the patches in-between touch everything almost everything else.

I'm sorry if you didn't realise that, but we do rebase and change
patches in patch series *all the time*. That's how it works. I someone
spots a problem in the middle of one of my patch series, I don't stick
a fix on top. I fix it in situ, and rebase the following patches.

>
> If it is really that problematic to review the changes while they use
> the old API, please let me know and I will rebase it. But if you could
> find it in yourself to review the patches with old API usage, it would
> really save a lot of time and the result will be the same, to your
> satisfaction.

I've said what I expected to see in August. I haven't changed my mind.

Thanks,

	M.

Marek Behún Oct. 28, 2021, 4:25 p.m. UTC | #11

On Thu, 28 Oct 2021 17:51:50 +0200
Marek Behún <[e-mail]> wrote:

> Marc, we have ~70 patches ready for the aardvark controller driver.
> 
> It is patch 53 [1] that converts the old irq_find_mapping() +
> generic_handle_irq() API to the new API, so it isn't that Pali did
> not address your comments, it is that, due to convenience, he addressed
> them in a later patch.
> 
> The last time Pali sent a larger number of paches (in a previous
> version, which was 42 patches [1]), it was requested that we split the
> series into smaller sets, so that it is easier to merge.
> 
> Since then some more changes accumulated, resulting in the current ~70
> patches, which I have been sending in smaller batches.
> 
> I could rebase the entire thing so that the patch changing the usage of
> the old irq_find_mapping() + generic_handle_irq() API is first. But
> that would require rebasing and testing all the patches one by one,
> since the patches in-between touch everything almost everything else.
> 
> If it is really that problematic to review the changes while they use
> the old API, please let me know and I will rebase it. But if you could
> find it in yourself to review the patches with old API usage, it would
> really save a lot of time and the result will be the same, to your
> satisfaction.

Lorenzo, Marc, Bjorn,

I have one more question.

Pali prepared the ~70 patches so that fixes come first, and
new features / changes to new API later.

He did it in this way so that the patches could be then conventiently
backported to stable versions - were we to first change the API usage
to the new API, and then fix the ~20 IRQ related things, we would
afterwards have to backport the fixes by rewriting them one by one.

Is this really how we should do this? Should we ignore stable while
developing for master, regardless of how much other work would need to
be spent by backporting to master, even if it could be much simpler by
applying the patches in master in another order?

Marek

Marc Zyngier Oct. 28, 2021, 5 p.m. UTC | #12

On Thu, 28 Oct 2021 17:25:14 +0100,
Marek Behún <[e-mail]> wrote:
> 
> On Thu, 28 Oct 2021 17:51:50 +0200
> Marek Behún <[e-mail]> wrote:
> 
> > Marc, we have ~70 patches ready for the aardvark controller driver.
> > 
> > It is patch 53 [1] that converts the old irq_find_mapping() +
> > generic_handle_irq() API to the new API, so it isn't that Pali did
> > not address your comments, it is that, due to convenience, he addressed
> > them in a later patch.
> > 
> > The last time Pali sent a larger number of paches (in a previous
> > version, which was 42 patches [1]), it was requested that we split the
> > series into smaller sets, so that it is easier to merge.
> > 
> > Since then some more changes accumulated, resulting in the current ~70
> > patches, which I have been sending in smaller batches.
> > 
> > I could rebase the entire thing so that the patch changing the usage of
> > the old irq_find_mapping() + generic_handle_irq() API is first. But
> > that would require rebasing and testing all the patches one by one,
> > since the patches in-between touch everything almost everything else.
> > 
> > If it is really that problematic to review the changes while they use
> > the old API, please let me know and I will rebase it. But if you could
> > find it in yourself to review the patches with old API usage, it would
> > really save a lot of time and the result will be the same, to your
> > satisfaction.
> 
> Lorenzo, Marc, Bjorn,
> 
> I have one more question.
> 
> Pali prepared the ~70 patches so that fixes come first, and
> new features / changes to new API later.
> 
> He did it in this way so that the patches could be then conventiently
> backported to stable versions - were we to first change the API usage
> to the new API, and then fix the ~20 IRQ related things, we would
> afterwards have to backport the fixes by rewriting them one by one.
> 
> Is this really how we should do this? Should we ignore stable while
> developing for master, regardless of how much other work would need to
> be spent by backporting to master, even if it could be much simpler by
> applying the patches in master in another order?

I already replied to that in August. Upstream is the primary
development target. If you want to backport patches, do them and make
the changes required so that they are correct for whatever kernel you
target. Stable doesn't matter to upstream at all.

	M.

Lorenzo Pieralisi Oct. 28, 2021, 5:47 p.m. UTC | #13

On Thu, Oct 28, 2021 at 06:00:47PM +0100, Marc Zyngier wrote:
> On Thu, 28 Oct 2021 17:25:14 +0100,
> Marek Behún <[e-mail]> wrote:
> > 
> > On Thu, 28 Oct 2021 17:51:50 +0200
> > Marek Behún <[e-mail]> wrote:
> > 
> > > Marc, we have ~70 patches ready for the aardvark controller driver.
> > > 
> > > It is patch 53 [1] that converts the old irq_find_mapping() +
> > > generic_handle_irq() API to the new API, so it isn't that Pali did
> > > not address your comments, it is that, due to convenience, he addressed
> > > them in a later patch.
> > > 
> > > The last time Pali sent a larger number of paches (in a previous
> > > version, which was 42 patches [1]), it was requested that we split the
> > > series into smaller sets, so that it is easier to merge.
> > > 
> > > Since then some more changes accumulated, resulting in the current ~70
> > > patches, which I have been sending in smaller batches.
> > > 
> > > I could rebase the entire thing so that the patch changing the usage of
> > > the old irq_find_mapping() + generic_handle_irq() API is first. But
> > > that would require rebasing and testing all the patches one by one,
> > > since the patches in-between touch everything almost everything else.
> > > 
> > > If it is really that problematic to review the changes while they use
> > > the old API, please let me know and I will rebase it. But if you could
> > > find it in yourself to review the patches with old API usage, it would
> > > really save a lot of time and the result will be the same, to your
> > > satisfaction.
> > 
> > Lorenzo, Marc, Bjorn,
> > 
> > I have one more question.
> > 
> > Pali prepared the ~70 patches so that fixes come first, and
> > new features / changes to new API later.
> > 
> > He did it in this way so that the patches could be then conventiently
> > backported to stable versions - were we to first change the API usage
> > to the new API, and then fix the ~20 IRQ related things, we would
> > afterwards have to backport the fixes by rewriting them one by one.
> > 
> > Is this really how we should do this? Should we ignore stable while
> > developing for master, regardless of how much other work would need to
> > be spent by backporting to master, even if it could be much simpler by
> > applying the patches in master in another order?
> 
> I already replied to that in August. Upstream is the primary
> development target. If you want to backport patches, do them and make
> the changes required so that they are correct for whatever kernel you
> target. Stable doesn't matter to upstream at all.

+1

Please don't write patch series with stable backports in mind, don't.

Let's focus on mailine with one series at a time, I understand it is
hard but that's the only way we can work and I can keep track of what
you are doing, if we keep splitting patch series I can't track reviews
and then we end up in this situation. I asked if you received Marc's
feedback exactly because I can't track the original discussion and if I
merged the series (the MSI bits) I would have ignored what Marc
requested you to do and that's not OK.

So, given the timing, I will try to merge patches [1-3] and [11-14]
if I can rebase the series cleanly; maybe I can include patch 9 if
it does not depend on previous patches.

Thanks,
Lorenzo

Marek Behún Oct. 28, 2021, 6:24 p.m. UTC | #14

On Thu, 28 Oct 2021 18:47:18 +0100
Lorenzo Pieralisi <[e-mail]> wrote:

> On Thu, Oct 28, 2021 at 06:00:47PM +0100, Marc Zyngier wrote:
> > On Thu, 28 Oct 2021 17:25:14 +0100,
> > Marek Behún <[e-mail]> wrote:  
> > > 
> > > On Thu, 28 Oct 2021 17:51:50 +0200
> > > Marek Behún <[e-mail]> wrote:
> > >   
> > > > Marc, we have ~70 patches ready for the aardvark controller driver.
> > > > 
> > > > It is patch 53 [1] that converts the old irq_find_mapping() +
> > > > generic_handle_irq() API to the new API, so it isn't that Pali did
> > > > not address your comments, it is that, due to convenience, he addressed
> > > > them in a later patch.
> > > > 
> > > > The last time Pali sent a larger number of paches (in a previous
> > > > version, which was 42 patches [1]), it was requested that we split the
> > > > series into smaller sets, so that it is easier to merge.
> > > > 
> > > > Since then some more changes accumulated, resulting in the current ~70
> > > > patches, which I have been sending in smaller batches.
> > > > 
> > > > I could rebase the entire thing so that the patch changing the usage of
> > > > the old irq_find_mapping() + generic_handle_irq() API is first. But
> > > > that would require rebasing and testing all the patches one by one,
> > > > since the patches in-between touch everything almost everything else.
> > > > 
> > > > If it is really that problematic to review the changes while they use
> > > > the old API, please let me know and I will rebase it. But if you could
> > > > find it in yourself to review the patches with old API usage, it would
> > > > really save a lot of time and the result will be the same, to your
> > > > satisfaction.  
> > > 
> > > Lorenzo, Marc, Bjorn,
> > > 
> > > I have one more question.
> > > 
> > > Pali prepared the ~70 patches so that fixes come first, and
> > > new features / changes to new API later.
> > > 
> > > He did it in this way so that the patches could be then conventiently
> > > backported to stable versions - were we to first change the API usage
> > > to the new API, and then fix the ~20 IRQ related things, we would
> > > afterwards have to backport the fixes by rewriting them one by one.
> > > 
> > > Is this really how we should do this? Should we ignore stable while
> > > developing for master, regardless of how much other work would need to
> > > be spent by backporting to master, even if it could be much simpler by
> > > applying the patches in master in another order?  
> > 
> > I already replied to that in August. Upstream is the primary
> > development target. If you want to backport patches, do them and make
> > the changes required so that they are correct for whatever kernel you
> > target. Stable doesn't matter to upstream at all.  
> 
> +1
> 
> Please don't write patch series with stable backports in mind, don't.
> 
> Let's focus on mailine with one series at a time, I understand it is
> hard but that's the only way we can work and I can keep track of what
> you are doing, if we keep splitting patch series I can't track reviews
> and then we end up in this situation. I asked if you received Marc's
> feedback exactly because I can't track the original discussion and if I
> merged the series (the MSI bits) I would have ignored what Marc
> requested you to do and that's not OK.
> 
> So, given the timing, I will try to merge patches [1-3] and [11-14]
> if I can rebase the series cleanly; maybe I can include patch 9 if
> it does not depend on previous patches.

Very well. Ignore patch 9, since it touches IRQ. In the next batch I
shall send IRQ changes, with the first or second patch converting to
this new API.

Marek

Patch

diff --git a/drivers/pci/controller/pci-aardvark.c b/drivers/pci/controller/pci-aardvark.c
index b703b271c6b1..337b61508799 100644
--- a/drivers/pci/controller/pci-aardvark.c
+++ b/drivers/pci/controller/pci-aardvark.c
@@ -1274,7 +1274,7 @@  static int advk_pcie_init_msi_irq_domain(struct advk_pcie *pcie)
 
 	msi_di = &pcie->msi_domain_info;
 	msi_di->flags = MSI_FLAG_USE_DEF_DOM_OPS | MSI_FLAG_USE_DEF_CHIP_OPS |
-		MSI_FLAG_MULTI_PCI_MSI;
+			MSI_FLAG_MULTI_PCI_MSI | MSI_FLAG_PCI_MSIX;
 	msi_di->chip = msi_ic;
 
 	pcie->msi_inner_domain =