linux-kernel - Re: [PATCH] DM9000: Reimplement 2-resource device specification (fwd)

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]

Message-ID: <20080425090403.W61380@saturn.araneidae.co.uk>
Date:	Fri, 25 Apr 2008 09:39:26 +0000 (GMT)
From:	Michael Abbott <michael@...neidae.co.uk>
To:	Ben Dooks <ben-linux@...ff.org>
cc:	linux-net@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] DM9000: Reimplement 2-resource device specification
 (fwd)

On Thu, 24 Apr 2008, Ben Dooks wrote:
> On Thu, Apr 24, 2008 at 12:57:08PM +0000, Michael Abbott wrote:
>> From: Michael Abbott <michael.abbott@...mond.ac.uk>
>>
>> [PATCH] DM9000: Reimplement 2-resource device specification
>>
>> In commit 1a5f1c4ff80f522555d78d4dd0109f18395c6d83 a change was made to
>> the handling of the resources passed the DM9000 device.  Unfortunately,
>> if only two resources are passed (the read and write register in the same
>> area) the pointer db->irq_res ends up being used without being initialised.
>>
>> This patch tidies this up a little.  Only one architecture class,
>> blackfin, uses two resource specification, so it may be better to enforce
>> three resource specification as before by simply removing the 0 and 2
>> resource branches from dm9000_probe.
>
> I think then we should probably make the driver simpler and remove the
> special case for the two resources... any small saving of using just
> two resources is probably cancelled out by the extra code needed.

I think this is a good idea.  The only qualms I had about this were: 1/ 
this involves changes to a device which I'm completely unable to test (but 
I think we agree it's already broken without this patch);  2/ I don't know 
how much extra resources are involved in the extra memory region request 
and io remap.

>> +#define res_size(_r) (((_r)->end - (_r)->start) + 1)
> I'd like to see something like this as an inline function in the
> relevant header, and some patches to get other users to find it.

This macro was in the original commit which introduced dm9000 support,
a1365275e745bb0a173c918a52bcdfa6ce122f7e (I just had to move it up one 
function).

I guess the natural home for something like this would be linux/ioport.h.
(I do wish there were more detailed comments in the header files on *how* 
the functions defined there behave and are intended to be used!)

>> -
>>  		if (pdata->outblk != NULL)
>>  			db->outblk = pdata->outblk;
>> -
>>  		if (pdata->dumpblk != NULL)
>>  			db->dumpblk = pdata->dumpblk;
> I'd rather not see spacing changes done, especially as I don't like
> to see lots of code cramped together.

Sorry, had a feeling that didn't really belong in the patch.  For my own 
code I prefer whitespace to serve a stronger role (breaking up logical 
groups; in my mind the assignments above go together), but I'll refrain 
from layout hacks to code I'm not rewriting!

> -- 
> Ben (ben@...ff.org, http://www.fluff.org/)
>  'a smiley only costs 4 bytes'
4 bytes?  In what encoding? ;)
--
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/