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

[Bartlomiej Zolnierkiewicz <bzolnier@gmail.com>]

On Friday 03 July 2009 03:50:19 unsik Kim wrote:
> 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);
> }

I'm not sure about this -- I chose more invasive way cause it seems that
mg_end_request() may already complete the request (marking it as successful
and making block layer pass the data to upper layers) _before_ we really
check the status _after_ transferring the last sector of the command..