Page MenuHomeFreeBSD

Define xpt_path_inq.
ClosedPublic

Authored by imp on Dec 6 2017, 1:23 AM.

Details

Summary

This provides a nice wrarpper around the XPT_PATH_INQ ccb creation and
calling.

Sponsored by: Netflix

Diff Detail

Repository
rS FreeBSD src repository
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

imp created this revision.Dec 6 2017, 1:23 AM
imp added reviewers: cem, scottl, ken.Dec 6 2017, 1:24 AM
cem added inline comments.Dec 6 2017, 1:42 AM
sys/cam/ata/ata_xpt.c
2101 ↗(On Diff #36276)

was NORMAL

sys/cam/cam_periph.c
746 ↗(On Diff #36276)

was NORMAL

imp updated this revision to Diff 36281.Dec 6 2017, 1:50 AM

oops.

imp added a comment.Dec 6 2017, 1:50 AM

Fix the ooopses cem@ noticed.

sys/cam/ata/ata_xpt.c
2101 ↗(On Diff #36276)

Doh!

sys/cam/cam_periph.c
746 ↗(On Diff #36276)

Doh!

cem accepted this revision.Dec 6 2017, 1:52 AM
This revision is now accepted and ready to land.Dec 6 2017, 1:52 AM
asomers added a subscriber: asomers.Dec 6 2017, 3:13 AM
asomers added inline comments.
sys/cam/cam_xpt.h
41 ↗(On Diff #36281)

I think this #include should lie within the #ifdef _KERNEL block.

imp updated this revision to Diff 36301.Dec 6 2017, 4:34 PM

fix include location

This revision now requires review to proceed.Dec 6 2017, 4:34 PM
imp marked an inline comment as done.Dec 6 2017, 4:39 PM

Fix the ooopses cem@ noticed.

rpokala accepted this revision.Dec 6 2017, 5:12 PM
rpokala added a subscriber: rpokala.

Heh. I've had a half-done, semi-related change sitting in one of my sandboxes for months.

This change de-dupes code in the PERIPHs, which is good.

Months ago, I somehow noticed that most of the SIMs use the exact same values when populating ccb_pathinq: version_num (1), sim_vid ("FreeBSD"), dev_name (cam_sim_name(sim)), unit_number (cam_sim_unit(sim)), and bus_id (cam_sim_bus(sim)). Thus:

static __inline void
cam_init_pathinq(struct ccb_pathinq *cpi, const char* hba_vid,
		struct cam_sim *sim)
{

	cpi->version_num = 1;
	strlcpy(cpi->sim_vid, "FreeBSD", SIM_IDLEN);
	strlcpy(cpi->hba_vid, hba_vid, HBA_IDLEN);
	strlcpy(cpi->dev_name, cam_sim_name(sim), DEV_IDLEN);
	cpi->unit_number = cam_sim_unit(sim);
	cpi->bus_id = cam_sim_bus(sim);
}

And (from sys/dev/ahci/ahci.c):

--- sys/dev/ahci/ahci.c	(revision 326289)
+++ sys/dev/ahci/ahci.c	(working copy)
@@ -2688,7 +2688,7 @@
 	{
 		struct ccb_pathinq *cpi = &ccb->cpi;
 
-		cpi->version_num = 1; /* XXX??? */
+		cam_init_pathinq(cpi, "AHCI", sim);
 		cpi->hba_inquiry = PI_SDTR_ABLE;
 		if (ch->caps & AHCI_CAP_SNCQ)
 			cpi->hba_inquiry |= PI_TAG_ABLE;
@@ -2705,12 +2705,7 @@
 			cpi->max_target = 0;
 		cpi->max_lun = 0;
 		cpi->initiator_id = 0;
-		cpi->bus_id = cam_sim_bus(sim);
 		cpi->base_transfer_speed = 150000;
-		strlcpy(cpi->sim_vid, "FreeBSD", SIM_IDLEN);
-		strlcpy(cpi->hba_vid, "AHCI", HBA_IDLEN);
-		strlcpy(cpi->dev_name, cam_sim_name(sim), DEV_IDLEN);
-		cpi->unit_number = cam_sim_unit(sim);
 		cpi->transport = XPORT_SATA;
 		cpi->transport_version = XPORT_VERSION_UNSPECIFIED;
 		cpi->protocol = PROTO_ATA;

I forget why I abandoned it... probably got busy, and didn't have hardware to test against. Let me know, and I'll shoot you the patch.

In any case, this change looks good to me.

This revision is now accepted and ready to land.Dec 6 2017, 5:12 PM
imp updated this revision to Diff 36315.Dec 6 2017, 9:24 PM

After talking to scottl@, we don't need priority here at all. It's not
a queued command. So pick a priority to use in xpt_path_inq() and just
use that. All these subtle differences turn out to be uninteresting
and we should go for simplicity.

This revision now requires review to proceed.Dec 6 2017, 9:24 PM
cem accepted this revision.Dec 6 2017, 9:26 PM
This revision is now accepted and ready to land.Dec 6 2017, 9:26 PM
ken accepted this revision.Dec 6 2017, 9:43 PM

Looks good to me! Thanks for doing this!

scottl accepted this revision.Dec 6 2017, 9:44 PM
imp added a comment.Dec 6 2017, 10:19 PM
In D13387#279956, @ken wrote:
Looks good to me!  Thanks for doing this!

Excellent. There's about half a dozen of these that I wanted to do. Glad you liked the first one, since the rest look to be identical.

Closed by commit rS326645: Define xpt_path_inq. (authored by imp, committed by ). · Explain WhyDec 7 2017, 7:02 AM
This revision was automatically updated to reflect the committed changes.