linux-kernel - Re: [MeeGo-Dev][PATCH] Topcliff: Update PCH

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
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Date:	Fri, 3 Sep 2010 19:01:52 +0900
From:	"Masayuki Ohtake" <masa-korg@....okisemi.com>
To:	"Joe Perches" <joe@...ches.com>
Cc:	"Jean Delvare \(PC drivers, core\)" <khali@...ux-fr.org>,
	"Ben Dooks \(embedded platforms\)" <ben-linux@...ff.org>,
	"Crane Cai" <crane.cai@....com>,
	"Samuel Ortiz" <sameo@...ux.intel.com>,
	"Linus Walleij" <linus.walleij@...ricsson.com>,
	"Ralf Baechle" <ralf@...ux-mips.org>,
	"srinidhi kasagar" <srinidhi.kasagar@...ricsson.com>,
	<linux-i2c@...r.kernel.org>, <linux-kernel@...r.kernel.org>,
	<yong.y.wang@...el.com>, <qi.wang@...el.com>,
	<andrew.chih.howe.khor@...el.com>, <arjan@...ux.intel.com>,
	"Tomoya MORINAGA" <morinaga526@....okisemi.com>,
	"Arnd Bergmann" <arnd@...db.de>
Subject: Re: [MeeGo-Dev][PATCH] Topcliff: Update PCH_I2C driver to 2.6.35

----- Original Message ----- 
From: "Joe Perches" <joe@...ches.com>
To: "Masayuki Ohtak" <masa-korg@....okisemi.com>
Cc: "Jean Delvare (PC drivers, core)" <khali@...ux-fr.org>; "Ben Dooks (embedded platforms)" <ben-linux@...ff.org>;
"Crane Cai" <crane.cai@....com>; "Samuel Ortiz" <sameo@...ux.intel.com>; "Linus Walleij" <linus.walleij@...ricsson.com>;
"Ralf Baechle" <ralf@...ux-mips.org>; "srinidhi kasagar" <srinidhi.kasagar@...ricsson.com>; <linux-i2c@...r.kernel.org>;
<linux-kernel@...r.kernel.org>; <yong.y.wang@...el.com>; <qi.wang@...el.com>; <andrew.chih.howe.khor@...el.com>;
<arjan@...ux.intel.com>; "Tomoya MORINAGA" <morinaga526@....okisemi.com>; "Arnd Bergmann" <arnd@...db.de>
Sent: Friday, September 03, 2010 5:10 PM
Subject: Re: [MeeGo-Dev][PATCH] Topcliff: Update PCH_I2C driver to 2.6.35


> On Fri, 2010-09-03 at 16:15 +0900, Masayuki Ohtak wrote:
> []
> > +#define pch_dbg(adap, fmt, arg...)  \
> > + dev_dbg(adap->pch_adapter.dev.parent, "%s :"fmt, __func__, ##arg)
> > +
> > +#define pch_err(adap, fmt, arg...)  \
> > + dev_err(adap->pch_adapter.dev.parent, "%s :"fmt, __func__, ##arg)
> > +
> > +#define pch_pci_err(pdev, fmt, arg...)  \
> > + dev_err(&pdev->dev, "%s :"fmt, __func__, ##arg)
> > +#define pch_pci_dbg(pdev, fmt, arg...)  \
> > + dev_dbg(&pdev->dev, "%s :"fmt, __func__, ##arg)
>
> OK, but it seems careless because the two types
> are not uniformly indented, there's a blank line
> between pch_dbg and pch_err, and the two pch_pci_<level>
> defines are in the reverse order without a blank line
> between them.
>
> I think it's better to use separate multiple strings
> that are concatentated by the preprocessor like:
> "%s :" fmt
> not
> "%s :"fmt
>
> Almost all code in kernel uses "%s: " to format __func__.
> Some use "%s(): ".  I think "%s :" is unique.
>
> The rest of the logging messages look good.
>
> Some other comments:
>
> > + if ((pch_wait_for_xfer_complete(adap) == 0) &&
> > + (pch_getack(adap) == 0)) {
>
> This would look better as:
>
> if ((pch_wait_for_xfer_complete(adap) == 0) &&
>     (pch_getack(adap) == 0)) {
>
> > + if ((pch_wait_for_xfer_complete(adap) == 0)
> > +     && (pch_getack(adap) == 0)) {
>
> Here too.
>
> > + for (i = 0; i < PCH_MAX_CHN; i++) {
> > + while ((adap_info->pch_data[i].pch_xfer_in_progress)) {
> > + /* Wait until all channel transfers are completed */
> > + msleep(1);
> > + }
> > + /* Disable the i2c interrupts */
> > + pch_disbl_int(&adap_info->pch_data[i]);
> > + }
>
> Would it be better to disable all possible interrupts first
> or do you need to disable them in order?

* Your proposal
If pch_disbl_int is executed firstly,
queued data  is destroyed.

 * Current spec
If checking status firstly,
all data can be sent.

Thus, I think current spec is better than yours.

>
> Something like:
>
> bool *disabled = kzalloc(PCH_MAX_CHN * sizeof(bool), GFP_KERNEL);
> /*
>  * or a static with a memset, or check something
>  * like pch_is_int_enabled(&adap_info->pch_data[i])
>  * then remove the else because the kzalloc couldn't fail.
>  */
> if (disabled) {
> bool alldone;
> do {
> alldone = true;
> for (i = 0; i < PCH_MAX_CHN; i++) {
> if (!adap_info->pch_data[i].pch_xfer_in_progress &&
>     !disabled[i])) {
> pch_disbl_int(&adap_info->pch_data[i]);
> disabled[i] = true;
> } else
> alldone = false;
> }
> if (!alldone) {
> /* Wait until all channel transfers are completed */
> msleep(1);
> }
> } while (!alldone);
> kfree(disabled);
>
> /* remove the else if there's a static etc */
>
> } else {
> for (i = 0; i < PCH_MAX_CHN; i++) {
> while ((adap_info->pch_data[i].pch_xfer_in_progress)) {
> /* Wait until all channel transfers are completed */
> msleep(1);
> }
> /* Disable the i2c interrupts */
> pch_disbl_int(&adap_info->pch_data[i]);
> }
> }
>
> cheers, Joe
>

I will resubmit modified patch soon.


Thanks, Ohtake(OKISemi)


--
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/