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
| ||
|
Date: Mon, 1 Feb 2016 10:51:24 +0800 From: Peter Hung <hpeter@...il.com> To: Andy Shevchenko <andriy.shevchenko@...ux.intel.com>, linus.walleij@...aro.org, gnurou@...il.com, gregkh@...uxfoundation.org, paul.gortmaker@...driver.com, lee.jones@...aro.org, jslaby@...e.com, peter_hong@...tek.com.tw Cc: heikki.krogerus@...ux.intel.com, peter@...leysoftware.com, soeren.grunewald@...y.de, udknight@...il.com, adam.lee@...onical.com, arnd@...db.de, manabian@...il.com, scottwood@...escale.com, yamada.masahiro@...ionext.com, paul.burton@...tec.com, mans@...sr.com, matthias.bgg@...il.com, ralf@...ux-mips.org, linux-kernel@...r.kernel.org, linux-gpio@...r.kernel.org, linux-serial@...r.kernel.org, tom_tsai@...tek.com.tw, Peter Hung <hpeter+linux_kernel@...il.com> Subject: Re: [PATCH V2 1/4] mfd: f81504-core: Add Fintek F81504/508/512 PCIE-to-UART/GPIO core support Hi Andy, Andy Shevchenko 於 2016/1/29 下午 09:41 寫道: > On Fri, 2016-01-29 at 13:50 +0800, Peter Hung wrote: >> Andy Shevchenko 於 2016/1/28 下午 07:55 寫道: >>> I would suggest to rearrange definition block here (and in the rest >>> of >>> the functions in entire series) to somehow follow the following >>> pattern >>> >>> 1) assignments go first, especially container_of() based ones; >>> 2) group variables by meaning and put longer lines upper; >>> 3) return value variable, if exists, goes last. >>> >>> Besides that choose types carefully. If there is known limit for >>> counters, no need to define wider type than needed. Use proper >>> types >>> for corresponding values. >> >> I'll try to rearrange the definition blocks. > > >> For the counter issue, some senior recommend me to change count from >> int >> to size_t for performance. > > Wow, how come? On which arch? > > Also mark this as (1) to refer below. > >> In this case, should I use u8 to replace >> i & j ? It should be less than 12 & 6. > > At least you tell compiler that it may use any suitable type. In any > case the last decision is by compiler if you don't do any tricks. > > So, I suggest to use non-fixed width type like (unsigned) int and leave > everything else on compiler. Sorry for my misunderstanding, not for performance. The senior just recommend me to replace all size/count variables to size_t. https://lkml.org/lkml/2016/1/3/193 size_t in x86 is unsigned int, x86_64 is unsigned long, It maybe good for prefetch or match register size I guess. I'll make the size_t of series patches to unsigned int. >>>> + pci_write_config_byte(dev, F81504_GPIO_IO_LSB_REG, >>>> gpio_addr >>>> & 0xff); >>>> + pci_write_config_byte(dev, F81504_GPIO_IO_MSB_REG, >>>> (gpio_addr >> 8) & >>>> + 0xff); >>> >>> Could it be written at once as a word? >> >> I tested with writing a word to "F81504_GPIO_IO_LSB_REG", but >> it'll failed, so I'll remain it. > > Please, add a comment line above. ok >> >>>> + if (priv) { >>>> + /* Reinit from resume(), read the previous value >>>> from priv */ >>>> >>> >>>> + gpio_en = priv->gpio_en; >>> >>> I would suggest to move this line down to follow same pattern in >>> else >>> branch. > > I'm talking here to make simple rearrangement like > > if () { > … > gpio_en = … > } else { > … > gpio_en = … > } > > Thus the gpio_en assignment goes last in both branches. ok >>>> + >>>> + pci_write_config_byte(dev, config_base + 0x06, >>>> dev- >>>>> irq); >>>> + >>>> + /* >>>> + * Force init to RS232 / Share Mode, recovery >>>> previous mode >>>> + * will done in F81504 8250 platform driver >>>> resume() >>> >>> Period at the end of sentences in multi-line comments. >> >> Sorry, what this mean for ? >> I cant use multi-line comments in the end ?? > > Sentences are started with capital letters and end with period '.' > character, like this one. ok. >> + if (status) { >>>> + dev_warn(&dev->dev, "%s: add device >>>> failed: >>>> %d\n", >>>> + __func__, status); >>> >>> Indent _ under &. >> >> Some senior recommend me to do at least 2-tabs to do indent when >> code cross multi-line. So I'll use to 2-tabs from "dev" to do indent. > >> How should I do with indent ?? It seems no consensus, but Lindent >> will do indent like your comments. > > I would suggest to use what is used in most recent drivers and modules. > I don't remember that 2 tabs fact is somehow reflected in CodingStyle. > > Maybe your seniour was talkin about multi-line function definition? ok, I'll indent multi-line statement as your comment and multi-line function with 2 tabs. >> + f81504_gpio_cell.pdata_size = sizeof(i); >>> >>> Static. The problem here is badly chosen type of i. Like I >>> mentioned at >>> the top of review… >> >> I'll try to rewrite it with pass a structure. It seems more make >> sense. > > That's correct on one side, on the other I'm not sure you got my > comment. size_t is arch-dependent type, sizeof() is not the same > everywhere. I'll change it to pass unsigned int. >> + pci_read_config_byte(dev, F81504_GPIO_IO_MSB_REG, &tmp); >>>> + priv->gpio_ioaddr = tmp << 8; >>>> + pci_read_config_byte(dev, F81504_GPIO_IO_LSB_REG, &tmp); >>>> + priv->gpio_ioaddr |= tmp; >>> >>> One read as word? >> >> It can't read as word. The "F81504_GPIO_IO_LSB_REG" is 0xf1, It >> seems can't be read word/dword from a odd address. >> >> I'll remain it. > > Put comment about that. ok >>> So, if you have default y for this and 8250_pci, which one will be >>> loaded? >> >> I had tested on x86, It'll handle by 8250_pci.c when this & >> 8250_pci.c >> all built-in mode. > > Yeah. here is the problem. When you introduce that you have to be sure > that no-one will try to build kernel with both included right now. Paul had recommend me to reduce the impact of code change. I'll remove all F81504/508/512 code in 8250_pci.c until the series patches had applied. The device will handle with 8250_pci.c before applide patch no4, and handle with f81504_code.c when applied patch no4. This maybe reduce the bisect misleading. >> >> BTW, due to the series patches across 3 subsystems MFD/GPIO/8250. >> I make the series patches under MFD subsystem, but the buildbot >> report git repository conflict with GPIO & 8250 (patch 2 & 3). >> >> Should I split the series patches into 3 independent patch and >> based on their maintainer git repository? > > It should go via one subsystem, otherwise user may end up with non- > working interim kernel version, i.e. when bisecting. > > In case if it based on some stuff which is not in upstream yet, you > have to describe this carefully to maintainers that they may create > immutable branches and cross-apply the stuff. But I suppose it's not > your case and your series is quite straightforward. > I'll still try the series patch based on MFD subsystem. Thanks for your advices. -- With Best Regards, Peter Hung
Powered by blists - more mailing lists