[...]
> 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-ide" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
