When the device isn't there, we don't have valid inq data. Pass NULL in
this case. All the routines that receive this test against NULL already.
Sponsored by: Netflix
Reviewed by: adrian
Differential Revision: https://reviews.freebsd.org/D54470
Start to provide robust tracing in cam now that clang has broken my
fbt-based dtrace scripts a couple of times.
Sponsored by: Netflix
Reviewed by: adrian
Differential Revision: https://reviews.freebsd.org/D54468
Don't split the error messages across lines. We used to do that ages
ago, but have relaxed style(9) to encourage the opposite so all error
messages can be grepped. This constantly slows me down when I'm helping
others find issues, so start here by splitting according to normal
style(9) rules with a relaxed line length of 90.
Sponsored by: Netflix
wlun probing was added after my initial work on this and was overlooked
in merging forward. Add the timeout here too, for the same reasons as
for REPORT LUNS. This doesn't change the default.
Fixes: 8ac7a3801c cam: Reduce overly long timeout values for initial device probing
Sponsored by: Netflix
Reviewed by: jaeyoon
Differential Revision: https://reviews.freebsd.org/D54184
Currently, we have very long timeouts for the initial probing
commands. However, these are not appropriate for modern (post 2010) SCSI
disks. Sandards since SPC3 state that these commands should not wait for
media access. Since we retry them several times during the initial bus
scan, these delays can delay the boot by minutes (5 minutes per errant
disk in our expereince). These delays don't help and only hurt, so
reduce the TESTUNITREADY, INQUIRY and MODESENSE commands (during the
initial probe). Provide sysctl/tuneables to change the time for these
and also the REPORTLUNS commands for people that might need to adjust
them for devices that violate this belief but none-the-less work with
longer timeouts.
kern.cam.tur_timeout (default was 60s, now 1s)
kern.cam.inquiry_timeout (default was 60s, now 1s)
kern.cam.reportluns_timeout (default is 60s)
kern.cam.modesense_timeout (default was 60s, now 1s)
This can be partially merged: the sysctls can, but the new defaults likely
shouldn't.
Sponsored by: Netflix
Differential Revision: https://reviews.freebsd.org/D52427
The reladr field wasn't being set, so pmi and reladr args were
nops. That's OK, because they are passed as 0 in the one place in the
tree we use this.
Sponsored by: Netflix
This patch adds a power_condition parameter to the
scsi_start_stop() function and sets the power condition via SSU.
Reviewed by: imp (mentor)
Sponsored by: Samsung Electronic
Differential Revision: https://reviews.freebsd.org/D53922
This patch adds an additional state to probe well-known logical units
before probing normal logical units.
Reviewed by: imp (mentor)
Sponsored by: Samsung Electronics
Differential Revision: https://reviews.freebsd.org/D53920
The upstream refactoring of ndaregister() to split out ndasetgeom()
accidentally used an uninitialed variable to decide whether or not
to set DISKFLAG_UNMAPPED_BIO. Fix this by moving that portion of
ndasetgeom() back up to ndaregister(). The check for PIM_UNMAPPED
is not really needed because nvme devices always have that set,
so it cannot change in the other path that ndasetgeom() is now called.
Reviewed by: imp
Fixes: dffd882d12
Sponsored by: Netflix
MFC After: 1 minute
Register for AC_GETDEV_CHANGED. When we receive a namespace
notification, we only create a new device if it was unconfigured. If it
was configured, generate this async event. Rely on the fact that we
reconstruct namespace to just get the data from the identify data and
call disk_resised.
Reviewed by: imp
Differential Revision: https://reviews.freebsd.org/D33032
Refactor setting of geometry for the disk to its own function. No
functional changes.
Reviewed by: imp
Differential Revision: https://reviews.freebsd.org/D33032
Ensure that we're in the right state / priority for each of the states
in the driver. These asserts assured that a prior patch that I committed
to fix a priority leak worked when a drive departed (and bounced back
too!). These have been running in our production since I committed the
change and haven't trigged.
Sponsored by: Netflix
Differential Revision: https://reviews.freebsd.org/D53259
The descriptions for these unmapped_io and rotating sysctls indicated
that they're deprecated and being removed for FreeBSD 15.0. That did
not happen, so update to FreeBSD 16 instead.
Sponsored by: The FreeBSD Foundation
This reverts commit ce89c8f47a.
There's other parts to this, and GENERIC doesn't include iosched so
I missed that. Back out while I gather them together.
Sponsored by: Netflix
Add enum sleep_type stype parameter in power_suspend/resume event
handlers, as with the introduction of s2idle there are more than one
type of suspend.
Reviewed by: bz
Approved by: bz
Sponsored by: The FreeBSD Foundation
All queued CCBs should be created with a real priority (one that's not
CAM_PRIORITY_NONE). Recently, I introduced a bug that revealed a latent
MMC bug where it would stop enumerating due to a bad priority. Add an
assert to catch that (the other bug in mmc_da that it found has been
fixed).
Sponsored by: Netflix
Queued CCBs usually are queued at CAM_PRIORITY_NORMAL, unless they are
doing error recovery, or device enumeration of some kind. They should
never be queued at CAM_PRIORITY_NONE, which should only be used for CCBs
that are immediate. For sdda_start_init_task(), we allocate a ccb,
initialize it then use it to talk to the SD/MMC card to query it,
negotiate the speed and lane sizes, etc. Most of these commands are
queued, so use the normal priority.
Sponsored by: Netflix
Turns out, we don't use the results of xpt_path_inq here at all. And it
also causes problems. Since it calls xpt_cam_inq to do this useless
XPT_PATH_INQ, it loses the original priority we had for the CCB. This
priority should be CAM_PRIORITY_XPT, but was oringially set to
CAM_PRIORITY_NORMAL. This worked to enumerate the device because no
normal priority CCBs were queued by anything doing the
enumeration. However, when I changed xpt_path_inq to use the more proper
PRIORITY_NONE, it exposed this bug because queued CCBs with
PRIORITY_NONE sometimes won't run. This caused the probe device to stop
after its first operation. Removing the xpt_path_inq means we no longer
step on the important fields we get from xpt_schedule, allowing probing
to work correctly.
Noticed by: bz@
Sponsored by: Netflix
The new xpt_gdev_type() function shouldn't have had a blank line. It was
copied from xpt_path_inq(). Remove it from both.
Noticed by: jhb
Sponsored by: Netflix
HGST disks that are sick are returning 44/0 for START UNIT (which we
ignore) and then 4/2 on READ CAPACITY. START UNIT should be enough for
READ CAPACITY to succeed or UNIT ATTENTION. However, we get NOT_READ +
4/2 back. I've seen this on several models of HGST drives. Invalidate
the peripheral when we detect this condition. This is likely the least
bad thing we can do: It removes access to daX, but leaves passY so logs
may be extracted (if awkwardly). Removing daX access removes the disk
device that causes problems to geom outlined below.
Although the timeout is 5s for READ_CAPACITY, we wait the full 30s for
READ_CAPACITY_16. This causes us to stall booting as we start to taste
as soon as we release the final hold... but the tasting means
g_wait_idle() takes now takes over 5 minutes to clear since we do this
for all the opens. Even using a timeout of 3s instead of 30s leads to
boot times of almost 5 minutes in these cases, so there are other,
downstream operations that are taking a while, so it's not just a matter
of adjusting the timeout. Failing the periph early solves the bulk of
this problem (the tasting related delays). What the HBA does is HBA
specific and some have firmwares that are also confused by this when
they enumerate or discover the drive, leading to long (but still shorter
than 5 minute) delays. This patch won't solve that aspect of startup
delays with sick disks.
Perhaps we should fail the periph when START UNIT fails with the same
codes we check in the read capacity path. I'm reluctant to do such a
global change since it's in cam_periph, and there seems no good way to
flag that we want this behavior. It's also a bit magical when it runs
(some drive report 44/0 always, and some just report it on START UNIT,
and these HGST drive fall into the latter category).
Sponsored by: Netflix
Differential Revision: https://reviews.freebsd.org/D51218
Add a convenience wrapper to XPT_GDEV_TYPE in the same way we have one
for XPT_PATH_INQ. The changes from PRIORITY_NORMAL to PRIORITY_NONE are
intentional because this isn't a queued CCB. Please note: we have
several places still that construct a XPT_GDEV_TYPE message by
overwriting a CCB that happens to be laying around. I've not used this
method, by and large, in those places since I didn't want to risk
upsetting allocation flags that might be present (since we use a specail
allocator for some CCB types in *_da.c).
Sponsored by: Netflix
Differential Revision: https://reviews.freebsd.org/D51216
Use less stack space by using the specific type of ccb to do the
callback.
Sponsored by: Netflix
Differential Revision: https://reviews.freebsd.org/D51215
We free the scsi bug scan info in 4 places. 1 was wrong until I fixed a
bug there in a recent commit. Introduce a helper function that will free
the cpi always (it must always be valid since we set it right after
allocation).
Sponsored by: Netflix
Reviewed by: jhb
Differential Revision: https://reviews.freebsd.org/D51170
If we can't allocate the new path when loopoing over the target range,
then we have to free the scan_info->cpi CCB, not the work_ccb. This was
accidentally correct for the first iteration (because work_ccb ==
scan_info->cpi), but incorrect after that since we'll be freeing the CCB
for XPT_SCAN_LUN for the prior LUN we kicked off. Reorder the free so we
free it before we free scan_info so the pointer is still valid.
I do not have a test case for this since it requires that we fail in the
second or later iteration of the loop due to low memory, and only
fuzzing would catch that.
Sponsored by: Netflix
Reviewed by: jhb
Differential Revision: https://reviews.freebsd.org/D51169
There's no reason to inline xpt_path_inq here. While there was a
mismatch between this use (CAM_PRIORITY_NONE) and the old xpt_path_inq
code (which used CAM_PRIORITY_NORMAL), xpt_path_inq has been corrected
to use _NONE since this is a non-queued command.
Sponsored by: Netflix
Reviewed by: jhb
Differential Revision: https://reviews.freebsd.org/D51168
XPT_PATH_INQ is not a queued request. Therefore, the priority
information is not relevant, and so should be marked with
CAM_PRIORITY_NONE.
Sponsored by: Netflix
Reviewed by: jhb
Differential Revision: https://reviews.freebsd.org/D51167
Fix a comment about bzero maybe unnecessary. It can sometimes be
redunant, but a common usage pattern puts the ccb_pathinq structure on
the stack since it's small and for that scenario, it's required. It's
reundant for the few places the ccb is allocated, and in those cases it
does no harm.
Sponsored by: Netflix
Reviewed by: jhb
Differential Revision: https://reviews.freebsd.org/D51166
Use xpt_path_inq() for all XPT_PATH_INQ requests. They all should be
CAM_PRIORITY_NONE since XPT_PATH_INQ is an unqueued command, so the
minor changes here from _NORMAL to _NONE don't matter. And the one place
we preseve the priority doesn't matter either: It's an allocated CCB,
true, but it only ever stores the CPI from XPT_PATH_INQ.
Sponsored by: Netflix
Reviewed by: jhb
Differential Revision: https://reviews.freebsd.org/D51121
The function is unused since 5aedb8b1d4.
For information, in MMC CAM both XPT_GET_TRAN_SETTINGS and XPT_PATH_INQ
obtain data using MMC_SIM_GET_TRAN_SETTINGS. So, "overlapping"
information like ccb_trans_settings_mmc::host_max_data and
ccb_pathinq::maxio is derived from the same source.
That's why sdda_get_max_data was redundant.
Reported by: bz
MFC after: 5 days
The garbage resulted from reading the value from a ccb which was
originally populated by XPT_PATH_INQ operation but then overwritten by
XPT_GET_TRAN_SETTINGS operation.
The problem could probably be fixed by re-ordering the
XPT_GET_TRAN_SETTINGS operation, but it seems like the operation was
redundant. Besides, the ccb is declared not as union ccb but as struct
ccb_pathinq, so using it for XPT_GET_TRAN_SETTINGS was questionable.
I opted for replacing a call to sdda_get_max_data (which uses
XPT_GET_TRAN_SETTINGS internally) with using maxio provided by the
XPT_PATH_INQ operation.
This also required fixing mmc_cam_sim_default_action as controllers
return maximum I/O size in sectors, but maxio value should be in bytes.
MFC after: 2 weeks
For me, this fixes a problem with using eMMC storage in MMCCAM
configuration with dwmmc driver (on Rock64).
The problem appeared after commit 07da3bb5d5 "mmc: support for SPI
bus type".
The problem was caused by the said commit changing the layout of struct
mmc_ios which is embedded into struct ccb_trans_settings_mmc with the
latter being embedded into struct ccb_trans_settings, a member of
union ccb.
The layout mattered for two reasons:
1. dwmmc sets cmd.error only in case of an error;
2. mmc_da's sdda_start_init uses the same ccb for different transaction
types without explicitly clearing the object between transactions.
As a result, cmd.error could start out with a non-zero value and dwmmc
would keep that value which would then be interpreted as a failure.
Such a failure happened in mmc_set_timing resulting in incorrect timing
settings and subsequent complete failure to communicate with the eMMC
module.
Reviewed by: pkelsey
MFC after: 2 weeks
scsi is the only transport to do tag_action, so is the only one that
needs that support in devstat. Make a note of that. nvme and ata do
support some ordering, but that's done in the [an]da driver for each of
these devices and not for passthru commands and not via these tags.
Sponsored by: Netflix
Take a pointer to the ccb header in a few places where it's not
ambiguous (eg we have multiple ccbs in the routine). The code is a
little shorter this way. In places we had mulitple ccbs, I refrained
from doing this since that's more complex to manage. This also means
that we're making a stronger guarantee to the compiler we're only
accessing this part of the ccb, which reduces a few warnings from gcc
with picky settings we normally disable for CAM).
This makes the driver slightly less SCSI specific where csio was used to
do this needlessly.
There's no functional change.
Sponsored by: Netflix
Warner Losh