Re: [patch 6/7] ps3: ROM Storage Driver
On Tue, May 29, 2007 at 01:11:41PM +0200, Geert Uytterhoeven wrote: This looks very inefficient. Just set sg_tablesize of your driver to 1 to avoid getting mutiple segments. The disadvantage of setting sg_tablesize = 1 is that the driver will get small requests (PAGE_SIZE) most of the time, which is very bad for performance. If you set .clustering = 1 in your host template you will frequently get larger requests. For any sane hypervisor or hardware the copy should be worth than that. Then again a sane hardware or hypervisor would support SG requests.. - To unsubscribe from this list: send the line unsubscribe linux-scsi in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [patch 6/7] ps3: ROM Storage Driver
On Wed, 2007-05-30 at 12:13 +0200, Christoph Hellwig wrote: For any sane hypervisor or hardware the copy should be worth than that. Then again a sane hardware or hypervisor would support SG requests.. Agreed... Sony should fix that, it's a bit ridiculous. Ben. - To unsubscribe from this list: send the line unsubscribe linux-scsi in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [patch 6/7] ps3: ROM Storage Driver
Benjamin Herrenschmidt wrote: On Wed, 2007-05-30 at 12:13 +0200, Christoph Hellwig wrote: For any sane hypervisor or hardware the copy should be worth than that. Then again a sane hardware or hypervisor would support SG requests.. Agreed... Sony should fix that, it's a bit ridiculous. Yes, if only to put an end to seeing this kind of comment over and over again. -Geoff - To unsubscribe from this list: send the line unsubscribe linux-scsi in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [patch 6/7] ps3: ROM Storage Driver
[Note that all scsi lldds should go to linux-scsi]
+config PS3_ROM
+ tristate PS3 ROM Storage Driver
+ depends on PPC_PS3 BLK_DEV_SR
+ select PS3_STORAGE
+ default y
please don't put any default y statements in.
+#define DEVICE_NAME ps3rom
+
+#define BOUNCE_SIZE (64*1024)
+
+#define PS3ROM_MAX_SECTORS (BOUNCE_SIZE / CD_FRAMESIZE)
+
+#define LV1_STORAGE_SEND_ATAPI_COMMAND (1)
+
+
+struct ps3rom_private {
+ spinlock_t lock;
+ struct task_struct *thread;
+ struct Scsi_Host *host;
+ struct scsi_cmnd *cmd;
+ void (*scsi_done)(struct scsi_cmnd *);
+};
+#define ps3rom_priv(dev) ((dev)-sbd.core.driver_data)
+/*
+ * to position parameter
+ */
+enum {
+ NOT_AVAIL = -1,
+ USE_SRB_10 = -2,
+ USE_SRB_6 = -3,
+ USE_CDDA_FRAME_RAW = -4
+};
none of these seem to be used at all in the driver.
+
+#ifdef DEBUG
+static const char *scsi_command(unsigned char cmd)
+{
+ switch (cmd) {
+ case TEST_UNIT_READY: return
TEST_UNIT_READY/GPCMD_TEST_UNIT_READY;
+ case REZERO_UNIT: return REZERO_UNIT;
+ case REQUEST_SENSE: return
REQUEST_SENSE/GPCMD_REQUEST_SENSE;
...
this kind of things shouldn't be in a low level driver. Either keep it
in your out of tree debug patches or if you feel adventurous send a
patch to linux-scsi that implements it in drivers/scsi/constant.c which
has debug code for other protocol-level scsi constants.
+static int ps3rom_slave_alloc(struct scsi_device *scsi_dev)
+{
+ struct ps3_storage_device *dev;
+
+ dev = (struct ps3_storage_device *)scsi_dev-host-hostdata[0];
+
+ dev_dbg(dev-sbd.core, %s:%u: id %u, lun %u, channel %u\n, __func__,
+ __LINE__, scsi_dev-id, scsi_dev-lun, scsi_dev-channel);
+
+ scsi_dev-hostdata = dev;
This seems rather pointless. The scsi_device has a pointer to the
host, so every access to scsi_dev-hostdata can simply be replaced
by an access through the host.
+static int ps3rom_slave_configure(struct scsi_device *scsi_dev)
+{
+ struct ps3_storage_device *dev = scsi_dev-hostdata;
+
+ dev_dbg(dev-sbd.core, %s:%u: id %u, lun %u, channel %u\n, __func__,
+ __LINE__, scsi_dev-id, scsi_dev-lun, scsi_dev-channel);
+
+ /*
+ * ATAPI SFF8020 devices use MODE_SENSE_10,
+ * so we can prohibit MODE_SENSE_6
+ */
+ scsi_dev-use_10_for_ms = 1;
+
+ return 0;
+}
+
+static void ps3rom_slave_destroy(struct scsi_device *scsi_dev)
+{
+}
No need to implement an empty method here.
+static int ps3rom_queuecommand(struct scsi_cmnd *cmd,
+void (*done)(struct scsi_cmnd *))
+{
+ struct ps3_storage_device *dev = cmd-device-hostdata;
+ struct ps3rom_private *priv = ps3rom_priv(dev);
+
+ dev_dbg(dev-sbd.core, %s:%u: command 0x%02x (%s)\n, __func__,
+ __LINE__, cmd-cmnd[0], scsi_command(cmd-cmnd[0]));
+
+ spin_lock_irq(priv-lock);
+ if (priv-cmd) {
+ /* no more than one can be processed */
+ dev_err(dev-sbd.core, %s:%u: more than 1 command queued\n,
+ __func__, __LINE__);
+ spin_unlock_irq(priv-lock);
+ return SCSI_MLQUEUE_HOST_BUSY;
+ }
Just set can_queue to 1 in the host template and the midlayer will take
care of this.
+
+ // FIXME Prevalidate commands?
+ priv-cmd = cmd;
+ priv-scsi_done = done;
No need to keep your own scsi_done pointer. What you should do instead
in queuecommand is to set the scsi_done pointer in the scsi_cmnd here
and just use it later.
+ spin_unlock_irq(priv-lock);
+ wake_up_process(priv-thread);
Offloading every I/O to a thread is very bad for I/O performance.
Why do you need this?
+ return 0;
+}
+
+/*
+ * copy data from device into scatter/gather buffer
+ */
+static int fill_from_dev_buffer(struct scsi_cmnd *cmd, const void *buf,
+ int buflen)
+{
+ int k, req_len, act_len, len, active;
+ void *kaddr;
+ struct scatterlist *sgpnt;
+
+ if (!cmd-request_bufflen)
+ return 0;
+
+ if (!cmd-request_buffer)
+ return DID_ERROR 16;
+
+ if (cmd-sc_data_direction != DMA_BIDIRECTIONAL
+ cmd-sc_data_direction != DMA_FROM_DEVICE)
+ return DID_ERROR 16;
+
+ if (!cmd-use_sg) {
+ req_len = cmd-request_bufflen;
+ act_len = min(req_len, buflen);
+ memcpy(cmd-request_buffer, buf, act_len);
+ cmd-resid = req_len - act_len;
+ return 0;
+ }
This is never true anymore.
+
+ sgpnt = cmd-request_buffer;
+ active = 1;
+ for (k = 0, req_len = 0, act_len = 0; k cmd-use_sg; ++k, ++sgpnt) {
+ if (active) {
+ kaddr = kmap_atomic(sgpnt-page, KM_USER0);
+
Re: [patch 6/7] ps3: ROM Storage Driver
On Tue, 29 May 2007, Christoph Hellwig wrote:
[Note that all scsi lldds should go to linux-scsi]
I'll Cc linux-scsi next time.
+ sgpnt = cmd-request_buffer;
+ active = 1;
+ for (k = 0, req_len = 0, act_len = 0; k cmd-use_sg; ++k, ++sgpnt) {
+ if (active) {
+ kaddr = kmap_atomic(sgpnt-page, KM_USER0);
+ if (!kaddr)
+ return DID_ERROR 16;
+ len = sgpnt-length;
+ if ((req_len + len) buflen) {
+ active = 0;
+ len = buflen - req_len;
+ }
+ memcpy(kaddr + sgpnt-offset, buf + req_len, len);
+ kunmap_atomic(kaddr, KM_USER0);
+ act_len += len;
+ }
+ req_len += sgpnt-length;
+ }
+ cmd-resid = req_len - act_len;
This looks very inefficient. Just set sg_tablesize of your driver
to 1 to avoid getting mutiple segments.
The disadvantage of setting sg_tablesize = 1 is that the driver will get small
requests (PAGE_SIZE) most of the time, which is very bad for performance.
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- Sony Network and Software Technology Center Europe (NSCE)
[EMAIL PROTECTED] --- The Corporate Village, Da Vincilaan 7-D1
Voice +32-2-7008453 Fax +32-2-7008622 B-1935 Zaventem, Belgium
-
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [patch 6/7] ps3: ROM Storage Driver
On Tue, 2007-05-29 at 13:11 +0200, Geert Uytterhoeven wrote: This looks very inefficient. Just set sg_tablesize of your driver to 1 to avoid getting mutiple segments. The disadvantage of setting sg_tablesize = 1 is that the driver will get small requests (PAGE_SIZE) most of the time, which is very bad for performance. And the joke is that not only the HW can do scatter gather but you also have an iommu ... Ben. - To unsubscribe from this list: send the line unsubscribe linux-scsi in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [patch 6/7] ps3: ROM Storage Driver
On Tue, 29 May 2007, Christoph Hellwig wrote:
+/*
+ * copy data from device into scatter/gather buffer
+ */
+static int fill_from_dev_buffer(struct scsi_cmnd *cmd, const void *buf,
+ int buflen)
+{
+ int k, req_len, act_len, len, active;
+ void *kaddr;
+ struct scatterlist *sgpnt;
+
+ if (!cmd-request_bufflen)
+ return 0;
+
+ if (!cmd-request_buffer)
+ return DID_ERROR 16;
+
+ if (cmd-sc_data_direction != DMA_BIDIRECTIONAL
+ cmd-sc_data_direction != DMA_FROM_DEVICE)
+ return DID_ERROR 16;
+
+ if (!cmd-use_sg) {
+ req_len = cmd-request_bufflen;
+ act_len = min(req_len, buflen);
+ memcpy(cmd-request_buffer, buf, act_len);
+ cmd-resid = req_len - act_len;
+ return 0;
+ }
This is never true anymore.
Just to be sure: all four if-cases or only the last one?
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- Sony Network and Software Technology Center Europe (NSCE)
[EMAIL PROTECTED] --- The Corporate Village, Da Vincilaan 7-D1
Voice +32-2-7008453 Fax +32-2-7008622 B-1935 Zaventem, Belgium
-
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
