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

[Kage-Yami]

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.

[elaforma]

(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.