Will you be running a git tree for this?  If so, please send me the link.

Will this enable that thus-far useless slot on my Vaio?

Where did the info come from which enabled this driver to be written?  I

Are you sure this has enough dependencies?  CONFIG_TIFM_* for a start?

<fiddles with Kconfig>

We can create a config which has CONFIG_TIFM_CORE=m, CONFIG_MEMSTICK=y. 
That might not work?


I trust higher-level code cleans up the MEMSTICK_TYPE string if this call

Could just do:

	int rc = -ENODEV;
	if (dev->driver && drv->probe) {
		rc = drv->probe(card);
		if (!rc)
			get_device(dev);
	}
	return rc;

If the earlier get_device() is indeed needed then we already have a


It's kinda polite to add some commentary to global, exported-to-modules
functions.

It leads to more effective code review too.  Reviewing code is the process
of determining whether implementation != intention.  But without comments,
the reviewer's starting point is intention = implementation.  So we end up

Without comments this little reader is mystified as to why this function

You may find that gcc generates crappy code for this: assembles a struct on
the stack them does a memcpy (or even a memb-er-by-member copy).  Doing

Why are these packed?  Do they map onto something whcih hardware knows
about?


You're aware that these four fields occupy the same machine word and that
the compiler provides no locking?  So if one thread attempts to modify
"read_only" while another modifies "has_request", one can corrupt the work
of the other?

If this is indeed safe then a comment here whcih clearly explains that



Again, this will return a wrong result if the output was truncated. 

ow. my brain hurts.

The `if (cnt)' is actually unneeded.  But if it were mine I'd be doing
something simpler and obviouser here.  Like `for (i = 0; i < cnt; i++)'


It would be simpler and clearer to do

	struct ms_status_register *msr;
	msr = (struct ms_status_register *)(*mrq)->data;
	if ...
[ message continues ]

It's suboptimal and doesn't work for non-fs request. Just use
end_queued_request() instead:

if (msb->q_thread) {
        msb->has_request = 1;
        wake_up(&msb->q_wait);
} else {
        while ((req = elv_next_request(q)) != NULL)
                end_queued_request(req, -ENODEV);
}

which is simpler and gets all cases correct. Reordering for normal case
as well.

-- 
Jens Axboe

--

[ message continues ]

I'm going to set-up git tree, yes.

It should support some vaios (newer ones for sure), as far as I know.

The primary reference for the driver is this one:
http://zeniv.linux.org.uk/~winbond/ (link is somewhat slow, but works). Winbond developed GPLv2
driver for linux and there was enough info in their source code for my implementation (totally
different from their's). Some reverse engineering of the windows driver was needed too (for the TI

This is supposed to be a generic layer, akin to mmc. I have a jmicron backend in the works, and



Yes, of course; all structures with "packed" attribute correspond to appropriate protocol ones

The access to these fields should be exclusively under q_lock (I'll check the locking again, just

The idea here is that MSPro media have an attribute namespace, which may contain various entries.
Some of them I know how to decode, others I still want to be presented in sysfs for further study



There's no IO path at this point, as IO thread was stopped on suspend. The fine point here is that
thread is restarted only with "UNSAFE_RESUME" set, otherwise the block device just sits dead in

Somebody was already kind enough to fix this.

I hope it'll be ok for me to address the rest of the issues with incremental patches.I expect to
have a web accessible git within a few days.



      ____________________________________________________________________________________
Looking for last minute shopping deals?  
Find them fast with Yahoo! Search.  http://tools.search.yahoo.com/newsearch/category.php?category=shopping
--

[ message continues ]

I rectified several additional issues with the driver. I hope you can pull the changes from here:

http://58.96.64.63/~oakad/git/.git memstick




      ____________________________________________________________________________________
Be a better friend, newshound, and 
know-it-all with Yahoo! Mobile.  Try it now.  http://mobile.yahoo.com/;_ylt=Ahu06i62sR8HDtDypao8Wcj9tAcJ 

--

[ message continues ]

Would prefer an emailed, changelogged diff against the previous version, please.
--

[ message continues ]

* Mark shared inline functions as static

* Use member-at-a-time assignment for protocol structures

* Comments for publicly exported functions

* Use end_queued_request to end unhandled block layer requests

* Use sysfs attribute group to export MSPro attributes

* Fix includes

* Use scnprintf instead of snprintf where string length matters

* Remove spurious get_device/put_device in probe method


diff --git a/drivers/memstick/core/memstick.c b/drivers/memstick/core/memstick.c
index 46e5f9b..bba467f 100644
--- a/drivers/memstick/core/memstick.c
+++ b/drivers/memstick/core/memstick.c
@@ -12,11 +12,10 @@
  *
  */

-#include <linux/tifm.h>
 #include <linux/memstick.h>
 #include <linux/idr.h>
-#include <linux/scatterlist.h>
 #include <linux/fs.h>
+#include <linux/delay.h>

 #define DRIVER_NAME "memstick"
 #define DRIVER_VERSION "0.2"
@@ -86,13 +85,11 @@ static int memstick_device_probe(struct device *dev)
 						   driver);
 	int rc = -ENODEV;

-	get_device(dev);
 	if (dev->driver && drv->probe) {
 		rc = drv->probe(card);
 		if (!rc)
-			return 0;
+			get_device(dev);
 	}
-	put_device(dev);
 	return rc;
 }

@@ -205,12 +202,26 @@ static int memstick_dummy_check(struct memstick_dev *card)
 	return 0;
 }

+/**
+ * memstick_detect_change - schedule media detection on memstick host
+ * @host - host to use
+ */
 void memstick_detect_change(struct memstick_host *host)
 {
 	queue_work(workqueue, &host->media_checker);
 }
 EXPORT_SYMBOL(memstick_detect_change);

+/**
+ * memstick_next_req - called by host driver to obtain next request to process
+ * @host - host to use
+ * @mrq - pointer to stick the request to
+ *
+ * Host calls this function from idle state (*mrq == NULL) or after finishing
+ * previous request (*mrq should point to it). If previous request was
+ * unsuccessful, it is retried for predetermined number of times. Return value
+ * of 0 means that new request was assigned to the host.
+ */
 int memstick_next_req(struct memstick_host ...
[ message continues ]

Please sign off your patches.

Please don't send wordwrapped patches.

Please put the subsystem identifier "memstick" outside [], for reasons
described in http://www.zip.com.au/~akpm/linux/patches/stuff/tpp.txt

Your patch contained a fix which I already merged into -mm, which suggests
that you haven't been testing the code in 2.6.24-rc8-mm1.  Please do test
-mm when you have patches in there, so that we know that we'll be merging
things which work.

Thanks.
--

[ message continues ]

Changes to the block core in mainline have destroyed this driver.  This was
hitherto not known because I was unable to carry git-block in -mm
because it blithely tromped all over other people's code.  

I'll disable the memstick driver in config for now.  Please send fixes?
--

[ message continues ]

Signed-off-by: Alex Dubov <oakad@yahoo.com>

--- mspro_block.c.orig	2008-02-04 15:25:16.000000000 +1100
+++ mspro_block.c	2008-02-04 15:26:28.226886699 +1100
@@ -668,20 +668,13 @@

 		spin_lock_irqsave(&msb->q_lock, flags);
 		if (rc >= 0)
-			chunk = end_that_request_chunk(req, 1, rc);
+			chunk = __blk_end_request(req, 0, rc);
 		else
-			chunk = end_that_request_first(req, rc,
-						       req->current_nr_sectors);
+			chunk = __blk_end_request(req, rc, 0);

 		dev_dbg(&card->dev, "end chunk %d, %d\n", rc, chunk);
-		if (!chunk) {
-			add_disk_randomness(req->rq_disk);
-			blkdev_dequeue_request(req);
-			end_that_request_last(req, rc > 0 ? 1 : rc);
-		}
 		spin_unlock_irqrestore(&msb->q_lock, flags);
 	} while (chunk);
-
 }

 static int mspro_block_has_request(struct mspro_block_data *msb)



      ____________________________________________________________________________________
Be a better friend, newshound, and 
know-it-all with Yahoo! Mobile.  Try it now.  http://mobile.yahoo.com/;_ylt=Ahu06i62sR8HDtDypao8Wcj9tAcJ 

--

[ message continues ]

Thanks.  However please do prepare patches in `patch -p1' form.

Could you please fix the build error in the code in 2.6.24-mm1?

I part-fixed it (then disabled it) with this:

--- a/drivers/memstick/core/mspro_block.c~a
+++ a/drivers/memstick/core/mspro_block.c
@@ -1233,11 +1232,12 @@ static int mspro_block_resume(struct mem
 	unsigned long flags;
 	int rc = 0;
 
-#ifdef CONFIG_MEMSTICK_UNSAFE_RESUME
+#if defined(CONFIG_MEMSTICK_UNSAFE_RESUME) && 0
 
 	struct mspro_block_data *new_msb;
 	struct memstick_host *host = card->host;
-	struct mspro_sys_attr s_attr, r_attr;
+	struct mspro_sys_attr *s_attr;
+	struct mspro_sys_attr *r_attr;
 	unsigned char cnt;
 
 	mutex_lock(&host->lock);
_

see, this:

		s_attr = container_of(new_msb->attr_group.attrs[cnt],
				      struct mspro_sys_attr,
				      dev_attr);

is broken.  Attribute groups hold `struct attribute' but this code thinks
they hold `struct device_attribute'.  I could bodge it to compile cleanly,
but I don't know if it will work.


--

[ message continues ]

Signed-off-by: Alex Dubov <oakad@yahoo.com>
---
 drivers/memstick/core/mspro_block.c |   10 +++-------
 1 files changed, 3 insertions(+), 7 deletions(-)

diff --git a/drivers/memstick/core/mspro_block.c b/drivers/memstick/core/mspro_block.c
index b9bd0aa..423ad8c 100644
--- a/drivers/memstick/core/mspro_block.c
+++ b/drivers/memstick/core/mspro_block.c
@@ -1237,7 +1237,7 @@ static int mspro_block_resume(struct memstick_dev *card)
 
 	struct mspro_block_data *new_msb;
 	struct memstick_host *host = card->host;
-	struct mspro_sys_attr s_attr, r_attr;
+	struct mspro_sys_attr *s_attr, *r_attr;
 	unsigned char cnt;
 
 	mutex_lock(&host->lock);
@@ -1254,12 +1254,8 @@ static int mspro_block_resume(struct memstick_dev *card)
 
 	for (cnt = 0; new_msb->attr_group.attrs[cnt]
 		      && msb->attr_group.attrs[cnt]; ++cnt) {
-		s_attr = container_of(new_msb->attr_group.attrs[cnt],
-				      struct mspro_sys_attr,
-				      dev_attr);
-		r_attr = container_of(msb->attr_group.attrs[cnt],
-				      struct mspro_sys_attr,
-				      dev_attr);
+		s_attr = mspro_from_sysfs_attr(new_msb->attr_group.attrs[cnt]);
+		r_attr = mspro_from_sysfs_attr(msb->attr_group.attrs[cnt]);
 
 		if (s_attr->id == MSPRO_BLOCK_ID_SYSINFO
 		    && r_attr->id == s_attr->id) {
-- 
1.5.3.6


      ____________________________________________________________________________________
Looking for last minute shopping deals?  
Find them fast with Yahoo! Search.  http://tools.search.yahoo.com/newsearch/category.php?category=shopping
--

[ message continues ]

I tried it here and it doesn't work. My Vaio (PCG-FR285M) is from ~2003 (Is it too old
for this?). I have some memory stick cards around so If you want a tester just drop me
an email.

Regards,

	Mariusz
--

[ message continues ]

The build year is nowhere as helpful as 'lspci -vv' output. Then, given that your vaio is equipped
with tifm controller, you'll have to build the driver with debugging enabled and send me the
relevant excerpt of your system log.

You should have the following modules loaded, by the way:
memstick
mspro_block
tifm_core
tifm_7xx1
tifm_ms

The autoloading is handled via udev (so the relevant rules are not there yet).



      ____________________________________________________________________________________
Never miss a thing.  Make Yahoo your home page. 
http://www.yahoo.com/r/hs
--

[ message continues ]