Re: pata_sch: fix PSTS register reading.

[Sergei Shtylyov <sshtylyov@ru.mvista.com>]

Hello.

Denis Turischev wrote:

> pata_sch: fix PSTS register reading.
>
> According to Poulsbo SCH datasheet (p. 353): "Reading reserved bits 
> returns
> an indeterminate, inconsistent value".
> Bit 7 of PSTS register read as 1 when there is slave disk drive attached,
> despite being described as reserved with zero value. This causes wrong 
> feature
> decoding in ata_sff_tf_read.

   ata_sff_tf_read() doesn't decode anything.

> Fix it by implementing pata_sch specific sff_tf_read with appropriate bit
> masks on PSTS register.

   Your patch doesn't make any sense to me -- sorry...

> Signed-off-by: Denis Turischev <denis@compulab.co.il>
> CC: stable@kernel.org
>
> --- linux-2.6.33-rc4.orig/drivers/ata/pata_sch.c    2010-01-13 
> 07:15:00.000000000 +0200
> +++ linux-2.6.33-rc4/drivers/ata/pata_sch.c    2010-01-20 
> 11:19:16.000000000 +0200
> @@ -49,6 +49,11 @@
>     USD    = (1 << 31),    /* Use Synchronous DMA */
> };
>
> +/* see SCH datasheet page 353 */
> +enum {
> +    PSTSM    = 0x7f,        /* PSTS Bit Mask */
> +};
> +
> static int sch_init_one(struct pci_dev *pdev,
>              const struct pci_device_id *ent);
> static void sch_set_piomode(struct ata_port *ap, struct ata_device 
> *adev);
> @@ -75,11 +80,47 @@
>     ATA_BMDMA_SHT(DRV_NAME),
> };
>
> +/**
> + *    sch_tf_read - input device's ATA taskfile shadow registers
> + *    @ap: Port from which input is read
> + *    @tf: ATA taskfile register set for storing input
> + *
> + *    Note: Original code is ata_sff_tf_read().
> + */
> +
> +static void sch_tf_read(struct ata_port *ap, struct ata_taskfile *tf)
> +{
> +    struct ata_ioports *ioaddr = &ap->ioaddr;
> +
> +    tf->command = ata_sff_check_status(ap);
> +    tf->feature = ioread8(ioaddr->error_addr) && PSTSM;

   1) you want a bitwise AND here (&);
   2) what you're doing is wrong, you can't mask the features register 
like that -- are you sure that *this* is the PSTS register?

>
> +    tf->nsect = ioread8(ioaddr->nsect_addr);
> +    tf->lbal = ioread8(ioaddr->lbal_addr);
> +    tf->lbam = ioread8(ioaddr->lbam_addr);
> +    tf->lbah = ioread8(ioaddr->lbah_addr);
> +    tf->device = ioread8(ioaddr->device_addr);
> +
> +    if (tf->flags & ATA_TFLAG_LBA48) {
> +        if (likely(ioaddr->ctl_addr)) {

   Useless check I think -- ioaddr->ctl_addr should always be non-zero 
with pata_sch.

> +            iowrite8(tf->ctl | ATA_HOB, ioaddr->ctl_addr);
> +            tf->hob_feature = ioread8(ioaddr->error_addr) && PSTSM;

   Same comments here.

> +            tf->hob_nsect = ioread8(ioaddr->nsect_addr);
> +            tf->hob_lbal = ioread8(ioaddr->lbal_addr);
> +            tf->hob_lbam = ioread8(ioaddr->lbam_addr);
> +            tf->hob_lbah = ioread8(ioaddr->lbah_addr);
> +            iowrite8(tf->ctl, ioaddr->ctl_addr);
> +            ap->last_ctl = tf->ctl;
> +        } else
> +            WARN_ON_ONCE(1);
> +    }
> +}
> +
> static struct ata_port_operations sch_pata_ops = {
>     .inherits        = &ata_bmdma_port_ops,
>     .cable_detect        = ata_cable_unknown,
>     .set_piomode        = sch_set_piomode,
>     .set_dmamode        = sch_set_dmamode,
> +    .sff_tf_read        = sch_tf_read,
> };
>
MBR, Sergei