Messages in this thread | | | Subject | Re: [PATCH v3 1/9] misc: sram: fix enabled clock leak on error path | From | Philipp Zabel <> | Date | Tue, 26 May 2015 10:54:22 +0200 |
| |
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 <vladimir_zapolskiy@mentor.com> > >>>> --- > >>>> 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
| |