linux-kernel - Re: [PATCH v4 12/24] PM / devfreq: tegra30: Inline all one-line functions

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: <12ea5667-bd93-1103-b20f-95b012047d1d@gmail.com>
Date:   Fri, 19 Jul 2019 19:52:09 +0300
From:   Dmitry Osipenko <digetx@...il.com>
To:     Chanwoo Choi <cw00.choi@...sung.com>
Cc:     Thierry Reding <thierry.reding@...il.com>,
        MyungJoo Ham <myungjoo.ham@...sung.com>,
        Kyungmin Park <kyungmin.park@...sung.com>,
        Jonathan Hunter <jonathanh@...dia.com>,
        Tomeu Vizoso <tomeu.vizoso@...labora.com>,
        linux-pm@...r.kernel.org, linux-tegra@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH v4 12/24] PM / devfreq: tegra30: Inline all one-line
 functions

19.07.2019 9:01, Chanwoo Choi пишет:
> On 19. 7. 19. 오전 11:14, Dmitry Osipenko wrote:
>> В Fri, 19 Jul 2019 10:27:16 +0900
>> Chanwoo Choi <cw00.choi@...sung.com> пишет:
>>
>>> On 19. 7. 19. 오전 10:24, Chanwoo Choi wrote:
>>>> On 19. 7. 19. 오전 10:22, Dmitry Osipenko wrote:  
>>>>> В Thu, 18 Jul 2019 18:09:05 +0900
>>>>> Chanwoo Choi <cw00.choi@...sung.com> пишет:
>>>>>  
>>>>>> On 19. 7. 16. 오후 10:35, Dmitry Osipenko wrote:  
>>>>>>> 16.07.2019 15:26, Chanwoo Choi пишет:    
>>>>>>>> Hi Dmitry,
>>>>>>>>
>>>>>>>> I'm not sure that it is necessary.
>>>>>>>> As I knew, usally, the 'inline' is used on header file
>>>>>>>> to define the empty functions.
>>>>>>>>
>>>>>>>> Do we have to change it with 'inline' keyword?    
>>>>>>>
>>>>>>> The 'inline' attribute tells compiler that instead of jumping
>>>>>>> into the function, it should take the function's code and
>>>>>>> replace the function's invocation with that code. This is done
>>>>>>> in order to help compiler optimize code properly, please see
>>>>>>> [1]. There is absolutely no need to create a function call into
>>>>>>> a function that consists of a single instruction.
>>>>>>>
>>>>>>> [1] https://gcc.gnu.org/onlinedocs/gcc-9.1.0/gcc/Inline.html
>>>>>>>     
>>>>>>
>>>>>> If you want to add 'inline' keyword, I recommend that 
>>>>>> you better to remove the modified function in this patch
>>>>>> and then just call the 'write_relaxed or read_relaxed' function
>>>>>> directly. It is same result when using inline keyword.  
>>>>>
>>>>> That could be done, but it makes code less readable.
>>>>>
>>>>> See the difference:
>>>>>
>>>>> device_writel(dev, ACTMON_INTR_STATUS_CLEAR,
>>>>> ACTMON_DEV_INTR_STATUS);
>>>>>
>>>>> writel_relaxed(ACTMON_INTR_STATUS_CLEAR,
>>>>> 	       dev->regs + ACTMON_DEV_INTR_STATUS);  
>>>>
>>>> No problem if you add the detailed comment and you want to use
>>>> the 'inline' keyword.  
>>>
>>> Basically, I think that 'inline' keyword is not necessary.
>>
>> Sure, but I'm finding that it's always nicer to explicitly inline a very
>> simple functions because compiler may not do it properly itself in some
>> cases.
>>
>>> But if you want to use 'inline' keyword, I recommend
>>> that call the 'write_relaxed or read_relaxed' function directly
>>> with detailed description. 
>>
>> Could you please reword this sentence? Not sure that I'm understanding
>> it correctly.
>>
> 
> If you want to used 'inline' keyword,
> Instead, I recommend that remove 'actmon_readl/writel' wrapper functions
> and then you calls 'write_relaxed or read_relaxed' function directly
> with detailed description.
> 

This is a step into a wrong direction. Look, there is no need for extra
comments and the code is clean with the variant I'm proposing, while you
are asking to make code less readable and then paper that over with
comments.

I'll probably just drop this, #11 and #17 for now. Since these patches
and not essential for the functionality of the driver and they are
raising more questions than should be. Maybe we could get back to them
at some point later.