[PATCH 1/7] PL330: Add common core driver\

[jassi brar]

On Sun, May 2, 2010 at 6:01 AM, Dan Williams <dan.j.williams at intel.com> wrote:
> On Mon, Apr 26, 2010 at 12:09 AM, Ben Dooks <ben-linux at fluff.org> wrote:
>> On Mon, Apr 26, 2010 at 03:20:59PM +0900, jassi brar wrote:
>>> On Mon, Apr 26, 2010 at 1:54 PM, Ben Dooks <ben-linux at fluff.org> wrote:
>>> > On Wed, Apr 21, 2010 at 04:27:28PM +0900, jassisinghbrar at gmail.com wrote:
>>> >> From: Jassi Brar <jassi.brar at samsung.com>
>>> >> + * Mark a _pl330_req as free.
>>> >> + * We do it by writing DMAEND as the first instruction
>>> >> + * because no valid request is going to have DMAEND as
>>> >> + * its first instruction to execute.
>>> >> + */
>>> >> +#define MARK_FREE(req)       do { \
>>> >> +                             _emit_END(0, (req)->mc_cpu); \
>>> >> +                             (req)->mc_len = 0; \
>>> >> +                     } while (0)
>>> >
>>> > I think inline functions for these would be easier to read.
>>> you mean alignment that we see here? it's actually correctly
>>> aligned in an editor.
>>
>> firstly, on the grounds it avoids the whole \
>> and secondly on the grounds inline functions get checked by the compiler
>> whether they're used or not.
>
> ...thirdly any macro that evaluates its arguments more than once is
> potentially dangerous for cases like MARK_FREE(req++).
>
>>
>> I belive any macro over a couple of lines is probably doing too much
>> work.
Well, the three points are valid and we might have even more against
using macros _generally_. But just for the sake of clarification/learning..
a) We do have many multi-line macros in the kernel
b) The macro MARK_FREE does two independent things, where as a
    function usually does a sequence of logical steps. The macro
    MARK_FREE is clearly being used in our case.
c) Since the driver uses exactly two requests and we always explicitly
    free req[0] or req[1], I think we are in no danger of referencing invalid
    memory here.
Though having been asked by two maintainers, I will change it to
inline func though.
Thanks.