From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 75E8AC433F5 for ; Thu, 28 Oct 2021 16:22:53 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id 5A426610CF for ; Thu, 28 Oct 2021 16:22:53 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S229846AbhJ1QZT convert rfc822-to-8bit (ORCPT ); Thu, 28 Oct 2021 12:25:19 -0400 Received: from mail.kernel.org ([198.145.29.99]:50414 "EHLO mail.kernel.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S230248AbhJ1QZQ (ORCPT ); Thu, 28 Oct 2021 12:25:16 -0400 Received: from disco-boy.misterjones.org (disco-boy.misterjones.org [51.254.78.96]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPSA id F1231610CF; Thu, 28 Oct 2021 16:22:48 +0000 (UTC) Received: from sofa.misterjones.org ([185.219.108.64] helo=why.misterjones.org) by disco-boy.misterjones.org with esmtpsa (TLS1.3) tls TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384 (Exim 4.94.2) (envelope-from ) id 1mg8Aw-002Fts-Ng; Thu, 28 Oct 2021 17:22:46 +0100 Date: Thu, 28 Oct 2021 17:22:46 +0100 Message-ID: <87pmrp9l0p.wl-maz@kernel.org> From: Marc Zyngier To: Marek =?UTF-8?B?QmVow7pu?= Cc: Pali =?UTF-8?B?Um9ow6Fy?= , Lorenzo Pieralisi , Bjorn Helgaas , linux-pci@vger.kernel.org Subject: Re: [PATCH 10/14] PCI: aardvark: Enable MSI-X support In-Reply-To: <20211028175150.7faa6481@thinkpad> References: <20211012164145.14126-1-kabel@kernel.org> <20211012164145.14126-11-kabel@kernel.org> <20211027141246.GA27543@lpieralisi> <20211027142307.lrrix5yfvroxl747@pali> <20211028110835.GA1846@lpieralisi> <20211028111302.gfd73ifoyudttpee@pali> <20211028113030.GA2026@lpieralisi> <20211028113724.gm6zhqt7qcyxtgkq@pali> <87r1c59nqf.wl-maz@kernel.org> <20211028175150.7faa6481@thinkpad> User-Agent: Wanderlust/2.15.9 (Almost Unreal) SEMI-EPG/1.14.7 (Harue) FLIM-LB/1.14.9 (=?UTF-8?B?R29qxY0=?=) APEL-LB/10.8 EasyPG/1.0.0 Emacs/27.1 (x86_64-pc-linux-gnu) MULE/6.0 (HANACHIRUSATO) MIME-Version: 1.0 (generated by SEMI-EPG 1.14.7 - "Harue") Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8BIT X-SA-Exim-Connect-IP: 185.219.108.64 X-SA-Exim-Rcpt-To: kabel@kernel.org, pali@kernel.org, lorenzo.pieralisi@arm.com, bhelgaas@google.com, linux-pci@vger.kernel.org X-SA-Exim-Mail-From: maz@kernel.org X-SA-Exim-Scanned: No (on disco-boy.misterjones.org); SAEximRunCond expanded to false Precedence: bulk List-ID: X-Mailing-List: linux-pci@vger.kernel.org On Thu, 28 Oct 2021 16:51:50 +0100, Marek Behún wrote: > > On Thu, 28 Oct 2021 16:24:08 +0100 > Marc Zyngier wrote: > > > On Thu, 28 Oct 2021 12:37:24 +0100, > > Pali Rohár 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 > > > > > > > > > Reviewed-by: Marek Behún > > > > > > > > > > > > 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. 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. -- Without deviation from the norm, progress is not possible.