On Thu, 18 Aug 2005, Bob Copeland wrote:

> This patch adds a block device for the Rio Karma.  Not terribly useful
> at the moment because there is no FS driver.

No filesystem driver?  What sort of filesystem does the Rio Karma use?

>  This is my first try at
> doing anything in the kernel and I know almost nothing about USB and
> SCSI other than what it took to figure this out, so be nice :)  Please
> let me know if there is anything weird.

It would have been better to send this to the usb-storage mailing list, 
but never mind.  There are several aspects of your patch that should be
improved.


+#define RIOP_SIGN "RIOP"
+#define CMD_LEN 40
+#define RESP_LEN 0x200

+int rio_karma_init(struct us_data *us) 
+{
+       int result, partial;
+
+       char cmd[CMD_LEN];
+       char buf[RESP_LEN];

Buffers as large as 512 bytes should not be allocated on the stack.  In
addition, on some architectures it isn't possible to do DMA to addresses
on the stack (so these buffers could not be used for USB transfers).  You
should use kmalloc instead.

+       /* SINGLE_LUN flag is reset for Bulk devices - fix it */
+       us->max_lun = 0; 

The comment is certainly wrong, and I don't think the assignment is
needed either.

+       memset(cmd, 0, sizeof(cmd));
+       memset(buf, 0, sizeof(buf));

Why do you need to store 0's in buf?  They will be overwritten by the 
reply from the device.

+       strcpy(cmd, RIOP_SIGN);
+       cmd[5] = 1;
+       cmd[6] = 8;

How come those two bytes aren't also part of RIOP_SIGN?  And since cmd
is really a byte array rather than a string, you should use memcpy
rather than strcpy.

+       result = usb_stor_bulk_transfer_buf(us, us->send_bulk_pipe, cmd,
+               sizeof(cmd), &partial);
+
+       if (result != USB_STOR_XFER_GOOD)
+               goto init_failed;
+
+       result = usb_stor_bulk_transfer_buf(us, us->recv_bulk_pipe, 
+               buf, sizeof(buf), &partial);
+
+       if (result != USB_STOR_XFER_GOOD)
+               goto init_failed;
+
+       for (;;) {
+
+               memset(cmd, 0, sizeof(cmd));
+               memset(buf, 0, sizeof(buf));

You don't need to set buf to 0's, and you don't need to reinitialize
cmd, apart from bytes 4 and 5.  And the change to those two bytes can be
made before the "for" loop begins.

+               strcpy(cmd, RIOP_SIGN);
+               cmd[4] = 0x80;
+               cmd[6] = 0x08;
+
+               US_DEBUGP("Sending init command\n");
+               result = usb_stor_bulk_transfer_buf(us, us->send_bulk_pipe, 
+                       cmd, sizeof(cmd), &partial);
+
+               if (result != USB_STOR_XFER_GOOD)
+                       goto init_failed;
+
+               result = usb_stor_bulk_transfer_buf(us, us->recv_bulk_pipe, 
+                       buf, sizeof(buf), &partial);
+
+               if (result != USB_STOR_XFER_GOOD)
+                       goto init_failed;
+               
+               if (buf[5] == cmd[6]) {

This is a strange comparison.  Why not compare against 0x08?

+                       US_DEBUGP("Karma initialized.\n");
+                       return 0;
+               }
+
+               if (time_after(timeout, jiffies))

You probably mean time_after(jiffies, timeout).

+                       break;
+
+               msleep(10);
+       }
+
+init_failed:
+       US_DEBUGP("Could not initialize karma.\n");
+       return USB_STOR_TRANSPORT_FAILED;
+}
+
diff -uprN linux-2.6.12/drivers/usb/storage/rio_karma.h 
linux-2.6.12-work/drivers/usb/storage/rio_karma.h
--- linux-2.6.12/drivers/usb/storage/rio_karma.h        1969-12-31 
19:00:00.000000000 -0500
+++ linux-2.6.12-work/drivers/usb/storage/rio_karma.h   2005-08-18 
10:14:50.000000000 -0400

+#ifndef _RIO_KARMA_H
+#define _RIO_KARMA_H
+
+#include <linux/config.h>
+#include "usb.h"
+
+int rio_karma_init(struct us_data *);
+
+#endif

Although there's nothing wrong with creating rio_karma.[ch], since they
contain nothing more than an initialization routine it might be more
appropriate to merge your code into initializers.[ch].

diff -uprN linux-2.6.12/drivers/usb/storage/.tmp_versions/usb-storage.mod 
linux-2.6.12-work/drivers/usb/storage/.tmp_versions/usb-storage.mod
--- linux-2.6.12/drivers/usb/storage/.tmp_versions/usb-storage.mod      
1969-12-31 19:00:00.000000000 -0500
+++ linux-2.6.12-work/drivers/usb/storage/.tmp_versions/usb-storage.mod 
2005-08-18 10:29:27.000000000 -0400

Including this file in the patch was a mistake.  It's not part of the source
code.

diff -uprN linux-2.6.12/drivers/usb/storage/unusual_devs.h 
linux-2.6.12-work/drivers/usb/storage/unusual_devs.h
--- linux-2.6.12/drivers/usb/storage/unusual_devs.h     2005-06-17 
15:48:29.000000000 -0400
+++ linux-2.6.12-work/drivers/usb/storage/unusual_devs.h        2005-08-18 
09:15:56.000000000 -0400
@@ -1039,3 +1039,12 @@ UNUSUAL_DEV(  0x55aa, 0xa103, 0x0000, 0x
                US_SC_SCSI, US_PR_SDDR55, NULL,
                US_FL_SINGLE_LUN),
 #endif
+
+#ifdef CONFIG_USB_STORAGE_KARMA
+UNUSUAL_DEV(  0x045a, 0x5210, 0x0000, 0x9999,
+               "Rio",
+               "Rio Karma",
+               US_SC_SCSI, US_PR_BULK, rio_karma_init,
+               US_FL_FIX_INQUIRY | US_FL_SINGLE_LUN ),
+#endif
+

I doubt that you need the FIX_INQUIRY flag, and the SINGLE_LUN flag is
certainly not necessary.

Alan Stern



-------------------------------------------------------
SF.Net email is Sponsored by the Better Software Conference & EXPO
September 19-22, 2005 * San Francisco, CA * Development Lifecycle Practices
Agile & Plan-Driven Development * Managing Projects & Teams * Testing & QA
Security * Process Improvement & Measurement * http://www.sqe.com/bsce5sf
_______________________________________________
linux-usb-devel@lists.sourceforge.net
To unsubscribe, use the last form field at:
https://lists.sourceforge.net/lists/listinfo/linux-usb-devel

Reply via email to