SOLO/86 platform support (including ATA-CF driver)#2345
SOLO/86 platform support (including ATA-CF driver)#2345ghaerr merged 26 commits intoghaerr:masterfrom
Conversation
|
Hello @fhendrikx, Very nicely done, thank you! I have just a very few questions/comments which I'll post next to the corresponding code, and we should be good to go for commit. |
|
The above posts conclude my comments, thank you. |
|
Hi @ghaerr |
|
I'm going to commit this now. I did look at the ATA driver itself in more detail and found potentially dangerous stack overflow issue, which I will comment on separately. However, it's likely OK for now so this can be addressed later. Thank you very much for this very nice enhancement! Your ATA driver is quite clean and I'm thinking could possibly be used as the basis for a direct access HD driver not using any BIOS. I'd like to separate out the existing partition handling code from the BIOSHD driver so it could be used with this and an HD ATA driver more easily. Out of curiosity, does the ATA spec deal with interrupt-complete notifications on read/write commands, or is that handled on a per-system basis? Since all ATA I/O is PIO, I'm really not sure how much advantage an interrupt-driven HD driver might bring on a single processor system with kernel buffering, but on hard drives there will always be rotational delays that wouldn't have to be waited on within the driver. |
| */ | ||
| sector_t ata_init(unsigned int drive) | ||
| { | ||
| unsigned short buffer[ATA_SECTOR_SIZE/2]; |
There was a problem hiding this comment.
FYI: declaring any large buffers like this on the stack is usually strictly disallowed, since the kernel stack pointer is either within the 640-700 byte KSTACK_BYTES section of the currently running task structure, or, for interrupt handlers called when already executing kernel code or any functions called by them, on the fixed 512-byte ISTACK_BYTES interrupt stack.
For now, this is only working because ata_init is called only during kernel startup, on the idle task's stack, prior to any task switching, but not necessarily prior to any interrupt handler activity (e.g. direct floppy I/O (not BIOS I/O)).
Since the idle task stack is 640-700 bytes depending on CONFIG_ASYNCIO and this buffer is 512 bytes and Solo/86 isn't configured for direct floppy, it all works.
For an example of a workaround using a buffer allocated from the kernel heap, see heap_alloc() in idequery.c. That code happens to be performing the same Identify function in some old messy code dealing with IDE and CHS identification.
I'm OK with leaving this as is, with a warning comment, or you could add heap_alloc/heap_free in your next driver revision.
There was a problem hiding this comment.
I'll do another PR in the coming few days/weeks to fix this issue. TBH I didn't consider it all... too many years spent on projects where heap size wasn't a real issue. :)
| int ata_read(unsigned int drive, sector_t sector, char *buf, ramdesc_t seg) | ||
| { | ||
| unsigned char __far *buffer = _MK_FP((seg_t)seg, (unsigned)buf); | ||
| unsigned char status; |
There was a problem hiding this comment.
FYI: This code is also OK, but would fail in the case that CONFIG_XMS were enabled. For Solo/86, no issue since that won't happen, but in the general case, the ramdesc_t type becomes a 32-bit linear physical address and can't be used with _MK_FP; instead the xms_fmemcpy functions are used. For PIO read/write, a new set of wrapper functions would be required, which gets a bit complicated depending on unreal/loadall/int15 modes etc. All this would be dealt with later if this driver were turned into a full ATA HD device driver for PCs.
There was a problem hiding this comment.
Sounds like a plan.
|
I also just noticed that the CI build failed; I had CI turned off on pushes (but not PRs) so we didn't see this earlier. I'll fix this in a commit shortly. |
Yes, there are definitely interrupt driven notifications as part of the spec. You could also use DMA to drive the transfers instead of PIO. |
Thanks.. sorry, wasn't aware of that.. my builds here at home have been good. |
OK, thanks
Thank you for the support. Let me know if you want any help; happy to help. |
Fixed in 22dcd69. The problem was that CONFIG_BLK_DEV_ATA_CF defaulted to 'y' in config.in, which won't work when the BIOSHD driver is configured with either CONFIG_BLK_DEV_BFD or CONFIG_BLK_DEV_BHD. This is because they share the same major device number, but have separate I/O request routines. Thus for now, the ATA CF driver isn't easily tested outside of Solo/86 unless BIOSHD is turned off and the direct floppy driver is on. I'll think about how to best rectify this situation so that @toncho11 can play with the ATA CF driver on the Amstrad. |
Sounds good. BTW Will have a PR for the heap alloc stuff to you later today. |
2 participants
Changes to support the Solo/86 platform.
Changes include:
Please note that BIOSHD_MAJOR definition is changed to ATA_MAJOR (but we leave in place an alias so things keep working).