Message-ID: <4F80DA15.1090909@tilera.com>
Date:	Sat, 7 Apr 2012 20:21:41 -0400
From:	Chris Metcalf <cmetcalf@...era.com>
To:	Jesper Juhl <jj@...osbits.net>
CC:	<linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 1/3] arch/tile: provide kernel support for the tilegx
 TRIO shim

On 4/7/2012 7:39 PM, Jesper Juhl wrote:
> On Sat, 7 Apr 2012, Chris Metcalf wrote:
>> Provide kernel support for the tilegx "Transaction I/O" (TRIO) on-chip
>> hardware.  This hardware implements the PCIe interface for tilegx;
>> the driver changes to use TRIO for PCIe are in a subsequent commit.
>>
>> The change is layered on top of the tilegx GXIO IORPC subsystem.
>>
>> Signed-off-by: Chris Metcalf <cmetcalf@...era.com>
>> [...]
>> +/* This file is machine-generated; DO NOT EDIT! */
>> +#include "gxio/iorpc_trio.h"
>> +
>> +typedef struct {
>> +     unsigned int count;
>> +     unsigned int first;
>> +     unsigned int flags;
>> +} alloc_asids_param_t;
>> +
>> +int gxio_trio_alloc_asids(gxio_trio_context_t * context, unsigned int count,
>> +                       unsigned int first, unsigned int flags)
>> +{
>> +     uint64_t __offset;
>> +     int __result;
>> +     alloc_asids_param_t temp;
>> +     alloc_asids_param_t *params = &temp;
>> +     size_t __size = sizeof(*params);
>> +
>> +     params->count = count;
>> +     params->first = first;
>> +     params->flags = flags;
>> +
>> +     __offset = GXIO_TRIO_OP_ALLOC_ASIDS;
>> +     __result =
>> +         hv_dev_pwrite(context->fd, 0, (HV_VirtAddr) params, __size,
>> +                       __offset);
>> +     return __result;
>> +}
> Having the "__offset" variable seems redundant here. It is declared, then
> later initialized to GXIO_TRIO_OP_ALLOC_ASIDS and then later used just
> once.
> I'd say either initialize it to GXIO_TRIO_OP_ALLOC_ASIDS when it's
> declared (and make that variable const) or just get rid of it entirely and
> just use GXIO_TRIO_OP_ALLOC_ASIDS directly in the one place the variable
> is used.
> This goes for other functions as well, using a __offset variable in a
> similar way - not going to explicitly point them all out.

Yes, there's certainly a tension here: as the comment at the top points
out, this file is machine-generated.  The compiled object code will clearly
be just as good whether __offset is done in a terser and more normal style,
or if it is done the way the generating code does it now.  This code also
comes from "upstream" internally at Tilera in the sense that Linux just
uses files that are generated by code in the hypervisor build process.  But
I can certainly look into whether the generated files can be easily made
terser.

> What's the point of the "__result" variable? why not just get rid of it
> and just "return hv_dev_pwrite(context->fd, 0, (HV_VirtAddr) params,
> __size, __offset);" ??
> Goes for other functions as well..

Yes, same issue here too, and I'll look into this as well.

>> [...]
>> +int gxio_trio_config_legacy_intr(gxio_trio_context_t * context, int inter_x,
>> +                              int inter_y, int inter_ipi, int inter_event,
>> +                              unsigned int mac, unsigned int intx)
>> +{
>> +     uint64_t __offset;
>> +     int __result;
>> +     config_legacy_intr_param_t temp;
>> +     config_legacy_intr_param_t *params = &temp;
>> +     size_t __size = sizeof(*params);
>> +
> const size_t __size = sizeof(*params);  ??? or maybe just get rid of the
> variable since it is only used once.?

I don't see "const" used that much in the kernel (though there does seem to
be a slight swing towards more of it despite the threat of "const
poisoning" issues).  Getting rid of the variable might or might not be
easy, again, depending on the code generator issues.  I'll check.

>> [...]
>> +#define GXIO_TRIO_OP_GET_PORT_PROPERTY IORPC_OPCODE(IORPC_FORMAT_NONE_NOUSER, 0x1419)
>> +#define GXIO_TRIO_OP_CONFIG_LEGACY_INTR IORPC_OPCODE(IORPC_FORMAT_KERNEL_INTERRUPT, 0x141a)
>> +#define GXIO_TRIO_OP_CONFIG_MSI_INTR   IORPC_OPCODE(IORPC_FORMAT_KERNEL_INTERRUPT, 0x141b)
>> +#define GXIO_TRIO_OP_CONFIG_CHAR_INTR  IORPC_OPCODE(IORPC_FORMAT_KERNEL_INTERRUPT, 0x141c)
>> +#define GXIO_TRIO_OP_SET_MPS_MRS       IORPC_OPCODE(IORPC_FORMAT_NONE_NOUSER, 0x141d)
>> +#define GXIO_TRIO_OP_FORCE_LINK_UP     IORPC_OPCODE(IORPC_FORMAT_NONE_NOUSER, 0x141e)
>> +#define GXIO_TRIO_OP_GET_MMIO_BASE     IORPC_OPCODE(IORPC_FORMAT_NONE_NOUSER, 0x8000)
>> +#define GXIO_TRIO_OP_CHECK_MMIO_OFFSET IORPC_OPCODE(IORPC_FORMAT_NONE_NOUSER, 0x8001)
>> +
>
> There is some funky formatting going on here (or my email client is
> playing tricks on me) :

The code generator here is trying to align the #define values at column 40
and if the #define symbol is too long, it just uses one space and leaves it
at that.

>> [...]
>> +int gxio_trio_force_link_up(gxio_trio_context_t * context, unsigned int mac);
>> +
>> +int gxio_trio_get_mmio_base(gxio_trio_context_t * context, HV_PTE *base);
>> +
>> +int gxio_trio_check_mmio_offset(gxio_trio_context_t * context,
>> +                             unsigned long offset, unsigned long size);
>> +
> First of all, when you have "pointer_type * variable", please use the
> style "pointer_type *variable" - as per accepted kernel coding style.

Yes, this is a bug from my missing some typenames when using "indent" to
try to force the upstream files into something closer to standard Linux
format.  I can add some "-T" arguments to remove the extra whitespace.

> Also, I find the indentation to be inconsistent; varuing from function to
> function - is it just my email client or??  If not, please fix.

The upstream code style is closer to FSF, with the return typename on the
line before the function.  Using "indent -npsl" is theoretically supposed
to fix this to Linux style, but in fact it appears that if the function and
its arguments is longer than 80 columns, indent "helpfully" doesn't do
this.  I'd like to fix this in a mechanical way so I don't end up in merge
hell in a year or two, but failing that I'm planning to just leave this
code with some signatures in the wrong style; it seems the lesser of
several evils.

>> +/** TRIO requires that all mappings have 4kB aligned start addresses. */
>> +#define HV_TRIO_PAGE_SIZE (1ull << HV_TRIO_PAGE_SHIFT)
>> +
>> +
>> +
> 3 blank lines seems excessive - surely 2 is enough.

Fixed.

Thanks for your review - I appreciate it!

-- 
Chris Metcalf, Tilera Corp.
http://www.tilera.com

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

Powered by blists - more mailing lists