RFC: PS3 Storage Drivers

PATCH In review [email protected] 6 participants 48 messages May 25, 2007 – May 30, 2007

Subsystems (5)

SCSI PowerPC Block layer Char and misc drivers +1 more subsystems

lore

Share & tools

lore

[email protected]

author

2007-05-25 08:37 UTC #0 lore raw

cover RFC: PS3 Storage Drivers

Hi,

This is the first submission of the new PS3 storage drivers:
[1] ps3: Preallocate bootmem memory for the PS3 FLASH ROM storage driver
[2] ps3: Extract ps3_repository_find_bus()
[3] ps3: Storage Driver Core
[4] ps3: Storage Driver Probing
[5] ps3: Disk Storage Driver
[6] ps3: ROM Storage Driver
[7] ps3: FLASH ROM Storage Driver

They are submitted purely for review, as some of the underlying infrastructure code hasn't been submitted yet (for reference, you can take a look at Geoff's git tree (git://git.kernel.org/pub/scm/linux/kernel/git/geoff/ps3-linux.git)).

Thanks for you comments!

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

[email protected]

author

2007-05-25 08:37 UTC #1 lore raw

PATCH 1/7 ps3: Preallocate bootmem memory for the PS3 FLASH ROM storage driver

Preallocate 256 KiB of bootmem memory for the PS3 FLASH ROM storage driver.

Signed-off-by Geert Uytterhoeven <[email protected]>

Signed-off-by Geoff Levand <[email protected]>

---
 arch/powerpc/platforms/ps3/setup.c |   19 ++++++++++++++++++-
 include/asm-powerpc/ps3.h          |    1 +
 2 files changed, 19 insertions(+), 1 deletion(-)

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

@@ -99,7 +99,8 @@ static void ps3_panic(char *str)
 	while(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)
 static void prealloc(struct ps3_prealloc *p)
 {
 	if (!p->size)

@@ -115,7 +116,9 @@ static void prealloc(struct ps3_prealloc
 	printk(KERN_INFO "%s: %lu bytes at %p\n", p->name, p->size,
 	       p->address);
 }
+#endif
 
+#if defined(CONFIG_FB_PS3) || defined(CONFIG_FB_PS3_MODULE)
 struct ps3_prealloc ps3fb_videomemory = {
 	.name = "ps3fb videomemory",
 	.size = CONFIG_FB_PS3_DEFAULT_SIZE_M*1024*1024,

@@ -138,6 +141,18 @@ early_param("ps3fb", early_parse_ps3fb);
 #define prealloc_ps3fb_videomemory()	do { } while (0)
 #endif
 
+#if defined(CONFIG_PS3_FLASH) || defined(CONFIG_PS3_FLASH_MODULE)
+struct ps3_prealloc ps3flash_bounce_buffer = {
+	.name = "ps3flash bounce buffer",
+	.size = 256*1024,
+	.align = 256*1024
+};
+EXPORT_SYMBOL_GPL(ps3flash_bounce_buffer);
+#define prealloc_ps3flash_bounce_buffer()	prealloc(&ps3flash_bounce_buffer)
+#else
+#define prealloc_ps3flash_bounce_buffer()	do { } while (0)
+#endif
+
 static int ps3_set_dabr(u64 dabr)
 {
 	enum {DABR_USER = 1, DABR_KERNEL = 2,};

@@ -167,6 +182,8 @@ static void __init ps3_setup_arch(void)
 #endif
 
 	prealloc_ps3fb_videomemory();
+	prealloc_ps3flash_bounce_buffer();
+
 	ppc_md.power_save = ps3_power_save;
 
 	DBG(" <- %s:%d\n", __func__, __LINE__);

include/asm-powerpc/ps3.h +1 -0

@@ -442,6 +442,7 @@ struct ps3_prealloc {
 };
 
 extern struct ps3_prealloc ps3fb_videomemory;
+extern struct ps3_prealloc ps3flash_bounce_buffer;
 
 
 #endif

Signature · 8 lines

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

[email protected]

author

2007-05-25 08:37 UTC #6 lore raw

PATCH 2/7 ps3: Extract ps3_repository_find_bus()

In reply to [email protected]

Repository updates:
- Extract ps3_repository_find_bus() from ps3_repository_find_device(), as the storage driver needs it
- Make ps3_repository_find_device() return -ENODEV if a device is not found, just like if a bus is not found

Signed-off-by Geert Uytterhoeven <[email protected]>

Signed-off-by Geoff Levand <[email protected]>

---
 arch/powerpc/platforms/ps3/platform.h   |    2 +
 arch/powerpc/platforms/ps3/repository.c |   50 ++++++++++++++++++++------------
 2 files changed, 34 insertions(+), 18 deletions(-)

arch/powerpc/platforms/ps3/platform.h +2 -0

@@ -134,6 +134,8 @@ struct ps3_repository_device {
 	struct ps3_device_id did;
 };
 
+int ps3_repository_find_bus(enum ps3_bus_type bus_type, unsigned int from,
+	unsigned int *bus_index);
 int ps3_repository_find_device(enum ps3_bus_type bus_type,
 	enum ps3_dev_type dev_type,
 	const struct ps3_repository_device *start_dev,

arch/powerpc/platforms/ps3/repository.c +32 ~~-18~~

70 lines not on this page · raw message

Signature · 8 lines

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

[email protected]

author

2007-05-25 08:37 UTC #7 lore raw

PATCH 3/7 ps3: Storage Driver Core

In reply to [email protected]

Add storage driver core support for the PS3.
PS3 storage devices are a special kind of PS3 system bus devices.

Signed-off-by Geert Uytterhoeven <[email protected]>

---
 arch/powerpc/platforms/ps3/Kconfig    |    4 
 arch/powerpc/platforms/ps3/platform.h |    1 
 drivers/ps3/Makefile                  |    1 
 drivers/ps3/ps3stor_lib.c             |  333 ++++++++++++++++++++++++++++++++++
 include/asm-powerpc/ps3stor.h         |   72 +++++++
 5 files changed, 411 insertions(+)

arch/powerpc/platforms/ps3/Kconfig +4 -0

@@ -97,4 +97,8 @@ config PS3_SYS_MANAGER
 	  This support is required for system control.  In
 	  general, all users will say Y or M.
 
+config PS3_STORAGE
+	depends on PPC_PS3
+	tristate
+
 endmenu

arch/powerpc/platforms/ps3/platform.h +1 -0

@@ -82,6 +82,7 @@ enum ps3_dev_type {
 	PS3_DEV_TYPE_STOR_ROM = TYPE_ROM,	/* 5 */
 	PS3_DEV_TYPE_SB_GPIO = 6,
 	PS3_DEV_TYPE_STOR_FLASH = TYPE_RBC,	/* 14 */
+	PS3_DEV_TYPE_NONE = 255,
 };
 
 int ps3_repository_read_bus_str(unsigned int bus_index, const char *bus_str,

drivers/ps3/Makefile +1 -0

@@ -3,3 +3,4 @@ obj-$(CONFIG_PS3_PS3AV) += ps3av_mod.o
 ps3av_mod-objs		+= ps3av.o ps3av_cmd.o
 obj-$(CONFIG_PPC_PS3) += sys-manager-core.o
 obj-$(CONFIG_PS3_SYS_MANAGER) += sys-manager.o
+obj-$(CONFIG_PS3_STORAGE) += ps3stor_lib.o

drivers/ps3/ps3stor_lib.c new file +333 -0

333 lines not on this page · raw message

include/asm-powerpc/ps3stor.h new file +72 -0

72 lines not on this page · raw message

Signature · 8 lines

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

[email protected]

author

2007-05-25 08:37 UTC #8 lore raw

PATCH 4/7 ps3: Storage Driver Probing

In reply to [email protected]

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

Signed-off-by Geert Uytterhoeven <[email protected]>

---
 arch/powerpc/platforms/ps3/device-init.c |  344 +++++++++++++++++++++++++++++++
 1 files changed, 344 insertions(+)

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

@@ -28,6 +28,7 @@
 
 #include <asm/firmware.h>
 #include <asm/lv1call.h>
+#include <asm/ps3stor.h>
 
 #include "platform.h"

@@ -508,6 +509,348 @@ static int __devinit ps3_register_av(voi
 	return result;
 }
 
+#ifdef DEBUG
+static const char *ps3stor_dev_type(enum ps3_dev_type dev_type)
+{
+	switch (dev_type) {
+	case PS3_DEV_TYPE_STOR_DISK:
+		return "disk";
+
+	case PS3_DEV_TYPE_STOR_ROM:
+		return "rom";
+
+	case PS3_DEV_TYPE_STOR_FLASH:
+		return "flash";
+
+	case PS3_DEV_TYPE_NONE:
+		return "not present";
+
+	default:
+		return "unknown";
+	}
+}
+#else
+static inline const char *ps3stor_dev_type(enum ps3_dev_type dev_type)
+{
+    return NULL;
+}
+#endif /* DEBUG */
+
+#define NOTIFICATION_DEVID	((u64)(-1L))
+#define NOTIFICATION_TIMEOUT	HZ
+
+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,
+						     &status);
+		if (!res)
+			break;
+		set_current_state(TASK_INTERRUPTIBLE);
+		schedule_timeout(1);
+	}
+	if (res)
+		pr_debug("%s:%u: check_async_status returns %ld status %lx\n",
+			 __func__, __LINE__, res, status);
+
+	return res;
+}
+
+static int ps3stor_probe_notification(struct ps3_storage_device *dev,
+				      enum ps3_dev_type dev_type)
+{
+	int error = -ENODEV, res;
+	u64 *buf;
+	u64 lpar;
+
+	pr_info("%s:%u: Requesting notification\n", __func__, __LINE__);
+
+	buf = kzalloc(512, GFP_KERNEL);
+	if (!buf)
+		return -ENOMEM;
+
+	lpar = ps3_mm_phys_to_lpar(__pa(buf));
+
+	/* 2-1) open special event device */
+	res = lv1_open_device(dev->sbd.did.bus_id, NOTIFICATION_DEVID, 0);
+	if (res) {
+		printk(KERN_ERR "%s:%u: open notification device failed %d\n",
+		       __func__, __LINE__, res);
+		goto fail_free;
+	}
+
+	/* 2-2) write info to request notify */
+	buf[0] = 0;
+	buf[1] = (1 << 1); /* region update info only */
+	res = lv1_storage_write(NOTIFICATION_DEVID, 0, 0, 1, 0, lpar,
+				&dev->tag);
+	if (res) {
+		printk(KERN_ERR "%s:%u: notify request write failed %d\n",
+		       __func__, __LINE__, res);
+		goto fail_close;
+	}
+
+	/* wait for completion in one second */
+	res = ps3stor_wait_for_completion(NOTIFICATION_DEVID, dev->tag,
+					  NOTIFICATION_TIMEOUT);
+	if (res) {
+		/* write not completed */
+		printk(KERN_ERR "%s:%u: write not completed %d\n", __func__,
+		       __LINE__, res);
+		goto fail_close;
+	}
+
+	/* 2-3) read to wait region notification for each device */
+	while (1) {
+		memset(buf, 0, 512);
+		lv1_storage_read(NOTIFICATION_DEVID, 0, 0, 1, 0, lpar,
+				 &dev->tag);
+		res = ps3stor_wait_for_completion(NOTIFICATION_DEVID, dev->tag,
+						  NOTIFICATION_TIMEOUT);
+		if (res) {
+			/* read not completed */
+			printk(KERN_ERR "%s:%u: read not completed %d\n",
+			       __func__, __LINE__, res);
+			break;
+		}
+
+		/* 2-4) verify the notification */
+		if (buf[0] != 1 || buf[1] != dev->sbd.did.bus_id) {
+			/* other info notified */
+			pr_debug("%s:%u: notification info %ld dev=%lx type=%lx\n",
+				 __func__, __LINE__, buf[0], buf[2], buf[3]);
+			break;
+		}
+
+		if (buf[2] == dev->sbd.did.dev_id && buf[3] == dev_type) {
+			pr_debug("%s:%u: device ready\n", __func__, __LINE__);
+			error = 0;
+			break;
+		}
+	}
+
+fail_close:
+	lv1_close_device(dev->sbd.did.bus_id, NOTIFICATION_DEVID);
+
+fail_free:
+	kfree(buf);
+	return error;
+}
+
+static int ps3stor_probe_dev(struct ps3_repository_device *repo)
+{
+	int error;
+	u64 port, blk_size, num_blocks;
+	unsigned int num_regions, i;
+	struct ps3_storage_device *dev;
+	enum ps3_dev_type dev_type;
+	unsigned int match_id;
+
+	pr_info("%s:%u: Probing new storage device %u\n", __func__, __LINE__,
+		 repo->dev_index);
+
+	error = ps3_repository_read_dev_id(repo->bus_index, repo->dev_index,
+					   &repo->did.dev_id);
+	if (error) {
+		printk(KERN_ERR "%s:%u: read_dev_id failed %d\n", __func__,
+		       __LINE__, error);
+		return -ENODEV;
+	}
+
+	error = ps3_repository_read_dev_type(repo->bus_index, repo->dev_index,
+					     &dev_type);
+	if (error) {
+		printk(KERN_ERR "%s:%u: read_dev_type failed %d\n", __func__,
+		       __LINE__, error);
+		return -ENODEV;
+	}
+
+	pr_debug("%s:%u: index %u:%u: id %u:%u dev_type %u (%s)\n", __func__,
+		 __LINE__, repo->bus_index, repo->dev_index, repo->did.bus_id,
+		 repo->did.dev_id, dev_type, ps3stor_dev_type(dev_type));
+
+	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;
+
+	default:
+		return 0;
+	}
+
+	error = ps3_repository_read_stor_dev_info(repo->bus_index,
+						  repo->dev_index, &port,
+						  &blk_size, &num_blocks,
+						  &num_regions);
+	if (error) {
+		printk(KERN_ERR "%s:%u: _read_stor_dev_info failed %d\n",
+		       __func__, __LINE__, error);
+		return -ENODEV;
+	}
+	pr_debug("%s:%u: index %u:%u: port %lu blk_size %lu num_blocks %lu "
+		 "num_regions %u\n",
+		 __func__, __LINE__, repo->bus_index, repo->dev_index, port,
+		 blk_size, num_blocks, num_regions);
+
+	dev = kzalloc(sizeof(struct ps3_storage_device)+
+		      num_regions*sizeof(struct ps3_storage_region),
+		      GFP_KERNEL);
+	if (!dev)
+		return -ENOMEM;
+
+	dev->sbd.did = repo->did;
+	ps3_system_bus_device_init(&dev->sbd, match_id, &dev->dma_region,
+				   NULL);
+	dev->blk_size = blk_size;
+	dev->num_regions = num_regions;
+
+	error = ps3_repository_find_interrupt(repo,
+					      PS3_INTERRUPT_TYPE_EVENT_PORT,
+					      &dev->sbd.interrupt_id);
+	if (error) {
+		printk(KERN_ERR "%s:%u: find_interrupt failed %d\n", __func__,
+			__LINE__, error);
+		goto cleanup;
+	}
+
+	/* FIXME Do we really need this? I guess for kboot only? */
+	error = ps3stor_probe_notification(dev, dev_type);
+	if (error) {
+		printk(KERN_ERR "%s:%u: probe_notification failed %d\n",
+		       __func__, __LINE__, error);
+		goto cleanup;
+	}
+
+	for (i = 0; i < num_regions; i++) {
+		unsigned int id;
+		u64 start, size;
+
+		error = ps3_repository_read_stor_dev_region(repo->bus_index,
+							    repo->dev_index, i,
+							    &id, &start,
+							    &size);
+		if (error) {
+			printk(KERN_ERR
+			       "%s:%u: read_stor_dev_region failed %d\n",
+			       __func__, __LINE__, error);
+			goto cleanup;
+		}
+		pr_debug("%s:%u: region %u: id %u start %lu size %lu\n",
+			 __func__, __LINE__, i, id, start, size);
+
+		dev->regions[i].id = id;
+		dev->regions[i].start = start;
+		dev->regions[i].size = size;
+	}
+
+	error = ps3_system_bus_device_register(&dev->sbd, PS3_IOBUS_SB);
+	if (error) {
+		printk(KERN_ERR
+		       "%s:%u: ps3_system_bus_device_register failed %d\n",
+		       __func__, __LINE__, error);
+		goto cleanup;
+	}
+	return 0;
+
+cleanup:
+	kfree(dev);
+	return -ENODEV;
+}
+
+static int ps3stor_thread(void *data)
+{
+	struct ps3_repository_device *repo = data;
+	int error;
+	unsigned int n, ms = 250;
+
+	pr_debug("%s:%u: kthread started\n", __func__, __LINE__);
+
+	do {
+		try_to_freeze();
+
+//		pr_debug("%s:%u: Checking for new storage devices...\n",
+//			 __func__, __LINE__);
+		error = ps3_repository_read_bus_num_dev(repo->bus_index, &n);
+		if (error) {
+			printk(KERN_ERR "%s:%u: read_bus_num_dev failed %d\n",
+			       __func__, __LINE__, error);
+			break;
+		}
+
+		if (n > repo->dev_index) {
+			pr_debug("%s:%u: Found %u storage devices (%u new)\n",
+				 __func__, __LINE__, n, n - repo->dev_index);
+
+			while (repo->dev_index < n && !error) {
+				error = ps3stor_probe_dev(repo);
+				repo->dev_index++;
+			}
+
+			ms = 250;
+		}
+
+		msleep_interruptible(ms);
+		if (ms < 60000)
+			ms <<= 1;
+	} while (!kthread_should_stop());
+
+	pr_debug("%s:%u: kthread finished\n", __func__, __LINE__);
+
+	return 0;
+}
+
+static int __devinit ps3_register_storage_devices(void)
+{
+	int error;
+	static struct ps3_repository_device repo;
+	struct task_struct *task;
+
+	if (!firmware_has_feature(FW_FEATURE_PS3_LV1))
+		return -ENODEV;
+
+	error = ps3_repository_find_bus(PS3_BUS_TYPE_STORAGE, 0,
+					&repo.bus_index);
+	if (error) {
+		printk(KERN_ERR "%s: Cannot find storage bus (%d)\n", __func__,
+		       error);
+		return -ENODEV;
+	}
+	pr_debug("%s:%u: Storage bus has index %u\n", __func__, __LINE__,
+		 repo.bus_index);
+
+	error = ps3_repository_read_bus_id(repo.bus_index, &repo.did.bus_id);
+	if (error) {
+		printk(KERN_ERR "%s: read_bus_id failed %d\n", __func__,
+		       error);
+		return -ENODEV;
+	}
+
+	pr_debug("%s:%u: Storage bus has id %u\n", __func__, __LINE__,
+		 repo.did.bus_id);
+
+	task = kthread_run(ps3stor_thread, &repo, "ps3stor-probe");
+	if (IS_ERR(task)) {
+		error = PTR_ERR(task);
+		printk(KERN_ERR "%s: kthread_run failed %d\n", __func__,
+		       error);
+		return error;
+	}
+
+	return 0;
+}
+
 static int __devinit ps3_register_fb(void)
 {
 	int error;

@@ -556,6 +899,7 @@ static int __init ps3_register_known_dev
 	result = ps3_register_sys_manager();
 	result = ps3_register_sound();
 	result = ps3_register_gelic();
+	result = ps3_register_storage_devices();
 
 	pr_debug(" <- %s:%d\n", __func__, __LINE__);
 	return result;

Signature · 8 lines

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

[email protected]

author

2007-05-25 08:37 UTC #15 lore raw

PATCH 5/7 ps3: Disk Storage Driver

In reply to [email protected]

Add a Disk Storage Driver for the PS3:
- Implemented as a block device driver with a dynamic major
- Disk names (and partitions) are of the format ps3d%c(%u)
- Uses software scatter-gather with a 64 KiB bounce buffer as the hypervisor doesn't support scatter-gather

Signed-off-by Geert Uytterhoeven <[email protected]>

---
 arch/powerpc/platforms/ps3/Kconfig |   11 +
 drivers/block/Makefile             |    1 
 drivers/block/ps3disk.c            |  402 +++++++++++++++++++++++++++++++++++++
 3 files changed, 414 insertions(+)

arch/powerpc/platforms/ps3/Kconfig +11 -0

@@ -101,4 +101,15 @@ config PS3_STORAGE
 	depends on PPC_PS3
 	tristate
 
+config PS3_DISK
+	tristate "PS3 Disk Storage Driver"
+	depends on PPC_PS3 && BLOCK
+	select PS3_STORAGE
+	default y
+	help
+	  Include support for the PS3 Disk Storage.
+
+	  This support is required to access the PS3 hard disk.
+	  In general, all users will say Y or M.
+
 endmenu

drivers/block/Makefile +1 -0

@@ -29,3 +29,4 @@ obj-$(CONFIG_VIODASD)		+= viodasd.o
 obj-$(CONFIG_BLK_DEV_SX8)	+= sx8.o
 obj-$(CONFIG_BLK_DEV_UB)	+= ub.o
 
+obj-$(CONFIG_PS3_DISK)		+= ps3disk.o

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

402 lines not on this page · raw message

Signature · 8 lines

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

[email protected]

author

2007-05-25 08:37 UTC #26 lore raw

PATCH 6/7 ps3: ROM Storage Driver

In reply to [email protected]

Add a CD/DVD/BD Storage Driver for the PS3:
- Implemented as a SCSI device driver
- Uses software scatter-gather with a 64 KiB bounce buffer as the hypervisor doesn't support scatter-gather

Signed-off-by Geert Uytterhoeven <[email protected]>

---
 arch/powerpc/platforms/ps3/Kconfig |   11 
 drivers/scsi/Makefile              |    1 
 drivers/scsi/ps3rom.c              |  816 +++++++++++++++++++++++++++++++++++++
 3 files changed, 828 insertions(+)

arch/powerpc/platforms/ps3/Kconfig +11 -0

@@ -112,4 +112,15 @@ config PS3_DISK
 	  This support is required to access the PS3 hard disk.
 	  In general, all users will say Y or M.
 
+config PS3_ROM
+	tristate "PS3 ROM Storage Driver"
+	depends on PPC_PS3 && BLK_DEV_SR
+	select PS3_STORAGE
+	default y
+	help
+	  Include support for the PS3 ROM Storage.
+
+	  This support is required to access the PS3 BD/DVD/CD-ROM drive.
+	  In general, all users will say Y or M.
+
 endmenu

drivers/scsi/Makefile +1 -0

@@ -133,6 +133,7 @@ obj-$(CONFIG_SCSI_IBMVSCSI)	+= ibmvscsi/
 obj-$(CONFIG_SCSI_IBMVSCSIS)	+= ibmvscsi/
 obj-$(CONFIG_SCSI_HPTIOP)	+= hptiop.o
 obj-$(CONFIG_SCSI_STEX)		+= stex.o
+obj-$(CONFIG_PS3_ROM)		+= ps3rom.o
 
 obj-$(CONFIG_ARM)		+= arm/

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

816 lines not on this page · raw message

Signature · 8 lines

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

[email protected]

author

2007-05-25 08:37 UTC #44 lore raw

PATCH 7/7 ps3: FLASH ROM Storage Driver

In reply to [email protected]

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

Signed-off-by Geert Uytterhoeven <[email protected]>

---
 arch/powerpc/platforms/ps3/Kconfig |   12 +
 drivers/char/Makefile              |    2 
 drivers/char/ps3flash.c            |  400 +++++++++++++++++++++++++++++++++++++
 3 files changed, 414 insertions(+)

arch/powerpc/platforms/ps3/Kconfig +12 -0

@@ -123,4 +123,16 @@ config PS3_ROM
 	  This support is required to access the PS3 BD/DVD/CD-ROM drive.
 	  In general, all users will say Y or M.
 
+config PS3_FLASH
+	tristate "PS3 FLASH ROM Storage Driver"
+	depends on PPC_PS3
+	select PS3_STORAGE
+	default y
+	help
+	  Include support for the PS3 FLASH ROM Storage.
+
+	  This support is required to access the PS3 FLASH ROM, which
+	  contains the boot loader and some boot options.
+	  In general, all users will say Y or M.
+
 endmenu

drivers/char/Makefile +2 -0

@@ -104,6 +104,8 @@ obj-$(CONFIG_IPMI_HANDLER)	+= ipmi/
 obj-$(CONFIG_HANGCHECK_TIMER)	+= hangcheck-timer.o
 obj-$(CONFIG_TCG_TPM)		+= tpm/
 
+obj-$(CONFIG_PS3_FLASH)		+= ps3flash.o
+
 # Files generated that shall be removed upon make clean
 clean-files := consolemap_deftbl.c defkeymap.c

drivers/char/ps3flash.c new file +400 -0

400 lines not on this page · raw message

Signature · 8 lines

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

Olaf Hering

2007-05-25 11:23 UTC #27 lore raw

Re: [patch 6/7] ps3: ROM Storage Driver

In reply to [email protected]

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.

Olaf Hering

2007-05-25 11:45 UTC #16 lore raw

Re: [patch 5/7] ps3: Disk Storage Driver

In reply to [email protected]

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/

Arnd Bergmann

maintainer

2007-05-25 16:18 UTC #9 lore raw

Re: [patch 4/7] ps3: Storage Driver Probing

In reply to [email protected]

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

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

Hi Geert,

the driver looks pretty good, just a few details I noticed:

Quoted 4 lines

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 6 lines

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:

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

Quoted 10 lines

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

+		match_id = PS3_MATCH_ID_STOR_ROM;
+		break;
+
+	case PS3_DEV_TYPE_STOR_FLASH:
+		match_id = PS3_MATCH_ID_STOR_FLASH;
+		break;
+
+	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 4 lines

arch/powerpc/platforms/ps3/device-init.c +4 -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.

	Arnd <><

Arnd Bergmann

maintainer

2007-05-25 16:26 UTC #19 lore raw

Re: [patch 5/7] ps3: Disk Storage Driver

In reply to [email protected]

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

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 line 105

Quoted 10 lines

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

+				memcpy(dev->bounce_buf+offset, buf, size);
+			else
+				memcpy(buf, dev->bounce_buf+offset, size);
+			offset += size;
+			__bio_kunmap_atomic(bio, KM_USER0);
+		}
+		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 23 lines

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

drivers/block/ps3disk.c line 181

Quoted 10 lines

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

+			spin_unlock_irq(&priv->lock);
+			continue;
+		}
+		spin_unlock_irq(&priv->lock);
+		ps3disk_handle_request_sg(dev, req);
+	}
+
+	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.

	Arnd <><

Arnd Bergmann

maintainer

2007-05-25 16:50 UTC #32 lore raw

Re: [patch 6/7] ps3: ROM Storage Driver

In reply to [email protected]

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?

	Arnd <><

Geoff Levand

2007-05-25 17:09 UTC #10 lore raw

Re: [patch 4/7] ps3: Storage Driver Probing

In reply to Arnd Bergmann

Hi.

Quoted 13 lines · Arnd Bergmann

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

Quoted 8 lines

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.

I noticed we have some redundancy in the constants and such now that we have unified the device support. I planned to go through and try to clean up what I can.

-Geoff

Geert Uytterhoeven

author

2007-05-25 19:36 UTC #33 lore raw

Re: [patch 6/7] ps3: ROM Storage Driver

In reply to Arnd Bergmann

Quoted 9 lines · Arnd Bergmann

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?

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.

Gr{oetje,eeting}s,

Geert

Geert Uytterhoeven

author

2007-05-25 19:40 UTC #20 lore raw

Re: [patch 5/7] ps3: Disk Storage Driver

In reply to Arnd Bergmann

Quoted 28 lines · Arnd Bergmann

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

Quoted 8 lines

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.

Yes, it uses logical partion addresses, so we cannot create a virtually contiguous mapping.

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

Quoted 8 lines

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.

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().

Gr{oetje,eeting}s,

Geert

Geert Uytterhoeven

author

2007-05-25 19:43 UTC #17 lore raw

Re: [patch 5/7] ps3: Disk Storage Driver

In reply to Olaf Hering

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?

Gr{oetje,eeting}s,

Geert

Geert Uytterhoeven

author

2007-05-25 19:48 UTC #11 lore raw

Re: [patch 4/7] ps3: Storage Driver Probing

In reply to Arnd Bergmann

Quoted 17 lines · Arnd Bergmann

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?

Not really. I'll change it.

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

Quoted 8 lines

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.

The PS3_DEV_TYPE_* IDs are imposed by the repository, as created by the hypervisor.
The PS3_MATCH_ID_* IDs are created by us, for all PS3-specific devices. As Geoff already pointed out, we may be able to use PS3_DEV_TYPE_* IDs for everything, but unfortunately not all PS3-specific devices are present in the repository.

Quoted 6 lines

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.

Oops, forgot to uncomment it (it was a bit noisy while I lived with DEBUG defined ;-)

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

Gr{oetje,eeting}s,

Geert

Arnd Bergmann

maintainer

2007-05-25 20:43 UTC #21 lore raw

Re: [patch 5/7] ps3: Disk Storage Driver

In reply to Geert Uytterhoeven

Quoted 7 lines · Geert Uytterhoeven

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().

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

	Arnd <><

Olaf Hering

2007-05-25 20:47 UTC #18 lore raw

Re: [patch 5/7] ps3: Disk Storage Driver

In reply to Geert Uytterhoeven

Quoted 7 lines · Geert Uytterhoeven

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?

No, that was not obvious.
But for gelic it goes like this:

+++ b/drivers/net/gelic_net.c
@@ -1364,6 +1364,7 @@ static int gelic_net_setup_netdev(struct

        u64 v1, v2;
 
        SET_MODULE_OWNER(netdev);
+       SET_NETDEV_DEV(netdev, &card->dev->core);
        spin_lock_init(&card->tx_dma_lock);
 
        card->rx_csum = GELIC_NET_RX_CSUM_DEFAULT;

Arnd Bergmann

maintainer

2007-05-25 21:04 UTC #34 lore raw

Re: [patch 6/7] ps3: ROM Storage Driver

In reply to Geert Uytterhoeven

Quoted 6 lines · Geert Uytterhoeven

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.

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.

	Arnd <><

Geert Uytterhoeven

author

2007-05-25 21:22 UTC #22 lore raw

Re: [patch 5/7] ps3: Disk Storage Driver

In reply to Arnd Bergmann

Quoted 10 lines · Arnd Bergmann

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.

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.

Gr{oetje,eeting}s,

Geert

Benjamin Herrenschmidt

2007-05-25 22:35 UTC #2 lore raw

Re: [patch 1/7] ps3: Preallocate bootmem memory for the PS3 FLASH ROM storage driver

In reply to [email protected]

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.

Ben.

Arnd Bergmann

maintainer

2007-05-25 22:45 UTC #23 lore raw

Re: [patch 5/7] ps3: Disk Storage Driver

In reply to Geert Uytterhoeven

Quoted 11 lines · Geert Uytterhoeven

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.

Not sure how much difference it will make performance-wise, but the context switch for each bio adds some extra cost at least. Changing it means you no longer share the ps3stor_read_write_sectors, but can at the same time simplify the disk driver, so that won't hurt in total.

I don't care much, but I think it's worth trying.

	Arnd <><

Benjamin Herrenschmidt

2007-05-25 22:45 UTC #28 lore raw

Re: [patch 6/7] ps3: ROM Storage Driver

In reply to Olaf Hering

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

Ben.

Benjamin Herrenschmidt

2007-05-25 22:47 UTC #13 lore raw

Re: [patch 4/7] ps3: Storage Driver Probing

In reply to Arnd Bergmann

Quoted 9 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 8 lines

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?

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

Ben.

Benjamin Herrenschmidt

2007-05-25 22:49 UTC #25 lore raw

Re: [patch 5/7] ps3: Disk Storage Driver

In reply to Arnd Bergmann

Quoted 4 lines

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.

Ugh ? Maybe s390 can do that but no other hypervisor that I know about :-) It would be nice, sure, but heh.

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.

Yeah, me neither... the driver looks very very very unefficient to me. I though the kthread was useful for hotplug detection becasue the hypervisor don't signal us, but from the patch, it looks like it's also used for actual request processing which is very yucky.

Ben.

Benjamin Herrenschmidt

2007-05-25 22:53 UTC #24 lore raw

Re: [patch 5/7] ps3: Disk Storage Driver

In reply to Geert Uytterhoeven

Quoted 5 lines · Geert Uytterhoeven

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().

Which as I said before looks terribly sad... Why the heck would it have to do that ?

Ben.

Benjamin Herrenschmidt

2007-05-25 22:54 UTC #12 lore raw

Re: [patch 4/7] ps3: Storage Driver Probing

In reply to Geert Uytterhoeven

Quoted 9 lines · Geert Uytterhoeven

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.

Quoted 7 lines

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.

Why not just have a kthread poll at 2 second interval for new devices or removed ones ?

(And not for request processing)

Ben.

Geert Uytterhoeven

author

2007-05-26 08:51 UTC #3 lore raw

Re: [patch 1/7] ps3: Preallocate bootmem memory for the PS3 FLASH ROM storage driver

In reply to Benjamin Herrenschmidt

Quoted 11 lines · Benjamin Herrenschmidt

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?

Gr{oetje,eeting}s,

Geert

Geert Uytterhoeven

author

2007-05-26 08:52 UTC #29 lore raw

Re: [patch 6/7] ps3: ROM Storage Driver

In reply to Benjamin Herrenschmidt

Quoted 10 lines · Benjamin Herrenschmidt

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

So what should I use instead?

Gr{oetje,eeting}s,

Geert

Geert Uytterhoeven

author

2007-05-26 08:56 UTC #14 lore raw

Re: [patch 4/7] ps3: Storage Driver Probing

In reply to Benjamin Herrenschmidt

Quoted 17 lines · Benjamin Herrenschmidt

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 3 lines

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 ?

The special notification device (NOTIFICATION_DEVID = -1) is not in the repository and AFAIK it doesn't have an interrupt attached to it. Note that this is used during probing only.

Gr{oetje,eeting}s,

Geert

Benjamin Herrenschmidt

2007-05-26 22:17 UTC #4 lore raw

Re: [patch 1/7] ps3: Preallocate bootmem memory for the PS3 FLASH ROM storage driver

In reply to Geert Uytterhoeven

Quoted 6 lines · Geert Uytterhoeven

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?

kmalloc & a good prayer ? :-0

Ben.

Benjamin Herrenschmidt

2007-05-26 22:18 UTC #30 lore raw

Re: [patch 6/7] ps3: ROM Storage Driver

In reply to Geert Uytterhoeven

Quoted 6 lines · Geert Uytterhoeven

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 7 lines

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?

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.

Ben.

Arnd Bergmann

maintainer

2007-05-27 18:26 UTC #5 lore raw

Re: [patch 1/7] ps3: Preallocate bootmem memory for the PS3 FLASH ROM storage driver

In reply to Benjamin Herrenschmidt

Quoted 6 lines · Benjamin Herrenschmidt

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

s/kmalloc/get_free_pages/

kmalloc is limited to 128kb.

	Arnd <><

Christoph Hellwig

2007-05-29 09:54 UTC #45 lore raw

Re: [patch 7/7] ps3: FLASH ROM Storage Driver

In reply to [email protected]

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.

Christoph Hellwig

2007-05-29 09:55 UTC #31 lore raw

Re: [patch 6/7] ps3: ROM Storage Driver

In reply to Benjamin Herrenschmidt

Quoted 9 lines · Benjamin Herrenschmidt

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.

Generally I'd prefer to use kmap everywhere, to keep code future-proof. Similar to how I advocate using spinlocks even in drivers for UP-only architectures.

Geert Uytterhoeven

author

2007-05-29 09:58 UTC #46 lore raw

Re: [patch 7/7] ps3: FLASH ROM Storage Driver

In reply to Christoph Hellwig

Quoted 9 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
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.

Gr{oetje,eeting}s,

Geert

Christoph Hellwig

2007-05-29 10:49 UTC #36 lore raw

Re: [patch 6/7] ps3: ROM Storage Driver

In reply to [email protected]

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

arch/powerpc/platforms/ps3/Kconfig config PS3_DISK · line 119

Quoted 5 lines

arch/powerpc/platforms/ps3/Kconfig +5 -0

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

Quoted 17 lines

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 line 70

Quoted 10 lines

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

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

drivers/scsi/ps3rom.c line 93

Quoted 8 lines

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

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

drivers/scsi/ps3rom.c line 218

Quoted 10 lines

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

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

Quoted 9 lines

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 line 240

Quoted 10 lines

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

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

Quoted 7 lines

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 line 258

Quoted 10 lines

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

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

drivers/scsi/ps3rom.c line 262

Quoted 4 lines

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

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

Review

Quoted 2 lines

(quoted diff) +2 -0

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

Quoted 20 lines

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 line 294

Quoted 10 lines

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

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

Quoted 10 lines

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 line 314

Quoted 10 lines

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

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

Quoted 15 lines

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 line 603

Quoted 10 lines

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

+	case REQUEST_SENSE:
+		ps3rom_atapi_request(dev, cmd, srb6_len(cmd),
+				     PIO_DATA_IN_PROTO, DIR_READ, 0);
+		break;
+
+	case ALLOW_MEDIUM_REMOVAL:
+	case START_STOP:
+	case TEST_UNIT_READY:
+		ps3rom_atapi_request(dev, cmd, 0, NON_DATA_PROTO, DIR_NA, 1);
+		break;

This switch statement looks very wrong. The data direction and length can easily be derived from the scsi_cmnd structure.

drivers/scsi/ps3rom.c line 645

Quoted 7 lines

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

+	default:
+		dev_err(&dev->sbd.core, "%s:%u: illegal request 0x%02x (%s)\n",
+			__func__, __LINE__, opcode, scsi_command(opcode));
+		cmd->result = DID_ERROR << 16;
+		memset(cmd->sense_buffer, 0, SCSI_SENSE_BUFFERSIZE);
+		cmd->sense_buffer[0] = 0x70;
+		cmd->sense_buffer[2] = ILLEGAL_REQUEST;

Normally you should just hand down any command to the device, that's the whole point of the modular scsi protocol stack.

Quoted 6 lines

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

drivers/scsi/ps3rom.c line 715

Quoted 10 lines

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

+	if (dev->blk_size != CD_FRAMESIZE) {
+		dev_err(&dev->sbd.core,
+			"%s:%u: cannot handle block size %lu\n", __func__,
+			__LINE__, dev->blk_size);
+		return -EINVAL;
+	}
+
+	priv = kzalloc(sizeof(*priv), GFP_KERNEL);
+	if (!priv)
+		return -ENOMEM;

Normally you should allocate the private data using scsi_host_alloc, that's why it has a priv_size argument.

drivers/scsi/ps3rom.c line 787

Quoted 11 lines

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

+static int ps3rom_remove(struct ps3_system_bus_device *_dev)
+{
+	struct ps3_storage_device *dev = to_ps3_storage_device(&_dev->core);
+	struct ps3rom_private *priv = ps3rom_priv(dev);
+
+	scsi_remove_host(priv->host);
+	scsi_host_put(priv->host);
+	kthread_stop(priv->thread);
+	ps3stor_teardown(dev);
+	kfree(dev->bounce_buf);
+	kfree(priv);

the scsi_host_put should come last.

Christoph Hellwig

2007-05-29 10:51 UTC #35 lore raw

Re: [patch 6/7] ps3: ROM Storage Driver

In reply to Arnd Bergmann

Quoted 5 lines · Arnd Bergmann

Quoted 4 lines · Geert Uytterhoeven
Quoted 3 lines
What is the problem? Is there infrastructure missing in the CD-ROM layer?

Quoted 7 lines

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.

ps3rom is just a normal scsi low level driver. Currently the only attached devices or MMC devices so the sr driver attaches to it, but if you want to hack the ps3 hardware you should be able to attach a tape or disk aswell, and linux would work (lv1 and gameos might of course not like this)

Christoph Hellwig

2007-05-29 10:51 UTC #47 lore raw

Re: [patch 7/7] ps3: FLASH ROM Storage Driver

In reply to Geert Uytterhoeven

Quoted 7 lines · Geert Uytterhoeven

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 5 lines

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.

You're right, I missed that bit.

Geert Uytterhoeven

author

2007-05-29 11:11 UTC #37 lore raw

Re: [patch 6/7] ps3: ROM Storage Driver

In reply to Christoph Hellwig

Quoted 1 line · Christoph Hellwig

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

I'll Cc linux-scsi next time.

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

Quoted 8 lines

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.

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

Benjamin Herrenschmidt

2007-05-29 11:32 UTC #38 lore raw

Re: [patch 6/7] ps3: ROM Storage Driver

Quoted 7 lines · Geert Uytterhoeven

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.

And the joke is that not only the HW can do scatter & gather but you also have an iommu ...

Ben.

Geert Uytterhoeven

author

2007-05-29 16:21 UTC #42 lore raw

Re: [patch 6/7] ps3: ROM Storage Driver

In reply to Christoph Hellwig

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?

Gr{oetje,eeting}s,

Geert

Christoph Hellwig

2007-05-30 10:01 UTC #43 lore raw

Re: [patch 6/7] ps3: ROM Storage Driver

Quoted 31 lines · Geert Uytterhoeven

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.

Quoted 1 line

Just to be sure: all four if-cases or only the last one?

That's just in reference to the last one. The checks above could be condensed a little more aswell, but I'll comment on further in the second round of review, in the hope that the command submission path is a lot more streamline by then already.

Christoph Hellwig

2007-05-30 10:13 UTC #39 lore raw

Re: [patch 6/7] ps3: ROM Storage Driver

In reply to Geert Uytterhoeven

Quoted 5 lines · Geert Uytterhoeven

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.

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

Benjamin Herrenschmidt

2007-05-30 11:46 UTC #40 lore raw

Re: [patch 6/7] ps3: ROM Storage Driver

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.

Ben.

Geoff Levand

2007-05-30 17:37 UTC #41 lore raw

Re: [patch 6/7] ps3: ROM Storage Driver

Quoted 7 lines · Benjamin Herrenschmidt

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.

Yes, if only to put an end to seeing this kind of comment over and over again.

-Geoff