linux-kernel - Re: Results of my work on memorystick subsystem

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]

Message-ID: <1287149672.3473.28.camel@maxim-laptop>
Date:	Fri, 15 Oct 2010 15:34:32 +0200
From:	Maxim Levitsky <maximlevitsky@...il.com>
To:	Alex Dubov <oakad@...oo.com>
Cc:	Andrew Morton <akpm@...ux-foundation.org>,
	LKML <linux-kernel@...r.kernel.org>
Subject: Re: Results of my work on memorystick subsystem

On Thu, 2010-10-14 at 23:45 -0700, Alex Dubov wrote:
> > 
> > 
> > And what I had to go through to understand the
> > mspro_block_read_attributes....
> > 
> 
> Ellipsis to the rescue! :-)
> This is just one straightforward function which parses the header block
> in an originally intended fashion. You can simplify it by making it "less
> correct", windows driver style, but otherwise I fail to see what could be
> so difficult about it.

Alex, it is now straight forward.

Lets see what gems I found in old version:

* It does obscure math with sizes, offsets.
multiplies, divides offsets, sizes, uses data stored in 'param' register
to 'recover' the data.

* It overloads the 'rc' variable to store attribute size.
That is outright nasty as you would say.

* It has not a single comment, especially about its caching.

Alex, don't get me wrong.

Let me explain why did I refactor the mspro_blk.c

I have created the ms_block.c, and I just had to create a set of common
helpers to reduce code duplication because I hate it.
It works well, very well now.

Now I had put quite a lot of changes to common code, including few state
variables in the 'card' structure.

I really had to do that. I added common INT polling feature to reduce
code duplication. I also added a timeout for INT polling. It saved me
from restart for a lot of times (while I debugged the code).
And I really can't put a piece of code that can enter an endless loop
if conditions are right.

If I were to inline the INT read function, it would create a huge code
duplication.

I added a register read/write functions.
These made it possible to be free of constant fear of sending TPC of
wrong size because register window isn't updated.
It allowed me to read/write just the right registers everywhere without
additional effor, and most importantly reduce the >8 bytes TPC which
improve performance significantly on Jmicron.

It also helped me a lot to debug and workaround that damn Jmicon reader.

The way I handle card states not only allowed me to have several states
that handle a same TPC, but allowed me to have states that don't handle
any.

Now on the other hand, the mspro_blk.c did't use these changes, and
therefore was in constant danger of beeing broken.
I had to think carefully how to add changes, while keeping the old way
working.

So I bit the bullet, and made the mspro_blk.c use new support code.
Among the way I tried to make it simpler.
I understand that you might not like that I 'made your code better',
That is very subjective, and besides,
I'll say maybe I didn't. Yet at least it is consistent with ms_block.c

Alex, maybe I shouldn’t say that new code is nice and clean.
Don't hate me because of that.

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