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

On Mon, 16 Jul 2007 18:15:42 +0200
Geert Uytterhoeven [off-list ref] wrote:

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