linux-kernel - Re: [PATCH] ipwireless: driver for 3G PC Card

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:	Thu, 31 Jan 2008 00:15:21 +0100
From:	Pavel Machek <pavel@....cz>
To:	"Stephen Blackheath [to Foxconn]" <stephen@...metric.co.nz>
Cc:	David Sterba <dsterba@...e.cz>, torvalds@...ux-foundation.org,
	linux-kernel@...r.kernel.org, jkosina@...e.cz, benm@...metric.co.nz
Subject: Re: [PATCH] ipwireless: driver for 3G PC Card

On Thu 2008-01-31 10:29:30, Stephen Blackheath [to Foxconn] wrote:
> Pavel & all,
> 
> 
> Pavel Machek wrote:
> 
> >> +/* I/O ports and bit definitions for version 2 of the hardware */
> >> +
> >> +struct MEMCCR {
> >> +	unsigned short PCCOR;		/* Configuration Option Register */
> >> +	unsigned short PCCSR;		/* Configuration and Status Register */
> >> +	unsigned short PCPRR;		/* Pin Replacemant Register */
> >> +	unsigned short PCSCR;		/* Socket and Copy Register */
> >> +	unsigned short PCESR;		/* Extendend Status Register */
> >> +	unsigned short PCIOB;		/* I/O Base Register */
> >> +};

> > Could we get better names? PCIOB is cryptic, pci_io_base is pretty
> > good.
> >
> >   
> We should keep these names because they are part of the interface
> between host and card defined by the manufacturer.

No. Use sensible names, and put manufacturer-defined 5-letter crap in
the comments. Heck, notice that they just took first letter of each
word of good name....

> > Is some locking needed around *hw?
> >   
> I don't think so, but I'm happy to be corrected.

You have a structure, and are accessing its fields from interrupts. I
assume you access the fields outside interrupt, too? As the fields are
not of atomic_t, I believe you need locking.

(Oh, and I should have said that earlier: Thanks for the driver and
congratulations for getting it this far).
									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
--
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/