RFC: PS3 Storage Drivers

arch/powerpc/platforms/ps3/setup.c +2 -1

-#if defined(CONFIG_FB_PS3) || defined(CONFIG_FB_PS3_MODULE)
+#if defined(CONFIG_FB_PS3) || defined(CONFIG_FB_PS3_MODULE) || \
+    defined(CONFIG_PS3_FLASH_MODULE) ||

defined(CONFIG_PS3_FLASH_MODULE)

Quoted 4 lines · On Fri, 2007-05-25 at 10:36 +0200, [email protected] wrote:

arch/powerpc/platforms/ps3/setup.c +2 -1

-#if defined(CONFIG_FB_PS3) || defined(CONFIG_FB_PS3_MODULE)
+#if defined(CONFIG_FB_PS3) || defined(CONFIG_FB_PS3_MODULE) || \
+    defined(CONFIG_PS3_FLASH_MODULE) ||

defined(CONFIG_PS3_FLASH_MODULE)

As I said multiple times, imho, #ifdef CONFIG_xxx_MODULE in the kernel is always a bug.

You should always be able to build the module out of tree afteward and use it on a kernel that didn't have the CONFIG_xxx_MODULE set imho.

I know.

Do you know another way to allocate an aligned chunk of 256 KiB of physically
contiguous memory, possibly a long time after boot up?

Quoted 3 lines · Geert Uytterhoeven

Do you know another way to allocate an aligned chunk of 256 KiB of physically
contiguous memory, possibly a long time after boot up?

kmalloc & a good prayer ? :-0

Add storage driver probing.
New storage devices are detected and added by a kthread.

arch/powerpc/platforms/ps3/device-init.c +4 -0

+static u64 ps3stor_wait_for_completion(u64 devid, u64 tag,
+				       unsigned int timeout)
+{
+	unsigned int retries = 0;

arch/powerpc/platforms/ps3/device-init.c +6 -0

+	switch (dev_type) {
+	case PS3_DEV_TYPE_STOR_DISK:
+		match_id = PS3_MATCH_ID_STOR_DISK;
+		break;
+
+	case PS3_DEV_TYPE_STOR_ROM:

Quoted 13 lines

arch/powerpc/platforms/ps3/device-init.c +13 -0

+	switch (dev_type) {
+	case PS3_DEV_TYPE_STOR_DISK:
+		match_id = PS3_MATCH_ID_STOR_DISK;
+		break;
+
+	case PS3_DEV_TYPE_STOR_ROM:
+		match_id = PS3_MATCH_ID_STOR_ROM;
+		break;
+
+	case PS3_DEV_TYPE_STOR_FLASH:
+		match_id = PS3_MATCH_ID_STOR_FLASH;
+		break;
+

arch/powerpc/platforms/ps3/device-init.c static int __devinit ps3_register_av(voi · line 691

Quoted 3 lines

arch/powerpc/platforms/ps3/device-init.c +3 -0

+	default:
+		return 0;
+	}

Why do you have separate constants for PS3_DEV_TYPE_* and PS3_MATCH_ID_*? If you don't do any conversion, this driver will immediately work for additional types as well, if more get added later.

Quoted 4 lines · On Friday 25 May 2007, [email protected] wrote:

arch/powerpc/platforms/ps3/device-init.c +4 -0

+static u64 ps3stor_wait_for_completion(u64 devid, u64 tag,
+				       unsigned int timeout)
+{
+	unsigned int retries = 0;

arch/powerpc/platforms/ps3/device-init.c static int __devinit ps3_register_av(voi · line 555

Quoted 10 lines

arch/powerpc/platforms/ps3/device-init.c +10 -0

+	u64 res = -1, status;
+
+	for (retries = 0; retries < timeout; retries++) {
+		res = lv1_storage_check_async_status(NOTIFICATION_DEVID, tag,
+						     &status);
+		if (!res)
+			break;
+		set_current_state(TASK_INTERRUPTIBLE);
+		schedule_timeout(1);
+	}

Any reason not to use msleep(1) instead of the schedule_timeout?

Quoted 13 lines

arch/powerpc/platforms/ps3/device-init.c +13 -0

+	switch (dev_type) {
+	case PS3_DEV_TYPE_STOR_DISK:
+		match_id = PS3_MATCH_ID_STOR_DISK;
+		break;
+
+	case PS3_DEV_TYPE_STOR_ROM:
+		match_id = PS3_MATCH_ID_STOR_ROM;
+		break;
+
+	case PS3_DEV_TYPE_STOR_FLASH:
+		match_id = PS3_MATCH_ID_STOR_FLASH;
+		break;
+

arch/powerpc/platforms/ps3/device-init.c static int __devinit ps3_register_av(voi · line 691

Quoted 3 lines

arch/powerpc/platforms/ps3/device-init.c +3 -0

+	default:
+		return 0;
+	}

Why do you have separate constants for PS3_DEV_TYPE_* and PS3_MATCH_ID_*? If you don't do any conversion, this driver will immediately work for additional types as well, if more get added later.

arch/powerpc/platforms/ps3/device-init.c static int __devinit ps3_register_av(voi · line 784

Quoted 3 lines

arch/powerpc/platforms/ps3/device-init.c +3 -0

+
+//		pr_debug("%s:%u: Checking for new storage devices...\n",
+//			 __func__, __LINE__);

Should be removed, or not in comments, either way is fine, as pr_debug normally does not get compiled in anyway.

arch/powerpc/platforms/ps3/device-init.c static int __devinit ps3_register_av(voi · line 806

Quoted 3 lines

arch/powerpc/platforms/ps3/device-init.c +3 -0

+		msleep_interruptible(ms);
+		if (ms < 60000)
+			ms <<= 1;

Is this timeout only for the disk spinup, or also for detecting media added at run time, like inserting a DVD? One minute timeout for detecting a DVD would sound very long to me.

Quoted 5 lines

arch/powerpc/platforms/ps3/device-init.c static int __devinit ps3_register_av(voi · line 806

Quoted 3 lines

arch/powerpc/platforms/ps3/device-init.c +3 -0

+           msleep_interruptible(ms);
+           if (ms < 60000)
+                   ms <<= 1;

Is this timeout only for the disk spinup, or also for detecting

media

Quoted 2 lines

added at run time, like inserting a DVD? One minute timeout for detecting a DVD would sound very long to me.

It's not for inserting DVDs, only for new devices showing up in the repository.
Apparently new devices may keep on showing up a while after boot up, but I
think this matters only for the kboot kernel, that's why I went with the
exponential back-off with upper limit.

arch/powerpc/platforms/ps3/device-init.c static int __devinit ps3_register_av(voi

Quoted 8 lines

arch/powerpc/platforms/ps3/device-init.c +8 -0

+static u64 ps3stor_wait_for_completion(u64 devid, u64 tag,
+                                    unsigned int timeout)
+{
+     unsigned int retries = 0;
+     u64 res = -1, status;
+
+     for (retries = 0; retries < timeout; retries++) {
+             res =

lv1_storage_check_async_status(NOTIFICATION_DEVID, tag,

arch/powerpc/platforms/ps3/device-init.c static int __devinit ps3_register_av(voi · line 555

Quoted 6 lines

arch/powerpc/platforms/ps3/device-init.c +6 -0

+                                                  &status);
+             if (!res)
+                     break;
+             set_current_state(TASK_INTERRUPTIBLE);
+             schedule_timeout(1);
+     }

Any reason not to use msleep(1) instead of the schedule_timeout?

Quoted 16 lines · Arnd Bergmann

arch/powerpc/platforms/ps3/device-init.c static int __devinit ps3_register_av(voi

Quoted 8 lines

arch/powerpc/platforms/ps3/device-init.c +8 -0

+static u64 ps3stor_wait_for_completion(u64 devid, u64 tag,
+                                    unsigned int timeout)
+{
+     unsigned int retries = 0;
+     u64 res = -1, status;
+
+     for (retries = 0; retries < timeout; retries++) {
+             res =

lv1_storage_check_async_status(NOTIFICATION_DEVID, tag,

Quoted 6 lines

arch/powerpc/platforms/ps3/device-init.c +6 -0

+                                                  &status);
+             if (!res)
+                     break;
+             set_current_state(TASK_INTERRUPTIBLE);
+             schedule_timeout(1);
+     }

Quoted 1 line

Any reason not to use msleep(1) instead of the schedule_timeout?

Both look equally ugly though... do you really have to poll ?

Add a Disk Storage Driver for the PS3:

Quoted 1 line · On Fri, May 25, [email protected] wrote:

Add a Disk Storage Driver for the PS3:

There is no device symlink in /sys/block/ps3da/

Quoted 4 lines · Olaf Hering

Quoted 1 line · On Fri, May 25, [email protected] wrote:

Add a Disk Storage Driver for the PS3:

There is no device symlink in /sys/block/ps3da/

Interesting... Do you know how to create it?

drivers/block/ps3disk.c new file +21 -0

+static void ps3disk_scatter_gather(struct ps3_storage_device *dev,
+				   struct request *req, int gather)
+{
+	unsigned int sectors = 0, offset = 0;
+	struct bio *bio;
+	sector_t sector;
+	struct bio_vec *bvec;
+	unsigned int i = 0, j;
+	size_t size;
+	void *buf;
+
+	rq_for_each_bio(bio, req) {
+		sector = bio->bi_sector;
+		dev_dbg(&dev->sbd.core,
+			"%s:%u: bio %u: %u segs %u sectors from %lu\n",
+			__func__, __LINE__, i, bio_segments(bio),
+			bio_sectors(bio), sector);
+		bio_for_each_segment(bvec, bio, j) {
+			size = bio_cur_sectors(bio)*KERNEL_SECTOR_SIZE;
+			buf = __bio_kmap_atomic(bio, j, KM_USER0);
+			if (gather)

drivers/block/ps3disk.c new file +23 -0

+static int ps3disk_thread(void *data)
+{
+	struct ps3_storage_device *dev = data;
+	struct ps3disk_private *priv = ps3disk_priv(dev);
+	request_queue_t *q = priv->queue;
+	struct request *req;
+
+	dev_dbg(&dev->sbd.core, "%s thread init\n", __func__);
+
+	current->flags |= PF_NOFREEZE;
+
+	while (!kthread_should_stop()) {
+		spin_lock_irq(&priv->lock);
+		set_current_state(TASK_INTERRUPTIBLE);
+		req = elv_next_request(q);
+		if (!req) {
+			spin_unlock_irq(&priv->lock);
+			schedule();
+			continue;
+		}
+		if (!blk_fs_request(req)) {
+			blk_dump_rq_flags(req, DEVICE_NAME " bad request");
+			end_request(req, 0);

Quoted 27 lines · On Friday 25 May 2007, [email protected] wrote:

drivers/block/ps3disk.c new file +27 -0

+static void ps3disk_scatter_gather(struct ps3_storage_device *dev,
+				   struct request *req, int gather)
+{
+	unsigned int sectors = 0, offset = 0;
+	struct bio *bio;
+	sector_t sector;
+	struct bio_vec *bvec;
+	unsigned int i = 0, j;
+	size_t size;
+	void *buf;
+
+	rq_for_each_bio(bio, req) {
+		sector = bio->bi_sector;
+		dev_dbg(&dev->sbd.core,
+			"%s:%u: bio %u: %u segs %u sectors from %lu\n",
+			__func__, __LINE__, i, bio_segments(bio),
+			bio_sectors(bio), sector);
+		bio_for_each_segment(bvec, bio, j) {
+			size = bio_cur_sectors(bio)*KERNEL_SECTOR_SIZE;
+			buf = __bio_kmap_atomic(bio, j, KM_USER0);
+			if (gather)
+				memcpy(dev->bounce_buf+offset, buf, size);
+			else
+				memcpy(buf, dev->bounce_buf+offset, size);
+			offset += size;
+			__bio_kunmap_atomic(bio, KM_USER0);
+		}

drivers/block/ps3disk.c line 105

Quoted 4 lines

drivers/block/ps3disk.c new file +4 -0

+		sectors += bio_sectors(bio);
+		i++;
+	}
+}

So the hypervison uses guest-real addresses here? I would have expected it to use the kernel page tables, which lets you use vmap() to do scatter-gather.

Quoted 29 lines

drivers/block/ps3disk.c new file +29 -0

+static int ps3disk_thread(void *data)
+{
+	struct ps3_storage_device *dev = data;
+	struct ps3disk_private *priv = ps3disk_priv(dev);
+	request_queue_t *q = priv->queue;
+	struct request *req;
+
+	dev_dbg(&dev->sbd.core, "%s thread init\n", __func__);
+
+	current->flags |= PF_NOFREEZE;
+
+	while (!kthread_should_stop()) {
+		spin_lock_irq(&priv->lock);
+		set_current_state(TASK_INTERRUPTIBLE);
+		req = elv_next_request(q);
+		if (!req) {
+			spin_unlock_irq(&priv->lock);
+			schedule();
+			continue;
+		}
+		if (!blk_fs_request(req)) {
+			blk_dump_rq_flags(req, DEVICE_NAME " bad request");
+			end_request(req, 0);
+			spin_unlock_irq(&priv->lock);
+			continue;
+		}
+		spin_unlock_irq(&priv->lock);
+		ps3disk_handle_request_sg(dev, req);
+	}

drivers/block/ps3disk.c line 181

Quoted 4 lines

drivers/block/ps3disk.c new file +4 -0

+
+	dev_dbg(&dev->sbd.core, "%s thread exit\n", __func__);
+	return 0;
+}

I don't really understand what the kthread is needed for. You probably thought about multiple options and ended up with this, but having a comment in front of it might be helpful.

Quoted 4 lines

I don't really understand what the kthread is needed for. You probably thought about multiple options and ended up with this, but having a comment in front of it might be helpful.

I used a kthread because the request function of a block device driver must be non-blocking, and ps3stor_read_write_sectors() calls wait_for_completion().

Quoted 6 lines · Geert Uytterhoeven

Quoted 3 lines

I don't really understand what the kthread is needed for. You probably thought about multiple options and ended up with this, but having a comment in front of it might be helpful.

I used a kthread because the request function of a block device driver must be non-blocking, and ps3stor_read_write_sectors() calls wait_for_completion().

Ok, but why does it call wait_for_completion() then?
I thought you could end_that_request_* from the interrupt handler instead.

Quoted 2 lines · Arnd Bergmann

Ok, but why does it call wait_for_completion() then?
I thought you could end_that_request_* from the interrupt handler instead.

Actually I tried that first, but I ran into other problems, like my request handler being called continuously and requests gotten stuck. But maybe it was just a locking bug on my side.

I can retry, but a disadvantage will be that there will be less code shared with ps3flash and ps3rom.

I used a kthread because the request function of a block device driver must be
non-blocking, and ps3stor_read_write_sectors() calls wait_for_completion().

So the hypervison uses guest-real addresses here? I would have expected it to use the kernel page tables, which lets you use vmap() to do scatter-gather.

I don't really understand what the kthread is needed for. You probably thought about multiple options and ended up with this, but having a comment in front of it might be helpful.

Review

Quoted 3 lines · On Fri, May 25, [email protected] wrote:

+++ b/drivers/scsi/ps3rom.c

(quoted diff) +1 -0

+			kaddr = kmap_atomic(sgpnt->page, KM_USER0);

linux/highmem.h is not included to get the kmap_* prototypes.

Quoted 7 lines · Olaf Hering

Review

Quoted 3 lines · On Fri, May 25, [email protected] wrote:

+++ b/drivers/scsi/ps3rom.c

(quoted diff) +1 -0

+			kaddr = kmap_atomic(sgpnt->page, KM_USER0);

linux/highmem.h is not included to get the kmap_* prototypes.

Beside, I don't see the point of using kmap on ppc64...

Quoted 5 lines · Benjamin Herrenschmidt

Quoted 4 lines · Olaf Hering

Quoted 1 line · On Fri, May 25, [email protected] wrote:

+++ b/drivers/scsi/ps3rom.c

Quoted 5 lines

Quoted 3 lines

Review

Quoted 1 line

(quoted diff) +1 -0

+			kaddr = kmap_atomic(sgpnt->page, KM_USER0);

linux/highmem.h is not included to get the kmap_* prototypes.

Beside, I don't see the point of using kmap on ppc64...

So what should I use instead?

Quoted 5 lines

Quoted 3 lines

Quoted 1 line

linux/highmem.h is not included to get the kmap_* prototypes.

Beside, I don't see the point of using kmap on ppc64...

So what should I use instead?

you don't need to map ... the linear mapping is there.... page_address() should just work. But then, kmap will resolve to just that anyway so I suppose it doesn't matter.

Add a CD/DVD/BD Storage Driver for the PS3:
- Implemented as a SCSI device driver

Quoted 2 lines · On Friday 25 May 2007, [email protected] wrote:

Add a CD/DVD/BD Storage Driver for the PS3:
- Implemented as a SCSI device driver

I assume you tried implementing it as a block device driver, like you PS3 disk driver does, and failed for some reason.

What is the problem? Is there infrastructure missing in the CD-ROM layer?

Quoted 3 lines

What is the problem? Is there infrastructure missing in the CD-ROM layer?

As the CD/DVD/BD part just accepts SCSI/ATAPI commands (except for plain read/write), I was suggested to keep it as a SCSI driver.

Quoted 4 lines · Geert Uytterhoeven

Quoted 3 lines

What is the problem? Is there infrastructure missing in the CD-ROM layer?

Quoted 2 lines

As the CD/DVD/BD part just accepts SCSI/ATAPI commands (except for plain read/write), I was suggested to keep it as a SCSI driver.

Ok, so I guess the tradeoff here is that by writing it as a SCSI driver, you don't need to implement any of the cdrom_device_ops yourself but instead need to fake a few of the SCSI commands in ps3rom_request(). Fair enough.

drivers/scsi/ps3rom.c new file +17 -0

+#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)

drivers/scsi/ps3rom.c new file +9 -0

+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,

drivers/scsi/ps3rom.c new file +7 -0

+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__,

drivers/scsi/ps3rom.c new file +20 -0

+	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 &&

drivers/scsi/ps3rom.c new file +10 -0

+
+	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) {

drivers/scsi/ps3rom.c new file +15 -0

+static void ps3rom_request(struct ps3_storage_device *dev,
+			   struct scsi_cmnd *cmd)
+{
+	unsigned char opcode = cmd->cmnd[0];
+	struct ps3rom_private *priv = ps3rom_priv(dev);
+
+	dev_dbg(&dev->sbd.core, "%s:%u: command 0x%02x (%s)\n", __func__,
+		__LINE__, opcode, scsi_command(opcode));
+
+	switch (opcode) {
+	case INQUIRY:
+		ps3rom_atapi_request(dev, cmd, srb6_len(cmd),
+				     PIO_DATA_IN_PROTO, DIR_READ, 1);
+		break;
+

drivers/scsi/ps3rom.c new file +6 -0

+	struct ps3_storage_device *dev = to_ps3_storage_device(&_dev->core);
+	struct ps3rom_private *priv;
+	int error;
+	struct Scsi_Host *host;
+	struct task_struct *task;
+

[Note that all scsi lldds should go to linux-scsi]

Quoted 14 lines

drivers/scsi/ps3rom.c new file +14 -0

+	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);

drivers/scsi/ps3rom.c line 314

Quoted 5 lines

drivers/scsi/ps3rom.c new file +5 -0

+			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.

Quoted 2 lines

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.

Quoted 2 lines

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.

For any sane hypervisor or hardware the copy should be worth than that. Then again a sane hardware or hypervisor would support SG requests..

Quoted 4 lines · Christoph Hellwig

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.

Quoted 21 lines

drivers/scsi/ps3rom.c new file +21 -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) {

drivers/scsi/ps3rom.c line 294

Quoted 6 lines

drivers/scsi/ps3rom.c new file +6 -0

+		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.

Quoted 21 lines · Christoph Hellwig

Quoted 21 lines

drivers/scsi/ps3rom.c new file +21 -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) {

Quoted 8 lines

drivers/scsi/ps3rom.c line 294

Quoted 6 lines

drivers/scsi/ps3rom.c new file +6 -0

+		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?

Add a FLASH ROM Storage Driver for the PS3:
- Implemented as a misc character device driver
- Uses a fixed 256 KiB buffer allocated from boot memory as the hypervisor requires the writing of aligned 256 KiB blocks

Quoted 4 lines · On Fri, May 25, 2007 at 10:36:14AM +0200, [email protected] wrote:

Add a FLASH ROM Storage Driver for the PS3:
- Implemented as a misc character device driver
- Uses a fixed 256 KiB buffer allocated from boot memory as the hypervisor requires the writing of aligned 256 KiB blocks

Looks good, but please either make the driver aware of multiple devices even if they can't happen currently, or alternatively error out in ->probe if of some reason it's called for a second device.

Quoted 6 lines · Christoph Hellwig

Quoted 4 lines · On Fri, May 25, 2007 at 10:36:14AM +0200, [email protected] wrote:

Add a FLASH ROM Storage Driver for the PS3:
- Implemented as a misc character device driver
- Uses a fixed 256 KiB buffer allocated from boot memory as the hypervisor requires the writing of aligned 256 KiB blocks

Quoted 3 lines

Looks good, but please either make the driver aware of multiple devices even if they can't happen currently, or alternatively error out in ->probe if of some reason it's called for a second device.

ps3flash_probe() does return -EBUSY when called for a second device.