tpm_if.m: declare bus addresses as bus_size_t not bus_addr_t
ClosedPublic
Actions

Authored by rlibby on May 21 2024, 5:04 PM.

Details

Reviewers

jhibbits
kd

Commits

rGbea2bf458d2c: tpm_if.m: declare bus addresses as bus_size_t not bus_addr_t

Summary

Do like bus_space(9) does. This fixes the build on platforms where
bus_addr_t and bus_size_t are different (like i386 PAE).

Fixes: c2e9c5bbf022 tpm: Refactor TIS and add a SPI attachment

Test Plan

I don't know how to test this beyond make tinderbox. @jhibbits please feel free to take this patch over if desired.

Diff Detail

Repository

rG FreeBSD src repository

Lint

Lint Skipped

Unit

Tests Skipped

Build Status

Buildable 57820
Build 54708: arc lint + arc unit

Event Timeline

rlibby created this revision.May 21 2024, 5:04 PM

Herald added a subscriber: imp. · View Herald TranscriptMay 21 2024, 5:04 PM

rlibby requested review of this revision.May 21 2024, 5:04 PM

Harbormaster completed remote builds in B57820: Diff 138848.May 21 2024, 5:04 PM

rlibby edited the test plan for this revision. (Show Details)May 21 2024, 5:05 PM

The only time this makes a difference is if the sizes of bus_addr_t and bus_size_t are different, and I don't have any test hardware where that's the case and I can test TPM. (Only hardware I have where that's the case is ppc Book-E, but that doesn't have a TPM).

This revision is now accepted and ready to land.May 21 2024, 5:34 PM

In D45287#1033196, @jhibbits wrote:

The only time this makes a difference is if the sizes of bus_addr_t and bus_size_t are different, and I don't have any test hardware where that's the case and I can test TPM. (Only hardware I have where that's the case is ppc Book-E, but that doesn't have a TPM).

Right. Not that I think this is risky, but would you like to do any testing of tpm at all before pushing to main? Otherwise I'll go ahead and push it without testing beyond make.

In D45287#1033199, @rlibby wrote:

In D45287#1033196, @jhibbits wrote:

The only time this makes a difference is if the sizes of bus_addr_t and bus_size_t are different, and I don't have any test hardware where that's the case and I can test TPM. (Only hardware I have where that's the case is ppc Book-E, but that doesn't have a TPM).

Right. Not that I think this is risky, but would you like to do any testing of tpm at all before pushing to main? Otherwise I'll go ahead and push it without testing beyond make.

No need for me to test. There's no risk for any platform Juniper uses, or my RPi I tested the original change with. Thanks for taking this!

Closed by commit rGbea2bf458d2c: tpm_if.m: declare bus addresses as bus_size_t not bus_addr_t (authored by rlibby). · Explain WhyMay 22 2024, 2:56 PM

This revision was automatically updated to reflect the committed changes.

rlibby added a commit: rGbea2bf458d2c: tpm_if.m: declare bus addresses as bus_size_t not bus_addr_t.

Revision Contents
Changeset List

Path

Size

sys/

dev/

tpm/

tpm_bus.c

2 lines

tpm_if.m

12 lines

Diff 138848

View Options

sys/dev/tpm/tpm_bus.c

	Show First 20 Lines • Show All 450 Lines • ▼ Show 20 Lines
	* while the latter sets things up from a root context. Such as might			* while the latter sets things up from a root context. Such as might
	* be called from login(1).			* be called from login(1).
	*			*
	*/			*/

	int			int
	setusercontext(login_cap_t lc, const struct passwd pwd, uid_t uid, unsigned int flags)			setusercontext(login_cap_t lc, const struct passwd pwd, uid_t uid, unsigned int flags)
	{			{
	rlim_t p;
	login_cap_t *llc = NULL;			login_cap_t *llc = NULL;
	struct rtprio rtp;
	int error;			int error;

	if (lc == NULL) {			if (lc == NULL) {
	if (pwd != NULL && (lc = login_getpwclass(pwd)) != NULL)			if (pwd != NULL && (lc = login_getpwclass(pwd)) != NULL)
	llc = lc; /* free this when we're done */			llc = lc; /* free this when we're done */
	}			}

	if (flags & LOGIN_SETPATH)			if (flags & LOGIN_SETPATH)
	pathvars[0].def = uid ? _PATH_DEFPATH : _PATH_STDPATH;			pathvars[0].def = uid ? _PATH_DEFPATH : _PATH_STDPATH;

	/* we need a passwd entry to set these */			/* we need a passwd entry to set these */
	if (pwd == NULL)			if (pwd == NULL)
	flags &= ~(LOGIN_SETGROUP \| LOGIN_SETLOGIN \| LOGIN_SETMAC);			flags &= ~(LOGIN_SETGROUP \| LOGIN_SETLOGIN \| LOGIN_SETMAC);

	/* Set the process priority */			/* Set the process priority */
	if (flags & LOGIN_SETPRIORITY) {			if (flags & LOGIN_SETPRIORITY) {
	p = login_getcapnum(lc, "priority", LOGIN_DEFPRI, LOGIN_DEFPRI);			/*
				* Make it unlikely that someone would input our default sentinel
				* (which indicates no specification).
				*/
				rlim_t const def_val = INT64_MIN + 1, err_val = INT64_MIN;
				rlim_t p = login_getcapnum(lc, "priority", def_val, err_val);
				int rc;

				if (p != def_val) {
				/* Invariant: 'lc' != NULL from here. */
				if (p == err_val) {
				syslog(LOG_WARNING,
				"%s%s%sLogin class '%s': "
				kibUnsubmitted Done Inline Actions Indents are wrong. kib: Indents are wrong.
				"Invalid priority specification: '%s'",
				pwd ? "Login '" : "",
				pwd ? pwd->pw_name : "",
				pwd ? "': " : "",
				lc->lc_class,
				login_getcapstr(lc, "priority", "", ""));
				} else {
	if (p > PRIO_MAX) {			if (p > PRIO_MAX) {
				struct rtprio rtp;
	rtp.type = RTP_PRIO_IDLE;			rtp.type = RTP_PRIO_IDLE;
	p += RTP_PRIO_MIN - (PRIO_MAX + 1);			p += RTP_PRIO_MIN - (PRIO_MAX + 1);
	rtp.prio = p > RTP_PRIO_MAX ? RTP_PRIO_MAX : p;			rtp.prio = p > RTP_PRIO_MAX ? RTP_PRIO_MAX : p;
	if (rtprio(RTP_SET, 0, &rtp))			rc = rtprio(RTP_SET, 0, &rtp);
	syslog(LOG_WARNING, "rtprio '%s' (%s): %m",
	pwd ? pwd->pw_name : "-",
	lc ? lc->lc_class : LOGIN_DEFCLASS);
	} else if (p < PRIO_MIN) {			} else if (p < PRIO_MIN) {
				struct rtprio rtp;
	rtp.type = RTP_PRIO_REALTIME;			rtp.type = RTP_PRIO_REALTIME;
	p += RTP_PRIO_MAX - (PRIO_MIN - 1);			p += RTP_PRIO_MAX - (PRIO_MIN - 1);
	rtp.prio = p < RTP_PRIO_MIN ? RTP_PRIO_MIN : p;			rtp.prio = p < RTP_PRIO_MIN ? RTP_PRIO_MIN : p;
	if (rtprio(RTP_SET, 0, &rtp))			rc = rtprio(RTP_SET, 0, &rtp);
	syslog(LOG_WARNING, "rtprio '%s' (%s): %m",			} else
	pwd ? pwd->pw_name : "-",			rc = setpriority(PRIO_PROCESS, 0, (int)p);
	lc ? lc->lc_class : LOGIN_DEFCLASS);
	} else {			if (rc != 0)
	if (setpriority(PRIO_PROCESS, 0, (int)p) != 0)			syslog(LOG_WARNING,
	syslog(LOG_WARNING, "setpriority '%s' (%s): %m",			"%s%s%sLogin class '%s': "
	pwd ? pwd->pw_name : "-",			"Setting priority failed: %m",
	lc ? lc->lc_class : LOGIN_DEFCLASS);			pwd ? "Login '" : "",
				pwd ? pwd->pw_name : "",
				pwd ? "': " : "",
				lc->lc_class);
				}
	}			}
	}			}
				emasteUnsubmitted Not Done Inline Actions as with comment in some previous review, I prefer braces around all if-else blocks if any have them emaste: as with comment in some previous review, I prefer braces around all if-else blocks if any have…
				impUnsubmitted Not Done Inline Actions style(9) tries to make this point, but is a hopeless. on clearity in its striving for brevity: Space after keywords (if, while, for, return, switch). Two styles of braces (‘{’ and ‘}’) are allowed for single line statements. Either they are used for all single statements, or they are used only where needed for clarity. Usage within a function should be consistent. Forever which can be read many ways. Certainly within a single if statement, I prefer what you're advocating too and that seemed to be the consensus of the discussions when I added the above text, but it's not 100% clear. imp: style(9) tries to make this point, but is a hopeless. on clearity in its striving for brevity…

	/* Setup the user's group permissions */			/* Setup the user's group permissions */
	if (flags & LOGIN_SETGROUP) {			if (flags & LOGIN_SETGROUP) {
	if (setgid(pwd->pw_gid) != 0) {			if (setgid(pwd->pw_gid) != 0) {
	syslog(LOG_ERR, "setgid(%lu): %m", (u_long)pwd->pw_gid);			syslog(LOG_ERR, "setgid(%lu): %m", (u_long)pwd->pw_gid);
	login_close(llc);			login_close(llc);
	return (-1);			return (-1);
	}			}
	▲ Show 20 Lines • Show All 71 Lines • Show Last 20 Lines