The sleeping code in iscsi_target_tx_thread() is susceptible to the classic
missed wakeup race:
- TX thread finishes handle_immediate_queue() and handle_response_queue(),
thinks both queues are empty.
- Another thread adds a queue entry and does wake_up_process(), which does
nothing because the TX thread is still awake.
- TX thread does schedule_timeout() and sleeps forever.
In practice this can kill an iSCSI connection if for example an initiator
does single-threaded writes and the target misses the wakeup window when
queueing an R2T; in this case the connection will be stuck until the
initiator loses patience and does some task management operation (or kills
the connection entirely).
Fix this by converting to wait_event_interruptible(), which does not
suffer from this sort of race.
Signed-off-by: Roland Dreier <[email protected]>
Cc: Andy Grover <[email protected]>
Cc: Hannes Reinecke <[email protected]>
Cc: Christoph Hellwig <[email protected]>
Cc: [email protected]
Signed-off-by: Nicholas Bellinger <[email protected]>
The expression (max_sectors * block_size) might overflow a u32
(indeed, since iblock sets max_hw_sectors to UINT_MAX, it is
guaranteed to overflow and end up with a much-too-small result in many
common cases). Fix this by doing an equivalent calculation that
doesn't require multiplication.
While we're touching this code, avoid splitting a printk format across
two lines and use pr_info(...) instead of printk(KERN_INFO ...).
Signed-off-by: Roland Dreier <[email protected]>
Cc: [email protected]
Signed-off-by: Nicholas Bellinger <[email protected]>
If the call to core_dev_release_virtual_lun0() fails, then nothing
sets ret to anything other than 0, so even though everything is
torn down and freed, target_core_init_configfs() will seem to succeed
and the module will be loaded. Fix this by passing the return value
on up the chain.
Signed-off-by: Roland Dreier <[email protected]>
Cc: [email protected]
Signed-off-by: Nicholas Bellinger <[email protected]>
It is possible for the target code to change the loop_id or s_id of a
target session in reaction to an FC fabric change. However, the
session structures are stored in tables that are indexed by these two
keys, and if we just change the session structure but leave the
pointers to it in the old places in the table, havoc can ensue. For
example, a new session might come along that should go in the old slot
in the table and overwrite the old session pointer.
To handle this, add a new tgt_ops->update_sess() method that also
updates the "by loop_id" and "by s_id" lookup tables when a session
changes, so that the keys where a session pointer is stored in these
tables always matches the keys in the session structure itself.
(nab: Drop unnecessary double inversion with FCF_CONF_COMP_SUPPORTED
usage)
Signed-off-by: Roland Dreier <[email protected]>
Cc: Chad Dupuis <[email protected]>
Cc: Arun Easi <[email protected]>
Cc: Saurav Kashyap <[email protected]>
Cc: [email protected]
Signed-off-by: Nicholas Bellinger <[email protected]>
My draft of SPC-4 says the following about the SCSI name string in
inquiry VPD page 83h:
The SCSI NAME STRING field starts with either:
a) the four UTF-8 characters 'eui.' concatenated with 16, 24, or
32 hexadecimal digits (i.e., the UTF-8 characters 0 through 9
and A through F) for an EUI-64 based identifier (see
7.8.6.5). The first hexadecimal digit shall be the most
significant four bits of the first byte (i.e., most significant
byte) of the EUI-64 based identifier;
b) the four UTF-8 characters 'naa.' concatenated with 16 or 32
hexadecimal digits for an NAA identifier (see 7.8.6.6). The
first hexadecimal digit shall be the most significant four bits
of the first byte (i.e., most significant byte) of the NAA
identifier; or
c) the four UTF-8 characters 'iqn.' concatenated with an iSCSI
Name for an iSCSI-name based identifier (see iSCSI).
However, the .tpg_get_wwn method for tcm_qla2xxx formats the WWN so
the SCSI name string looks like "52:4a:93:7d:24:5f:b2:12,t,0x0001".
This patch corrects the code so that VPD 83h gives a SPC-compliant
SCSI name string like "naa.524a937d245fb212,t,0x0001" while leavig
other uses alone (so configfs will still work with ':' separated WWNs).
Signed-off-by: Roland Dreier <[email protected]>
Cc: Chad Dupuis <[email protected]>
Cc: Arun Easi <[email protected]>
Cc: Saurav Kashyap <[email protected]>
Signed-off-by: Nicholas Bellinger <[email protected]>
With the new netlink support in commit 862096a8bb ("IB/ipoib: Add more
rtnl_link_ops callbacks") we need ipoib_set_mode() to be available even
if connected mode isn't built. Move the function from ipoib_cm.c to
ipoib_main.c (and make a few CM-related macros available unconditonally).
This fixes the build error
drivers/built-in.o: In function 'ipoib_changelink':
ipoib_netlink.c:(.text+0x6a5fc9): undefined reference to 'ipoib_set_mode'
ipoib_netlink.c:(.text+0x6a5fe3): undefined reference to 'ipoib_set_mode'
when CONFIG_INFINIBAND_IPOIB_CM isn't set.
Reported-by: Randy Dunlap <[email protected]>
Reported-by: Michael Neuling <[email protected]>
Signed-off-by: Roland Dreier <[email protected]>
In the current driver, the SENSE_PORT firmware command is issued as a
"wrapped" command, but the command handling code doesn't have a
wrapper, so it will never do anything other than log an error message.
The latest ConnectX-3 2.11.500 firmware reports the SENSE_PORT
capability even in multi-function (SR-IOV) mode, so the driver will
try to issue the command.
At least until the driver has a proper wrapper for SENSE_PORT, make
sure we disable the command for multi-function devices.
Signed-off-by: Roland Dreier <[email protected]>
Instead of having a hard-coded "PCI device ID != 0x1003" (which
obviously breaks as newer devices with ID != 0x1003 become available),
instead let's set a flag in our PCI device table for the older devices
where we're supposed to force using SENSE_PORT. This also avoids
enabling SENSE_PORT for virtual functions by mistake.
Signed-off-by: Roland Dreier <[email protected]>
That way we can check flags later on, when we've finished with the
pci_device_id structure. Also convert the "is VF" flag to an enum:
"Never do in the preprocessor what can be done in C."
Signed-off-by: Roland Dreier <[email protected]>
On an SR-IOV master device, __mlx4_init_one() calls mlx4_init_hca()
before mlx4_multi_func_init(). However, for unlucky configurations,
mlx4_init_hca() might call mlx4_SENSE_PORT() (via mlx4_dev_cap()), and
that calls mlx4_cmd_imm() with MLX4_CMD_WRAPPED set.
However, on a multifunction device with MLX4_CMD_WRAPPED, __mlx4_cmd()
calls into mlx4_slave_cmd(), and that immediately tries to do
down(&priv->cmd.slave_sem);
but priv->cmd.slave_sem isn't initialized until mlx4_multi_func_init()
(which we haven't called yet). The next thing it tries to do is access
priv->mfunc.vhcr, but that hasn't been allocated yet.
Fix this by moving the initialization of slave_sem and vhcr up into
mlx4_cmd_init(). Also, since slave_sem is really just being used as a
mutex, convert it into a slave_cmd_mutex.
Signed-off-by: Roland Dreier <[email protected]>
The qla2xxx firmware actually expects the task management response
code in a CTIO IOCB with SCSI status mode 1 to be in little-endian
byte order, ie the response code should be the first byte in the
sense_data[] array. The old code erroneously byte-swapped the
response code, which puts it in the wrong place on the wire and leads
to initiators thinking every task management request succeeds (since
they see 0 in the byte where they look for the response code).
Signed-off-by: Roland Dreier <[email protected]>
Cc: Chad Dupuis <[email protected]>
Cc: Arun Easi <[email protected]>
Acked-by: Saurav Kashyap <[email protected]>
Cc: [email protected]
Signed-off-by: Nicholas Bellinger <[email protected]>
There's no need for iscsi_target_init_negotiation() to print
iSCSI Login negotiation failed.
on failure, since its only caller (__iscsi_target_login_thread())
prints exactly the same message if it gets an error return back.
Signed-off-by: Roland Dreier <[email protected]>
Signed-off-by: Nicholas Bellinger <[email protected]>
Every fabric driver has to supply a se_tfo->set_fabric_sense_len()
method, just so iSCSI can return an offset of 2. However, every fabric
driver is already allocating a sense buffer and passing it into the
target core, either via transport_init_se_cmd() or target_submit_cmd().
So instead of having iSCSI pass the start of its sense buffer into the
core and then later tell the core to skip the first 2 bytes, it seems
easier for iSCSI just to do the offset of 2 when it passes the sense
buffer into the core. Then we can drop the se_tfo->set_fabric_sense_len()
everywhere, and just add a couple of lines of code to iSCSI to set the
sense data length to the beginning of the buffer right before it sends
it over the network.
(nab: Remove .set_fabric_sense_len usage from tcm_qla2xxx_npiv_ops +
change transport_get_sense_buffer to follow v3.6-rc6 code w/o
->set_fabric_sense_len usage)
Signed-off-by: Roland Dreier <[email protected]>
Signed-off-by: Nicholas Bellinger <[email protected]>
There are no callers of se_tfo->get_fabric_sense_len(), so we should
stop having every fabric driver implement it.
Signed-off-by: Roland Dreier <[email protected]>
Signed-off-by: Nicholas Bellinger <[email protected]>
This was originally for helping fabrics to determine overflow/underflow
status, and has been superceeded by SCF_OVERFLOW_BIT + SCF_UNDERFLOW_BIT.
Signed-off-by: Roland Dreier <[email protected]>
Signed-off-by: Nicholas Bellinger <[email protected]>
- Use kcalloc() / vzalloc() instead of an extra bitmap_zero().
- Add __GFP_NOWARN to kcalloc() since we'll try vzalloc() if it fails.
Signed-off-by: Roland Dreier <[email protected]>
If CONFIG_VLAN_8021Q is not set, then vlan_dev_real_dev() just goes BUG(),
so we shouldn't call it unless we're actually dealing with a VLAN netdev.
Reported-by: Fengguang Wu <[email protected]>
Signed-off-by: Roland Dreier <[email protected]>
It looks like one check was accidentally duplicated, and the other 3
checks were left out. This was detected by scripts/coccinelle/tests/doubletest.cocci:
drivers/infiniband/hw/ocrdma/ocrdma_verbs.c:895:6-54: duplicated argument to && or ||
Reported-by: Fengguang Wu <[email protected]>
Signed-off-by: Roland Dreier <[email protected]>
We want it to be possible for target_submit_cmd() to return errors up
to its fabric module callers. For now just update the prototype to
return an int, and update all callers to handle non-zero return values
as an error.
This is immediately useful for tcm_qla2xxx to fix a long-standing active
I/O session shutdown race, but tcm_fc, usb-gadget, and sbp-target the
fabric maintainers need to check + ACK that handling a target_submit_cmd()
failure due to session shutdown does not introduce regressions
(nab: Respin against for-next after initial NACK + update docbook comment +
fix double se_cmd init in exception path for usb-gadget)
Cc: Chad Dupuis <[email protected]>
Cc: Arun Easi <[email protected]>
Cc: Chris Boot <[email protected]>
Cc: Stefan Richter <[email protected]>
Cc: Mark Rustad <[email protected]>
Cc: Sebastian Andrzej Siewior <[email protected]>
Cc: Felipe Balbi <[email protected]>
Cc: Andy Grover <[email protected]>
Signed-off-by: Roland Dreier <[email protected]>
Signed-off-by: Nicholas Bellinger <[email protected]>
Fail UNMAP commands that have more than our reported limit on unmap
descriptors.
Signed-off-by: Roland Dreier <[email protected]>
Cc: [email protected]
Signed-off-by: Nicholas Bellinger <[email protected]>
It's possible for an initiator to send us an UNMAP command with a
descriptor that is less than 8 bytes; in that case it's really bad for
us to set an unsigned int to that value, subtract 8 from it, and then
use that as a limit for our loop (since the value will wrap around to
a huge positive value).
Fix this by making size be signed and only looping if size >= 16 (ie
if we have at least a full descriptor available).
Also remove offset as an obfuscated name for the constant 8.
Signed-off-by: Roland Dreier <[email protected]>
Cc: [email protected]
Signed-off-by: Nicholas Bellinger <[email protected]>
The UNMAP DATA LENGTH and UNMAP BLOCK DESCRIPTOR DATA LENGTH fields
are in the unmap descriptor (the payload transferred to our data out
buffer), not in the CDB itself. Read them from the correct place in
target_emulated_unmap.
Signed-off-by: Roland Dreier <[email protected]>
Cc: [email protected]
Signed-off-by: Nicholas Bellinger <[email protected]>
When processing an UNMAP command, we need to make sure that the number
of blocks we're asked to UNMAP does not exceed our reported maximum
number of blocks per UNMAP, and that the range of blocks we're
unmapping doesn't go past the end of the device.
Signed-off-by: Roland Dreier <[email protected]>
Cc: [email protected]
Signed-off-by: Nicholas Bellinger <[email protected]>
Many SCSI commands are defined to return a CHECK CONDITION / ILLEGAL
REQUEST with ASC set to LOGICAL BLOCK ADDRESS OUT OF RANGE if the
initiator sends a command that accesses a too-big LBA. Add an enum
value and case entries so that target code can return this status.
Signed-off-by: Roland Dreier <[email protected]>
Cc: [email protected]
Signed-off-by: Nicholas Bellinger <[email protected]>
Since we set se_session.sess_tearing_down and stop new commands from
being added to se_session.sess_cmd_list before we wait for commands to
finish when freeing a session, there's no need for a separate
sess_wait_list -- if we let new commands be added to sess_cmd_list
after setting sess_tearing_down, that would be a bug that breaks the
logic of waiting in-flight commands.
Also rename target_splice_sess_cmd_list() to
target_sess_cmd_list_set_waiting(), since we are no longer splicing
onto a separate list.
Signed-off-by: Roland Dreier <[email protected]>
Signed-off-by: Nicholas Bellinger <[email protected]>
Now that target_submit_cmd() / target_get_sess_cmd() check
sess_tearing_down before adding commands to the list, we no longer
need the check in qlt_do_work(). In fact this check is racy anyway
(and that race is what inspired the change to add the check of
sess_tearing_down to the target core).
Cc: Chad Dupuis <[email protected]>
Cc: Arun Easi <[email protected]>
Signed-off-by: Roland Dreier <[email protected]>
Signed-off-by: Nicholas Bellinger <[email protected]>
Target core code assumes that target_splice_sess_cmd_list() has set
sess_tearing_down and moved the list of pending commands to
sess_wait_list, no more commands will be added to the session; if any
are added, nothing keeps the se_session from being freed while the
command is still in flight, which e.g. leads to use-after-free of
se_cmd->se_sess in target_release_cmd_kref().
To enforce this invariant, put a check of sess_tearing_down inside of
sess_cmd_lock in target_get_sess_cmd(); any checks before this are
racy and can lead to the use-after-free described above. For example,
the qla_target check in qlt_do_work() checks sess_tearing_down from
work thread context but then drops all locks before calling
target_submit_cmd() (as it must, since that is a sleeping function).
However, since no locks are held, anything can happen with respect to
the session it has looked up -- although it does correctly get
sess_kref within its lock, so the memory won't be freed while
target_submit_cmd() is actually running, nothing stops eg an ACL from
being dropped and calling ->shutdown_session() (which calls into
target_splice_sess_cmd_list()) before we get to target_get_sess_cmd().
Once this happens, the se_session memory can be freed as soon as
target_submit_cmd() returns and qlt_do_work() drops its reference,
even though we've just added a command to sess_cmd_list.
To prevent this use-after-free, check sess_tearing_down inside of
sess_cmd_lock right before target_get_sess_cmd() adds a command to
sess_cmd_list; this is synchronized with target_splice_sess_cmd_list()
so that every command is either waited for or not added to the queue.
(nab: Keep target_submit_cmd() returning void for now..)
Signed-off-by: Roland Dreier <[email protected]>
Signed-off-by: Nicholas Bellinger <[email protected]>
There are no in-tree users of target_get_sess_cmd() outside of
target_core_transport.c. Any new code should use the higher-level
target_submit_cmd() interface. So let's un-export target_get_sess_cmd()
and make it static to the one file where it's actually used.
(nab: Fix up minor fuzz to for-next)
Signed-off-by: Roland Dreier <[email protected]>
Signed-off-by: Nicholas Bellinger <[email protected]>
The only place that sets qla_tgt_sess.tearing_down calls
target_splice_sess_cmd_list() immediately afterwards, without dropping
the lock it holds. That function sets se_session.sess_tearing_down,
so we can get rid of the qla_target-specific flag, and in the one
place that looks at the qla_tgt_sess.tearing_down flag just test
se_session.sess_tearing_down instead.
Cc: Chad Dupuis <[email protected]>
Cc: Arun Easi <[email protected]>
Signed-off-by: Roland Dreier <[email protected]>
Signed-off-by: Nicholas Bellinger <[email protected]>
When NUMBER OF LOGICAL BLOCKS is 0, WRITE SAME is supposed to write
all the blocks from the specified LBA through the end of the device.
However, dev->transport->get_blocks(dev) (perhaps confusingly) returns
the last valid LBA rather than the number of blocks, so the correct
number of blocks to write starting with lba is
dev->transport->get_blocks(dev) - lba + 1
(nab: Backport roland's for-3.6 patch to for-3.5)
Signed-off-by: Roland Dreier <[email protected]>
Cc: Cc: <[email protected]>
Signed-off-by: Nicholas Bellinger <[email protected]>
- instead of (PTR_ERR(file) < 0) just use IS_ERR(file)
- return -EINVAL instead of EINVAL
- all other error returns in target_scsi3_emulate_pr_out() use
"goto out" -- get rid of the one remaining straight "return."
Signed-off-by: Roland Dreier <[email protected]>
Cc: <[email protected]>
Signed-off-by: Nicholas Bellinger <[email protected]>
sparse correctly warns that if mpa->private_data_size is __be16, then
doing += on it is wrong, even if we do += htons(<something>) -- on a
little endian system, carries will go the wrong way. Fix this up by
doing the addition in native byte order.
Acked-by: Steve Wise <[email protected]>
Signed-off-by: Roland Dreier <[email protected]>
We want to set attr->max_srq_sge to dev->attr.max_srq_sge, not to itself.
This was detected by Coverity (CID 709210).
Signed-off-by: Roland Dreier <[email protected]>
If we make a variable an unsigned int and then expect it to be < 0 on
a bad character, we're going to have a bad time. Fix the tcm_qla2xxx
code to actually notice if hex_to_bin() returns a negative variable.
This was detected by the compiler warning:
scsi/qla2xxx/tcm_qla2xxx.c: In function ‘tcm_qla2xxx_npiv_extract_wwn’:
scsi/qla2xxx/tcm_qla2xxx.c:148:3: warning: comparison of unsigned expression >= 0 is always true [-Wtype-limits]
Signed-off-by: Roland Dreier <[email protected]>
Signed-off-by: Nicholas Bellinger <[email protected]>
If we go to the "out_term:" exit path in qlt_do_work(), we call
qlt_send_term_exchange() with a NULL cmd, which means that it can't
possibly free the cmd for us. Add an explicit call to free the
command memory, so we don't leak the allocation.
This will also fix warnings about "BUG qla_tgt_cmd_cachep: Objects
remaining on kmem_cache_close" from slub when unloading the qla2xxx
target module.
Signed-off-by: Roland Dreier <[email protected]>
Signed-off-by: Nicholas Bellinger <[email protected]>
In qlt_do_ctio_completion(), there's no point in calling
qlt_term_ctio_exchange() with a NULL cmd -- all that it does is crash
in a NULL pointer dereference, since it does
qlt_send_term_exchange(vha, cmd, &cmd->atio, 1);
and dereferencing &cmd->atio is a bad idea if cmd itself is NULL.
If we really need to do this, we could take the values from the
failed CTIO we're processing, but it's not clear if it's worth
the replumbing to do that.
Signed-off-by: Roland Dreier <[email protected]>
Signed-off-by: Nicholas Bellinger <[email protected]>
When we create an explicit node ACL in tcm_qla2xxx_make_nodeacl(),
there is a call to tcm_qla2xxx_setup_nacl_from_rport(), which puts the
node ACL into the lport_fcport_map even though there is no session yet
for the initiator. Since the only time we remove entries from this
map is when we free a session, this means that if we later delete this
node ACL without the initiator ever creating a session, we'll leave
the nacl pointer in the btree pointing at freed memory.
This is especially bad if that initiator later does send us a command
that would cause us to create a dynamic ACL and session: we'll find
the stale freed nacl pointer in the btree and end up with use-after-free.
We could add more code to clear the btree entry when deleting the
explicit nacl, but the original insertion is pointless: without a
session attached, we'll just have to update the entry when a session
appears anyway. So we can just delete tcm_qla2xxx_setup_nacl_from_rport()
and the code that calls it.
Signed-off-by: Roland Dreier <[email protected]>
Cc: Chad Dupuis <[email protected]>
Cc: Giridhar Malavali <[email protected]>
Cc: Arun Easi <[email protected]>
Signed-off-by: Nicholas Bellinger <[email protected]>
The error paths in target_emulate_set_target_port_groups() are all
essentially "rc = -EINVAL; goto out;" but the code at "out:" ignores
rc and always returns success. This means that even if eg explicit
ALUA is turned off, the initiator will always see a good SCSI status
for SET TARGET PORT GROUPS.
Fix this by returning rc as is intended. It appears this bug was
added by the following patch:
commit 05d1c7c0d0
Author: Andy Grover <[email protected]>
Date: Wed Jul 20 19:13:28 2011 +0000
target: Make all control CDBs scatter-gather
Signed-off-by: Roland Dreier <[email protected]>
Cc: Andy Grover <[email protected]>
Cc: <[email protected]>
Signed-off-by: Nicholas Bellinger <[email protected]>
If I build with W=1, for every file that includes <net/route.h>, I get the warning
include/net/route.h: In function 'ip_route_output':
include/net/route.h:135:3: warning: initialized field overwritten [-Woverride-init]
include/net/route.h:135:3: warning: (near initialization for 'fl4') [-Woverride-init]
(This is with "gcc (Debian 4.6.3-1) 4.6.3")
A fix seems pretty trivial: move the initialization of .flowi4_tos
earlier. As far as I can tell, this has no effect on code generation.
Signed-off-by: Roland Dreier <[email protected]>
Signed-off-by: David S. Miller <[email protected]>
The memory the parameter __key points to is used as an iterator in
btree_get_prev(), so if we save off a bkey() pointer in retry_key and
then assign that to __key, we'll end up corrupting the btree internals
when we do eg
longcpy(__key, bkey(geo, node, i), geo->keylen);
to return the key value. What we should do instead is use longcpy() to
copy the key value that retry_key points to __key.
This can cause a btree to get corrupted by seemingly read-only
operations such as btree_for_each_safe.
[[email protected]: avoid the double longcpy()]
Signed-off-by: Roland Dreier <[email protected]>
Acked-by: Joern Engel <[email protected]>
Cc: <[email protected]>
Signed-off-by: Andrew Morton <[email protected]>
Signed-off-by: Linus Torvalds <[email protected]>
When IPV6 is not enabled:
ERROR: "register_inet6addr_notifier" [drivers/infiniband/hw/ocrdma/ocrdma.ko] undefined!
ERROR: "unregister_inet6addr_notifier" [drivers/infiniband/hw/ocrdma/ocrdma.ko] undefined!
Fix this by wrapping the inet6 calls in #ifdef IPV6. Also make the
ocrdma module depend on (IPV6 || IPV6=n) to forbid the case of modular
ipv6 but built-in ocrdma (which can't work, because ocrdma calls ipv6
functions).
Reported-by: Randy Dunlap <[email protected]>
Signed-off-by: Roland Dreier <[email protected]>
We need to set ib_evt.device, or else ib_dispatch_event() will crash
when we call it for unaffiliated events (and consumers may get
confused in their QP/CQ/SRQ event handler for affiliated events).
Also fix sparse warning:
drivers/infiniband/hw/ocrdma/ocrdma_hw.c:678:36: warning: Using plain integer as NULL pointer
There's no need to clear ib_evt, since every member is initialized.
Signed-off-by: Roland Dreier <[email protected]>
First, fix
drivers/infiniband/hw/ocrdma/ocrdma_verbs.c: In function 'ocrdma_alloc_pd':
drivers/infiniband/hw/ocrdma/ocrdma_verbs.c:371:17: warning: 'dpp_page_addr' may be used uninitialized in this function [-Wuninitialized]
drivers/infiniband/hw/ocrdma/ocrdma_verbs.c:337:6: note: 'dpp_page_addr' was declared here
which seems that it may border on a bug (the call to ocrdma_del_mmap()
might conceivably do bad things if pd->dpp_enabled is not set and
dpp_page_addr ends up with just the wrong value).
Also take care of:
drivers/infiniband/hw/ocrdma/ocrdma_hw.c: In function 'ocrdma_init_hw':
drivers/infiniband/hw/ocrdma/ocrdma_hw.c:2587:5: warning: 'status' may be used uninitialized in this function [-Wuninitialized]
drivers/infiniband/hw/ocrdma/ocrdma_hw.c:2549:17: note: 'status' was declared here
which is only real if num_eq == 0, which should be impossible.
Signed-off-by: Roland Dreier <[email protected]>
Add names for our lockdep classes, so instead of having to decipher
lockdep output with mysterious names:
Chain exists of:
key#14 --> key#11 --> key#13
lockdep will give us something nicer:
Chain exists of:
SRQ-uobj --> PD-uobj --> CQ-uobj
Signed-off-by: Roland Dreier <[email protected]>
The driver calls cxgb_vlan_mode() from init_one(). This calls into
synchronize_rx(), which locks all the q locks, but the q locks are not
initialized until cxgb_up() -> setup_sge_qsets(). So move the call to
cxgb_vlan_mode() into cxgb_up(), after the call to setup_sge_qsets().
We also move the body of these functions up higher to avoid having to
a forward declaration.
This was found because of the lockdep warning:
INFO: trying to register non-static key.
the code is fine but needs lockdep annotation.
turning off the locking correctness validator.
Pid: 323, comm: work_for_cpu Not tainted 3.4.0-rc5 #28
Call Trace:
[<ffffffff8106e767>] register_lock_class+0x108/0x2d0
[<ffffffff8106ff42>] __lock_acquire+0xd3/0xd06
[<ffffffff81070fd0>] lock_acquire+0xbf/0xfe
[<ffffffff813862a6>] _raw_spin_lock_irq+0x36/0x45
[<ffffffffa01e71aa>] cxgb_vlan_mode+0x96/0xcb [cxgb3]
[<ffffffffa01f90eb>] init_one+0x8c4/0x980 [cxgb3]
[<ffffffff811fcbf0>] local_pci_probe+0x3f/0x70
[<ffffffff81042206>] do_work_for_cpu+0x10/0x22
[<ffffffff810482de>] kthread+0xa1/0xa9
[<ffffffff8138e234>] kernel_thread_helper+0x4/0x10
Contrary to what lockdep says, the code is not fine: we are locking an
uninitialized spinlock.
Signed-off-by: Roland Dreier <[email protected]>
Signed-off-by: David S. Miller <[email protected]>
Now that all fabrics are converted over to using se_cmd->t_data_sg
directly, we can drop the task sg chaining support. With the modern
memory allocation in target core, task sg chaining is needless
overhead -- we would split up the main cmd sglist into pieces, and
then splice those pieces back together instead of just using the
original list directly.
Signed-off-by: Roland Dreier <[email protected]>
Reviewed-by: Christoph Hellwig <[email protected]>
Signed-off-by: Nicholas Bellinger <[email protected]>
With the modern target core, se_cmd->t_data_sg already points to a
sglist that covers the whole command. So task_sg chaining is needless
overhead and obfuscation -- instead of splicing the split up task
sglists back into one list, we can just use the original list directly.
Signed-off-by: Roland Dreier <[email protected]>
Reviewed-by: Christoph Hellwig <[email protected]>
Signed-off-by: Nicholas Bellinger <[email protected]>
With the modern target core, se_cmd->t_data_sg already points to a
sglist that covers the whole command. So task_sg chaining is needless
overhead and obfuscation -- instead of splicing the split up task
sglists back into one list, we can just use the original list directly.
Signed-off-by: Roland Dreier <[email protected]>
Reviewed-by: Christoph Hellwig <[email protected]>
Acked-by: Kiran Patil <[email protected]>
Signed-off-by: Nicholas Bellinger <[email protected]>
Since commit 96104eda01 ("RDMA/core: Add SRQ type field"), kernel
users of SRQs need to specify srq_type = IB_SRQT_BASIC in struct
ib_srq_init_attr, or else most low-level drivers will fail in
when srpt_add_one() calls ib_create_srq() and gets -ENOSYS.
(mlx4_ib works OK nearly all of the time, because it just needs
srq_type != IB_SRQT_XRC. And apparently nearly everyone using
ib_srpt is using mlx4 hardware)
Reported-by: Alexey Shvetsov <[email protected]>
Cc: <[email protected]>
Signed-off-by: Roland Dreier <[email protected]>
Commit e9319b0cb0 ("IB/core: Fix SDR rates in sysfs") changed our
sysfs rate attribute to return EINVAL to userspace if the underlying
device driver returns an invalid rate. Apparently some drivers do this
when the link is down and some userspace pukes if it gets an error when
reading this attribute, so avoid a regression by not return an error to
match the old code.
Signed-off-by: Roland Dreier <[email protected]>
In some circumstances (eg when running a build in an emacs shell
buffer), I get a spew of messages like
grep: writing output: Broken pipe
from setlocalversion, because the "read" subshell apparently exits as
soon as it reads one line and gives EPIPE to grep. It's not clear to
me why this way of writing the check was used instead of just using
grep -q to suppress output, but unless there is some deep reason I
don't know, this way looks cleaner to me anyway, and gets rid of the
ugly message spew.
(I double checked at http://pubs.opengroup.org/onlinepubs/009604499/utilities/grep.html
and "grep -q" is specified in POSIX / SuS, so hopefully even people
cross-compiling the kernel on some bizarre host OS can't complain
about this change)
Signed-off-by: Roland Dreier <[email protected]>
Signed-off-by: Michal Marek <[email protected]>
If we don't handle a given service action, we're supposed to return
INVALID FIELD IN CDB, since we do handle the SERVICE ACTION IN opcode.
Signed-off-by: Roland Dreier <[email protected]>
Signed-off-by: Nicholas Bellinger <[email protected]>
Remove the hack that has READ CAPACITY(10) return 0xFFFFFFFF as the
number of sectors when thin provisioning is enabled. This is supposed
to trigger the initiator to use READ CAPACITY(16) in this case so that
it finds out about thin provisioning. But an initiator that cares about
thin provisioning is going to ask anyway, and an initiator that doesn't
know about READ CAPACITY(16) is going to get the wrong capacity. So
just have READ CAPACITY(10) return the size it's supposed to.
Signed-off-by: Roland Dreier <[email protected]>
Signed-off-by: Nicholas Bellinger <[email protected]>
The current driver defaults to 1M MTT segments, where each segment holds
8 MTT entries. This limits the total memory registered to 8M * PAGE_SIZE
which is 32GB with 4K pages. Since systems that have much more memory
are pretty common now (at least among systems with InfiniBand hardware),
this limit ends up getting hit in practice quite a bit.
Handle this by having the driver allocate at least enough MTT entries to
cover 2 * totalram pages.
Signed-off-by: Roland Dreier <[email protected]>
Commit 22c8bff6fa ("mlx4_core: Exported functions can't be static")
fixed most of this up, but forgot about mlx4_is_slave_active(). Fix
this one too.
Cc: <[email protected]>
Signed-off-by: Roland Dreier <[email protected]>
Commit 71eeba16 ("IB: Add new InfiniBand link speeds") introduced a bug
where eg the rate for IB 4X SDR links iss displayed as "8.5 Gb/sec"
instead of "10 Gb/sec" as it used to be. Fix that.
Reported-by: Or Gerlitz <[email protected]>
Signed-off-by: Roland Dreier <[email protected]>
The old code did (MSB << 8) & 0xff, which always evaluates to 0. Just use
get_unaligned_be16() so we don't have to worry about whether our open-coded
version is correct or not.
Signed-off-by: Roland Dreier <[email protected]>
Cc: [email protected]
Signed-off-by: Nicholas Bellinger <[email protected]>
If the target core signals an over- or under-run, tcm_loop should call
scsi_set_resid() to tell the SCSI midlayer about the residual data length.
The difference can be seen by doing something like
strace -eioctl sg_raw -r 1024 /dev/sda 8 0 0 0 1 0 > /dev/null
and looking at the "resid=" part of the SG_IO ioctl -- after this patch,
the field is correctly reported as 512.
Signed-off-by: Roland Dreier <[email protected]>
Cc: [email protected]
Signed-off-by: Nicholas Bellinger <[email protected]>
se_dev_attrib.max_sectors currently has two independent meanings:
- It is reported in the block limits VPD page as the maximum transfer
length, ie the largest IO that the front-end (fabric) can handle.
Also the target core doesn't enforce this maximum transfer length.
- It is used to hold the size of the largest IO that the back-end can
handle, so we know when to split SCSI commands into multiple tasks.
Fix this by adding a new se_dev_attrib.fabric_max_sectors to hold the
maximum transfer length, and checking incoming IOs against that limit.
Signed-off-by: Roland Dreier <[email protected]>
Signed-off-by: Nicholas Bellinger <[email protected]>
SPC-4 says about the WBUS16 and SYNC bits:
The meanings of these fields are specific to SPI-5 (see 6.4.3).
For SCSI transport protocols other than the SCSI Parallel
Interface, these fields are reserved.
We don't have a SPI fabric module, so we should never set these bits.
(The comment was misleading, since it only mentioned Sync but the
actual code set WBUS16 too).
Signed-off-by: Roland Dreier <[email protected]>
Cc: [email protected]
Signed-off-by: Nicholas Bellinger <[email protected]>
Current code sets the peripheral device type to 0x3f == "not present
unknown" for virtual LUN 0 for standard INQUIRY commands, but leaves it
as 0 == "connected direct access block" for VPD INQUIRY commands. This
is just because the check for LUN 0 only happens in some code paths.
Make our peripheral device type consistent by moving the LUN 0 check
into the common emulate_inquiry() code.
Signed-off-by: Roland Dreier <[email protected]>
Cc: [email protected]
Signed-off-by: Nicholas Bellinger <[email protected]>
If the initiator sends us an INQUIRY command with an allocation length
that's shorter than what we want to return, we're simply supposed to
truncate our response and return what the initiator gave us space for,
without signaling any error. Current target code has various tests that
don't fill out the full response if the buffer is too short and
sometimes return errors incorrectly.
Fix this up by allocating a bounce buffer for INQUIRY responses if we
need to, ie if we have cmd->data_length too small as well as
SCF_PASSTHROUGH_SG_TO_MEM_NOALLOC set in cmd->se_cmd_flags -- for most
fabrics, we always allocate at least a full page, but for tcm_loop we
may have a small buffer coming directly from the SCSI stack.
This lets us delete a lot of cmd->data_length checking, and also makes
our INQUIRY handling correct per SPC in a lot more cases.
Signed-off-by: Roland Dreier <[email protected]>
Cc: [email protected]
Signed-off-by: Nicholas Bellinger <[email protected]>
Instead of
static struct list_head foo;
static struct mutex bar;
...
INIT_LIST_HEAD(&foo);
mutex_init(&bar);
just do
static LIST_HEAD(foo);
static DEFINE_MUTEX(bar);
Also remove some superfluous struct list_head and spinlock_t
initialization calls where the variables are already defined using
macros that initialize them.
This saves a decent amount of compiled code too:
add/remove: 0/0 grow/shrink: 0/3 up/down: 0/-178 (-178)
function old new delta
target_core_init_configfs 898 850 -48
core_scsi3_emulate_pro_preempt 1742 1683 -59
iscsi_thread_set_init 159 88 -71
Signed-off-by: Roland Dreier <[email protected]>
Signed-off-by: Nicholas Bellinger <[email protected]>
Commit a0417fa3a1 ("net: Make qdisc_skb_cb upper size bound
explicit.") made it possible for a netdev driver to use skb->cb
between its header_ops.create method and its .ndo_start_xmit
method. Use this in ipoib_hard_header() to stash away the LL address
(GID + QPN), instead of the "ipoib_pseudoheader" hack. This allows
IPoIB to stop lying about its hard_header_len, which will let us fix
the L2 check for GRO.
Signed-off-by: Roland Dreier <[email protected]>
Signed-off-by: David S. Miller <[email protected]>
Commit a0417fa3a1 ("net: Make qdisc_skb_cb upper size bound
explicit.") made it possible for a netdev driver to use skb->cb
between its header_ops.create method and its .ndo_start_xmit
method. Use this in ipoib_hard_header() to stash away the LL address
(GID + QPN), instead of the "ipoib_pseudoheader" hack. This allows
IPoIB to stop lying about its hard_header_len, which will let us fix
the L2 check for GRO.
Signed-off-by: Roland Dreier <[email protected]>
Signed-off-by: David S. Miller <[email protected]>
Make sure all exit paths from this function unlock everything.
Reported-by: Bart Van Assche <[email protected]>
Signed-off-by: Roland Dreier <[email protected]>
My draft of SPC-4 says:
If the PAGE CODE field is not set to zero when the EVPD bit is set
to zero, the command shall be terminated with CHECK CONDITION
status, with the sense key set to ILLEGAL REQUEST, and the
additional sense code set to INVALID FIELD IN CDB.
Signed-off-by: Roland Dreier <[email protected]>
Cc: <[email protected]>
Signed-off-by: Nicholas Bellinger <[email protected]>
My draft of SPC-4 says:
If the device server does not implement the requested vital product
data page, then the command shall be terminated with CHECK CONDITION
status, with the sense key set to ILLEGAL REQUEST, and the
additional sense code set to INVALID FIELD IN CDB.
Signed-off-by: Roland Dreier <[email protected]>
Cc: <[email protected]>
Signed-off-by: Nicholas Bellinger <[email protected]>
According to SPC-4, the sense key for commands that are failed with
INVALID FIELD IN PARAMETER LIST and INVALID FIELD IN CDB should be
ILLEGAL REQUEST (5h) rather than ABORTED COMMAND (Bh). Without this
patch, a tcm_loop LUN incorrectly gives:
# sg_raw -r 1 -v /dev/sda 3 1 0 0 ff 0
Sense Information:
Fixed format, current; Sense key: Aborted Command
Additional sense: Invalid field in cdb
Raw sense data (in hex):
70 00 0b 00 00 00 00 0a 00 00 00 00 24 00 00 00
00 00
While a real SCSI disk gives:
Sense Information:
Fixed format, current; Sense key: Illegal Request
Additional sense: Invalid field in cdb
Raw sense data (in hex):
70 00 05 00 00 00 00 18 00 00 00 00 24 00 00 00
00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
with the main point being that the real disk gives a sense key of
ILLEGAL REQUEST (5h).
Signed-off-by: Roland Dreier <[email protected]>
Cc: <[email protected]>
Signed-off-by: Nicholas Bellinger <[email protected]>
Doing alloc_page(GFP_KERNEL | __GFP_ZERO) to get pages used for data
buffers wastes a lot of CPU clearing pages that will be quickly be
overwritten by the actual data. However, for emulated control
commands such as INQUIRY and so on, the code does assume that the
buffer is zeroed.
To avoid this CPU overhead, skip the __GFP_ZERO for commands that are
actually moving data, ie cmds that have SCF_SCSI_DATA_SG_IO_CDB set.
Signed-off-by: Roland Dreier <[email protected]>
Signed-off-by: Nicholas Bellinger <[email protected]>
rds_iw_flush_goal() just returns a count, but it is only called in one
place and its return value is ignored there. So delete all the dead code.
Signed-off-by: Roland Dreier <[email protected]>
Signed-off-by: David S. Miller <[email protected]>
Roland Dreier