linux-kernel - Re: [PATCH v8 03/13] PCI: Reimplement plural devres functions

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date: Wed, 12 Jun 2024 15:42:35 -0500
From: Bjorn Helgaas <helgaas@...nel.org>
To: Philipp Stanner <pstanner@...hat.com>
Cc: Hans de Goede <hdegoede@...hat.com>,
	Maarten Lankhorst <maarten.lankhorst@...ux.intel.com>,
	Maxime Ripard <mripard@...nel.org>,
	Thomas Zimmermann <tzimmermann@...e.de>,
	David Airlie <airlied@...il.com>, Daniel Vetter <daniel@...ll.ch>,
	Bjorn Helgaas <bhelgaas@...gle.com>,
	Sam Ravnborg <sam@...nborg.org>, dakr@...hat.com,
	dri-devel@...ts.freedesktop.org, linux-kernel@...r.kernel.org,
	linux-pci@...r.kernel.org
Subject: Re: [PATCH v8 03/13] PCI: Reimplement plural devres functions

On Wed, Jun 12, 2024 at 10:51:40AM +0200, Philipp Stanner wrote:
> On Tue, 2024-06-11 at 16:44 -0500, Bjorn Helgaas wrote:
> > I'm trying to merge these into pci/next, but I'm having a hard time
> > writing the merge commit log.  I want a one-sentence description of
> > each patch that tells me what the benefit of the patch is.  Usually
> > the subject line is a good start.
> > 
> > "Reimplement plural devres functions" is kind of vague and doesn't
> > quite motivate this patch, and I'm having a hard time extracting the
> > relevant details from the commit log below.
> 
> I would say that the summary would be something along the lines:
> "Set ground layer for devres simplification and extension"
> 
> because this patch simplifies the existing functions and adds
> infrastructure that can later be used to deprecate the bloated existing
> functions, remove the hybrid mechanism and add pcim_iomap_range().

I think something concrete like "Add partial-BAR devres support" would
give people a hint about what to look for.

This patch contains quite a bit more than that, and if it were
possible, it might be nice to split the rest to a different patch, but
I'm not sure it's even possible and I just want to get this series out
the door.

If the commit log includes the partial-BAR idea and the specific
functions added, I think that will hold together.  And then it makes
sense for why the "plural" functions would be implemented on top of
the "singular" ones.

> > > Implement a set of singular functions 
> > 
> > What is this set of functions?  My guess is below.
> > 
> > > that use devres as it's intended and
> > > use those singular functions to reimplement the plural functions.
> > 
> > What does "as it's intended" mean?  Too nebulous to fit here.
> 
> Well, the idea behind devres is that you allocate a device resource
> _for each_ object you want to be freed / deinitialized automatically.
> One devres object per driver / subsystem object, one devres callback
> per cleanup job for the driver / subsystem.
> 
> What PCI devres did instead was to use just ONE devres object _for
> everything_ and then it had to implement all sorts of checks to check
> which sub-resource this master resource is actually about:
> 
> (from devres.c)
> static void pcim_release(struct device *gendev, void *res)
> {
> 	struct pci_dev *dev = to_pci_dev(gendev);
> 	struct pci_devres *this = res;
> 	int i;
> 
> 	for (i = 0; i < DEVICE_COUNT_RESOURCE; i++)
> 		if (this->region_mask & (1 << i))
> 			pci_release_region(dev, i);
> 
> 	if (this->mwi)
> 		pci_clear_mwi(dev);
> 
> 	if (this->restore_intx)
> 		pci_intx(dev, this->orig_intx);
> 
> 	if (this->enabled && !this->pinned)
> 		pci_disable_device(dev);
> }
> 
> 
> So one could dare to say that devres was partially re-implemented on
> top of devres.
> 
> The for-loop and the if-conditions constitute that "re-implementation".
> No one has any clue why it has been done that way, because it provides
> 0 upsides and would have been far easier to implement by just letting
> devres do its job.
> 
> Would you like to see the above details in the commit message?

No.  Just remove the "use devres as it's intended" since that's not
needed to motivate this patch.  I think we need fewer and
more-specific words.

Bjorn