Skip to content

ide: Handle unsupported IDE commands #23

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
jeanthom merged 1 commit into from
May 25, 2025
Merged

ide: Handle unsupported IDE commands #23

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
47 changes: 26 additions & 21 deletions clicky-core/src/devices/generic/ide/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -164,9 +164,9 @@
enum IdeTransferMode {
Pio,
PioNoIORDY,
PioFlowControl(u8),

Check warning on line 167 in clicky-core/src/devices/generic/ide/mod.rs

View workflow job for this annotation

GitHub Actions / clippy

field `0` is never read 

warning: field `0` is never read
   --> clicky-core/src/devices/generic/ide/mod.rs:167:20
    |
167 |     PioFlowControl(u8),
    |     -------------- ^^
    |     |
    |     field in this variant
    |
    = note: `IdeTransferMode` has a derived impl for the trait `Debug`, but this is intentionally ignored during dead code analysis
    = note: `#[warn(dead_code)]` on by default
help: consider changing the field to be of unit type to suppress this warning while preserving the field numbering, or remove the field
    |
167 -     PioFlowControl(u8),
167 +     PioFlowControl(()),
    |
DMASingleWord(u8),

Check warning on line 168 in clicky-core/src/devices/generic/ide/mod.rs

View workflow job for this annotation

GitHub Actions / clippy

field `0` is never read 

warning: field `0` is never read
   --> clicky-core/src/devices/generic/ide/mod.rs:168:19
    |
168 |     DMASingleWord(u8),
    |     ------------- ^^
    |     |
    |     field in this variant
    |
    = note: `IdeTransferMode` has a derived impl for the trait `Debug`, but this is intentionally ignored during dead code analysis
help: consider changing the field to be of unit type to suppress this warning while preserving the field numbering, or remove the field
    |
168 -     DMASingleWord(u8),
168 +     DMASingleWord(()),
    |
DMAMultiWord(u8),

Check warning on line 169 in clicky-core/src/devices/generic/ide/mod.rs

View workflow job for this annotation

GitHub Actions / clippy

field `0` is never read 

warning: field `0` is never read
   --> clicky-core/src/devices/generic/ide/mod.rs:169:18
    |
169 |     DMAMultiWord(u8),
    |     ------------ ^^
    |     |
    |     field in this variant
    |
    = note: `IdeTransferMode` has a derived impl for the trait `Debug`, but this is intentionally ignored during dead code analysis
help: consider changing the field to be of unit type to suppress this warning while preserving the field numbering, or remove the field
    |
169 -     DMAMultiWord(u8),
169 +     DMAMultiWord(()),
    |
Reserved,
Invalid,
}
Expand Down Expand Up @@ -288,7 +288,7 @@
return None;
}

((cyl * NUM_HEADS as u64 + head) * NUM_SECTORS as u64 + sector) as u64

Check warning on line 291 in clicky-core/src/devices/generic/ide/mod.rs

View workflow job for this annotation

GitHub Actions / clippy

casting to the same type is unnecessary (`u64` -> `u64`) 

warning: casting to the same type is unnecessary (`u64` -> `u64`)
   --> clicky-core/src/devices/generic/ide/mod.rs:291:13
    |
291 |             ((cyl * NUM_HEADS as u64 + head) * NUM_SECTORS as u64 + sector) as u64
    |             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `((cyl * NUM_HEADS as u64 + head) * NUM_SECTORS as u64 + sector)`
    |
    = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#unnecessary_cast
    = note: `#[warn(clippy::unnecessary_cast)]` on by default
};

Some(offset)
Expand Down Expand Up @@ -333,10 +333,10 @@

futures_executor::block_on(async {
// TODO: async this!
if let Err(e) = self.blockdev.read_exact(self.iobuf.as_raw()).await {
// XXX: actually set error bits
return Err(e);
}

Check warning on line 339 in clicky-core/src/devices/generic/ide/mod.rs

View workflow job for this annotation

GitHub Actions / clippy

this block may be rewritten with the `?` operator 

warning: this block may be rewritten with the `?` operator
   --> clicky-core/src/devices/generic/ide/mod.rs:336:21
    |
336 | /                     if let Err(e) = self.blockdev.read_exact(self.iobuf.as_raw()).await {
337 | |                         // XXX: actually set error bits
338 | |                         return Err(e);
339 | |                     }
    | |_____________________^ help: replace it with: `self.blockdev.read_exact(self.iobuf.as_raw()).await?;`
    |
    = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#question_mark
    = note: `#[warn(clippy::question_mark)]` on by default

self.iobuf.new_transfer();
self.state = IdeDriveState::ReadReady;
Expand Down Expand Up @@ -385,10 +385,10 @@

// TODO: async this!
futures_executor::block_on(async {
if let Err(e) = self.blockdev.write_all(self.iobuf.as_raw()).await {
// XXX: actually set error bits
return Err(e);
}

Check warning on line 391 in clicky-core/src/devices/generic/ide/mod.rs

View workflow job for this annotation

GitHub Actions / clippy

this block may be rewritten with the `?` operator 

warning: this block may be rewritten with the `?` operator
   --> clicky-core/src/devices/generic/ide/mod.rs:388:17
    |
388 | /                 if let Err(e) = self.blockdev.write_all(self.iobuf.as_raw()).await {
389 | |                     // XXX: actually set error bits
390 | |                     return Err(e);
391 | |                 }
    | |_________________^ help: replace it with: `self.blockdev.write_all(self.iobuf.as_raw()).await?;`
    |
    = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#question_mark

self.iobuf.new_transfer();
self.state = IdeDriveState::WriteReady;
Expand Down Expand Up @@ -429,21 +429,14 @@
});
}

// TODO?: handle unsupported IDE command according to ATA spec
let cmd = IdeCmd::try_from(cmd).map_err(|_| ContractViolation {
msg: format!("unknown IDE command: {:#04x?}", cmd),
severity: Error, // TODO: this should be Warn, and IDE error bits should be set
stub_val: None,
})?;

(self.reg.status)
.set_bit(reg::STATUS::BSY, true)
.set_bit(reg::STATUS::ERR, false);
self.reg.error = 0;

use IdeCmd::*;
match cmd {
IdentifyDevice => {
match IdeCmd::try_from(cmd) {
Ok(IdentifyDevice) => {
let len = self.blockdev.len();

// fill the iobuf with identification info
Expand Down Expand Up @@ -475,7 +468,7 @@

Ok(())
}
ReadMultiple => {
Ok(ReadMultiple) => {
if self.cfg.multi_sect == 0 {
// TODO?: use the ATA abort mechanism instead of loudly failing
return Err(ContractViolation {
Expand All @@ -492,7 +485,7 @@
self.exec_cmd(ReadSectors as u8)
}
}
ReadDMA | ReadDMANoRetry => {
Ok(ReadDMA) | Ok(ReadDMANoRetry) => {
if !self.cfg.transfer_mode.is_dma() {
// TODO?: use the ATA abort mechanism instead of loudly failing
return Err(ContractViolation {
Expand All @@ -508,7 +501,7 @@
(self.reg.status).set_bit(reg::STATUS::BSY, false);
self.exec_cmd(ReadSectors as u8)
}
ReadSectors | ReadSectorsNoRetry => {
Ok(ReadSectors) | Ok(ReadSectorsNoRetry) => {
let offset = match self.get_sector_offset() {
Some(offset) => offset,
None => {
Expand All @@ -520,18 +513,18 @@
self.state = IdeDriveState::ReadAsyncLoad;
futures_executor::block_on(async {
// Seek into the blockdev
if let Err(e) = self.blockdev.seek(io::SeekFrom::Start(offset * 512)).await {
// XXX: actually set error bits
return Err(e);
}

Check warning on line 519 in clicky-core/src/devices/generic/ide/mod.rs

View workflow job for this annotation

GitHub Actions / clippy

this block may be rewritten with the `?` operator 

warning: this block may be rewritten with the `?` operator
   --> clicky-core/src/devices/generic/ide/mod.rs:516:21
    |
516 | /                     if let Err(e) = self.blockdev.seek(io::SeekFrom::Start(offset * 512)).await {
517 | |                         // XXX: actually set error bits
518 | |                         return Err(e);
519 | |                     }
    | |_____________________^ help: replace it with: `self.blockdev.seek(io::SeekFrom::Start(offset * 512)).await?;`
    |
    = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#question_mark

// Read the first sector from the blockdev
// TODO: this should be done asynchronously, with a separate task/thread
// notifying the IDE device when the read is completed.
if let Err(e) = self.blockdev.read_exact(self.iobuf.as_raw()).await {
// XXX: actually set error bits
return Err(e);
}

Check warning on line 527 in clicky-core/src/devices/generic/ide/mod.rs

View workflow job for this annotation

GitHub Actions / clippy

this block may be rewritten with the `?` operator 

warning: this block may be rewritten with the `?` operator
   --> clicky-core/src/devices/generic/ide/mod.rs:524:21
    |
524 | /                     if let Err(e) = self.blockdev.read_exact(self.iobuf.as_raw()).await {
525 | |                         // XXX: actually set error bits
526 | |                         return Err(e);
527 | |                     }
    | |_____________________^ help: replace it with: `self.blockdev.read_exact(self.iobuf.as_raw()).await?;`
    |
    = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#question_mark

self.remaining_sectors = if self.reg.sector_count == 0 {
256
Expand All @@ -554,14 +547,14 @@

Ok(())
}
StandbyImmediate | StandbyImmediateAlt => {
Ok(StandbyImmediate) | Ok(StandbyImmediateAlt) => {
// I mean, it's a virtual disk, there is no "spin up / spin down"
self.reg.status.set_bit(reg::STATUS::BSY, false);

// TODO: fire interrupt
Ok(())
}
WriteMultiple => {
Ok(WriteMultiple) => {
if self.cfg.multi_sect == 0 {
// TODO?: use the ATA abort mechanism instead of loudly failing
return Err(ContractViolation {
Expand All @@ -579,7 +572,7 @@
self.exec_cmd(WriteSectors as u8)
}
}
WriteDMA | WriteDMANoRetry => {
Ok(WriteDMA) | Ok(WriteDMANoRetry) => {
if !self.cfg.transfer_mode.is_dma() {
// TODO?: use the ATA abort mechanism instead of loudly failing
return Err(ContractViolation {
Expand All @@ -595,7 +588,7 @@
(self.reg.status).set_bit(reg::STATUS::BSY, false);
self.exec_cmd(WriteSectors as u8)
}
WriteSectors | WriteSectorsNoRetry => {
Ok(WriteSectors) | Ok(WriteSectorsNoRetry) => {
// NOTE: this code is somewhat UNTESTED

let offset = match self.get_sector_offset() {
Expand All @@ -609,10 +602,10 @@
self.state = IdeDriveState::WriteAsyncFlush;
futures_executor::block_on(async {
// Seek into the blockdev
if let Err(e) = self.blockdev.seek(io::SeekFrom::Start(offset * 512)).await {
// XXX: actually set error bits
return Err(e);
}

Check warning on line 608 in clicky-core/src/devices/generic/ide/mod.rs

View workflow job for this annotation

GitHub Actions / clippy

this block may be rewritten with the `?` operator 

warning: this block may be rewritten with the `?` operator
   --> clicky-core/src/devices/generic/ide/mod.rs:605:21
    |
605 | /                     if let Err(e) = self.blockdev.seek(io::SeekFrom::Start(offset * 512)).await {
606 | |                         // XXX: actually set error bits
607 | |                         return Err(e);
608 | |                     }
    | |_____________________^ help: replace it with: `self.blockdev.seek(io::SeekFrom::Start(offset * 512)).await?;`
    |
    = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#question_mark

self.remaining_sectors = if self.reg.sector_count == 0 {
256
Expand All @@ -636,7 +629,7 @@
Ok(())
}

SetMultipleMode => {
Ok(SetMultipleMode) => {
self.cfg.multi_sect = self.reg.sector_count;

// TODO: implement proper multi-sector support
Expand All @@ -650,7 +643,7 @@
Ok(())
}

SetFeatures => {
Ok(SetFeatures) => {
match self.reg.feature {
// Enable 8-bit data transfers
0x01 => self.cfg.eightbit = true,
Expand All @@ -671,7 +664,7 @@
Ok(())
}

FlushCache => {
Ok(FlushCache) => {
// uhh, we don't implement caching
(self.reg.status)
.set_bit(reg::STATUS::BSY, false)
Expand All @@ -681,7 +674,7 @@
Ok(())
}

Sleep | SleepAlt => {
Ok(Sleep) | Ok(SleepAlt) => {
// uhh, it's an emulated drive.
// just assert the irq and go on our merry way
(self.reg.status).set_bit(reg::STATUS::BSY, false);
Expand All @@ -690,11 +683,23 @@
Ok(())
}

InitializeDriveParameters => {
Ok(InitializeDriveParameters) => {
(self.reg.status).set_bit(reg::STATUS::BSY, false);
self.irq.assert();
Ok(())
}

Err(_) => {
(self.reg.status)
.set_bit(reg::STATUS::BSY, false)
.set_bit(reg::STATUS::ERR, true);
self.reg.error = 1;
Err(ContractViolation {
msg: format!("unknown IDE command: {:#04x?}", cmd),
severity: Warn,
stub_val: None,
})
}
}
}
}
Expand Down
Loading