linux-kernel - RE: [PATCH 00/83] staging: comedi: rtd520: initial cleanup

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:	Wed, 11 Jul 2012 11:27:46 -0500
From:	H Hartley Sweeten <hartleys@...ionengravers.com>
To:	Ian Abbott <abbotti@....co.uk>
CC:	Linux Kernel <linux-kernel@...r.kernel.org>,
	"devel@...verdev.osuosl.org" <devel@...verdev.osuosl.org>,
	Ian Abbott <ian.abbott@....co.uk>,
	"gregkh@...uxfoundation.org" <gregkh@...uxfoundation.org>
Subject: RE: [PATCH 00/83] staging: comedi: rtd520: initial cleanup

On Wednesday, July 11, 2012 3:20 AM, Ian Abbott wrote:
> On 2012-07-11 00:36, H Hartley Sweeten wrote:
>> The 'devpriv' macro usage in this driver is holding up other cleanup
>> of the comedi drivers.
>>
>> This patch series removes all the macros used to read/write the
>> hardware registers. All of them are simple wrappers around standard
>> {read,write}[rwl] calls with some of them caching the write value
>> in a shadow variable in the private data.
>>
>> Each of the macros is removed in a separate patch to ease reviewing.
>> They can all be squashed into one patch if desired.
>>
>> After all the macros are gone, the devpriv macro itself can be removed
>> along with the boardinfo macro.
>>
>> The "find pci device" code is also refactored to follow the style of
>> the other comedi pci drivers.
>>
>> The range tables are also cleaned up for aesthetic reasons. The large
>> whitespace causes some pretty nasty line breaks in order to keep the
>> lines < 80 characters.
>
> My main concern with this series of patches is they make it harder to 
> determine what the register accesses actually do, mainly due to the 
> removal of useful comments.  For example:
>
> -	RtdPacerStart(dev);	/* Start PACER */
> +	readl(devpriv->las0 + LAS0_PACER);
>
> And:
>
> -	RtdPacerStop(dev);	/* Stop PACER */
> +	writel(0, devpriv->las0 + LAS0_PACER);
>
> The comments in the original code were quite superfluous, but they would 
> be useful for the replacement code.  In the above cases you could figure 
> it out after looking at the rtd520.h header file where its reasonably 
> clear that reading the LAS0_PACER register starts the pacer and writing 
> the register (presumably with any value) stops the pacer.  But it's less 
> clear in cases such as:
>
> -		RtdAdcConversionSource(dev, 2);	/* BURST triggers ADC */
> +		writel(2, devpriv->las0 + LAS0_ADC_CONVERSION);
>
> Since the values for the LAS0_ADC_CONVERSION register are no longer 
> documented anywhere.
> 

Sorry about that. I meant to leave some of the comments as appropriate
but overlooked them during the cleanup.

Some of the magic write values are actually documented in the header with
#define'd values. But, the header is a bit of a mess and it's hard to determine
which bit values go with which register.

The header really needs to be cleaned up and then merged directly into the
source. It's only used by this driver so the defines shouldn't be exposed for
external use. I located the manual for the device and plan on doing that cleanup
eventually.

Regardless, thanks for adding back the comments. I'll look them over and
give you a Reviewed-by shortly.

Thanks,
Hartley