Messages in this thread | | | Date | Thu, 8 May 2003 14:26:41 +0200 | From | Jens Axboe <> | Subject | Re: [PATCH] 2.5 ide 48-bit usage |
| |
On Thu, May 08 2003, Bartlomiej Zolnierkiewicz wrote: > > On Thu, 8 May 2003, Jens Axboe wrote: > > > On Thu, May 08 2003, Bartlomiej Zolnierkiewicz wrote: > > > > > > On Thu, 8 May 2003, Jens Axboe wrote: > > > > On Wed, May 07 2003, Jens Axboe wrote: > > > > > > Jens you your patch sets hwif->rqsize to 65535 in setup-pci.c for all > > > > > > PCI hwifs which is simply wrong as not all of them supports LBA48. > > > > > > You should check for hwif->addressing and if true set rqsize to 65536 > > > > > > (not 65535) and not in IDE PCI code but in ide_init_queue() in ide-probe.c. > > > > > > > > > > Yes you are right, that would be the best way of doing it. As it happens > > > > > for that patch, it does not hurt or break anything. But it is certainly > > > > > cleaner, I'll fix that up. > > > > > > > > That part is added, I still kept it at 65535 though akin to how we don't > > > > use that last sector in 28-bit commands either. For 48-bit commands this > > > > > > No, ide_init_queue() sets it to 256, so I want 65536 too. > > > > Alright, I don't care enough about that 1 sector to argue. > > > > > Note that it should be done after setting queue max sectors to 256, > > > because not only ide-disk depends on this code: > > > > > > max_sectors = 256; > > > > > > (...) > > > > > > /* > > > * Added "< max_sectors" check for safety if it will > > > * be called again later with rq->size = 65536. > > > * I don't believe it ever is. > > > */ > > > if (hwif->rqsize < max_sectors) > > > max_sectors = hwif->rqsize; > > > blk_queue_max_sectors(q, max_sectors); > > > if (!hwif->rqsize) > > > hwif->rqsize = hwif->addressing ? 65536 : 256; > > > > You have the logic reversed here, the hwif and drive addressing are > > reversed. Yeah, it's convoluted, dunno who thought that logic up... > > Not me. > Logic is to prevent some bugs and actually my comment "I don't believe it > ever is." is totally wrong. > > ide_init_queue() is called for all drives on hwif. > > ie. failure scenario: > 1st drive 48-bit: !rqsize -> max_sectors = 256, rqsize = 65536 > 2nd drive 28-bit: rqsize -> max_sectors = 65536 -> doh! > > so "< max_sectors" is really needed. > > It can look a bit saner: > > if (!hwif->rqsize) > hwif->rqsize = hwif->addressing ? 65536 : 256; > if (hwif->rqsize < max_sectors) > max_sectors = hwif->rqsize; > blk_queue_max_sectors(q, max_sectors);
Ugh yeah, that stinks. Your changed version looks better.
> Looks good. > Now test/review it for some time, we don't want any bugs to slip in. > :-)
I'll give it a test spin.
-- Jens Axboe
- 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/
| |