Received: by 2002:ab2:3c46:0:b0:1f5:f2ab:c469 with SMTP id x6csp118653lqf; Fri, 26 Apr 2024 01:07:31 -0700 (PDT) X-Forwarded-Encrypted: i=3; AJvYcCWAvxD0qegaUG5DGrDqeaKNPk21y4kPPPHsQ+Rg9rzjkG6ec07ZPrNk4YfVU0gJa8FJ7IyJzCMmFh/mUu3Zq6ML5LxOhSSYq/gFJyr5ug== X-Google-Smtp-Source: AGHT+IE52wchos5SU1x5tjW7hGVz/VlbHHC95FAi/+tm/5SMVrfy6/v966kZtlnbmt2vv0SCtx9d X-Received: by 2002:a05:620a:b4a:b0:78f:1bef:6dc0 with SMTP id x10-20020a05620a0b4a00b0078f1bef6dc0mr2100248qkg.18.1714118843320; Fri, 26 Apr 2024 01:07:23 -0700 (PDT) ARC-Seal: i=2; a=rsa-sha256; t=1714118843; cv=pass; d=google.com; s=arc-20160816; b=cZ/kbVCGreq5OyoiLa4+GXHydH6VjUgkx+GVAdp8S7acgcIrGTpfRC4KZFL5zUPKxk 8AsMpH10w3K/roc7CCzMGnCmL6KbE6bgOmRW1AYQJa2CAPjA0yudjUCLkRmG2pyDll4N gkJvagnbnX9iOtDB1eFVhfqGkQ2IIddVn2nbOogQqoSrZtie8oLS39PaHXDbI4LEIsgL m89xFYS8kCWdy9iUsyUWSiivK8zLLKwpnNSZaZ3yse1dsgAaS2VymmKeg3WNcXo2G8HC 5leXRsQWF02dsl2E0+DWiBmA7QVFOj6s+xs9f5KKyeaBjYoHWNfo/I4/bm4DwkRpmmzD QHqg== ARC-Message-Signature: i=2; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=mime-version:list-unsubscribe:list-subscribe:list-id:precedence :user-agent:content-transfer-encoding:references:in-reply-to:date:cc :to:from:subject:message-id:dkim-signature; bh=TGAcZesai+U0xVAWvA7J/MAJQ7c0rregms0Ggpsl5S0=; fh=jFlciG5CNd8SZ1606x6VntRwMewq1AhkePnH2nMneMQ=; b=cHd9UMpYNB+pa5DuElBlm8i2218NvVARctJ3JMLcLONrPWQ7x4Vn74rzf6TXnbecD/ 6vb5DrWR2Y2Na8yZ+ZW8FS3U40llS1Rsfpqe0tyJxYmmLzEEc1wOh9ubEf+JUPoNO6G0 NOKfXpBxh4IZn2uamG8oBYjCSlClh7X9v8z8Hlemi3tGvK1Qc3B5QPO8FoniYwMZcf/A aZHjUQBMJSsBdGjoDOmgP4nmDT5CH6YF2N243cIplEtwgvY+CmzQlWM9QBswqxHSui1s yR7xxojWz/QD24YRdRy4Lqek0eYLqP4/+SOT3o/kaebJjcS9ySAlib6eplDd1XivsZHT 4DDA==; dara=google.com ARC-Authentication-Results: i=2; mx.google.com; dkim=pass header.i=@redhat.com header.s=mimecast20190719 header.b=iUu67gYV; arc=pass (i=1 spf=pass spfdomain=redhat.com dkim=pass dkdomain=redhat.com dmarc=pass fromdomain=redhat.com); spf=pass (google.com: domain of linux-kernel+bounces-159675-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) smtp.mailfrom="linux-kernel+bounces-159675-linux.lists.archive=gmail.com@vger.kernel.org"; dmarc=pass (p=NONE sp=NONE dis=NONE) header.from=redhat.com Return-Path: Received: from ny.mirrors.kernel.org (ny.mirrors.kernel.org. [2604:1380:45d1:ec00::1]) by mx.google.com with ESMTPS id h9-20020a05620a21c900b0079071fac07esi11791961qka.433.2024.04.26.01.07.19 for (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Fri, 26 Apr 2024 01:07:23 -0700 (PDT) Received-SPF: pass (google.com: domain of linux-kernel+bounces-159675-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) client-ip=2604:1380:45d1:ec00::1; Authentication-Results: mx.google.com; dkim=pass header.i=@redhat.com header.s=mimecast20190719 header.b=iUu67gYV; arc=pass (i=1 spf=pass spfdomain=redhat.com dkim=pass dkdomain=redhat.com dmarc=pass fromdomain=redhat.com); spf=pass (google.com: domain of linux-kernel+bounces-159675-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) smtp.mailfrom="linux-kernel+bounces-159675-linux.lists.archive=gmail.com@vger.kernel.org"; dmarc=pass (p=NONE sp=NONE dis=NONE) header.from=redhat.com Received: from smtp.subspace.kernel.org (wormhole.subspace.kernel.org [52.25.139.140]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ny.mirrors.kernel.org (Postfix) with ESMTPS id 483DE1C221A8 for ; Fri, 26 Apr 2024 08:07:19 +0000 (UTC) Received: from localhost.localdomain (localhost.localdomain [127.0.0.1]) by smtp.subspace.kernel.org (Postfix) with ESMTP id E09E713C90F; Fri, 26 Apr 2024 08:07:12 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; dkim=pass (1024-bit key) header.d=redhat.com header.i=@redhat.com header.b="iUu67gYV" Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.133.124]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 3C6C742A9B for ; Fri, 26 Apr 2024 08:07:09 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=170.10.133.124 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1714118831; cv=none; b=OFP4tXDcntgWrjQDuW8p+1c1VEhPd01OGLEFqIn/Xupfr1mLGj6XYcVy+vy9ZajYq7rJoYvZlCyJiZ32GHTOV/To6GCEULL/D7chichVbEQjK3Diz/KIaWiRiODClZbgq5N3+e8og+T8HeLYHSg0Ikge2cw6pHmoFQv7ViPHYIM= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1714118831; c=relaxed/simple; bh=TGAcZesai+U0xVAWvA7J/MAJQ7c0rregms0Ggpsl5S0=; h=Message-ID:Subject:From:To:Cc:Date:In-Reply-To:References: Content-Type:MIME-Version; b=Q3y7k5W+DML/T4GdKeOJtKo9zXhqLR98glDGKSIP2jmKiFy1ABEekdMPFlJTQbOr4HkJLH8n+UBSt7YBpmwk0PIMGy3VGQUEiCkxDNmt7OetilIjjtz/Pgo03rdQrHE5840mogEEF67ohTlvuDqQIpBmYYWR+h7tTFVypFTnGlE= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=redhat.com; spf=pass smtp.mailfrom=redhat.com; dkim=pass (1024-bit key) header.d=redhat.com header.i=@redhat.com header.b=iUu67gYV; arc=none smtp.client-ip=170.10.133.124 Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=redhat.com Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=redhat.com DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1714118829; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=TGAcZesai+U0xVAWvA7J/MAJQ7c0rregms0Ggpsl5S0=; b=iUu67gYVsOfcyjG+kN3vXsAneSSnXQkTVp9hfFTqttetZvbqD2FJIP0w/pGLoKykusMSgJ lZm92ZXTg09PD58PrG7IzWi32fqknR0KAfpDVvjz1+KJl88UGqzI+pF6zB0G+s0yE4a+F8 No+6drArGznBHC5gFVATD0FULwLFf0Y= Received: from mail-qt1-f199.google.com (mail-qt1-f199.google.com [209.85.160.199]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id us-mta-378--I1e8NbVMcym4a1Ad3exWA-1; Fri, 26 Apr 2024 04:07:07 -0400 X-MC-Unique: -I1e8NbVMcym4a1Ad3exWA-1 Received: by mail-qt1-f199.google.com with SMTP id d75a77b69052e-4379bbdc9b4so6614121cf.2 for ; Fri, 26 Apr 2024 01:07:07 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1714118827; x=1714723627; h=mime-version:user-agent:content-transfer-encoding:references :in-reply-to:date:cc:to:from:subject:message-id:x-gm-message-state :from:to:cc:subject:date:message-id:reply-to; bh=TGAcZesai+U0xVAWvA7J/MAJQ7c0rregms0Ggpsl5S0=; b=SjTVEb3NTS5R5zbyp/nnB63dCn0zOko1ibQyDo2W0b22Phwzo/V+wg9JWFq0Q63/8k /Ira1SwjCwUMgmGxDmGw0VOVYCV/qbcHcl3YNu1y50qDC0nxW8XqPJEC+hS8bq3Gc2dc 4lbZuV1J5HLByg4f1EKVzFopeBL5KariKaBJ89M0hnvtwLE4gPYBh0+92eiQTq0VnZay u8odDoPIc/PeLcoxgQBlS3X3BnCOISy2uEHqqkseM+fYe0xAWHYEVfZ5PFZk8tQSFSPs JCAzgnBqH9qE2oMHAmQ86KNdPKol8lOPKFsFMc9vSgZRiXVhu3iO8fKbcXnRdgbhd2kN YACQ== X-Forwarded-Encrypted: i=1; AJvYcCWM3GMA/q1j+2hN8MrsF7Hru/6QM5zRjnuUpwmbDTHcODRlmQnP+0TwuScctMtFBXMBTILEwfC7ST7vk3CkyEWeU6J3Ytb/HY9IFc0t X-Gm-Message-State: AOJu0YzHTegh4+7mRMTZNmDZG+MMyajMJW3sLGiThyoXdsEheB+nLeZ8 A3NBSDPSEDO0BPP6Wk69Kb1mheQ8h8wvvg985MbKLLkvV/AhxkSHeD4N1EC+fBB8FuNfOu1ogTI GH0WtktD9EzuGQ6RrAzsfRRNnTf0nuMuBjBjK2p4j7WTVR8ofJsWR7KC2AO64Fg== X-Received: by 2002:a05:6214:4008:b0:6a0:b2e4:583c with SMTP id kd8-20020a056214400800b006a0b2e4583cmr268841qvb.3.1714118826879; Fri, 26 Apr 2024 01:07:06 -0700 (PDT) X-Received: by 2002:a05:6214:4008:b0:6a0:b2e4:583c with SMTP id kd8-20020a056214400800b006a0b2e4583cmr268820qvb.3.1714118826480; Fri, 26 Apr 2024 01:07:06 -0700 (PDT) Received: from pstanner-thinkpadt14sgen1.remote.csb (nat-pool-muc-t.redhat.com. [149.14.88.26]) by smtp.gmail.com with ESMTPSA id y18-20020a0c9a92000000b0069f77a0079fsm7761670qvd.34.2024.04.26.01.07.04 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Fri, 26 Apr 2024 01:07:06 -0700 (PDT) Message-ID: <77ee27e2f155abd340b166e2856181577fdf08cc.camel@redhat.com> Subject: Re: [PATCH v6 00/10] Make PCI's devres API more consistent From: Philipp Stanner To: Bjorn Helgaas Cc: Hans de Goede , Maarten Lankhorst , Maxime Ripard , Thomas Zimmermann , David Airlie , Daniel Vetter , Bjorn Helgaas , Sam Ravnborg , dakr@redhat.com, dri-devel@lists.freedesktop.org, linux-kernel@vger.kernel.org, linux-pci@vger.kernel.org Date: Fri, 26 Apr 2024 10:07:02 +0200 In-Reply-To: <20240424201229.GA503230@bhelgaas> References: <20240424201229.GA503230@bhelgaas> Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable User-Agent: Evolution 3.48.4 (3.48.4-1.fc38) Precedence: bulk X-Mailing-List: linux-kernel@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 On Wed, 2024-04-24 at 15:12 -0500, Bjorn Helgaas wrote: > On Mon, Apr 08, 2024 at 10:44:12AM +0200, Philipp Stanner wrote: > > ... > > PCI's devres API suffers several weaknesses: > >=20 > > 1. There are functions prefixed with pcim_. Those are always > > managed > > =C2=A0=C2=A0 counterparts to never-managed functions prefixed with pci_= =E2=80=93 or > > so one > > =C2=A0=C2=A0 would like to think. There are some apparently unmanaged > > functions > > =C2=A0=C2=A0 (all region-request / release functions, and pci_intx()) w= hich > > =C2=A0=C2=A0 suddenly become managed once the user has initialized the = device > > with > > =C2=A0=C2=A0 pcim_enable_device() instead of pci_enable_device(). This > > "sometimes > > =C2=A0=C2=A0 yes, sometimes no" nature of those functions is confusing = and > > =C2=A0=C2=A0 therefore bug-provoking. In fact, it has already caused a = bug in > > DRM. > > =C2=A0=C2=A0 The last patch in this series fixes that bug. > > 2. iomappings: Instead of giving each mapping its own callback, the > > =C2=A0=C2=A0 existing API uses a statically allocated struct tracking o= ne > > mapping > > =C2=A0=C2=A0 per bar. This is not extensible. Especially, you can't cre= ate > > =C2=A0=C2=A0 _ranged_ managed mappings that way, which many drivers wan= t. > > 3. Managed request functions only exist as "plural versions" with a > > =C2=A0=C2=A0 bit-mask as a parameter. That's quite over-engineered > > considering > > =C2=A0=C2=A0 that each user only ever mapps one, maybe two bars. > >=20 > > This series: > > - add a set of new "singular" devres functions that use devres the > > way > > =C2=A0 its intended, with one callback per resource. > > - deprecates the existing iomap-table mechanism. > > - deprecates the hybrid nature of pci_ functions. > > - preserves backwards compatibility so that drivers using the > > existing > > =C2=A0 API won't notice any changes. > > - adds documentation, especially some warning users about the > > =C2=A0 complicated nature of PCI's devres. >=20 > There's a lot of good work here; thanks for working on it. Thanks! Good to get some more feedback from you >=20 > > Philipp Stanner (10): > > =C2=A0 PCI: Add new set of devres functions >=20 > This first patch adds some infrastructure and several new exported > interfaces: >=20 > =C2=A0 void __iomem *pcim_iomap_region(struct pci_dev *pdev, int bar, > const char *name) > =C2=A0 void pcim_iounmap_region(struct pci_dev *pdev, int bar) > =C2=A0 int pcim_request_region(struct pci_dev *pdev, int bar, const char > *name) > =C2=A0 void pcim_release_region(struct pci_dev *pdev, int bar) > =C2=A0 void __iomem *pcim_iomap_range(struct pci_dev *pdev, int bar, > =C2=A0 void __iomem *pcim_iomap_region_range(struct pci_dev *pdev, int > bar, > =C2=A0 void pcim_iounmap_region_range(struct pci_dev *pdev, int bar, >=20 > > =C2=A0 PCI: Deprecate iomap-table functions >=20 > This adds a little bit of infrastructure (add/remove to > legacy_table), > reimplements these existing interfaces: >=20 > =C2=A0 void __iomem *pcim_iomap(struct pci_dev *pdev, int bar, unsigned > long maxlen) > =C2=A0 void pcim_iounmap(struct pci_dev *pdev, void __iomem *addr) > =C2=A0 int pcim_iomap_regions(struct pci_dev *pdev, int mask, const char > *name) > =C2=A0 int pcim_iomap_regions_request_all(struct pci_dev *pdev, int mask, > =C2=A0 void pcim_iounmap_regions(struct pci_dev *pdev, int mask) >=20 > and adds a couple new exported interfaces: >=20 > =C2=A0 void pcim_release_all_regions(struct pci_dev *pdev) > =C2=A0 int pcim_request_all_regions(struct pci_dev *pdev, const char > *name) >=20 > There's a lot going on in these two patches, so they're hard to > review.=C2=A0 I think it would be easier if you could do the fixes to > existing interfaces first, I agree that the patches can be further split into smaller chunks to make them more atomic and easier to review. I can do that. BUT I'd need some more details about what you mean by "do the fixes first" =E2=80=93 which fixes? The later patches at least in part rely on the new better functions being available. > followed by adding new things, maybe > something like separate patches that: >=20 > =C2=A0 - Add pcim_addr_devres_alloc(), pcim_addr_devres_free(), > =C2=A0=C2=A0=C2=A0 pcim_addr_devres_clear(). >=20 > =C2=A0 - Add pcim_add_mapping_to_legacy_table(), > =C2=A0=C2=A0=C2=A0 pcim_remove_mapping_from_legacy_table(), > =C2=A0=C2=A0=C2=A0 pcim_remove_bar_from_legacy_table(). >=20 > =C2=A0 - Reimplement pcim_iomap(), pcim_iomap_regions(), pcim_iounmap(). >=20 > =C2=A0 - Add new interfaces like pcim_iomap_region(), > =C2=A0=C2=A0=C2=A0 pcim_request_region(), etc. >=20 > =C2=A0=C2=A0=C2=A0 AFAICS, except for pcim_iomap_range() (used by vbox), = these new > =C2=A0=C2=A0=C2=A0 interfaces have no users outside drivers/pci, so ... w= e might > =C2=A0=C2=A0=C2=A0 defer adding them, or at least defer exposing them via > =C2=A0=C2=A0=C2=A0 include/linux/pci.h, until we have users for them. Dropping (the export of) functions like pcim_request_region_range() or pcim_request_all_regions() is not a problem. What I quite fundamentally have to disagree with, however, is not to export the functions=C2=A0 * pcim_request_region() * pcim_iomap_region() the main point of this series is to deprecate that hybrid nature of those existing pci_* functions. You can only deprecate something when you provide users with new, better alternatives. Not exporting them would inevitably tempt driver programmers into using pcim_enable_device() + pci_*() as they did so far, which caused at least that leak in vboxvideo and another one that my plan was to address after we got this merged. Once we have those new pcim_ functions exported, we could successively port the older drivers which use the aforementioned combination with pcim_enable_device(). Then we could drop the hybrid nature of pci_ functions once and for all and would end up with a consistent, clean API. I intended to do that over the months after we merged this. So I'd suggest let me cancel the export of the "luxury functions" and let's keep the two listed above exported P. >=20 > > =C2=A0 PCI: Warn users about complicated devres nature > > =C2=A0 PCI: Make devres region requests consistent > > =C2=A0 PCI: Move dev-enabled status bit to struct pci_dev > > =C2=A0 PCI: Move pinned status bit to struct pci_dev > > =C2=A0 PCI: Give pcim_set_mwi() its own devres callback > > =C2=A0 PCI: Give pci(m)_intx its own devres callback > > =C2=A0 PCI: Remove legacy pcim_release() > > =C2=A0 drm/vboxvideo: fix mapping leaks > >=20 > > =C2=A0drivers/gpu/drm/vboxvideo/vbox_main.c |=C2=A0=C2=A0 20 +- > > =C2=A0drivers/pci/devres.c=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 | 1011 > > +++++++++++++++++++++---- > > =C2=A0drivers/pci/iomap.c=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 |=C2=A0=C2= =A0 18 + > > =C2=A0drivers/pci/pci.c=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 |= =C2=A0 123 ++- > > =C2=A0drivers/pci/pci.h=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 |= =C2=A0=C2=A0 21 +- > > =C2=A0include/linux/pci.h=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 |=C2=A0=C2= =A0 18 +- > > =C2=A06 files changed, 999 insertions(+), 212 deletions(-) > >=20 > > --=20 > > 2.44.0 > >=20 >=20