linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Maxim Levitsky <maximlevitsky@gmail.com>
To: FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp>
Cc: akpm@linux-foundation.org, James.Bottomley@HansenPartnership.com,
	linux-kernel@vger.kernel.org, oakad@yahoo.com
Subject: Re: [PATCH 1/4] scatterlist: new helper functions
Date: Mon, 07 Mar 2011 04:20:37 +0200	[thread overview]
Message-ID: <1299464437.24051.13.camel@maxim-laptop> (raw)
In-Reply-To: <20110307064900X.fujita.tomonori@lab.ntt.co.jp>

On Mon, 2011-03-07 at 06:49 +0900, FUJITA Tomonori wrote:
> On Sun, 06 Mar 2011 17:14:30 +0200
> Maxim Levitsky <maximlevitsky@gmail.com> wrote:
> 
> > On Sun, 2011-03-06 at 16:29 +0900, FUJITA Tomonori wrote:
> > > On Fri,  4 Mar 2011 06:16:50 +0200
> > > Maxim Levitsky <maximlevitsky@gmail.com> wrote:
> > > 
> > > > While developing memstick driver for legacy memsticks
> > > > I found the need in few helpers that I think should be
> > > > in common scatterlist library
> > > > 
> > > > The functions that were added:
> > > > 
> > > > * sg_nents/sg_total_len - iterate over scatterlist to figure
> > > > out total length of memory it covers / number of entries.
> > > 
> > > You should invent a data structure per I/O request, something like
> > > msb_request structure. Then you can store nents and total_len in
> > > that.
> > > 
> > > That's what block subsystems and drivers do. I took a look at your
> > > driver but I can't see why your driver can't do the same.
> > I also need to break the request into small grained chunks.
> > If I invent such structure, I will end up writing these helpers for it.
> > 
> > The I have this lifetime of a request:
> > 
> > I get arbitary sized request from block layer (I can of course control
> > maximum size/number of segments in it, etc).
> > 
> > I break it into eraseblock sized chunks, and for each I translate the
> > the LBA, into flash address.
> > 
> > Then I break it into flash page sized requests (512 bytes), and yet its
> > better not to assume that such requests always contained in one sg
> > entry.
> > 
> > Worse than that, I have to pass an sg list that spans always one 512
> > page to lowlevel driver, because thats how Alex defined the interface.
> 
> This restriction is due to hardware specification or the software
> design (e.g. memstick layer)? If it is due to the latter, why can't
> you fix that?

Yes.
I already tried addressing some shortcomings of memstick layer, no no, I
don't want to deal with its author, Alex Dubov again.
I think this code tries to be too clever/complex for the range of
devices/speeds it supports, but I rather leave it as is.


To be honest, the code in question is for >5 year old memstick standard
cards, thats hardly anybody uses.
It works, it is more or less simple, its not performance bound, its
testd, and thus I want to keep it as is _for_ now.


Why I break sg lists into chunks?
Because unlike vast majority of block devices, I need to do FTL in the
driver, thus its easier to work on eraseblock boundary.
Also unlike anything else, you can't just read/write a sector from a
memorystick (especially the legacy one), you have to perform full dance
of commands.

Not to mention error handling (like if you failed to write to block, you
must try to choose another one, etc...)

(Of course writes follow same rules as raw nand flash, thats is writes
only clear bits, and you can erase a eraseblock only).



> 
> Why can't the block layer split requests for you? It's better to let
> the block layer handle that.
You mean tell it not to give me more that one eraseblock to handle?
Could you explain that a bit more?


Anyway, could we merge the code?
I would happy to improve it later, but currently merge window is very
close, and the code is more or less agreed upon everyone, and written
more that 1/2 of year ago.

Andrew Morton, could you help me with this, Please?


Best regards,
	Maxim Levitsky

> 
> 
> > Folks, really what the status of this, when to expect it to be merged?
> > 
> > If you think some of helper functions don't belong to scatterlist.c,
> > just tell me to move them back to ms_block.c.
> > 
> > Andrew, please note again that richoh lowlevel driver doesn't need any
> > helper functions, its patch is standalone and thus should be merged
> > regardless.
> 
> I think that we need to make the design of the driver easily
> understandable to kernel developers and maintainable by them. I don't
> think that this is 'standalone or not' issue.
> 
> Adding a doc about why the driver is designed in such odd way would be
> helpful. But I still think that we could design the driver in a better
> way.



  reply	other threads:[~2011-03-07  2:20 UTC|newest]

Thread overview: 42+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2011-03-04  4:16 [PATH 0/4] Memstick patches for 2.6.39 Maxim Levitsky
2011-03-04  4:16 ` [PATCH 1/4] scatterlist: new helper functions Maxim Levitsky
2011-03-06  7:29   ` FUJITA Tomonori
2011-03-06 15:14     ` Maxim Levitsky
2011-03-06 21:49       ` FUJITA Tomonori
2011-03-07  2:20         ` Maxim Levitsky [this message]
2011-03-09 17:24           ` Maxim Levitsky
2011-03-16  0:44           ` Andrew Morton
2011-03-16  2:35             ` FUJITA Tomonori
2011-03-16  4:18               ` Alex Dubov
2011-03-16  4:55                 ` FUJITA Tomonori
2011-03-17  3:46                   ` Alex Dubov
2011-03-17  3:57                     ` James Bottomley
2011-03-16 22:33                 ` Andrew Morton
2011-03-17  6:41                   ` Alex Dubov
2011-03-17 13:17                     ` Maxim Levitsky
2011-03-18  0:59                     ` FUJITA Tomonori
2011-03-04  4:16 ` [PATCH 2/4] memstick: add support for legacy memorysticks Maxim Levitsky
2011-03-04  4:16 ` [PATCH 3/4] memstick: Add driver for Ricoh R5C592 card reader Maxim Levitsky
2011-03-16  4:25   ` Alex Dubov
2011-03-04  4:16 ` [PATCH 4/4] memstick: add Alex Dubov to MAINTAINERS of the memstick core Maxim Levitsky
2011-03-12 16:23 ` [PATH 0/4] Memstick patches for 2.6.39 Maxim Levitsky
2011-03-15 20:00   ` Maxim Levitsky
2011-03-15 21:04     ` Andrew Morton
2011-03-16  0:25       ` Maxim Levitsky
2011-03-20  3:09       ` Maxim Levitsky
2011-03-20  6:47         ` Andrew Morton
2011-03-20 11:42           ` Maxim Levitsky
2011-03-22 13:01             ` Maxim Levitsky
2011-03-22 23:21             ` Andrew Morton
2011-03-23  0:59               ` Maxim Levitsky
2011-03-23  3:42                 ` Greg KH
2011-03-23 15:30                   ` Maxim Levitsky
2011-03-24  3:27                     ` Alex Dubov
  -- strict thread matches above, loose matches on Subject: below --
2011-01-11 23:36 [PATCH V2 0/4]: MEMSTICK: My 2 drivers for 2.6.38 inclusion Maxim Levitsky
2011-01-11 23:36 ` [PATCH 1/4] scatterlist: new helper functions Maxim Levitsky
2011-01-11 23:51   ` Randy Dunlap
2011-01-11 23:55     ` Andrew Morton
2011-01-11 23:58       ` Randy Dunlap
2011-01-12  2:52         ` Maxim Levitsky
2011-01-12  6:41           ` Randy Dunlap
2011-01-11  2:58 Alex Dubov
2011-01-11  1:50 MEMSTICK: My 2 drivers for 2.6.38 inclusion Maxim Levitsky
2011-01-11  1:50 ` [PATCH 1/4] scatterlist: new helper functions Maxim Levitsky

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=1299464437.24051.13.camel@maxim-laptop \
    --to=maximlevitsky@gmail.com \
    --cc=James.Bottomley@HansenPartnership.com \
    --cc=akpm@linux-foundation.org \
    --cc=fujita.tomonori@lab.ntt.co.jp \
    --cc=linux-kernel@vger.kernel.org \
    --cc=oakad@yahoo.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).