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: <1287090073.4301.20.camel@maxim-laptop>
Date:	Thu, 14 Oct 2010 23:01:12 +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 01:00 -0700, Alex Dubov wrote:
> > From: Maxim Levitsky <maximlevitsky@...il.com>
> > 
> > and patch #6 contines adds even more cleanups, and makes
> > whole
> > memctick subsystem look nice and clean.
> > Many non obivous functions were removen.
> 
> Replacing functional state handlers with nasty 3-page long switches on
> undiscriminated integer state variables does not count either "cleaner"
> nor "more obvious" in my notebook.
Yes it is.
In the original state machines, the switch value was a TPC that was
already sent, and therefore it usually wasn't related to code inside
switch block.
In addition to that if one tries to send same TPC and treat it
differently he is for a big surprise.
With my scheme the states are clearly marked.

'nasty 3-page long switches' - what are you taking about?
h_mspro_block_transfer_data - 89 lines before
h_mspro_block_transfer_data - 129 lines after

Sure the new handler is a bit longer, but I moved there a chunk of
request processing code, so that mspro_block_issue_req and
mspro_block_complete_req
do now one thing only and used only for block requests.

> 
> This is a matter of personal preference, I suppose.
> 
> Yet, at the very least, you should define an appropriate enum and give
> your states readable names.
I didn't use a enum on purpose.
This allows me to go to previous/next state by doing card->state++ or
card->state--;
enum will hide the numeric values and thus make this dangerous.
I explicitly jump to a state very rarely, and it is obvious from code
when I do so.
Also states are private to each state machine, so it is easy to locate a
state within it.

Since I introduced the helpers to read/write the regs which make code
simpler and safer (less assumptions about what current window is)
and function to read the INT register which removes a chunk of redundant
code (and a goto), and most importantly guard against infinite INT poll
(which can happen with old code), I can't really use a TPC switch
anyway.


And what I had to go through to understand the
mspro_block_read_attributes....

Best regards,
	Maxim Levitsky

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