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

[Geert Uytterhoeven <Geert.Uytterhoeven@sonycom.com>]

	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