Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow BoringUtils.bore to work on probes #3908

Merged
merged 2 commits into from
Mar 6, 2024

Conversation

seldridge
Copy link
Member

Extends BorginUtils.bore (and related) to work where the source is a probe. Currently, if a user tries to do this, they will get an error about being unable to probe a probe because the old code would unconditionally wrap whatever the source was in Probe/RWProbe. Change this to not wrap if the source is already a probe.

Release Notes

Allow boring from a source which is already a probe.

Extends BorginUtils.bore (and related) to work where the source is a
probe.  Currently, if a user tries to do this, they will get an error
about being unable to probe a probe because the old code would
unconditionally wrap whatever the source was in `Probe`/`RWProbe`.  Change
this to not wrap if the source is already a probe.

Signed-off-by: Schuyler Eldridge <schuyler.eldridge@sifive.com>
@seldridge seldridge added the Bugfix Fixes a bug, will be included in release notes label Mar 6, 2024
@seldridge seldridge requested a review from dtzSiFive March 6, 2024 20:21
@seldridge seldridge enabled auto-merge (squash) March 6, 2024 20:28
Copy link
Member

@dtzSiFive dtzSiFive left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like a useful extension, although I'm a little uncertain about this method's behavior in all the various cases.

Don't see this breaking things that work today AFAIK, so LGTM.

case None => purePortTypeBase
case Some(pi) =>
// If the source is already a probe, don't double wrap it in a probe.
purePortTypeBase.probeInfo match {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Things get strange if the bore/tap target is mixed probe/non-probe, but that's already true.

(or if has incompatible probe type vs requested kind)

Not sure what the errors look like if this is wrong.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not something to be addressed here unless it seems easy/reasonable to address while visiting (?), just a note.

case None if pi.writable => RWProbe(purePortTypeBase)
case None => Probe(purePortTypeBase)
}
case None => purePortTypeBase
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some of the below logic maybe can/should be tweaked, not sure and maybe better to defer that to a later simplification.
For example, if the target is a rwprobe port (new L251, old L246), and this is used by rwTap, it'll add another RWProbe for that. Possibly error but haven't traced all the paths here...

@seldridge seldridge merged commit 67f51b8 into main Mar 6, 2024
14 checks passed
@seldridge seldridge deleted the dev/seldridge/allow-boring-of-probes branch March 6, 2024 20:38
@jackkoenig jackkoenig added this to the 6.x milestone Mar 6, 2024
@mergify mergify bot added the Backported This PR has been backported label Mar 6, 2024
mergify bot pushed a commit that referenced this pull request Mar 6, 2024
Extends BorginUtils.bore (and related) to work where the source is a
probe.  Currently, if a user tries to do this, they will get an error
about being unable to probe a probe because the old code would
unconditionally wrap whatever the source was in `Probe`/`RWProbe`.  Change
this to not wrap if the source is already a probe.

Signed-off-by: Schuyler Eldridge <schuyler.eldridge@sifive.com>
(cherry picked from commit 67f51b8)
chiselbot pushed a commit that referenced this pull request Mar 6, 2024
Extends BorginUtils.bore (and related) to work where the source is a
probe.  Currently, if a user tries to do this, they will get an error
about being unable to probe a probe because the old code would
unconditionally wrap whatever the source was in `Probe`/`RWProbe`.  Change
this to not wrap if the source is already a probe.

Signed-off-by: Schuyler Eldridge <schuyler.eldridge@sifive.com>
(cherry picked from commit 67f51b8)

Co-authored-by: Schuyler Eldridge <schuyler.eldridge@sifive.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Backported This PR has been backported Bugfix Fixes a bug, will be included in release notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants