lists.openwall.net   lists  /  announce  owl-users  owl-dev  john-users  john-dev  passwdqc-users  yescrypt  popa3d-users  /  oss-security  kernel-hardening  musl  sabotage  tlsify  passwords  /  crypt-dev  xvendor  /  Bugtraq  Full-Disclosure  linux-kernel  linux-netdev  linux-ext4  linux-hardening  linux-cve-announce  PHC 
Open Source and information security mailing list archives
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date:   Fri, 14 Apr 2023 13:51:13 +0200
From:   Robert Richter <rrichter@....com>
To:     Terry Bowman <Terry.Bowman@....com>
CC:     Jonathan Cameron <Jonathan.Cameron@...wei.com>,
        <alison.schofield@...el.com>, <vishal.l.verma@...el.com>,
        <ira.weiny@...el.com>, <bwidawsk@...nel.org>,
        <dan.j.williams@...el.com>, <dave.jiang@...el.com>,
        <linux-cxl@...r.kernel.org>, <linux-kernel@...r.kernel.org>,
        <bhelgaas@...gle.com>
Subject: Re: [PATCH v3 1/6] cxl/pci: Add RCH downstream port AER and RAS
 register discovery

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@....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);
> >> +}
> >> +

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ