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 X-Spam-Level: X-Spam-Status: No, score=-15.3 required=3.0 tests=BAYES_00,DKIM_INVALID, DKIM_SIGNED,INCLUDES_CR_TRAILER,INCLUDES_PATCH,MAILING_LIST_MULTI, SPF_HELO_NONE,SPF_PASS,URIBL_BLOCKED,USER_AGENT_SANE_1 autolearn=ham autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 82404C433DB for ; Fri, 5 Feb 2021 10:22:03 +0000 (UTC) Received: from kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by mail.kernel.org (Postfix) with ESMTP id DBF1F64E58 for ; Fri, 5 Feb 2021 10:22:02 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org DBF1F64E58 Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=kernel.org Authentication-Results: mail.kernel.org; spf=pass smtp.mailfrom=owner-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix) id 4D7D96B0070; Fri, 5 Feb 2021 05:22:02 -0500 (EST) Received: by kanga.kvack.org (Postfix, from userid 40) id 461DF6B0073; Fri, 5 Feb 2021 05:22:02 -0500 (EST) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 32A426B0074; Fri, 5 Feb 2021 05:22:02 -0500 (EST) X-Delivered-To: linux-mm@kvack.org Received: from forelay.hostedemail.com (smtprelay0020.hostedemail.com [216.40.44.20]) by kanga.kvack.org (Postfix) with ESMTP id 164CD6B0070 for ; Fri, 5 Feb 2021 05:22:02 -0500 (EST) Received: from smtpin18.hostedemail.com (10.5.19.251.rfc1918.com [10.5.19.251]) by forelay04.hostedemail.com (Postfix) with ESMTP id D34761EE6 for ; Fri, 5 Feb 2021 10:22:01 +0000 (UTC) X-FDA: 77783823642.18.show23_5f09b2c275e4 Received: from filter.hostedemail.com (10.5.16.251.rfc1918.com [10.5.16.251]) by smtpin18.hostedemail.com (Postfix) with ESMTP id A4EF9100ED0E0 for ; Fri, 5 Feb 2021 10:22:01 +0000 (UTC) X-HE-Tag: show23_5f09b2c275e4 X-Filterd-Recvd-Size: 9645 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by imf47.hostedemail.com (Postfix) with ESMTP for ; Fri, 5 Feb 2021 10:22:00 +0000 (UTC) Received: by mail.kernel.org (Postfix) with ESMTPSA id 8DCF064E55; Fri, 5 Feb 2021 10:21:59 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1612520519; bh=vTdK7tqpnai+N4Mr1MYvbIdxctZy8n6ZlIZgJ2TTGIs=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=aw9IX3t8/+kJNRGDZ+eN+AcOe96RKkF8t8qkjRXLa5ntx8zUbf71H7CUidQRP/3oy /jYL5MOzNTkXCNVb+pWxV8zK/1XWdVgzZi8nk/3ExJgEbVYSOS32dh8UG2wBx57LbT OfJqPh+TzyXNYzzIQJO8xNFIIAvNiGmkVtRk+/nwkolGXcSYv9TH6LThlKK8ci0QPF et1/cuIWO6xPGl6ZAON1zoRCnud1K6EP3sTct9KVU3TGucYouiF7zH/V758kBhQQB7 LyqKl1Z8P4+8ePkkWvfs31Oj9jOtCO+JuKcTd7+EAmVmnnXr+rST7vmtWkEsN6rZhM 4Y7uHIzJ0CqJQ== Received: by pali.im (Postfix) id 41ADC8A2; Fri, 5 Feb 2021 11:21:57 +0100 (CET) Date: Fri, 5 Feb 2021 11:21:57 +0100 From: Pali =?utf-8?B?Um9ow6Fy?= To: Daniel Vetter Cc: Bjorn Helgaas , LKML , Stephen Rothwell , linux-samsung-soc , Jan Kara , Kees Cook , Greg Kroah-Hartman , Linux PCI , Linux MM , Jason Gunthorpe , =?utf-8?B?SsOpcsO0bWU=?= Glisse , John Hubbard , Bjorn Helgaas , Daniel Vetter , Dan Williams , Andrew Morton , Linux ARM , "open list:DMA BUFFER SHARING FRAMEWORK" , Oliver O'Halloran , Krzysztof =?utf-8?Q?Wilczy=C5=84ski?= Subject: Re: [PATCH 1/2] PCI: also set up legacy files only after sysfs init Message-ID: <20210205102157.n7avchjbzwbfkpdm@pali> References: <20210204165831.2703772-2-daniel.vetter@ffwll.ch> <20210204215019.GA104698@bjorn-Precision-5520> <20210204222407.pkx7wvmcvugdwqdd@pali> <20210205100449.w2vzqozgnolxqh4h@pali> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: User-Agent: NeoMutt/20180716 Content-Transfer-Encoding: quoted-printable X-Bogosity: Ham, tests=bogofilter, spamicity=0.000000, version=1.2.4 Sender: owner-linux-mm@kvack.org Precedence: bulk X-Loop: owner-majordomo@kvack.org List-ID: On Friday 05 February 2021 11:16:00 Daniel Vetter wrote: > On Fri, Feb 5, 2021 at 11:04 AM Pali Roh=C3=A1r wrote= : > > > > On Friday 05 February 2021 10:59:50 Daniel Vetter wrote: > > > On Thu, Feb 4, 2021 at 11:24 PM Pali Roh=C3=A1r w= rote: > > > > > > > > On Thursday 04 February 2021 15:50:19 Bjorn Helgaas wrote: > > > > > [+cc Oliver, Pali, Krzysztof] > > > > > > > > Just to note that extending or using sysfs_initialized introduces > > > > another race condition into kernel code which results in PCI fata= l > > > > errors. Details are in email discussion which Bjorn already sent. > > > > > > Yeah I wondered why this doesn't race. > > > > It races, but with smaller probability. I have not seen this race > > condition on x86. But I was able to reproduce it with native PCIe > > drivers on ARM64 (Marvell Armada 3720; pci-aardvark). In mentioned > > discussion I wrote when this race condition happen. But I understand > > that it is hard to simulate it. >=20 > btw I looked at your patch, and isn't that just reducing the race windo= w? I probably have not wrote reply to that thread and only to Krzysztof on IRC, but my "hack" really does not solve that race condition. And as you wrote it only reduced occurrence on tested HW. Krzysztof wrote that would look at this issue and try to solve it properly. So I have not doing more investigation on that my "hack" patch, race conditions are hard to catch and solve... > I think we have a very similar problem in drm, where the > drm_dev_register() for the overall device (which also registers all > drm_connector) can race with the hotplug of an individual connector in > drm_connector_register() which is hotplugged at runtime. >=20 > I went with a per-connector registered boolean + a lock to make sure > that really only one of the two call paths can end up registering the > connector. Part of registering connectors is setting up sysfs files, > so I think it's exactly the same problem as here. >=20 > Cheers, Daniel >=20 > > > > > but since the history goes back > > > to pre-git times I figured it would have been addressed somehow > > > already if it indeed does race. > > > -Daniel > > > > > > > > s/also/Also/ in subject > > > > > > > > > > On Thu, Feb 04, 2021 at 05:58:30PM +0100, Daniel Vetter wrote: > > > > > > We are already doing this for all the regular sysfs files on = PCI > > > > > > devices, but not yet on the legacy io files on the PCI buses.= Thus far > > > > > > now problem, but in the next patch I want to wire up iomem re= voke > > > > > > support. That needs the vfs up an running already to make so = that > > > > > > iomem_get_mapping() works. > > > > > > > > > > s/now problem/no problem/ > > > > > s/an running/and running/ > > > > > s/so that/sure that/ ? > > > > > > > > > > iomem_get_mapping() doesn't exist; I don't know what that shoul= d be. > > > > > > > > > > > Wire it up exactly like the existing code. Note that > > > > > > pci_remove_legacy_files() doesn't need a check since the one = for > > > > > > pci_bus->legacy_io is sufficient. > > > > > > > > > > I'm not sure exactly what you mean by "the existing code." I c= ould > > > > > probably figure it out, but it would save time to mention the e= xisting > > > > > function here. > > > > > > > > > > This looks like another instance where we should really apply O= liver's > > > > > idea of converting these to attribute_groups [1]. > > > > > > > > > > The cover letter mentions options discussed with Greg in [2], b= ut I > > > > > don't think the "sysfs_initialized" hack vs attribute_groups wa= s part > > > > > of that discussion. > > > > > > > > > > It's not absolutely a show-stopper, but it *is* a shame to exte= nd the > > > > > sysfs_initialized hack if attribute_groups could do this more c= leanly > > > > > and help solve more than one issue. > > > > > > > > > > Bjorn > > > > > > > > > > [1] https://lore.kernel.org/r/CAOSf1CHss03DBSDO4PmTtMp0tCEu5kSc= n704ZEwLKGXQzBfqaA@mail.gmail.com > > > > > [2] https://lore.kernel.org/dri-devel/CAKMK7uGrdDrbtj0OyzqQc0CG= rQwc2F3tFJU9vLfm2jjufAZ5YQ@mail.gmail.com/ > > > > > > > > > > > Signed-off-by: Daniel Vetter > > > > > > Cc: Stephen Rothwell > > > > > > Cc: Jason Gunthorpe > > > > > > Cc: Kees Cook > > > > > > Cc: Dan Williams > > > > > > Cc: Andrew Morton > > > > > > Cc: John Hubbard > > > > > > Cc: J=C3=A9r=C3=B4me Glisse > > > > > > Cc: Jan Kara > > > > > > Cc: Dan Williams > > > > > > Cc: Greg Kroah-Hartman > > > > > > Cc: linux-mm@kvack.org > > > > > > Cc: linux-arm-kernel@lists.infradead.org > > > > > > Cc: linux-samsung-soc@vger.kernel.org > > > > > > Cc: linux-media@vger.kernel.org > > > > > > Cc: Bjorn Helgaas > > > > > > Cc: linux-pci@vger.kernel.org > > > > > > --- > > > > > > drivers/pci/pci-sysfs.c | 7 +++++++ > > > > > > 1 file changed, 7 insertions(+) > > > > > > > > > > > > diff --git a/drivers/pci/pci-sysfs.c b/drivers/pci/pci-sysfs.= c > > > > > > index fb072f4b3176..0c45b4f7b214 100644 > > > > > > --- a/drivers/pci/pci-sysfs.c > > > > > > +++ b/drivers/pci/pci-sysfs.c > > > > > > @@ -927,6 +927,9 @@ void pci_create_legacy_files(struct pci_b= us *b) > > > > > > { > > > > > > int error; > > > > > > > > > > > > + if (!sysfs_initialized) > > > > > > + return; > > > > > > + > > > > > > b->legacy_io =3D kcalloc(2, sizeof(struct bin_attribute), > > > > > > GFP_ATOMIC); > > > > > > if (!b->legacy_io) > > > > > > @@ -1448,6 +1451,7 @@ void pci_remove_sysfs_dev_files(struct = pci_dev *pdev) > > > > > > static int __init pci_sysfs_init(void) > > > > > > { > > > > > > struct pci_dev *pdev =3D NULL; > > > > > > + struct pci_bus *pbus =3D NULL; > > > > > > int retval; > > > > > > > > > > > > sysfs_initialized =3D 1; > > > > > > @@ -1459,6 +1463,9 @@ static int __init pci_sysfs_init(void) > > > > > > } > > > > > > } > > > > > > > > > > > > + while ((pbus =3D pci_find_next_bus(pbus))) > > > > > > + pci_create_legacy_files(pbus); > > > > > > + > > > > > > return 0; > > > > > > } > > > > > > late_initcall(pci_sysfs_init); > > > > > > -- > > > > > > 2.30.0 > > > > > > > > > > > > > > > > > > _______________________________________________ > > > > > > linux-arm-kernel mailing list > > > > > > linux-arm-kernel@lists.infradead.org > > > > > > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel > > > > > > > > > > > > -- > > > Daniel Vetter > > > Software Engineer, Intel Corporation > > > http://blog.ffwll.ch >=20 >=20 >=20 > --=20 > Daniel Vetter > Software Engineer, Intel Corporation > http://blog.ffwll.ch