Message-Id: <20070718163637.3f0e0164.akpm@linux-foundation.org>
Date:	Wed, 18 Jul 2007 16:36:37 -0700
From:	Andrew Morton <akpm@...ux-foundation.org>
To:	Geert Uytterhoeven <Geert.Uytterhoeven@...ycom.com>,
	Andy Whitcroft <apw@...dowen.org>
Cc:	Paul Mackerras <paulus@...ba.org>, Jens Axboe <axboe@...nel.dk>,
	"James E.J. Bottomley" <James.Bottomley@...elEye.com>,
	Alessandro Rubini <rubini@...ion.unipv.it>,
	linuxppc-dev@...abs.org, linux-kernel@...r.kernel.org,
	linux-scsi@...r.kernel.org,
	Geoff Levand <geoffrey.levand@...sony.com>,
	Jens Axboe <jens.axboe@...cle.com>
Subject: Re: [patch 1/3] ps3: Disk Storage Driver

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

> From: Geert Uytterhoeven <Geert.Uytterhoeven@...ycom.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;
> +}
> +

-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists