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

[unsik Kim <donari75@gmail.com>]

2009/6/29 Bartlomiej Zolnierkiewicz <bzolnier@gmail.com>:
> From: Bartlomiej Zolnierkiewicz <bzolnier@gmail.com>
> 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@gmail.com>