Received-SPF: pass (google.com: domain of linux-kernel+bounces-31617-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223;
Date: Fri, 19 Jan 2024 16:52:50 -0600
From: Bjorn Helgaas <helgaas@kernel.org>
To: Philipp Stanner <pstanner@redhat.com>
Cc: Jonathan Corbet <corbet@lwn.net>, Hans de Goede <hdegoede@redhat.com>,
	Maarten Lankhorst <maarten.lankhorst@linux.intel.com>,
	Maxime Ripard <mripard@kernel.org>,
	Thomas Zimmermann <tzimmermann@suse.de>,
	David Airlie <airlied@gmail.com>, Daniel Vetter <daniel@ffwll.ch>,
	Bjorn Helgaas <bhelgaas@google.com>,
	Sam Ravnborg <sam@ravnborg.org>, dakr@redhat.com,
	linux-pci@vger.kernel.org, linux-kernel@vger.kernel.org,
	dri-devel@lists.freedesktop.org, linux-doc@vger.kernel.org
Subject: Re: [PATCH 01/10] pci: add new set of devres functions
Message-ID: <20240119225250.GA191270@bhelgaas>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <1983517bf5d0c98894a7d40fbec353ad75160cb4.camel@redhat.com>

On Wed, Jan 17, 2024 at 09:54:47AM +0100, Philipp Stanner wrote:
> On Tue, 2024-01-16 at 12:44 -0600, Bjorn Helgaas wrote:
> > On Mon, Jan 15, 2024 at 03:46:12PM +0100, Philipp Stanner wrote:
> > > PCI's devres API is not extensible to ranged mappings and has
> > > bug-provoking features. Improve that by providing better
> > > alternatives.
> > 
> > I guess "ranged mappings" means a mapping that doesn't cover an
> > entire BAR?  Maybe there's a way to clarify?
> 
> That's what it's supposed to mean, yes.  We could give it the longer
> title "mappings smaller than the whole BAR" or something, I guess.

"partial BAR mappings"?

> > > to the creation of a set of "pural functions" such as

s/pural/plural/ (I missed this before).

> > >         c) The iomap-table mechanism is over-engineered,
> > > complicated and
> > >            can by definition not perform bounds checks, thus,
> > > provoking
> > >            memory faults: pcim_iomap_table(pdev)[42]
> > 
> > Not sure what "pcim_iomap_table(pdev)[42]" means.
> 
> That function currently is implemented with this prototype:
> void __iomem * const *pcim_iomap_table(struct pci_dev *pdev);
> 
> And apparently, it's intended to index directly over the function. And
> that's how at least part of the users use it indeed.
> 
> Here in drivers/crypto/inside-secure/safexcel.c, L.1919 for example:
> 
> 	priv->base = pcim_iomap_table(pdev)[0];
> 
> I've never seen something that wonderful in C ever before, so it's not
> surprising that you weren't sure what I mean....
> 
> pcim_iomap_table() can not and does not perform any bounds check. If
> you do
> 
> void __iomem *mappy_map_mapface = pcim_iomap_table(pdev)[42];
> 
> then it will just return random garbage, or it faults. No -EINVAL or
> anything. You won't even get NULL.
> 
> That's why this function must die.

No argument except that this example only makes sense after one looks
up the prototype and connects the dots.

Bjorn