DomainKey-Signature: a=rsa-sha1; q=dns; c=nofws;
  s=s1024; d=yahoo.com;
  h=Message-ID:X-YMail-OSG:Received:X-Mailer:Date:From:Subject:To:Cc:MIME-Version:Content-Type:Content-Transfer-Encoding;
  b=pepxm+NfY6VySfcNz77su5NA4JTGnvoKO4b8p0bdvQ8OW9DmA3HwLh4RpixLNVZYbgwFJsVzoGDU5Qvuy8G8hMZ/DU3vh3HtHoHJDddHuA2eBf/o/94lGZFQFy0ynQ3kqEEy1RGf3w7p6KbuWfQXxsXc7SGVQz+oheP7ftEWFDk=;
Message-ID: <952321.94696.qm@web37608.mail.mud.yahoo.com>
Date: Thu, 5 Aug 2010 01:30:08 -0700 (PDT)
From: Alex Dubov <oakad@yahoo.com>
Subject: Re: [PATCH 2/2] MEMSTICK: Add driver for Ricoh R5C592 Card reader.
To: Maxim Levitsky <maximlevitsky@gmail.com>
Cc: LKML <linux-kernel@vger.kernel.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: 8BIT
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 2734
Lines: 99

> Maxim Levitsky wrote: 
> > On Wed, 2010-08-04 at 00:57 -0700, Alex Dubov wrote: 
> > > I see two immediate problems with this patch:
> > > 
> > > 1. On cosmetic level, custom debug macros should
> not be employed. Device
> > > core already have this functionality (dynamic
> debug levels and such). Please,
> > > use dev_dbg and friends for print-outs.
> > This allows much easier control for debug.
> > Single module parameter is enough to adjust it.
> > This helps me help users.
> > (Eg, kernel compilation is out of question)

I doubt it will be that useful, but it's not my call to make anyway.


> > 
> > 
> > > 
> > > 2. On a structural level, I'd rather prefer host
> drivers to not start their
> > > own threads. If you look at both current host
> implementations, they operate
> > > in callback fashion. Apart from saving some
> resources, this reduces the
> > > amount of problems encountered during
> suspend/resume and shutdown.
> > This isn't possible.
> > Hardware doesn't support interrupts on memstick bus
> changes, it only
> > supports DMA done from/to internal FIFO, and DMA it
> only possible for
> > 512 byte TPCs.
> > 
> 

How depressing.

> 
> Another question.
> 
> I see that current code ignores MEMSTICK_CAP_AUTO_GET_INT
> Instread mspro_blk.c enables this capability for parallel
> mode, assuming
> that hw supports it. Its true in my case, but might not be
> true in other
> cases.
> I think I should fix that, right?

This is mandated by the spec. INT should be available automatically in
parallel mode, and some hardware does it in serial as well.

> 
> Also I see that you bath
> TPC_READ_LONG_DATA/TPC_READ_LONG_DATA
> Does that mean that every HW sector is larger that 512?
> If so, you are doing copy on write, right?
> I have small caching in my sm_ftl of last sector. It helps
> performance a
> lot.


That's how its called in the spec.
Sectors can be larger than 512b on Pro-HG sticks, and there's additional
TPC_READ/WRITE_QUAD_DATA which operates on larger quantities.

> 
> 
> Also I want to clarify that the only kind of interrupts
> supported by hw
> (besides usual card detection interrupt), is DMA done
> interrupt.
> Thats why I have to use thread.
> Doing polling in r592_submit_req (which runs in atomic
> context is just
> cruel).

Yes, I see you have a timed wait there.


> Besides, under moderate IO load, the IO thread doesn't
> sleep, thus there
> is no overhead of wake/sleep.
> 
> 
> Best regards,
> Maxim Levitsky
> 
>


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/