From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1758556AbcAYTau (ORCPT ); Mon, 25 Jan 2016 14:30:50 -0500 Received: from mail-pa0-f66.google.com ([209.85.220.66]:34173 "EHLO mail-pa0-f66.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1758537AbcAYTaq (ORCPT ); Mon, 25 Jan 2016 14:30:46 -0500 Date: Mon, 25 Jan 2016 11:30:42 -0800 From: Brian Norris To: "Maciej W. Rozycki" Cc: =?utf-8?B?UmFmYcWCIE1pxYJlY2tp?= , linux-mtd@lists.infradead.org, Javier Martinez Canillas , Linux Kernel Mailing List , Fengguang Wu , Michael Ellerman , Luis de Bethencourt , Jeremy Kerr , Neelesh Gupta , David Woodhouse , Cyril Bur , Ralf Baechle Subject: Re: [PATCH] mtd: bcm47xxsflash: use ioremap_cachable() instead of KSEG0ADDR() Message-ID: <20160125193042.GA47436@google.com> References: <1452991370-20121-1-git-send-email-zajec5@gmail.com> <20160125040459.GA4386@localhost> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.5.21 (2010-09-15) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Mon, Jan 25, 2016 at 07:15:49PM +0000, Maciej W. Rozycki wrote: > On Mon, 25 Jan 2016, Brian Norris wrote: > > > > > + 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. > > Why does `devm_ioremap_resource' (in lib/devres.c) do that manually then? > > dest_ptr = devm_ioremap(dev, res->start, size); > if (!dest_ptr) { > dev_err(dev, "ioremap failed for resource %pR\n", res); > devm_release_mem_region(dev, res->start, size); > dest_ptr = IOMEM_ERR_PTR(-ENOMEM); > } > > Is this an oversight or was it a deliberate design decision? I didn't design it, but I suspect it's because the API for devm_ioremap_resource() doesn't assume that a failed devm_ioremap_resource() means probe() will exit. Perhaps the driver will have some alternative action instead. It's still good to release the resources implicitly requested by devm_ioremap_resource(). The difference for us is that we *know* we're exiting the probe(), so we can rely on the automatic cleanup. > > 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. > > As from 4.5-rc1 we now have `ioremap_cache' available for MIPS as well > (thanks, Ralf, for a quick action on that!), so you can use that instead > to make your code generic. OK, but there's still not a devm_* version of ioremap_cache(), so we'd still need to do the iounmap() ourselves. Also, it looks like kernel/memremap.c is suggesting we use memremap() instead of ioremap_cache()... But I'm not sure if that's really what we want. Brian