Skip to content
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

Jump to bottom

Suggestion: roll init into new, test with loopback + scratch pad register, and return Result instead of marking SerialPort as unsafe #30

Open
Kage-Yami opened this issue Sep 30, 2023 · 1 comment · May be fixed by #31
Open

Suggestion: roll init into new, test with loopback + scratch pad register, and return Result instead of marking SerialPort as unsafe #30

Kage-Yami opened this issue Sep 30, 2023 · 1 comment · May be fixed by #31

Comments

@Kage-Yami
Copy link

I'll preface this by saying that it's very possible that I've totally misunderstood how this all works.

As I understand it from reading https://wiki.osdev.org/Serial_Ports#Port_Addresses, it's possible that not even COM1 may exist, and so it's recommended to always test a port via loopback and the "scratch pad register". There's some example code further down the page that appears to do this:

#define PORT 0x3f8          // COM1
 
static int init_serial() {
   outb(PORT + 1, 0x00);    // Disable all interrupts
   outb(PORT + 3, 0x80);    // Enable DLAB (set baud rate divisor)
   outb(PORT + 0, 0x03);    // Set divisor to 3 (lo byte) 38400 baud
   outb(PORT + 1, 0x00);    //                  (hi byte)
   outb(PORT + 3, 0x03);    // 8 bits, no parity, one stop bit
   outb(PORT + 2, 0xC7);    // Enable FIFO, clear them, with 14-byte threshold
   outb(PORT + 4, 0x0B);    // IRQs enabled, RTS/DSR set
   outb(PORT + 4, 0x1E);    // Set in loopback mode, test the serial chip
   outb(PORT + 0, 0xAE);    // Test serial chip (send byte 0xAE and check if serial returns same byte)
 
   // Check if serial is faulty (i.e: not same byte as sent)
   if(inb(PORT + 0) != 0xAE) {
      return 1;
   }
 
   // If serial is not faulty set it in normal operation mode
   // (not-loopback with IRQs enabled and OUT#1 and OUT#2 bits enabled)
   outb(PORT + 4, 0x0F);
   return 0;
}

I was thinking it might be nice if this crate, rather than marking SerialPort::new as unsafe, instead rolled init into new (I don't see that there would ever be a reason not to call init?), and that it does the loopback + scratch pad register test. new could then return a Result depending on the outcome.
Wasabi375 added a commit to Wasabi375/uart_16550 that referenced this issue Oct 5, 2023
@Wasabi375
Add try_create and loopback_test for SerialPort
6c5e129 
fixes rust-osdev#30
Wasabi375 added a commit to Wasabi375/uart_16550 that referenced this issue Oct 5, 2023
@Wasabi375
Add try_create and loopback_test for SerialPort
17dd119 
fixes rust-osdev#30
@Wasabi375 Wasabi375 linked a pull request Oct 5, 2023 that will close this issue
Open
@elaforma
Copy link

(Not really involved with the project, so take this with a grain of salt.)

Maybe I'm misunderstanding this, but I always thought that the main reason why creating a SerialPort is unsafe is in case something other than a UART is mapped to the I/O port range you are trying to use. In principle, writing random data to a random port can do anything, including things that Rust would consider triggers for undefined behaviour (e.g., overwriting the contents of a variable you are holding a shared reference for). Remember that SerialPort::new takes an arbitrary I/O port.

This patch will not help with that: It still writes to the I/O ports before it can know whether or not a UART is attached.

The scratch-and-loopback test is still useful for detecting two situations: The port range you are probing does not have any device installed, or the UART you are talking to is malfunctioning. Neither of these cases seem unsafe-worthy to me, though. In the very worst case, without this patch, you will be sending data into the void -- not what you want, but hardly undefined behaviour.

TL;DR: If that had been all the authors were worried about, the method would not have needed to be unsafe to begin with.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Add try_create and loopback_test for SerialPort Wasabi375/uart_16550
2 participants
@Kage-Yami @elaforma