Re: libata: use WARN_ON_ONCE on hot paths

[Alan Cox]

On Fri, 9 Jan 2009 21:00:28 GMT
Linux Kernel Mailing List <linux-kernel@xxxxxxxxxxxxxxx> 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;

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