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

From: "Marek Behún" <kabel@kernel.org>
To: Marc Zyngier <maz@kernel.org>
Cc: "Pali Rohár" <pali@kernel.org>,
	"Lorenzo Pieralisi" <lorenzo.pieralisi@arm.com>,
	"Bjorn Helgaas" <bhelgaas@google.com>,
	linux-pci@vger.kernel.org
Subject: Re: [PATCH 10/14] PCI: aardvark: Enable MSI-X support
Date: Thu, 28 Oct 2021 17:51:50 +0200	[thread overview]
Message-ID: <20211028175150.7faa6481@thinkpad> (raw)
In-Reply-To: <87r1c59nqf.wl-maz@kernel.org>

On Thu, 28 Oct 2021 16:24:08 +0100
Marc Zyngier <maz@kernel.org> wrote:

> On Thu, 28 Oct 2021 12:37:24 +0100,
> Pali Rohár <pali@kernel.org> 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 <pali@kernel.org>
> > > > > > > > Reviewed-by: Marek Behún <kabel@kernel.org>  
> > > > > 
> > > > > 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/20211012164145.14126-5-kabel@kernel.org/  
> 
> 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/8735r0qfab.wl-maz@kernel.org
> [2] https://lore.kernel.org/r/871r6kqf2d.wl-maz@kernel.org
> 

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/20210506153153.30454-1-pali@kernel.org/