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

[Bjorn Helgaas <helgaas@kernel.org>]

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