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
