PATA Driver supports multiple drives #365

Quaker762 · 2019-07-26T10:35:26Z

The previous implementation of the PIIX3/4 PATA/IDE channel driver
only supported a single drive, as the object model was wrong (the channel
inherits the IRQ, not the disk drive itself). This fixes
it by 'attaching' two PATADiskDevices to a PATAChannel, which
makes more sense.

The reading/writing code is presented as is, which (somewhat) violates the
spec outlined by Seagate in the linked datasheet. That spec
is rather old, so it might not be 100% up to date, though may cause
issues on real hardware, so until we can actually test it, this will
suffice.

Quaker762 · 2019-07-26T10:36:48Z

Kernel/Devices/PATADiskDevice.h

- RefPtr<PhysicalPage> m_dma_buffer_page;
- u16 m_bus_master_base { 0 };
- Lockable<bool> m_dma_enabled;
+ PATAChannel& m_ptr_channel;


@awesomekling I'm not too sure if this is okay. I was struggling a bit with some of the smart pointers inAK so I felt it was best to just use a normal reference.

This looks correct.
A PATADiskDevice object is always owned by a PATAChannel object, and so we know that the PATAChannel will always exist as long as the PATADiskDevice does.
When we know that something is guaranteed to exist, and we're not responsible for keeping it alive, a plain reference is the most appropriate. :)
I'd suggest dropping ptr from the name though, since it's not a pointer.

Ahh good, I wasn't 100% sure but I assumed as much because the disk exists as long as it's actually connected to the channel.

I'll change the name now :)

Kernel/Devices/PATAChannel.h

awesomekling · 2019-07-26T11:34:04Z

Kernel/Devices/PATAChannel.h

+ Lockable<bool> m_dma_enabled;
+
+
+ RefPtr<PATADiskDevice> m_hd0;


How about we call these m_master and m_slave respectively?

And what do you think about this general object model description:

There should be two PATAChannel objects, with type() either Primary or Secondary.

Each PATAChannel object should in turn have two PATADiskDevice objects, master() and slave().

Yeah that sounds about right. Would I have them as static members inside the class, or would they just be instantiated somewhere??

I think the PATADiskDevice objects should be non-static members of PATAChannel, since they logically belong to the channel.

awesomekling · 2019-07-26T11:37:22Z

Kernel/Devices/PATADiskDevice.h

- RefPtr<PhysicalPage> m_dma_buffer_page;
- u16 m_bus_master_base { 0 };
- Lockable<bool> m_dma_enabled;
+ PATAChannel& m_ptr_channel;


This looks correct.
A PATADiskDevice object is always owned by a PATAChannel object, and so we know that the PATAChannel will always exist as long as the PATADiskDevice does.
When we know that something is guaranteed to exist, and we're not responsible for keeping it alive, a plain reference is the most appropriate. :)
I'd suggest dropping ptr from the name though, since it's not a pointer.

awesomekling

Please conform to the project coding style, you can use clang-format (version 8) or newer to auto-format the whitespace, braces etc.

awesomekling · 2019-07-26T14:19:52Z

Kernel/Devices/PATAChannel.cpp


-//#define DISK_DEBUG
+#define PATA_MASTER_IRQ 14


PATA_PRIMARY_IRQ and PATA_SECONDARY_IRQ, no?

awesomekling · 2019-07-26T14:21:39Z

Kernel/init.cpp

@@ -68,9 +69,10 @@ VFS* vfs;
 hang();
 }

- auto dev_hd0 = IDEDiskDevice::create(IDEDiskDevice::DriveType::MASTER);
+ auto pata0 = PATAChannel::create(PATAChannel::ChannelType::Primary);
+ //auto dev_hd0 = IDEDiskDevice::create(*pata0);


You can just delete this. Let's not leave commented-out code around. If something is worth keeping for debugging, we can put it behind an #ifdef FOO_DEBUG

awesomekling · 2019-07-26T14:26:55Z

Kernel/Devices/PATAChannel.h

+ volatile bool m_interrupted { false };
+
+ Lock m_lock { "PATAChannel" };
+ PCI::Address m_pci_address;


Hmm, so both channels will find the same PCI address here.
What do we need to do to ensure that using both disks at the same time doesn't cause problems?

Should we use a static global lock so only one disk can access at any one time? I think the controller itself can handle multiple channel requests without issues, but I'd have to test it/re-read the PIIX spec a bit.

Also I'm not too familiar with the PCI spec/code. What's the side effects of them having the same address??

Quaker762 · 2019-07-27T10:39:59Z

Please conform to the project coding style, you can use clang-format (version 8) or newer to auto-format the whitespace, braces etc.

Hmmm woops, I thought I ran this... I must've forgotten again....

The previous implementation of the PIIX3/4 PATA/IDE channel driver only supported a single drive, as the object model was wrong (the channel inherits the IRQ, not the disk drive itself). This fixes it by 'attaching' two `PATADiskDevices` to a `PATAChannel`, which makes more sense. The reading/writing code is presented as is, which violates the spec outlined by Seagate in the linked datasheet. That spec is rather old, so it might not be 100% up to date, though may cause issues on real hardware, so until we can actually test it, this will suffice.

…S#365) The previous implementation of the PIIX3/4 PATA/IDE channel driver only supported a single drive, as the object model was wrong (the channel inherits the IRQ, not the disk drive itself). This fixes it by 'attaching' two `PATADiskDevices` to a `PATAChannel`, which makes more sense. The reading/writing code is presented as is, which violates the spec outlined by Seagate in the linked datasheet. That spec is rather old, so it might not be 100% up to date, though may cause issues on real hardware, so until we can actually test it, this will suffice.

Quaker762 commented Jul 26, 2019

View reviewed changes

awesomekling requested changes Jul 26, 2019

View reviewed changes

Quaker762 force-pushed the ata_channel branch from cdbdc23 to 3e123b7 Compare July 26, 2019 12:41

awesomekling requested changes Jul 26, 2019

View reviewed changes

Quaker762 force-pushed the ata_channel branch from 3e123b7 to 18e46fe Compare July 28, 2019 13:28

awesomekling merged commit 59e122f into SerenityOS:master Jul 28, 2019

Quaker762 deleted the ata_channel branch November 3, 2019 11:19

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

PATA Driver supports multiple drives #365

PATA Driver supports multiple drives #365

Quaker762 commented Jul 26, 2019 •

edited

Loading

Quaker762 Jul 26, 2019 •

edited

Loading

awesomekling Jul 26, 2019

Quaker762 Jul 26, 2019

awesomekling Jul 26, 2019

Quaker762 Jul 26, 2019

awesomekling Jul 26, 2019

awesomekling Jul 26, 2019

awesomekling left a comment

awesomekling Jul 26, 2019

Quaker762 Jul 27, 2019

awesomekling Jul 26, 2019

awesomekling Jul 26, 2019

Quaker762 Jul 27, 2019 •

edited

Loading

Quaker762 commented Jul 27, 2019


		//#define DISK_DEBUG
		#define PATA_MASTER_IRQ 14

PATA Driver supports multiple drives #365

PATA Driver supports multiple drives #365

Conversation

Quaker762 commented Jul 26, 2019 • edited Loading

Quaker762 Jul 26, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

awesomekling left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Quaker762 Jul 27, 2019 • edited Loading

Choose a reason for hiding this comment

Quaker762 commented Jul 27, 2019

Quaker762 commented Jul 26, 2019 •

edited

Loading

Quaker762 Jul 26, 2019 •

edited

Loading

Quaker762 Jul 27, 2019 •

edited

Loading