Re: [PATCH 06/21] ide-floppy: remove struct idefloppy_flexible_disk_page
On Saturday 12 January 2008, Borislav Petkov wrote: [...] > > > 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, >flexible_disk_page, sizeof > > > (idefloppy_flexible_disk_page_t))) > > > + > > > + transfer_rate = be16_to_cpu(*(u16 *)[8 + 2]); > > > + sector_size = be16_to_cpu(*(u16 *)[8 + 6]); > > > + cyls = be16_to_cpu(*(u16 *)[8 + 8]); > > > + rpm = be16_to_cpu(*(u16 *)[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). > > How about we get rid of that chunk altogether? floppy->flexible_disk_page is > used only here for the purpose of printk-ing to the syslog and has no "real" > purpose otherwise. Do we need that info spewed into the syslog at all? Well, it has some debugging value since drive's capabilities are given in 'Flexible Disk Page' but fine with me given that this change is separated out from idefloppy_flexible_disk_page_t removal and pushed at the end of patch series. Thanks, Bart -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 06/21] ide-floppy: remove struct idefloppy_flexible_disk_page
[...] > 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). upps, sorry, that was silly. I changed it to: floppy->wp = !!(pc.buffer[3] & 0x80); > > 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, >flexible_disk_page, sizeof > > (idefloppy_flexible_disk_page_t))) > > + > > + transfer_rate = be16_to_cpu(*(u16 *)[8 + 2]); > > + sector_size = be16_to_cpu(*(u16 *)[8 + 6]); > > + cyls = be16_to_cpu(*(u16 *)[8 + 8]); > > + rpm = be16_to_cpu(*(u16 *)[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). How about we get rid of that chunk altogether? floppy->flexible_disk_page is used only here for the purpose of printk-ing to the syslog and has no "real" purpose otherwise. Do we need that info spewed into the syslog at all? -- Regards/Gruß, Boris. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 06/21] ide-floppy: remove struct idefloppy_flexible_disk_page
On Saturday 12 January 2008, Bartlomiej Zolnierkiewicz wrote: [...] > > - 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). Update: this change belongs to patch #10 (+ the need for such change in patch #6 is a hint that #10 should be before #6) -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
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 <[EMAIL PROTECTED]>
> ---
> 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(, IDEFLOPPY_FLEXIBLE_DISK_PAGE,
> MODE_SENSE_CURRENT);
> - if (idefloppy_queue_pc_tail(drive,)) {
> - printk(KERN_ERR "ide-floppy: Can't get flexible disk "
> - "page parameters\n");
> + idefloppy_create_mode_sense_cmd(, IDEFLOPPY_FLEXIBLE_DISK_PAGE,
> + MODE_SENSE_CURRENT);
idefloppy_create_mode_sense_cmd(, IDEFLOPPY_FLEXIBLE_DISK_PAGE,
MODE_SENSE_CURRENT);
> + if (idefloppy_queue_pc_tail(drive, )) {
> + 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, >flexible_disk_page, sizeof
> (idefloppy_flexible_disk_page_t)))
> +
> + transfer_rate = be16_to_cpu(*(u16 *)[8 + 2]);
> + sector_size = be16_to_cpu(*(u16 *)[8 + 6]);
> + cyls = be16_to_cpu(*(u16 *)[8 + 8]);
> + rpm = be16_to_cpu(*(u16 *)[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",
> -
Re: [PATCH 06/21] ide-floppy: remove struct idefloppy_flexible_disk_page
On Saturday 12 January 2008, Bartlomiej Zolnierkiewicz wrote: [...] - 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). Update: this change belongs to patch #10 (+ the need for such change in patch #6 is a hint that #10 should be before #6) -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 06/21] ide-floppy: remove struct idefloppy_flexible_disk_page
[...] 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). upps, sorry, that was silly. I changed it to: floppy-wp = !!(pc.buffer[3] 0x80); 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). How about we get rid of that chunk altogether? floppy-flexible_disk_page is used only here for the purpose of printk-ing to the syslog and has no real purpose otherwise. Do we need that info spewed into the syslog at all? -- Regards/Gruß, Boris. -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 06/21] ide-floppy: remove struct idefloppy_flexible_disk_page
On Saturday 12 January 2008, Borislav Petkov wrote: [...] 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). How about we get rid of that chunk altogether? floppy-flexible_disk_page is used only here for the purpose of printk-ing to the syslog and has no real purpose otherwise. Do we need that info spewed into the syslog at all? Well, it has some debugging value since drive's capabilities are given in 'Flexible Disk Page' but fine with me given that this change is separated out from idefloppy_flexible_disk_page_t removal and pushed at the end of patch series. Thanks, Bart -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
