RE: [PATCH v5 8/9] PCI: Define scoped based management functions

RE: [PATCH v5 8/9] PCI: Define scoped based management functions

[Dan Williams]

[ add linux-pci ]

Ira Weiny wrote:
> Users of pci_dev_get() can benefit from a scoped based put.  Also,
> locking a PCI device is often done within a single scope.
> 
> Define a pci_dev_put() free function and a PCI device lock guard.  These
> will initially be used in new CXL event processing code but is defined
> in a separate patch for others to pickup and use/backport easier.

Hi Bjorn,

Any heartburn if I take this through cxl.git with the rest in this
series? Patch 9 has a dependency on this one.

> 
> Cc: Bjorn Helgaas      <bhelgaas@google.com>
> Signed-off-by: Ira Weiny <ira.weiny@intel.com>
> 
> ---
> Changes for v5:
> [Jonathan: New patch]
> ---
>  include/linux/pci.h | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/include/linux/pci.h b/include/linux/pci.h
> index 60ca768bc867..290d0a2651b2 100644
> --- a/include/linux/pci.h
> +++ b/include/linux/pci.h
> @@ -1170,6 +1170,7 @@ int pci_get_interrupt_pin(struct pci_dev *dev, struct pci_dev **bridge);
>  u8 pci_common_swizzle(struct pci_dev *dev, u8 *pinp);
>  struct pci_dev *pci_dev_get(struct pci_dev *dev);
>  void pci_dev_put(struct pci_dev *dev);
> +DEFINE_FREE(pci_dev_put, struct pci_dev *, if (_T) pci_dev_put(_T))
>  void pci_remove_bus(struct pci_bus *b);
>  void pci_stop_and_remove_bus_device(struct pci_dev *dev);
>  void pci_stop_and_remove_bus_device_locked(struct pci_dev *dev);
> @@ -1871,6 +1872,7 @@ void pci_cfg_access_unlock(struct pci_dev *dev);
>  void pci_dev_lock(struct pci_dev *dev);
>  int pci_dev_trylock(struct pci_dev *dev);
>  void pci_dev_unlock(struct pci_dev *dev);
> +DEFINE_GUARD(pci_dev, struct pci_dev *, pci_dev_lock(_T), pci_dev_unlock(_T))
>  
>  /*
>   * PCI domain support.  Sometimes called PCI segment (eg by ACPI),
> 
> -- 
> 2.43.0
> 

Re: [PATCH v5 8/9] PCI: Define scoped based management functions

[Bjorn Helgaas]

On Wed, Jan 03, 2024 at 02:38:57PM -0800, Dan Williams wrote:
> Ira Weiny wrote:
> > Users of pci_dev_get() can benefit from a scoped based put.  Also,
> > locking a PCI device is often done within a single scope.
> > 
> > Define a pci_dev_put() free function and a PCI device lock guard.  These
> > will initially be used in new CXL event processing code but is defined
> > in a separate patch for others to pickup and use/backport easier.
> 
> Any heartburn if I take this through cxl.git with the rest in this
> series? Patch 9 has a dependency on this one.

No real heartburn.  I was trying to figure out what this does
since I'm not familiar with the "scoped based put" idea and
'git grep -i "scope.*base"' wasn't much help.

I would kind of like the commit log to say a little more about what
the "scoped based put" (does that have too many past tenses in it?)
means and how users of pci_dev_get() will benefit.

I don't know what "locking a PCI device is often done within a single
scope" is trying to tell me either.  What if it's *not* done within a
single scope?

Does this change anything for callers of pci_dev_get() and
pci_dev_put()?

Does this avoid a common programming error?  I just don't know what
the benefit of this is yet.

I'm sure this is really cool stuff, but there's little documentation,
few existing users, and I don't know what I need to look for when
reviewing things.

> > --- a/include/linux/pci.h
> > +++ b/include/linux/pci.h
> > @@ -1170,6 +1170,7 @@ int pci_get_interrupt_pin(struct pci_dev *dev, struct pci_dev **bridge);
> >  u8 pci_common_swizzle(struct pci_dev *dev, u8 *pinp);
> >  struct pci_dev *pci_dev_get(struct pci_dev *dev);
> >  void pci_dev_put(struct pci_dev *dev);
> > +DEFINE_FREE(pci_dev_put, struct pci_dev *, if (_T) pci_dev_put(_T))
> >  void pci_remove_bus(struct pci_bus *b);
> >  void pci_stop_and_remove_bus_device(struct pci_dev *dev);
> >  void pci_stop_and_remove_bus_device_locked(struct pci_dev *dev);
> > @@ -1871,6 +1872,7 @@ void pci_cfg_access_unlock(struct pci_dev *dev);
> >  void pci_dev_lock(struct pci_dev *dev);
> >  int pci_dev_trylock(struct pci_dev *dev);
> >  void pci_dev_unlock(struct pci_dev *dev);
> > +DEFINE_GUARD(pci_dev, struct pci_dev *, pci_dev_lock(_T), pci_dev_unlock(_T))
> >  
> >  /*
> >   * PCI domain support.  Sometimes called PCI segment (eg by ACPI),
> > 
> > -- 
> > 2.43.0
> > 
> 
> 

Re: [PATCH v5 8/9] PCI: Define scoped based management functions

[Dan Williams]

Bjorn Helgaas wrote:
> On Wed, Jan 03, 2024 at 02:38:57PM -0800, Dan Williams wrote:
> > Ira Weiny wrote:
> > > Users of pci_dev_get() can benefit from a scoped based put.  Also,
> > > locking a PCI device is often done within a single scope.
> > > 
> > > Define a pci_dev_put() free function and a PCI device lock guard.  These
> > > will initially be used in new CXL event processing code but is defined
> > > in a separate patch for others to pickup and use/backport easier.
> > 
> > Any heartburn if I take this through cxl.git with the rest in this
> > series? Patch 9 has a dependency on this one.
> 
> No real heartburn.  I was trying to figure out what this does
> since I'm not familiar with the "scoped based put" idea and
> 'git grep -i "scope.*base"' wasn't much help.
> 
> I would kind of like the commit log to say a little more about what
> the "scoped based put" (does that have too many past tenses in it?)
> means and how users of pci_dev_get() will benefit.

That is completely fair, and I should have checked to make sure that the
changelog clarified the impact of the change.

> I don't know what "locking a PCI device is often done within a single
> scope" is trying to tell me either.  What if it's *not* done within a
> single scope?
> 
> Does this change anything for callers of pci_dev_get() and
> pci_dev_put()?
> 
> Does this avoid a common programming error?  I just don't know what
> the benefit of this is yet.
> 
> I'm sure this is really cool stuff, but there's little documentation,
> few existing users, and I don't know what I need to look for when
> reviewing things.

Here a is a re-write of the changelog:

---
PCI: Introduce cleanup helpers for device reference counts and locks

The "goto error" pattern is notorious for introducing subtle resource
leaks. Use the new cleanup.h helpers for PCI device reference counts and
locks.

Similar to the new put_device() and device_lock() cleanup helpers,
__free(put_device) and guard(device), define the same for PCI devices,
__free(pci_dev_put) and guard(pci_dev).  These helpers eliminate the
need for "goto free;" and "goto unlock;" patterns. For example, A
'struct pci_dev *' instance declared as:

	struct pci_dev *pdev __free(pci_dev_put) = NULL;

...will automatically call pci_dev_put() if @pdev is non-NULL when @pdev
goes out of scope (automatic variable scope). If a function wants to
invoke pci_dev_put() on error, but return @pdev on success, it can do:

	return no_free_ptr(pdev);

...or:

	return_ptr(pdev);

For potential cleanup opportunity there are 587 open-coded calls to
pci_dev_put() in the kernel with 65 instances within 10 lines of a goto
statement with the CXL driver threatening to add another one.

The guard() helper holds the associated lock for the remainder of the
current scope in which it was invoked. So, for example:

	func(...)
	{
		if (...) {
			...
			guard(pci_dev); /* pci_dev_lock() invoked here */
			...
		} /* <- implied pci_dev_unlock() triggered here */
	}

There are 15 invocations of pci_dev_unlock() in the kernel with 5
instances within 10 lines of a goto statement. Again, the CXL driver is
threatening to add another.

Introduce these helpers to preclude the addition of new more error prone
goto put; / goto unlock; sequences. For now, these helpers are used in
drivers/cxl/pci.c to allow ACPI error reports to be fed back into the
CXL driver associated with the PCI device identified in the report.

---

As for reviewing conversions that use these new helpers, one of the
gotchas I have found is that it is easy to make a mistake if a goto
still exists in the function after the conversion. So unless and until
all of the resources a function acquires, that currently need a goto to
unwind them on error, can be converted to cleanup.h based helpers it is
best not to mix styles.

I think the function documentation in include/linux/cleanup.h does a
decent job of explaining how to use the helpers. However, I am happy to
suggest some updates if you think it would help.

Re: [PATCH v5 8/9] PCI: Define scoped based management functions

[Ira Weiny]

Dan Williams wrote:
> Bjorn Helgaas wrote:
> > On Wed, Jan 03, 2024 at 02:38:57PM -0800, Dan Williams wrote:
> > > Ira Weiny wrote:
> > > > Users of pci_dev_get() can benefit from a scoped based put.  Also,
> > > > locking a PCI device is often done within a single scope.
> > > > 
> > > > Define a pci_dev_put() free function and a PCI device lock guard.  These
> > > > will initially be used in new CXL event processing code but is defined
> > > > in a separate patch for others to pickup and use/backport easier.
> > > 
> > > Any heartburn if I take this through cxl.git with the rest in this
> > > series? Patch 9 has a dependency on this one.
> > 
> > No real heartburn.  I was trying to figure out what this does
> > since I'm not familiar with the "scoped based put" idea and
> > 'git grep -i "scope.*base"' wasn't much help.
> > 
> > I would kind of like the commit log to say a little more about what
> > the "scoped based put" (does that have too many past tenses in it?)
> > means and how users of pci_dev_get() will benefit.
> 
> That is completely fair, and I should have checked to make sure that the
> changelog clarified the impact of the change.

Agreed.  Apologies for the brevity.

> 
> > I don't know what "locking a PCI device is often done within a single
> > scope" is trying to tell me either.  What if it's *not* done within a
> > single scope?

I was not trying to fix that but Dan covers it below indicating that the
pointer can be returned outside the scope if needed with no_free_ptr().

> > 
> > Does this change anything for callers of pci_dev_get() and
> > pci_dev_put()?

Current callers don't need to use this.

> > 
> > Does this avoid a common programming error?  I just don't know what
> > the benefit of this is yet.

Dan covered it well below.

> > 
> > I'm sure this is really cool stuff, but there's little documentation,
> > few existing users, and I don't know what I need to look for when
> > reviewing things.
> 
> Here a is a re-write of the changelog:

FWIW

Signed-off-by: Ira Weiny <ira.weiny@intel.com>

> 
> ---
> PCI: Introduce cleanup helpers for device reference counts and locks
> 
> The "goto error" pattern is notorious for introducing subtle resource
> leaks. Use the new cleanup.h helpers for PCI device reference counts and
> locks.
> 
> Similar to the new put_device() and device_lock() cleanup helpers,
> __free(put_device) and guard(device), define the same for PCI devices,
> __free(pci_dev_put) and guard(pci_dev).  These helpers eliminate the
> need for "goto free;" and "goto unlock;" patterns. For example, A
> 'struct pci_dev *' instance declared as:
> 
> 	struct pci_dev *pdev __free(pci_dev_put) = NULL;
> 
> ...will automatically call pci_dev_put() if @pdev is non-NULL when @pdev
> goes out of scope (automatic variable scope). If a function wants to
> invoke pci_dev_put() on error, but return @pdev on success, it can do:
> 
> 	return no_free_ptr(pdev);
> 
> ...or:
> 
> 	return_ptr(pdev);
> 
> For potential cleanup opportunity there are 587 open-coded calls to
> pci_dev_put() in the kernel with 65 instances within 10 lines of a goto
> statement with the CXL driver threatening to add another one.
> 
> The guard() helper holds the associated lock for the remainder of the
> current scope in which it was invoked. So, for example:
> 
> 	func(...)
> 	{
> 		if (...) {
> 			...
> 			guard(pci_dev); /* pci_dev_lock() invoked here */
> 			...
> 		} /* <- implied pci_dev_unlock() triggered here */
> 	}
> 
> There are 15 invocations of pci_dev_unlock() in the kernel with 5
> instances within 10 lines of a goto statement. Again, the CXL driver is
> threatening to add another.
> 
> Introduce these helpers to preclude the addition of new more error prone
> goto put; / goto unlock; sequences. For now, these helpers are used in
> drivers/cxl/pci.c to allow ACPI error reports to be fed back into the
> CXL driver associated with the PCI device identified in the report.
> 
> ---
> 
> As for reviewing conversions that use these new helpers, one of the
> gotchas I have found is that it is easy to make a mistake if a goto
> still exists in the function after the conversion. So unless and until
> all of the resources a function acquires, that currently need a goto to
> unwind them on error, can be converted to cleanup.h based helpers it is
> best not to mix styles.
> 
> I think the function documentation in include/linux/cleanup.h does a
> decent job of explaining how to use the helpers. However, I am happy to
> suggest some updates if you think it would help.

Re: [PATCH v5 8/9] PCI: Define scoped based management functions

[Bjorn Helgaas]

On Wed, Jan 03, 2024 at 04:21:02PM -0800, Dan Williams wrote:
> Bjorn Helgaas wrote:
> > On Wed, Jan 03, 2024 at 02:38:57PM -0800, Dan Williams wrote:
> > > Ira Weiny wrote:
> > > > Users of pci_dev_get() can benefit from a scoped based put.  Also,
> > > > locking a PCI device is often done within a single scope.
> > > > 
> > > > Define a pci_dev_put() free function and a PCI device lock guard.  These
> > > > will initially be used in new CXL event processing code but is defined
> > > > in a separate patch for others to pickup and use/backport easier.
> > > 
> > > Any heartburn if I take this through cxl.git with the rest in this
> > > series? Patch 9 has a dependency on this one.
> > 
> > No real heartburn.  I was trying to figure out what this does
> > since I'm not familiar with the "scoped based put" idea and
> > 'git grep -i "scope.*base"' wasn't much help.
> > 
> > I would kind of like the commit log to say a little more about what
> > the "scoped based put" (does that have too many past tenses in it?)
> > means and how users of pci_dev_get() will benefit.
> 
> That is completely fair, and I should have checked to make sure that the
> changelog clarified the impact of the change.

I see "scoped based put" follows a similar use in cleanup.h, but I
think "scope-based X" reads better.

> > I don't know what "locking a PCI device is often done within a single
> > scope" is trying to tell me either.  What if it's *not* done within a
> > single scope?
> > 
> > Does this change anything for callers of pci_dev_get() and
> > pci_dev_put()?
> > 
> > Does this avoid a common programming error?  I just don't know what
> > the benefit of this is yet.
> > 
> > I'm sure this is really cool stuff, but there's little documentation,
> > few existing users, and I don't know what I need to look for when
> > reviewing things.
> 
> Here a is a re-write of the changelog:
> 
> ---
> PCI: Introduce cleanup helpers for device reference counts and locks
> 
> The "goto error" pattern is notorious for introducing subtle resource
> leaks. Use the new cleanup.h helpers for PCI device reference counts and
> locks.
> 
> Similar to the new put_device() and device_lock() cleanup helpers,
> __free(put_device) and guard(device), define the same for PCI devices,
> __free(pci_dev_put) and guard(pci_dev).  These helpers eliminate the
> need for "goto free;" and "goto unlock;" patterns. For example, A
> 'struct pci_dev *' instance declared as:
> 
> 	struct pci_dev *pdev __free(pci_dev_put) = NULL;

I see several similar __free() uses with NULL initializations in gpio,
but I think this idiom would be slightly improved if the __free()
function were more closely associated with the actual pci_dev_get():

  struct pci_dev *pdev __free(pci_dev_put) = pci_get_device(...);

Not always possible, I know, but easier to analyze when it is.

> ...will automatically call pci_dev_put() if @pdev is non-NULL when @pdev
> goes out of scope (automatic variable scope). If a function wants to
> invoke pci_dev_put() on error, but return @pdev on success, it can do:
> 
> 	return no_free_ptr(pdev);
> 
> ...or:
> 
> 	return_ptr(pdev);
> 
> For potential cleanup opportunity there are 587 open-coded calls to
> pci_dev_put() in the kernel with 65 instances within 10 lines of a goto
> statement with the CXL driver threatening to add another one.
> 
> The guard() helper holds the associated lock for the remainder of the
> current scope in which it was invoked. So, for example:
> 
> 	func(...)
> 	{
> 		if (...) {
> 			...
> 			guard(pci_dev); /* pci_dev_lock() invoked here */
> 			...
> 		} /* <- implied pci_dev_unlock() triggered here */
> 	}

Thanks for this!  I had skimmed cleanup.h previously, but it makes a
lot more sense after your description here.  

I think a little introduction along these lines would be even more
useful in cleanup.h since the concept is general and not PCI-specific.
E.g., the motivation (avoid resource leaks with "goto error" pattern),
a definition of "__free() based cleanup function" (IIUC, a function to
be run when a variable goes out of scope), maybe something about
ordering (it's important in the "goto error" pattern that the cleanups
are done in a specific order; how does that translate to __free()?)

But the commit log above is fine with me.  (It does contain tabs,
which get slightly mangled when "git log" indents it.)

> There are 15 invocations of pci_dev_unlock() in the kernel with 5
> instances within 10 lines of a goto statement. Again, the CXL driver is
> threatening to add another.
> 
> Introduce these helpers to preclude the addition of new more error prone
> goto put; / goto unlock; sequences. For now, these helpers are used in
> drivers/cxl/pci.c to allow ACPI error reports to be fed back into the
> CXL driver associated with the PCI device identified in the report.

This part is also fine but doesn't seem strictly necessary to me.  I
think the part about error reports being fed back needs a lot more
background to understand the connection, and probably only makes sense
in the context of that patch.

> As for reviewing conversions that use these new helpers, one of the
> gotchas I have found is that it is easy to make a mistake if a goto
> still exists in the function after the conversion. So unless and until
> all of the resources a function acquires, that currently need a goto to
> unwind them on error, can be converted to cleanup.h based helpers it is
> best not to mix styles.
> 
> I think the function documentation in include/linux/cleanup.h does a
> decent job of explaining how to use the helpers. However, I am happy to
> suggest some updates if you think it would help.

Thanks, Dan!

Acked-by: Bjorn Helgaas <bhelgaas@google.com>

Re: [PATCH v5 8/9] PCI: Define scoped based management functions

[Dan Williams]

Bjorn Helgaas wrote:
[..]
> > ---
> > PCI: Introduce cleanup helpers for device reference counts and locks
> > 
> > The "goto error" pattern is notorious for introducing subtle resource
> > leaks. Use the new cleanup.h helpers for PCI device reference counts and
> > locks.
> > 
> > Similar to the new put_device() and device_lock() cleanup helpers,
> > __free(put_device) and guard(device), define the same for PCI devices,
> > __free(pci_dev_put) and guard(pci_dev).  These helpers eliminate the
> > need for "goto free;" and "goto unlock;" patterns. For example, A
> > 'struct pci_dev *' instance declared as:
> > 
> > 	struct pci_dev *pdev __free(pci_dev_put) = NULL;
> 
> I see several similar __free() uses with NULL initializations in gpio,
> but I think this idiom would be slightly improved if the __free()
> function were more closely associated with the actual pci_dev_get():
> 
>   struct pci_dev *pdev __free(pci_dev_put) = pci_get_device(...);
> 
> Not always possible, I know, but easier to analyze when it is.

I tend to agree, but it does lead to some long lines, for example:

diff --git a/drivers/cxl/pci.c b/drivers/cxl/pci.c
index 4fd1f207c84e..549ba4b8294e 100644
--- a/drivers/cxl/pci.c
+++ b/drivers/cxl/pci.c
@@ -975,15 +975,14 @@ static void cxl_cper_event_call(enum cxl_event_type ev_type,
 				struct cxl_cper_event_rec *rec)
 {
 	struct cper_cxl_event_devid *device_id = &rec->hdr.device_id;
-	struct pci_dev *pdev __free(pci_dev_put) = NULL;
 	enum cxl_event_log_type log_type;
 	struct cxl_dev_state *cxlds;
 	unsigned int devfn;
 	u32 hdr_flags;
 
 	devfn = PCI_DEVFN(device_id->device_num, device_id->func_num);
-	pdev = pci_get_domain_bus_and_slot(device_id->segment_num,
-					   device_id->bus_num, devfn);
+	struct pci_dev *pdev __free(pci_dev_put) = pci_get_domain_bus_and_slot(
+		device_id->segment_num, device_id->bus_num, devfn);
 	if (!pdev)
 		return;
 
...so I think people are choosing the "... __free(x) = NULL;" style for
code density readability.

> 
> > ...will automatically call pci_dev_put() if @pdev is non-NULL when @pdev
> > goes out of scope (automatic variable scope). If a function wants to
> > invoke pci_dev_put() on error, but return @pdev on success, it can do:
> > 
> > 	return no_free_ptr(pdev);
> > 
> > ...or:
> > 
> > 	return_ptr(pdev);
> > 
> > For potential cleanup opportunity there are 587 open-coded calls to
> > pci_dev_put() in the kernel with 65 instances within 10 lines of a goto
> > statement with the CXL driver threatening to add another one.
> > 
> > The guard() helper holds the associated lock for the remainder of the
> > current scope in which it was invoked. So, for example:
> > 
> > 	func(...)
> > 	{
> > 		if (...) {
> > 			...
> > 			guard(pci_dev); /* pci_dev_lock() invoked here */
> > 			...
> > 		} /* <- implied pci_dev_unlock() triggered here */
> > 	}
> 
> Thanks for this!  I had skimmed cleanup.h previously, but it makes a
> lot more sense after your description here.  
> 
> I think a little introduction along these lines would be even more
> useful in cleanup.h since the concept is general and not PCI-specific.

Ok, let me ponder an update here.

> E.g., the motivation (avoid resource leaks with "goto error" pattern),
> a definition of "__free() based cleanup function" (IIUC, a function to
> be run when a variable goes out of scope), maybe something about
> ordering (it's important in the "goto error" pattern that the cleanups
> are done in a specific order; how does that translate to __free()?)

The __free() callbacks are invoked in reverse declaration (FILO) order.
However, as I say that another reviewer recommendation falls out. Be
careful about the variable declaration order diverging from the init
order. 

I.e. save the reader from needing to wonder if there are intra variable
init order dependencies by making it clear that init order ==
declaration order.

> 
> But the commit log above is fine with me.  (It does contain tabs,
> which get slightly mangled when "git log" indents it.)
> 
> > There are 15 invocations of pci_dev_unlock() in the kernel with 5
> > instances within 10 lines of a goto statement. Again, the CXL driver is
> > threatening to add another.
> > 
> > Introduce these helpers to preclude the addition of new more error prone
> > goto put; / goto unlock; sequences. For now, these helpers are used in
> > drivers/cxl/pci.c to allow ACPI error reports to be fed back into the
> > CXL driver associated with the PCI device identified in the report.
> 
> This part is also fine but doesn't seem strictly necessary to me.  I
> think the part about error reports being fed back needs a lot more
> background to understand the connection, and probably only makes sense
> in the context of that patch.

Sure I can trim that out and just say that the CXL driver is one such
occasion where a new goto for pci_dev_put() and pci_dev_unlock() was
about to be introduced.

> > As for reviewing conversions that use these new helpers, one of the
> > gotchas I have found is that it is easy to make a mistake if a goto
> > still exists in the function after the conversion. So unless and until
> > all of the resources a function acquires, that currently need a goto to
> > unwind them on error, can be converted to cleanup.h based helpers it is
> > best not to mix styles.
> > 
> > I think the function documentation in include/linux/cleanup.h does a
> > decent job of explaining how to use the helpers. However, I am happy to
> > suggest some updates if you think it would help.
> 
> Thanks, Dan!
> 
> Acked-by: Bjorn Helgaas <bhelgaas@google.com>

Thanks, Bjorn!

Re: [PATCH v5 8/9] PCI: Define scoped based management functions

[Ira Weiny]

Dan Williams wrote:
> Bjorn Helgaas wrote:
> [..]
> > > ---
> > > PCI: Introduce cleanup helpers for device reference counts and locks
> > > 
> > > The "goto error" pattern is notorious for introducing subtle resource
> > > leaks. Use the new cleanup.h helpers for PCI device reference counts and
> > > locks.
> > > 
> > > Similar to the new put_device() and device_lock() cleanup helpers,
> > > __free(put_device) and guard(device), define the same for PCI devices,
> > > __free(pci_dev_put) and guard(pci_dev).  These helpers eliminate the
> > > need for "goto free;" and "goto unlock;" patterns. For example, A
> > > 'struct pci_dev *' instance declared as:
> > > 
> > > 	struct pci_dev *pdev __free(pci_dev_put) = NULL;
> > 
> > I see several similar __free() uses with NULL initializations in gpio,
> > but I think this idiom would be slightly improved if the __free()
> > function were more closely associated with the actual pci_dev_get():
> > 
> >   struct pci_dev *pdev __free(pci_dev_put) = pci_get_device(...);
> > 
> > Not always possible, I know, but easier to analyze when it is.
> 
> I tend to agree, but it does lead to some long lines, for example:
> 
> diff --git a/drivers/cxl/pci.c b/drivers/cxl/pci.c
> index 4fd1f207c84e..549ba4b8294e 100644
> --- a/drivers/cxl/pci.c
> +++ b/drivers/cxl/pci.c
> @@ -975,15 +975,14 @@ static void cxl_cper_event_call(enum cxl_event_type ev_type,
>  				struct cxl_cper_event_rec *rec)
>  {
>  	struct cper_cxl_event_devid *device_id = &rec->hdr.device_id;
> -	struct pci_dev *pdev __free(pci_dev_put) = NULL;
>  	enum cxl_event_log_type log_type;
>  	struct cxl_dev_state *cxlds;
>  	unsigned int devfn;
>  	u32 hdr_flags;
>  
>  	devfn = PCI_DEVFN(device_id->device_num, device_id->func_num);
> -	pdev = pci_get_domain_bus_and_slot(device_id->segment_num,
> -					   device_id->bus_num, devfn);
> +	struct pci_dev *pdev __free(pci_dev_put) = pci_get_domain_bus_and_slot(
> +		device_id->segment_num, device_id->bus_num, devfn);
>  	if (!pdev)
>  		return;
>  
> ...so I think people are choosing the "... __free(x) = NULL;" style for
> code density readability.
> 

Also in this case we need devfn assigned first.

Is the above patch compliant with current style guidelines?

Or would it be better to do?

diff --git a/drivers/cxl/pci.c b/drivers/cxl/pci.c
index b14237f824cf..8a180c6abb67 100644
--- a/drivers/cxl/pci.c
+++ b/drivers/cxl/pci.c
@@ -975,15 +975,14 @@ static void cxl_cper_event_call(enum cxl_event_type ev_type,
                                struct cxl_cper_event_rec *rec)
 {
        struct cper_cxl_event_devid *device_id = &rec->hdr.device_id;
-       struct pci_dev *pdev __free(pci_dev_put) = NULL;
        enum cxl_event_log_type log_type;
        struct cxl_dev_state *cxlds;
-       unsigned int devfn;
+       unsigned int devfn = PCI_DEVFN(device_id->device_num, device_id->func_num);
+       struct pci_dev *pdev __free(pci_dev_put) = pci_get_domain_bus_and_slot(
+                                                       device_id->segment_num,
+                                                       device_id->bus_num, devfn);
        u32 hdr_flags;
 
-       devfn = PCI_DEVFN(device_id->device_num, device_id->func_num);
-       pdev = pci_get_domain_bus_and_slot(device_id->segment_num,
-                                          device_id->bus_num, devfn);
        if (!pdev)
                return;
 

Ira

Re: [PATCH v5 8/9] PCI: Define scoped based management functions

[Bjorn Helgaas]

On Thu, Jan 04, 2024 at 01:46:56PM -0800, Ira Weiny wrote:
> Dan Williams wrote:
> > Bjorn Helgaas wrote:
> > [..]
> > > > ---
> > > > PCI: Introduce cleanup helpers for device reference counts and locks
> > > > 
> > > > The "goto error" pattern is notorious for introducing subtle resource
> > > > leaks. Use the new cleanup.h helpers for PCI device reference counts and
> > > > locks.
> > > > 
> > > > Similar to the new put_device() and device_lock() cleanup helpers,
> > > > __free(put_device) and guard(device), define the same for PCI devices,
> > > > __free(pci_dev_put) and guard(pci_dev).  These helpers eliminate the
> > > > need for "goto free;" and "goto unlock;" patterns. For example, A
> > > > 'struct pci_dev *' instance declared as:
> > > > 
> > > > 	struct pci_dev *pdev __free(pci_dev_put) = NULL;
> > > 
> > > I see several similar __free() uses with NULL initializations in gpio,
> > > but I think this idiom would be slightly improved if the __free()
> > > function were more closely associated with the actual pci_dev_get():
> > > 
> > >   struct pci_dev *pdev __free(pci_dev_put) = pci_get_device(...);
> > > 
> > > Not always possible, I know, but easier to analyze when it is.
> > 
> > I tend to agree, but it does lead to some long lines, for example:
> > 
> > diff --git a/drivers/cxl/pci.c b/drivers/cxl/pci.c
> > index 4fd1f207c84e..549ba4b8294e 100644
> > --- a/drivers/cxl/pci.c
> > +++ b/drivers/cxl/pci.c
> > @@ -975,15 +975,14 @@ static void cxl_cper_event_call(enum cxl_event_type ev_type,
> >  				struct cxl_cper_event_rec *rec)
> >  {
> >  	struct cper_cxl_event_devid *device_id = &rec->hdr.device_id;
> > -	struct pci_dev *pdev __free(pci_dev_put) = NULL;
> >  	enum cxl_event_log_type log_type;
> >  	struct cxl_dev_state *cxlds;
> >  	unsigned int devfn;
> >  	u32 hdr_flags;
> >  
> >  	devfn = PCI_DEVFN(device_id->device_num, device_id->func_num);
> > -	pdev = pci_get_domain_bus_and_slot(device_id->segment_num,
> > -					   device_id->bus_num, devfn);
> > +	struct pci_dev *pdev __free(pci_dev_put) = pci_get_domain_bus_and_slot(
> > +		device_id->segment_num, device_id->bus_num, devfn);
> >  	if (!pdev)
> >  		return;
> >  
> > ...so I think people are choosing the "... __free(x) = NULL;" style for
> > code density readability.
> > 
> 
> Also in this case we need devfn assigned first.
> 
> Is the above patch compliant with current style guidelines?
> 
> Or would it be better to do?
> 
> diff --git a/drivers/cxl/pci.c b/drivers/cxl/pci.c
> index b14237f824cf..8a180c6abb67 100644
> --- a/drivers/cxl/pci.c
> +++ b/drivers/cxl/pci.c
> @@ -975,15 +975,14 @@ static void cxl_cper_event_call(enum cxl_event_type ev_type,
>                                 struct cxl_cper_event_rec *rec)
>  {
>         struct cper_cxl_event_devid *device_id = &rec->hdr.device_id;
> -       struct pci_dev *pdev __free(pci_dev_put) = NULL;
>         enum cxl_event_log_type log_type;
>         struct cxl_dev_state *cxlds;
> -       unsigned int devfn;
> +       unsigned int devfn = PCI_DEVFN(device_id->device_num, device_id->func_num);
> +       struct pci_dev *pdev __free(pci_dev_put) = pci_get_domain_bus_and_slot(
> +                                                       device_id->segment_num,
> +                                                       device_id->bus_num, devfn);

I don't really care about this specific instance; my comment was more
about the commit log for the "Define scope based management functions"
patch, thinking maybe the example could encourage get/put togetherness
when it's practical.

Bjorn

Re: [PATCH v5 8/9] PCI: Define scoped based management functions

[Ira Weiny]

Bjorn Helgaas wrote:
> On Thu, Jan 04, 2024 at 01:46:56PM -0800, Ira Weiny wrote:
> > Dan Williams wrote:
> > > Bjorn Helgaas wrote:

[snip]

> > 
> > Also in this case we need devfn assigned first.
> > 
> > Is the above patch compliant with current style guidelines?
> > 
> > Or would it be better to do?
> > 
> > diff --git a/drivers/cxl/pci.c b/drivers/cxl/pci.c
> > index b14237f824cf..8a180c6abb67 100644
> > --- a/drivers/cxl/pci.c
> > +++ b/drivers/cxl/pci.c
> > @@ -975,15 +975,14 @@ static void cxl_cper_event_call(enum cxl_event_type ev_type,
> >                                 struct cxl_cper_event_rec *rec)
> >  {
> >         struct cper_cxl_event_devid *device_id = &rec->hdr.device_id;
> > -       struct pci_dev *pdev __free(pci_dev_put) = NULL;
> >         enum cxl_event_log_type log_type;
> >         struct cxl_dev_state *cxlds;
> > -       unsigned int devfn;
> > +       unsigned int devfn = PCI_DEVFN(device_id->device_num, device_id->func_num);
> > +       struct pci_dev *pdev __free(pci_dev_put) = pci_get_domain_bus_and_slot(
> > +                                                       device_id->segment_num,
> > +                                                       device_id->bus_num, devfn);
> 
> I don't really care about this specific instance; my comment was more
> about the commit log for the "Define scope based management functions"
> patch, thinking maybe the example could encourage get/put togetherness
> when it's practical.

Ok thanks!
Ira