Page MenuHomeFreeBSD
Differential D48420

clock: Simplify subr_ticks and rename
ClosedPublic
Actions

Authored by markj on Jan 10 2025, 5:25 PM.
Tags
None
Referenced Files
Unknown Object (File)
Mon, Mar 10, 1:15 PM
Unknown Object (File)
Sat, Mar 1, 11:04 AM
Unknown Object (File)
Fri, Feb 21, 5:42 PM
Unknown Object (File)
Tue, Feb 18, 10:41 PM
Unknown Object (File)
Feb 14 2025, 10:52 PM
Unknown Object (File)
Feb 12 2025, 7:31 PM
Unknown Object (File)
Feb 10 2025, 8:54 PM
Unknown Object (File)
Feb 8 2025, 3:32 PM
View All 24 Files
Subscribers
emaste

Details

Reviewers
jrtc27
olce
kib
imp
emaste
Commits
rGa267ceaf39a6: clock: Simplify subr_ticks and rename
rGb2b974f7ef4c: clock: Simplify subr_ticks and rename
Summary
  • We can use builtin constants for the size of int and long.
  • The file should have a .S prefix since we want to run it through the preprocessor, though apparently this happens anyway with .s...

Reported by: jrtc27
Fixes: 6b82130e6c9a ("clock: Add a long ticks variable, ticksl")

Diff Detail

Repository
rG FreeBSD src repository
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

markj created this revision.Jan 10 2025, 5:25 PM
Herald added a subscriber: emaste. · View Herald TranscriptJan 10 2025, 5:25 PM
markj requested review of this revision.Jan 10 2025, 5:25 PM
Harbormaster completed remote builds in B61650: Diff 149077.Jan 10 2025, 5:25 PM
jrtc27 added inline comments.Jan 10 2025, 5:32 PM
Makefile.inc1
1773 ↗(On Diff #149077)

If the window of time is small enough do we need to bother?...

sys/kern/subr_ticks.S
22

You missed the change to this line, otherwise it's wrong for 32-bit little endian.

25

As mentioned in my later follow-up, this should be .bss

jrtc27 added inline comments.Jan 10 2025, 5:34 PM
sys/kern/subr_ticks.S
22

Uh, 32-bit *big* endian, even

markj updated this revision to Diff 149078.Jan 10 2025, 5:37 PM
markj marked 3 inline comments as done.

Remove the .depend cleanup hack, fix the TICKS_OFFSET definition, move to .bss.

Harbormaster completed remote builds in B61651: Diff 149078.Jan 10 2025, 5:37 PM
jrtc27 accepted this revision.Jan 10 2025, 5:41 PM

(but please mention BSS in the commit message)

This revision is now accepted and ready to land.Jan 10 2025, 5:41 PM
emaste accepted this revision.Jan 10 2025, 5:45 PM
kib accepted this revision.Jan 10 2025, 6:17 PM
Closed by commit rGb2b974f7ef4c: clock: Simplify subr_ticks and rename (authored by markj). · Explain WhyJan 11 2025, 12:01 AM
This revision was automatically updated to reflect the committed changes.
markj added a commit: rGb2b974f7ef4c: clock: Simplify subr_ticks and rename.
kib added inline comments.Jan 11 2025, 5:16 AM
sys/kern/subr_ticks.S
29

According to the gas document, arm .align n means something different from other arches .align n. This is why I suggested initially to use .p2align.

jrtc27 added inline comments.Jan 11 2025, 6:37 AM
sys/kern/subr_ticks.S
29

Oh yeah. Use .balign. (.p2align works but it's more readable to use the actual number of bytes rather than log2)

markj added inline comments.Jan 11 2025, 5:00 PM
Makefile.inc1
1773 ↗(On Diff #149077)

Probably not, I was just trying to suppress a one-time warning about a stale .depend.

sys/kern/subr_ticks.S
29

I avoided .p2align to keep the definitions a bit simpler, indeed. See D48430.

markj added a commit: rGa267ceaf39a6: clock: Simplify subr_ticks and rename.Feb 14 2025, 7:27 PM

Revision Contents
Changeset List

PathSize
sys/
conf/
2 lines
kern/
subr_ticks.S
subr_ticks.s
22 lines

Diff 149107

View Options

sys/conf/files

Show First 20 Lines • Show All 4,688 Lines • ▼ Show 20 Lines
* ixgbe_sbuf_fw_version * ixgbe_sbuf_fw_version
************************************************************************/ ************************************************************************/
static void static void
ixgbe_sbuf_fw_version(struct ixgbe_hw *hw, struct sbuf *buf) ixgbe_sbuf_fw_version(struct ixgbe_hw *hw, struct sbuf *buf)
{ {
struct ixgbe_nvm_version nvm_ver = {0}; struct ixgbe_nvm_version nvm_ver = {0};
uint16_t phyfw = 0; uint16_t phyfw = 0;
int status; int status;
const char *space = "";
impUnsubmitted
Not Done
Inline Actions

I'd add a const char *space = ""; here

imp: I'd add a const char *space = ""; here
ixgbe_get_oem_prod_version(hw, &nvm_ver); /* OEM's NVM version */ ixgbe_get_oem_prod_version(hw, &nvm_ver); /* OEM's NVM version */
ixgbe_get_orom_version(hw, &nvm_ver); /* Option ROM */ ixgbe_get_orom_version(hw, &nvm_ver); /* Option ROM */
ixgbe_get_etk_id(hw, &nvm_ver); /* eTrack identifies a build in Intel's SCM */ ixgbe_get_etk_id(hw, &nvm_ver); /* eTrack identifies a build in Intel's SCM */
status = ixgbe_get_phy_firmware_version(hw, &phyfw); status = ixgbe_get_phy_firmware_version(hw, &phyfw);
if (nvm_ver.oem_valid) if (nvm_ver.oem_valid) {
sbuf_printf(buf, "NVM OEM V%d.%d R%d ", nvm_ver.oem_major, sbuf_printf(buf, "NVM OEM V%d.%d R%d", nvm_ver.oem_major,
nvm_ver.oem_minor, nvm_ver.oem_release); nvm_ver.oem_minor, nvm_ver.oem_release);
space = " ";
}
impUnsubmitted
Done
Inline Actions

Then add space =" "; here with {}

imp: Then add space =" "; here with {}
if (nvm_ver.or_valid) if (nvm_ver.or_valid) {
sbuf_printf(buf, "Option ROM V%d-b%d-p%d ", nvm_ver.or_major, sbuf_printf(buf, "%sOption ROM V%d-b%d-p%d",
impUnsubmitted
Done
Inline Actions

and use space here instead of this express....

imp: and use space here instead of this express....
nvm_ver.or_build, nvm_ver.or_patch); space, nvm_ver.or_major, nvm_ver.or_build, nvm_ver.or_patch);
impUnsubmitted
Done
Inline Actions

with the same space = " "; here.

imp: with the same space = " "; here.
space = " ";
}
if (nvm_ver.etk_id != ((NVM_VER_INVALID << NVM_ETK_SHIFT) | NVM_VER_INVALID)) if (nvm_ver.etk_id != ((NVM_VER_INVALID << NVM_ETK_SHIFT) |
sbuf_printf(buf, "eTrack 0x%08x ", nvm_ver.etk_id); NVM_VER_INVALID)) {
impUnsubmitted
Done
Inline Actions

and replace this with space.

imp: and replace this with space.
sbuf_printf(buf, "%seTrack 0x%08x", space, nvm_ver.etk_id);
space = " ";
}
if (phyfw != 0 && status == IXGBE_SUCCESS) if (phyfw != 0 && status == IXGBE_SUCCESS)
sbuf_printf(buf, "PHY FW V%d ", phyfw); sbuf_printf(buf, "%sPHY FW V%d", space, phyfw);
impUnsubmitted
Done
Inline Actions

I think you missed one of the trailing white spaces here.. It would have been trimmed before.

imp: I think you missed one of the trailing white spaces here.. It would have been trimmed before.
sbuf_trim(buf);
} /* ixgbe_sbuf_fw_version */ } /* ixgbe_sbuf_fw_version */
/************************************************************************ /************************************************************************
* ixgbe_print_fw_version * ixgbe_print_fw_version
************************************************************************/ ************************************************************************/
static void static void
ixgbe_print_fw_version(if_ctx_t ctx) ixgbe_print_fw_version(if_ctx_t ctx)
{ {
▲ Show 20 Lines • Show All 51 Lines • Show Last 20 Lines
View Options

sys/kern/subr_ticks.S

Show First 20 Lines • Show All 4,688 Lines • ▼ Show 20 Lines
* ixgbe_sbuf_fw_version * ixgbe_sbuf_fw_version
************************************************************************/ ************************************************************************/
static void static void
ixgbe_sbuf_fw_version(struct ixgbe_hw *hw, struct sbuf *buf) ixgbe_sbuf_fw_version(struct ixgbe_hw *hw, struct sbuf *buf)
{ {
struct ixgbe_nvm_version nvm_ver = {0}; struct ixgbe_nvm_version nvm_ver = {0};
uint16_t phyfw = 0; uint16_t phyfw = 0;
int status; int status;
const char *space = "";
impUnsubmitted
Not Done
Inline Actions

I'd add a const char *space = ""; here

imp: I'd add a const char *space = ""; here
ixgbe_get_oem_prod_version(hw, &nvm_ver); /* OEM's NVM version */ ixgbe_get_oem_prod_version(hw, &nvm_ver); /* OEM's NVM version */
ixgbe_get_orom_version(hw, &nvm_ver); /* Option ROM */ ixgbe_get_orom_version(hw, &nvm_ver); /* Option ROM */
ixgbe_get_etk_id(hw, &nvm_ver); /* eTrack identifies a build in Intel's SCM */ ixgbe_get_etk_id(hw, &nvm_ver); /* eTrack identifies a build in Intel's SCM */
status = ixgbe_get_phy_firmware_version(hw, &phyfw); status = ixgbe_get_phy_firmware_version(hw, &phyfw);
if (nvm_ver.oem_valid) if (nvm_ver.oem_valid) {
sbuf_printf(buf, "NVM OEM V%d.%d R%d ", nvm_ver.oem_major, sbuf_printf(buf, "NVM OEM V%d.%d R%d", nvm_ver.oem_major,
nvm_ver.oem_minor, nvm_ver.oem_release); nvm_ver.oem_minor, nvm_ver.oem_release);
space = " ";
}
impUnsubmitted
Done
Inline Actions

Then add space =" "; here with {}

imp: Then add space =" "; here with {}
if (nvm_ver.or_valid) if (nvm_ver.or_valid) {
sbuf_printf(buf, "Option ROM V%d-b%d-p%d ", nvm_ver.or_major, sbuf_printf(buf, "%sOption ROM V%d-b%d-p%d",
impUnsubmitted
Done
Inline Actions

and use space here instead of this express....

imp: and use space here instead of this express....
nvm_ver.or_build, nvm_ver.or_patch); space, nvm_ver.or_major, nvm_ver.or_build, nvm_ver.or_patch);
impUnsubmitted
Done
Inline Actions

with the same space = " "; here.

imp: with the same space = " "; here.
space = " ";
}
if (nvm_ver.etk_id != ((NVM_VER_INVALID << NVM_ETK_SHIFT) | NVM_VER_INVALID)) if (nvm_ver.etk_id != ((NVM_VER_INVALID << NVM_ETK_SHIFT) |
sbuf_printf(buf, "eTrack 0x%08x ", nvm_ver.etk_id); NVM_VER_INVALID)) {
impUnsubmitted
Done
Inline Actions

and replace this with space.

imp: and replace this with space.
sbuf_printf(buf, "%seTrack 0x%08x", space, nvm_ver.etk_id);
space = " ";
}
if (phyfw != 0 && status == IXGBE_SUCCESS) if (phyfw != 0 && status == IXGBE_SUCCESS)
sbuf_printf(buf, "PHY FW V%d ", phyfw); sbuf_printf(buf, "%sPHY FW V%d", space, phyfw);
impUnsubmitted
Done
Inline Actions

I think you missed one of the trailing white spaces here.. It would have been trimmed before.

imp: I think you missed one of the trailing white spaces here.. It would have been trimmed before.
sbuf_trim(buf);
} /* ixgbe_sbuf_fw_version */ } /* ixgbe_sbuf_fw_version */
/************************************************************************ /************************************************************************
* ixgbe_print_fw_version * ixgbe_print_fw_version
************************************************************************/ ************************************************************************/
static void static void
ixgbe_print_fw_version(if_ctx_t ctx) ixgbe_print_fw_version(if_ctx_t ctx)
{ {
▲ Show 20 Lines • Show All 51 Lines • Show Last 20 Lines
View Options

sys/kern/subr_ticks.s

Show First 20 Lines • Show All 4,688 Lines • ▼ Show 20 Lines
* ixgbe_sbuf_fw_version * ixgbe_sbuf_fw_version
************************************************************************/ ************************************************************************/
static void static void
ixgbe_sbuf_fw_version(struct ixgbe_hw *hw, struct sbuf *buf) ixgbe_sbuf_fw_version(struct ixgbe_hw *hw, struct sbuf *buf)
{ {
struct ixgbe_nvm_version nvm_ver = {0}; struct ixgbe_nvm_version nvm_ver = {0};
uint16_t phyfw = 0; uint16_t phyfw = 0;
int status; int status;
const char *space = "";
impUnsubmitted
Not Done
Inline Actions

I'd add a const char *space = ""; here

imp: I'd add a const char *space = ""; here
ixgbe_get_oem_prod_version(hw, &nvm_ver); /* OEM's NVM version */ ixgbe_get_oem_prod_version(hw, &nvm_ver); /* OEM's NVM version */
ixgbe_get_orom_version(hw, &nvm_ver); /* Option ROM */ ixgbe_get_orom_version(hw, &nvm_ver); /* Option ROM */
ixgbe_get_etk_id(hw, &nvm_ver); /* eTrack identifies a build in Intel's SCM */ ixgbe_get_etk_id(hw, &nvm_ver); /* eTrack identifies a build in Intel's SCM */
status = ixgbe_get_phy_firmware_version(hw, &phyfw); status = ixgbe_get_phy_firmware_version(hw, &phyfw);
if (nvm_ver.oem_valid) if (nvm_ver.oem_valid) {
sbuf_printf(buf, "NVM OEM V%d.%d R%d ", nvm_ver.oem_major, sbuf_printf(buf, "NVM OEM V%d.%d R%d", nvm_ver.oem_major,
nvm_ver.oem_minor, nvm_ver.oem_release); nvm_ver.oem_minor, nvm_ver.oem_release);
space = " ";
}
impUnsubmitted
Done
Inline Actions

Then add space =" "; here with {}

imp: Then add space =" "; here with {}
if (nvm_ver.or_valid) if (nvm_ver.or_valid) {
sbuf_printf(buf, "Option ROM V%d-b%d-p%d ", nvm_ver.or_major, sbuf_printf(buf, "%sOption ROM V%d-b%d-p%d",
impUnsubmitted
Done
Inline Actions

and use space here instead of this express....

imp: and use space here instead of this express....
nvm_ver.or_build, nvm_ver.or_patch); space, nvm_ver.or_major, nvm_ver.or_build, nvm_ver.or_patch);
impUnsubmitted
Done
Inline Actions

with the same space = " "; here.

imp: with the same space = " "; here.
space = " ";
}
if (nvm_ver.etk_id != ((NVM_VER_INVALID << NVM_ETK_SHIFT) | NVM_VER_INVALID)) if (nvm_ver.etk_id != ((NVM_VER_INVALID << NVM_ETK_SHIFT) |
sbuf_printf(buf, "eTrack 0x%08x ", nvm_ver.etk_id); NVM_VER_INVALID)) {
impUnsubmitted
Done
Inline Actions

and replace this with space.

imp: and replace this with space.
sbuf_printf(buf, "%seTrack 0x%08x", space, nvm_ver.etk_id);
space = " ";
}
if (phyfw != 0 && status == IXGBE_SUCCESS) if (phyfw != 0 && status == IXGBE_SUCCESS)
sbuf_printf(buf, "PHY FW V%d ", phyfw); sbuf_printf(buf, "%sPHY FW V%d", space, phyfw);
impUnsubmitted
Done
Inline Actions

I think you missed one of the trailing white spaces here.. It would have been trimmed before.

imp: I think you missed one of the trailing white spaces here.. It would have been trimmed before.
sbuf_trim(buf);
} /* ixgbe_sbuf_fw_version */ } /* ixgbe_sbuf_fw_version */
/************************************************************************ /************************************************************************
* ixgbe_print_fw_version * ixgbe_print_fw_version
************************************************************************/ ************************************************************************/
static void static void
ixgbe_print_fw_version(if_ctx_t ctx) ixgbe_print_fw_version(if_ctx_t ctx)
{ {
▲ Show 20 Lines • Show All 51 Lines • Show Last 20 Lines