1) the TRACE stuff uses too much stack space
2) style: way too many interal headers; feel free to disagree, though, this is maintainer's preference.
3) non-standard function definitions:
+extern void iscsi_non_scsi_queue_cmd (
+ iscsi_cmd_t *cmd,
+ iscsi_session_t *sess)
+{Use the standard style instead.
4) My initial reaction upon reading the following code is, "where is the document describing this mess of locking?"
+ if (atomic_read(&sess->nconn) == 1) {
+ up(&sess->schedule_conn->tx_sem);
+ spin_unlock_irqrestore(&sess->conn_schedule_lock, flags);
+ return;
+ }Presumably, sess->nconn need not be atomic?
In practice, I have found that use of atomic_t often _creates_ races.
5) style: these function headers are completely pointless, conveying no additional insight or information:
+/* iscsi_add_nopout_noqueue(): + * + * + */
6) This is very worrisome, at the beginning of your queuecommand hook:
+ /* + * Get the assoicated iSCSI Channel for this active SCSI Task. + */ + spin_unlock_irq(sc->device->host->host_lock);
It's fairly dangerous to assume that this will work, if, e.g. ->queuecommand() is called from BH context.
Also, it means you are accessing the internal LUN list without any locking at all.
7) leak on error:
+ if (!(cmd = iscsi_allocate_cmd(sess, NULL)))
+ return(-1);
+
+ if ((padding = ((text_len) & 3)) != 0) {
+ TRACE(TRACE_ISCSI, "Attaching %u additional bytes for"
+ " padding.\n", padding);
+ }
+
+ if (!(text = (unsigned char *) kmalloc((text_len + padding), GFP_ATOMIC)
)) {
+ TRACE_ERROR("Unable to allocate memory for outgoing text\n");
+ return(-1);
+ }
8) iscsi_build_scsi_cmd() appears to leak cmd->buf_ptr on error, but I could be wrong
9) style: Linux kernel style discourages use of "foo_t", and prefers "struct foo"
10) iscsi_build_dataout() leak on error:
+ if (!(pdu = iscsi_get_pdu_holder_from_r2t(cmd, r2t))) + return(-1); ... + if ((iov_ret = iscsi_map_scatterlists((void *)&map_sg, + (void *)unmap_sg)) < 0) + return(-1);
11) excessive stack usage. will cause crash with 4K stacks:
+ unsigned char ping_data[ISCSI_MAX_PING_DATA_BYTES];
12) general comment: It is damned difficult to figure out which context these functions are operating in. In iscsi_build_text_req() I see you grab a spinlock with spin_lock(). May we presume that local_irq_save() is active? Have you audited this code for ABBA deadlocks?
13) delete all of the following #ifdefs:
+#ifdef CRYPTO_API_CRC32C
and replace with a requirement in Kconfig.
14) Many instances where iscsi_find_cmd_from_itt_or_dump() returns a command, and then function returns on error without releasing cmd.
This -may- be a leak on error, maybe not.
15) locking bug: conn->state_lock is locked with spin_lock() in process context, spin_lock_bh() in other contexts.
16) stack usage:
+extern int iscsi_initiator_rx_thread (void *arg)
+{
+ int ret;
+ u8 buffer[ISCSI_HDR_LEN], opcode;
17) potential race in iscsi_create_connection():
+ if ((atomic_read(&sess->nconn) + atomic_read(&c->connection_count)) >
18) excessive stack usage:
+extern int iscsi_free_session (iscsi_session_t *sess)
+{
+ int i = 0, j = 0;
+ iscsi_conn_t *conn, *conn_next = NULL;
+ iscsi_conn_t *conn_array[ISCSI_MAX_CONNECTIONS];
19) ditto:
+extern int iscsi_stop_session (iscsi_session_t *sess, int session_sleep, int co
nnection_sleep)
+{
+ int i = 0, j = 0;
+ iscsi_conn_t *conn, *conn_next = NULL;
+ iscsi_conn_t *conn_array[ISCSI_MAX_CONNECTIONS];
20) numerous code bloat, by checking for NULL before calling kfree():
+ if (c) + kfree(c);
kfree(NULL) is ok.
21) delete FULL_PARAMS_COUNT constant, use ARRAY_SIZE() macro in linux/kernel.h
22) excessive stack usage:
+static int iscsi_setup_full_params (iscsi_channel_t *c)
+{
+ unsigned char buf[512];
23) ditto:
+extern int iscsi_init_channel (iscsi_channel_t *channel, u32 max_sectors, int f
ull_init)
+{
+ unsigned char buf[512];
I stopped reading at this point.
In general,
a) locking is completely unreviewable, incomprehensible and unmaintainable by anyone except the author(s). Either a locking rewrite, or a locking proof document, is needed.
b) aside from locking, stack usage, and leaks on error, the code seems reasonably clean.
- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
