linux-kernel - Re: [PATCH] mtd: bcm47xxsflash: use ioremap

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]

Message-ID: <20160125040459.GA4386@localhost>
Date:	Sun, 24 Jan 2016 20:04:59 -0800
From:	Brian Norris <computersforpeace@...il.com>
To:	"Maciej W. Rozycki" <macro@...tec.com>
Cc:	Rafał Miłecki <zajec5@...il.com>,
	linux-mtd@...ts.infradead.org,
	Javier Martinez Canillas <javier@....samsung.com>,
	Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
	Fengguang Wu <fengguang.wu@...el.com>,
	Michael Ellerman <mpe@...erman.id.au>,
	Luis de Bethencourt <luisbg@....samsung.com>,
	Jeremy Kerr <jk@...abs.org>,
	Neelesh Gupta <neelegup@...ux.vnet.ibm.com>,
	David Woodhouse <dwmw2@...radead.org>,
	Cyril Bur <cyrilbur@...il.com>,
	Ralf Baechle <ralf@...ux-mips.org>
Subject: Re: [PATCH] mtd: bcm47xxsflash: use ioremap_cachable() instead of
 KSEG0ADDR()

On Sun, Jan 24, 2016 at 08:17:37PM +0000, Maciej W. Rozycki wrote:
> On Sun, 17 Jan 2016, Rafał Miłecki wrote:
[...]

> > diff --git a/drivers/mtd/devices/bcm47xxsflash.c b/drivers/mtd/devices/bcm47xxsflash.c
> > index 347bb83..84f8fae 100644
> > --- a/drivers/mtd/devices/bcm47xxsflash.c
> > +++ b/drivers/mtd/devices/bcm47xxsflash.c
> > @@ -275,15 +275,33 @@ static void bcm47xxsflash_bcma_cc_write(struct bcm47xxsflash *b47s, u16 offset,
> >  
> >  static int bcm47xxsflash_bcma_probe(struct platform_device *pdev)
> >  {
> > -	struct bcma_sflash *sflash = dev_get_platdata(&pdev->dev);
> > +	struct device *dev = &pdev->dev;
> > +	struct bcma_sflash *sflash = dev_get_platdata(dev);
> >  	struct bcm47xxsflash *b47s;
> > +	struct resource *res;
> >  	int err;
> >  
> > -	b47s = devm_kzalloc(&pdev->dev, sizeof(*b47s), GFP_KERNEL);
> > +	b47s = devm_kzalloc(dev, sizeof(*b47s), GFP_KERNEL);
> >  	if (!b47s)
> >  		return -ENOMEM;
> >  	sflash->priv = b47s;
> >  
> > +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > +	if (!res) {
> > +		dev_err(dev, "invalid resource\n");
> > +		return -EINVAL;
> > +	}
> > +	if (!devm_request_mem_region(dev, res->start, resource_size(res),
> > +				     res->name)) {
> > +		dev_err(dev, "can't request region for resource %pR\n", res);
> > +		return -EBUSY;
> > +	}
> > +	b47s->window = ioremap_cachable(res->start, resource_size(res));
> > +	if (!b47s->window) {
> > +		dev_err(dev, "ioremap failed for resource %pR\n", res);
> 
>  You need to call `devm_release_mem_region' in this case.

No he doesn't. devm_* functions automatically release their resources
when either the device is removed, or the probe() fails. So the whole
point is that we don't have to explicitly manage the error case.

But...since he's not using a devm_* version of ioremap (there isn't one
for ioremap_cachable()), we actually need to add an iounmap() for the
case where mtd_device_parse_register() fails. If we fix that one, I can
apply this.

> > +		return -ENOMEM;
> > +	}
> > +
> >  	b47s->bcma_cc = container_of(sflash, struct bcma_drv_cc, sflash);
> >  	b47s->cc_read = bcm47xxsflash_bcma_cc_read;
> >  	b47s->cc_write = bcm47xxsflash_bcma_cc_write;
> 
>  LGTM otherwise.

Thanks for the review!

Brian