feat: Add /dev/mvsw_fw_comm_debug device for FW debugging #305
Conversation
| @@ -0,0 +1,680 @@ | |||
| commit 586d6dd722b331bc0500cbe0e239aef4110f8ca3 | |||
There was a problem hiding this comment.
Please create the patch file with git format-patch and do not use the output of git show.
| of the firmware and gather information useful for debugging firmware-related | ||
| issues. | ||
|
|
||
| Signed-off-by: Vlad GEORGESCU <[email protected]> |
There was a problem hiding this comment.
Has this patch been sent (and reviewed) upstream?
There was a problem hiding this comment.
While the patch has undergone thorough internal reviews, including technical and security assessments, it has not been submitted or reviewed upstream yet. My understanding is that upstream review will be part of this pull request. Let me know if we need to initiate any review or process.
I want to mention that the patch was initially developed and reviewed for the 5.10 kernel version (used by us on TN48 builds). This commit represents a port of the original patch to the current github kernel version (5.15).
There was a problem hiding this comment.
Sorry for the misunderstanding. I meant upstream Linux kernel. I think the “dentOS GitHub community” lacks the resources of a proper review. Sometimes Marvell developers are around though. It’d be great if you could port it to Linux master branch and sent it upstream. From MAINTAINERS:
Taras Chornyi <[email protected]> (supporter:MARVELL PRESTERA ETHERNET SWITCH DRIVER)
"David S. Miller" <[email protected]> (maintainer:NETWORKING DRIVERS)
Eric Dumazet <[email protected]> (maintainer:NETWORKING DRIVERS)
Jakub Kicinski <[email protected]> (maintainer:NETWORKING DRIVERS)
Paolo Abeni <[email protected]> (maintainer:NETWORKING DRIVERS)
[email protected] (open list:NETWORKING DRIVERS)
[email protected] (open list)
@taraschornyiplv, was/is active in DENT though.
There was a problem hiding this comment.
Agree. I will create a new PR in https://github.com/dentproject/linux with changes from the patch.
This PR will remain open until the PR in https://github.com/dentproject/linux is merged and after that I will remove the patch.
Is this ok ?
There was a problem hiding this comment.
That archive is quite outdated. Better use https://git.kernel.org/pub/scm/linux/kernel/git/netdev/net-next.git/.
There was a problem hiding this comment.
1. Have a targeted commit in current Prestera driver (4.0) * using the patch in this PR, or * using a new PR in [dent-linux-5.15.y](https://github.com/dentproject/linux/commits/dent-linux-5.15.y/drivers/net/ethernet/marvell/prestera)
I think we should have only one of these, either upstream 5.15 + a set of patches, or a branch with changes applied, but not both.
Since we currently have the latter, I think this should be a PR in https://github.com/dentproject/linux/, not here.
There was a problem hiding this comment.
I think we should have only one of these, either upstream 5.15 + a set of patches, or a branch with changes applied, but not both.
Please give a reason. We want to work upstream for easier maintenance in the long run, so I do not understand your comment.
There was a problem hiding this comment.
My reason is that all changes Dent has for the linux kernel should be either as patches, or as commits in a forked kernel, to have them all in the same place.
Storing changes in multiple places is confusing, and makes it hard to keep track of the changes. It also creates ambiguity about where a new change should be stored, and can cause conflicts easily if changes for the same kernel parts are submitted at different places.
This is independent of any upstream work or upstream suitability. Since this is a non bug fix change, even if it were submitted now and accepted right away, it would only be available in kernel 6.12 (or later).
The change would also not be added to 5.15, which Dent uses. So if we would want the feature, we would need to add the change ourselves anyway.
And even it it were being added, the prestera driver in Dent's linux fork is significantly different than the one in mainline linux.
There was a problem hiding this comment.
Sorry, I should have read the quote you referred to better. I think, @vlaggeor meant either … or, and already looked into adding it to dent-linux-5.15.y.
The change would also not be added to 5.15
Lot’s of stuff is backported nowadays, but it’d probably be better, if dentOS used more recent Linux kernels.
There was a problem hiding this comment.
I have created PR#13 in dent-linux-5.15.y branch.
I will close this PR in 1-2 days to allow time for feedback and review.
Thank you very much for feedback and responses.
This commit introduces a new character device that allows sending commands to the secondary CPU for debugging purposes. With this device, users can execute commands on the firmware level to inspect the current status and gather information useful for debugging firmware-related issues. Signed-off-by: Vlad GEORGESCU <[email protected]>
This commit introduces a new character device
/dev/mvsw_fw_comm_debugthat allows sending commands to the secondary CPU for debugging purposes. With this device, users can execute commands on the firmware level to inspect the current status and gather information useful for debugging firmware-related issues.