[patch 0/3] PS3 Storage Drivers for 2.6.23, take 5

[patch 0/3] PS3 Storage Drivers for 2.6.23, take 5

[Geert Uytterhoeven]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain, Size: 1391 bytes --]

	Hi,

These are the remaining patches of the new PS3 storage drivers series:
  [1] ps3: Disk Storage Driver
  [2] ps3: BD/DVD/CD-ROM Storage Driver
  [3] ps3: FLASH ROM Storage Driver

They are based on:
  - the current Linux kernel source tree,
  - plus the PS3 patches already submitted by Geoff Levand on linuxppc-dev,
    which are in the for-2.6.23 branch of Paulus' tree,
  - plus the first 3 patches from the PS3 storage drivers series I submitted
    before, which are also in the for-2.6.23 branch of Paulus' tree.

The only changes compared to the previous submission are the kmap updates, as
requested by Jens Axboe and James E.J. Bottomley.

I didn't hear anything from the misc device maintainer (for the FLASH ROM
Storage Driver).

Paulus, please apply.  Thanks in advance!

With kind regards,
 
Geert Uytterhoeven
Software Architect

Sony Network and Software Technology Center Europe
The Corporate Village · Da Vincilaan 7-D1 · B-1935 Zaventem · Belgium
 
Phone:    +32 (0)2 700 8453	
Fax:      +32 (0)2 700 8622	
E-mail:   Geert.Uytterhoeven@sonycom.com	
Internet: http://www.sony-europe.com/
 	
Sony Network and Software Technology Center Europe	
A division of Sony Service Centre (Europe) N.V.	
Registered office: Technologielaan 7 · B-1840 Londerzeel · Belgium	
VAT BE 0413.825.160 · RPR Brussels	
Fortis Bank Zaventem · Swift GEBABEBB08A · IBAN BE39001382358619

[patch 1/3] ps3: Disk Storage Driver

[Geert Uytterhoeven]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain, Size: 18961 bytes --]

From: Geert Uytterhoeven <Geert.Uytterhoeven@sonycom.com>

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

CC: Geoff Levand <geoffrey.levand@am.sony.com>
Signed-off-by: Geert Uytterhoeven <Geert.Uytterhoeven@sonycom.com>
Acked-by: Jens Axboe <jens.axboe@oracle.com>
---
v4:
  - Replace KM_USER0 by KM_IRQ0 (all kmap routines are either called from an
    interrupt handler or from .request_fn())
  - Add a call to flush_kernel_dcache_page() before kunmap in routines that
    write to buffers

v3:
  - Cleanup #includes
  - Kill PS3DISK_MAJOR, always use zero to obtain a dynamic major
  - Group LV1 API related stuff
  - Kill empty ps3disk_open
  - Reset ps3disk_priv(dev) to NULL during cleanup
  - Move call to put_disk() down
  - Pass the interrupt handler to ps3stor_setup(), so it doesn't have to be
    overridden later
  - Fold ps3disk_read_write_sectors() into ps3disk_submit_request_sg()
  - Identify the hard drive capacity and model name during probing

v2:
  - Don't use `default y' in Kconfig
  - Remove the thread for handling requests
  - Set up sysfs links between block device and PS3 system device:
      o /sys/block/ps3da/device -> ../../devices/ps3_system/sb_02
      o /sys/devices/ps3_system/sb_02/block:ps3da -> ../../../block/ps3da
  - Fix all FIXMEs
  - Implement proper barrier support
  - Call add_disk_randomness()
  - Add a test to verify we are running on a PS3
  - Fix error path in ps3disk_init()
  - Fix cleanup order in ps3disk_exit()

 arch/powerpc/platforms/ps3/Kconfig |   10 
 drivers/block/Makefile             |    1 
 drivers/block/ps3disk.c            |  624 +++++++++++++++++++++++++++++++++++++
 3 files changed, 635 insertions(+)

--- a/arch/powerpc/platforms/ps3/Kconfig
+++ b/arch/powerpc/platforms/ps3/Kconfig
@@ -102,4 +102,14 @@ config PS3_STORAGE
 	depends on PPC_PS3
 	tristate
 
+config PS3_DISK
+	tristate "PS3 Disk Storage Driver"
+	depends on PPC_PS3 && BLOCK
+	select PS3_STORAGE
+	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
@@ -28,3 +28,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,624 @@
+/*
+ * 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/ata.h>
+#include <linux/blkdev.h>
+
+#include <asm/lv1call.h>
+#include <asm/ps3stor.h>
+#include <asm/firmware.h>
+
+
+#define DEVICE_NAME		"ps3disk"
+
+#define BOUNCE_SIZE		(64*1024)
+
+#define PS3DISK_MAX_DISKS	16
+#define PS3DISK_MINORS		16
+
+#define KERNEL_SECTOR_SIZE	512
+
+
+#define PS3DISK_NAME		"ps3d%c"
+
+
+struct ps3disk_private {
+	spinlock_t lock;		/* Request queue spinlock */
+	struct request_queue *queue;
+	struct gendisk *gendisk;
+	unsigned int blocking_factor;
+	struct request *req;
+	u64 raw_capacity;
+	unsigned char model[ATA_ID_PROD_LEN+1];
+};
+#define ps3disk_priv(dev)	((dev)->sbd.core.driver_data)
+
+
+#define LV1_STORAGE_SEND_ATA_COMMAND	(2)
+#define LV1_STORAGE_ATA_HDDOUT		(0x23)
+
+struct lv1_ata_cmnd_block {
+	u16	features;
+	u16	sector_count;
+	u16	LBA_low;
+	u16	LBA_mid;
+	u16	LBA_high;
+	u8	device;
+	u8	command;
+	u32	is_ext;
+	u32	proto;
+	u32	in_out;
+	u32	size;
+	u64	buffer;
+	u32	arglen;
+};
+
+enum lv1_ata_proto {
+	NON_DATA_PROTO     = 0,
+	PIO_DATA_IN_PROTO  = 1,
+	PIO_DATA_OUT_PROTO = 2,
+	DMA_PROTO = 3
+};
+
+enum lv1_ata_in_out {
+	DIR_WRITE = 0,			/* memory -> device */
+	DIR_READ = 1			/* device -> memory */
+};
+
+static int ps3disk_major;
+
+
+static struct block_device_operations ps3disk_fops = {
+	.owner		= THIS_MODULE,
+};
+
+
+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_IRQ0);
+			if (gather)
+				memcpy(dev->bounce_buf+offset, buf, size);
+			else
+				memcpy(buf, dev->bounce_buf+offset, size);
+			offset += size;
+			flush_kernel_dcache_page(bio_iovec_idx(bio, j)->bv_page);
+			__bio_kunmap_atomic(bio, KM_IRQ0);
+		}
+		sectors += bio_sectors(bio);
+		i++;
+	}
+}
+
+static int ps3disk_submit_request_sg(struct ps3_storage_device *dev,
+				     struct request *req)
+{
+	struct ps3disk_private *priv = ps3disk_priv(dev);
+	int write = rq_data_dir(req), res;
+	const char *op = write ? "write" : "read";
+	u64 start_sector, sectors;
+	unsigned int region_id = dev->regions[dev->region_idx].id;
+
+#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
+
+	start_sector = req->sector*priv->blocking_factor;
+	sectors = req->nr_sectors*priv->blocking_factor;
+	dev_dbg(&dev->sbd.core, "%s:%u: %s %lu sectors starting at %lu\n",
+		__func__, __LINE__, op, sectors, start_sector);
+
+	if (write) {
+		ps3disk_scatter_gather(dev, req, 1);
+
+		res = lv1_storage_write(dev->sbd.dev_id, region_id,
+					start_sector, sectors, 0,
+					dev->bounce_lpar, &dev->tag);
+	} else {
+		res = lv1_storage_read(dev->sbd.dev_id, region_id,
+				       start_sector, sectors, 0,
+				       dev->bounce_lpar, &dev->tag);
+	}
+	if (res) {
+		dev_err(&dev->sbd.core, "%s:%u: %s failed %d\n", __func__,
+			__LINE__, op, res);
+		end_request(req, 0);
+		return 0;
+	}
+
+	priv->req = req;
+	return 1;
+}
+
+static int ps3disk_submit_flush_request(struct ps3_storage_device *dev,
+					struct request *req)
+{
+	struct ps3disk_private *priv = ps3disk_priv(dev);
+	u64 res;
+
+	dev_dbg(&dev->sbd.core, "%s:%u: flush request\n", __func__, __LINE__);
+
+	res = lv1_storage_send_device_command(dev->sbd.dev_id,
+					      LV1_STORAGE_ATA_HDDOUT, 0, 0, 0,
+					      0, &dev->tag);
+	if (res) {
+		dev_err(&dev->sbd.core, "%s:%u: sync cache failed 0x%lx\n",
+			__func__, __LINE__, res);
+		end_request(req, 0);
+		return 0;
+	}
+
+	priv->req = req;
+	return 1;
+}
+
+static void ps3disk_do_request(struct ps3_storage_device *dev,
+			       request_queue_t *q)
+{
+	struct request *req;
+
+	dev_dbg(&dev->sbd.core, "%s:%u\n", __func__, __LINE__);
+
+	while ((req = elv_next_request(q))) {
+		if (blk_fs_request(req)) {
+			if (ps3disk_submit_request_sg(dev, req))
+				break;
+		} else if (req->cmd_type == REQ_TYPE_FLUSH) {
+			if (ps3disk_submit_flush_request(dev, req))
+				break;
+		} else {
+			blk_dump_rq_flags(req, DEVICE_NAME " bad request");
+			end_request(req, 0);
+			continue;
+		}
+	}
+}
+
+static void ps3disk_request(request_queue_t *q)
+{
+	struct ps3_storage_device *dev = q->queuedata;
+	struct ps3disk_private *priv = ps3disk_priv(dev);
+
+	if (priv->req) {
+		dev_dbg(&dev->sbd.core, "%s:%u busy\n", __func__, __LINE__);
+		return;
+	}
+
+	ps3disk_do_request(dev, q);
+}
+
+static irqreturn_t ps3disk_interrupt(int irq, void *data)
+{
+	struct ps3_storage_device *dev = data;
+	struct ps3disk_private *priv;
+	struct request *req;
+	int res, read, uptodate;
+	u64 tag, status;
+	unsigned long num_sectors;
+	const char *op;
+
+	res = lv1_storage_get_async_status(dev->sbd.dev_id, &tag, &status);
+
+	if (tag != dev->tag)
+		dev_err(&dev->sbd.core,
+			"%s:%u: tag mismatch, got %lx, expected %lx\n",
+			__func__, __LINE__, tag, dev->tag);
+
+	if (res) {
+		dev_err(&dev->sbd.core, "%s:%u: res=%d status=0x%lx\n",
+			__func__, __LINE__, res, status);
+		return IRQ_HANDLED;
+	}
+
+	priv = ps3disk_priv(dev);
+	req = priv->req;
+	if (!req) {
+		dev_dbg(&dev->sbd.core,
+			"%s:%u non-block layer request completed\n", __func__,
+			__LINE__);
+		dev->lv1_status = status;
+		complete(&dev->done);
+		return IRQ_HANDLED;
+	}
+
+	if (req->cmd_type == REQ_TYPE_FLUSH) {
+		read = 0;
+		num_sectors = req->hard_cur_sectors;
+		op = "flush";
+	} else {
+		read = !rq_data_dir(req);
+		num_sectors = req->nr_sectors;
+		op = read ? "read" : "write";
+	}
+	if (status) {
+		dev_dbg(&dev->sbd.core, "%s:%u: %s failed 0x%lx\n", __func__,
+			__LINE__, op, status);
+		uptodate = 0;
+	} else {
+		dev_dbg(&dev->sbd.core, "%s:%u: %s completed\n", __func__,
+			__LINE__, op);
+		uptodate = 1;
+		if (read)
+			ps3disk_scatter_gather(dev, req, 0);
+	}
+
+	spin_lock(&priv->lock);
+	if (!end_that_request_first(req, uptodate, num_sectors)) {
+		add_disk_randomness(req->rq_disk);
+		blkdev_dequeue_request(req);
+		end_that_request_last(req, uptodate);
+	}
+	priv->req = NULL;
+	ps3disk_do_request(dev, priv->queue);
+	spin_unlock(&priv->lock);
+
+	return IRQ_HANDLED;
+}
+
+static int ps3disk_sync_cache(struct ps3_storage_device *dev)
+{
+	u64 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__, res);
+		return -EIO;
+	}
+	return 0;
+}
+
+
+/* ATA helpers copied from drivers/ata/libata-core.c */
+
+static void swap_buf_le16(u16 *buf, unsigned int buf_words)
+{
+#ifdef __BIG_ENDIAN
+	unsigned int i;
+
+	for (i = 0; i < buf_words; i++)
+		buf[i] = le16_to_cpu(buf[i]);
+#endif /* __BIG_ENDIAN */
+}
+
+static u64 ata_id_n_sectors(const u16 *id)
+{
+	if (ata_id_has_lba(id)) {
+		if (ata_id_has_lba48(id))
+			return ata_id_u64(id, 100);
+		else
+			return ata_id_u32(id, 60);
+	} else {
+		if (ata_id_current_chs_valid(id))
+			return ata_id_u32(id, 57);
+		else
+			return id[1] * id[3] * id[6];
+	}
+}
+
+static void ata_id_string(const u16 *id, unsigned char *s, unsigned int ofs,
+			  unsigned int len)
+{
+	unsigned int c;
+
+	while (len > 0) {
+		c = id[ofs] >> 8;
+		*s = c;
+		s++;
+
+		c = id[ofs] & 0xff;
+		*s = c;
+		s++;
+
+		ofs++;
+		len -= 2;
+	}
+}
+
+static void ata_id_c_string(const u16 *id, unsigned char *s, unsigned int ofs,
+			    unsigned int len)
+{
+	unsigned char *p;
+
+	WARN_ON(!(len & 1));
+
+	ata_id_string(id, s, ofs, len - 1);
+
+	p = s + strnlen(s, len - 1);
+	while (p > s && p[-1] == ' ')
+		p--;
+	*p = '\0';
+}
+
+static int ps3disk_identify(struct ps3_storage_device *dev)
+{
+	struct ps3disk_private *priv = ps3disk_priv(dev);
+	struct lv1_ata_cmnd_block ata_cmnd;
+	u16 *id = dev->bounce_buf;
+	u64 res;
+
+	dev_dbg(&dev->sbd.core, "%s:%u: identify disk\n", __func__, __LINE__);
+
+	memset(&ata_cmnd, 0, sizeof(struct lv1_ata_cmnd_block));
+	ata_cmnd.command = ATA_CMD_ID_ATA;
+	ata_cmnd.sector_count = 1;
+	ata_cmnd.size = ata_cmnd.arglen = ATA_ID_WORDS * 2;
+	ata_cmnd.buffer = dev->bounce_lpar;
+	ata_cmnd.proto = PIO_DATA_IN_PROTO;
+	ata_cmnd.in_out = DIR_READ;
+
+	res = ps3stor_send_command(dev, LV1_STORAGE_SEND_ATA_COMMAND,
+				   ps3_mm_phys_to_lpar(__pa(&ata_cmnd)),
+				   sizeof(ata_cmnd), ata_cmnd.buffer,
+				   ata_cmnd.arglen);
+	if (res) {
+		dev_err(&dev->sbd.core, "%s:%u: identify disk failed 0x%lx\n",
+			__func__, __LINE__, res);
+		return -EIO;
+	}
+
+	swap_buf_le16(id, ATA_ID_WORDS);
+
+	/* All we're interested in are raw capacity and model name */
+	priv->raw_capacity = ata_id_n_sectors(id);
+	ata_id_c_string(id, priv->model, ATA_ID_PROD, sizeof(priv->model));
+	return 0;
+}
+
+static void ps3disk_prepare_flush(request_queue_t *q, struct request *req)
+{
+	struct ps3_storage_device *dev = q->queuedata;
+
+	dev_dbg(&dev->sbd.core, "%s:%u\n", __func__, __LINE__);
+
+	memset(req->cmd, 0, sizeof(req->cmd));
+	req->cmd_type = REQ_TYPE_FLUSH;
+}
+
+static int ps3disk_issue_flush(request_queue_t *q, struct gendisk *gendisk,
+			       sector_t *sector)
+{
+	struct ps3_storage_device *dev = q->queuedata;
+	struct request *req;
+	int res;
+
+	dev_dbg(&dev->sbd.core, "%s:%u\n", __func__, __LINE__);
+
+	req = blk_get_request(q, WRITE, __GFP_WAIT);
+	ps3disk_prepare_flush(q, req);
+	res = blk_execute_rq(q, gendisk, req, 0);
+	if (res)
+		dev_err(&dev->sbd.core, "%s:%u: flush request failed %d\n",
+			__func__, __LINE__, res);
+	blk_put_request(req);
+	return res;
+}
+
+
+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;
+
+	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, ps3disk_interrupt);
+	if (error)
+		goto fail_free_bounce;
+
+	ps3disk_identify(dev);
+
+	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;
+	gendisk->driverfs_dev = &dev->sbd.core;
+	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);
+
+	dev_info(&dev->sbd.core,
+		 "%s is a %s (%lu MiB total, %lu MiB for OtherOS)\n",
+		 gendisk->disk_name, priv->model, priv->raw_capacity >> 11,
+		 get_capacity(gendisk) >> 11);
+
+	add_disk(gendisk);
+	return 0;
+
+fail_cleanup_queue:
+	blk_cleanup_queue(queue);
+fail_teardown:
+	ps3stor_teardown(dev);
+fail_free_bounce:
+	kfree(dev->bounce_buf);
+fail_free_priv:
+	kfree(priv);
+	ps3disk_priv(dev) = NULL;
+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);
+
+	__clear_bit(priv->gendisk->first_minor / PS3DISK_MINORS,
+		    &ps3disk_mask);
+	del_gendisk(priv->gendisk);
+	blk_cleanup_queue(priv->queue);
+	put_disk(priv->gendisk);
+	dev_notice(&dev->sbd.core, "Synchronizing disk cache\n");
+	ps3disk_sync_cache(dev);
+	ps3stor_teardown(dev);
+	kfree(dev->bounce_buf);
+	kfree(priv);
+	ps3disk_priv(dev) = NULL;
+	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;
+
+	if (!firmware_has_feature(FW_FEATURE_PS3_LV1))
+		return -ENODEV;
+
+	error = register_blkdev(0, DEVICE_NAME);
+	if (error <= 0) {
+		printk(KERN_ERR "%s:%u: register_blkdev failed %d\n", __func__,
+		       __LINE__, error);
+		return error;
+	}
+	ps3disk_major = error;
+
+	pr_info("%s:%u: registered block device major %d\n", __func__,
+		__LINE__, ps3disk_major);
+
+	error = ps3_system_bus_driver_register(&ps3disk);
+	if (error)
+		unregister_blkdev(ps3disk_major, DEVICE_NAME);
+
+	return error;
+}
+
+static void __exit ps3disk_exit(void)
+{
+	ps3_system_bus_driver_unregister(&ps3disk);
+	unregister_blkdev(ps3disk_major, DEVICE_NAME);
+}
+
+module_init(ps3disk_init);
+module_exit(ps3disk_exit);
+
+MODULE_LICENSE("GPL");
+MODULE_DESCRIPTION("PS3 Disk Storage Driver");
+MODULE_AUTHOR("Sony Corporation");
+MODULE_ALIAS(PS3_MODULE_ALIAS_STOR_DISK);

-- 
With kind regards,
 
Geert Uytterhoeven
Software Architect

Sony Network and Software Technology Center Europe
The Corporate Village · Da Vincilaan 7-D1 · B-1935 Zaventem · Belgium
 
Phone:    +32 (0)2 700 8453	
Fax:      +32 (0)2 700 8622	
E-mail:   Geert.Uytterhoeven@sonycom.com	
Internet: http://www.sony-europe.com/
 	
Sony Network and Software Technology Center Europe	
A division of Sony Service Centre (Europe) N.V.	
Registered office: Technologielaan 7 · B-1840 Londerzeel · Belgium	
VAT BE 0413.825.160 · RPR Brussels	
Fortis Bank Zaventem · Swift GEBABEBB08A · IBAN BE39001382358619

Re: [patch 1/3] ps3: Disk Storage Driver

[Jan Engelhardt]

On Jul 16 2007 18:15, Geert Uytterhoeven wrote:
>
>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

I wonder what virtualization has to do with a block device driver?


	Jan
-- 

Re: [patch 1/3] ps3: Disk Storage Driver

[Geert Uytterhoeven]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 1170 bytes --]

On Wed, 18 Jul 2007, Jan Engelhardt wrote:
> On Jul 16 2007 18:15, Geert Uytterhoeven wrote:
> >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
> 
> I wonder what virtualization has to do with a block device driver?

On the PS3, storage devices are virtualized by the hypervisor (yes, the PS3
runs all OSes under a hypervisor).

With kind regards,
 
Geert Uytterhoeven
Software Architect

Sony Network and Software Technology Center Europe
The Corporate Village · Da Vincilaan 7-D1 · B-1935 Zaventem · Belgium
 
Phone:    +32 (0)2 700 8453	
Fax:      +32 (0)2 700 8622	
E-mail:   Geert.Uytterhoeven@sonycom.com	
Internet: http://www.sony-europe.com/
 	
Sony Network and Software Technology Center Europe	
A division of Sony Service Centre (Europe) N.V.	
Registered office: Technologielaan 7 · B-1840 Londerzeel · Belgium	
VAT BE 0413.825.160 · RPR Brussels	
Fortis Bank Zaventem · Swift GEBABEBB08A · IBAN BE39001382358619

Re: [patch 1/3] ps3: Disk Storage Driver

[Andrew Morton]

On Mon, 16 Jul 2007 18:15:40 +0200
Geert Uytterhoeven <Geert.Uytterhoeven@sonycom.com> wrote:

> From: Geert Uytterhoeven <Geert.Uytterhoeven@sonycom.com>
> 
> Add a Disk Storage Driver for the PS3:

Your patchset significantly hits powerpc, scsi and block.  So who gets to
merge this?  Jens?  James?  Paul?

Me, I guess ;)

>   - 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
> 
> ...
>
> --- /dev/null
> +++ b/drivers/block/ps3disk.c
> @@ -0,0 +1,624 @@
> +/*
> + * 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/ata.h>
> +#include <linux/blkdev.h>
> +
> +#include <asm/lv1call.h>
> +#include <asm/ps3stor.h>
> +#include <asm/firmware.h>
> +
> +
> +#define DEVICE_NAME		"ps3disk"
> +
> +#define BOUNCE_SIZE		(64*1024)
> +
> +#define PS3DISK_MAX_DISKS	16
> +#define PS3DISK_MINORS		16
> +
> +#define KERNEL_SECTOR_SIZE	512

Sigh.  We have at least ten separate definitions of SECTOR_SIZE< none of
them in the right place.  Cleanup opportunity for someone.

> +
> +#define PS3DISK_NAME		"ps3d%c"
> +
> +
> +struct ps3disk_private {
> +	spinlock_t lock;		/* Request queue spinlock */
> +	struct request_queue *queue;
> +	struct gendisk *gendisk;
> +	unsigned int blocking_factor;
> +	struct request *req;
> +	u64 raw_capacity;
> +	unsigned char model[ATA_ID_PROD_LEN+1];
> +};
> +#define ps3disk_priv(dev)	((dev)->sbd.core.driver_data)

hm, this code has "ata" all over it and actually uses a libata #define (at
least) but there is no Kconfig dependency on ATA.  Fair enough, I guess,
but a bit funny-looking.

> +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_IRQ0);
> +			if (gather)
> +				memcpy(dev->bounce_buf+offset, buf, size);
> +			else
> +				memcpy(buf, dev->bounce_buf+offset, size);
> +			offset += size;
> +			flush_kernel_dcache_page(bio_iovec_idx(bio, j)->bv_page);
> +			__bio_kunmap_atomic(bio, KM_IRQ0);
> +		}
> +		sectors += bio_sectors(bio);
> +		i++;
> +	}
> +}

Local variable `sectors' doesn't do anything.

> +static int ps3disk_submit_request_sg(struct ps3_storage_device *dev,
> +				     struct request *req)
> +{
> +	struct ps3disk_private *priv = ps3disk_priv(dev);
> +	int write = rq_data_dir(req), res;
> +	const char *op = write ? "write" : "read";
> +	u64 start_sector, sectors;
> +	unsigned int region_id = dev->regions[dev->region_idx].id;

So we're ignoring the sector_t stuff.  I guess it's 64-bit only.  Still, it
might be nice to use sector_t for consistency, readability and possible
future 32-bitness?

> +#ifdef DEBUG
> +	unsigned int n = 0;
> +	struct bio *bio;
> +	rq_for_each_bio(bio, req)
> +		n++;

I'm surprised that the block core doesn't have a helper to count the number
of bios in a request.

Please prefer to put a blank line between end-of-locals and start-of-code.

> +	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
> +
> +	start_sector = req->sector*priv->blocking_factor;
> +	sectors = req->nr_sectors*priv->blocking_factor;

s/*/ * /.  checkpatch missed this.

I suspect you didn't run cehckpatch anyway.

Please run checkpatch.

> +	dev_dbg(&dev->sbd.core, "%s:%u: %s %lu sectors starting at %lu\n",
> +		__func__, __LINE__, op, sectors, start_sector);
> +
> +	if (write) {
> +		ps3disk_scatter_gather(dev, req, 1);
> +
> +		res = lv1_storage_write(dev->sbd.dev_id, region_id,
> +					start_sector, sectors, 0,
> +					dev->bounce_lpar, &dev->tag);
> +	} else {
> +		res = lv1_storage_read(dev->sbd.dev_id, region_id,
> +				       start_sector, sectors, 0,
> +				       dev->bounce_lpar, &dev->tag);
> +	}
> +	if (res) {
> +		dev_err(&dev->sbd.core, "%s:%u: %s failed %d\n", __func__,
> +			__LINE__, op, res);
> +		end_request(req, 0);
> +		return 0;
> +	}
> +
> +	priv->req = req;
> +	return 1;
> +}
> +
>
> ...
>
> +
> +/* ATA helpers copied from drivers/ata/libata-core.c */

ooh, bad person.

> +static void swap_buf_le16(u16 *buf, unsigned int buf_words)
> +{
> +#ifdef __BIG_ENDIAN
> +	unsigned int i;
> +
> +	for (i = 0; i < buf_words; i++)
> +		buf[i] = le16_to_cpu(buf[i]);
> +#endif /* __BIG_ENDIAN */
> +}
> +
> +static u64 ata_id_n_sectors(const u16 *id)
> +{
> +	if (ata_id_has_lba(id)) {
> +		if (ata_id_has_lba48(id))
> +			return ata_id_u64(id, 100);
> +		else
> +			return ata_id_u32(id, 60);
> +	} else {
> +		if (ata_id_current_chs_valid(id))
> +			return ata_id_u32(id, 57);
> +		else
> +			return id[1] * id[3] * id[6];
> +	}
> +}
> +
> +static void ata_id_string(const u16 *id, unsigned char *s, unsigned int ofs,
> +			  unsigned int len)
> +{
> +	unsigned int c;
> +
> +	while (len > 0) {
> +		c = id[ofs] >> 8;
> +		*s = c;
> +		s++;
> +
> +		c = id[ofs] & 0xff;
> +		*s = c;
> +		s++;
> +
> +		ofs++;
> +		len -= 2;
> +	}
> +}
> +
> +static void ata_id_c_string(const u16 *id, unsigned char *s, unsigned int ofs,
> +			    unsigned int len)
> +{
> +	unsigned char *p;
> +
> +	WARN_ON(!(len & 1));
> +
> +	ata_id_string(id, s, ofs, len - 1);
> +
> +	p = s + strnlen(s, len - 1);
> +	while (p > s && p[-1] == ' ')
> +		p--;
> +	*p = '\0';
> +}
> +
>
> ...
>
> +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;
> +
> +	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, ps3disk_interrupt);
> +	if (error)
> +		goto fail_free_bounce;
> +
> +	ps3disk_identify(dev);

ps3disk_identify() can return an error?

> +	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;
> +	gendisk->driverfs_dev = &dev->sbd.core;
> +	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);
> +
> +	dev_info(&dev->sbd.core,
> +		 "%s is a %s (%lu MiB total, %lu MiB for OtherOS)\n",
> +		 gendisk->disk_name, priv->model, priv->raw_capacity >> 11,
> +		 get_capacity(gendisk) >> 11);
> +
> +	add_disk(gendisk);
> +	return 0;
> +
> +fail_cleanup_queue:
> +	blk_cleanup_queue(queue);
> +fail_teardown:
> +	ps3stor_teardown(dev);
> +fail_free_bounce:
> +	kfree(dev->bounce_buf);
> +fail_free_priv:
> +	kfree(priv);
> +	ps3disk_priv(dev) = NULL;
> +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);
> +
> +	__clear_bit(priv->gendisk->first_minor / PS3DISK_MINORS,
> +		    &ps3disk_mask);

I see no locking here which makes this __clear_bit and the above __set_bit
non-racy?

> +	del_gendisk(priv->gendisk);
> +	blk_cleanup_queue(priv->queue);
> +	put_disk(priv->gendisk);
> +	dev_notice(&dev->sbd.core, "Synchronizing disk cache\n");
> +	ps3disk_sync_cache(dev);
> +	ps3stor_teardown(dev);
> +	kfree(dev->bounce_buf);
> +	kfree(priv);
> +	ps3disk_priv(dev) = NULL;

I suspect this nulling here will just hide any bugs?  If we're going to
write anything there, probably 0xdeadbeef would be better?

> +	return 0;
> +}
> +

Re: [patch 1/3] ps3: Disk Storage Driver

[Jens Axboe]

On Wed, Jul 18 2007, Andrew Morton wrote:
> On Mon, 16 Jul 2007 18:15:40 +0200
> Geert Uytterhoeven <Geert.Uytterhoeven@sonycom.com> wrote:
> 
> > From: Geert Uytterhoeven <Geert.Uytterhoeven@sonycom.com>
> > 
> > Add a Disk Storage Driver for the PS3:
> 
> Your patchset significantly hits powerpc, scsi and block.  So who gets to
> merge this?  Jens?  James?  Paul?
> 
> Me, I guess ;)

I think Paul was going to take it, or at least Geert hinted as such.

> > +#define PS3DISK_MAX_DISKS	16
> > +#define PS3DISK_MINORS		16
> > +
> > +#define KERNEL_SECTOR_SIZE	512
> 
> Sigh.  We have at least ten separate definitions of SECTOR_SIZE< none of
> them in the right place.  Cleanup opportunity for someone.

Indeed, it's universally 512 or << 9, just use that...

> > +#ifdef DEBUG
> > +	unsigned int n = 0;
> > +	struct bio *bio;
> > +	rq_for_each_bio(bio, req)
> > +		n++;
> 
> I'm surprised that the block core doesn't have a helper to count the number
> of bios in a request.

What would be the point of such a helper? I've never seen a need for it.
Geert uses it as debug code here, but the fact is that the number of
bios in a request is a pretty pointless number. It doesn't tell you
anything. There's no 1:1 mapping between bios and segments (or anything
else for that matter), so the exercise is purely pointless.

-- 
Jens Axboe

Re: [patch 1/3] ps3: Disk Storage Driver

[Geert Uytterhoeven]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 1215 bytes --]

On Thu, 19 Jul 2007, Jens Axboe wrote:
> On Wed, Jul 18 2007, Andrew Morton wrote:
> > On Mon, 16 Jul 2007 18:15:40 +0200
> > Geert Uytterhoeven <Geert.Uytterhoeven@sonycom.com> wrote:
> > 
> > > From: Geert Uytterhoeven <Geert.Uytterhoeven@sonycom.com>
> > > 
> > > Add a Disk Storage Driver for the PS3:
> > 
> > Your patchset significantly hits powerpc, scsi and block.  So who gets to
> > merge this?  Jens?  James?  Paul?
> > 
> > Me, I guess ;)
> 
> I think Paul was going to take it, or at least Geert hinted as such.

Yep, but as I heard Paul is on holidays, I was just going to send it to Andrew
anyway.

With kind regards,
 
Geert Uytterhoeven
Software Architect

Sony Network and Software Technology Center Europe
The Corporate Village · Da Vincilaan 7-D1 · B-1935 Zaventem · Belgium
 
Phone:    +32 (0)2 700 8453	
Fax:      +32 (0)2 700 8622	
E-mail:   Geert.Uytterhoeven@sonycom.com	
Internet: http://www.sony-europe.com/
 	
Sony Network and Software Technology Center Europe	
A division of Sony Service Centre (Europe) N.V.	
Registered office: Technologielaan 7 · B-1840 Londerzeel · Belgium	
VAT BE 0413.825.160 · RPR Brussels	
Fortis Bank Zaventem · Swift GEBABEBB08A · IBAN BE39001382358619

Re: [patch 1/3] ps3: Disk Storage Driver

[Geert Uytterhoeven]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 5237 bytes --]

	Hi Andrew,

On Wed, 18 Jul 2007, Andrew Morton wrote:
> On Mon, 16 Jul 2007 18:15:40 +0200
> Geert Uytterhoeven <Geert.Uytterhoeven@sonycom.com> wrote:
> 
> > From: Geert Uytterhoeven <Geert.Uytterhoeven@sonycom.com>
> > 
> > Add a Disk Storage Driver for the PS3:
> 
> Your patchset significantly hits powerpc, scsi and block.  So who gets to
> merge this?  Jens?  James?  Paul?
> 
> Me, I guess ;)

As Paul is on holidays, please take it. The PS3 storage driver core support is
already in mainline, but the actual drivers aren't, as Paul was waiting for
acks from the maintainers.

BTW, do you prefer incremental patches for the comments below, or an update of
the full patchset?

> > +#define KERNEL_SECTOR_SIZE	512
> 
> Sigh.  We have at least ten separate definitions of SECTOR_SIZE< none of
> them in the right place.  Cleanup opportunity for someone.

Will replace by 512 resp. >> 9, as per Jens' suggestion.

> > +struct ps3disk_private {
> > +	spinlock_t lock;		/* Request queue spinlock */
> > +	struct request_queue *queue;
> > +	struct gendisk *gendisk;
> > +	unsigned int blocking_factor;
> > +	struct request *req;
> > +	u64 raw_capacity;
> > +	unsigned char model[ATA_ID_PROD_LEN+1];
> > +};
> > +#define ps3disk_priv(dev)	((dev)->sbd.core.driver_data)
> 
> hm, this code has "ata" all over it and actually uses a libata #define (at
> least) but there is no Kconfig dependency on ATA.  Fair enough, I guess,
> but a bit funny-looking.

I needed the definitions just for drive identification.

> > +static void ps3disk_scatter_gather(struct ps3_storage_device *dev,
> > +				   struct request *req, int gather)
> > +{
> > +	unsigned int sectors = 0, offset = 0;

> Local variable `sectors' doesn't do anything.

Good catch!

> > +static int ps3disk_submit_request_sg(struct ps3_storage_device *dev,
> > +				     struct request *req)
> > +{
> > +	struct ps3disk_private *priv = ps3disk_priv(dev);
> > +	int write = rq_data_dir(req), res;
> > +	const char *op = write ? "write" : "read";
> > +	u64 start_sector, sectors;
> > +	unsigned int region_id = dev->regions[dev->region_idx].id;
> 
> So we're ignoring the sector_t stuff.  I guess it's 64-bit only.  Still, it
> might be nice to use sector_t for consistency, readability and possible
> future 32-bitness?

I used u64 as they're directly passed to hypervisor calls and that's what the
hypervisor calls take.

BTW, Cell will never support a 32-bit kernel, as lots of on-chip stuff lies
outside the 32-bit address space.

> > +#ifdef DEBUG
> > +	unsigned int n = 0;
> > +	struct bio *bio;
> > +	rq_for_each_bio(bio, req)
> > +		n++;
> 
> I'm surprised that the block core doesn't have a helper to count the number
> of bios in a request.
> 
> Please prefer to put a blank line between end-of-locals and start-of-code.

Done.

> > +	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
> > +
> > +	start_sector = req->sector*priv->blocking_factor;
> > +	sectors = req->nr_sectors*priv->blocking_factor;
> 
> s/*/ * /.  checkpatch missed this.

Will change (yes, checkpatch missed it).

> I suspect you didn't run cehckpatch anyway.

I did ;-)

> Please run checkpatch.

All it complains about is (bogus) <asm/firmware.h> and a single too long line I
don't want to break.

> > +/* ATA helpers copied from drivers/ata/libata-core.c */
> 
> ooh, bad person.

I didn't have much choice, as most of it was static and I don't need the full
libata core anyway.

If I would factor it out, any good suggestion where to put the factored out
code?

> > +	ps3disk_identify(dev);
> 
> ps3disk_identify() can return an error?

Sending storage commands may fail, but being unable to identify the disk isn't
fatal, as it's just for informational purposes.

> > +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);
> > +
> > +	__clear_bit(priv->gendisk->first_minor / PS3DISK_MINORS,
> > +		    &ps3disk_mask);
> 
> I see no locking here which makes this __clear_bit and the above __set_bit
> non-racy?

Were .probe()/.remove() made concurrent again? I thought that idea was dropped
because it caused too many problems?

> > +	kfree(priv);
> > +	ps3disk_priv(dev) = NULL;
> 
> I suspect this nulling here will just hide any bugs?  If we're going to
> write anything there, probably 0xdeadbeef would be better?

Hmm, earlier reviewers explicitly asked for putting it there...

Thanks for your comments!

With kind regards,
 
Geert Uytterhoeven
Software Architect

Sony Network and Software Technology Center Europe
The Corporate Village · Da Vincilaan 7-D1 · B-1935 Zaventem · Belgium
 
Phone:    +32 (0)2 700 8453	
Fax:      +32 (0)2 700 8622	
E-mail:   Geert.Uytterhoeven@sonycom.com	
Internet: http://www.sony-europe.com/
 	
Sony Network and Software Technology Center Europe	
A division of Sony Service Centre (Europe) N.V.	
Registered office: Technologielaan 7 · B-1840 Londerzeel · Belgium	
VAT BE 0413.825.160 · RPR Brussels	
Fortis Bank Zaventem · Swift GEBABEBB08A · IBAN BE39001382358619

Re: [patch 1/3] ps3: Disk Storage Driver

[Andrew Morton]

On Thu, 19 Jul 2007 10:57:53 +0200 (CEST) Geert Uytterhoeven <Geert.Uytterhoeven@sonycom.com> wrote:

> 	Hi Andrew,
> 
> On Wed, 18 Jul 2007, Andrew Morton wrote:
> > On Mon, 16 Jul 2007 18:15:40 +0200
> > Geert Uytterhoeven <Geert.Uytterhoeven@sonycom.com> wrote:
> > 
> > > From: Geert Uytterhoeven <Geert.Uytterhoeven@sonycom.com>
> > > 
> > > Add a Disk Storage Driver for the PS3:
> > 
> > Your patchset significantly hits powerpc, scsi and block.  So who gets to
> > merge this?  Jens?  James?  Paul?
> > 
> > Me, I guess ;)
> 
> As Paul is on holidays, please take it.

OK.

> The PS3 storage driver core support is
> already in mainline, but the actual drivers aren't, as Paul was waiting for
> acks from the maintainers.
> 
> BTW, do you prefer incremental patches for the comments below, or an update of
> the full patchset?

Incremental is preferred, but I convert replacement patches into
incremental patches at about 10Hz nowadays.  Whatever's easier.

(Actually, if it's a replacement patch then only I get to see the
incremental patch, and the incremental patch is more reviewer-friendly).

> I didn't have much choice, as most of it was static and I don't need the full
> libata core anyway.
> 
> If I would factor it out, any good suggestion where to put the factored out
> code?

Take it up with Jeff, please.  If you're keen.  It isn't a lot of code.

> 
> > > +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);
> > > +
> > > +	__clear_bit(priv->gendisk->first_minor / PS3DISK_MINORS,
> > > +		    &ps3disk_mask);
> > 
> > I see no locking here which makes this __clear_bit and the above __set_bit
> > non-racy?
> 
> Were .probe()/.remove() made concurrent again? I thought that idea was dropped
> because it caused too many problems?

I don't _think_ there's any global exclusion on ->probe calls.  For a
particular driver instance it's hard to see how these thigns can run
concurrently, dunno.  I guess two hotunplugs coud happen concurrently.

[patch 0/4] PS3 storage driver updates (was: Re: [patch 1/3] ps3: Disk Storage Driver)

[Geert Uytterhoeven]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 1912 bytes --]

	Hi Andrew,

On Thu, 19 Jul 2007, Andrew Morton wrote:
> On Thu, 19 Jul 2007 10:57:53 +0200 (CEST) Geert Uytterhoeven <Geert.Uytterhoeven@sonycom.com> wrote:
> > On Wed, 18 Jul 2007, Andrew Morton wrote:
> > > Your patchset significantly hits powerpc, scsi and block.  So who gets to
> > > merge this?  Jens?  James?  Paul?
> > > 
> > > Me, I guess ;)
> > 
> > As Paul is on holidays, please take it.
> 
> OK.
> 
> > The PS3 storage driver core support is
> > already in mainline, but the actual drivers aren't, as Paul was waiting for
> > acks from the maintainers.
> > 
> > BTW, do you prefer incremental patches for the comments below, or an update of
> > the full patchset?
> 
> Incremental is preferred, but I convert replacement patches into
> incremental patches at about 10Hz nowadays.  Whatever's easier.
> 
> (Actually, if it's a replacement patch then only I get to see the
> incremental patch, and the incremental patch is more reviewer-friendly).

Here are the updates:
  [1] ps3disk: use correct bio vector size
  [2] ps3disk: updates after final review
  [3] ps3rom: updates after final review
  [4] ps3flash: updates after final review

The first one is the bugfix for O_DIRECT, the other 3 patches are the updates
in response to your review.

Thanks for integrating (and forwarding to Linus)!

With kind regards,
 
Geert Uytterhoeven
Software Architect

Sony Network and Software Technology Center Europe
The Corporate Village · Da Vincilaan 7-D1 · B-1935 Zaventem · Belgium
 
Phone:    +32 (0)2 700 8453	
Fax:      +32 (0)2 700 8622	
E-mail:   Geert.Uytterhoeven@sonycom.com	
Internet: http://www.sony-europe.com/
 	
Sony Network and Software Technology Center Europe	
A division of Sony Service Centre (Europe) N.V.	
Registered office: Technologielaan 7 · B-1840 Londerzeel · Belgium	
VAT BE 0413.825.160 · RPR Brussels	
Fortis Bank Zaventem · Swift GEBABEBB08A · IBAN BE39001382358619

[patch 1/4] ps3disk: use correct bio vector size

[Geert Uytterhoeven]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 1322 bytes --]

ps3disk: use correct bio vector size

This fixes the O_DIRECT corruptions, as reported by Olaf Hering (triggered by
e.g. parted >= 1.8.0).

Signed-off-by: Geert Uytterhoeven <Geert.Uytterhoeven@sonycom.com>
---
 drivers/block/ps3disk.c |    2 +-
 1 files changed, 1 insertion(+), 1 deletion(-)

--- a/drivers/block/ps3disk.c
+++ b/drivers/block/ps3disk.c
@@ -108,7 +108,7 @@ static void ps3disk_scatter_gather(struc
 			__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;
+			size = bvec->bv_len;
 			buf = __bio_kmap_atomic(bio, j, KM_IRQ0);
 			if (gather)
 				memcpy(dev->bounce_buf+offset, buf, size);

With kind regards,
 
Geert Uytterhoeven
Software Architect

Sony Network and Software Technology Center Europe
The Corporate Village · Da Vincilaan 7-D1 · B-1935 Zaventem · Belgium
 
Phone:    +32 (0)2 700 8453	
Fax:      +32 (0)2 700 8622	
E-mail:   Geert.Uytterhoeven@sonycom.com	
Internet: http://www.sony-europe.com/
 	
Sony Network and Software Technology Center Europe	
A division of Sony Service Centre (Europe) N.V.	
Registered office: Technologielaan 7 · B-1840 Londerzeel · Belgium	
VAT BE 0413.825.160 · RPR Brussels	
Fortis Bank Zaventem · Swift GEBABEBB08A · IBAN BE39001382358619

[patch 2/4] ps3disk: updates after final review

[Geert Uytterhoeven]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 7440 bytes --]

ps3disk: updates after final review:
  o Kill KERNEL_SECTOR_SIZE, just hardcode `512' or `>> 9'
  o Kill confusing ps3disk_priv() macro, open code it instead
  o ps3disk_scatter_gather(): Kill unused variable  `sectors'
  o Introduce ps3disk_mask_mutex to protect ps3disk_mask
  o Minor coding style fixes not reported by checkpatch

Signed-off-by: Geert Uytterhoeven <Geert.Uytterhoeven@sonycom.com>
---
 drivers/block/ps3disk.c |   44 +++++++++++++++++++++++++-------------------
 1 files changed, 25 insertions(+), 19 deletions(-)

--- a/drivers/block/ps3disk.c
+++ b/drivers/block/ps3disk.c
@@ -33,8 +33,6 @@
 #define PS3DISK_MAX_DISKS	16
 #define PS3DISK_MINORS		16
 
-#define KERNEL_SECTOR_SIZE	512
-
 
 #define PS3DISK_NAME		"ps3d%c"
 
@@ -48,7 +46,6 @@ struct ps3disk_private {
 	u64 raw_capacity;
 	unsigned char model[ATA_ID_PROD_LEN+1];
 };
-#define ps3disk_priv(dev)	((dev)->sbd.core.driver_data)
 
 
 #define LV1_STORAGE_SEND_ATA_COMMAND	(2)
@@ -93,7 +90,7 @@ static struct block_device_operations ps
 static void ps3disk_scatter_gather(struct ps3_storage_device *dev,
 				   struct request *req, int gather)
 {
-	unsigned int sectors = 0, offset = 0;
+	unsigned int offset = 0;
 	struct bio *bio;
 	sector_t sector;
 	struct bio_vec *bvec;
@@ -118,7 +115,6 @@ static void ps3disk_scatter_gather(struc
 			flush_kernel_dcache_page(bio_iovec_idx(bio, j)->bv_page);
 			__bio_kunmap_atomic(bio, KM_IRQ0);
 		}
-		sectors += bio_sectors(bio);
 		i++;
 	}
 }
@@ -126,7 +122,7 @@ static void ps3disk_scatter_gather(struc
 static int ps3disk_submit_request_sg(struct ps3_storage_device *dev,
 				     struct request *req)
 {
-	struct ps3disk_private *priv = ps3disk_priv(dev);
+	struct ps3disk_private *priv = dev->sbd.core.driver_data;
 	int write = rq_data_dir(req), res;
 	const char *op = write ? "write" : "read";
 	u64 start_sector, sectors;
@@ -135,6 +131,7 @@ static int ps3disk_submit_request_sg(str
 #ifdef DEBUG
 	unsigned int n = 0;
 	struct bio *bio;
+
 	rq_for_each_bio(bio, req)
 		n++;
 	dev_dbg(&dev->sbd.core,
@@ -143,8 +140,8 @@ static int ps3disk_submit_request_sg(str
 		req->hard_nr_sectors);
 #endif
 
-	start_sector = req->sector*priv->blocking_factor;
-	sectors = req->nr_sectors*priv->blocking_factor;
+	start_sector = req->sector * priv->blocking_factor;
+	sectors = req->nr_sectors * priv->blocking_factor;
 	dev_dbg(&dev->sbd.core, "%s:%u: %s %lu sectors starting at %lu\n",
 		__func__, __LINE__, op, sectors, start_sector);
 
@@ -173,7 +170,7 @@ static int ps3disk_submit_request_sg(str
 static int ps3disk_submit_flush_request(struct ps3_storage_device *dev,
 					struct request *req)
 {
-	struct ps3disk_private *priv = ps3disk_priv(dev);
+	struct ps3disk_private *priv = dev->sbd.core.driver_data;
 	u64 res;
 
 	dev_dbg(&dev->sbd.core, "%s:%u: flush request\n", __func__, __LINE__);
@@ -217,7 +214,7 @@ static void ps3disk_do_request(struct ps
 static void ps3disk_request(request_queue_t *q)
 {
 	struct ps3_storage_device *dev = q->queuedata;
-	struct ps3disk_private *priv = ps3disk_priv(dev);
+	struct ps3disk_private *priv = dev->sbd.core.driver_data;
 
 	if (priv->req) {
 		dev_dbg(&dev->sbd.core, "%s:%u busy\n", __func__, __LINE__);
@@ -250,7 +247,7 @@ static irqreturn_t ps3disk_interrupt(int
 		return IRQ_HANDLED;
 	}
 
-	priv = ps3disk_priv(dev);
+	priv = dev->sbd.core.driver_data;
 	req = priv->req;
 	if (!req) {
 		dev_dbg(&dev->sbd.core,
@@ -374,7 +371,7 @@ static void ata_id_c_string(const u16 *i
 
 static int ps3disk_identify(struct ps3_storage_device *dev)
 {
-	struct ps3disk_private *priv = ps3disk_priv(dev);
+	struct ps3disk_private *priv = dev->sbd.core.driver_data;
 	struct lv1_ata_cmnd_block ata_cmnd;
 	u16 *id = dev->bounce_buf;
 	u64 res;
@@ -439,6 +436,8 @@ static int ps3disk_issue_flush(request_q
 
 static unsigned long ps3disk_mask;
 
+static DEFINE_MUTEX(ps3disk_mask_mutex);
+
 static int __devinit ps3disk_probe(struct ps3_system_bus_device *_dev)
 {
 	struct ps3_storage_device *dev = to_ps3_storage_device(&_dev->core);
@@ -448,7 +447,7 @@ static int __devinit ps3disk_probe(struc
 	struct request_queue *queue;
 	struct gendisk *gendisk;
 
-	if (dev->blk_size < KERNEL_SECTOR_SIZE) {
+	if (dev->blk_size < 512) {
 		dev_err(&dev->sbd.core,
 			"%s:%u: cannot handle block size %lu\n", __func__,
 			__LINE__, dev->blk_size);
@@ -456,13 +455,16 @@ static int __devinit ps3disk_probe(struc
 	}
 
 	BUILD_BUG_ON(PS3DISK_MAX_DISKS > BITS_PER_LONG);
+	mutex_lock(&ps3disk_mask_mutex);
 	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__);
+		mutex_unlock(&ps3disk_mask_mutex);
 		return -ENOSPC;
 	}
 	__set_bit(devidx, &ps3disk_mask);
+	mutex_unlock(&ps3disk_mask_mutex);
 
 	priv = kzalloc(sizeof(*priv), GFP_KERNEL);
 	if (!priv) {
@@ -470,7 +472,7 @@ static int __devinit ps3disk_probe(struc
 		goto fail;
 	}
 
-	ps3disk_priv(dev) = priv;
+	dev->sbd.core.driver_data = priv;
 	spin_lock_init(&priv->lock);
 
 	dev->bounce_size = BOUNCE_SIZE;
@@ -499,7 +501,7 @@ static int __devinit ps3disk_probe(struc
 
 	blk_queue_bounce_limit(queue, BLK_BOUNCE_HIGH);
 
-	blk_queue_max_sectors(queue, dev->bounce_size/KERNEL_SECTOR_SIZE);
+	blk_queue_max_sectors(queue, dev->bounce_size >> 9);
 	blk_queue_segment_boundary(queue, -1UL);
 	blk_queue_dma_alignment(queue, dev->blk_size-1);
 	blk_queue_hardsect_size(queue, dev->blk_size);
@@ -529,7 +531,7 @@ static int __devinit ps3disk_probe(struc
 	gendisk->driverfs_dev = &dev->sbd.core;
 	snprintf(gendisk->disk_name, sizeof(gendisk->disk_name), PS3DISK_NAME,
 		 devidx+'a');
-	priv->blocking_factor = dev->blk_size/KERNEL_SECTOR_SIZE;
+	priv->blocking_factor = dev->blk_size >> 9;
 	set_capacity(gendisk,
 		     dev->regions[dev->region_idx].size*priv->blocking_factor);
 
@@ -549,19 +551,23 @@ fail_free_bounce:
 	kfree(dev->bounce_buf);
 fail_free_priv:
 	kfree(priv);
-	ps3disk_priv(dev) = NULL;
+	dev->sbd.core.driver_data = NULL;
 fail:
+	mutex_lock(&ps3disk_mask_mutex);
 	__clear_bit(devidx, &ps3disk_mask);
+	mutex_unlock(&ps3disk_mask_mutex);
 	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);
+	struct ps3disk_private *priv = dev->sbd.core.driver_data;
 
+	mutex_lock(&ps3disk_mask_mutex);
 	__clear_bit(priv->gendisk->first_minor / PS3DISK_MINORS,
 		    &ps3disk_mask);
+	mutex_unlock(&ps3disk_mask_mutex);
 	del_gendisk(priv->gendisk);
 	blk_cleanup_queue(priv->queue);
 	put_disk(priv->gendisk);
@@ -570,7 +576,7 @@ static int ps3disk_remove(struct ps3_sys
 	ps3stor_teardown(dev);
 	kfree(dev->bounce_buf);
 	kfree(priv);
-	ps3disk_priv(dev) = NULL;
+	dev->sbd.core.driver_data = NULL;
 	return 0;
 }
 

With kind regards,
 
Geert Uytterhoeven
Software Architect

Sony Network and Software Technology Center Europe
The Corporate Village · Da Vincilaan 7-D1 · B-1935 Zaventem · Belgium
 
Phone:    +32 (0)2 700 8453	
Fax:      +32 (0)2 700 8622	
E-mail:   Geert.Uytterhoeven@sonycom.com	
Internet: http://www.sony-europe.com/
 	
Sony Network and Software Technology Center Europe	
A division of Sony Service Centre (Europe) N.V.	
Registered office: Technologielaan 7 · B-1840 Londerzeel · Belgium	
VAT BE 0413.825.160 · RPR Brussels	
Fortis Bank Zaventem · Swift GEBABEBB08A · IBAN BE39001382358619

[patch 3/4] ps3rom: updates after final review

[Geert Uytterhoeven]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 2901 bytes --]

ps3rom: updates after final review:
  o Kill confusing ps3rom_priv() macro, open code it instead
  o kmap_atomic() cannot fail

Signed-off-by: Geert Uytterhoeven <Geert.Uytterhoeven@sonycom.com>
---
 drivers/scsi/ps3rom.c |   15 +++++----------
 1 files changed, 5 insertions(+), 10 deletions(-)

--- a/drivers/scsi/ps3rom.c
+++ b/drivers/scsi/ps3rom.c
@@ -42,7 +42,6 @@ struct ps3rom_private {
 	struct ps3_storage_device *dev;
 	struct scsi_cmnd *curr_cmd;
 };
-#define ps3rom_priv(dev)	((dev)->sbd.core.driver_data)
 
 
 #define LV1_STORAGE_SEND_ATAPI_COMMAND	(1)
@@ -113,8 +112,6 @@ static int fill_from_dev_buffer(struct s
 	for (k = 0, req_len = 0, act_len = 0; k < cmd->use_sg; ++k, ++sgpnt) {
 		if (active) {
 			kaddr = kmap_atomic(sgpnt->page, KM_IRQ0);
-			if (!kaddr)
-				return -1;
 			len = sgpnt->length;
 			if ((req_len + len) > buflen) {
 				active = 0;
@@ -151,8 +148,6 @@ static int fetch_to_dev_buffer(struct sc
 	sgpnt = cmd->request_buffer;
 	for (k = 0, req_len = 0, fin = 0; k < cmd->use_sg; ++k, ++sgpnt) {
 		kaddr = kmap_atomic(sgpnt->page, KM_IRQ0);
-		if (!kaddr)
-			return -1;
 		len = sgpnt->length;
 		if ((req_len + len) > buflen) {
 			len = buflen - req_len;
@@ -379,7 +374,7 @@ static irqreturn_t ps3rom_interrupt(int 
 		return IRQ_HANDLED;
 	}
 
-	host = ps3rom_priv(dev);
+	host = dev->sbd.core.driver_data;
 	priv = shost_priv(host);
 	cmd = priv->curr_cmd;
 
@@ -469,7 +464,7 @@ static int __devinit ps3rom_probe(struct
 	}
 
 	priv = shost_priv(host);
-	ps3rom_priv(dev) = host;
+	dev->sbd.core.driver_data = host;
 	priv->dev = dev;
 
 	/* One device/LUN per SCSI bus */
@@ -489,7 +484,7 @@ static int __devinit ps3rom_probe(struct
 
 fail_host_put:
 	scsi_host_put(host);
-	ps3rom_priv(dev) = NULL;
+	dev->sbd.core.driver_data = NULL;
 fail_teardown:
 	ps3stor_teardown(dev);
 fail_free_bounce:
@@ -500,12 +495,12 @@ fail_free_bounce:
 static int ps3rom_remove(struct ps3_system_bus_device *_dev)
 {
 	struct ps3_storage_device *dev = to_ps3_storage_device(&_dev->core);
-	struct Scsi_Host *host = ps3rom_priv(dev);
+	struct Scsi_Host *host = dev->sbd.core.driver_data;
 
 	scsi_remove_host(host);
 	ps3stor_teardown(dev);
 	scsi_host_put(host);
-	ps3rom_priv(dev) = NULL;
+	dev->sbd.core.driver_data = NULL;
 	kfree(dev->bounce_buf);
 	return 0;
 }

With kind regards,
 
Geert Uytterhoeven
Software Architect

Sony Network and Software Technology Center Europe
The Corporate Village · Da Vincilaan 7-D1 · B-1935 Zaventem · Belgium
 
Phone:    +32 (0)2 700 8453	
Fax:      +32 (0)2 700 8622	
E-mail:   Geert.Uytterhoeven@sonycom.com	
Internet: http://www.sony-europe.com/
 	
Sony Network and Software Technology Center Europe	
A division of Sony Service Centre (Europe) N.V.	
Registered office: Technologielaan 7 · B-1840 Londerzeel · Belgium	
VAT BE 0413.825.160 · RPR Brussels	
Fortis Bank Zaventem · Swift GEBABEBB08A · IBAN BE39001382358619

[patch 4/4] ps3flash: updates after final review

[Geert Uytterhoeven]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 6253 bytes --]

ps3flash: updates after final review:
  o Kill confusing ps3flash_priv() macro, open code it instead
  o ps3flash_llseek(): Add missing locking (using i_mutex)
  o Replace do_div_llr() by `/' and `%'
  o ps3flash_read(): Use a common return for the error path
  o Minor coding style fixes not reported by checkpatch

Signed-off-by: Geert Uytterhoeven <Geert.Uytterhoeven@sonycom.com>
---
 drivers/char/ps3flash.c |   59 ++++++++++++++++++++++++++++--------------------
 1 files changed, 35 insertions(+), 24 deletions(-)

--- a/drivers/char/ps3flash.c
+++ b/drivers/char/ps3flash.c
@@ -34,7 +34,6 @@
 struct ps3flash_private {
 	struct mutex mutex;	/* Bounce buffer mutex */
 };
-#define ps3flash_priv(dev)	((dev)->sbd.core.driver_data)
 
 static struct ps3_storage_device *ps3flash_dev;
 
@@ -81,28 +80,35 @@ static ssize_t ps3flash_write_chunk(stru
 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;
+	loff_t res;
 
+	mutex_lock(&file->f_mapping->host->i_mutex);
 	switch (origin) {
 	case 1:
 		offset += file->f_pos;
 		break;
 	case 2:
-		offset += size;
+		offset += dev->regions[dev->region_idx].size*dev->blk_size;
 		break;
 	}
-	if (offset < 0)
-		return -EINVAL;
+	if (offset < 0) {
+		res = -EINVAL;
+		goto out;
+	}
 
 	file->f_pos = offset;
-	return file->f_pos;
+	res = file->f_pos;
+
+out:
+	mutex_unlock(&file->f_mapping->host->i_mutex);
+	return res;
 }
 
 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);
+	struct ps3flash_private *priv = dev->sbd.core.driver_data;
 	u64 size, start_sector, end_sector, offset;
 	ssize_t sectors_read;
 	size_t remaining, n;
@@ -115,15 +121,16 @@ static ssize_t ps3flash_read(struct file
 	if (*pos >= size || !count)
 		return 0;
 
-	if (*pos+count > size) {
+	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);
+	start_sector = *pos / dev->blk_size;
+	offset = *pos % dev->blk_size;
+	end_sector = DIV_ROUND_UP(*pos + count, dev->blk_size);
 
 	remaining = count;
 	do {
@@ -134,7 +141,7 @@ static ssize_t ps3flash_read(struct file
 						     0);
 		if (sectors_read < 0) {
 			mutex_unlock(&priv->mutex);
-			return sectors_read;
+			goto fail;
 		}
 
 		n = min(remaining, sectors_read*dev->blk_size-offset);
@@ -143,7 +150,8 @@ static ssize_t ps3flash_read(struct file
 			__func__, __LINE__, n, dev->bounce_buf+offset, buf);
 		if (copy_to_user(buf, dev->bounce_buf+offset, n)) {
 			mutex_unlock(&priv->mutex);
-			return -EFAULT;
+			sectors_read = -EFAULT;
+			goto fail;
 		}
 
 		mutex_unlock(&priv->mutex);
@@ -156,13 +164,16 @@ static ssize_t ps3flash_read(struct file
 	} while (remaining > 0);
 
 	return count;
+
+fail:
+	return sectors_read;
 }
 
 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);
+	struct ps3flash_private *priv = dev->sbd.core.driver_data;
 	u64 size, chunk_sectors, start_write_sector, end_write_sector,
 	    end_read_sector, start_read_sector, head, tail, offset;
 	ssize_t res;
@@ -177,7 +188,7 @@ static ssize_t ps3flash_write(struct fil
 	if (*pos >= size || !count)
 		return 0;
 
-	if (*pos+count > size) {
+	if (*pos + count > size) {
 		dev_dbg(&dev->sbd.core,
 			"%s:%u Truncating count from %zu to %llu\n", __func__,
 			__LINE__, count, size - *pos);
@@ -186,13 +197,13 @@ static ssize_t ps3flash_write(struct fil
 
 	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) *
+	start_write_sector = *pos / dev->bounce_size * chunk_sectors;
+	offset = *pos % dev->bounce_size;
+	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;
+	start_read_sector = (*pos + count) / dev->blk_size;
 
 	/*
 	 * As we have to write in 256 KiB chunks, while we can read in blk_size
@@ -204,8 +215,8 @@ static ssize_t ps3flash_write(struct fil
 	 * 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;
+	head = end_read_sector - start_write_sector;
+	tail = end_write_sector - start_read_sector;
 
 	remaining = count;
 	do {
@@ -355,7 +366,7 @@ static int __devinit ps3flash_probe(stru
 		goto fail;
 	}
 
-	ps3flash_priv(dev) = priv;
+	dev->sbd.core.driver_data = priv;
 	mutex_init(&priv->mutex);
 
 	dev->bounce_size = ps3flash_bounce_buffer.size;
@@ -381,7 +392,7 @@ fail_teardown:
 	ps3stor_teardown(dev);
 fail_free_priv:
 	kfree(priv);
-	ps3flash_priv(dev) = NULL;
+	dev->sbd.core.driver_data = NULL;
 fail:
 	ps3flash_dev = NULL;
 	return error;
@@ -393,8 +404,8 @@ static int ps3flash_remove(struct ps3_sy
 
 	misc_deregister(&ps3flash_misc);
 	ps3stor_teardown(dev);
-	kfree(ps3flash_priv(dev));
-	ps3flash_priv(dev) = NULL;
+	kfree(dev->sbd.core.driver_data);
+	dev->sbd.core.driver_data = NULL;
 	ps3flash_dev = NULL;
 	return 0;
 }

With kind regards,
 
Geert Uytterhoeven
Software Architect

Sony Network and Software Technology Center Europe
The Corporate Village · Da Vincilaan 7-D1 · B-1935 Zaventem · Belgium
 
Phone:    +32 (0)2 700 8453	
Fax:      +32 (0)2 700 8622	
E-mail:   Geert.Uytterhoeven@sonycom.com	
Internet: http://www.sony-europe.com/
 	
Sony Network and Software Technology Center Europe	
A division of Sony Service Centre (Europe) N.V.	
Registered office: Technologielaan 7 · B-1840 Londerzeel · Belgium	
VAT BE 0413.825.160 · RPR Brussels	
Fortis Bank Zaventem · Swift GEBABEBB08A · IBAN BE39001382358619

Re: [patch 1/3] ps3: Disk Storage Driver

[Cornelia Huck]

On Thu, 19 Jul 2007 10:57:53 +0200 (CEST),
Geert Uytterhoeven <Geert.Uytterhoeven@sonycom.com> wrote:

> Were .probe()/.remove() made concurrent again? I thought that idea was dropped
> because it caused too many problems?

Well, for a given device, ->probe()/->remove() are locked by dev->sem,
so that there cannot be two probes/removes for the same device at the
same time. However, if you have multiple hotplug events pending at the
same time, it depends on your bus whether it does some serialization or
whether it allows multiple probes/removes running for different devices.
(Initial probing of a bus and probing of all devices with a new driver
is done serialized again, I think that's what you're referring to.)

Re: [patch 1/3] ps3: Disk Storage Driver

[Geert Uytterhoeven]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 1663 bytes --]

On Thu, 19 Jul 2007, Cornelia Huck wrote:
> On Thu, 19 Jul 2007 10:57:53 +0200 (CEST),
> Geert Uytterhoeven <Geert.Uytterhoeven@sonycom.com> wrote:
> 
> > Were .probe()/.remove() made concurrent again? I thought that idea was dropped
> > because it caused too many problems?
> 
> Well, for a given device, ->probe()/->remove() are locked by dev->sem,
> so that there cannot be two probes/removes for the same device at the
> same time. However, if you have multiple hotplug events pending at the

OK.

> same time, it depends on your bus whether it does some serialization or
> whether it allows multiple probes/removes running for different devices.
> (Initial probing of a bus and probing of all devices with a new driver
> is done serialized again, I think that's what you're referring to.)

We have a probe thread that checks for new storage devices and adds them to the
bus with ps3_system_bus_device_register(), which calls device_register().

I guess the actual bus probe() routine gets called through the notifier call
chain? That's where I got lost...

With kind regards,
 
Geert Uytterhoeven
Software Architect

Sony Network and Software Technology Center Europe
The Corporate Village · Da Vincilaan 7-D1 · B-1935 Zaventem · Belgium
 
Phone:    +32 (0)2 700 8453	
Fax:      +32 (0)2 700 8622	
E-mail:   Geert.Uytterhoeven@sonycom.com	
Internet: http://www.sony-europe.com/
 	
Sony Network and Software Technology Center Europe	
A division of Sony Service Centre (Europe) N.V.	
Registered office: Technologielaan 7 · B-1840 Londerzeel · Belgium	
VAT BE 0413.825.160 · RPR Brussels	
Fortis Bank Zaventem · Swift GEBABEBB08A · IBAN BE39001382358619

Re: [patch 1/3] ps3: Disk Storage Driver

[Cornelia Huck]

On Thu, 19 Jul 2007 11:36:31 +0200 (CEST),
Geert Uytterhoeven <Geert.Uytterhoeven@sonycom.com> wrote:

> We have a probe thread that checks for new storage devices and adds them to the
> bus with ps3_system_bus_device_register(), which calls device_register().
> 
> I guess the actual bus probe() routine gets called through the notifier call
> chain? That's where I got lost...

No, ->probe() is called from device_register() directly. If you just
have one probe thread, you should have enough serialization.

(Unless you're doing something interesting from the bus_notifier, which
is called via the notifier chain before ->probe()...)

Re: [patch 1/3] ps3: Disk Storage Driver

[Geert Uytterhoeven]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 1316 bytes --]

On Thu, 19 Jul 2007, Cornelia Huck wrote:
> On Thu, 19 Jul 2007 11:36:31 +0200 (CEST),
> Geert Uytterhoeven <Geert.Uytterhoeven@sonycom.com> wrote:
> 
> > We have a probe thread that checks for new storage devices and adds them to the
> > bus with ps3_system_bus_device_register(), which calls device_register().
> > 
> > I guess the actual bus probe() routine gets called through the notifier call
> > chain? That's where I got lost...
> 
> No, ->probe() is called from device_register() directly. If you just
> have one probe thread, you should have enough serialization.

IC, through bus_attach_device() in device_add().

Any chance this will change? I already added a mutex to ps3disk to protect
against this.

With kind regards,
 
Geert Uytterhoeven
Software Architect

Sony Network and Software Technology Center Europe
The Corporate Village · Da Vincilaan 7-D1 · B-1935 Zaventem · Belgium
 
Phone:    +32 (0)2 700 8453	
Fax:      +32 (0)2 700 8622	
E-mail:   Geert.Uytterhoeven@sonycom.com	
Internet: http://www.sony-europe.com/
 	
Sony Network and Software Technology Center Europe	
A division of Sony Service Centre (Europe) N.V.	
Registered office: Technologielaan 7 · B-1840 Londerzeel · Belgium	
VAT BE 0413.825.160 · RPR Brussels	
Fortis Bank Zaventem · Swift GEBABEBB08A · IBAN BE39001382358619

Re: [patch 1/3] ps3: Disk Storage Driver

[Cornelia Huck]

On Thu, 19 Jul 2007 14:13:33 +0200 (CEST),
Geert Uytterhoeven <Geert.Uytterhoeven@sonycom.com> wrote:

> Any chance this will change? I already added a mutex to ps3disk to protect
> against this.

Probably not in the near future. A mutex looks like a good idea though,
since one never knows (and the driver core generally doesn't make many
guarantees regarding serialization).

Re: [patch 1/3] ps3: Disk Storage Driver

[Andy Whitcroft]

Andrew Morton wrote:

>> +	start_sector = req->sector*priv->blocking_factor;
>> +	sectors = req->nr_sectors*priv->blocking_factor;
> 
> s/*/ * /.  checkpatch missed this.

Ok, this is something we need to decide on.  Currently we only ask for
consistent spacing on all the mathematic operators.  This is mostly as
we do see a large number of non-spaced uses in defines and the like.

I am happy to expand these tests so they are always spaced on both sides
style if that is the preference.

-apw

Re: [patch 1/3] ps3: Disk Storage Driver

[Jeff Garzik]

Andy Whitcroft wrote:
> Andrew Morton wrote:
> 
>>> +	start_sector = req->sector*priv->blocking_factor;
>>> +	sectors = req->nr_sectors*priv->blocking_factor;
>> s/*/ * /.  checkpatch missed this.
> 
> Ok, this is something we need to decide on.  Currently we only ask for
> consistent spacing on all the mathematic operators.  This is mostly as
> we do see a large number of non-spaced uses in defines and the like.
> 
> I am happy to expand these tests so they are always spaced on both sides
> style if that is the preference.

That is most definitely the preference:  spaces surround operators.

	Jeff

Re: [patch 1/3] ps3: Disk Storage Driver

[Andreas Schwab]

Jeff Garzik <jeff@garzik.org> writes:

> Andy Whitcroft wrote:
>> Andrew Morton wrote:
>> 
>>>> +	start_sector = req->sector*priv->blocking_factor;
>>>> +	sectors = req->nr_sectors*priv->blocking_factor;
>>> s/*/ * /.  checkpatch missed this.
>> 
>> Ok, this is something we need to decide on.  Currently we only ask for
>> consistent spacing on all the mathematic operators.  This is mostly as
>> we do see a large number of non-spaced uses in defines and the like.
>> 
>> I am happy to expand these tests so they are always spaced on both sides
>> style if that is the preference.
>
> That is most definitely the preference:  spaces surround operators.
                                                          ^binary

Andreas.

-- 
Andreas Schwab, SuSE Labs, schwab@suse.de
SuSE Linux Products GmbH, Maxfeldstraße 5, 90409 Nürnberg, Germany
PGP key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

Re: [patch 1/3] ps3: Disk Storage Driver

[Jeff Garzik]

Andreas Schwab wrote:
> Jeff Garzik <jeff@garzik.org> writes:
> 
>> Andy Whitcroft wrote:
>>> Andrew Morton wrote:
>>>
>>>>> +	start_sector = req->sector*priv->blocking_factor;
>>>>> +	sectors = req->nr_sectors*priv->blocking_factor;
>>>> s/*/ * /.  checkpatch missed this.
>>> Ok, this is something we need to decide on.  Currently we only ask for
>>> consistent spacing on all the mathematic operators.  This is mostly as
>>> we do see a large number of non-spaced uses in defines and the like.
>>>
>>> I am happy to expand these tests so they are always spaced on both sides
>>> style if that is the preference.
>> That is most definitely the preference:  spaces surround operators.
>                                                           ^binary

Yes.  :)

	Jeff

Re: [patch 1/3] ps3: Disk Storage Driver

[Andrew Morton]

On Tue, 24 Jul 2007 08:37:09 -0400 Jeff Garzik <jeff@garzik.org> wrote:

> Andy Whitcroft wrote:
> > Andrew Morton wrote:
> > 
> >>> +	start_sector = req->sector*priv->blocking_factor;
> >>> +	sectors = req->nr_sectors*priv->blocking_factor;
> >> s/*/ * /.  checkpatch missed this.
> > 
> > Ok, this is something we need to decide on.  Currently we only ask for
> > consistent spacing on all the mathematic operators.  This is mostly as
> > we do see a large number of non-spaced uses in defines and the like.
> > 
> > I am happy to expand these tests so they are always spaced on both sides
> > style if that is the preference.
> 
> That is most definitely the preference:  spaces surround operators.
> 

I must say that I find it hard to object to

	start = radix_tree_next_hole(&mapping->page_tree, offset, max+1);

but when the expression is more complex the spaces help.

Re: [patch 1/3] ps3: Disk Storage Driver

[Paul Mackerras]

Andy Whitcroft writes:

> Ok, this is something we need to decide on.  Currently we only ask for
> consistent spacing on all the mathematic operators.  This is mostly as
> we do see a large number of non-spaced uses in defines and the like.
> 
> I am happy to expand these tests so they are always spaced on both sides
> style if that is the preference.

It depends very much on the context - on the precedence and relative
importance of one operator with respect to other operators and the
statement as a whole.  In general I prefer spaces around binary
operators, but there are situations where not putting spaces around
some operators can enhance the readability of the statement as a
whole.

If checkpatch.pl starts whinging about operators without spaces that
will just be yet another reason not to use it IMHO.

Also, I prefer the style where the ? and : operators have a space
after them but not before them, rather than a space either side.

Paul.

Re: [patch 1/3] ps3: Disk Storage Driver

[Andrew Morton]

On Wed, 25 Jul 2007 11:09:21 +1000 Paul Mackerras <paulus@samba.org> wrote:

> Also, I prefer the style where the ? and : operators have a space
> after them but not before them, rather than a space either side.

Could I point out that your likes and dislikes are immaterial?  The whole
point here is to get kernel code looking consistent.  That means that
basically everyone ends up doing things which they'd prefer not to do. 
That certainly applies to me.  

The idea is that the benefit of making things consistent exceeds the costs
of some individuals adopting styles which they are less used to.


So telling people what you do and don't like is simply irrelevant, except
for when it is used as an input in determining what the standard kernel
style is to be.  (And that is largely determined by observing what we have
now).

And sure, major subsytems can and do go off and do their own thing - ia64
for example has done a lot of that, pretty consistently.  The world hasn't
ended as a result.

[patch 2/3] ps3: BD/DVD/CD-ROM Storage Driver

[Geert Uytterhoeven]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain, Size: 17923 bytes --]

From: Geert Uytterhoeven <Geert.Uytterhoeven@sonycom.com>

Add a BD/DVD/CD-ROM 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

CC: Geoff Levand <geoffrey.levand@am.sony.com>
Signed-off-by: Geert Uytterhoeven <Geert.Uytterhoeven@sonycom.com>
---
v4:
  - Replace KM_USER0 by KM_IRQ0 (all kmap routines are either called from an
    interrupt handler or from .queuecommand())
  - Add a call to flush_kernel_dcache_page() before kunmap in routines that
    write to buffers

v3:
  - Replace dependency on BLK_DEV_SR by SCSI
  - Use `BD/DVD/CD-ROM' instead of `ROM' in driver descriptions
  - Inform the user to enable "SCSI CDROM support"
  - Cleanup #includes
  - Group LV1 API related stuff
  - Rename ps3rom_private.cmd to ps3rom_private.curr_cmd
  - Use shost_priv() (from scsi-misc)
  - Set scsi_device.use_10_for_rw and remove support for {READ,WRITE}_6
  - Explain why we prefer to use lv1_storage_{read,write}()
  - Pass the interrupt handler to ps3stor_setup(), so it doesn't have to be
    overridden later
  - Reset ps3rom_priv(dev) to NULL during cleanup

v2:
  - Don't use `default y' in Kconfig
  - Add missing #include <linux/highmem.h>
  - Remove ps3rom_private.scsi_done, use scsi_cmnd.scsi_done instead
  - Use scsi_device.host.hostdata
  - Remove empty ps3rom_slave_{alloc,destroy}()
  - Kill superfluous test for command in progress
  - Move scsi_host_put() last in cleanup sequence
  - Remove scsi_command(), use scsi_print_command() for debugging
  - scsi_cmnd.use_sg is always > 0 these days
  - Allocate struct ps3rom_private using scsi_host_alloc()
  - Remove the thread for handling requests
  - Remove unused position parameter enum
  - Remove unused NA_PROTO and DIR_NA
  - Derive buffer length, data direction, and transfer protocol from the
    struct scsi_command, instead of using a big switch() statement
  - Kill superfluous spinlock
  - Remove manual request sense, as modern hypervisors always do auto sense
  - Pass all SCSI commands to the hypervisor as ATAPI commands, except for
    READ_*/WRITE_*
  - Don't print errors for SCSI commands that are not allowed for an Other OS
    by the hypervisor
  - Remove superfluous tests for data directions in
    {fill_from,fetch_to}_dev_buffer()
  - Handle errors in {fill_from,fetch_to}_dev_buffer()
  - Reorder routines
  - Manually inline ps3rom_send_atapi_command()
  - Fix all FIXMEs

 arch/powerpc/platforms/ps3/Kconfig |   11 
 drivers/scsi/Makefile              |    1 
 drivers/scsi/ps3rom.c              |  538 +++++++++++++++++++++++++++++++++++++
 3 files changed, 550 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 BD/DVD/CD-ROM Storage Driver"
+	depends on PPC_PS3 && SCSI
+	select PS3_STORAGE
+	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.
+	  Also make sure to say Y or M to "SCSI CDROM support" later.
+
 endmenu
--- a/drivers/scsi/Makefile
+++ b/drivers/scsi/Makefile
@@ -132,6 +132,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,538 @@
+/*
+ * PS3 BD/DVD/CD-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/highmem.h>
+
+#include <scsi/scsi.h>
+#include <scsi/scsi_cmnd.h>
+#include <scsi/scsi_dbg.h>
+#include <scsi/scsi_device.h>
+#include <scsi/scsi_host.h>
+
+#include <asm/lv1call.h>
+#include <asm/ps3stor.h>
+
+
+#define DEVICE_NAME			"ps3rom"
+
+#define BOUNCE_SIZE			(64*1024)
+
+#define PS3ROM_MAX_SECTORS		(BOUNCE_SIZE / CD_FRAMESIZE)
+
+
+struct ps3rom_private {
+	struct ps3_storage_device *dev;
+	struct scsi_cmnd *curr_cmd;
+};
+#define ps3rom_priv(dev)	((dev)->sbd.core.driver_data)
+
+
+#define LV1_STORAGE_SEND_ATAPI_COMMAND	(1)
+
+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                   */
+};
+
+enum lv1_atapi_proto {
+	NON_DATA_PROTO     = 0,
+	PIO_DATA_IN_PROTO  = 1,
+	PIO_DATA_OUT_PROTO = 2,
+	DMA_PROTO = 3
+};
+
+enum lv1_atapi_in_out {
+	DIR_WRITE = 0,		/* memory -> device */
+	DIR_READ = 1		/* device -> memory */
+};
+
+
+static int ps3rom_slave_configure(struct scsi_device *scsi_dev)
+{
+	struct ps3rom_private *priv = shost_priv(scsi_dev->host);
+	struct ps3_storage_device *dev = priv->dev;
+
+	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;
+
+	/* we don't support {READ,WRITE}_6 */
+	scsi_dev->use_10_for_rw = 1;
+
+	return 0;
+}
+
+/*
+ * copy data from device into scatter/gather buffer
+ */
+static int fill_from_dev_buffer(struct scsi_cmnd *cmd, const void *buf)
+{
+	int k, req_len, act_len, len, active;
+	void *kaddr;
+	struct scatterlist *sgpnt;
+	unsigned int buflen;
+
+	buflen = cmd->request_bufflen;
+	if (!buflen)
+		return 0;
+
+	if (!cmd->request_buffer)
+		return -1;
+
+	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_IRQ0);
+			if (!kaddr)
+				return -1;
+			len = sgpnt->length;
+			if ((req_len + len) > buflen) {
+				active = 0;
+				len = buflen - req_len;
+			}
+			memcpy(kaddr + sgpnt->offset, buf + req_len, len);
+			flush_kernel_dcache_page(sgpnt->page);
+			kunmap_atomic(kaddr, KM_IRQ0);
+			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 k, req_len, len, fin;
+	void *kaddr;
+	struct scatterlist *sgpnt;
+	unsigned int buflen;
+
+	buflen = cmd->request_bufflen;
+	if (!buflen)
+		return 0;
+
+	if (!cmd->request_buffer)
+		return -1;
+
+	sgpnt = cmd->request_buffer;
+	for (k = 0, req_len = 0, fin = 0; k < cmd->use_sg; ++k, ++sgpnt) {
+		kaddr = kmap_atomic(sgpnt->page, KM_IRQ0);
+		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_IRQ0);
+		if (fin)
+			return req_len + len;
+		req_len += sgpnt->length;
+	}
+	return req_len;
+}
+
+static int ps3rom_atapi_request(struct ps3_storage_device *dev,
+				struct scsi_cmnd *cmd)
+{
+	struct lv1_atapi_cmnd_block atapi_cmnd;
+	unsigned char opcode = cmd->cmnd[0];
+	int res;
+	u64 lpar;
+
+	dev_dbg(&dev->sbd.core, "%s:%u: send ATAPI command 0x%02x\n", __func__,
+		__LINE__, opcode);
+
+	memset(&atapi_cmnd, 0, sizeof(struct lv1_atapi_cmnd_block));
+	memcpy(&atapi_cmnd.pkt, cmd->cmnd, 12);
+	atapi_cmnd.pktlen = 12;
+	atapi_cmnd.block_size = 1; /* transfer size is block_size * blocks */
+	atapi_cmnd.blocks = atapi_cmnd.arglen = cmd->request_bufflen;
+	atapi_cmnd.buffer = dev->bounce_lpar;
+
+	switch (cmd->sc_data_direction) {
+	case DMA_FROM_DEVICE:
+		if (cmd->request_bufflen >= CD_FRAMESIZE)
+			atapi_cmnd.proto = DMA_PROTO;
+		else
+			atapi_cmnd.proto = PIO_DATA_IN_PROTO;
+		atapi_cmnd.in_out = DIR_READ;
+		break;
+
+	case DMA_TO_DEVICE:
+		if (cmd->request_bufflen >= CD_FRAMESIZE)
+			atapi_cmnd.proto = DMA_PROTO;
+		else
+			atapi_cmnd.proto = PIO_DATA_OUT_PROTO;
+		atapi_cmnd.in_out = DIR_WRITE;
+		res = fetch_to_dev_buffer(cmd, dev->bounce_buf);
+		if (res < 0)
+			return DID_ERROR << 16;
+		break;
+
+	default:
+		atapi_cmnd.proto = NON_DATA_PROTO;
+		break;
+	}
+
+	lpar = ps3_mm_phys_to_lpar(__pa(&atapi_cmnd));
+	res = lv1_storage_send_device_command(dev->sbd.dev_id,
+					      LV1_STORAGE_SEND_ATAPI_COMMAND,
+					      lpar, sizeof(atapi_cmnd),
+					      atapi_cmnd.buffer,
+					      atapi_cmnd.arglen, &dev->tag);
+	if (res == LV1_DENIED_BY_POLICY) {
+		dev_dbg(&dev->sbd.core,
+			"%s:%u: ATAPI command 0x%02x denied by policy\n",
+			__func__, __LINE__, opcode);
+		return DID_ERROR << 16;
+	}
+
+	if (res) {
+		dev_err(&dev->sbd.core,
+			"%s:%u: ATAPI command 0x%02x failed %d\n", __func__,
+			__LINE__, opcode, res);
+		return DID_ERROR << 16;
+	}
+
+	return 0;
+}
+
+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 int ps3rom_read_request(struct ps3_storage_device *dev,
+			       struct scsi_cmnd *cmd, u32 start_sector,
+			       u32 sectors)
+{
+	int res;
+
+	dev_dbg(&dev->sbd.core, "%s:%u: read %u sectors starting at %u\n",
+		__func__, __LINE__, sectors, start_sector);
+
+	res = lv1_storage_read(dev->sbd.dev_id,
+			       dev->regions[dev->region_idx].id, start_sector,
+			       sectors, 0, dev->bounce_lpar, &dev->tag);
+	if (res) {
+		dev_err(&dev->sbd.core, "%s:%u: read failed %d\n", __func__,
+			__LINE__, res);
+		return DID_ERROR << 16;
+	}
+
+	return 0;
+}
+
+static int ps3rom_write_request(struct ps3_storage_device *dev,
+				struct scsi_cmnd *cmd, u32 start_sector,
+				u32 sectors)
+{
+	int res;
+
+	dev_dbg(&dev->sbd.core, "%s:%u: write %u sectors starting at %u\n",
+		__func__, __LINE__, sectors, start_sector);
+
+	res = fetch_to_dev_buffer(cmd, dev->bounce_buf);
+	if (res < 0)
+		return DID_ERROR << 16;
+
+	res = lv1_storage_write(dev->sbd.dev_id,
+				dev->regions[dev->region_idx].id, start_sector,
+				sectors, 0, dev->bounce_lpar, &dev->tag);
+	if (res) {
+		dev_err(&dev->sbd.core, "%s:%u: write failed %d\n", __func__,
+			__LINE__, res);
+		return DID_ERROR << 16;
+	}
+
+	return 0;
+}
+
+static int ps3rom_queuecommand(struct scsi_cmnd *cmd,
+			       void (*done)(struct scsi_cmnd *))
+{
+	struct ps3rom_private *priv = shost_priv(cmd->device->host);
+	struct ps3_storage_device *dev = priv->dev;
+	unsigned char opcode;
+	int res;
+
+#ifdef DEBUG
+	scsi_print_command(cmd);
+#endif
+
+	priv->curr_cmd = cmd;
+	cmd->scsi_done = done;
+
+	opcode = cmd->cmnd[0];
+	/*
+	 * While we can submit READ/WRITE SCSI commands as ATAPI commands,
+	 * it's recommended for various reasons (performance, error handling,
+	 * ...) to use lv1_storage_{read,write}() instead
+	 */
+	switch (opcode) {
+	case READ_10:
+		res = ps3rom_read_request(dev, cmd, srb10_lba(cmd),
+					  srb10_len(cmd));
+		break;
+
+	case WRITE_10:
+		res = ps3rom_write_request(dev, cmd, srb10_lba(cmd),
+					   srb10_len(cmd));
+		break;
+
+	default:
+		res = ps3rom_atapi_request(dev, cmd);
+		break;
+	}
+
+	if (res) {
+		memset(cmd->sense_buffer, 0, SCSI_SENSE_BUFFERSIZE);
+		cmd->result = res;
+		cmd->sense_buffer[0] = 0x70;
+		cmd->sense_buffer[2] = ILLEGAL_REQUEST;
+		priv->curr_cmd = NULL;
+		cmd->scsi_done(cmd);
+	}
+
+	return 0;
+}
+
+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 irqreturn_t ps3rom_interrupt(int irq, void *data)
+{
+	struct ps3_storage_device *dev = data;
+	struct Scsi_Host *host;
+	struct ps3rom_private *priv;
+	struct scsi_cmnd *cmd;
+	int res;
+	u64 tag, status;
+	unsigned char sense_key, asc, ascq;
+
+	res = lv1_storage_get_async_status(dev->sbd.dev_id, &tag, &status);
+	/*
+	 * 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 (tag != dev->tag)
+		dev_err(&dev->sbd.core,
+			"%s:%u: tag mismatch, got %lx, expected %lx\n",
+			__func__, __LINE__, tag, dev->tag);
+
+	if (res) {
+		dev_err(&dev->sbd.core, "%s:%u: res=%d status=0x%lx\n",
+			__func__, __LINE__, res, status);
+		return IRQ_HANDLED;
+	}
+
+	host = ps3rom_priv(dev);
+	priv = shost_priv(host);
+	cmd = priv->curr_cmd;
+
+	if (!status) {
+		/* OK, completed */
+		if (cmd->sc_data_direction == DMA_FROM_DEVICE) {
+			res = fill_from_dev_buffer(cmd, dev->bounce_buf);
+			if (res) {
+				cmd->result = DID_ERROR << 16;
+				goto done;
+			}
+		}
+		cmd->result = DID_OK << 16;
+		goto done;
+	}
+
+	if (cmd->cmnd[0] == REQUEST_SENSE) {
+		/* SCSI spec says request sense should never get error */
+		dev_err(&dev->sbd.core, "%s:%u: end error without autosense\n",
+			__func__, __LINE__);
+		cmd->result = DID_ERROR << 16 | SAM_STAT_CHECK_CONDITION;
+		goto done;
+	}
+
+	if (decode_lv1_status(status, &sense_key, &asc, &ascq)) {
+		cmd->result = DID_ERROR << 16;
+		goto done;
+	}
+
+	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;
+
+done:
+	priv->curr_cmd = NULL;
+	cmd->scsi_done(cmd);
+	return IRQ_HANDLED;
+}
+
+static struct scsi_host_template ps3rom_host_template = {
+	.name =			DEVICE_NAME,
+	.slave_configure =	ps3rom_slave_configure,
+	.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);
+	int error;
+	struct Scsi_Host *host;
+	struct ps3rom_private *priv;
+
+	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;
+	}
+
+	dev->bounce_size = BOUNCE_SIZE;
+	dev->bounce_buf = kmalloc(BOUNCE_SIZE, GFP_DMA);
+	if (!dev->bounce_buf)
+		return -ENOMEM;
+
+	error = ps3stor_setup(dev, ps3rom_interrupt);
+	if (error)
+		goto fail_free_bounce;
+
+	host = scsi_host_alloc(&ps3rom_host_template,
+			       sizeof(struct ps3rom_private));
+	if (!host) {
+		dev_err(&dev->sbd.core, "%s:%u: scsi_host_alloc failed\n",
+			__func__, __LINE__);
+		goto fail_teardown;
+	}
+
+	priv = shost_priv(host);
+	ps3rom_priv(dev) = host;
+	priv->dev = 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;
+	}
+
+	scsi_scan_host(host);
+	return 0;
+
+fail_host_put:
+	scsi_host_put(host);
+	ps3rom_priv(dev) = NULL;
+fail_teardown:
+	ps3stor_teardown(dev);
+fail_free_bounce:
+	kfree(dev->bounce_buf);
+	return error;
+}
+
+static int ps3rom_remove(struct ps3_system_bus_device *_dev)
+{
+	struct ps3_storage_device *dev = to_ps3_storage_device(&_dev->core);
+	struct Scsi_Host *host = ps3rom_priv(dev);
+
+	scsi_remove_host(host);
+	ps3stor_teardown(dev);
+	scsi_host_put(host);
+	ps3rom_priv(dev) = NULL;
+	kfree(dev->bounce_buf);
+	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);
+}
+
+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 BD/DVD/CD-ROM Storage Driver");
+MODULE_AUTHOR("Sony Corporation");
+MODULE_ALIAS(PS3_MODULE_ALIAS_STOR_ROM);

-- 
With kind regards,
 
Geert Uytterhoeven
Software Architect

Sony Network and Software Technology Center Europe
The Corporate Village · Da Vincilaan 7-D1 · B-1935 Zaventem · Belgium
 
Phone:    +32 (0)2 700 8453	
Fax:      +32 (0)2 700 8622	
E-mail:   Geert.Uytterhoeven@sonycom.com	
Internet: http://www.sony-europe.com/
 	
Sony Network and Software Technology Center Europe	
A division of Sony Service Centre (Europe) N.V.	
Registered office: Technologielaan 7 · B-1840 Londerzeel · Belgium	
VAT BE 0413.825.160 · RPR Brussels	
Fortis Bank Zaventem · Swift GEBABEBB08A · IBAN BE39001382358619

Re: [patch 2/3] ps3: BD/DVD/CD-ROM Storage Driver

[Andrew Morton]

On Mon, 16 Jul 2007 18:15:41 +0200
Geert Uytterhoeven <Geert.Uytterhoeven@sonycom.com> wrote:

> From: Geert Uytterhoeven <Geert.Uytterhoeven@sonycom.com>
> 
> Add a BD/DVD/CD-ROM 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
> 
> --- /dev/null
> +++ b/drivers/scsi/ps3rom.c
> @@ -0,0 +1,538 @@
> +/*
> + * PS3 BD/DVD/CD-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/highmem.h>
> +
> +#include <scsi/scsi.h>
> +#include <scsi/scsi_cmnd.h>
> +#include <scsi/scsi_dbg.h>
> +#include <scsi/scsi_device.h>
> +#include <scsi/scsi_host.h>
> +
> +#include <asm/lv1call.h>
> +#include <asm/ps3stor.h>
> +
> +
> +#define DEVICE_NAME			"ps3rom"
> +
> +#define BOUNCE_SIZE			(64*1024)
> +
> +#define PS3ROM_MAX_SECTORS		(BOUNCE_SIZE / CD_FRAMESIZE)
> +
> +
> +struct ps3rom_private {
> +	struct ps3_storage_device *dev;
> +	struct scsi_cmnd *curr_cmd;
> +};
> +#define ps3rom_priv(dev)	((dev)->sbd.core.driver_data)
> +

Someone should invent a keyboard which delivers an electric shock when the
operator types "#define".   In the meanwhile, I get to do the honours.

Please don't implement in a macro anything which can be implemented in C.

> +
> +#define LV1_STORAGE_SEND_ATAPI_COMMAND	(1)
> +
> +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                   */
> +};
> +
> +enum lv1_atapi_proto {
> +	NON_DATA_PROTO     = 0,
> +	PIO_DATA_IN_PROTO  = 1,
> +	PIO_DATA_OUT_PROTO = 2,
> +	DMA_PROTO = 3
> +};
> +
>
> ...
>
> +/*
> + * copy data from device into scatter/gather buffer
> + */
> +static int fill_from_dev_buffer(struct scsi_cmnd *cmd, const void *buf)
> +{
> +	int k, req_len, act_len, len, active;
> +	void *kaddr;
> +	struct scatterlist *sgpnt;
> +	unsigned int buflen;
> +
> +	buflen = cmd->request_bufflen;
> +	if (!buflen)
> +		return 0;
> +
> +	if (!cmd->request_buffer)
> +		return -1;
> +
> +	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_IRQ0);
> +			if (!kaddr)
> +				return -1;

kmap_atomic() cannot fail.  On i386, at least.  If it can fail on any other
arch then we have a biiiiig problem.  (Multiple instances of this)

> +			len = sgpnt->length;
> +			if ((req_len + len) > buflen) {
> +				active = 0;
> +				len = buflen - req_len;
> +			}
> +			memcpy(kaddr + sgpnt->offset, buf + req_len, len);
> +			flush_kernel_dcache_page(sgpnt->page);
> +			kunmap_atomic(kaddr, KM_IRQ0);
> +			act_len += len;
> +		}
> +		req_len += sgpnt->length;
> +	}
> +	cmd->resid = req_len - act_len;
> +	return 0;
> +}
> +

Re: [patch 2/3] ps3: BD/DVD/CD-ROM Storage Driver

[Geert Uytterhoeven]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 1464 bytes --]

On Wed, 18 Jul 2007, Andrew Morton wrote:
> > +struct ps3rom_private {
> > +	struct ps3_storage_device *dev;
> > +	struct scsi_cmnd *curr_cmd;
> > +};
> > +#define ps3rom_priv(dev)	((dev)->sbd.core.driver_data)
> > +
> 
> Someone should invent a keyboard which delivers an electric shock when the
> operator types "#define".   In the meanwhile, I get to do the honours.
> 
> Please don't implement in a macro anything which can be implemented in C.

All I needed was a shorthand to access driver_data, for both read and write
access (you cannot do the latter with C, unless you decouple read and write).

> > +			kaddr = kmap_atomic(sgpnt->page, KM_IRQ0);
> > +			if (!kaddr)
> > +				return -1;
> 
> kmap_atomic() cannot fail.  On i386, at least.  If it can fail on any other
> arch then we have a biiiiig problem.  (Multiple instances of this)

Thanks, fixed!

With kind regards,
 
Geert Uytterhoeven
Software Architect

Sony Network and Software Technology Center Europe
The Corporate Village · Da Vincilaan 7-D1 · B-1935 Zaventem · Belgium
 
Phone:    +32 (0)2 700 8453	
Fax:      +32 (0)2 700 8622	
E-mail:   Geert.Uytterhoeven@sonycom.com	
Internet: http://www.sony-europe.com/
 	
Sony Network and Software Technology Center Europe	
A division of Sony Service Centre (Europe) N.V.	
Registered office: Technologielaan 7 · B-1840 Londerzeel · Belgium	
VAT BE 0413.825.160 · RPR Brussels	
Fortis Bank Zaventem · Swift GEBABEBB08A · IBAN BE39001382358619

Re: [patch 2/3] ps3: BD/DVD/CD-ROM Storage Driver

[Andrew Morton]

On Thu, 19 Jul 2007 11:02:07 +0200 (CEST) Geert Uytterhoeven <Geert.Uytterhoeven@sonycom.com> wrote:

> On Wed, 18 Jul 2007, Andrew Morton wrote:
> > > +struct ps3rom_private {
> > > +	struct ps3_storage_device *dev;
> > > +	struct scsi_cmnd *curr_cmd;
> > > +};
> > > +#define ps3rom_priv(dev)	((dev)->sbd.core.driver_data)
> > > +
> > 
> > Someone should invent a keyboard which delivers an electric shock when the
> > operator types "#define".   In the meanwhile, I get to do the honours.
> > 
> > Please don't implement in a macro anything which can be implemented in C.
> 
> All I needed was a shorthand to access driver_data, for both read and write
> access (you cannot do the latter with C, unless you decouple read and write).

Oh dear.

	ps3rom_priv(dev) = host;

that's 'orrid.  We have an identifier pretending to be a function, only we
go and treat it as an lvalue.

I mean, C code should look like C code, and the above just doesn't.

Sigh.

Re: [patch 2/3] ps3: BD/DVD/CD-ROM Storage Driver

[Geert Uytterhoeven]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 1948 bytes --]

On Thu, 19 Jul 2007, Andrew Morton wrote:
> On Thu, 19 Jul 2007 11:02:07 +0200 (CEST) Geert Uytterhoeven <Geert.Uytterhoeven@sonycom.com> wrote:
> 
> > On Wed, 18 Jul 2007, Andrew Morton wrote:
> > > > +struct ps3rom_private {
> > > > +	struct ps3_storage_device *dev;
> > > > +	struct scsi_cmnd *curr_cmd;
> > > > +};
> > > > +#define ps3rom_priv(dev)	((dev)->sbd.core.driver_data)
> > > > +
> > > 
> > > Someone should invent a keyboard which delivers an electric shock when the
> > > operator types "#define".   In the meanwhile, I get to do the honours.
> > > 
> > > Please don't implement in a macro anything which can be implemented in C.
> > 
> > All I needed was a shorthand to access driver_data, for both read and write
> > access (you cannot do the latter with C, unless you decouple read and write).
> 
> Oh dear.
> 
> 	ps3rom_priv(dev) = host;
> 
> that's 'orrid.  We have an identifier pretending to be a function, only we
> go and treat it as an lvalue.
> 
> I mean, C code should look like C code, and the above just doesn't.
> 
> Sigh.

Do you prefer

static inline struct ps3rom_private *ps3rom_priv_get(struct ps3_storage_device
*dev)
{
	return dev->sbd.core.driver_data;
}

static inline void ps3rom_priv_set(struct ps3_storage_device *dev,
				   struct ps3rom_private *priv)
{
	dev->sbd.core.driver_data = priv;
}

instead?

With kind regards,
 
Geert Uytterhoeven
Software Architect

Sony Network and Software Technology Center Europe
The Corporate Village · Da Vincilaan 7-D1 · B-1935 Zaventem · Belgium
 
Phone:    +32 (0)2 700 8453	
Fax:      +32 (0)2 700 8622	
E-mail:   Geert.Uytterhoeven@sonycom.com	
Internet: http://www.sony-europe.com/
 	
Sony Network and Software Technology Center Europe	
A division of Sony Service Centre (Europe) N.V.	
Registered office: Technologielaan 7 · B-1840 Londerzeel · Belgium	
VAT BE 0413.825.160 · RPR Brussels	
Fortis Bank Zaventem · Swift GEBABEBB08A · IBAN BE39001382358619

Re: [patch 2/3] ps3: BD/DVD/CD-ROM Storage Driver

[Jens Axboe]

On Thu, Jul 19 2007, Geert Uytterhoeven wrote:
> On Thu, 19 Jul 2007, Andrew Morton wrote:
> > On Thu, 19 Jul 2007 11:02:07 +0200 (CEST) Geert Uytterhoeven <Geert.Uytterhoeven@sonycom.com> wrote:
> > 
> > > On Wed, 18 Jul 2007, Andrew Morton wrote:
> > > > > +struct ps3rom_private {
> > > > > +	struct ps3_storage_device *dev;
> > > > > +	struct scsi_cmnd *curr_cmd;
> > > > > +};
> > > > > +#define ps3rom_priv(dev)	((dev)->sbd.core.driver_data)
> > > > > +
> > > > 
> > > > Someone should invent a keyboard which delivers an electric shock when the
> > > > operator types "#define".   In the meanwhile, I get to do the honours.
> > > > 
> > > > Please don't implement in a macro anything which can be implemented in C.
> > > 
> > > All I needed was a shorthand to access driver_data, for both read and write
> > > access (you cannot do the latter with C, unless you decouple read and write).
> > 
> > Oh dear.
> > 
> > 	ps3rom_priv(dev) = host;
> > 
> > that's 'orrid.  We have an identifier pretending to be a function, only we
> > go and treat it as an lvalue.
> > 
> > I mean, C code should look like C code, and the above just doesn't.
> > 
> > Sigh.
> 
> Do you prefer
> 
> static inline struct ps3rom_private *ps3rom_priv_get(struct ps3_storage_device
> *dev)
> {
> 	return dev->sbd.core.driver_data;
> }
> 
> static inline void ps3rom_priv_set(struct ps3_storage_device *dev,
> 				   struct ps3rom_private *priv)
> {
> 	dev->sbd.core.driver_data = priv;
> }

how about just killing them? it makes the code harder to read, what is
the point of abstracting something like that out?

-- 
Jens Axboe

Re: [patch 2/3] ps3: BD/DVD/CD-ROM Storage Driver

[Andrew Morton]

On Thu, 19 Jul 2007 11:39:32 +0200 (CEST) Geert Uytterhoeven <Geert.Uytterhoeven@sonycom.com> wrote:

> > Oh dear.
> > 
> > 	ps3rom_priv(dev) = host;
> > 
> > that's 'orrid.  We have an identifier pretending to be a function, only we
> > go and treat it as an lvalue.
> > 
> > I mean, C code should look like C code, and the above just doesn't.
> > 
> > Sigh.
> 
> Do you prefer
> 
> static inline struct ps3rom_private *ps3rom_priv_get(struct ps3_storage_device
> *dev)
> {
> 	return dev->sbd.core.driver_data;
> }
> 
> static inline void ps3rom_priv_set(struct ps3_storage_device *dev,
> 				   struct ps3rom_private *priv)
> {
> 	dev->sbd.core.driver_data = priv;
> }
> 
> instead?

Yes.  Not that it's a terribly important issue, particularly down in the
dark and dusty corners where this code lurks.  The "function" as an lvalue
thing would be more obnoxious in some top-level interface.  We probably
already have some :(

If it were mine I'd open-code the "set" and do the "get" as a static
inline.  Or you could leave it as-is and go off and do something more
important ;)

Re: [patch 2/3] ps3: BD/DVD/CD-ROM Storage Driver

[Rene Herman]

On 07/19/2007 11:47 AM, Andrew Morton wrote:

> On Thu, 19 Jul 2007 11:39:32 +0200 (CEST) Geert Uytterhoeven <Geert.Uytterhoeven@sonycom.com> wrote:
> 
>>> Oh dear.
>>>
>>> 	ps3rom_priv(dev) = host;
>>>
>>> that's 'orrid.  We have an identifier pretending to be a function, only we
>>> go and treat it as an lvalue.
>>>
>>> I mean, C code should look like C code, and the above just doesn't.
>>>
>>> Sigh.

You could insist that it be PS3ROM_PRIV() because then it at least also 
_looks_ like cpp...

Rene.

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

[Geert Uytterhoeven]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain, Size: 14401 bytes --]

From: Geert Uytterhoeven <Geert.Uytterhoeven@sonycom.com>

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

CC: Geoff Levand <geoffrey.levand@am.sony.com>
Signed-off-by: Geert Uytterhoeven <Geert.Uytterhoeven@sonycom.com>
---
v3:
  - Cleanup #includes
  - Allow to disable ps3flash (and the preallocated 256 KiB buffer) using
    `ps3flash=off'
  - Move selection of write/read strings to error path
  - Rename and move ps3stor_interrupt() to ps3flash_interrupt()
  - Pass the interrupt handler to ps3stor_setup()
  - Reset ps3flash_priv(dev) to NULL during cleanup

v2:
  - Don't use `default y' in Kconfig
  - #include <linux/uaccess.h> instead of <asm/uaccess.h>
  - Set up sysfs links between misc character device and PS3 system device:
      o /sys/class/misc/ps3flash/device -> ../../../devices/ps3_system/sb_01
      o /sys/devices/ps3_system/sb_01/misc:ps3flash -> ../../../class/misc/ps3flash

 arch/powerpc/platforms/ps3/Kconfig |   15 +
 drivers/char/Makefile              |    2 
 drivers/char/ps3flash.c            |  429 +++++++++++++++++++++++++++++++++++++
 3 files changed, 446 insertions(+)

--- a/arch/powerpc/platforms/ps3/Kconfig
+++ b/arch/powerpc/platforms/ps3/Kconfig
@@ -123,4 +123,19 @@ config PS3_ROM
 	  In general, all users will say Y or M.
 	  Also make sure to say Y or M to "SCSI CDROM support" later.
 
+config PS3_FLASH
+	tristate "PS3 FLASH ROM Storage Driver"
+	depends on PPC_PS3
+	select PS3_STORAGE
+	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.
+
+	  As this driver needs a fixed buffer of 256 KiB of memory, it can
+	  be disabled on the kernel command line using "ps3flash=off", to
+	  not allocate this fixed buffer.
+
 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,429 @@
+/*
+ * 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/fs.h>
+#include <linux/miscdevice.h>
+#include <linux/uaccess.h>
+
+#include <asm/lv1call.h>
+#include <asm/ps3stor.h>
+
+
+#define DEVICE_NAME		"ps3flash"
+
+#define FLASH_BLOCK_SIZE	(256*1024)
+
+
+struct ps3flash_private {
+	struct mutex mutex;	/* Bounce buffer 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)
+{
+	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__, write ? "write" : "read", 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;
+	unsigned int sec_off;
+
+	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);
+				sec_off = start_read_sector-start_write_sector;
+				res = ps3flash_read_sectors(dev,
+							    start_read_sector,
+							    tail, sec_off);
+				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 irqreturn_t ps3flash_interrupt(int irq, void *data)
+{
+	struct ps3_storage_device *dev = data;
+	int res;
+	u64 tag, status;
+
+	res = lv1_storage_get_async_status(dev->sbd.dev_id, &tag, &status);
+
+	if (tag != dev->tag)
+		dev_err(&dev->sbd.core,
+			"%s:%u: tag mismatch, got %lx, expected %lx\n",
+			__func__, __LINE__, tag, dev->tag);
+
+	if (res) {
+		dev_err(&dev->sbd.core, "%s:%u: res=%d status=0x%lx\n",
+			__func__, __LINE__, res, status);
+	} else {
+		dev->lv1_status = status;
+		complete(&dev->done);
+	}
+	return IRQ_HANDLED;
+}
+
+
+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 -ENODEV;
+
+	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, ps3flash_interrupt);
+	if (error)
+		goto fail_free_priv;
+
+	ps3flash_misc.parent = &dev->sbd.core;
+	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);
+	ps3flash_priv(dev) = NULL;
+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_priv(dev) = NULL;
+	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);
+}
+
+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");
+MODULE_ALIAS(PS3_MODULE_ALIAS_STOR_FLASH);

-- 
With kind regards,
 
Geert Uytterhoeven
Software Architect

Sony Network and Software Technology Center Europe
The Corporate Village · Da Vincilaan 7-D1 · B-1935 Zaventem · Belgium
 
Phone:    +32 (0)2 700 8453	
Fax:      +32 (0)2 700 8622	
E-mail:   Geert.Uytterhoeven@sonycom.com	
Internet: http://www.sony-europe.com/
 	
Sony Network and Software Technology Center Europe	
A division of Sony Service Centre (Europe) N.V.	
Registered office: Technologielaan 7 · B-1840 Londerzeel · Belgium	
VAT BE 0413.825.160 · RPR Brussels	
Fortis Bank Zaventem · Swift GEBABEBB08A · IBAN BE39001382358619

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

[Andrew Morton]

On Mon, 16 Jul 2007 18:15:42 +0200
Geert Uytterhoeven <Geert.Uytterhoeven@sonycom.com> wrote:

> From: Geert Uytterhoeven <Geert.Uytterhoeven@sonycom.com>
> 
> 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
> 
> --- /dev/null
> +++ b/drivers/char/ps3flash.c
> @@ -0,0 +1,429 @@
> +/*
> + * 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/fs.h>
> +#include <linux/miscdevice.h>
> +#include <linux/uaccess.h>
> +
> +#include <asm/lv1call.h>
> +#include <asm/ps3stor.h>
> +
> +
> +#define DEVICE_NAME		"ps3flash"
> +
> +#define FLASH_BLOCK_SIZE	(256*1024)
> +
> +
> +struct ps3flash_private {
> +	struct mutex mutex;	/* Bounce buffer mutex */
> +};
> +#define ps3flash_priv(dev)	((dev)->sbd.core.driver_data)

bzzzt!

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

lseek implementations usually need locking.  file->f_mapping->host->i_mutex
would be typical.

That locking mostly protects 64-bit f_pos on 32-bit architectures so you
can perahps kinda get away with it here.  However the code is a bit racy
even on 64-bit, for silly userspace.

That being said, I'd have thought that you could use one of our many
generic lseek library fucntions here, or even an newly-exported
block_llseek().  That assumes that i_size is correct, which I trust is the
case.

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

There are several nasty deeply-embedded return points in this function.

> +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;
> +	unsigned int sec_off;
> +
> +	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) {

checkpatch missed stuff here.

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

It's strange to see a do_div_llr() in the middle of all this 64-bit-only
code.  Could use / and %?

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

checkpatch missed this too.

> +	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);
> +				sec_off = start_read_sector-start_write_sector;
> +				res = ps3flash_read_sectors(dev,
> +							    start_read_sector,
> +							    tail, sec_off);
> +				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 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 -ENODEV;
> +
> +	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, ps3flash_interrupt);
> +	if (error)
> +		goto fail_free_priv;
> +
> +	ps3flash_misc.parent = &dev->sbd.core;
> +	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);
> +	ps3flash_priv(dev) = NULL;
> +fail:
> +	ps3flash_dev = NULL;
> +	return error;
> +}
> +

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

[Geert Uytterhoeven]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 4687 bytes --]

	Hi Andrew,

On Wed, 18 Jul 2007, Andrew Morton wrote:
> > +struct ps3flash_private {
> > +	struct mutex mutex;	/* Bounce buffer mutex */
> > +};
> > +#define ps3flash_priv(dev)	((dev)->sbd.core.driver_data)
> 
> bzzzt!

Same defense as ps3rom ;-)

> > +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;
> > +}
> 
> lseek implementations usually need locking.  file->f_mapping->host->i_mutex
> would be typical.
> 
> That locking mostly protects 64-bit f_pos on 32-bit architectures so you
> can perahps kinda get away with it here.  However the code is a bit racy
> even on 64-bit, for silly userspace.
> 
> That being said, I'd have thought that you could use one of our many
> generic lseek library fucntions here, or even an newly-exported
> block_llseek().  That assumes that i_size is correct, which I trust is the
> case.

You mean generic_file_llseek()?

Who is responsible for setting i_size in that case? This is not a file, but a
character device, so currently i_size is always zero.

So I'll just add the missing locks.

> > +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;
> > +}
> 
> There are several nasty deeply-embedded return points in this function.

Will change to `goto fail' and common error return.

> > +	if (*pos+count > size) {
> 
> checkpatch missed stuff here.

Checkpatch is silent...

> > +		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;
> 
> It's strange to see a do_div_llr() in the middle of all this 64-bit-only
> code.  Could use / and %?

Sure.

Note that in theory do_div_llr() could do the division and modulo in one
instruction on architectures that support that (hmm, that's div_long_long_rem).

With kind regards,
 
Geert Uytterhoeven
Software Architect

Sony Network and Software Technology Center Europe
The Corporate Village · Da Vincilaan 7-D1 · B-1935 Zaventem · Belgium
 
Phone:    +32 (0)2 700 8453	
Fax:      +32 (0)2 700 8622	
E-mail:   Geert.Uytterhoeven@sonycom.com	
Internet: http://www.sony-europe.com/
 	
Sony Network and Software Technology Center Europe	
A division of Sony Service Centre (Europe) N.V.	
Registered office: Technologielaan 7 · B-1840 Londerzeel · Belgium	
VAT BE 0413.825.160 · RPR Brussels	
Fortis Bank Zaventem · Swift GEBABEBB08A · IBAN BE39001382358619

Re: [patch 0/3] PS3 Storage Drivers for 2.6.23, take 5

[Alessandro Rubini]

Hello.

> I didn't hear anything from the misc device maintainer (for the FLASH ROM
> Storage Driver).

Actually, I am not acting as a maintainer. I'm not active enough nor
up to date with all the structure of kernel maintainance. So please
don't wait for me.

Actually, I tried a pair of times to have my name removed from the
MAINTAINERS file over the years without success. Actually, I didn't
care a lot because nobody relly used that entry. I think it's time for
me to learn how to do it in the proper way.

Regards
/alessandro