Page MenuHomeFreeBSD

Various cleanups: remove unused / under-used stuff.
ClosedPublic

Authored by imp on Mar 9 2020, 9:27 PM.
Tags
None
Referenced Files
Unknown Object (File)
Fri, Mar 22, 10:21 PM
Unknown Object (File)
Fri, Mar 22, 10:21 PM
Unknown Object (File)
Fri, Mar 22, 10:21 PM
Unknown Object (File)
Fri, Mar 8, 7:10 AM
Unknown Object (File)
Feb 11 2024, 9:07 PM
Unknown Object (File)
Jan 6 2024, 3:35 AM
Unknown Object (File)
Jan 6 2024, 3:35 AM
Unknown Object (File)
Jan 6 2024, 3:35 AM
Subscribers
None

Details

Summary

Remove unused cam ccb flags

Eliminate xpt_copy_path.

It's used in exactly one place. In that place it's used so we can hold the lock
on the device associated with the path (since we do a xpt_path_lock and unlock
pair around the callback). Instead, inline taking and dropping the reference to
the device so we can ensure we can unlock the mutex after the callback finishes
if the path in the ccb that's queued to be processed by xpt_scanner_thread is
destroyed while being processed. We don't actually need the path itself for
anything other than dereferencing it to get the device to do the lock and
unlock.

This also makes the locking / use model for cam_path a little cleaner by
eliminating a case where we needlessly copy the object.

Eliminate camq_alloc() and camq_free()

These are no longer needed now that it's embedded in cam_ccbq. They are also
unused.

Test Plan

This is the first round of cleanups I've done. I have a second round to do refcounts better, but that's not ready yet.
All but the xpt_copy_path ones should be non-controversial. Even that one is a simple optimization.

Diff Detail

Repository
rS FreeBSD src repository - subversion
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

imp added reviewers: ken, scottl, mav.

Looks OK to me

sys/cam/cam_xpt.c
826 ↗(On Diff #69336)

Out of curiosity, why did the previous code also acquire the bus and target? Or is this an optimization because the path bus and target must be NULL here?

sys/cam/cam_xpt.c
826 ↗(On Diff #69336)

Never mind, I see now.

This revision is now accepted and ready to land.Mar 10 2020, 3:52 PM
sys/cam/cam_ccb.h
66 ↗(On Diff #69336)

This flag is only checked by the aic7xxx/aic79xx drivers, and never set. Good candidate for elimination.

71 ↗(On Diff #69336)

This flag is only ever checked, not set. It looks like it's part of some ancient cut-and-paste in the SIMs that do check it. We could probably eliminate this flag and fix up the SIMs.

94 ↗(On Diff #69336)

These have been used by out-of-tree drivers in the distant past in conjunction with the passthru driver. I don't know if they're still in use now, but they have been useful.

sys/cam/cam_ccb.h
66 ↗(On Diff #69336)

There's others I was going to do in a second pass. Thanks for confirming that is a good idea :)

71 ↗(On Diff #69336)

I'll take a look. this is one that looked possibly remove, but had enough use I was unsure.

94 ↗(On Diff #69336)

Nothing in tree refers to them. How would I find out if out of tree drivers used them?

sys/cam/cam_xpt.c
826 ↗(On Diff #69336)

I believe that we don't need to take references to those items here.
The reference to the device keeps it from going away
Which also keeps its reference to the bus/target, so those won't go away either.
So this is safe to do since all we care about is the mtx, though this code now 'knows' where that mutex lives even though it uses the xpt_path_mtx accessor to find it.

sys/cam/cam_ccb.h
94 ↗(On Diff #69336)

Follow up: should I keep them in this pass?

sys/cam/cam_ccb.h
94 ↗(On Diff #69336)

go ahead and garbage collect them. If they need to come back, they can come back.

I think this is ok. Do we need to do anything for compatibility?

sys/cam/cam_ccb.h
66 ↗(On Diff #69336)

Was this a precursor to XPT_SET_TRAN_SETTINGS?

71 ↗(On Diff #69336)

I think the disable autosense flag is useful if you want to test out REQUEST SENSE handling, especially when testing a target implementation.

You send a Test Unit Ready, with autosense disabled. Then send a separate request sense, e.g. with the descriptor sense bit set. Otherwise the driver (at least for protocols that don't include sense with the status information, like parallel SCSI) will send the request sense for you. And you might want to be able to do it yourself.

94 ↗(On Diff #69336)

I think you can take them out. Most of the physical address handling came in at Pluto. I doubt anyone is using physical message and sense buffers.

sys/cam/cam_ccb.h
71 ↗(On Diff #69336)

Nothing sets these today...

sys/cam/cam_ccb.h
71 ↗(On Diff #69336)

I think Ken meant that you could create a CCB in userspace and send it via the pass driver. This is entirely DIY territory though. That said, no modern SAS drivers respect this flag, and I'd have to go look at the LSI spec to see if it's even possible to disable autosense. Oddly enough, it's mostly SATA drivers that respect this flag despite it not being a valid concept in SATA.

94 ↗(On Diff #69336)

Yeah, it's been almost 15 years since I wrote a SIM that used these, and that platform was nearing end-of-life even back then.

This revision was automatically updated to reflect the committed changes.
sys/cam/cam_ccb.h
71 ↗(On Diff #69336)

Gotcha. That makes sense.

I'll GC it from SATA drivers since it makes no sense there, pun intended.