Re: [PATCH v3 1/6] cxl/pci: Add RCH downstream port AER and RAS register discovery

[Robert Richter <rrichter@amd.com>]

On 13.04.23 14:13:16, Terry Bowman wrote:
> On 4/13/23 10:30, Jonathan Cameron wrote:
> > On Tue, 11 Apr 2023 13:02:57 -0500
> > Terry Bowman <terry.bowman@amd.com> wrote:

> >> +static void __iomem *cxl_map_reg(struct device *dev, struct cxl_register_map *map,
> >> +				 char *name)
> > 
> > dev isn't used.
> > 
> 
> 'dev' was used earlier for logging that is since removed.
> 
> >> +{
> >> +
> > 
> > Trivial but no point in blank line here.
> > 
> 
> I'll remove it.
> 
> >> +	if (!request_mem_region(map->resource, map->max_size, name))
> >> +		return NULL;
> >> +
> >> +	map->base = ioremap(map->resource, map->max_size);
> >> +	if (!map->base) {
> >> +		release_mem_region(map->resource, map->max_size);
> >> +		return NULL;
> >> +	}
> >> +
> >> +	return map->base;
> > 
> > Why return a value you've already stashed in map->base?
> > 
> This allowed for a clean return check where cxl_map_reg() is called.
> This could/should have been a boolean. This will be fixed with the refactoring 
> mentioned below.

The intention was to have a shortcut to get the base addr directly
which could be often the case. While the remaining struct map is only
used to unmap things. To be precise, we do not check a bool here but
instead an address to be non-zero. Please to not change the return
value.

We did not use devm_* here to allow temporary mappings during init
(which might happen multiple times). With devm_* only one permanent
mapping would be possible and we would need to store and maintain the
base addr in some struct. This implementation here allows a local
usage.

> 
> >> +}
> >> +
> > 
> > This is similar enough to devm_cxl_iomap_block() that I'd kind
> > of like them them take the same parameters.  That would mean
> > moving the map structure outside of the calls and instead passing
> > in the 3 relevant parameters.  Perhaps not worth it.
> > 
> The intent was to cleanup the cxl_map_reg() callers.  Using a 'struct 
> cxl_register_map' carries all the variables required for mapping and reduces 
> the number of variables otherwise declared in the callers. But, I understand 
> why a common interface is preferred in this case.
> 
> Ok. I'll change the parameters and return value to match devm_cxl_iomap_block(). 

See my comment above.

Struct cxl_register_map was choosen to keep data in one place and also
for paired use with cxl_map_reg() and cxl_unmap_reg() (in the sense of
an object-oriented programming style). The struct is widespread used
in CXL code for similar reasons. I would prefer to keep the struct as
argument.

> 
> >> +static void cxl_unmap_reg(struct device *dev, struct cxl_register_map *map)
> >> +{
> > 
> > dev isn't used here either. Makes little sense to pass it in to either funtion.

Yes, dev should be removed for both functions. Thanks for catching
this.

-Robert

> > 
> >> +	iounmap(map->base);
> >> +	release_mem_region(map->resource, map->max_size);
> >> +}
> >> +