Skip to content

drivers: eth: phy_mii: Don't block system workqueue #87114

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

Merged
merged 2 commits into from
Apr 3, 2025

Conversation

kevinior
Copy link
Contributor

Looping while waiting for auto-negotiation to complete can block the system workqueue for several seconds.

@kevinior
Copy link
Contributor Author

@kevinior kevinior force-pushed the phy_mii_autoneg_block branch from 5649be2 to 209a063 Compare March 14, 2025 12:40
if (rc == -EINPROGRESS) {
/* Check for autonegotiation completion */
k_work_reschedule(&data->autoneg_work,
K_MSEC(100));
Copy link
Collaborator

Choose a reason for hiding this comment

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

please make the 100 here and in line 191 and 341 a define, so we know they belong together.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've added MII_AUTONEG_POLL_INTERVAL_MS to make it clearer what the 100's are for.

@kevinior kevinior force-pushed the phy_mii_autoneg_block branch from 209a063 to 6df23de Compare March 18, 2025 10:08
maass-hamburg
maass-hamburg previously approved these changes Mar 18, 2025
};

/* Offset to align capabilities bits of 1000BASE-T Control and Status regs */
#define MII_1KSTSR_OFFSET 2

#define MII_INVALID_PHY_ID UINT32_MAX

/* How often to poll auto-negotiation status while waiting for it to complete */
#define MII_AUTONEG_POLL_INTERVAL_MS 100
Copy link
Member

Choose a reason for hiding this comment

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

kconfig?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This wasn't configurable before, so making it configurable now seems like a separate issue from the one I'm trying to fix here.

Copy link
Collaborator

Choose a reason for hiding this comment

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

If this file is refactored later, I would like to add, that the timeout could now be implemented using the timepoint_expired-function.
Not needed for this PR though.

Copy link
Member

Choose a reason for hiding this comment

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

This wasn't configurable before, so making it configurable now seems like a separate issue from the one I'm trying to fix here.

I thought you are adding this in this PR, I am suggesting it be added in a configurable way rather than hardcoded

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This wasn't configurable before, so making it configurable now seems like a separate issue from the one I'm trying to fix here.

I thought you are adding this in this PR, I am suggesting it be added in a configurable way rather than hardcoded

No, the polling interval existed before and was not configurable.

@kevinior
Copy link
Contributor Author

@pdgendt @lmajewski This PR is waiting on a review from one of you.

@kevinior kevinior force-pushed the phy_mii_autoneg_block branch 4 times, most recently from a5c893e to fd3af65 Compare March 31, 2025 08:36
@pdgendt
Copy link
Collaborator

pdgendt commented Mar 31, 2025

Please do not combine formatting with functional changes in a single commit.

@kevinior
Copy link
Contributor Author

Please do not combine formatting with functional changes in a single commit.

@pdgendt Sorry, I was doing too many things at once and trying to make the CI check complaints go away. I'll have to find the original commits...

@kevinior kevinior force-pushed the phy_mii_autoneg_block branch 2 times, most recently from 0fb74a1 to 7f27099 Compare March 31, 2025 09:22
@pdgendt
Copy link
Collaborator

pdgendt commented Mar 31, 2025

@pdgendt Sorry, I was doing too many things at once and trying to make the CI check complaints go away. I'll have to find the original commits...

FYI formatting is not enforced in CI

pdgendt
pdgendt previously approved these changes Apr 2, 2025
kevinior added 2 commits April 2, 2025 14:51
Looping while waiting for auto-negotiation to complete can block the
system workqueue for several seconds.

Signed-off-by: Kevin ORourke <[email protected]>
CI compliance checks demanded reformatting.

Signed-off-by: Kevin ORourke <[email protected]>
@kevinior kevinior force-pushed the phy_mii_autoneg_block branch from 7f27099 to 24c60b1 Compare April 2, 2025 12:51
@kevinior kevinior requested a review from pdgendt April 3, 2025 06:39
@kartben kartben merged commit dfe2848 into zephyrproject-rtos:main Apr 3, 2025
25 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants