Message-ID: <20180114201008.GM13338@ZenIV.linux.org.uk>
Date:   Sun, 14 Jan 2018 20:10:08 +0000
From:   Al Viro <viro@...IV.linux.org.uk>
To:     Naveen Panwar <naveen.panwar27@...il.com>
Cc:     gregkh@...uxfoundation.org, devel@...verdev.osuosl.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH] Staging: netlogic: platform_net: Fixed '(' at the EOL

On Sun, Jan 14, 2018 at 11:47:11PM +0530, Naveen Panwar wrote:
> Removed '(' from the end of line, coding style issue.

The one and only reason for warnings is that they point to
places more likely to be dodgy.  There is no inherent value
in having e.g. checkpatch.pl STFU, all wanking about uniformity
of style nonwithstanding.

If you end up figuring out how to manipulate some code so that
some warning would go away, something is wrong.  Either the
warning is bogus (which it might very well be - the point of
a warning is "might be worth looking here" and considering
the... level of sophistication of some of those, you can't
expect to get no false alarms) or there *is* something
dodgy in the vicinity.  Dodgy beyond the "this heuristic
triggers", that is.

The first thing to do when dealing with any warning is to
look around and see if it has caught something interesting.
In this case the warning points to excessively long expressions.
With your patch they *still* are just as long, but split in
more unnatural way, making checkpatch.pl miss them.

All of them appear to have the same form - CPHYSADDR applied to
nlm_mmio_base result.  Moreover, looking around that file
shows that all uses of CPHYSADDR and nlm_mmio_base in it
are parts of such expressions.

Most of those expressions are too long and hard to read, so
cleaning them up would probably be a good idea.  And seeing
that composition of these functions keeps occuring in that
sucker, making that composition an inlined helper appears
to be called for.

So
static inline <some type> something(<some type> offset)
{
	return CPHYSADDR(nlm_mmio_base(offset));
}

and turn those into

	gmac4_addr = ioremap(something(NETLOGIC_IO_GMAC_4_OFFSET), 0xfff);

	void __iomem *gmac0_addr = ioremap(something(NETLOGIC_IO_GMAC_0_OFFSET),
					   0xfff);
 
	gpio_addr = ioremap(something(NETLOGIC_IO_GPIO_OFFSET), 0xfff);

	ndata0.mii_addr = ioremap(something(NETLOGIC_IO_GMAC_0_OFFSET), 0xfff);

and turn
        res->start = CPHYSADDR(nlm_mmio_base(offset));
in xlr_resource_init() into
        res->start = something(offset);
as well.  What should the types be?  Result is either fed to ioremap(), or
stored in struct resource 'start' field.  The latter is resource_size_t;
the former varies among the architectures.  Some take resource_size_t,
some - phys_addr_t, some - unsigned long.  On mips (that driver appears to
be mips-only) ioremap takes phys_addr_t; the difference is cosmetical,
anyway, since resource_size_t is typedefed to phys_addr_t on all architectures.

So let it return phys_addr_t.  As for the argument type, looks like it's
an explicit constant or, in case of xlr_resource_init(), an int.  Grepping
shows that all constants involved do fit into int, so we arrive at

static inline phys_addr_t something(int offset)
{
        return CPHYSADDR(nlm_mmio_base(offset));
}

probably best placed just before the first use.  OTOH, nlm_mmio_base() is
declared as taking uint32_t, so that might be a better fit for argument.
All constants fed there are between 0 and 2^31, so the only question is
what values does xlr_resource_init() get passed in 'offset' argument.
Callers are
                xlr_resource_init(&xlr_net1_res[mac * 2],
                                  xlr_gmac_offsets[mac + 4],
                                  xlr_gmac_irqs[mac + 4]);
                xlr_resource_init(&xlr_net0_res[0], xlr_gmac_offsets[0],
                                  xlr_gmac_irqs[0]);
                        xlr_resource_init(&xlr_net0_res[mac * 2],
                                          xlr_gmac_offsets[mac],  
                                        xlr_gmac_irqs[mac]);
                xlr_resource_init(&xlr_net0_res[mac * 2], xlr_gmac_offsets[mac],
                                  xlr_gmac_irqs[mac]);

so in all cases we have xlr_gmac_offsets[<something>] passed.  What's
going on with that array?  Initialized as
static u32 xlr_gmac_offsets[] = {
        NETLOGIC_IO_GMAC_0_OFFSET, NETLOGIC_IO_GMAC_1_OFFSET,
        NETLOGIC_IO_GMAC_2_OFFSET, NETLOGIC_IO_GMAC_3_OFFSET,
        NETLOGIC_IO_GMAC_4_OFFSET, NETLOGIC_IO_GMAC_5_OFFSET,
        NETLOGIC_IO_GMAC_6_OFFSET, NETLOGIC_IO_GMAC_7_OFFSET
};
and never modified, apparently.  OK, that pretty much settles it -
xlr_resource_init() should be getting u32 instead of int.  It's harmless
(the constants here are in the same range), but the things will be easier
for reader that way:

static inline phys_addr_t something(u32 offset)
{
        return CPHYSADDR(nlm_mmio_base(offset));
}

static void xlr_resource_init(struct resource *res, u32 offset, int irq)
{
        res->name = "gmac";

        res->start = something(offset);

While we are at it, 'irq' argument appears to be always taken from
xlr_gmac_irqs[], which is also u32.  So I'd probably do 
static void xlr_resource_init(struct resource *res, u32 offset, u32 irq)
instead.

Now, what do we call that helper?  Definition of CPHYSADDR is
/*
 * Returns the physical address of a CKSEGx / XKPHYS address
 */
#define CPHYSADDR(a)            ((_ACAST32_(a)) & 0x1fffffff)
so we are getting physical addresses here.  Getting too creative is
probably not worth it, so let's just call it 'physaddr' and call it
quits.  Grepping doesn't catch any existing functions with such
name, so we should be OK.  So,

static inline phys_addr_t physaddr(u32 offset)
{
        return CPHYSADDR(nlm_mmio_base(offset));
}

static void xlr_resource_init(struct resource *res, u32 offset, u32 irq)
{
        res->name = "gmac";

        res->start = physaddr(offset);
...
and turn the lines caught by that warning in the first place into
	gmac4_addr = ioremap(physaddr(NETLOGIC_IO_GMAC_4_OFFSET), 0xfff);
etc.  All those calls do look sane - there might have been brainos hidden
by hard-to-read expressions, but nothing earth-shattering has turned up.
Which means that we'd probably squeezed all we were going to get out of
that warning.  Now all we need is a sane commit message.  We'd introduced
a helper for physical address calculation that used to be open-coded in
a bunch of places in netlogic/platform_net.c, and adjusted the types of
arguments of xlr_resource_init() there.  So I'd go with something along
the lines of

[netlogic] introduce a helper for physical address calculation

Several places in drivers/staging/netlogic/platform_net.c open-code the
identical logics for calculating physical addresses of memory-mapped
objects on the device.  Put that into an inlined helper and use it.
Simplifies a bunch of expressions in there...

Adjust the types of arguments of xlr_resource_init() - 'offset' and 'irq'
are actually unsigned.  The values passed to those fit into 0..2G range,
so the behaviour is unchanged, but it's more consistent that way.

... and if you feel like crediting checkpatch.pl, possibly add something
about that thing having pointed to excessively long expressions in the
first place.

Powered by blists - more mailing lists