Re: [PATCH v3 1/9] misc: sram: fix enabled clock leak on error path

[Vladimir Zapolskiy]

Hi Philipp,

On 26.05.2015 11:54, Philipp Zabel wrote:
> 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@xxxxxxxxxx>
>>>>>> ---
>>>>>>  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 agree.

> I wonder if it wouldn't be more appropriate to use resource_size_t for
> the sram_reserve .start field.

Assuming that the sram_reserve .start field represents only the difference
of two res->start and this difference fits into u32 storage, it should be
safe to keep it as is.

In my opinion integer overflow case should not be considered or handled
by the driver, so probably the best option would be just to drop
phys_addr_t commit, since it attempts to solve a nonexistent problem.

Please let me know your opinion, if it is fine with you, I'll remove
"use phys_addr_t instead of u32 for physical address" commit and resend
the series.

>>> 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.
> 

--
With best wishes,
Vladimir

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/