RE: [PATCH 06/16] x86/amd_nb: Simplify root device search

["Ilpo Järvinen" <ilpo.jarvinen@linux.intel.com>]

On Thu, 31 Oct 2024, Zhuo, Qiuxu wrote:

> > From: Yazen Ghannam <yazen.ghannam@amd.com>
> > [...]
> > +struct pci_dev *amd_node_get_root(u16 node) {
> > +	struct pci_dev *df_f0 __free(pci_dev_put) = NULL;
> 
> NULL pointer initialization is not necessary.

It is, because __free() is used...

> > +	struct pci_dev *root;
> > +	u16 cntl_off;
> > +	u8 bus;
> > +
> > +	if (!boot_cpu_has(X86_FEATURE_ZEN))
> > +		return NULL;

...This would try to free() whatever garbage df_f0 holds...

> > +	/*
> > +	 * D18F0xXXX [Config Address Control] (DF::CfgAddressCntl)
> > +	 * Bits [7:0] (SecBusNum) holds the bus number of the root device for
> > +	 * this Data Fabric instance. The segment, device, and function will be
> > 0.
> > +	 */
> > +	df_f0 = amd_node_get_func(node, 0);

...However, the recommended practice when using __free() is this (as 
documented in include/linux/cleanup.h):

 * Given that the "__free(...) = NULL" pattern for variables defined at
 * the top of the function poses this potential interdependency problem
 * the recommendation is to always define and assign variables in one
 * statement and not group variable definitions at the top of the
 * function when __free() is used.

I know the outcome will look undesirable to some, me included, but 
there's little that can be done to that because there's no other way for 
the compiler to infer the order.

That being said, strictly speaking it isn't causing issue in this function 
as is but it's still a bad pattern to initialize to = NULL because in 
other instances it will cause problems. So better to steer away from the
pattern entirely rather than depend on reviewers noticing the a cleaup 
ordering problem gets introduced by some later change to the function.

> > +	if (!df_f0)
> > +		return NULL;


-- 
 i.