linux-kernel - Re: [PATCH 2/2] memstick: add support for legacy memorysticks

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: <1348604673.12068.30.camel@maxim-laptop>
Date:	Tue, 25 Sep 2012 22:24:33 +0200
From:	Maxim Levitsky <maximlevitsky@...il.com>
To:	Tejun Heo <tj@...nel.org>
Cc:	Andrew Morton <akpm@...ux-foundation.org>,
	Alex Dubov <oakad@...oo.com>, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 2/2] memstick: add support for legacy memorysticks

On Tue, 2012-09-25 at 12:38 -0700, Tejun Heo wrote: 
> Hello, Maxim.
> 
> On Tue, Sep 25, 2012 at 09:26:13PM +0200, Maxim Levitsky wrote:
> > > Probably not the best idea to use a name this generic in driver code.
> > > linux/scatterlist.h likely might wanna use the name.
> >
> > Lets not go this route again. I already once submitted these, and had
> > a share of problems with merging these poor functions into the scatter
> > list.
> > scatter list users mostly dont need these as they just translate it into
> > hardware specific representation.
> > In my case, I don't and yet its easier that working with BIOs. 
> 
> Hmmm... then please at least add a prefix to the names.
Will do! 
> 
> > > Also, from what it does, it seems sg_copy() is a bit of misnomer.
> > > Rename it to sg_remap_with_offset() or something and move it to
> > > lib/scatterlist.c?
> >
> > Don't think so. This copies part of a scatter list into another
> > scatterlist.
> > I have to use is as memstick underlying drivers expect a single
> > scatterlist for each 512 bytes sector I read. Yes, it contains just one
> > entry, but still. I haven't designed the interface. 
> 
> It doesn't really matter if it's a function only used in the driver,
> but please don't use sg_copy() as its name.
Sure! 
> 
> > > Maybe we can make sg_copy_buffer() more generic so that it takes a
> > > callback and implement this on top of it?  Having sglist manipulation
> > > scattered isn't too attractive.
> >
> > Again this is very specific to my driver. Usually nobody pokes the
> > scatterlists. 
> 
> The problem is that there are talks of improving sglist handling (make
> it more generic, unify it with bvec and so on) and this sort of one
> off direct manipulations often become headaches afterwards, so if at
> all possible it's best to keep stuff centralized.
If the sglist gets improved, I will be glad to update my driver to use
the new generic code.

Compared to my xD driver, which just works directly with pointers to 512
byte buffers (with the price of not utilizing highmem), the sglist (and
bios are even worse, I recall), gave me lot of headaches :-(

> 
> > > Is it really necessary to implement explicit state machine?  Can't you
> > > just throw a work item at it and process it synchronously?  Explicit
> > > state machines can save some resources at the cost of a lot more
> > > complexity and generally making things a lot more fragile.  Is it
> > > really worth it here?
> >
> > It would be awesome to not use these ugly state machines, but I have to,
> > because this is required by underlying driver interface.
> > Its callback driven, that is underlying driver calls into this driver,
> > when it wants, and I supply it new command to execute.
> > Unfortunately with legacy memsticks, to read or write a sector, one
> > needs to send it many different commands. Thats why these state
> > machines. I at least made them more or less consistent.
> > Just take a look at mspro_blk.c....
> 
> I see.  Eeeek...

I tried once to improve it, dunno if I did or made it even worse, its a
long story.... Anyway it wasn't liked my its author.

Anyway, my goal now is to get this driver merged, as its the last piece
of the puzzle to make my card reader perfect. If can now read/write
everything, and its the only such reader supported by Linux.

To be honest the Jmicron reader I have doesn't have a driver for xD
part, and I need to stop beeing lazy and write a driver, I do have even
docs for it.

Thanks for review, I will address these comments soon,

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/