Messages in this thread | | | Date | Sat, 7 Apr 2012 20:21:41 -0400 | From | Chris Metcalf <> | 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@tilera.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
| |