Re: [PATCH 06/21] ide-floppy: remove struct idefloppy_flexible_disk_page


On Friday 11 January 2008, Borislav Petkov wrote:
The driver used to test whether the flexible disk page has changed by memcmp-ing
it with a cached copy of a previous version of the page from a different remo-
vable medium. Since, according to the SFF-8070i spec, the flexible disk page
"specifies parameters relating to the currently installed medium type," this
comparison is now done by simply checking whether the medium has changed.

Signed-off-by: Borislav Petkov <bbpetkov@xxxxxxxx>
---
drivers/ide/ide-floppy.c | 89 ++++++++++++++++-----------------------------
1 files changed, 32 insertions(+), 57 deletions(-)

diff --git a/drivers/ide/ide-floppy.c b/drivers/ide/ide-floppy.c
index 2b9885f..679d48e 100644
--- a/drivers/ide/ide-floppy.c
+++ b/drivers/ide/ide-floppy.c

[...]

@@ -1188,50 +1159,54 @@ static int idefloppy_queue_pc_tail (ide_drive_t *drive,idefloppy_pc_t *pc)
}

/*
- * Look at the flexible disk page parameters. We will ignore the CHS
- * capacity parameters and use the LBA parameters instead.
+ * Look at the flexible disk page parameters. We will ignore the CHS capacity
+ * parameters and use the LBA parameters instead.
*/
-static int idefloppy_get_flexible_disk_page (ide_drive_t *drive)
+static int idefloppy_get_flexible_disk_page(ide_drive_t *drive)

Care to rename it to ide_floppy_get_flexible_disk_page() while at it
(it has only one user)?

{
idefloppy_floppy_t *floppy = drive->driver_data;
idefloppy_pc_t pc;
- idefloppy_mode_parameter_header_t *header;
- idefloppy_flexible_disk_page_t *page;
int capacity, lba_capacity;
+ u8 heads, sectors;
+ u16 transfer_rate, sector_size, cyls, rpm;

some CodingStyle nitpicks (not really that important, rather personal taste):

u16 transfer_rate, sector_size, cyls, rpm;
u8 heads, sectors;

- idefloppy_create_mode_sense_cmd(&pc, IDEFLOPPY_FLEXIBLE_DISK_PAGE, MODE_SENSE_CURRENT);
- if (idefloppy_queue_pc_tail(drive,&pc)) {
- printk(KERN_ERR "ide-floppy: Can't get flexible disk "
- "page parameters\n");
+ idefloppy_create_mode_sense_cmd(&pc, IDEFLOPPY_FLEXIBLE_DISK_PAGE,
+ MODE_SENSE_CURRENT);

idefloppy_create_mode_sense_cmd(&pc, IDEFLOPPY_FLEXIBLE_DISK_PAGE,
MODE_SENSE_CURRENT);

+ if (idefloppy_queue_pc_tail(drive, &pc)) {
+ printk(KERN_ERR "ide-floppy: Can't get flexible disk page"
+ " parameters\n");
return 1;
}
- header = (idefloppy_mode_parameter_header_t *) pc.buffer;
- floppy->wp = header->wp;
+ floppy->wp = pc.buffer[3] & 0x80;

This is not an equivalent transformation:

header->wp is 0 or 1
pc.buffer[3] & 0x80 is 0 or 0x80

It seems to work fine for ->wp (because it is needlessly defined as 'int')
but may seriously confuse set_disk_ro() and thus bdev_read_only() users.

Should be fixed to '(pc.buffer[3] & 0x80) ? 1 : 0' (or something similar).

set_disk_ro(floppy->disk, floppy->wp);
- page = (idefloppy_flexible_disk_page_t *) (header + 1);
-
- page->transfer_rate = be16_to_cpu(page->transfer_rate);
- page->sector_size = be16_to_cpu(page->sector_size);
- page->cyls = be16_to_cpu(page->cyls);
- page->rpm = be16_to_cpu(page->rpm);
- capacity = page->cyls * page->heads * page->sectors * page->sector_size;
- if (memcmp (page, &floppy->flexible_disk_page, sizeof (idefloppy_flexible_disk_page_t)))
+
+ transfer_rate = be16_to_cpu(*(u16 *)&pc.buffer[8 + 2]);
+ sector_size = be16_to_cpu(*(u16 *)&pc.buffer[8 + 6]);
+ cyls = be16_to_cpu(*(u16 *)&pc.buffer[8 + 8]);
+ rpm = be16_to_cpu(*(u16 *)&pc.buffer[8 + 28]);
+ heads = pc.buffer[8 + 4];
+ sectors = pc.buffer[8 + 5];
+
+ capacity = cyls * heads * sectors * sector_size;
+
+ if ((1UL << IDEFLOPPY_MEDIA_CHANGED) & floppy->flags)

IDEFLOPPY_MEDIA_CHANGED is set when block device is opened for the first
time (please check idefloppy_open() for details) so I don't think it is
the right change. 'Flexible Disk Page' is only 32 bytes so we are better
off with leaving 'u8 flexible_disk_page[32]' in idefloppy_floppy_t and
doing things the old way.

Besides please do not intermix real changes like the above one with purely
cleanup ones like idefloppy_flexible_disk_page_t removal. This is bad from
maintainability point of view. If some patch causes problems it is easier
to narrow it down by heaving purely cleanup changes separated out + if we
would need to revert the real change we would have to make a separate patch
doing it instead of just reverting the guilty commit (given that we don't
want cleanup changes to be reverted as well).

printk(KERN_INFO "%s: %dkB, %d/%d/%d CHS, %d kBps, "
"%d sector size, %d rpm\n",
- drive->name, capacity / 1024, page->cyls,
- page->heads, page->sectors,
- page->transfer_rate / 8, page->sector_size, page->rpm);
-
- floppy->flexible_disk_page = *page;
- drive->bios_cyl = page->cyls;
- drive->bios_head = page->heads;
- drive->bios_sect = page->sectors;
+ drive->name, capacity / 1024, cyls, heads, sectors,
+ transfer_rate / 8, sector_size, rpm);

more CodingStyle nitpicks:

printk(KERN_INFO "%s: %dkB, %d/%d/%d CHS, %d kBps, "
"%d sector size, %d rpm\n",
drive->name, capacity / 1024, cyls, heads, sectors,
transfer_rate / 8, sector_size, rpm);

would be more readable IMO

+ drive->bios_cyl = cyls;
+ drive->bios_head = heads;
+ drive->bios_sect = sectors;

extra newline would distinguish the above block from the code below

lba_capacity = floppy->blocks * floppy->block_size;
+
if (capacity < lba_capacity) {
printk(KERN_NOTICE "%s: The disk reports a capacity of %d "
"bytes, but the drive only handles %d\n",
drive->name, lba_capacity, capacity);
- floppy->blocks = floppy->block_size ? capacity / floppy->block_size : 0;
+ floppy->blocks = floppy->block_size ?
+ capacity / floppy->block_size : 0;
}
return 0;
}

Otherwise looks fine, please recast and resubmit.
--
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/

Relevant Pages

  • Re: 61GB in bad sectors (ref: 200GB drive reads as 128)
    ... >My problem is that I have corrupted data (61132684 KB in bad sectors) on my ... > 195350368 KB total disk space. ... A drive with 30% of its capacity in bad sectors is almost certainly ... values for reportion the capacity of their drives and so to them a ...
    (microsoft.public.windowsxp.hardware)
  • Re: [PATCH v2] libata: Dont trust current capacity values in identify words 57-58
    ... This was caused by libata trusting the drive's reported current capacity in sectors in identify words 57 and 58 if the drive does not support LBA and the ... Signed-off-by: Robert Hancock ...
    (Linux-Kernel)
  • Re: HDD size
    ... sectors is hidden from the user and will not be taken from the declared ... 100GB + the hot fix capacity. ... As hard disks manage their own hot fixing, ... to the end user and the disk appears defect fee UNTIL the hotfix space ...
    (uk.comp.homebuilt)
  • Re: [PATCH v2] libata: Dont trust current capacity values in identify words 57-58
    ... This was caused by libata trusting the drive's reported current capacity in sectors in identify words 57 and 58 if the drive does not support LBA and the ... current CHS translation values appear valid. ...
    (Linux-Kernel)
  • Determining optical media capacity
    ... When I insert a new blank CD or DVD into my burner is there some way I ... can access the capacity of that medium? ... Bob Parker ...
    (Ubuntu)