Received: by 2002:a5d:925a:0:0:0:0:0 with SMTP id e26csp792747iol; Thu, 9 Jun 2022 14:13:10 -0700 (PDT) X-Google-Smtp-Source: ABdhPJzKGukk+oSsxXR7WY3CFRzOxSDQIa7xx/A7PLOAbXY1Uvw/VpyyWqvN3bdPv6N6CupQEZUA X-Received: by 2002:a17:902:bd05:b0:158:544d:6557 with SMTP id p5-20020a170902bd0500b00158544d6557mr41446808pls.70.1654809190536; Thu, 09 Jun 2022 14:13:10 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1654809190; cv=none; d=google.com; s=arc-20160816; b=fPiUb07ppBd7BIHomKa88jhbLSaZlKtsFooCFuVol6GNjCr3HsHqYR2uvWyP2LsLL4 lK5j9qytGHCKR1RPLG/FfPB9GIVRwhPI7mQ+UZwnk76Du3Gg7KJFp18SJGyq1FMF+0z8 MdKhW3AI5X5NO+LMEkue3IvmAJiIqxv2V8lCWNnrZgaWqHe+NtJ9Up5ehsd7eT0kLMMa jOG9aNbha2cY+Vn6fzJHSRJT9/y7FgV58TrTjj1L19Q17EmGP4dh887TLoCoV15a3gyx apSg//VkYwbKoDkGIXLnLpxynBNvO+vRJvax5SI7u2VVetL6r36s6ot1c8Mq8OSsdrJc 4wLA== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:user-agent:in-reply-to:content-transfer-encoding :content-disposition:mime-version:references:message-id:subject:cc :to:from:date:dkim-signature; bh=8uTaP2OTxlDndrRwzJbCi/dJlsB+Mm+1vNadj1i51L8=; b=DTTcYCceb6iCZc379wzHMkNjouSmpSmW3YSgOdF7DPKvteZGksqQ9cEjBqY0rvuwiO SMc76Fymm012AUVJRZruLDHA5hLxFaepIXZiakbeS7Cx1Stw3LIlW8JCBdPG3GNKQ9Ka 0YhCncUdQs0PK3XxLCJsM0F50B9wQRrP+gn0oiqyDXM2ktsOz5voVRgnVG4TbBEE3eJD fQlmGQB+BLcrOJErtGBRSHHdhSNEaam+HPcXMaurEkey4pqquHhAVeZ7QQtM5xB+tyEX 5BjuIWw2ybiLgsPqEtx9RD527IRZkG8Spcgun3nhvVR04Ybm6VotUX/tr+FopRJp++eA +fhQ== ARC-Authentication-Results: i=1; mx.google.com; dkim=pass header.i=@kernel.org header.s=k20201202 header.b=CXHjjZnR; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=NONE sp=NONE dis=NONE) header.from=kernel.org Return-Path: Received: from out1.vger.email (out1.vger.email. [2620:137:e000::1:20]) by mx.google.com with ESMTP id pg1-20020a17090b1e0100b001cab35fecb9si434834pjb.187.2022.06.09.14.12.55; Thu, 09 Jun 2022 14:13:10 -0700 (PDT) Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20; Authentication-Results: mx.google.com; dkim=pass header.i=@kernel.org header.s=k20201202 header.b=CXHjjZnR; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=NONE sp=NONE dis=NONE) header.from=kernel.org Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1345013AbiFITly (ORCPT + 99 others); Thu, 9 Jun 2022 15:41:54 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:49990 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S235428AbiFITlx (ORCPT ); Thu, 9 Jun 2022 15:41:53 -0400 Received: from dfw.source.kernel.org (dfw.source.kernel.org [IPv6:2604:1380:4641:c500::1]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id DEF1533A06; Thu, 9 Jun 2022 12:41:51 -0700 (PDT) Received: from smtp.kernel.org (relay.kernel.org [52.25.139.140]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by dfw.source.kernel.org (Postfix) with ESMTPS id 7A3D861E60; Thu, 9 Jun 2022 19:41:51 +0000 (UTC) Received: by smtp.kernel.org (Postfix) with ESMTPSA id 816D0C3411B; Thu, 9 Jun 2022 19:41:50 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1654803710; bh=usp7vlypnoRPUZlOxA7XzkKFFN8uZFpljRhUQOQCXSY=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=CXHjjZnRMxm7gjsTnh5d/BHqqB6a5+dqORV98BhSfltX7jxnDrJM4tOIhXDmg4cBc cHUYbkcgFRWd98LnGWylvXTfsQcVARgq52aaTkgWgxKOriUtiJUjpSReiuysZGi7Bj 4Qh0LXrw2X0ObmO+sdSyHAWMeXQ3hDhRQDAGmieA95EzsAgpuot/UMHnbhcvdf2kXK h68kj1i1aMnlrTmxHWb8K/x7Op9oJkSi4tkQl7SxTOAoRDovODz5v5tTAltGCnmn0g ND2ycBDPTSTKx43DhQKj/6dg/VrU/iwhAMPkMnCAM3kRsn0WPqMhBxA+qAT90WxdSC a2AhvtcPN+2bA== Received: by pali.im (Postfix) id 82EDB2104; Thu, 9 Jun 2022 21:41:47 +0200 (CEST) Date: Thu, 9 Jun 2022 21:41:47 +0200 From: Pali =?utf-8?B?Um9ow6Fy?= To: Bjorn Helgaas Cc: Tyrel Datwyler , linux-pci@vger.kernel.org, linux-kernel@vger.kernel.org, "Guilherme G. Piccoli" , Paul Mackerras , Bjorn Helgaas , linuxppc-dev@lists.ozlabs.org Subject: Re: [PATCH] powerpc/pci: Add config option for using OF 'reg' for PCI domain Message-ID: <20220609194147.g7fr7xtyvedhpgy5@pali> References: <20220609180526.7dwyzezyu5zxncar@pali> <20220609193451.GA525883@bhelgaas> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <20220609193451.GA525883@bhelgaas> User-Agent: NeoMutt/20180716 X-Spam-Status: No, score=-8.3 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH, DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,RCVD_IN_DNSWL_HI, SPF_HELO_NONE,SPF_PASS,T_SCC_BODY_TEXT_LINE autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on lindbergh.monkeyblade.net Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Thursday 09 June 2022 14:34:51 Bjorn Helgaas wrote: > On Thu, Jun 09, 2022 at 08:05:26PM +0200, Pali Rohár wrote: > > On Thursday 09 June 2022 12:10:22 Bjorn Helgaas wrote: > > > On Thu, Jun 09, 2022 at 06:27:25PM +0200, Pali Rohár wrote: > > > > On Thursday 09 June 2022 11:22:55 Bjorn Helgaas wrote: > > > > > [+cc Guilherme, Michael, Ben (author of 63a72284b159 and PPC folks), thread: > > > > > https://lore.kernel.org/r/20220504175718.29011-1-pali@kernel.org] > > > > > > > > > > On Fri, May 06, 2022 at 12:33:02AM +0200, Pali Rohár wrote: > > > > > > On Thursday 05 May 2022 15:10:01 Tyrel Datwyler wrote: > > > > > > > On 5/5/22 02:31, Pali Rohár wrote: > > > > > > > > On Thursday 05 May 2022 07:16:40 Christophe Leroy wrote: > > > > > > > >> Le 04/05/2022 à 19:57, Pali Rohár a écrit : > > > > > > > >>> Since commit 63a72284b159 ("powerpc/pci: Assign fixed PHB > > > > > > > >>> number based on device-tree properties"), powerpc kernel > > > > > > > >>> always fallback to PCI domain assignment from OF / Device Tree > > > > > > > >>> 'reg' property of the PCI controller. > > > > > > > >>> > > > > > > > >>> PCI code for other Linux architectures use increasing > > > > > > > >>> assignment of the PCI domain for individual controllers > > > > > > > >>> (assign the first free number), like it was also for powerpc > > > > > > > >>> prior mentioned commit. > > > > > > > >>> > > > > > > > >>> Upgrading powerpc kernels from LTS 4.4 version (which does not > > > > > > > >>> contain mentioned commit) to new LTS versions brings a > > > > > > > >>> regression in domain assignment. > > > > > > > >> > > > > > > > >> Can you elaborate why it is a regression ? > > > > > > > >> 63a72284b159 That commit says 'no functionnal changes', I'm > > > > > > > >> having hard time understanding how a nochange can be a > > > > > > > >> regression. > > > > > > > > > > > > > > > > It is not 'no functional change'. That commit completely changed > > > > > > > > PCI domain assignment in a way that is incompatible with other > > > > > > > > architectures and also incompatible with the way how it was done > > > > > > > > prior that commit. > > > > > > > > > > > > > > I agree that the "no functional change" statement is incorrect. > > > > > > > However, for most powerpc platforms it ended up being simply a > > > > > > > cosmetic behavior change. As far as I can tell there is nothing > > > > > > > requiring domain ids to increase montonically from zero or that > > > > > > > each architecture is required to use the same domain numbering > > > > > > > scheme. > > > > > > > > > > > > That is truth. But it looks really suspicious why domains are not > > > > > > assigned monotonically. Some scripts / applications are using PCI > > > > > > location (domain:bus:dev:func) for remembering PCI device and domain > > > > > > change can cause issue for config files. And some (older) applications > > > > > > expects existence of domain zero. In systems without hot plug support > > > > > > with small number of domains (e.g. 3) it means that there are always > > > > > > domains 0, 1 and 2. > > > > > > > > > > > > > Its hard to call this a true regression unless it actually broke > > > > > > > something. The commit in question has been in the kernel since 4.8 > > > > > > > which was released over 5 1/2 years ago. > > > > > > > > > > > > I agree, it really depends on how you look at it. > > > > > > > > > > > > The important is that lot of people are using LTS versions and are > > > > > > doing upgrades when LTS support is dropped. Which for 4.4 now > > > > > > happened. So not all smaller or "cosmetic" changes could be detected > > > > > > until longer LTS period pass. > > > > > > > > > > > > > With all that said looking closer at the code in question I think > > > > > > > it is fair to assume that the author only intended this change for > > > > > > > powernv and pseries platforms and not every powerpc platform. That > > > > > > > change was done to make persistent naming easier to manage in > > > > > > > userspace. > > > > > > > > > > > > I agree that this behavior change may be useful in some situations > > > > > > and I do not object this need. > > > > > > > > > > > > > Your change defaults back to the old behavior which will now break > > > > > > > both powernv and pseries platforms with regard to hotplugging and > > > > > > > persistent naming. > > > > > > > > > > > > I was aware of it, that change could cause issues. And that is why I > > > > > > added config option for choosing behavior. So users would be able to > > > > > > choose what they need. > > > > > > > > > > > > > We could properly limit it to powernv and pseries by using > > > > > > > ibm,fw-phb-id instead of reg property in the look up that follows > > > > > > > a failed ibm,opal-phbid lookup. I think this is acceptable as long > > > > > > > as no other powerpc platforms have started using this behavior for > > > > > > > persistent naming. > > > > > > > > > > > > And what about setting that new config option to enabled by default > > > > > > for those series? > > > > > > > > > > > > Or is there issue with introduction of the new config option? > > > > > > > > > > > > One of the point is that it is really a good idea to have > > > > > > similar/same behavior for all linux platforms. And if it cannot be > > > > > > enabled by default (for backward compatibility) add at least some > > > > > > option, so new platforms can start using it or users can decide to > > > > > > switch behavior. > > > > > > > > > > This is a powerpc thing so I'm just kibbitzing a little. > > > > > > > > > > This basically looks like a new config option to selectively revert > > > > > 63a72284b159. That seems hard to maintain and doesn't seem like > > > > > something that needs to be baked into the kernel at compile-time. > > > > > > > > > > The 63a72284b159 commit log says persistent NIC names are tied to PCI > > > > > domain/bus/dev/fn addresses, which seems like something we should > > > > > discourage because we can't predict PCI addresses in general. I > > > > > assume other platforms typically use udev with MAC addresses or > > > > > something? > > > > > > > > This is not about ethernet NIC cards only. But affects also WiFi cards > > > > (which registers phy dev, not netdev) and also all other PCIe cards > > > > which do not have to be network-based. Hence MAC address or udev does > > > > not play role there. > > > > > > What persistent naming mechanism do other platforms use in those > > > cases? > > > > For example sysfs path which contains domain/bus/dev/fn numbers. And > > these numbers were changed in that mentioned commit. > > > > > I forgot to ask before about the actual regression here. The commit > > > log says domain numbers are different, but I don't know the connection > > > from there to something failing. I assume there's some script or > > > config file that depends on specific domain numbers? And that > > > dependency is (hopefully) powerpc-specific? > > > > You assume correct. For example this is the way how OpenWRT handles PCI > > devices (but not only OpenWRT). This OpenWRT case is not > > powerpc-specific but generic to all architectures. This is just one > > example. > > So basically everybody uses D/b/d/f for persistent names. That's ... > well, somewhat stable for things soldered down or in a motherboard > slot, but a terrible idea for things that can be hot-plugged. I agree! > Even for more core things, it's possible for firmware to change bus > numbering between boots. For example, if a complicated hierarchy is > cold-plugged into one slot, firmware is likely to assign different bus > numbers on the next boot to make room for it. Obviously this can also > happen as a hot-add, and Linux needs the flexibility to do similar > renumbering then, although we don't support it yet. Yes. But in soldered design where you have just 3 devices without hotplug or any complicated topology then all those issues do not apply. That is why I wrote that in some scenario makes sense new behavior (e.g. big hot plug system or system with complicated topology with many bridges) and in some other scenario makes sense old behavior (e.g. simple soldered PCIe devices on board). And that is why I proposed config option, so everybody can choose what is better and better fits for chosen design. > It looks like 63a72284b159 was intended to make domain numbers *more* > consistent, so it's ironic that this actually broke something by > changing domain numbers. Maybe there's a way to limit the scope of > 63a72284b159 so it avoids the breakage. I don't know enough about the > powerpc landscape to even guess at how. > > Bjorn