[patch 0/7] RFC: PS3 Storage Drivers

[patch 0/7] RFC: PS3 Storage Drivers

[Geert.Uytterhoeven]

	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)
Geert.Uytterhoeven@sonycom.com ------- The Corporate Village, Da Vincilaan 7-D1
Voice +32-2-7008453 Fax +32-2-7008622 ---------------- B-1935 Zaventem, Belgium

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

[Geert.Uytterhoeven]

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

Signed-off-by: Geert Uytterhoeven <Geert.Uytterhoeven@sonycom.com>
Signed-off-by: Geoff Levand <geoffrey.levand@am.sony.com>
---
 arch/powerpc/platforms/ps3/setup.c |   19 ++++++++++++++++++-
 include/asm-powerpc/ps3.h          |    1 +
 2 files changed, 19 insertions(+), 1 deletion(-)

--- a/arch/powerpc/platforms/ps3/setup.c
+++ b/arch/powerpc/platforms/ps3/setup.c
@@ -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__);
--- a/include/asm-powerpc/ps3.h
+++ b/include/asm-powerpc/ps3.h
@@ -442,6 +442,7 @@ struct ps3_prealloc {
 };
 
 extern struct ps3_prealloc ps3fb_videomemory;
+extern struct ps3_prealloc ps3flash_bounce_buffer;
 
 
 #endif

-- 
Gr{oetje,eeting}s,

						Geert

--
Geert Uytterhoeven -- Sony Network and Software Technology Center Europe (NSCE)
Geert.Uytterhoeven@sonycom.com ------- The Corporate Village, Da Vincilaan 7-D1
Voice +32-2-7008453 Fax +32-2-7008622 ---------------- B-1935 Zaventem, Belgium

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

[Benjamin Herrenschmidt]

On Fri, 2007-05-25 at 10:36 +0200, Geert.Uytterhoeven@sonycom.com wrote:
> -#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.

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

[Geert Uytterhoeven]

On Sat, 26 May 2007, Benjamin Herrenschmidt wrote:
> On Fri, 2007-05-25 at 10:36 +0200, Geert.Uytterhoeven@sonycom.com wrote:
> > -#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 -- Sony Network and Software Technology Center Europe (NSCE)
Geert.Uytterhoeven@sonycom.com ------- The Corporate Village, Da Vincilaan 7-D1
Voice +32-2-7008453 Fax +32-2-7008622 ---------------- B-1935 Zaventem, Belgium

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

[Benjamin Herrenschmidt]

On Sat, 2007-05-26 at 10:51 +0200, Geert Uytterhoeven wrote:
> 
> 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.

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

[Arnd Bergmann]

On Sunday 27 May 2007, Benjamin Herrenschmidt wrote:
> On Sat, 2007-05-26 at 10:51 +0200, Geert Uytterhoeven wrote:
> > 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 <><

[patch 2/7] ps3: Extract ps3_repository_find_bus()

[Geert.Uytterhoeven]

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 <Geert.Uytterhoeven@sonycom.com>
Signed-off-by: Geoff Levand <geoffrey.levand@am.sony.com>
---
 arch/powerpc/platforms/ps3/platform.h   |    2 +
 arch/powerpc/platforms/ps3/repository.c |   50 ++++++++++++++++++++------------
 2 files changed, 34 insertions(+), 18 deletions(-)

--- a/arch/powerpc/platforms/ps3/platform.h
+++ b/arch/powerpc/platforms/ps3/platform.h
@@ -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,
--- a/arch/powerpc/platforms/ps3/repository.c
+++ b/arch/powerpc/platforms/ps3/repository.c
@@ -513,6 +513,31 @@ int ps3_repository_dump_bus_info(void)
 }
 #endif /* defined(DEBUG) */
 
+int ps3_repository_find_bus(enum ps3_bus_type bus_type, unsigned int from,
+	unsigned int *bus_index)
+{
+	unsigned int i;
+	enum ps3_bus_type type;
+	int error;
+
+	for (i = from; i < 10; i++) {
+		error = ps3_repository_read_bus_type(i, &type);
+		if (error) {
+			pr_debug("%s:%d read_bus_type failed\n",
+				__func__, __LINE__);
+			*bus_index = UINT_MAX;
+			return error;
+		}
+		if (type == bus_type) {
+			*bus_index = i;
+			return 0;
+		}
+	}
+	*bus_index = UINT_MAX;
+	return -ENODEV;
+}
+EXPORT_SYMBOL_GPL(ps3_repository_find_bus);
+
 static int find_device(unsigned int bus_index, unsigned int num_dev,
 	unsigned int start_dev_index, enum ps3_dev_type dev_type,
 	struct ps3_repository_device *dev)
@@ -541,7 +566,7 @@ static int find_device(unsigned int bus_
 	}
 
 	if (dev_index == num_dev)
-		return -1;
+		return -ENODEV;
 
 	pr_debug("%s:%d: found dev_type %u at dev_index %u\n",
 		__func__, __LINE__, dev_type, dev_index);
@@ -577,25 +602,14 @@ int ps3_repository_find_device (enum ps3
 
 	BUG_ON(start_dev && start_dev->bus_index > 10);
 
-	for (bus_index = start_dev ? start_dev->bus_index : 0; bus_index < 10;
-		bus_index++) {
-		enum ps3_bus_type x;
-
-		result = ps3_repository_read_bus_type(bus_index, &x);
-
-		if (result) {
-			pr_debug("%s:%d read_bus_type failed\n",
-				__func__, __LINE__);
-			dev->bus_index = UINT_MAX;
-			return result;
-		}
-		if (x == bus_type)
-			break;
+	result = ps3_repository_find_bus(bus_type,
+					 start_dev ? start_dev->bus_index : 0,
+					 &bus_index);
+	if (result) {
+		dev->bus_index = UINT_MAX;
+		return result;
 	}
 
-	if (bus_index >= 10)
-		return -ENODEV;
-
 	pr_debug("%s:%d: found bus_type %u at bus_index %u\n",
 		__func__, __LINE__, bus_type, bus_index);
 

-- 
Gr{oetje,eeting}s,

						Geert

--
Geert Uytterhoeven -- Sony Network and Software Technology Center Europe (NSCE)
Geert.Uytterhoeven@sonycom.com ------- The Corporate Village, Da Vincilaan 7-D1
Voice +32-2-7008453 Fax +32-2-7008622 ---------------- B-1935 Zaventem, Belgium

[patch 3/7] ps3: Storage Driver Core

[Geert.Uytterhoeven]

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 <Geert.Uytterhoeven@sonycom.com>
---
 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(+)

--- a/arch/powerpc/platforms/ps3/Kconfig
+++ b/arch/powerpc/platforms/ps3/Kconfig
@@ -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
--- a/arch/powerpc/platforms/ps3/platform.h
+++ b/arch/powerpc/platforms/ps3/platform.h
@@ -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,
--- a/drivers/ps3/Makefile
+++ b/drivers/ps3/Makefile
@@ -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
--- /dev/null
+++ b/drivers/ps3/ps3stor_lib.c
@@ -0,0 +1,333 @@
+/*
+ * PS3 Storage Library
+ *
+ * Copyright (C) 2007 Sony Computer Entertainment Inc.
+ * Copyright 2007 Sony Corp.
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License as published
+ * by the Free Software Foundation; version 2 of the License.
+ *
+ * This program is distributed in the hope that it will be useful, but
+ * WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ * General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License along
+ * with this program; if not, write to the Free Software Foundation, Inc.,
+ * 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA.
+ */
+
+#include <linux/dma-mapping.h>
+#include <linux/interrupt.h>
+
+#include <asm/lv1call.h>
+#include <asm/ps3stor.h>
+
+
+static irqreturn_t ps3stor_interrupt(int irq, void *data)
+{
+	struct ps3_storage_device *dev = data;
+
+	dev->lv1_res = lv1_storage_get_async_status(dev->sbd.did.dev_id,
+						    &dev->lv1_tag,
+						    &dev->lv1_status);
+	/*
+	 * lv1_status = -1 may mean that ATAPI transport completed OK, but
+	 * ATAPI command itself resulted CHECK CONDITION
+	 * so, upper layer should issue REQUEST_SENSE to check the sense data
+	 */
+
+	if (dev->lv1_tag != dev->tag)
+		dev_err(&dev->sbd.core,
+			"%s:%u: tag mismatch, got %lx, expected %lx\n",
+			__func__, __LINE__, dev->lv1_tag, dev->tag);
+	if (dev->lv1_res)
+		dev_err(&dev->sbd.core, "%s:%u: res=%d status=0x%lx\n",
+			__func__, __LINE__, dev->lv1_res, dev->lv1_status);
+	else
+		complete(&dev->irq_done);
+	return IRQ_HANDLED;
+}
+
+
+static int ps3stor_probe_access(struct ps3_storage_device *dev)
+{
+	int res, error;
+	unsigned int i;
+	unsigned long n;
+
+	if (dev->sbd.match_id == PS3_MATCH_ID_STOR_ROM) {
+		/* special case: CD-ROM is assumed always accessible */
+		dev->accessible_regions = 1;
+		return 0;
+	}
+
+	error = -EPERM;
+	for (i = 0; i < dev->num_regions; i++) {
+		dev_dbg(&dev->sbd.core,
+			"%s:%u: checking accessibility of region %u\n",
+			__func__, __LINE__, i);
+
+		dev->region_idx = i;
+		res = ps3stor_read_write_sectors(dev, dev->bounce_lpar, 0, 1,
+						 0);
+		if (res) {
+			dev_dbg(&dev->sbd.core,
+				"%s:%u: read failed, region %u is not accessible\n",
+				__func__, __LINE__, i);
+			continue;
+		}
+
+		dev_dbg(&dev->sbd.core, "%s:%u: region %u is accessible\n",
+			__func__, __LINE__, i);
+		set_bit(i, &dev->accessible_regions);
+
+		/* We can access at least one region */
+		error = 0;
+	}
+	if (error)
+		return error;
+
+	n = hweight_long(dev->accessible_regions);
+	if (n > 1)
+		dev_info(&dev->sbd.core,
+			 "%s:%u: %lu accessible regions found. Only the first "
+			 "one will be used",
+			 __func__, __LINE__, n);
+	dev->region_idx = __ffs(dev->accessible_regions);
+	dev_dbg(&dev->sbd.core,
+		"First accessible region has index %u start %lu size %lu\n",
+		dev->region_idx, dev->regions[dev->region_idx].start,
+		dev->regions[dev->region_idx].size);
+
+	return 0;
+}
+
+
+/**
+ *	ps3stor_setup - Setup a storage device before use
+ *	@dev: Pointer to a struct ps3_storage_device
+ *	@name: Name of the storage driver
+ *
+ *	Returns 0 for success, or an error code
+ */
+int ps3stor_setup(struct ps3_storage_device *dev, const char *name)
+{
+	int error, res, alignment;
+	enum ps3_dma_page_size page_size;
+
+	error = ps3_open_hv_device(&dev->sbd);
+	if (error) {
+		dev_err(&dev->sbd.core,
+			"%s:%u: ps3_open_hv_device failed %d\n", __func__,
+			__LINE__, error);
+		goto fail;
+	}
+
+	error = ps3_sb_event_receive_port_setup(PS3_BINDING_CPU_ANY,
+						&dev->sbd.did,
+						dev->sbd.interrupt_id,
+						&dev->irq);
+	if (error) {
+		dev_err(&dev->sbd.core,
+			"%s:%u: ps3_sb_event_receive_port_setup failed %d\n",
+		       __func__, __LINE__, error);
+		goto fail_close_device;
+	}
+
+	error = request_irq(dev->irq, ps3stor_interrupt, IRQF_DISABLED, name,
+			    dev);
+	if (error) {
+		dev_err(&dev->sbd.core, "%s:%u: request_irq failed %d\n",
+			__func__, __LINE__, error);
+		goto fail_sb_event_receive_port_destroy;
+	}
+
+	alignment = min(__ffs(dev->bounce_size),
+			__ffs((unsigned long)dev->bounce_buf));
+	if (alignment < 12) {
+		dev_err(&dev->sbd.core,
+			"%s:%u: bounce buffer not aligned (%lx at 0x%p)\n",
+			__func__, __LINE__, dev->bounce_size, dev->bounce_buf);
+		error = -EINVAL;
+		goto fail_free_irq;
+	} else if (alignment < 16)
+		page_size = PS3_DMA_4K;
+	else
+		page_size = PS3_DMA_64K;
+	dev->sbd.d_region = &dev->dma_region;
+	ps3_dma_region_init(&dev->dma_region, &dev->sbd.did, page_size,
+			    PS3_DMA_OTHER, dev->bounce_buf, dev->bounce_size,
+			    PS3_IOBUS_SB);
+	res = ps3_dma_region_create(&dev->dma_region);
+	if (res) {
+		dev_err(&dev->sbd.core, "%s:%u: cannot create DMA region\n",
+			__func__, __LINE__);
+		error = -ENOMEM;
+		goto fail_free_irq;
+	}
+
+	dev->bounce_lpar = ps3_mm_phys_to_lpar(__pa(dev->bounce_buf));
+	dev->bounce_dma = dma_map_single(&dev->sbd.core, dev->bounce_buf,
+					 dev->bounce_size, DMA_BIDIRECTIONAL);
+	if (!dev->bounce_dma) {
+		dev_err(&dev->sbd.core, "%s:%u: map DMA region failed\n",
+			__func__, __LINE__);
+		error = -ENODEV;
+		goto fail_free_dma;
+	}
+
+	error = ps3stor_probe_access(dev);
+	if (error) {
+		dev_err(&dev->sbd.core, "%s:%u: No accessible regions found\n",
+			__func__, __LINE__);
+		goto fail_unmap_dma;
+	}
+	return 0;
+
+fail_unmap_dma:
+	dma_unmap_single(&dev->sbd.core, dev->bounce_dma, dev->bounce_size,
+			 DMA_BIDIRECTIONAL);
+fail_free_dma:
+	ps3_dma_region_free(&dev->dma_region);
+fail_free_irq:
+	free_irq(dev->irq, dev);
+fail_sb_event_receive_port_destroy:
+	ps3_sb_event_receive_port_destroy(&dev->sbd.did, dev->sbd.interrupt_id,
+					  dev->irq);
+fail_close_device:
+	ps3_close_hv_device(&dev->sbd);
+fail:
+	return error;
+}
+EXPORT_SYMBOL_GPL(ps3stor_setup);
+
+
+/**
+ *	ps3stor_teardown - Tear down a storage device after use
+ *	@dev: Pointer to a struct ps3_storage_device
+ */
+void ps3stor_teardown(struct ps3_storage_device *dev)
+{
+	int error;
+
+	dma_unmap_single(&dev->sbd.core, dev->bounce_dma, dev->bounce_size,
+			 DMA_BIDIRECTIONAL);
+	ps3_dma_region_free(&dev->dma_region);
+
+	free_irq(dev->irq, dev);
+
+	error = ps3_sb_event_receive_port_destroy(&dev->sbd.did,
+						  dev->sbd.interrupt_id,
+						  dev->irq);
+	if (error)
+		dev_err(&dev->sbd.core,
+			"%s:%u: destroy event receive port failed %d\n",
+			__func__, __LINE__, error);
+
+	error = ps3_close_hv_device(&dev->sbd);
+	if (error)
+		dev_err(&dev->sbd.core,
+			"%s:%u: ps3_close_hv_device failed %d\n", __func__,
+			__LINE__, error);
+}
+EXPORT_SYMBOL_GPL(ps3stor_teardown);
+
+
+/**
+ *	ps3stor_read_write_sectors - read/write from/to a storage device
+ *	@dev: Pointer to a struct ps3_storage_device
+ *	@lpar: HV logical partition address
+ *	@start_sector: First sector to read/write
+ *	@sectors: Number of sectors to read/write
+ *	@write: Flag indicating write (non-zero) or read (zero)
+ *
+ *	Returns 0 for success, -1 in case of failure to submit the command, or
+ *	an LV1 status value in case of other errors
+ */
+u64 ps3stor_read_write_sectors(struct ps3_storage_device *dev, u64 lpar,
+			       u64 start_sector, u64 sectors, int write)
+{
+	unsigned int region_id = dev->regions[dev->region_idx].id;
+	const char *op = write ? "write" : "read";
+	int res;
+
+	dev_dbg(&dev->sbd.core, "%s:%u: %s %lu sectors starting at %lu\n",
+		__func__, __LINE__, op, sectors, start_sector);
+
+	init_completion(&dev->irq_done);
+	res = write ? lv1_storage_write(dev->sbd.did.dev_id, region_id,
+					start_sector, sectors, 0, lpar,
+					&dev->tag)
+		    : lv1_storage_read(dev->sbd.did.dev_id, region_id,
+				       start_sector, sectors, 0, lpar,
+				       &dev->tag);
+	if (res) {
+		dev_dbg(&dev->sbd.core, "%s:%u: %s failed %d\n", __func__,
+			__LINE__, op, res);
+		return -1;
+	}
+
+	wait_for_completion(&dev->irq_done);
+	if (dev->lv1_status) {
+		dev_dbg(&dev->sbd.core, "%s:%u: %s failed 0x%lx\n", __func__,
+			__LINE__, op, dev->lv1_status);
+		return dev->lv1_status;
+	}
+
+	dev_dbg(&dev->sbd.core, "%s:%u: %s completed\n", __func__, __LINE__,
+		op);
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(ps3stor_read_write_sectors);
+
+
+/**
+ *	ps3stor_send_command - send a device command to a storage device
+ *	@dev: Pointer to a struct ps3_storage_device
+ *	@cmd: Command number
+ *	@arg1: First command argument
+ *	@arg2: Second command argument
+ *	@arg3: Third command argument
+ *	@arg4: Fourth command argument
+ *
+ *	Returns 0 for success, -1 in case of failure to submit the command, or
+ *	an LV1 status value in case of other errors
+ */
+u64 ps3stor_send_command(struct ps3_storage_device *dev, u64 cmd, u64 arg1,
+			 u64 arg2, u64 arg3, u64 arg4)
+{
+	int res;
+
+	dev_dbg(&dev->sbd.core, "%s:%u: send device command 0x%lx\n", __func__,
+		__LINE__, cmd);
+
+	init_completion(&dev->irq_done);
+
+	res = lv1_storage_send_device_command(dev->sbd.did.dev_id, cmd, arg1,
+					      arg2, arg3, arg4, &dev->tag);
+	if (res) {
+		dev_err(&dev->sbd.core,
+			"%s:%u: send_device_command 0x%lx failed %d\n",
+			__func__, __LINE__, cmd, res);
+		return -1;
+	}
+
+	wait_for_completion(&dev->irq_done);
+	if (dev->lv1_status)
+		dev_dbg(&dev->sbd.core, "%s:%u: command 0x%lx failed 0x%lx\n",
+			__func__, __LINE__, cmd, dev->lv1_status);
+	else
+		dev_dbg(&dev->sbd.core, "%s:%u: command 0x%lx completed\n",
+			__func__, __LINE__, cmd);
+
+	return dev->lv1_status;
+}
+EXPORT_SYMBOL_GPL(ps3stor_send_command);
+
+
+MODULE_LICENSE("GPL");
+MODULE_DESCRIPTION("PS3 Storage Bus Library");
+MODULE_AUTHOR("Sony Corporation");
--- /dev/null
+++ b/include/asm-powerpc/ps3stor.h
@@ -0,0 +1,72 @@
+/*
+ * PS3 Storage Devices
+ *
+ * Copyright (C) 2007 Sony Computer Entertainment Inc.
+ * Copyright 2007 Sony Corp.
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License as published
+ * by the Free Software Foundation; version 2 of the License.
+ *
+ * This program is distributed in the hope that it will be useful, but
+ * WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ * General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License along
+ * with this program; if not, write to the Free Software Foundation, Inc.,
+ * 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA.
+ */
+
+#ifndef _ASM_POWERPC_PS3STOR_H_
+#define _ASM_POWERPC_PS3STOR_H_
+
+#include <linux/irqreturn.h>
+
+#include <asm/ps3.h>
+
+
+struct ps3_storage_region {
+	unsigned int id;
+	u64 start;
+	u64 size;
+};
+
+struct ps3_storage_device {
+	struct ps3_system_bus_device sbd;
+
+	struct ps3_dma_region dma_region;
+	unsigned int irq;
+	u64 blk_size;
+
+	u64 tag;
+	int lv1_res;
+	u64 lv1_tag;
+	u64 lv1_status;
+	struct completion irq_done;
+
+	unsigned long bounce_size;
+	void *bounce_buf;
+	u64 bounce_lpar;
+	dma_addr_t bounce_dma;
+
+	unsigned int num_regions;
+	unsigned long accessible_regions;
+	unsigned int region_idx;		/* first accessible region */
+	struct ps3_storage_region regions[0];	/* Must be last */
+};
+
+static inline struct ps3_storage_device *to_ps3_storage_device(struct device *dev)
+{
+	return container_of(dev, struct ps3_storage_device, sbd.core);
+}
+
+extern int ps3stor_setup(struct ps3_storage_device *dev, const char *name);
+extern void ps3stor_teardown(struct ps3_storage_device *dev);
+extern u64 ps3stor_read_write_sectors(struct ps3_storage_device *dev, u64 lpar,
+				      u64 start_sector, u64 sectors,
+				      int write);
+extern u64 ps3stor_send_command(struct ps3_storage_device *dev, u64 cmd,
+				u64 arg1, u64 arg2, u64 arg3, u64 arg4);
+
+#endif /* _ASM_POWERPC_PS3STOR_H_ */

-- 
Gr{oetje,eeting}s,

						Geert

--
Geert Uytterhoeven -- Sony Network and Software Technology Center Europe (NSCE)
Geert.Uytterhoeven@sonycom.com ------- The Corporate Village, Da Vincilaan 7-D1
Voice +32-2-7008453 Fax +32-2-7008622 ---------------- B-1935 Zaventem, Belgium

[patch 4/7] ps3: Storage Driver Probing

[Geert.Uytterhoeven]

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

Signed-off-by: Geert Uytterhoeven <Geert.Uytterhoeven@sonycom.com>
---
 arch/powerpc/platforms/ps3/device-init.c |  344 +++++++++++++++++++++++++++++++
 1 files changed, 344 insertions(+)

--- a/arch/powerpc/platforms/ps3/device-init.c
+++ b/arch/powerpc/platforms/ps3/device-init.c
@@ -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;

-- 
Gr{oetje,eeting}s,

						Geert

--
Geert Uytterhoeven -- Sony Network and Software Technology Center Europe (NSCE)
Geert.Uytterhoeven@sonycom.com ------- The Corporate Village, Da Vincilaan 7-D1
Voice +32-2-7008453 Fax +32-2-7008622 ---------------- B-1935 Zaventem, Belgium

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

[Arnd Bergmann]

On Friday 25 May 2007, Geert.Uytterhoeven@sonycom.com 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:

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

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

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

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.

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

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

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

[Geoff Levand]

Hi.

Arnd Bergmann wrote:
>> +	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;
>> +	}
> 
> 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

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

[Geert Uytterhoeven]

On Fri, 25 May 2007, Arnd Bergmann wrote:
> On Friday 25 May 2007, Geert.Uytterhoeven@sonycom.com wrote:
> > +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);
> > +	}
> 
> Any reason not to use msleep(1) instead of the schedule_timeout?

Not really. I'll change it.

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

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

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

--
Geert Uytterhoeven -- Sony Network and Software Technology Center Europe (NSCE)
Geert.Uytterhoeven@sonycom.com ------- The Corporate Village, Da Vincilaan 7-D1
Voice +32-2-7008453 Fax +32-2-7008622 ---------------- B-1935 Zaventem, Belgium

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

[Benjamin Herrenschmidt]

On Fri, 2007-05-25 at 21:48 +0200, Geert Uytterhoeven wrote:
> > > +           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.

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

(And not for request processing)

Ben.

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

[Benjamin Herrenschmidt]

On Fri, 2007-05-25 at 18:18 +0200, Arnd Bergmann wrote:
> > +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);
> > +     }
> 
> Any reason not to use msleep(1) instead of the schedule_timeout? 

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

Ben.

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

[Geert Uytterhoeven]

On Sat, 26 May 2007, Benjamin Herrenschmidt wrote:
> On Fri, 2007-05-25 at 18:18 +0200, Arnd Bergmann wrote:
> > > +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);
> > > +     }
> > 
> > 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

--
Geert Uytterhoeven -- Sony Network and Software Technology Center Europe (NSCE)
Geert.Uytterhoeven@sonycom.com ------- The Corporate Village, Da Vincilaan 7-D1
Voice +32-2-7008453 Fax +32-2-7008622 ---------------- B-1935 Zaventem, Belgium

[patch 5/7] ps3: Disk Storage Driver

[Geert.Uytterhoeven]

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 <Geert.Uytterhoeven@sonycom.com>
---
 arch/powerpc/platforms/ps3/Kconfig |   11 +
 drivers/block/Makefile             |    1 
 drivers/block/ps3disk.c            |  402 +++++++++++++++++++++++++++++++++++++
 3 files changed, 414 insertions(+)

--- a/arch/powerpc/platforms/ps3/Kconfig
+++ b/arch/powerpc/platforms/ps3/Kconfig
@@ -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
--- a/drivers/block/Makefile
+++ b/drivers/block/Makefile
@@ -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
--- /dev/null
+++ b/drivers/block/ps3disk.c
@@ -0,0 +1,402 @@
+/*
+ * PS3 Disk Storage Driver
+ *
+ * Copyright (C) 2007 Sony Computer Entertainment Inc.
+ * Copyright 2007 Sony Corp.
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License as published
+ * by the Free Software Foundation; version 2 of the License.
+ *
+ * This program is distributed in the hope that it will be useful, but
+ * WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ * General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License along
+ * with this program; if not, write to the Free Software Foundation, Inc.,
+ * 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA.
+ */
+
+#include <linux/dma-mapping.h>
+#include <linux/blkdev.h>
+#include <linux/freezer.h>
+#include <linux/hdreg.h>
+#include <linux/interrupt.h>
+#include <linux/kthread.h>
+
+#include <asm/ps3stor.h>
+
+
+#define DEVICE_NAME		"ps3disk"
+
+#define BOUNCE_SIZE		(64*1024)
+
+// FIXME Use a fixed major assigned by LANANA?
+#define PS3DISK_MAJOR		0
+
+#define PS3DISK_MAX_DISKS	16
+#define PS3DISK_MINORS		16
+
+#define KERNEL_SECTOR_SIZE	512
+
+
+#define PS3DISK_NAME		"ps3d%c"
+
+#define LV1_STORAGE_ATA_HDDOUT		(0x23)
+
+
+struct ps3disk_private {
+	spinlock_t lock;
+	struct task_struct *thread;
+	struct request_queue *queue;
+	struct gendisk *gendisk;
+	unsigned int blocking_factor;
+};
+#define ps3disk_priv(dev)	((dev)->sbd.core.driver_data)
+
+static int ps3disk_major = PS3DISK_MAJOR;
+
+static int ps3disk_open(struct inode *inode, struct file *file)
+{
+	struct ps3_storage_device *dev = inode->i_bdev->bd_disk->private_data;
+
+	file->private_data = dev;
+	return 0;
+}
+
+
+
+static struct block_device_operations ps3disk_fops = {
+	.owner		= THIS_MODULE,
+	.open		= ps3disk_open,
+};
+
+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);
+		}
+		sectors += bio_sectors(bio);
+		i++;
+	}
+}
+
+static void ps3disk_handle_request_sg(struct ps3_storage_device *dev,
+				      struct request *req)
+{
+	struct ps3disk_private *priv = ps3disk_priv(dev);
+	int uptodate = 1;
+	int write = rq_data_dir(req);
+	const char *op = write ? "write" : "read";
+	u64 res;
+
+#ifdef DEBUG
+	unsigned int n = 0;
+	struct bio *bio;
+	rq_for_each_bio(bio, req)
+		n++;
+	dev_dbg(&dev->sbd.core,
+		"%s:%u: %s req has %u bios for %lu sectors %lu hard sectors\n",
+		__func__, __LINE__, op, n, req->nr_sectors,
+		req->hard_nr_sectors);
+#endif
+
+	if (write)
+		ps3disk_scatter_gather(dev, req, 1);
+
+	res = ps3stor_read_write_sectors(dev, dev->bounce_lpar,
+					 req->sector*priv->blocking_factor,
+					 req->nr_sectors*priv->blocking_factor,
+					 write);
+	if (res) {
+		dev_err(&dev->sbd.core, "%s:%u: %s failed 0x%lx\n", __func__,
+			__LINE__, op, res);
+		uptodate = 0;
+	} else if (!write)
+		ps3disk_scatter_gather(dev, req, 0);
+
+	spin_lock_irq(&priv->lock);
+	if (!end_that_request_first(req, uptodate, req->nr_sectors)) {
+		blkdev_dequeue_request(req);
+		end_that_request_last(req, uptodate);
+	}
+	spin_unlock_irq(&priv->lock);
+}
+
+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);
+	}
+
+	dev_dbg(&dev->sbd.core, "%s thread exit\n", __func__);
+	return 0;
+}
+
+static int ps3disk_sync_cache(struct ps3_storage_device *dev)
+{
+	int res;
+
+	dev_dbg(&dev->sbd.core, "%s:%u: sync cache\n", __func__, __LINE__);
+
+	res = ps3stor_send_command(dev, LV1_STORAGE_ATA_HDDOUT, 0, 0, 0, 0);
+	if (res) {
+		dev_err(&dev->sbd.core, "%s:%u: sync cache failed 0x%lx\n",
+			__func__, __LINE__, dev->lv1_status);
+		return -EIO;
+	}
+	return 0;
+}
+
+static int ps3disk_issue_flush(request_queue_t *q, struct gendisk *gendisk,
+			       sector_t *sector)
+{
+	struct ps3_storage_device *dev = q->queuedata;
+	return ps3disk_sync_cache(dev);
+}
+
+static void ps3disk_prepare_flush(request_queue_t *q, struct request *req)
+{
+	// FIXME Is this the correct thing to do?
+	struct ps3_storage_device *dev = q->queuedata;
+	ps3disk_sync_cache(dev);
+	memset(req->cmd, 0, sizeof(req->cmd));
+	req->cmd_type = REQ_TYPE_FLUSH;
+}
+
+static void ps3disk_request(request_queue_t *q)
+{
+	struct ps3_storage_device *dev = q->queuedata;
+	struct ps3disk_private *priv = ps3disk_priv(dev);
+	wake_up_process(priv->thread);
+}
+
+static unsigned long ps3disk_mask;
+
+static int __devinit ps3disk_probe(struct ps3_system_bus_device *_dev)
+{
+	struct ps3_storage_device *dev = to_ps3_storage_device(&_dev->core);
+	struct ps3disk_private *priv;
+	int error;
+	unsigned int devidx;
+	struct request_queue *queue;
+	struct gendisk *gendisk;
+	struct task_struct *task;
+
+	if (dev->blk_size < KERNEL_SECTOR_SIZE) {
+		dev_err(&dev->sbd.core,
+			"%s:%u: cannot handle block size %lu\n", __func__,
+			__LINE__, dev->blk_size);
+		return -EINVAL;
+	}
+
+	BUILD_BUG_ON(PS3DISK_MAX_DISKS > BITS_PER_LONG);
+	devidx = find_first_zero_bit(&ps3disk_mask, PS3DISK_MAX_DISKS);
+	if (devidx >= PS3DISK_MAX_DISKS) {
+		dev_err(&dev->sbd.core, "%s:%u: Too many disks\n", __func__,
+			__LINE__);
+		return -ENOSPC;
+	}
+	__set_bit(devidx, &ps3disk_mask);
+
+	priv = kzalloc(sizeof(*priv), GFP_KERNEL);
+	if (!priv) {
+		error = -ENOMEM;
+		goto fail;
+	}
+
+	ps3disk_priv(dev) = priv;
+	spin_lock_init(&priv->lock);
+
+	dev->bounce_size = BOUNCE_SIZE;
+	dev->bounce_buf = kmalloc(BOUNCE_SIZE, GFP_DMA);
+	if (!dev->bounce_buf) {
+		error = -ENOMEM;
+		goto fail_free_priv;
+	}
+
+	error = ps3stor_setup(dev, DEVICE_NAME);
+	if (error)
+		goto fail_free_bounce;
+
+	queue = blk_init_queue(ps3disk_request, &priv->lock);
+	if (!queue) {
+		dev_err(&dev->sbd.core, "%s:%u: blk_init_queue failed\n",
+			__func__, __LINE__);
+		error = -ENOMEM;
+		goto fail_teardown;
+	}
+
+	priv->queue = queue;
+	queue->queuedata = dev;
+
+	blk_queue_bounce_limit(queue, BLK_BOUNCE_HIGH);
+
+	blk_queue_max_sectors(queue, dev->bounce_size/KERNEL_SECTOR_SIZE);
+	blk_queue_segment_boundary(queue, -1UL);
+	blk_queue_dma_alignment(queue, dev->blk_size-1);
+	blk_queue_hardsect_size(queue, dev->blk_size);
+
+	blk_queue_issue_flush_fn(queue, ps3disk_issue_flush);
+	blk_queue_ordered(queue, QUEUE_ORDERED_DRAIN_FLUSH,
+			  ps3disk_prepare_flush);
+
+	blk_queue_max_phys_segments(queue, -1);
+	blk_queue_max_hw_segments(queue, -1);
+	blk_queue_max_segment_size(queue, dev->bounce_size);
+
+	gendisk = alloc_disk(PS3DISK_MINORS);
+	if (!gendisk) {
+		dev_err(&dev->sbd.core, "%s:%u: alloc_disk failed\n", __func__,
+			__LINE__);
+		error = -ENOMEM;
+		goto fail_cleanup_queue;
+	}
+
+	priv->gendisk = gendisk;
+	gendisk->major = ps3disk_major;
+	gendisk->first_minor = devidx * PS3DISK_MINORS;
+	gendisk->fops = &ps3disk_fops;
+	gendisk->queue = queue;
+	gendisk->private_data = dev;
+	snprintf(gendisk->disk_name, sizeof(gendisk->disk_name), PS3DISK_NAME,
+		 devidx+'a');
+	priv->blocking_factor = dev->blk_size/KERNEL_SECTOR_SIZE;
+	set_capacity(gendisk,
+		     dev->regions[dev->region_idx].size*priv->blocking_factor);
+
+	task = kthread_run(ps3disk_thread, dev, DEVICE_NAME);
+	if (IS_ERR(task)) {
+		error = PTR_ERR(task);
+		goto fail_free_disk;
+	}
+	priv->thread = task;
+
+	add_disk(gendisk);
+	return 0;
+
+fail_free_disk:
+	put_disk(priv->gendisk);
+fail_cleanup_queue:
+	blk_cleanup_queue(queue);
+fail_teardown:
+	ps3stor_teardown(dev);
+fail_free_bounce:
+	kfree(dev->bounce_buf);
+fail_free_priv:
+	kfree(priv);
+fail:
+	__clear_bit(devidx, &ps3disk_mask);
+	return error;
+}
+
+static int ps3disk_remove(struct ps3_system_bus_device *_dev)
+{
+	struct ps3_storage_device *dev = to_ps3_storage_device(&_dev->core);
+	struct ps3disk_private *priv = ps3disk_priv(dev);
+
+	kthread_stop(priv->thread);
+	__clear_bit(priv->gendisk->first_minor / PS3DISK_MINORS,
+		    &ps3disk_mask);
+	del_gendisk(priv->gendisk);
+	put_disk(priv->gendisk);
+	blk_cleanup_queue(priv->queue);
+	dev_notice(&dev->sbd.core, "Synchronizing disk cache\n");
+	ps3disk_sync_cache(dev);
+	ps3stor_teardown(dev);
+	kfree(dev->bounce_buf);
+	kfree(priv);
+	return 0;
+}
+
+
+static struct ps3_system_bus_driver ps3disk = {
+	.match_id	= PS3_MATCH_ID_STOR_DISK,
+	.core.name	= DEVICE_NAME,
+	.core.owner	= THIS_MODULE,
+	.probe		= ps3disk_probe,
+	.remove		= ps3disk_remove,
+	.shutdown	= ps3disk_remove,
+};
+
+
+static int __init ps3disk_init(void)
+{
+	int error;
+
+	error = register_blkdev(ps3disk_major, DEVICE_NAME);
+	if (error <= 0) {
+		printk(KERN_ERR "%s:%u: register_blkdev failed %d\n", __func__,
+		       __LINE__, error);
+		return error;
+	}
+	if (!ps3disk_major)
+		ps3disk_major = error;
+
+	pr_info("%s:%u: registered block device major %d\n", __func__,
+		__LINE__, ps3disk_major);
+
+	return ps3_system_bus_driver_register(&ps3disk, PS3_IOBUS_SB);
+}
+
+static void __exit ps3disk_exit(void)
+{
+	unregister_blkdev(ps3disk_major, DEVICE_NAME);
+
+	ps3_system_bus_driver_unregister(&ps3disk);
+}
+
+module_init(ps3disk_init);
+module_exit(ps3disk_exit);
+
+MODULE_LICENSE("GPL");
+MODULE_DESCRIPTION("PS3 Disk Storage Driver");
+MODULE_AUTHOR("Sony Corporation");
+

-- 
Gr{oetje,eeting}s,

						Geert

--
Geert Uytterhoeven -- Sony Network and Software Technology Center Europe (NSCE)
Geert.Uytterhoeven@sonycom.com ------- The Corporate Village, Da Vincilaan 7-D1
Voice +32-2-7008453 Fax +32-2-7008622 ---------------- B-1935 Zaventem, Belgium

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

[Olaf Hering]

On Fri, May 25, Geert.Uytterhoeven@sonycom.com wrote:

> Add a Disk Storage Driver for the PS3:

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

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

[Geert Uytterhoeven]

On Fri, 25 May 2007, Olaf Hering wrote:
> On Fri, May 25, Geert.Uytterhoeven@sonycom.com 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 -- Sony Network and Software Technology Center Europe (NSCE)
Geert.Uytterhoeven@sonycom.com ------- The Corporate Village, Da Vincilaan 7-D1
Voice +32-2-7008453 Fax +32-2-7008622 ---------------- B-1935 Zaventem, Belgium

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

[Olaf Hering]

On Fri, May 25, Geert Uytterhoeven wrote:

> On Fri, 25 May 2007, Olaf Hering wrote:
> > On Fri, May 25, Geert.Uytterhoeven@sonycom.com 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;

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

[Arnd Bergmann]

On Friday 25 May 2007, Geert.Uytterhoeven@sonycom.com wrote:
> +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);
> +		}
> +		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.

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

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

[Geert Uytterhoeven]

On Fri, 25 May 2007, Arnd Bergmann wrote:
> On Friday 25 May 2007, Geert.Uytterhoeven@sonycom.com wrote:
> > +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);
> > +		}
> > +		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.

> > +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);
> > +	}
> > +
> > +	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 -- Sony Network and Software Technology Center Europe (NSCE)
Geert.Uytterhoeven@sonycom.com ------- The Corporate Village, Da Vincilaan 7-D1
Voice +32-2-7008453 Fax +32-2-7008622 ---------------- B-1935 Zaventem, Belgium

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

[Arnd Bergmann]

On Friday 25 May 2007, Geert Uytterhoeven wrote:
> 
> > 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 <><

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

[Geert Uytterhoeven]

On Fri, 25 May 2007, Arnd Bergmann wrote:
> On Friday 25 May 2007, Geert Uytterhoeven wrote:
> > > 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

--
Geert Uytterhoeven -- Sony Network and Software Technology Center Europe (NSCE)
Geert.Uytterhoeven@sonycom.com ------- The Corporate Village, Da Vincilaan 7-D1
Voice +32-2-7008453 Fax +32-2-7008622 ---------------- B-1935 Zaventem, Belgium

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

[Arnd Bergmann]

On Friday 25 May 2007, Geert Uytterhoeven wrote:
> On Fri, 25 May 2007, Arnd Bergmann wrote:
>
> > 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 <><

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

[Benjamin Herrenschmidt]

On Fri, 2007-05-25 at 21:40 +0200, Geert Uytterhoeven wrote:
> 
> 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.

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

[Benjamin Herrenschmidt]

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

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

[patch 6/7] ps3: ROM Storage Driver

[Geert.Uytterhoeven]

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 <Geert.Uytterhoeven@sonycom.com>
---
 arch/powerpc/platforms/ps3/Kconfig |   11 
 drivers/scsi/Makefile              |    1 
 drivers/scsi/ps3rom.c              |  816 +++++++++++++++++++++++++++++++++++++
 3 files changed, 828 insertions(+)

--- a/arch/powerpc/platforms/ps3/Kconfig
+++ b/arch/powerpc/platforms/ps3/Kconfig
@@ -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
--- a/drivers/scsi/Makefile
+++ b/drivers/scsi/Makefile
@@ -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/
 
--- /dev/null
+++ b/drivers/scsi/ps3rom.c
@@ -0,0 +1,816 @@
+/*
+ * PS3 ROM Storage Driver
+ *
+ * Copyright (C) 2007 Sony Computer Entertainment Inc.
+ * Copyright 2007 Sony Corp.
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License as published
+ * by the Free Software Foundation; version 2 of the License.
+ *
+ * This program is distributed in the hope that it will be useful, but
+ * WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ * General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License along
+ * with this program; if not, write to the Free Software Foundation, Inc.,
+ * 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA.
+ */
+
+#include <linux/cdrom.h>
+#include <linux/interrupt.h>
+#include <linux/kthread.h>
+
+#include <scsi/scsi.h>
+#include <scsi/scsi_cmnd.h>
+#include <scsi/scsi_device.h>
+#include <scsi/scsi_host.h>
+
+#include <asm/ps3stor.h>
+
+
+#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)
+
+struct lv1_atapi_cmnd_block {
+	u8	pkt[32];	/* packet command block           */
+	u32	pktlen;		/* should be 12 for ATAPI 8020    */
+	u32	blocks;
+	u32	block_size;
+	u32	proto;		/* transfer mode                  */
+	u32	in_out;		/* transfer direction             */
+	u64	buffer;		/* parameter except command block */
+	u32	arglen;		/* length above                   */
+};
+
+/*
+ * to position parameter
+ */
+enum {
+	NOT_AVAIL          = -1,
+	USE_SRB_10         = -2,
+	USE_SRB_6          = -3,
+	USE_CDDA_FRAME_RAW = -4
+};
+
+enum lv1_atapi_proto {
+	NA_PROTO = -1,
+	NON_DATA_PROTO     = 0,
+	PIO_DATA_IN_PROTO  = 1,
+	PIO_DATA_OUT_PROTO = 2,
+	DMA_PROTO = 3
+};
+
+enum lv1_atapi_in_out {
+	DIR_NA = -1,
+	DIR_WRITE = 0, /* memory -> device */
+	DIR_READ = 1 /* device -> memory */
+};
+
+
+#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";
+	case FORMAT_UNIT:		return "FORMAT_UNIT/GPCMD_FORMAT_UNIT";
+	case READ_BLOCK_LIMITS:		return "READ_BLOCK_LIMITS";
+	case REASSIGN_BLOCKS:		return "REASSIGN_BLOCKS/INITIALIZE_ELEMENT_STATUS";
+	case READ_6:			return "READ_6";
+	case WRITE_6:			return "WRITE_6/MI_REPORT_TARGET_PGS";
+	case SEEK_6:			return "SEEK_6";
+	case READ_REVERSE:		return "READ_REVERSE";
+	case WRITE_FILEMARKS:		return "WRITE_FILEMARKS/SAI_READ_CAPACITY_16";
+	case SPACE:			return "SPACE";
+	case INQUIRY:			return "INQUIRY/GPCMD_INQUIRY";
+	case RECOVER_BUFFERED_DATA:	return "RECOVER_BUFFERED_DATA";
+	case MODE_SELECT:		return "MODE_SELECT";
+	case RESERVE:			return "RESERVE";
+	case RELEASE:			return "RELEASE";
+	case COPY:			return "COPY";
+	case ERASE:			return "ERASE";
+	case MODE_SENSE:		return "MODE_SENSE";
+	case START_STOP:		return "START_STOP/GPCMD_START_STOP_UNIT";
+	case RECEIVE_DIAGNOSTIC:	return "RECEIVE_DIAGNOSTIC";
+	case SEND_DIAGNOSTIC:		return "SEND_DIAGNOSTIC";
+	case ALLOW_MEDIUM_REMOVAL:	return "ALLOW_MEDIUM_REMOVAL/GPCMD_PREVENT_ALLOW_MEDIUM_REMOVAL";
+	case SET_WINDOW:		return "SET_WINDOW";
+	case READ_CAPACITY:		return "READ_CAPACITY/GPCMD_READ_CDVD_CAPACITY";
+	case READ_10:			return "READ_10/GPCMD_READ_10";
+	case WRITE_10:			return "WRITE_10/GPCMD_WRITE_10";
+	case SEEK_10:			return "SEEK_10/POSITION_TO_ELEMENT/GPCMD_SEEK";
+	case WRITE_VERIFY:		return "WRITE_VERIFY/GPCMD_WRITE_AND_VERIFY_10";
+	case VERIFY:			return "VERIFY/GPCMD_VERIFY_10";
+	case SEARCH_HIGH:		return "SEARCH_HIGH";
+	case SEARCH_EQUAL:		return "SEARCH_EQUAL";
+	case SEARCH_LOW:		return "SEARCH_LOW";
+	case SET_LIMITS:		return "SET_LIMITS";
+	case PRE_FETCH:			return "PRE_FETCH/READ_POSITION";
+	case SYNCHRONIZE_CACHE:		return "SYNCHRONIZE_CACHE/GPCMD_FLUSH_CACHE";
+	case LOCK_UNLOCK_CACHE:		return "LOCK_UNLOCK_CACHE";
+	case READ_DEFECT_DATA:		return "READ_DEFECT_DATA";
+	case MEDIUM_SCAN:		return "MEDIUM_SCAN";
+	case COMPARE:			return "COMPARE";
+	case COPY_VERIFY:		return "COPY_VERIFY";
+	case WRITE_BUFFER:		return "WRITE_BUFFER";
+	case READ_BUFFER:		return "READ_BUFFER";
+	case UPDATE_BLOCK:		return "UPDATE_BLOCK";
+	case READ_LONG:			return "READ_LONG";
+	case WRITE_LONG:		return "WRITE_LONG";
+	case CHANGE_DEFINITION:		return "CHANGE_DEFINITION";
+	case WRITE_SAME:		return "WRITE_SAME";
+	case READ_TOC:			return "READ_TOC/GPCMD_READ_TOC_PMA_ATIP";
+	case LOG_SELECT:		return "LOG_SELECT";
+	case LOG_SENSE:			return "LOG_SENSE";
+	case MODE_SELECT_10:		return "MODE_SELECT_10/GPCMD_MODE_SELECT_10";
+	case RESERVE_10:		return "RESERVE_10";
+	case RELEASE_10:		return "RELEASE_10";
+	case MODE_SENSE_10:		return "MODE_SENSE_10/GPCMD_MODE_SENSE_10";
+	case PERSISTENT_RESERVE_IN:	return "PERSISTENT_RESERVE_IN";
+	case PERSISTENT_RESERVE_OUT:	return "PERSISTENT_RESERVE_OUT";
+	case REPORT_LUNS:		return "REPORT_LUNS";
+	case MAINTENANCE_IN:		return "MAINTENANCE_IN/GPCMD_SEND_KEY";
+	case MOVE_MEDIUM:		return "MOVE_MEDIUM";
+	case EXCHANGE_MEDIUM:		return "EXCHANGE_MEDIUM/GPCMD_LOAD_UNLOAD";
+	case READ_12:			return "READ_12/GPCMD_READ_12";
+	case WRITE_12:			return "WRITE_12";
+	case WRITE_VERIFY_12:		return "WRITE_VERIFY_12";
+	case SEARCH_HIGH_12:		return "SEARCH_HIGH_12";
+	case SEARCH_EQUAL_12:		return "SEARCH_EQUAL_12";
+	case SEARCH_LOW_12:		return "SEARCH_LOW_12";
+	case READ_ELEMENT_STATUS:	return "READ_ELEMENT_STATUS";
+	case SEND_VOLUME_TAG:		return "SEND_VOLUME_TAG/GPCMD_SET_STREAMING";
+	case WRITE_LONG_2:		return "WRITE_LONG_2";
+	case READ_16:			return "READ_16";
+	case WRITE_16:			return "WRITE_16";
+	case VERIFY_16:			return "VERIFY_16";
+	case SERVICE_ACTION_IN:		return "SERVICE_ACTION_IN";
+	case ATA_16:			return "ATA_16";
+	case ATA_12:			return "ATA_12/GPCMD_BLANK";
+	case GPCMD_CLOSE_TRACK:		return "GPCMD_CLOSE_TRACK";
+	case GPCMD_GET_CONFIGURATION:	return "GPCMD_GET_CONFIGURATION";
+	case GPCMD_GET_EVENT_STATUS_NOTIFICATION:	return "GPCMD_GET_EVENT_STATUS_NOTIFICATION";
+	case GPCMD_GET_PERFORMANCE:	return "GPCMD_GET_PERFORMANCE";
+	case GPCMD_MECHANISM_STATUS:	return "GPCMD_MECHANISM_STATUS";
+	case GPCMD_PAUSE_RESUME:	return "GPCMD_PAUSE_RESUME";
+	case GPCMD_PLAY_AUDIO_10:	return "GPCMD_PLAY_AUDIO_10";
+	case GPCMD_PLAY_AUDIO_MSF:	return "GPCMD_PLAY_AUDIO_MSF";
+	case GPCMD_PLAY_AUDIO_TI:	return "GPCMD_PLAY_AUDIO_TI/GPCMD_PLAYAUDIO_TI";
+	case GPCMD_PLAY_CD:		return "GPCMD_PLAY_CD";
+	case GPCMD_READ_BUFFER_CAPACITY:	return "GPCMD_READ_BUFFER_CAPACITY";
+	case GPCMD_READ_CD:		return "GPCMD_READ_CD";
+	case GPCMD_READ_CD_MSF:		return "GPCMD_READ_CD_MSF";
+	case GPCMD_READ_DISC_INFO:	return "GPCMD_READ_DISC_INFO";
+	case GPCMD_READ_DVD_STRUCTURE:	return "GPCMD_READ_DVD_STRUCTURE";
+	case GPCMD_READ_FORMAT_CAPACITIES:	return "GPCMD_READ_FORMAT_CAPACITIES";
+	case GPCMD_READ_HEADER:		return "GPCMD_READ_HEADER";
+	case GPCMD_READ_TRACK_RZONE_INFO:	return "GPCMD_READ_TRACK_RZONE_INFO";
+	case GPCMD_READ_SUBCHANNEL:	return "GPCMD_READ_SUBCHANNEL";
+	case GPCMD_REPAIR_RZONE_TRACK:	return "GPCMD_REPAIR_RZONE_TRACK";
+	case GPCMD_REPORT_KEY:		return "GPCMD_REPORT_KEY";
+	case GPCMD_RESERVE_RZONE_TRACK:	return "GPCMD_RESERVE_RZONE_TRACK";
+	case GPCMD_SEND_CUE_SHEET:	return "GPCMD_SEND_CUE_SHEET";
+	case GPCMD_SCAN:		return "GPCMD_SCAN";
+	case GPCMD_SEND_DVD_STRUCTURE:	return "GPCMD_SEND_DVD_STRUCTURE";
+	case GPCMD_SEND_EVENT:		return "GPCMD_SEND_EVENT";
+	case GPCMD_SEND_OPC:		return "GPCMD_SEND_OPC";
+	case GPCMD_SET_READ_AHEAD:	return "GPCMD_SET_READ_AHEAD";
+	case GPCMD_STOP_PLAY_SCAN:	return "GPCMD_STOP_PLAY_SCAN";
+	case GPCMD_SET_SPEED:		return "GPCMD_SET_SPEED";
+	case GPCMD_GET_MEDIA_STATUS:	return "GPCMD_GET_MEDIA_STATUS";
+
+	default:
+	    return "***UNKNOWN***";
+	}
+}
+#else /* !DEBUG */
+static inline const char *scsi_command(unsigned char cmd) { return NULL; }
+#endif /* DEBUG */
+
+
+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;
+	return 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,
+	 * 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)
+{
+}
+
+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;
+	}
+
+	// FIXME Prevalidate commands?
+	priv->cmd = cmd;
+	priv->scsi_done = done;
+	spin_unlock_irq(&priv->lock);
+	wake_up_process(priv->thread);
+	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;
+	}
+
+	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;
+	return 0;
+}
+
+/*
+ * copy data from scatter/gather into device's buffer
+ */
+static int fetch_to_dev_buffer(struct scsi_cmnd *cmd, void *buf, int buflen)
+{
+	int k, req_len, len, fin;
+	void *kaddr;
+	struct scatterlist *sgpnt;
+
+	if (!cmd->request_bufflen)
+		return 0;
+
+	if (!cmd->request_buffer)
+		return -1;
+
+	if (cmd->sc_data_direction != DMA_BIDIRECTIONAL &&
+	    cmd->sc_data_direction != DMA_TO_DEVICE)
+		return -1;
+
+	if (!cmd->use_sg) {
+		req_len = cmd->request_bufflen;
+		len = min(req_len, buflen);
+		memcpy(buf, cmd->request_buffer, len);
+		return len;
+	}
+
+	sgpnt = cmd->request_buffer;
+	for (k = 0, req_len = 0, fin = 0; k < cmd->use_sg; ++k, ++sgpnt) {
+		kaddr = kmap_atomic(sgpnt->page, KM_USER0);
+		if (!kaddr)
+			return -1;
+		len = sgpnt->length;
+		if ((req_len + len) > buflen) {
+			len = buflen - req_len;
+			fin = 1;
+		}
+		memcpy(buf + req_len, kaddr + sgpnt->offset, len);
+		kunmap_atomic(kaddr, KM_USER0);
+		if (fin)
+			return req_len + len;
+		req_len += sgpnt->length;
+	}
+	return req_len;
+}
+
+static int decode_lv1_status(u64 status, unsigned char *sense_key,
+			     unsigned char *asc, unsigned char *ascq)
+{
+	if (((status >> 24) & 0xff) != SAM_STAT_CHECK_CONDITION)
+		return -1;
+
+	*sense_key = (status >> 16) & 0xff;
+	*asc       = (status >>  8) & 0xff;
+	*ascq      =  status        & 0xff;
+	return 0;
+}
+
+static inline unsigned int srb6_lba(const struct scsi_cmnd *cmd)
+{
+	BUG_ON(cmd->cmnd[1] & 0xe0);	// FIXME lun == 0
+	return cmd->cmnd[1] << 16 | cmd->cmnd[2] << 8 | cmd->cmnd[3];
+}
+
+static inline unsigned int srb6_len(const struct scsi_cmnd *cmd)
+{
+	return cmd->cmnd[4];
+}
+
+static inline unsigned int srb10_lba(const struct scsi_cmnd *cmd)
+{
+	return cmd->cmnd[2] << 24 | cmd->cmnd[3] << 16 | cmd->cmnd[4] << 8 |
+	       cmd->cmnd[5];
+}
+
+static inline unsigned int srb10_len(const struct scsi_cmnd *cmd)
+{
+	return cmd->cmnd[7] << 8 | cmd->cmnd[8];
+}
+
+static inline unsigned int cdda_raw_len(const struct scsi_cmnd *cmd)
+{
+	unsigned int nframes;
+
+	nframes = cmd->cmnd[6] << 16 | cmd->cmnd[7] <<  8 | cmd->cmnd[8];
+	return nframes * CD_FRAMESIZE_RAW;
+}
+
+static u64 ps3rom_send_atapi_command(struct ps3_storage_device *dev,
+				     struct lv1_atapi_cmnd_block *cmd)
+{
+	dev_dbg(&dev->sbd.core, "%s:%u: send ATAPI command 0x%02x (%s)\n",
+		__func__, __LINE__, cmd->pkt[0], scsi_command(cmd->pkt[0]));
+
+	return ps3stor_send_command(dev, LV1_STORAGE_SEND_ATAPI_COMMAND,
+				    ps3_mm_phys_to_lpar(__pa(cmd)),
+				    sizeof(*cmd), cmd->buffer, cmd->arglen);
+}
+
+static void ps3rom_atapi_request(struct ps3_storage_device *dev,
+				 struct scsi_cmnd *cmd, unsigned int len,
+				 int proto, int in_out, int auto_sense)
+{
+	struct lv1_atapi_cmnd_block atapi_cmnd;
+	unsigned char *cmnd = cmd->cmnd;
+	u64 status;
+	unsigned char sense_key, asc, ascq;
+
+	if (len > dev->bounce_size) {
+		static int printed;
+		if (!printed++)
+			dev_err(&dev->sbd.core,
+				"%s:%u: data size too large %u > %lu\n",
+			       __func__, __LINE__, len, dev->bounce_size);
+		cmd->result = DID_ERROR << 16;
+		memset(cmd->sense_buffer, 0, SCSI_SENSE_BUFFERSIZE);
+		cmd->sense_buffer[0] = 0x70;
+		cmd->sense_buffer[2] = ILLEGAL_REQUEST;
+		return;
+	}
+
+	memset(&atapi_cmnd, 0, sizeof(struct lv1_atapi_cmnd_block));
+	memcpy(&atapi_cmnd.pkt, cmnd, 12);
+	atapi_cmnd.pktlen = 12;
+	atapi_cmnd.proto = proto;
+	if (in_out != DIR_NA)
+		atapi_cmnd.in_out = in_out;
+
+	if (atapi_cmnd.in_out == DIR_WRITE) {
+		// FIXME check error
+		fetch_to_dev_buffer(cmd, dev->bounce_buf, len);
+	}
+
+	atapi_cmnd.block_size = 1; /* transfer size is block_size * blocks */
+
+	atapi_cmnd.blocks = atapi_cmnd.arglen = len;
+	atapi_cmnd.buffer = dev->bounce_lpar;
+
+	status = ps3rom_send_atapi_command(dev, &atapi_cmnd);
+	if (status == -1) {
+		cmd->result = DID_ERROR << 16; /* FIXME: is better other error code ? */
+		return;
+	}
+
+	if (!status) {
+		/* OK, completed */
+		if (atapi_cmnd.in_out == DIR_READ) {
+			// FIXME check error
+			fill_from_dev_buffer(cmd, dev->bounce_buf, len);
+		}
+		cmd->result = DID_OK << 16;
+		return;
+	}
+
+	/* error */
+	if (!auto_sense) {
+		cmd->result = (DID_ERROR << 16) | (CHECK_CONDITION << 1);
+		dev_err(&dev->sbd.core, "%s:%u: end error without autosense\n",
+		       __func__, __LINE__);
+		return;
+	}
+
+	if (!decode_lv1_status(status, &sense_key, &asc, &ascq)) {
+		/* lv1 may have issued autosense ... */
+		cmd->sense_buffer[0]  = 0x70;
+		cmd->sense_buffer[2]  = sense_key;
+		cmd->sense_buffer[7]  = 16 - 6;
+		cmd->sense_buffer[12] = asc;
+		cmd->sense_buffer[13] = ascq;
+		cmd->result = SAM_STAT_CHECK_CONDITION;
+		return;
+	}
+
+	/* do auto sense by ourselves */
+	memset(&atapi_cmnd, 0, sizeof(struct lv1_atapi_cmnd_block));
+	atapi_cmnd.pkt[0] = REQUEST_SENSE;
+	atapi_cmnd.pkt[4] = 18;
+	atapi_cmnd.pktlen = 12;
+	atapi_cmnd.arglen = atapi_cmnd.blocks = atapi_cmnd.pkt[4];
+	atapi_cmnd.block_size = 1;
+	atapi_cmnd.proto = DMA_PROTO;
+	atapi_cmnd.in_out = DIR_READ;
+	atapi_cmnd.buffer = dev->bounce_lpar;
+
+	/* issue REQUEST_SENSE command */
+	status = ps3rom_send_atapi_command(dev, &atapi_cmnd);
+	if (status == -1) {
+		cmd->result = DID_ERROR << 16; /* FIXME: is better other error code ? */
+		return;
+	}
+
+	/* scsi spec says request sense should never get error */
+	if (status) {
+		decode_lv1_status(status, &sense_key, &asc, &ascq);
+		dev_err(&dev->sbd.core,
+			"%s:%u: auto REQUEST_SENSE error %#x %#x %#x\n",
+			__func__, __LINE__, sense_key, asc, ascq);
+	}
+
+	memcpy(cmd->sense_buffer, dev->bounce_buf,
+	       min_t(size_t, atapi_cmnd.pkt[4], SCSI_SENSE_BUFFERSIZE));
+	cmd->result = SAM_STAT_CHECK_CONDITION;
+}
+
+static void ps3rom_read_request(struct ps3_storage_device *dev,
+				struct scsi_cmnd *cmd, u32 start_sector,
+				u32 sectors)
+{
+	u64 status;
+
+	status = ps3stor_read_write_sectors(dev, dev->bounce_lpar,
+					    start_sector, sectors, 0);
+	if (status == -1) {
+		cmd->result = DID_ERROR << 16; /* FIXME: other error code? */
+		return;
+	}
+
+	if (status) {
+		memset(cmd->sense_buffer, 0, SCSI_SENSE_BUFFERSIZE);
+		decode_lv1_status(dev->lv1_status, &cmd->sense_buffer[2],
+				  &cmd->sense_buffer[12],
+				  &cmd->sense_buffer[13]);
+		cmd->sense_buffer[7] = 16 - 6;	// FIXME hardcoded numbers?
+		cmd->result = SAM_STAT_CHECK_CONDITION;
+		return;
+	}
+
+	// FIXME check error
+	fill_from_dev_buffer(cmd, dev->bounce_buf, sectors * CD_FRAMESIZE);
+
+	cmd->result = DID_OK << 16;
+}
+
+static void ps3rom_write_request(struct ps3_storage_device *dev,
+				 struct scsi_cmnd *cmd, u32 start_sector,
+				 u32 sectors)
+{
+	u64 status;
+
+	// FIXME check error
+	fetch_to_dev_buffer(cmd, dev->bounce_buf, sectors * CD_FRAMESIZE);
+
+	status = ps3stor_read_write_sectors(dev, dev->bounce_lpar,
+					    start_sector, sectors, 1);
+	if (status == -1) {
+		cmd->result = DID_ERROR << 16; /* FIXME: other error code? */
+		return;
+	}
+
+	if (status) {
+		memset(cmd->sense_buffer, 0, SCSI_SENSE_BUFFERSIZE);
+		decode_lv1_status(dev->lv1_status, &cmd->sense_buffer[2],
+				  &cmd->sense_buffer[12],
+				  &cmd->sense_buffer[13]);
+		cmd->sense_buffer[7] = 16 - 6;	// FIXME hardcoded numbers?
+		cmd->result = SAM_STAT_CHECK_CONDITION;
+		return;
+	}
+
+	cmd->result = DID_OK << 16;
+}
+
+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;
+
+	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;
+
+	case READ_CAPACITY:
+		ps3rom_atapi_request(dev, cmd, 8, PIO_DATA_IN_PROTO, DIR_READ,
+				     1);
+		break;
+
+	case MODE_SENSE_10:
+	case READ_TOC:
+	case GPCMD_GET_CONFIGURATION:
+	case GPCMD_READ_DISC_INFO:
+		ps3rom_atapi_request(dev, cmd, srb10_len(cmd),
+				     PIO_DATA_IN_PROTO, DIR_READ, 1);
+		break;
+
+	case READ_6:
+		ps3rom_read_request(dev, cmd, srb6_lba(cmd), srb6_len(cmd));
+		break;
+
+	case READ_10:
+		ps3rom_read_request(dev, cmd, srb10_lba(cmd), srb10_len(cmd));
+		break;
+
+	case WRITE_6:
+		ps3rom_write_request(dev, cmd, srb6_lba(cmd), srb6_len(cmd));
+		break;
+
+	case WRITE_10:
+		ps3rom_write_request(dev, cmd, srb10_lba(cmd), srb10_len(cmd));
+		break;
+
+	case GPCMD_READ_CD:
+		ps3rom_atapi_request(dev, cmd, cdda_raw_len(cmd), DMA_PROTO,
+				     DIR_READ, 1);
+		break;
+
+	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;
+	}
+
+	spin_lock_irq(&priv->lock);
+	priv->cmd = NULL;
+	priv->scsi_done(cmd);
+	spin_unlock_irq(&priv->lock);
+}
+
+static int ps3rom_thread(void *data)
+{
+	struct ps3_storage_device *dev = data;
+	struct ps3rom_private *priv = ps3rom_priv(dev);
+	struct scsi_cmnd *cmd;
+
+	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);
+		cmd = priv->cmd;
+		spin_unlock_irq(&priv->lock);
+		if (!cmd) {
+			schedule();
+			continue;
+		}
+		ps3rom_request(dev, cmd);
+	}
+
+	dev_dbg(&dev->sbd.core, "%s thread exit\n", __func__);
+	return 0;
+}
+
+
+static struct scsi_host_template ps3rom_host_template = {
+	.name =			DEVICE_NAME,
+	.slave_alloc =		ps3rom_slave_alloc,
+	.slave_configure =	ps3rom_slave_configure,
+	.slave_destroy =	ps3rom_slave_destroy,
+	.queuecommand =		ps3rom_queuecommand,
+	.can_queue =		1,
+	.this_id =		7,
+	.sg_tablesize =		SG_ALL,
+	.cmd_per_lun =		1,
+	.emulated =             1,		/* only sg driver uses this */
+	.max_sectors =		PS3ROM_MAX_SECTORS,
+	.use_clustering =	ENABLE_CLUSTERING,
+	.module =		THIS_MODULE,
+};
+
+
+static int __devinit ps3rom_probe(struct ps3_system_bus_device *_dev)
+{
+	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;
+
+	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;
+
+	ps3rom_priv(dev) = priv;
+	spin_lock_init(&priv->lock);
+
+	dev->bounce_size = BOUNCE_SIZE;
+	dev->bounce_buf = kmalloc(BOUNCE_SIZE, GFP_DMA);
+	if (!dev->bounce_buf) {
+		error = -ENOMEM;
+		goto fail_free_priv;
+	}
+
+	error = ps3stor_setup(dev, DEVICE_NAME);
+	if (error)
+		goto fail_free_bounce;
+
+	host = scsi_host_alloc(&ps3rom_host_template,
+			       sizeof(struct ps3_system_bus_device *));
+	if (!host) {
+		dev_err(&dev->sbd.core, "%s:%u: scsi_host_alloc failed\n",
+			__func__, __LINE__);
+		goto fail_teardown;
+	}
+
+	priv->host = host;
+	host->hostdata[0] = (unsigned long)dev;
+
+	/* One device/LUN per SCSI bus */
+	host->max_id = 1;
+	host->max_lun = 1;
+
+	error = scsi_add_host(host, &dev->sbd.core);
+	if (error) {
+		dev_err(&dev->sbd.core, "%s:%u: scsi_host_alloc failed %d\n",
+			__func__, __LINE__, error);
+		error = -ENODEV;
+		goto fail_host_put;
+	}
+
+	task = kthread_run(ps3rom_thread, dev, DEVICE_NAME);
+	if (IS_ERR(task)) {
+		error = PTR_ERR(task);
+		goto fail_remove_host;
+	}
+	priv->thread = task;
+
+	scsi_scan_host(host);
+	return 0;
+
+fail_remove_host:
+	scsi_remove_host(host);
+fail_host_put:
+	scsi_host_put(host);
+fail_teardown:
+	ps3stor_teardown(dev);
+fail_free_bounce:
+	kfree(dev->bounce_buf);
+fail_free_priv:
+	kfree(priv);
+	return error;
+}
+
+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);
+	return 0;
+}
+
+
+static struct ps3_system_bus_driver ps3rom = {
+	.match_id	= PS3_MATCH_ID_STOR_ROM,
+	.core.name	= DEVICE_NAME,
+	.core.owner	= THIS_MODULE,
+	.probe		= ps3rom_probe,
+	.remove		= ps3rom_remove
+};
+
+
+static int __init ps3rom_init(void)
+{
+	return ps3_system_bus_driver_register(&ps3rom, PS3_IOBUS_SB);
+}
+
+static void __exit ps3rom_exit(void)
+{
+	ps3_system_bus_driver_unregister(&ps3rom);
+}
+
+module_init(ps3rom_init);
+module_exit(ps3rom_exit);
+
+MODULE_LICENSE("GPL");
+MODULE_DESCRIPTION("PS3 ROM Storage Driver");
+MODULE_AUTHOR("Sony Corporation");

-- 
Gr{oetje,eeting}s,

						Geert

--
Geert Uytterhoeven -- Sony Network and Software Technology Center Europe (NSCE)
Geert.Uytterhoeven@sonycom.com ------- The Corporate Village, Da Vincilaan 7-D1
Voice +32-2-7008453 Fax +32-2-7008622 ---------------- B-1935 Zaventem, Belgium

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

[Olaf Hering]

On Fri, May 25, Geert.Uytterhoeven@sonycom.com wrote:

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

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

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

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

[Benjamin Herrenschmidt]

On Fri, 2007-05-25 at 13:24 +0200, Olaf Hering wrote:
> On Fri, May 25, Geert.Uytterhoeven@sonycom.com wrote:
> 
> > +++ b/drivers/scsi/ps3rom.c
> 
> > +			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.

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

[Geert Uytterhoeven]

On Sat, 26 May 2007, Benjamin Herrenschmidt wrote:
> On Fri, 2007-05-25 at 13:24 +0200, Olaf Hering wrote:
> > On Fri, May 25, Geert.Uytterhoeven@sonycom.com wrote:
> > 
> > > +++ b/drivers/scsi/ps3rom.c
> > 
> > > +			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 -- Sony Network and Software Technology Center Europe (NSCE)
Geert.Uytterhoeven@sonycom.com ------- The Corporate Village, Da Vincilaan 7-D1
Voice +32-2-7008453 Fax +32-2-7008622 ---------------- B-1935 Zaventem, Belgium

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

[Benjamin Herrenschmidt]

On Sat, 2007-05-26 at 10:52 +0200, Geert Uytterhoeven wrote:
> On Sat, 26 May 2007, Benjamin Herrenschmidt wrote:
> > On Fri, 2007-05-25 at 13:24 +0200, Olaf Hering wrote:
> > > On Fri, May 25, Geert.Uytterhoeven@sonycom.com wrote:
> > > 
> > > > +++ b/drivers/scsi/ps3rom.c
> > > 
> > > > +			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.

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

[Christoph Hellwig]

On Sun, May 27, 2007 at 08:18:43AM +1000, Benjamin Herrenschmidt wrote:
> > > > 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.

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

[Arnd Bergmann]

On Friday 25 May 2007, Geert.Uytterhoeven@sonycom.com 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 <><

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

[Geert Uytterhoeven]

On Fri, 25 May 2007, Arnd Bergmann wrote:
> On Friday 25 May 2007, Geert.Uytterhoeven@sonycom.com 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 -- Sony Network and Software Technology Center Europe (NSCE)
Geert.Uytterhoeven@sonycom.com ------- The Corporate Village, Da Vincilaan 7-D1
Voice +32-2-7008453 Fax +32-2-7008622 ---------------- B-1935 Zaventem, Belgium

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

[Arnd Bergmann]

On Friday 25 May 2007, Geert Uytterhoeven wrote:
> 
> > 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 <><

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

[Christoph Hellwig]

On Fri, May 25, 2007 at 11:04:29PM +0200, Arnd Bergmann wrote:
> On Friday 25 May 2007, Geert Uytterhoeven wrote:
> > 
> > > 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.

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)

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

[Christoph Hellwig]

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

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

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

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

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

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

[Geert Uytterhoeven]

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)
Geert.Uytterhoeven@sonycom.com ------- The Corporate Village, Da Vincilaan 7-D1
Voice +32-2-7008453 Fax +32-2-7008622 ---------------- B-1935 Zaventem, Belgium

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

[Benjamin Herrenschmidt]

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.

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

[Christoph Hellwig]

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

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

[Benjamin Herrenschmidt]

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.

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

[Geoff Levand]

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

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

[Geert Uytterhoeven]

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)
Geert.Uytterhoeven@sonycom.com ------- The Corporate Village, Da Vincilaan 7-D1
Voice +32-2-7008453 Fax +32-2-7008622 ---------------- B-1935 Zaventem, Belgium

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

[Christoph Hellwig]

On Tue, May 29, 2007 at 06:21:36PM +0200, Geert Uytterhoeven wrote:
> 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?

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.

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

[Geert.Uytterhoeven]

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 <Geert.Uytterhoeven@sonycom.com>
---
 arch/powerpc/platforms/ps3/Kconfig |   12 +
 drivers/char/Makefile              |    2 
 drivers/char/ps3flash.c            |  400 +++++++++++++++++++++++++++++++++++++
 3 files changed, 414 insertions(+)

--- a/arch/powerpc/platforms/ps3/Kconfig
+++ b/arch/powerpc/platforms/ps3/Kconfig
@@ -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
--- a/drivers/char/Makefile
+++ b/drivers/char/Makefile
@@ -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
 
--- /dev/null
+++ b/drivers/char/ps3flash.c
@@ -0,0 +1,400 @@
+/*
+ * PS3 FLASH ROM Storage Driver
+ *
+ * Copyright (C) 2007 Sony Computer Entertainment Inc.
+ * Copyright 2007 Sony Corp.
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License as published
+ * by the Free Software Foundation; version 2 of the License.
+ *
+ * This program is distributed in the hope that it will be useful, but
+ * WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ * General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License along
+ * with this program; if not, write to the Free Software Foundation, Inc.,
+ * 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA.
+ */
+
+#include <linux/dma-mapping.h>
+#include <linux/interrupt.h>
+#include <linux/miscdevice.h>
+
+#include <asm/uaccess.h>
+#include <asm/ps3stor.h>
+
+
+#define DEVICE_NAME		"ps3flash"
+
+#define FLASH_BLOCK_SIZE	(256*1024)
+
+
+struct ps3flash_private {
+	struct mutex mutex;
+};
+#define ps3flash_priv(dev)	((dev)->sbd.core.driver_data)
+
+static struct ps3_storage_device *ps3flash_dev;
+
+static ssize_t ps3flash_read_write_sectors(struct ps3_storage_device *dev,
+					   u64 lpar, u64 start_sector,
+					   u64 sectors, int write)
+{
+	const char *op = write ? "write" : "read";
+	u64 res = ps3stor_read_write_sectors(dev, lpar, start_sector, sectors,
+					     write);
+	if (res) {
+		dev_err(&dev->sbd.core, "%s:%u: %s failed 0x%lx\n", __func__,
+			__LINE__, op, res);
+		return -EIO;
+	}
+	return sectors;
+}
+
+static ssize_t ps3flash_read_sectors(struct ps3_storage_device *dev,
+				     u64 start_sector, u64 sectors,
+				     unsigned int sector_offset)
+{
+	u64 max_sectors, lpar;
+
+	max_sectors = dev->bounce_size / dev->blk_size;
+	if (sectors > max_sectors) {
+		dev_dbg(&dev->sbd.core, "%s:%u Limiting sectors to %lu\n",
+			__func__, __LINE__, max_sectors);
+		sectors = max_sectors;
+	}
+
+	lpar = dev->bounce_lpar + sector_offset * dev->blk_size;
+	return ps3flash_read_write_sectors(dev, lpar, start_sector, sectors,
+					   0);
+}
+
+static ssize_t ps3flash_write_chunk(struct ps3_storage_device *dev,
+				    u64 start_sector)
+{
+       u64 sectors = dev->bounce_size / dev->blk_size;
+       return ps3flash_read_write_sectors(dev, dev->bounce_lpar, start_sector,
+					  sectors, 1);
+}
+
+static loff_t ps3flash_llseek(struct file *file, loff_t offset, int origin)
+{
+	struct ps3_storage_device *dev = ps3flash_dev;
+	u64 size = dev->regions[dev->region_idx].size*dev->blk_size;
+
+	switch (origin) {
+	case 1:
+		offset += file->f_pos;
+		break;
+	case 2:
+		offset += size;
+		break;
+	}
+	if (offset < 0)
+		return -EINVAL;
+
+	file->f_pos = offset;
+	return file->f_pos;
+}
+
+static ssize_t ps3flash_read(struct file *file, char __user *buf, size_t count,
+			     loff_t *pos)
+{
+	struct ps3_storage_device *dev = ps3flash_dev;
+	struct ps3flash_private *priv = ps3flash_priv(dev);
+	u64 size, start_sector, end_sector, offset;
+	ssize_t sectors_read;
+	size_t remaining, n;
+
+	dev_dbg(&dev->sbd.core,
+		"%s:%u: Reading %zu bytes at position %lld to user 0x%p\n",
+		__func__, __LINE__, count, *pos, buf);
+
+	size = dev->regions[dev->region_idx].size*dev->blk_size;
+	if (*pos >= size || !count)
+		return 0;
+
+	if (*pos+count > size) {
+		dev_dbg(&dev->sbd.core,
+			"%s:%u Truncating count from %zu to %llu\n", __func__,
+			__LINE__, count, size-*pos);
+		count = size-*pos;
+	}
+
+	start_sector = do_div_llr(*pos, dev->blk_size, &offset);
+	end_sector = DIV_ROUND_UP(*pos+count, dev->blk_size);
+
+	remaining = count;
+	do {
+		mutex_lock(&priv->mutex);
+
+		sectors_read = ps3flash_read_sectors(dev, start_sector,
+						     end_sector-start_sector,
+						     0);
+		if (sectors_read < 0) {
+			mutex_unlock(&priv->mutex);
+			return sectors_read;
+		}
+
+		n = min(remaining, sectors_read*dev->blk_size-offset);
+		dev_dbg(&dev->sbd.core,
+			"%s:%u: copy %lu bytes from 0x%p to user 0x%p\n",
+			__func__, __LINE__, n, dev->bounce_buf+offset, buf);
+		if (copy_to_user(buf, dev->bounce_buf+offset, n)) {
+			mutex_unlock(&priv->mutex);
+			return -EFAULT;
+		}
+
+		mutex_unlock(&priv->mutex);
+
+		*pos += n;
+		buf += n;
+		remaining -= n;
+		start_sector += sectors_read;
+		offset = 0;
+	} while (remaining > 0);
+
+	return count;
+}
+
+static ssize_t ps3flash_write(struct file *file, const char __user *buf,
+			      size_t count, loff_t *pos)
+{
+	struct ps3_storage_device *dev = ps3flash_dev;
+	struct ps3flash_private *priv = ps3flash_priv(dev);
+	u64 size, chunk_sectors, start_write_sector, end_write_sector,
+	    end_read_sector, start_read_sector, head, tail, offset;
+	ssize_t res;
+	size_t remaining, n;
+
+	dev_dbg(&dev->sbd.core,
+		"%s:%u: Writing %zu bytes at position %lld from user 0x%p\n",
+		__func__, __LINE__, count, *pos, buf);
+
+	size = dev->regions[dev->region_idx].size*dev->blk_size;
+	if (*pos >= size || !count)
+		return 0;
+
+	if (*pos+count > size) {
+		dev_dbg(&dev->sbd.core,
+			"%s:%u Truncating count from %zu to %llu\n", __func__,
+			__LINE__, count, size-*pos);
+		count = size-*pos;
+	}
+
+	chunk_sectors = dev->bounce_size / dev->blk_size;
+
+	start_write_sector = do_div_llr(*pos, dev->bounce_size, &offset) *
+			     chunk_sectors;
+	end_write_sector = DIV_ROUND_UP(*pos+count, dev->bounce_size) *
+			   chunk_sectors;
+
+	end_read_sector = DIV_ROUND_UP(*pos, dev->blk_size);
+	start_read_sector = (*pos+count) / dev->blk_size;
+
+	/*
+	 * As we have to write in 256 KiB chunks, while we can read in blk_size
+	 * (usually 512 bytes) chunks, we perform the following steps:
+	 *   1. Read from start_write_sector to end_read_sector ("head")
+	 *   2. Read from start_read_sector to end_write_sector ("tail")
+	 *   3. Copy data to buffer
+	 *   4. Write from start_write_sector to end_write_sector
+	 * All of this is complicated by using only one 256 KiB bounce buffer.
+	 */
+
+	head = end_read_sector-start_write_sector;
+	tail = end_write_sector-start_read_sector;
+
+	remaining = count;
+	do {
+		mutex_lock(&priv->mutex);
+
+		if (end_read_sector >= start_read_sector) {
+			/* Merge head and tail */
+			dev_dbg(&dev->sbd.core,
+				"Merged head and tail: %lu sectors at %lu\n",
+				chunk_sectors, start_write_sector);
+			res = ps3flash_read_sectors(dev, start_write_sector,
+						    chunk_sectors, 0);
+			if (res < 0)
+				goto fail;
+		} else {
+			if (head) {
+				/* Read head */
+			dev_dbg(&dev->sbd.core, "head: %lu sectors at %lu\n",
+				head, start_write_sector);
+				res = ps3flash_read_sectors(dev,
+							    start_write_sector,
+							    head, 0);
+				if (res < 0)
+					goto fail;
+			}
+			if (start_read_sector <
+			    start_write_sector+chunk_sectors) {
+				/* Read tail */
+				dev_dbg(&dev->sbd.core,
+					"tail: %lu sectors at %lu\n", tail,
+					start_read_sector-start_write_sector);
+				res = ps3flash_read_sectors(dev,
+							    start_read_sector,
+							    tail,
+							    start_read_sector-start_write_sector);
+				if (res < 0)
+					goto fail;
+			}
+		}
+
+		n = min(remaining, dev->bounce_size-offset);
+		dev_dbg(&dev->sbd.core,
+			"%s:%u: copy %lu bytes from user 0x%p to 0x%p\n",
+			__func__, __LINE__, n, buf, dev->bounce_buf+offset);
+		if (copy_from_user(dev->bounce_buf+offset, buf, n)) {
+			res = -EFAULT;
+			goto fail;
+		}
+
+		res = ps3flash_write_chunk(dev, start_write_sector);
+		if (res < 0)
+			goto fail;
+
+		mutex_unlock(&priv->mutex);
+
+		*pos += n;
+		buf += n;
+		remaining -= n;
+		start_write_sector += chunk_sectors;
+		head = 0;
+		offset = 0;
+	} while (remaining > 0);
+
+	return count;
+
+fail:
+	mutex_unlock(&priv->mutex);
+	return res;
+}
+
+
+static const struct file_operations ps3flash_fops = {
+	.owner	= THIS_MODULE,
+	.llseek	= ps3flash_llseek,
+	.read	= ps3flash_read,
+	.write	= ps3flash_write,
+};
+
+static struct miscdevice ps3flash_misc = {
+	.minor	= MISC_DYNAMIC_MINOR,
+	.name	= DEVICE_NAME,
+	.fops	= &ps3flash_fops,
+};
+
+static int __devinit ps3flash_probe(struct ps3_system_bus_device *_dev)
+{
+	struct ps3_storage_device *dev = to_ps3_storage_device(&_dev->core);
+	struct ps3flash_private *priv;
+	int error;
+	unsigned long tmp;
+
+	tmp = dev->regions[dev->region_idx].start*dev->blk_size;
+	if (tmp % FLASH_BLOCK_SIZE) {
+		dev_err(&dev->sbd.core,
+			"%s:%u region start %lu is not aligned\n", __func__,
+			__LINE__, tmp);
+		return -EINVAL;
+	}
+	tmp = dev->regions[dev->region_idx].size*dev->blk_size;
+	if (tmp % FLASH_BLOCK_SIZE) {
+		dev_err(&dev->sbd.core,
+			"%s:%u region size %lu is not aligned\n", __func__,
+			__LINE__, tmp);
+		return -EINVAL;
+	}
+
+	/* use static buffer, kmalloc cannot allocate 256 KiB */
+	if (!ps3flash_bounce_buffer.address)
+		return -ENOMEM;
+
+	if (ps3flash_dev) {
+		dev_err(&dev->sbd.core,
+			"Only one FLASH device is supported\n");
+		return -EBUSY;
+	}
+
+	ps3flash_dev = dev;
+
+	priv = kzalloc(sizeof(*priv), GFP_KERNEL);
+	if (!priv) {
+		error = -ENOMEM;
+		goto fail;
+	}
+
+	ps3flash_priv(dev) = priv;
+	mutex_init(&priv->mutex);
+
+	dev->bounce_size = ps3flash_bounce_buffer.size;
+	dev->bounce_buf = ps3flash_bounce_buffer.address;
+
+	error = ps3stor_setup(dev, DEVICE_NAME);
+	if (error)
+		goto fail_free_priv;
+
+	error = misc_register(&ps3flash_misc);
+	if (error) {
+		dev_err(&dev->sbd.core, "%s:%u: misc_register failed %d\n",
+			__func__, __LINE__, error);
+		goto fail_teardown;
+	}
+
+	dev_info(&dev->sbd.core, "%s:%u: registered misc device %d\n",
+		 __func__, __LINE__, ps3flash_misc.minor);
+	return 0;
+
+fail_teardown:
+	ps3stor_teardown(dev);
+fail_free_priv:
+	kfree(priv);
+fail:
+	ps3flash_dev = NULL;
+	return error;
+}
+
+static int ps3flash_remove(struct ps3_system_bus_device *_dev)
+{
+	struct ps3_storage_device *dev = to_ps3_storage_device(&_dev->core);
+
+	misc_deregister(&ps3flash_misc);
+	ps3stor_teardown(dev);
+	kfree(ps3flash_priv(dev));
+	ps3flash_dev = NULL;
+	return 0;
+}
+
+
+static struct ps3_system_bus_driver ps3flash = {
+	.match_id	= PS3_MATCH_ID_STOR_FLASH,
+	.core.name	= DEVICE_NAME,
+	.core.owner	= THIS_MODULE,
+	.probe		= ps3flash_probe,
+	.remove		= ps3flash_remove,
+	.shutdown	= ps3flash_remove,
+};
+
+
+static int __init ps3flash_init(void)
+{
+	return ps3_system_bus_driver_register(&ps3flash, PS3_IOBUS_SB);
+}
+
+static void __exit ps3flash_exit(void)
+{
+	ps3_system_bus_driver_unregister(&ps3flash);
+}
+
+module_init(ps3flash_init);
+module_exit(ps3flash_exit);
+
+MODULE_LICENSE("GPL");
+MODULE_DESCRIPTION("PS3 FLASH ROM Storage Driver");
+MODULE_AUTHOR("Sony Corporation");

-- 
Gr{oetje,eeting}s,

						Geert

--
Geert Uytterhoeven -- Sony Network and Software Technology Center Europe (NSCE)
Geert.Uytterhoeven@sonycom.com ------- The Corporate Village, Da Vincilaan 7-D1
Voice +32-2-7008453 Fax +32-2-7008622 ---------------- B-1935 Zaventem, Belgium

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

[Christoph Hellwig]

On Fri, May 25, 2007 at 10:36:14AM +0200, Geert.Uytterhoeven@sonycom.com 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.

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

[Geert Uytterhoeven]

On Tue, 29 May 2007, Christoph Hellwig wrote:
> On Fri, May 25, 2007 at 10:36:14AM +0200, Geert.Uytterhoeven@sonycom.com 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

--
Geert Uytterhoeven -- Sony Network and Software Technology Center Europe (NSCE)
Geert.Uytterhoeven@sonycom.com ------- The Corporate Village, Da Vincilaan 7-D1
Voice +32-2-7008453 Fax +32-2-7008622 ---------------- B-1935 Zaventem, Belgium

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

[Christoph Hellwig]

On Tue, May 29, 2007 at 11:57:52AM +0200, Geert Uytterhoeven wrote:
> On Tue, 29 May 2007, Christoph Hellwig wrote:
> > On Fri, May 25, 2007 at 10:36:14AM +0200, Geert.Uytterhoeven@sonycom.com 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.

You're right, I missed that bit.