If some device is plugged back in after unplug before the probe periph
destroyed, it will just restart the probe process. But I've found that
PROBE_INQUIRY_CKSUM flag not cleared between the iterations may cause
AC_FOUND_DEVICE not reported on the second iteration, and because of
AC_LOST_DEVICE reported during the first iteration, the device end up
configured, but without any periphs attached.
We've found that enabled serial console and 102-disk JBOD cause enough
probe delays to easily trigger the issue for half of the disks. This
change fixes it reliably on my tests.
MFC after: 2 weeks
Sponsored by: iXsystems, Inc.
When using iSCSI PDU offload (cxgbei) on T6 adapters, a burst of
received PDUs can be reported via a single message to the driver.
Previously the driver passed these multi-PDU bursts up to the iSCSI
stack up as a single "large" PDU by rewriting the buffer offset, data
segment length, and DataSN fields in the iSCSI header. The DataSN
field in particular was rewritten so that each of the "large" PDUs
used consecutively increasing values. While this worked, the forged
DataSN values did not match the ExpDataSN value in the subsequent SCSI
Response PDU. The initiator does not currently verify this value, but
the forged DataSN values prevent adding a check.
To avoid this, allow a logical iSCSI PDU (struct icl_pdu) to describe
a burst of PDUs via a new 'ip_additional_pdus' field. Normally this
field is set to zero when 'struct icl_pdu' represents a single PDU.
If logical PDU represents a burst of on-the-wire PDUs, then 'ip_npdus'
contains the count of additional on-the-wire PDUs. The header of this
"large" PDU is still modified, but the DataSN field now contains the
DataSN value of the first on-the-wire PDU in the burst.
Reviewed by: mav
Sponsored by: Chelsio Communications
Differential Revision: https://reviews.freebsd.org/D31577
This turns debugging printf() into a KASSERT().
It's for ATA for now; SCSI will came later.
Reviewed By: imp
Sponsored by: NetApp, Inc.
Sponsored by: Klara, Inc.
Differential Revision: https://reviews.freebsd.org/D31380
Similar to TSO, iSCSI segmentation offload permits the upper layers to
submit a "large" virtual PDU which is split up into multiple segments
(PDUs) on the wire. Similar to how the TCP/IP headers are used as
templates for TSO, the BHS at the start of a large PDU is used as a
template to construct the specific BHS at the start of each PDU. In
particular, the DataSN is incremented for each subsequent PDU, and the
'F' flag is only set on the last PDU.
struct icl_conn has a new 'ic_hw_isomax' field which defaults to 0,
but can be set to the largest virtual PDU a backend supports. If this
value is non-zero, the iSCSI target and initiator use this size
instead of 'ic_max_send_data_segment_length' to determine the maximum
size for SCSI Data-In and SCSI Data-Out PDUs. Note that since PDUs
can be constructed from multiple buffers before being dispatched, the
target and initiator must wait for the PDU to be fully constructed
before determining the number of DataSN values were consumed (and thus
updating the per-transfer DataSN value used for the start of the next
PDU).
The target generates large PDUs for SCSI Data-In PDUs in
cfiscsi_datamove_in(). The initiator generates large PDUs for SCSI
Data-Out PDUs generated in response to an R2T.
Reviewed by: mav
Sponsored by: Chelsio Communications
Differential Revision: https://reviews.freebsd.org/D31222
and remove repetetive code that calculates vnode locking type for write.
Reviewed by: khng, markj
Sponsored by: The FreeBSD Foundation
MFC after: 1 week
Differential revision: https://reviews.freebsd.org/D31405
This makes the da(4) driver use UMA for its CCBs by default,
like ada(4) already does. Please let me know via email
if you notice any suspicious kernel messages,
Reviewed By: imp
Sponsored by: NetApp, Inc.
Sponsored by: Klara, Inc.
Differential Revision: https://reviews.freebsd.org/D31257
Note that, as pointed out by scottl@, this code should really look
a bit different, in that the stack allocations should be replaced
with dynamic allocation, and the periph creation should be moved
to a context where one can use M_WAITOK. See the review for more
details. For now let's go with a minimal fix until we're done with
UMA CCBs.
Reviewed By: mav, imp
Sponsored by: NetApp, Inc.
Sponsored by: Klara, Inc.
Differential Revision: https://reviews.freebsd.org/D30298
kern.cam.do_dynamic_iosched is really a bool, so change its type to
bool. While I'm here, also use the CTLFLAG_TUN flag instead of a
separate tunable line for it and kern.cam.iosched_alpha_bits.
MFC After: 1 week
Sponsored by: Netflix
In ata_dev_advinfo() and nvme_dev_advinfo(), if the physical path is
being stored and there is a malloc failure (malloc(9) is called with
M_NOWAIT), we could wind up in a situation where the device's
physpath_len is set to the length the user provided, but the physpath
itself is NULL.
If another context then comes in to fetch the physical path value, we
would wind up trying to memcpy a NULL pointer into the caller's buffer.
So, set the physpath_len to 0 when we free the physpath on entry into
the store case for the physical path. Reset the length to a non-zero
value only after we've successfully malloced a buffer to hold it.
This code mirrors scsi_xpt.c does already as well.
Signed-off-by: Young Xiao <92siuyang@gmail.com>
Reviewed by: imp
PR: 238014
If MMC_SIM_CAM_REQUEST() is successful the ccb could be running or being
completed as the method returns. Modifying the ccb status could override
whatever status was already set by a MMC driver.
I am not sure what was the purpose of setting the status to CAM_REQ_INVALID
in the success path. I assume that it was to catch a possibility that the
ccb could be completed without its status explicitly set. So, I am keeping
the code, it's just moved to before the MMC_SIM_CAM_REQUEST call.
Without this change I was getting random and phantom EIO errors on Rock64
running off an SD card (dwmmc driver) plus occasional panics like:
Memory modified after free 0xffffa00003985800(2040) val=6 @ 0xffffa00003985854
panic: Most recently used by CAM CCB
MFC after: 1 week
This makes the ada(4) driver use UMA for its CCBs. While it's
da(4) counterpart needs some more testing, this one seems to be
safe now.
Please let me know via email if you notice any suspicious kernel
messages,
Reviewed By: imp
Sponsored by: NetApp, Inc.
Sponsored by: Klara, Inc.
Differential Revision: https://reviews.freebsd.org/D30567
No functional changes. Do not MFC this, it changes kernel ABI.
Sponsored by: NetApp, Inc.
Sponsored by: Klara, Inc.
Differential Revision: https://reviews.freebsd.org/D30698
Before UMA CCBs, all CCBs were of the same size, and could
be trivially copied using bcopy(9). Now we have to preserve
alloc_flags, otherwise we might end up attempting to free
stack-allocated CCB to UMA; we also need to take CCB size
into account.
This fixes kernel panic which would occur when trying to access
a stopped (as in, SCSI START STOP, also "ctladm stop") SCSI device.
Reported By: Gary Jennejohn <gljennjohn@gmail.com>
Tested By: Gary Jennejohn <gljennjohn@gmail.com>
Reviewed By: imp
Sponsored by: NetApp, Inc.
Sponsored by: Klara, Inc.
Differential Revision: https://reviews.freebsd.org/D31054
Fix forgotten argument and type error. MMCCAM isn't enabled by default,
and I'd mistakenly thought it was, so these went undetected precommit.
Sponsored by: Netflix
Its use was removed in 227d67aa54 by mav when locking was revamped.
Reviewed by: scottl@, mav@
Sponsored by: Netflix
Differential Revision: https://reviews.freebsd.org/D30890
xpt_clone_path originally returned a cam_status, but it doesn't do I/O
and should return an errno instead. I added it last year and it's only
used in one place. It's not yet documented, so no doc changes are
nneeded.
Reviewed by: scottl@, mav@
Sponsored by: Netflix
Differential Revision: https://reviews.freebsd.org/D30884
These were in the original CAM commit in 3.0, but were not used there,
nor have they been used since then. They also duplicate the now-standard
bool type. Remove them.
Reviewed by: scottl@
Sponsored by: Netflix
Differential Revision: https://reviews.freebsd.org/D30879
xpt_bus_register and xpt_bus_deregister returns a hybrid error that's
neither a cam_status, nor an errno, but a mix of both. Update
xpt_bus_register and xpt_bus_deregister to return an errno. The vast
majority of current users compare against zero, which can also be
spelled CAM_SUCCESS. Nobody uses CAM_FAILURE, so remove that symbol
to prevent comfusion (nothing returns it either).
Where the return value is saved, ensure that the variable 'error' is
used to store an errno and 'status' is used to store a cam_status where
it makes the code clearer (usually just in functions that already mix
and match). Where the return value isn't used at all, avoid storing it
at all.
Reviewed by: scottl@, mav@ (earlier version)
Sponsored by: Netflix
Differential Revision: https://reviews.freebsd.org/D30860
cam_sim_alloc_dev was only used internally by the MMC system. That has
been convered to using xpt_path_device() and has stopped using this
interface, so this can be retired.
Reviewed by: scottl@, mav@
Sponsored by: Netflix
Differential Revision: https://reviews.freebsd.org/D30858
Use the vanilla flavor of cam_sim_alloc. Now that sdiob has been
converted to get the device_t from the cam_path, this data is no longer
necessary.
Reviewed by: scottl@
Sponsored by: Netflix
Differential Revision: https://reviews.freebsd.org/D30856
Return the device associated with the sim's bus when it called
xpt_bus_register, if any. Most real SIMs in the tree set this device,
but some virtual ones do not have a device_t assocaited with them.
Reviewed by: scottl@, mav@ (earlier version)
Sponsored by: Netflix
Differential Revision: https://reviews.freebsd.org/D30853
The trick of subtracting one from the poitner returned from malloc
results in undefined behavior:
>>C89: 3.3.6 Unless both the pointer operand and the result point to a
>>member of the same array object, or one past the last member of the
>>array object, the behavior is undefined.
Instead, allocate 1 extra element and stop adjusting the pointer. While
a little wasteful, the extra is in the noise on today's systems.
Reviewed by: scottl@
Sponsored by: Netflix
Differential Revision: https://reviews.freebsd.org/D30847
When the elapsed time of the operation is complete and stored in the QOS
field, set the CAM_QOS_VALID bit. In iosched, test to make sure it's
set before using it.
Sponsored by: Netflix
Since periph_runccb now handles all the polling stuff, and
xpt_polled_action is now unused and can be removed.
Sponsored by: Netflix
Reviewed by: mav@
Differential Revision: https://reviews.freebsd.org/D30394
Nothing is using the sim callout to unfreeze the queue. Remove it to
simplify the SIM. This was introduced in the original CAM commit in 1998
but setting the CAM_SIM_REL_TIMEOUT_PENDING flag was removed in 1999 in
commit 87cfaf0e1fbd which reworked how bus reset worked. That work was
merged just after 3.2R was released. Remove the unused residuals.
Sponsored by: Netflix
Reviewed by: scottl@, mav@
Differential Revision: https://reviews.freebsd.org/D30383
For the discovery phase of SD/eMMC we need to do some transaction in a async
way.
The classic CAM XPT_{GET,SET}_TRAN_SETTING cannot be used in a async way.
This also allow us to split the discovery phase into a more complete state
machine and we don't mtx_sleep with a random number to wait for completion
of the tasks.
For mmc_sim we now do the SET_TRAN_SETTING in a taskqueue so we can call
the needed function for regulators/clocks without the cam lock(s). This part is
still needed to be done for sdhci.
We also now save the host OCR in the discovery phase as it wasn't done before and
only worked because the same ccb was reused.
Reviewed by: imp, kibab, bz
Differential Revision: https://reviews.freebsd.org/D30038
This fixes a few bugs in iSCSI backends where the backends were using
the limits they advertised initially during the login phase as the
final values instead of the values negotiated with the other end.
Reported by: Jithesh Arakkan @ Chelsio
Reviewed by: mav
Differential Revision: https://reviews.freebsd.org/D30271
cxgbei stores state about a target transfer in the ctl_private[] array
of a ctl_io that is freed when a target transfer (represented by the
cdw) is freed. As such, freeing a ctl_io before a cdw that references
it can result in a use after free in cxgbei. Two of the four places
freed the cdw first, and the other two freed the ctl_io first. Fix
the latter two places to free the cdw first.
Reported by: Jithesh Arakkan @ Chelsio
Reviewed by: mav
Differential Revision: https://reviews.freebsd.org/D30270
It looks like I've missed a couple of places where we don't clear
stack-allocated CCBs. Don't panic when that happens, just print
a warning.
This is a temporary measure until I get those cases fixed.
Reviewed By: markj
Sponsored by: NetApp, Inc.
Sponsored by: Klara, Inc.
Differential Revision: https://reviews.freebsd.org/D30296
Previously, daregister() could have been called before dainit()
initialized the UMA zone. This would trip a KASSERT.
Reported By: pho
Tested By: pho
Sponsored by: NetApp, Inc.
Sponsored by: Klara, Inc.
This patch makes it possible for CAM to use small CCBs allocated
from an periph-specific UMA zone instead of the usual, huge ones.
The end result is that CCBs issued via da(4) take 544B (size of
ccb_scsiio) instead of the usual 2kB (size of 'union ccb', ~1.5kB,
rounded up by malloc(9)). For ATA it's 272B. We waste less
memory, we avoid zeroing the unused 1kB, and it should be easier
to allocate those CCBs in low memory conditions. It should also
be possible to use uma_zone_reserve(9) to improve behaviour
in low memory conditions even further.
Note that this does not change the size, or the layout, of CCBs
as such. CCBs get allocated in various different ways, in particular
on the stack, and I don't want to redo all that. Instead, this
provides an opt-in mechanism for the periph to declare "my start()
callback is fine with receiving a CCB allocated from this UMA zone".
In other words, most of the code works exactly as it used to; the
change only happens to IOs issued by xpt_run_allockq(), which
is - conveniently - pretty much all that matters for performance.
The reason for doing it this way is that it's pretty small, localized
change, and can be implemented gradually and iteratively: take a
periph, make sure its start() callback only casts the CCBs it takes
to a particular type of CCB, for example ccb_scsiio, and that it only
casts CCBs returned by cam_periph_getccb() to that type, then add UMA
zone for that size, and declare it safe to XPT.
This is disabled by default. Set 'kern.cam.ada.enable_uma_ccbs=1'
and 'kern.cam.da.enable_uma_ccbs=1' tunables to enable it. Testing
is welcome; I will flip the default to enable in two weeks from now.
Reviewed By: imp
Sponsored by: NetApp, Inc.
Sponsored by: Klara, Inc.
Differential Revision: https://reviews.freebsd.org/D28674
Add description for what each of the parameters are to the cam_sim_alloc
call. Add some additional context for the mtx and queue parameters to
explain what special values passed in mean.
MFC After: 3 days
Reviewed by: mav@
Sponsored by: Netflix
Differential Revision: https://reviews.freebsd.org/D30115
