From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751367AbbEZIyb (ORCPT ); Tue, 26 May 2015 04:54:31 -0400 Received: from metis.ext.pengutronix.de ([92.198.50.35]:51428 "EHLO metis.ext.pengutronix.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751099AbbEZIy3 (ORCPT ); Tue, 26 May 2015 04:54:29 -0400 Message-ID: <1432630462.3743.15.camel@pengutronix.de> Subject: Re: [PATCH v3 1/9] misc: sram: fix enabled clock leak on error path From: Philipp Zabel To: Vladimir Zapolskiy Cc: Heiko =?ISO-8859-1?Q?St=FCbner?= , Arnd Bergmann , Greg Kroah-Hartman , linux-kernel@vger.kernel.org Date: Tue, 26 May 2015 10:54:22 +0200 In-Reply-To: <556230A9.8030307@mentor.com> References: <1431976122-4228-1-git-send-email-vladimir_zapolskiy@mentor.com> <1431976122-4228-2-git-send-email-vladimir_zapolskiy@mentor.com> <1432032089.15181.22.camel@pengutronix.de> <555B368E.4080102@mentor.com> <1432121401.4466.60.camel@pengutronix.de> <556230A9.8030307@mentor.com> Content-Type: text/plain; charset="UTF-8" X-Mailer: Evolution 3.12.9-1+b1 Mime-Version: 1.0 Content-Transfer-Encoding: 7bit X-SA-Exim-Connect-IP: 2001:67c:670:100:96de:80ff:fec2:9969 X-SA-Exim-Mail-From: p.zabel@pengutronix.de X-SA-Exim-Scanned: No (on metis.ext.pengutronix.de); SAEximRunCond expanded to false X-PTX-Original-Recipient: linux-kernel@vger.kernel.org Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Hi Vladimir, Am Sonntag, den 24.05.2015, 23:12 +0300 schrieb Vladimir Zapolskiy: > Hi Philipp, > > On 20.05.2015 14:30, Philipp Zabel wrote: > > Hi Vladimir, > > > > Am Dienstag, den 19.05.2015, 16:11 +0300 schrieb Vladimir Zapolskiy: > >> Hi Philipp, > >> > >> On 19.05.2015 13:41, Philipp Zabel wrote: > >>> Am Montag, den 18.05.2015, 22:08 +0300 schrieb Vladimir Zapolskiy: > >>>> If devm_gen_pool_create() fails, the previously enabled sram->clk is > >>>> not disabled on probe() exit. > >>>> > >>>> Signed-off-by: Vladimir Zapolskiy > >>>> --- > >>>> drivers/misc/sram.c | 9 +++++---- > >>>> 1 file changed, 5 insertions(+), 4 deletions(-) > >>>> > >>>> diff --git a/drivers/misc/sram.c b/drivers/misc/sram.c > >>>> index eeaaf5f..b44a423 100644 > >>>> --- a/drivers/misc/sram.c > >>>> +++ b/drivers/misc/sram.c > >>>> @@ -90,16 +90,17 @@ static int sram_probe(struct platform_device *pdev) > >>>> if (!sram) > >>>> return -ENOMEM; > >>>> > >>>> + sram->pool = devm_gen_pool_create(&pdev->dev, > >>>> + ilog2(SRAM_GRANULARITY), -1); > >>>> + if (!sram->pool) > >>>> + return -ENOMEM; > >>>> + > >>>> sram->clk = devm_clk_get(&pdev->dev, NULL); > >>>> if (IS_ERR(sram->clk)) > >>>> sram->clk = NULL; > >>>> else > >>>> clk_prepare_enable(sram->clk); > >>> > >>> Here you move sram->clk around, and later in patch 7 it gets moved > >>> again. To me it looks like the two should be squashed together. > >> > >> I agree with you, instead of moving sram->pool up it is better to place > >> sram->clk right at the end of probe(), in other words this patch can be > >> safely merged with patch 7 and the series becomes a bit shorter. > >> > >> Thank you for the finding, I'm going to resend the change, please let me > >> know your opinion about "%pa" vs "0x%llx", if it is needed to be changed > >> or not. > > > > I'd prefer to use %pa for the phys_addr_t types. You could argue that > > %pa is inappropriate as those are addresses relative to the SRAM region, > > not physical addresses as seen by the CPU. But following that argument, > > using phys_addr_t in the first place would not be correct either. > > The driver utilizes genalloc gen_pool_add_virt() function, whose > corresponding argument is of phys_addr_t type (and it is correct in my > opinion), the interface to this function dictates the type of its > arguments on client's side. res->start is of type phys_addr_t (well, resource_size_t) already. block->start/size and cur_start/size are just offsets added to it. I wonder if it wouldn't be more appropriate to use resource_size_t for the sram_reserve .start field. > > Which leads me to question whether we will see larger than 4 GiB SRAM > > regions in the foreseeable future? > > The question is not only about SRAM region size, but mainly it is about > the base address of SRAM, and don't want to exclude a situation, when > some kind of SRAM device is found outside of u32 addressable memory > space. Actually I believe an arbitrary physical memory region may be > claimed as it were a "SRAM device" and the driver still works fine. See above, start addresses above 4 GiB should be supported even without extending the offsets to 64-bit. > If phys_addr_t arguments are accepted, then back to "%pa" vs "0x%llx" > question: > [...] > I see that the change preserves the functionality, so I'll change printk > format to "%pa", will resend the series tomorrow. Thanks. regards Philipp