G1-ATA driver broken

If you have any questions on programming, this is the place to ask them, whether you're a newbie or an experienced programmer. Discussion on programming in general is also welcome. We will help you with programming homework, but we will not do your work for you! Any porting requests must be made in Developmental Ideas.
User avatar
SWAT
Insane DCEmu
Insane DCEmu
Posts: 191
https://www.artistsworkshop.eu/meble-kuchenne-na-wymiar-warszawa-gdzie-zamowic/
Joined: Sat Jan 31, 2004 2:34 pm
Location: Russia/Novosibirsk
Has thanked: 1 time
Been thanked: 0
Contact:

Re: G1-ATA driver broken

Post by SWAT »

I think normal mutex instead of recursive can solve this problem.
Image
User avatar
BlueCrab
The Crabby Overlord
The Crabby Overlord
Posts: 5652
Joined: Mon May 27, 2002 11:31 am
Location: Sailing the Skies of Arcadia
Has thanked: 9 times
Been thanked: 69 times
Contact:

Re: G1-ATA driver broken

Post by BlueCrab »

Well, the recursive mutex was used for a reason here -- with the way things are designed, one function path locks the mutex twice, which is why a recursive mutex is used. That would (obviously) cause a deadlock with a normal mutex. This could be solved by making that function private again, and inaccessible from the external interface, but I assume that Quzar had a good reason in mind for making it public. :wink:

That said, it shouldn't matter what kind of mutex is used. Both types of mutexes go through the same function calls, and I haven't seen any reason to believe that the recursive mutex codepaths are in any way broken. I still think that in your case, there's a problem somewhere else that's corrupting the mutex somehow. I'd guess something's either overwriting the thread id that locked the mutex or the number of times that it is locked, since neither of those matter in the non-recursive mutex. I'll look through the code of the recursive mutex again, but I don't think there's a problem there beyond the potential patch I posted earlier in the thread (that you said did not work).
User avatar
Quzar
Dream Coder
Dream Coder
Posts: 7497
Joined: Wed Jul 31, 2002 12:14 am
Location: Miami, FL
Has thanked: 4 times
Been thanked: 9 times
Contact:

Re: G1-ATA driver broken

Post by Quzar »

Recursive mutex was to prevent a possible race condition in the reinit with the datatype change. Since it was useful there, we applied it to allow the get_cmd function to be public. Making that private won't remove the need for the recursive mutex, unfortunately.
"When you post fewer lines of text than your signature, consider not posting at all." - A Wise Man
User avatar
BlueCrab
The Crabby Overlord
The Crabby Overlord
Posts: 5652
Joined: Mon May 27, 2002 11:31 am
Location: Sailing the Skies of Arcadia
Has thanked: 9 times
Been thanked: 69 times
Contact:

Re: G1-ATA driver broken

Post by BlueCrab »

Well, to be fair, you did make that function public before you did the recursive mutex change (you just had a note on it that you had to hold the mutex before calling it). :wink:
User avatar
Quzar
Dream Coder
Dream Coder
Posts: 7497
Joined: Wed Jul 31, 2002 12:14 am
Location: Miami, FL
Has thanked: 4 times
Been thanked: 9 times
Contact:

Re: G1-ATA driver broken

Post by Quzar »

Yea, I know. But I couldn't remember the reason either so I went back to the emails and chats. I would have made it static as suggested if it hadn't been for the fact that the recursion was needed elsewhere. I would go with either thread corruption (as suggested before) or some sort of as-of-yet unknown interaction with the swapping from master to slave on the G1. With the recursive mutex, it's significantly easier to slam the G1 bus compared to a standard mutex where each operation had to finish before the next.
"When you post fewer lines of text than your signature, consider not posting at all." - A Wise Man
User avatar
BlueCrab
The Crabby Overlord
The Crabby Overlord
Posts: 5652
Joined: Mon May 27, 2002 11:31 am
Location: Sailing the Skies of Arcadia
Has thanked: 9 times
Been thanked: 69 times
Contact:

Re: G1-ATA driver broken

Post by BlueCrab »

I don't see how a recursive mutex would make it any easier to slam the bus, as you put it. Requests still have to finish before another request can be made, in the end.

That said, now that you said that, I think I know what's going wrong, and it is indeed a problem because of the change to a recursive mutex.

What it amounts to is that the g1ata code locks the mutex in one thread and unlocks it in the DMA done IRQ handler, which won't work with a recursive mutex (since it prevents you from unlocking in a different thread than where it locked). That's why my patch earlier in the thread made things break sooner -- it was actually making sure that the thread ID for an IRQ handler was not the same as the previous thread that was running. Basically, SWAT's code is breaking when the thread he's doing his g1ata calls in is not the last one that was running before the IRQ handler kicks in.

So, there's two ways we can go about fixing this. 1) We can switch back to a non-recursive mutex and figure out a way around the issue that Quzar mentioned; or 2) I can hack the recursive mutex so that an IRQ can unlock the mutex regardless of what thread locked it. Since #2 is an ugly hack, I'd honestly prefer option #1 here. Any comments on that proposal, Quzar?
User avatar
Quzar
Dream Coder
Dream Coder
Posts: 7497
Joined: Wed Jul 31, 2002 12:14 am
Location: Miami, FL
Has thanked: 4 times
Been thanked: 9 times
Contact:

Re: G1-ATA driver broken

Post by Quzar »

Yea, it's easy enough to ditch the recursive mutex and simply remove the mutex protection from cdrom_exec_cmd and then in cdrom_reinit_ex have the mutex unlock directly before the call to cdrom_change_dataype instead of after. The race condition isn't a big deal, it simply means that if you call cdrom_reinit_ex, you have to ensure it completes before relying on the data coming back.
"When you post fewer lines of text than your signature, consider not posting at all." - A Wise Man
User avatar
BlueCrab
The Crabby Overlord
The Crabby Overlord
Posts: 5652
Joined: Mon May 27, 2002 11:31 am
Location: Sailing the Skies of Arcadia
Has thanked: 9 times
Been thanked: 69 times
Contact:

Re: G1-ATA driver broken

Post by BlueCrab »

Ok... New patch to try.

This one adds a new mutex function: int mutex_unlock_as_thread(mutex_t *m, kthread_t *thd). This function allows an interrupt handler (and only an interrupt handler) to pose as the specified thread for the purposes of unlocking a mutex held by that thread. It's a much cleaner solution than either of the two things I outlined earlier, but I suppose it is mostly a variant of #2 that I proposed above. That way, we don't compromise the synchronization in the cdrom code, and I didn't have to hack things up to allow any IRQ handler to unlock any mutex -- the IRQ handler has to know what thread locked the mutex in the first place. :wink:

So, if you get a chance, SWAT, try this patch against a clean checkout of current KOS and let me know if it works out for you. If I hear back that it does (or I get a chance to test before that), I'll go ahead and commit it. :)
Attachments
blah.diff
(5.57 KiB) Downloaded 55 times
User avatar
SWAT
Insane DCEmu
Insane DCEmu
Posts: 191
Joined: Sat Jan 31, 2004 2:34 pm
Location: Russia/Novosibirsk
Has thanked: 1 time
Been thanked: 0
Contact:

Re: G1-ATA driver broken

Post by SWAT »

A quick test showed a positive result! Now mutex is not locked forever.
Image
User avatar
BlueCrab
The Crabby Overlord
The Crabby Overlord
Posts: 5652
Joined: Mon May 27, 2002 11:31 am
Location: Sailing the Skies of Arcadia
Has thanked: 9 times
Been thanked: 69 times
Contact:

Re: G1-ATA driver broken

Post by BlueCrab »

SWAT wrote:A quick test showed a positive result! Now mutex is not locked forever.
Glad to hear it. I'll do one last check over the code and then I'll commit it to the repository.

Thanks for your patience! :grin:

EDIT: Committed as changeset cbb8b3.
User avatar
SWAT
Insane DCEmu
Insane DCEmu
Posts: 191
Joined: Sat Jan 31, 2004 2:34 pm
Location: Russia/Novosibirsk
Has thanked: 1 time
Been thanked: 0
Contact:

Re: G1-ATA driver broken

Post by SWAT »

Thanks for your fix! :grin:
Image
Post Reply