Re: SATA problems

[Marcus Haebler]

Tejun,

thanks. In preparation of your patch I installed a vanilla 2.6.20.1
kernel on my FC6
system. Amazingly the problem went away with the vanilla(!) kernel and NCQ
is enabled at boot time (queue_depth is 31). I guess I should have
tried that kernel
earlier.

The patches you sent earlier apply w/o problems against the 2.6.20.1
vanilla kernel
which is expected. I will test drive those patches tomorrow.

BTW thanks for saving me the 'cat' on the 3 patches. ;)

Thanks,

Marcus

On 2/20/07, Tejun Heo <htejun@xxxxxxxxx> wrote:
> Marcus Haebler wrote:
> > thanks for the patches! I am on an Intel P965/ICH8R.
>
> I see.  That can happen too.  There was a race window where in-flight
> r/w command which left SCSI midlayer but pending on libata gets executed
> in the wrong mode.  If possible, please verify that it doesn't happen
> with the patches applied.  I'm attaching combined patch against v2.6.20.
>
> Thanks.
>
> --
> tejun
>
> diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
> index 667acd2..348cc02 100644
> --- a/drivers/ata/libata-core.c
> +++ b/drivers/ata/libata-core.c
> @@ -308,9 +308,7 @@ int ata_build_rw_tf(struct ata_taskfile *tf, struct ata_device *dev,
>         tf->flags |= ATA_TFLAG_ISADDR | ATA_TFLAG_DEVICE;
>         tf->flags |= tf_flags;
>
> -       if ((dev->flags & (ATA_DFLAG_PIO | ATA_DFLAG_NCQ_OFF |
> -                          ATA_DFLAG_NCQ)) == ATA_DFLAG_NCQ &&
> -           likely(tag != ATA_TAG_INTERNAL)) {
> +       if (ata_ncq_enabled(dev) && likely(tag != ATA_TAG_INTERNAL)) {
>                 /* yay, NCQ */
>                 if (!lba_48_ok(block, n_block))
>                         return -ERANGE;
> diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
> index 73902d3..ebb9185 100644
> --- a/drivers/ata/libata-scsi.c
> +++ b/drivers/ata/libata-scsi.c
> @@ -945,29 +945,32 @@ int ata_scsi_change_queue_depth(struct scsi_device *sdev, int queue_depth)
>         struct ata_port *ap = ata_shost_to_port(sdev->host);
>         struct ata_device *dev;
>         unsigned long flags;
> -       int max_depth;
>
> -       if (queue_depth < 1)
> +       if (queue_depth < 1 || queue_depth == sdev->queue_depth)
>                 return sdev->queue_depth;
>
>         dev = ata_scsi_find_dev(ap, sdev);
>         if (!dev || !ata_dev_enabled(dev))
>                 return sdev->queue_depth;
>
> -       max_depth = min(sdev->host->can_queue, ata_id_queue_depth(dev->id));
> -       max_depth = min(ATA_MAX_QUEUE - 1, max_depth);
> -       if (queue_depth > max_depth)
> -               queue_depth = max_depth;
> -
> -       scsi_adjust_queue_depth(sdev, MSG_SIMPLE_TAG, queue_depth);
> -
> +       /* NCQ enabled? */
>         spin_lock_irqsave(ap->lock, flags);
> -       if (queue_depth > 1)
> -               dev->flags &= ~ATA_DFLAG_NCQ_OFF;
> -       else
> +       dev->flags &= ~ATA_DFLAG_NCQ_OFF;
> +       if (queue_depth == 1 || !ata_ncq_enabled(dev)) {
>                 dev->flags |= ATA_DFLAG_NCQ_OFF;
> +               queue_depth = 1;
> +       }
>         spin_unlock_irqrestore(ap->lock, flags);
>
> +       /* limit and apply queue depth */
> +       queue_depth = min(queue_depth, sdev->host->can_queue);
> +       queue_depth = min(queue_depth, ata_id_queue_depth(dev->id));
> +       queue_depth = min(queue_depth, ATA_MAX_QUEUE - 1);
> +
> +       if (sdev->queue_depth == queue_depth)
> +               return -EINVAL;
> +
> +       scsi_adjust_queue_depth(sdev, MSG_SIMPLE_TAG, queue_depth);
>         return queue_depth;
>  }
>
> @@ -1454,11 +1457,9 @@ static void ata_scsi_qc_complete(struct ata_queued_cmd *qc)
>  static int ata_scmd_need_defer(struct ata_device *dev, int is_io)
>  {
>         struct ata_port *ap = dev->ap;
> +       int is_ncq = is_io && ata_ncq_enabled(dev);
>
> -       if (!(dev->flags & ATA_DFLAG_NCQ))
> -               return 0;
> -
> -       if (is_io) {
> +       if (is_ncq) {
>                 if (!ata_tag_valid(ap->active_tag))
>                         return 0;
>         } else {
> diff --git a/include/linux/libata.h b/include/linux/libata.h
> index 91bb8ce..4e4e365 100644
> --- a/include/linux/libata.h
> +++ b/include/linux/libata.h
> @@ -1035,6 +1035,21 @@ static inline u8 ata_chk_status(struct ata_port *ap)
>         return ap->ops->check_status(ap);
>  }
>
> +/**
> + *     ata_ncq_enabled - Test whether NCQ is enabled
> + *     @dev: ATA device to test for
> + *
> + *     LOCKING:
> + *     spin_lock_irqsave(host lock)
> + *
> + *     RETURNS:
> + *     1 if NCQ is enabled for @dev, 0 otherwise.
> + */
> +static inline int ata_ncq_enabled(struct ata_device *dev)
> +{
> +       return (dev->flags & (ATA_DFLAG_PIO | ATA_DFLAG_NCQ_OFF |
> +                             ATA_DFLAG_NCQ)) == ATA_DFLAG_NCQ;
> +}
>
>  /**
>   *     ata_pause - Flush writes and pause 400 nanoseconds.
-
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/