lkml.org 
[lkml]   [2008]   [Jan]   [12]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [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 majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

    \
     
     \ /
      Last update: 2008-01-12 21:45    [W:3.406 / U:0.788 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site