ata_tf_read_block() question
I have a question on CHS calculation in ata_tf_read_block().
The calculation in ata_tf_read_block() is:
block = (cyl * dev->heads + head) * dev->sectors + sect;
but ata_build_rw_tf() does:
track = (u32)block / dev->sectors; cyl = track / dev->heads; head = track % dev->heads; sect = (u32)block % dev->sectors + 1;
It seems inconsistent. The correct calculation is:
block = (cyl * dev->heads + head) * dev->sectors + sect - 1;
isn't it?
I don't have any real problem. Just noticed by code reading.
---
Atsushi Nemoto
Hello, Atsushi.
Quoted 6 lines · Atsushi Nemoto
I have a question on CHS calculation in
ata_tf_read_block().The calculation in
ata_tf_read_block()is:block = (cyl * dev->heads + head) * dev->sectors + sect;but
ata_build_rw_tf()does:
Quoted 8 lines
track = (u32)block / dev->sectors; cyl = track / dev->heads; head = track % dev->heads; sect = (u32)block % dev->sectors + 1;It seems inconsistent. The correct calculation is:
block = (cyl * dev->heads + head) * dev->sectors + sect - 1;isn't it?
Yes, indeed.
Quoted 1 line
I don't have any real problem. Just noticed by code reading.
ata_tf_read_block() currently is used only when reporting failed block address to upper layer so off-by-one bug there wouldn't be too visible, especially for the venerable CHS addressing.
Care to submit a patch w/ warning message and capping for sect == 0 case?
Thanks.
Signature · 1 line
tejun
Quoted 11 lines · Tejun Heo
Quoted 3 lines
It seems inconsistent. The correct calculation is:
block = (cyl * dev->heads + head) * dev->sectors + sect - 1;isn't it?
Yes, indeed.
Quoted 1 line
I don't have any real problem. Just noticed by code reading.
ata_tf_read_block()currently is used only when reporting failed block address to upper layer so off-by-one bug there wouldn't be too visible, especially for the venerable CHS addressing.
Thank you for checking.
Quoted 2 lines
Care to submit a patch w/ warning message and capping for sect == 0 case?
Well, I expect fix by you (or other libata hackers) since I think you can write better warning message and changelog than me ;)
---
Atsushi Nemoto
PATCH 1/1 libata: fix off-by-one error in ata_tf_read_block()
ata_tf_read_block() has off-by-one error when converting CHS address to LBA. The bug isn't very visible because ata_tf_read_block() is used only when generating sense data for a failed RW command and CHS addressing isn't used too often these days.
This problem was spotted by Atsushi Nemoto.
---
Quoted 2 lines
Well, I expect fix by you (or other libata hackers) since I think you can write better warning message and changelog than me ;)
In that case, sure. Thanks a lot for the nice spotting. :-)
drivers/ata/libata-core.c | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-)
drivers/ata/libata-core.c +7 -1
@@ -709,7 +709,13 @@ u64 ata_tf_read_block(struct ata_taskfile *tf, struct ata_device *dev) head = tf->device &0xf ; sect = tf->lbal; - block = (cyl * dev->heads + head) * dev->sectors + sect; +if (!sect) { +ata_dev_printk (dev,KERN_WARNING ,"device reported " +"invalid CHS sector 0\n" ); + sect =1 ;/* oh well */ + } + + block = (cyl * dev->heads + head) * dev->sectors + sect -1 ; }return block;
Jeff Garzik
AppliedRe: [PATCH #upstream-fixes] libata: fix off-by-one error in ata_tf_read_block()
Quoted 9 lines · Tejun Heo
ata_tf_read_block()has off-by-one error when converting CHS address to LBA. The bug isn't very visible becauseata_tf_read_block()is used only when generating sense data for a failed RW command and CHS addressing isn't used too often these days.This problem was spotted by Atsushi Nemoto.
Signed-off-by Tejun Heo<[email protected]>Reported-by Atsushi Nemoto<[email protected]>
Quoted 8 lines
---
Quoted 2 lines
Well, I expect fix by you (or other libata hackers) since I think you can write better warning message and changelog than me ;)
In that case, sure. Thanks a lot for the nice spotting. :-)
drivers/ata/libata-core.c | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-)
applied