On Mon, 16 Jul 2007 18:15:40 +0200
Geert Uytterhoeven <[EMAIL PROTECTED]> wrote:
> From: Geert Uytterhoeven <[EMAIL PROTECTED]>
>
> 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 [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
