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