linux-kernel - Re: [PATCH v2 2/2] Staging: comedi: use inl() and outl() helper functions

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] [thread-next>] [day] [month] [year] [list]

Date:	Mon, 3 Mar 2014 12:40:34 +0300
From:	Dan Carpenter <dan.carpenter@...cle.com>
To:	Chase Southwood <chase.southwood@...oo.com>
Cc:	gregkh@...uxfoundation.org, devel@...verdev.osuosl.org,
	linux-kernel@...r.kernel.org, abbotti@....co.uk
Subject: Re: [PATCH v2 2/2] Staging: comedi: use inl() and outl() helper
 functions

On Sun, Mar 02, 2014 at 08:52:33PM -0600, Chase Southwood wrote:
> Use the newly created helper functions to improve code readability and shorten
> several lines to under the character limit.
> 
> Cc: Dan Carpenter <dan.carpenter@...cle.com>
> Signed-off-by: Chase Southwood <chase.southwood@...oo.com>
> ---
> 
> I've reviewed this as best as I can, but I know it's a bear to review.
> If there is some logical way that you'd like me to split this up into separate
> patches to make it easier to review, please let me know.
> 
> Also, Dan, as best as I can possibly tell, i_APCI1564_Reset() is absolutely
> buggy.  When making up this patch, I made the changes that seem correct (and
> somewhat obvious) from looking at the other addi-data drivers, the other
> functions in this file, and the hardware specs that I could find, but
> unfortunately I have no way to test the code.

You need to put this into the commit message...  You can't change how
the code works without at least mentioning it in the commit.

It would be easier to review these patches if you introduced the helpers
first and switched everything to use them.  Then in 2/2 you changed the
defines to the combined versions.

> @@ -333,81 +313,61 @@ static int i_APCI1564_ConfigTimerCounterWatchdog(struct comedi_device *dev,
>  		devpriv->b_TimerSelectMode = ADDIDATA_WATCHDOG;
>  
>  		/* Disable the watchdog */
> -		outl(0x0,
> -			devpriv->i_IobaseAmcc + APCI1564_DIGITAL_OP_WATCHDOG +
> -			APCI1564_TCW_PROG);
> +		outl_amcc(devpriv, 0x0,
> +				  APCI1564_DIGITAL_OP_WATCHDOG + APCI1564_TCW_PROG);


This isn't indented correctly.  It should be:

		outl_amcc(devpriv, 0x0,
			  APCI1564_DIGITAL_OP_WATCHDOG + APCI1564_TCW_PROG);

[tab][tab][tab][space][space]APCI1564_DIGITAL_OP_WATCHDOG...

The same thing in a couple other places as well.

regards,
dan carpenter

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