Received-SPF: pass (google.com: domain of linux-kernel+bounces-212655-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223;
Message-ID: <ef32b9184700a07048fbb387f7d42410f7db308d.camel@redhat.com>
Subject: Re: [PATCH v8 03/13] PCI: Reimplement plural devres functions
From: Philipp Stanner <pstanner@redhat.com>
To: Bjorn Helgaas <helgaas@kernel.org>
Cc: Hans de Goede <hdegoede@redhat.com>, Maarten Lankhorst
 <maarten.lankhorst@linux.intel.com>, Maxime Ripard <mripard@kernel.org>, 
 Thomas Zimmermann <tzimmermann@suse.de>, David Airlie <airlied@gmail.com>,
 Daniel Vetter <daniel@ffwll.ch>, Bjorn Helgaas <bhelgaas@google.com>, Sam
 Ravnborg <sam@ravnborg.org>, dakr@redhat.com, 
 dri-devel@lists.freedesktop.org, linux-kernel@vger.kernel.org, 
 linux-pci@vger.kernel.org
Date: Thu, 13 Jun 2024 08:51:02 +0200
In-Reply-To: <20240612204235.GA1037175@bhelgaas>
References: <20240612204235.GA1037175@bhelgaas>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
User-Agent: Evolution 3.48.4 (3.48.4-1.fc38) 
Precedence: bulk
MIME-Version: 1.0

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.=C2=A0 I want a one-sentence description
> > > of
> > > each patch that tells me what the benefit of the patch is.=C2=A0
> > > Usually
> > > the subject line is a good start.
> > >=20
> > > "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.
> >=20
> > I would say that the summary would be something along the lines:
> > "Set ground layer for devres simplification and extension"
> >=20
> > 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().
>=20
> I think something concrete like "Add partial-BAR devres support"
> would
> give people a hint about what to look for.

Okay, will do.

>=20
> 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=C2=A0

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 ^^'

>=20
> If the commit log includes the partial-BAR idea and the specific
> functions added, I think that will hold together.=C2=A0 And then it makes
> sense for why the "plural" functions would be implemented on top of
> the "singular" ones.
>=20
> > > > Implement a set of singular functions=20
> > >=20
> > > What is this set of functions?=C2=A0 My guess is below.
> > >=20
> > > > that use devres as it's intended and
> > > > use those singular functions to reimplement the plural
> > > > functions.
> > >=20
> > > What does "as it's intended" mean?=C2=A0 Too nebulous to fit here.
> >=20
> > 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.
> >=20
> > 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:
> >=20
> > (from devres.c)
> > static void pcim_release(struct device *gendev, void *res)
> > {
> > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0struct pci_dev *dev =3D=
 to_pci_dev(gendev);
> > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0struct pci_devres *this=
 =3D res;
> > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0int i;
> >=20
> > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0for (i =3D 0; i < DEVIC=
E_COUNT_RESOURCE; i++)
> > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=
=A0=C2=A0=C2=A0=C2=A0=C2=A0if (this->region_mask & (1 << i))
> > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=
=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=
pci_release_region(dev, i);
> >=20
> > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0if (this->mwi)
> > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=
=A0=C2=A0=C2=A0=C2=A0=C2=A0pci_clear_mwi(dev);
> >=20
> > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0if (this->restore_intx)
> > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=
=A0=C2=A0=C2=A0=C2=A0=C2=A0pci_intx(dev, this->orig_intx);
> >=20
> > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0if (this->enabled && !t=
his->pinned)
> > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=
=A0=C2=A0=C2=A0=C2=A0=C2=A0pci_disable_device(dev);
> > }
> >=20
> >=20
> > So one could dare to say that devres was partially re-implemented
> > on
> > top of devres.
> >=20
> > 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.
> >=20
> > Would you like to see the above details in the commit message?
>=20
> No.=C2=A0 Just remove the "use devres as it's intended" since that's not
> needed to motivate this patch.=C2=A0 I think we need fewer and
> more-specific words.

ACK. I will rework it


Thank you Bjorn for your time and effort,

P.


>=20
> Bjorn
>=20