Message-ID: <ef32b9184700a07048fbb387f7d42410f7db308d.camel@redhat.com>
Date: Thu, 13 Jun 2024 08:51:02 +0200
From: Philipp Stanner <pstanner@...hat.com>
To: Bjorn Helgaas <helgaas@...nel.org>
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, 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
> 

Powered by blists - more mailing lists