Re: [PATCH][RESEND] mg_disk: fix issue with data integrity on error in mg_write()

[unsik Kim]

2009/6/29 Bartlomiej Zolnierkiewicz <bzolnier@xxxxxxxxx>:
> From: Bartlomiej Zolnierkiewicz <bzolnier@xxxxxxxxx>
> Subject: [PATCH] mg_disk: fix issue with data integrity on error in mg_write()
>
> We cannot acknowledge the sector write before checking its status
> (which is done on the next loop iteration) and we also need to do
> the final status register check after writing the last sector.

Right. Current mg_disk polling driver doesn't check status register
(should be ready status) after writing last sector. Thanks.


> Âstatic void mg_write(struct request *req)
> Â{
> - Â Â Â u32 j;
> Â Â Â Âstruct mg_host *host = req->rq_disk->private_data;
> + Â Â Â bool rem;
>
> Â Â Â Âif (mg_out(host, blk_rq_pos(req), blk_rq_sectors(req),
> Â Â Â Â Â Â Â Â Â MG_CMD_WR, NULL) != MG_ERR_NONE) {
> @@ -512,27 +527,37 @@ static void mg_write(struct request *req
> Â Â Â ÂMG_DBG("requested %d sects (from %ld), buffer=0x%p\n",
> Â Â Â Â Â Â Â blk_rq_sectors(req), blk_rq_pos(req), req->buffer);
>
> - Â Â Â do {
> - Â Â Â Â Â Â Â u16 *buff = (u16 *)req->buffer;
> + Â Â Â if (mg_wait(host, ATA_DRQ,
> + Â Â Â Â Â Â Â Â Â MG_TMAX_WAIT_WR_DRQ) != MG_ERR_NONE) {
> + Â Â Â Â Â Â Â mg_bad_rw_intr(host);
> + Â Â Â Â Â Â Â return;
> + Â Â Â }
> +
> + Â Â Â mg_write_one(host, req);
> +
> + Â Â Â outb(MG_CMD_WR_CONF, (unsigned long)host->dev_base + MG_REG_COMMAND);
>
> - Â Â Â if (mg_wait(host, ATA_DRQ, MG_TMAX_WAIT_WR_DRQ) != MG_ERR_NONE) {
> + Â Â Â do {
> + Â Â Â Â Â Â Â if (blk_rq_sectors(req) > 1 &&
> + Â Â Â Â Â Â Â Â Â Âmg_wait(host, ATA_DRQ,
> + Â Â Â Â Â Â Â Â Â Â Â Â Â ÂMG_TMAX_WAIT_WR_DRQ) != MG_ERR_NONE) {
> Â Â Â Â Â Â Â Â Â Â Â Âmg_bad_rw_intr(host);
> Â Â Â Â Â Â Â Â Â Â Â Âreturn;
> Â Â Â Â Â Â Â Â}
> - Â Â Â Â Â Â Â for (j = 0; j < MG_SECTOR_SIZE >> 1; j++)
> - Â Â Â Â Â Â Â Â Â Â Â outw(*buff++, (unsigned long)host->dev_base +
> - Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â MG_BUFF_OFFSET + (j << 1));
>
> - Â Â Â Â Â Â Â outb(MG_CMD_WR_CONF, (unsigned long)host->dev_base +
> - Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â MG_REG_COMMAND);
> - Â Â Â } while (mg_end_request(host, 0, MG_SECTOR_SIZE));
> + Â Â Â Â Â Â Â rem = mg_end_request(host, 0, MG_SECTOR_SIZE);
> + Â Â Â Â Â Â Â if (rem)
> + Â Â Â Â Â Â Â Â Â Â Â mg_write_one(host, req);
> +
> + Â Â Â Â Â Â Â outb(MG_CMD_WR_CONF,
> + Â Â Â Â Â Â Â Â Â Â(unsigned long)host->dev_base + MG_REG_COMMAND);
> + Â Â Â } while (rem);
> Â}

I think checking ready status after do-while loop is enough.

static void mg_write(struct request *req)
{
	u32 j;
	struct mg_host *host = req->rq_disk->private_data;

	if (mg_out(host, blk_rq_pos(req), blk_rq_sectors(req),
		   MG_CMD_WR, NULL) != MG_ERR_NONE) {
		mg_bad_rw_intr(host);
		return;
	}

	MG_DBG("requested %d sects (from %ld), buffer=0x%p\n",
	       blk_rq_sectors(req), blk_rq_pos(req), req->buffer);

	do {
		u16 *buff = (u16 *)req->buffer;

		if (mg_wait(host, ATA_DRQ, MG_TMAX_WAIT_WR_DRQ) !=
				MG_ERR_NONE) {
			mg_bad_rw_intr(host);
			return;
		}
		for (j = 0; j < MG_SECTOR_SIZE >> 1; j++)
			outw(*buff++, (unsigned long)host->dev_base +
				      MG_BUFF_OFFSET + (j << 1));

		outb(MG_CMD_WR_CONF, (unsigned long)host->dev_base +
				MG_REG_COMMAND);
	} while (mg_end_request(host, 0, MG_SECTOR_SIZE));

+	if (mg_wait(host, MG_STAT_READY, MG_TMAX_WAIT_WR_DRQ) !=
+			MG_ERR_NONE)
+		mg_bad_rw_intr(host);
}

---
Regards,
unsik Kim <donari75@xxxxxxxxx>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/