Messages in this thread | | | Date | Fri, 9 Jan 2009 21:32:16 +0000 | From | Alan Cox <> | Subject | Re: libata: use WARN_ON_ONCE on hot paths |
| |
On Fri, 9 Jan 2009 21:00:28 GMT Linux Kernel Mailing List <linux-kernel@vger.kernel.org> wrote:
Most of these look ok but several should never have been WARN in the first place:
@@ -4556,7 +4556,7 @@ void ata_sg_clean(struct ata_queued_cmd *qc) > struct scatterlist *sg = qc->sg; > int dir = qc->dma_dir; > > - WARN_ON(sg == NULL); > + WARN_ON_ONCE(sg == NULL);
If that ever occurs we are in deep deep doodo - this should be a BUG() so we stop the box before we do serious damage.
> @@ -651,7 +651,7 @@ void ata_sff_tf_read(struct ata_port *ap, struct ata_taskfile *tf) > iowrite8(tf->ctl, ioaddr->ctl_addr); > ap->last_ctl = tf->ctl; > } else > - WARN_ON(1); > + WARN_ON_ONCE(1);
This one should always have been a BUG(). At the point we hit this (which we never have yet) the returned data for the command executed is *lost* and we return garbage. It's a case that will only trigger due to a clear internal bug (where we want the driver author to catch it promptly) or memory corruption. It's not something that will pop up from a quirky bit of timing.
> @@ -891,7 +891,7 @@ static void ata_pio_sectors(struct ata_queued_cmd *qc) > /* READ/WRITE MULTIPLE */ > unsigned int nsect; > > - WARN_ON(qc->dev->multi_count == 0); > + WARN_ON_ONCE(qc->dev->multi_count == 0);
Ditto should be a BUG().
> - WARN_ON(qc->dev->cdb_len < 12); > + WARN_ON_ONCE(qc->dev->cdb_len < 12);
If this ever occurs we are writing garbage to the ATAPI device and continuing is a very bad idea. It could remain a warn but the command issue should be aborted
(Should be a WARN/return) if (qc->dev->cdb_len < 12) { WARN_ON_ONCE(...) return;
| |