Re: [PATCH RFC v2] platform/x86: p2sb: Allow p2sb_bar() calls during PCI device probe

[Andy Shevchenko <andriy.shevchenko@linux.intel.com>]

On Fri, Dec 15, 2023 at 07:33:37AM +0000, Shinichiro Kawasaki wrote:
> On Dec 14, 2023 / 18:38, Andy Shevchenko wrote:
> > On Tue, Dec 12, 2023 at 08:47:46PM +0900, Shin'ichiro Kawasaki wrote:

...

> > > +/* Cache BAR0 of P2SB device from function 0 ot 7 */
> > > +#define NR_P2SB_RES_CACHE 8
> > 
> > This is fifth or so definition for the same, isn't it a good time to create
> > a treewide definition in pci.h?
> > 
> > See also below.
> > 
> > (In previous mail I even found all cases and listed, a bit lazy to repeat.)
> 
> I'm not sure where are other definitions. I guess PCI_CONF1_FUNC_SHIFT in
> drivers/pci/pci.h is one of them.

Maybe, but I'm talking about treewide (drivers/, arch/, etc) duplications
IIRC it is 5+ of them.

> As you suggested in another mail, I'll add a
> TODO comment and note that the NR_P2SB_RES_CACHE should be refactored later.

Yeah, there are two ways, make this one dependent on the definition or
introduce a definition and update later on. In the latter case it means
a technical debt +.

In the lost email I suggested to list all existing cases somewhere in TODO,
so we won't grep again, but okay we can do that (although I forgot the
exact `git grep ...` command line I have used for that search).

...

> > > -	/* if @bus is NULL, use bus 0 in domain 0 */
> > > -	bus = bus ?: pci_find_bus(0, 0);
> > > +	/* Assume P2SB is on the bus 0 in domain 0 */
> > > +	bus = pci_find_bus(0, 0);
> > 
> > The pci_find_bus() is called in two places now. Can we avoid doing
> > this duplication?
> 
> I will add a global variable "static struct pci_bus *p2sb_bus". It will keep the
> first call return value and allow to skip the second call.

Hmm... Global variables are prone to subtle race conditions, OTOH the P2SB is
not supposed to be hotplugged or moved, so I think it will be fine.

-- 
With Best Regards,
Andy Shevchenko