Went to the simplest approach. #54
Open
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Collaborator
ManOnTheMountainTech
commented
Feb 22, 2024
ManOnTheMountainTech
commented
- Look for intellimouse taillight arrival
- Start a worker thread to thunk to pageable code
- Set the taillight black
- Rebased with main
forderud
reviewed
Feb 22, 2024
forderud
reviewed
Feb 22, 2024
forderud
reviewed
Feb 22, 2024
forderud
reviewed
Feb 22, 2024
forderud
reviewed
Feb 22, 2024
|
This is my interpretation of what the documentation says to do at https://learn.microsoft.com/en-us/windows-hardware/drivers/ddi/wdfiotarget/nf-wdfiotarget-wdfiotargetopen.
“If a call to WdfIoTargetOpen fails, the driver should call WdfObjectDelete <https://learn.microsoft.com/en-us/windows-hardware/drivers/ddi/wdfobject/nf-wdfobject-wdfobjectdelete> to delete the I/O target object.”
From: Fredrik Orderud ***@***.***>
Sent: Thursday, February 22, 2024 2:38 AM
To: forderud/IntelliMouseDriver ***@***.***>
Cc: Bryan Allen Young ***@***.***>; Assign ***@***.***>
Subject: Re: [forderud/IntelliMouseDriver] Went to the simplest approach. (PR #54)
@forderud commented on this pull request.
_____
In TailLight/SetTaillightBlack.cpp <#54 (comment)> :
+ &openParams,
+ &DeviceContext.PdoName,
+ FILE_WRITE_ACCESS);
+
+ openParams.ShareAccess = FILE_SHARE_WRITE | FILE_SHARE_READ;
+
+ NTSTATUS status = STATUS_UNSUCCESSFUL;
+
+ // Ensure freed if fails.
+ status = WdfIoTargetOpen(target, &openParams);
+ if (!NT_SUCCESS(status)) {
+ KdPrintEx((DPFLTR_IHVDRIVER_ID,
+ DPFLTR_ERROR_LEVEL,
+ "TailLight: 0x%x while opening the I/O target from worker thread\n",
+ status));
+ NukeWdfHandle<WDFIOTARGET>(target);
I don't think you should delete target here, since the object is passed as an input argument to the function. Please instead move the NukeWdfHandle call to the client code, and call it if TryToOpenIoTarget fails.
—
Reply to this email directly, view it on GitHub <#54 (review)> , or unsubscribe <https://github.com/notifications/unsubscribe-auth/AGBDV72R7FAJDQ6BSL7BBGLYU4NXRAVCNFSM6AAAAABDUIRGV6VHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMYTQOJVGUYDMMBRG4> .
You are receiving this because you were assigned. <https://github.com/notifications/beacon/AGBDV75I4JTLAZR7JWFKRYTYU4NXRA5CNFSM6AAAAABDUIRGV6WGG33NNVSW45C7OR4XAZNRKB2WY3CSMVYXKZLTORJGK5TJMV32UY3PNVWWK3TUL5UWJTTQ7MQGC.gif> Message ID: ***@***.*** ***@***.***> >
|
I fully agree with you on that. You should still delete the |
|
WDF assigns a generic pool tag to allocations unless overridden. I was probably getting around to setting the Intellimouse WDF pool tag but never completed the task. The goal was to be able to narrow down leaky code by specific pool tags. In any case it’s removed.
From: Fredrik Orderud ***@***.***>
Sent: Thursday, February 22, 2024 2:40 AM
To: forderud/IntelliMouseDriver ***@***.***>
Cc: Bryan Allen Young ***@***.***>; Assign ***@***.***>
Subject: Re: [forderud/IntelliMouseDriver] Went to the simplest approach. (PR #54)
@forderud commented on this pull request.
_____
In TailLight/driver.h <#54 (comment)> :
/** Memory allocation tag name (for debugging leaks). */
-static constexpr ULONG POOL_TAG = 'ffly';
+static constexpr ULONG POOL_TAG = 'ylff';
+static constexpr ULONG WDF_POOL_TAG = 'wTLT'; // Taillight WPF allocated
Comments:
* Please don't bundle changes to POOL_TAG with the other changes in this PR. Submit it as a separate PR if you want to change it.
* Why are you introducing WDF_POOL_TAG? It doesn't seem to be used anywhere?
—
Reply to this email directly, view it on GitHub <#54 (review)> , or unsubscribe <https://github.com/notifications/unsubscribe-auth/AGBDV7532JM5VGCD5AS2HATYU4OBZAVCNFSM6AAAAABDUIRGV6VHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMYTQOJVGUYTCNRZGY> .
You are receiving this because you were assigned. <https://github.com/notifications/beacon/AGBDV73PW4QWO2L3B6W53CDYU4OBZA5CNFSM6AAAAABDUIRGV6WGG33NNVSW45C7OR4XAZNRKB2WY3CSMVYXKZLTORJGK5TJMV32UY3PNVWWK3TUL5UWJTTQ7M3JA.gif> Message ID: ***@***.*** ***@***.***> >
|
|
I had set it for all Taillight WDF allocations. In any case, for another PR.
From: Fredrik Orderud ***@***.***>
Sent: Thursday, February 22, 2024 2:40 AM
To: forderud/IntelliMouseDriver ***@***.***>
Cc: Bryan Allen Young ***@***.***>; Assign ***@***.***>
Subject: Re: [forderud/IntelliMouseDriver] Went to the simplest approach. (PR #54)
@forderud commented on this pull request.
_____
In TailLight/driver.h <#54 (comment)> :
/** Memory allocation tag name (for debugging leaks). */
-static constexpr ULONG POOL_TAG = 'ffly';
+static constexpr ULONG POOL_TAG = 'ylff';
+static constexpr ULONG WDF_POOL_TAG = 'wTLT'; // Taillight WPF allocated
Comments:
* Please don't bundle changes to POOL_TAG with the other changes in this PR. Submit it as a separate PR if you want to change it.
* Why are you introducing WDF_POOL_TAG? It doesn't seem to be used anywhere?
—
Reply to this email directly, view it on GitHub <#54 (review)> , or unsubscribe <https://github.com/notifications/unsubscribe-auth/AGBDV7532JM5VGCD5AS2HATYU4OBZAVCNFSM6AAAAABDUIRGV6VHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMYTQOJVGUYTCNRZGY> .
You are receiving this because you were assigned. <https://github.com/notifications/beacon/AGBDV73PW4QWO2L3B6W53CDYU4OBZA5CNFSM6AAAAABDUIRGV6WGG33NNVSW45C7OR4XAZNRKB2WY3CSMVYXKZLTORJGK5TJMV32UY3PNVWWK3TUL5UWJTTQ7M3JA.gif> Message ID: ***@***.*** ***@***.***> >
|
I still see the pool tag change in this PR. You'll need to (force) push your changes if you want to update the PR on the server. |
|
@ManOnTheMountainTech Any progress on updating this PR? |
|
Symbolic links are supposed to be opaque. Parsing the string representation of them into a hardware ID is not good.
First, get our hardware ID and USB configuration.
USB IO targets can have their interfaces and endpoints selected. I’m working on getting the USB device descriptor for the device that is represented by the symbolic link. Everything’s parsed out this way and defined by an open spec.
Next, configure the IO target’s interface and endpoint to match our endpoint.
Use that io target for the SetBlack() call.
I could also get the hardware ID from the device represented by the symbolic link, and match to see if it’s our hardware ID.
From: Fredrik Orderud ***@***.***>
Sent: Monday, February 26, 2024 10:11 AM
To: forderud/IntelliMouseDriver ***@***.***>
Cc: Bryan Allen Young ***@***.***>; Mention ***@***.***>
Subject: Re: [forderud/IntelliMouseDriver] Went to the simplest approach. (PR #54)
@ManOnTheMountainTech <https://github.com/ManOnTheMountainTech> Any progress on updating this PR?
—
Reply to this email directly, view it on GitHub <#54 (comment)> , or unsubscribe <https://github.com/notifications/unsubscribe-auth/AGBDV77IHEMVO3C6WWMVKO3YVTF2PAVCNFSM6AAAAABDUIRGV6VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTSNRUHAYTGMRUGM> .
You are receiving this because you were mentioned. <https://github.com/notifications/beacon/AGBDV7ZX4QFF372EARSEBJDYVTF2PA5CNFSM6AAAAABDUIRGV6WGG33NNVSW45C7OR4XAZNMJFZXG5LFINXW23LFNZ2KUY3PNVWWK3TUL5UWJTTVDSV3W.gif> Message ID: ***@***.*** ***@***.***> >
|
|
Gentle reminder. |
|
I’ve been working on it yesterday and today.
The PnP callback is only being called once on another old system that I set up. The symbolic link returned is failing to open. On another machine, OpenTarget is working as there are more callbacks.
From: Fredrik Orderud ***@***.***>
Sent: Sunday, March 03, 2024 9:44 AM
To: forderud/IntelliMouseDriver ***@***.***>
Cc: Bryan Allen Young ***@***.***>; Mention ***@***.***>
Subject: Re: [forderud/IntelliMouseDriver] Went to the simplest approach. (PR #54)
Gentle reminder.
—
Reply to this email directly, view it on GitHub <#54 (comment)> , or unsubscribe <https://github.com/notifications/unsubscribe-auth/AGBDV72JGTVVJIOPOYBM3LTYWNOOPAVCNFSM6AAAAABDUIRGV6VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTSNZVGI2DCOBUHA> .
You are receiving this because you were mentioned. <https://github.com/notifications/beacon/AGBDV7ZBXAETFLNHUHMYDYLYWNOOPA5CNFSM6AAAAABDUIRGV6WGG33NNVSW45C7OR4XAZNMJFZXG5LFINXW23LFNZ2KUY3PNVWWK3TUL5UWJTTVXPGHQ.gif> Message ID: ***@***.*** ***@***.***> >
|
That's probably good, but I'm struggling to understand why that prevents you from updating the PR to address the other review comments you received more than a week ago. |
|
Last week had six appointments between both houses and 12 hours of travel
time. In addition, I needed to attend a VP promotion party for my wife at
Microsoft
https://www.linkedin.com/feed/update/urn:li:activity:7169478179806547968/.
Setting up another test machine took time. The login screen came up black
after install which took some time to diagnose. Not attending the VP
promotion event would have been a marriage-testing event. In short, I did
not get all my hours in.
Instead of addressing all the issues in the PR at once I'll separate out
what is ready and check those in. I'll be catching up this week and through
the weekend.
…_____
From: Fredrik Orderud ***@***.***>
Sent: Monday, March 4, 2024 1:28:16 AM
To: forderud/IntelliMouseDriver ***@***.***>
Cc: Bryan Allen Young ***@***.***>; Mention
***@***.***>
Subject: Re: [forderud/IntelliMouseDriver] Went to the simplest approach.
(PR #54)
I've been working on it yesterday and today. The PnP callback is only being
called once on another old system that I set up. The symbolic link returned
is failing to open. On another machine, OpenTarget is working as there are
more callbacks.
That's probably good, but I'm struggling to understand why that prevents you
from updating the PR to address the other review comments you received more
than a week ago.
-
Reply to this email directly, view it on GitHub
<#54 (comment)
632> , or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AGBDV74B6HWWD3KRSIAOY73YW
Q5DBAVCNFSM6AAAAABDUIRGV6VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTSNZWGEYTA
NRTGI> .
You are receiving this because you were mentioned.
<https://github.com/notifications/beacon/AGBDV77F4QAWHOLZULY3LA3YWQ5DBA5CNFS
M6AAAAABDUIRGV6WGG33NNVSW45C7OR4XAZNMJFZXG5LFINXW23LFNZ2KUY3PNVWWK3TUL5UWJTT
VZEHCQ.gif> Message ID:
***@***.***>
|
|
Do you want (a) checkin to occur first, then the questions to the PR answered, or (b) can I answer first, then checkin?
From: Fredrik Orderud ***@***.***>
Sent: Monday, March 04, 2024 1:28 AM
To: forderud/IntelliMouseDriver ***@***.***>
Cc: Bryan Allen Young ***@***.***>; Mention ***@***.***>
Subject: Re: [forderud/IntelliMouseDriver] Went to the simplest approach. (PR #54)
I’ve been working on it yesterday and today. The PnP callback is only being called once on another old system that I set up. The symbolic link returned is failing to open. On another machine, OpenTarget is working as there are more callbacks.
That's probably good, but I'm struggling to understand why that prevents you from updating the PR to address the other review comments you received more than a week ago.
—
Reply to this email directly, view it on GitHub <#54 (comment)> , or unsubscribe <https://github.com/notifications/unsubscribe-auth/AGBDV74B6HWWD3KRSIAOY73YWQ5DBAVCNFSM6AAAAABDUIRGV6VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTSNZWGEYTANRTGI> .
You are receiving this because you were mentioned. <https://github.com/notifications/beacon/AGBDV77F4QAWHOLZULY3LA3YWQ5DBA5CNFSM6AAAAABDUIRGV6WGG33NNVSW45C7OR4XAZNMJFZXG5LFINXW23LFNZ2KUY3PNVWWK3TUL5UWJTTVZEHCQ.gif> Message ID: ***@***.*** ***@***.***> >
|
Do as you wish. |
|
I have not pushed anything yet. Testing changes. It is done on my side. I'll switch to commenting after pushing.
Get Outlook for Android<https://aka.ms/AAb9ysg>
________________________________
From: Fredrik Orderud ***@***.***>
Sent: Tuesday, March 5, 2024 12:51:40 AM
To: forderud/IntelliMouseDriver ***@***.***>
Cc: Bryan Allen Young ***@***.***>; Mention ***@***.***>
Subject: Re: [forderud/IntelliMouseDriver] Went to the simplest approach. (PR #54)
@forderud commented on this pull request.
________________________________
In TailLight/SetTaillightBlack.cpp<#54 (comment)>:
+ &openParams,
+ &DeviceContext.PdoName,
+ FILE_WRITE_ACCESS);
+
+ openParams.ShareAccess = FILE_SHARE_WRITE | FILE_SHARE_READ;
+
+ NTSTATUS status = STATUS_UNSUCCESSFUL;
+
+ // Ensure freed if fails.
+ status = WdfIoTargetOpen(target, &openParams);
+ if (!NT_SUCCESS(status)) {
+ KdPrintEx((DPFLTR_IHVDRIVER_ID,
+ DPFLTR_ERROR_LEVEL,
+ "TailLight: 0x%x while opening the I/O target from worker thread\n",
+ status));
+ NukeWdfHandle<WDFIOTARGET>(target);
What do you mean by "done"? The pull request have not yet been updated to address the comment.
—
Reply to this email directly, view it on GitHub<#54 (comment)>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/AGBDV76KJYUC7T2KT3ZX4UDYWWBRZAVCNFSM6AAAAABDUIRGV6VHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMYTSMJWGI3TCMBUGE>.
You are receiving this because you were mentioned.Message ID: ***@***.***>
|
ManOnTheMountainTech
force-pushed
the
private/bryan/simplest_black_taillight
branch
from
f4c40c2 to
06e7b02
Compare
forderud
reviewed
Mar 6, 2024
forderud
reviewed
Mar 6, 2024
forderud
reviewed
Mar 6, 2024
forderud
reviewed
Mar 6, 2024
forderud
reviewed
Mar 6, 2024
forderud
reviewed
Mar 6, 2024
forderud
reviewed
Mar 6, 2024
forderud
reviewed
Mar 15, 2024
forderud
reviewed
Mar 15, 2024
forderud
reviewed
Mar 15, 2024
forderud
reviewed
Mar 15, 2024
forderud
reviewed
Mar 15, 2024
ManOnTheMountainTech
force-pushed
the
private/bryan/simplest_black_taillight
branch
from
d9f4fed to
e240868
Compare
forderud
reviewed
Mar 18, 2024
forderud
reviewed
Mar 18, 2024
ManOnTheMountainTech
force-pushed
the
private/bryan/simplest_black_taillight
branch
from
e240868 to
b8f1b6c
Compare
forderud
reviewed
Mar 19, 2024
ManOnTheMountainTech
force-pushed
the
private/bryan/simplest_black_taillight
branch
from
b8f1b6c to
7d8200c
Compare
forderud
reviewed
Mar 21, 2024
forderud
reviewed
Mar 21, 2024
forderud
reviewed
Mar 21, 2024
forderud
reviewed
Mar 21, 2024
forderud
reviewed
Mar 21, 2024
forderud
reviewed
Mar 21, 2024
forderud
reviewed
Mar 21, 2024
ManOnTheMountainTech
force-pushed
the
private/bryan/simplest_black_taillight
branch
from
7d8200c to
53dc946
Compare
This was referenced Mar 23, 2024
Restore taillight pool tag to WDF allocations. Simplified code by querying strings from only IO targets. Change comments to not use American expressions. Got to matching based on the PDO name. Tie PnP callback removal to the lifetime of the device object. Revert KdPrintEx to KdPrint. Add documentation and document multithreaded code. Swith to RAII in SetBlackAsync for the target. Improvements for better readability and reliability versus performance. Use C copy trick and update a comment.
…m struct size of 8 bytes (3) Log SetBlackAsync failures
ManOnTheMountainTech
force-pushed
the
private/bryan/simplest_black_taillight
branch
from
6e6bbd6 to
5a479d7
Compare
forderud
reviewed
Apr 1, 2024
There was a problem hiding this comment.
Thanks for the updated PR Bryan. I've now tested your changes and hereby confirm that they do indeed appear to fix #44 with no observed side-effects.
I will perform some more tests on my end and try to incorporate your changes afterwards.
|
Kernel log observed when attaching an IntelliMouse: The 22 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.