* pata_sch: fix PSTS register reading.
@ 2010-01-20 11:45 Denis Turischev
2010-01-20 13:37 ` Alan Cox
2010-01-20 13:41 ` Sergei Shtylyov
0 siblings, 2 replies; 3+ messages in thread
From: Denis Turischev @ 2010-01-20 11:45 UTC (permalink / raw)
To: jgarzik; +Cc: linux-ide, stable
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.
Fix it by implementing pata_sch specific sff_tf_read with appropriate bit
masks on PSTS register.
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;
+ 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)) {
+ iowrite8(tf->ctl | ATA_HOB, ioaddr->ctl_addr);
+ tf->hob_feature = ioread8(ioaddr->error_addr) && PSTSM;
+ 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,
};
static struct ata_port_info sch_port_info = {
^ permalink raw reply [flat|nested] 3+ messages in thread* Re: pata_sch: fix PSTS register reading.
2010-01-20 11:45 pata_sch: fix PSTS register reading Denis Turischev
@ 2010-01-20 13:37 ` Alan Cox
2010-01-20 13:41 ` Sergei Shtylyov
1 sibling, 0 replies; 3+ messages in thread
From: Alan Cox @ 2010-01-20 13:37 UTC (permalink / raw)
To: Denis Turischev; +Cc: jgarzik, linux-ide, stable
O> According to Poulsbo SCH datasheet (p. 353): "Reading reserved bits returns
> an indeterminate, inconsistent value".
Ok but
> 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.
The feature/error register doesn't have reserved bits so your patch and
diagnostic don't seem to make sense.
If the top bit is set in your error register post reset it is because you
have a device flagging a fail
0x81 means the slave flagged a fail
0x80/82-FF means both devices flagged a fail
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: pata_sch: fix PSTS register reading.
2010-01-20 11:45 pata_sch: fix PSTS register reading Denis Turischev
2010-01-20 13:37 ` Alan Cox
@ 2010-01-20 13:41 ` Sergei Shtylyov
1 sibling, 0 replies; 3+ messages in thread
From: Sergei Shtylyov @ 2010-01-20 13:41 UTC (permalink / raw)
To: Denis Turischev; +Cc: jgarzik, linux-ide, stable
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
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2010-01-20 13:43 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2010-01-20 11:45 pata_sch: fix PSTS register reading Denis Turischev
2010-01-20 13:37 ` Alan Cox
2010-01-20 13:41 ` Sergei Shtylyov
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.