linux-kernel - Re: [PATCH] mflash linux support

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20090105023346.GE6115@zakalwe.fi>
Date:	Mon, 5 Jan 2009 04:33:46 +0200
From:	Heikki Orsila <shdl@...alwe.fi>
To:	unsik Kim <donari75@...il.com>
Cc:	linux-kernel@...r.kernel.org, akpm@...ux-foundation.org
Subject: Re: [PATCH] mflash linux support

A few style issues:

On Mon, Jan 05, 2009 at 10:18:54AM +0900, unsik Kim wrote:
> +/* This function also called from IRQ context */
> +static void mg_request(struct request_queue *q)
> +{
> +	struct request *req;
> +	struct mg_host *host;
> +	u32 sect_num, sect_cnt, i;
> +	u16 *buff;
> +
> +repeat:
> +	req = elv_next_request(q);
> +	if (!req)
> +		return;
> +
> +	host = req->rq_disk->private_data;
> +
> +	/* check unwanted request call */
> +	if (host->mg_do_intr)
> +		return;
> +
> +	del_timer(&host->timer);
> +
> +	if (host->reset) {
> +		if (mg_disk_init(host)) {
> +			end_request(req, 0);
> +			return;
> +		}
> +		host->reset = 0;
> +	}
> +
> +	sect_num = req->sector;
> +	/* deal whole segments */
> +	sect_cnt = req->nr_sectors;
> +
> +	/* sanity check */
> +	if (sect_num >= get_capacity(req->rq_disk) ||
> +	    ((sect_num + sect_cnt) > get_capacity(req->rq_disk))) {
> +		printk(KERN_WARNING "%s: bad access: sector=%d, count=%d\n",
> +			 req->rq_disk->disk_name, sect_num, sect_cnt);
> +		end_request(req, 0);
> +		goto repeat;
> +	}
> +
> +	if (blk_fs_request(req)) {

Do this

	if (!blk_fs_request(req))
		return;

	switch (...) {

and you can drop the following code one intendation level down.

> +		switch (rq_data_dir(req)) {
> +		case READ:
> +			if (mg_out(host, sect_num, sect_cnt, MG_CMD_RD, &mg_read_intr) !=
> MG_ERR_NONE) {
> +				mg_bad_rw_intr(host);
> +				goto repeat;

Jumping backwards with goto does not look nice. Can you create a top 
level while statement starting from a beginning of a function, and put 
this switch case list into a separate function?

> +			}
> +			break;
> +		case WRITE:
> +			/* TODO : handler */
> +			outb(MG_REG_CTRL_INTR_DISABLE, host->dev_base + MG_REG_DRV_CTRL);
> +			if (mg_out(host, sect_num, sect_cnt, MG_CMD_WR, &mg_write_intr) !=
> MG_ERR_NONE) {
> +				mg_bad_rw_intr(host);
> +				goto repeat;
> +			}
> +			del_timer(&host->timer);
> +			mg_wait(host, MG_REG_STATUS_BIT_DATA_REQ, 3000);
> +			outb(MG_REG_CTRL_INTR_ENABLE, host->dev_base + MG_REG_DRV_CTRL);
> +			if (host->error) {
> +				mg_bad_rw_intr(host);
> +				goto repeat;
> +			}
> +			buff = (u16 *)req->buffer;
> +			for (i = 0; i < MG_SECTOR_SIZE >> 1; i++) {
> +				outw(*buff, host->dev_base + MG_BUFF_OFFSET + (i << 1));
> +				buff++;
> +			}
> +			mod_timer(&host->timer, jiffies + 3 * HZ);
> +			outb(MG_CMD_WR_CONF, host->dev_base + MG_REG_COMMAND);
> +			break;
> +		default:
> +			printk(KERN_WARNING "%s:%d unknown command\n", __func__, __LINE__);
> +			end_request(req, 0);
> +			break;
> +		}
> +	}
> +}
> +
> +static int mg_ioctl(struct block_device *bdev, fmode_t fmode,
> unsigned cmd, unsigned long arg)
> +{
> +	struct hd_geometry geo;
> +	struct mg_host *host = bdev->bd_disk->private_data;
> +
> +	switch (cmd) {
> +	case HDIO_GETGEO:

	if (cmd == HDIO_GETGEO) {
		...

> +		if (!access_ok(VERIFY_WRITE, arg, sizeof(geo))) {
> +			return -EFAULT;
> +		}
> +
> +		geo.cylinders = host->id_data.cyls;
> +		geo.heads = host->id_data.heads;
> +		geo.sectors = host->id_data.sectors;
> +		geo.start = get_start_sect(bdev);
> +
> +		if (copy_to_user((void *)arg, &geo, sizeof(geo)))
> +			return -EFAULT;
> +
> +		return 0;
> +	}
> +
> +	return -ENOTTY;
> +}

Heikki
--
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/