On Wed, 2024-06-12 at 15:42 -0500, Bjorn Helgaas wrote:
> 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.
Okay, will do.
>
> 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
I tried and got screamed at by the build chain because of dead code. So
I don't really think they can be split more, unfortunately.
In possibly following series's to PCI I'll pay attention to design
things as atomically as possible from the start.
> and I just want to get this series out
> the door.
That's actually something you and I have in common. I have been working
on the preparations for this since November last year ^^'
>
> 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.
ACK. I will rework it
Thank you Bjorn for your time and effort,
P.
>
> Bjorn
>
