libata PIO fixes (revised)
cover libata PIO fixes (revised)
Jeff,
The PIO fixes revised according to the previous reviews.
1/2 pio1.diff:
- Modify ata_mmio_data_xfer() and ata_pio_data_xfer() to handle buffer with odd length.
2/2 pio2.diff:
- Modify __atapi_pio_bytes() to handle the case where device returns/needs extra data.
(Patch diff'ed against 2.6.13-rc6 tree, 7d69fa6266770eeb6317eddd46b64456e8a515bf)
Tested ok on x86 with Promise PDC20275 and LG DVD-Multi drive.
For your review and advice, thanks.
Albert
PATCH 1/2 libata ata_data_xfer() fix
Jeff,
PATCH 1/2: ata_data_xfer() fix
Changes:
- Modify ata_mmio_data_xfer() and ata_pio_data_xfer() to handle odd-lengthed buffer.
- Add some function comments
This patch does not reuse ap->pad as alignment buffer since using local variable seems good enough.
For your review, thanks.
Albert
drivers/scsi/libata-core.c +108 -3
@@ -2519,6 +2519,20 @@#endif /* __BIG_ENDIAN */ } +/** +* ata_mmio_data_xfer - Transfer data by MMIO +* @ap: port to read/write +* @buf: data buffer +* @buflen: buffer length +* @do_write: read/write +* +* Transfer data from/to the device data register by MMIO. +* +* LOCKING: +* Inherited from caller. +* +*/ +static void ata_mmio_data_xfer (struct ata_port *ap,unsigned char *buf,unsigned int buflen,int write_data) {
@@ -2527,6 +2541,7 @@u16 *buf16 = (u16 *) buf;void __iomem *mmio = (void __iomem *)ap->ioaddr.data_addr; +/* Transfer multiple of 2 bytes */ if (write_data) {for (i =0 ; i < words; i++)writew (le16_to_cpu (buf16[i]), mmio);
@@ -2534,19 +2549,76 @@for (i =0 ; i < words; i++) buf16[i] =cpu_to_le16 (readw (mmio)); } + +/* Transfer trailing 1 byte, if any. */ +if (unlikely (buflen &0x01 )) { +u16 align_buf[1 ] = {0 }; +unsigned char *trailing_buf = buf + buflen -1 ; + +if (write_data) { +memcpy (align_buf, trailing_buf,1 ); +writew (le16_to_cpu (align_buf[0 ]), mmio); + }else { + align_buf[0 ] =cpu_to_le16 (readw (mmio)); +memcpy (trailing_buf, align_buf,1 ); + } + } } +/** +* ata_pio_data_xfer - Transfer data by PIO +* @ap: port to read/write +* @buf: data buffer +* @buflen: buffer length +* @do_write: read/write +* +* Transfer data from/to the device data register by PIO. +* +* LOCKING: +* Inherited from caller. +* +*/ +static void ata_pio_data_xfer (struct ata_port *ap,unsigned char *buf,unsigned int buflen,int write_data) { -unsigned int dwords = buflen >>1 ; +unsigned int words = buflen >>1 ; +/* Transfer multiple of 2 bytes */ if (write_data) -outsw (ap->ioaddr.data_addr, buf, dwords); +outsw (ap->ioaddr.data_addr, buf, words);else -insw (ap->ioaddr.data_addr, buf, dwords); +insw (ap->ioaddr.data_addr, buf, words); + +/* Transfer trailing 1 byte, if any. */ +if (unlikely (buflen &0x01 )) { +u16 align_buf[1 ] = {0 }; +unsigned char *trailing_buf = buf + buflen -1 ; + +if (write_data) { +memcpy (align_buf, trailing_buf,1 ); +outw (le16_to_cpu (align_buf[0 ]), ap->ioaddr.data_addr); + }else { + align_buf[0 ] =cpu_to_le16 (inw (ap->ioaddr.data_addr)); +memcpy (trailing_buf, align_buf,1 ); + } + } } +/** +* ata_data_xfer - Transfer data from/to the data register. +* @ap: port to read/write +* @buf: data buffer +* @buflen: buffer length +* @do_write: read/write +* +* Transfer data from/to the device data register. +* +* LOCKING: +* Inherited from caller. +* +*/ +static void ata_data_xfer (struct ata_port *ap,unsigned char *buf,unsigned int buflen,int do_write) {
@@ -2556,6 +2628,16 @@ata_pio_data_xfer (ap, buf, buflen, do_write); } +/** +* ata_pio_sector - Transfer ATA_SECT_SIZE (512 bytes) of data. +* @qc: Command on going +* +* Transfer ATA_SECT_SIZE of data from/to the ATA device. +* +* LOCKING: +* Inherited from caller. +*/ +static void ata_pio_sector (struct ata_queued_cmd *qc) {int do_write = (qc->tf.flags &ATA_TFLAG_WRITE );
@@ -2594,6 +2676,18 @@kunmap (page); } +/** +* __atapi_pio_bytes - Transfer data from/to the ATAPI device. +* @qc: Command on going +* @bytes: number of bytes +* +* Transfer Transfer data from/to the ATAPI device. +* +* LOCKING: +* Inherited from caller. +* +*/ +static void __atapi_pio_bytes (struct ata_queued_cmd *qc,unsigned int bytes) {int do_write = (qc->tf.flags &ATA_TFLAG_WRITE );
@@ -2645,6 +2739,17 @@ } } +/** +* atapi_pio_bytes - Transfer data from/to the ATAPI device. +* @qc: Command on going +* +* Transfer Transfer data from/to the ATAPI device. +* +* LOCKING: +* Inherited from caller. +* +*/ +static void atapi_pio_bytes (struct ata_queued_cmd *qc) {struct ata_port *ap = qc->ap;
PATCH 2/2 libata handle the case when device returns/needs extra data
In reply to Albert LeeJeff,
PATCH 2/2:
Description:
Sometimes the device returns/needs extra data than expected.
Changes:
Modify __atapi_pio_bytes() to handle the case where device returns/needs extra data.
- for read case, discard trailing data from the device
- for write case, padding zero data to the device
For your review, thanks.
Albert
drivers/scsi/libata-core.c +25 -3
@@ -2697,10 +2697,33 @@unsigned char *buf;unsigned int offset, count; -if (qc->curbytes == qc->nbytes - bytes) +if (qc->curbytes + bytes >= qc->nbytes) ap->pio_task_state =PIO_ST_LAST ; next_sg: +if (unlikely (qc->cursg >= qc->n_elem)) { +/* +* The end of qc->sg is reached and the device expects +* more data to transfer. In order not to overrun qc->sg +* and fulfill length specified in the byte count register, +* - for read case, discard trailing data from the device +* - for write case, padding zero data to the device +*/ +u16 pad_buf[1 ] = {0 }; +unsigned int words = bytes >>1 ; +unsigned int i; + +if (words)/* warning if bytes > 1 */ +printk (KERN_WARNING "ata%u: %u bytes trailing data\n" , + ap->id, bytes); + +for (i =0 ; i < words; i++) +ata_data_xfer (ap, (unsigned char *)pad_buf,2 , do_write); + + ap->pio_task_state =PIO_ST_LAST ; +return ; + } + sg = &qc->sg[qc->cursg]; page = sg->page;
@@ -2734,9 +2757,8 @@kunmap (page); -if (bytes) { +if (bytes) goto next_sg; - } }/**
Jeff Garzik
AppliedQuoted 11 lines · Albert Lee
Jeff,
The PIO fixes revised according to the previous reviews.
1/2 pio1.diff:
- Modifyata_mmio_data_xfer()andata_pio_data_xfer()to handle buffer
with odd length.2/2 pio2.diff:
- Modify__atapi_pio_bytes()to handle the case where device
returns/needs extra data.
Applied both patches, thanks much.
Jeff