[PATCH v6 1/3] ubi: Introduce block devices for UBI volumes

[Artem Bityutskiy]

On Sun, 2014-02-16 at 17:03 -0300, Ezequiel Garcia wrote:
> +static int ubiblock_read(struct ubiblock *dev, char *buffer, int pos, int len)
> +{
> +	int leb, offset, ret;
> +	int bytes_left = len;
> +	int to_read = len;

"pos" sounds like byte offset, which cannot be true because 'int' would
be too short type for it. 

....

> +static int do_ubiblock_request(struct ubiblock *dev, struct request *req)
> +{
> +	int pos, len, ret;
> +
> +	if (req->cmd_type != REQ_TYPE_FS)
> +		return -EIO;
> +
> +	if (blk_rq_pos(req) + blk_rq_cur_sectors(req) >
> +	    get_capacity(req->rq_disk))
> +		return -EIO;
> +
> +	if (rq_data_dir(req) != READ)
> +		return -ENOSYS; /* Write not implemented */
> +
> +	pos = blk_rq_pos(req) << 9;

So 'pos' is actually the 512-byte sector number? Would you please better
name then, something self-documenting like 'sec' or 'sector' ?

> +
> +	/*
> +	 * Let's prevent the device from being removed while we're doing I/O
> +	 * work. Notice that this means we serialize all the I/O operations,
> +	 * but it's probably of no impact given the NAND core serializes
> +	 * flash acceses anywafy.
> +	 */
> +	mutex_lock(&dev->vol_mutex);
> +	ret = ubiblock_read(dev, req->buffer, pos, len);
> +	mutex_unlock(&dev->vol_mutex);

Would you please a better name for 'vol_mutex'. Just makes me confused
because this is what we use in UBI to lock the entire volume. And here
it is different mutex. Let's express the code in clearer terms and use
something like just 'device_lock' or something which would suggest that
this is locks the entire ubiblock device.

-- 
Best Regards,
Artem Bityutskiy