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

aic7xxx: make target mode enable a device hint #1208

Closed
wants to merge 0 commits into from

Conversation

hpvb
Copy link
Contributor

@hpvb hpvb commented Apr 27, 2024

Previously it was only possible to enable target mode for these drivers by rebuilding the kernel with AHC_TMODE_ENABLE or AHD_TMODE_ENABLE and a bitmask of which units to statically enable for target mode.

There is no space-savings in the driver by not having AHC_TMODE_ENABLE set, so in addition to the compile time option lets also introduce some tunables:

hint.ahc.<unit>.tmode_enable=0/1
hint.ahd.<unit>.tmode_enable=0/1

For compatibility the old behavior is retained, but it can be overridden with tunables

@hpvb hpvb force-pushed the ahc-tmode-hint branch 4 times, most recently from 78371cd to fa4396e Compare April 27, 2024 16:54
@hpvb hpvb changed the title ahc(4): make target mode enable a device hint aic7xxx: make target mode enable a device hint Apr 27, 2024
@hpvb
Copy link
Contributor Author

hpvb commented Apr 27, 2024

Sorry for the many edits and force-pushes. I changed my mind several times. I'm pretty sure this is good now though!

Copy link
Member

@amotin amotin left a comment

Choose a reason for hiding this comment

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

Why did you keep AHC_TARGET_MODE in code, while removed from configs? How it is supposed to be set and what for? I would understand if you kept it as alternative method of enabling target to keep compatibility with existing configs, but this way I just don't understand.

@hpvb
Copy link
Contributor Author

hpvb commented Apr 27, 2024

This is how it has always been. AHC_TARGET_MODE gets unconditionally enabled in aic7xxx_osm.h here:

/* This driver supports target mode */
#define AHC_TARGET_MODE 1

If you want a smaller driver I guess you can set that to 0?

@hpvb
Copy link
Contributor Author

hpvb commented Apr 29, 2024

@amotin Do you still feel this should be done differently? To address your concern I'd have to rip out all of AHC_TARGET_MODE from the driver. This has been in there since 2002 in this form, not possible to be disabled by any config setting.

Alternatively I could introduce a way to switch it in config and allow the driver to be (re) built without target support entirely. But I feel that this should be a separate PR then.

EDIT: also note that what I'm removing is the AHC_TMODE_ENABLE config option. This is wholly separate from AHC_TARGET_MODE, the two flags don't interact.

@amotin
Copy link
Member

amotin commented Apr 29, 2024

Thanks for the clarification. I indeed mixed AHC_TARGET_MODE with AHC_TMODE_ENABLE. I do not insist in massive driver modifications. But it seems we could trivially keep AHC_TMODE_ENABLE for compatibility, using it as default value to override with tunables.

sys/dev/aic7xxx/aic79xx.c Outdated Show resolved Hide resolved
@hpvb
Copy link
Contributor Author

hpvb commented Apr 30, 2024

@amotin I think I addressed your concerns.

Copy link
Member

@amotin amotin left a comment

Choose a reason for hiding this comment

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

The direction looks good to me, but I would further cut the printfs. BTW, there is device_printf() to print something for specific device without crafting local.

@hpvb
Copy link
Contributor Author

hpvb commented May 1, 2024

The direction looks good to me, but I would further cut the printfs. BTW, there is device_printf() to print something for specific device without crafting local.

I have removed most of the debug prints now, I thought they were kinda useful but perhaps too chatty even for verbose.

I'm aware of device_printf() but the driver uses this throughout, I do kind of want to replace all of the existing handcrafted ones with device_printf() but for consistency it seemed better to just keep using the local convention until I got around to replacing all of them.

sys/dev/aic7xxx/aic79xx.c Outdated Show resolved Hide resolved
@hpvb
Copy link
Contributor Author

hpvb commented May 3, 2024

@amotin are you happy with this now? I'd love to see this merged as I have some other changes pending that depend on these changes to the man pages :)

@bsdimp
Copy link
Member

bsdimp commented May 3, 2024

I'll add this to today's batch.

@bsdimp bsdimp self-assigned this May 3, 2024
@bsdimp bsdimp added the ready label May 3, 2024
freebsd-git pushed a commit that referenced this pull request May 4, 2024
Previously it was only possible to enable target mode for these drivers
by rebuilding the kernel with AHC_TMODE_ENABLE or AHD_TMODE_ENABLE and a
bitmask of which units to statically enable for target mode.

There is no space-savings in the driver by not having AHC_TMODE_ENABLE
set, so in addition to the compile time option lets also introduce some
tunables:

hint.ahc.<unit>.tmode_enable=0/1
hint.ahd.<unit>.tmode_enable=0/1

For compatibility the old behavior is retained, but it can be overridden
with tunables

Signed-off-by: HP van Braam <hp@tmm.cx>
Reviewed by: imp, mav
Pull Request: #1208
@bsdimp bsdimp closed this May 4, 2024
@bsdimp bsdimp removed the ready label May 4, 2024
@bsdimp bsdimp added the merged label May 4, 2024
@bsdimp
Copy link
Member

bsdimp commented May 4, 2024

Dang, was hoping I'd pushed the right thing to have this show up as merged.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
3 participants