| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-Id: <1305784824.2856.576.camel@haakon2.linux-iscsi.org>
Date: Wed, 18 May 2011 23:00:24 -0700
From: "Nicholas A. Bellinger" <nab@...ux-iscsi.org>
To: Christoph Hellwig <hch@...radead.org>
Cc: linux-kernel <linux-kernel@...r.kernel.org>,
linux-scsi <linux-scsi@...r.kernel.org>,
linux-rmda <linux-rdma@...r.kernel.org>,
Roland Dreier <roland@...estorage.com>,
Bart Van Assche <bvanassche@....org>,
Vu Pham <vu@...lanox.com>, David Dillow <dillowda@...l.gov>,
James Bottomley <James.Bottomley@...senPartnership.com>,
FUJITA Tomonori <fujita.tomonori@....ntt.co.jp>
Subject: Re: [RFC] ib_srpt: initial .40-rc1 drivers/infiniband/ulp/srpt
merge
On Wed, 2011-05-18 at 03:47 -0400, Christoph Hellwig wrote:
> > +ccflags-y := -Idrivers/target
>
> Why do you need the ccflags? Everything needed should be under
> include/target, and if not that needs to be fixed.
>
> > +#include <linux/kthread.h>
> > +#include <linux/string.h>
> > +#include <linux/delay.h>
> > +#include <linux/atomic.h>
> > +#include <scsi/libsas.h> /* TASK_ATTR_* */
>
> We really need to stop spreading that include. Care to submit a patch
> for .40 to move the TASK_ATTR_* defines to scsi/scsi.h?
>
Agreed, this conversion is definately over-due for target-core.
Pushing this change now in lio-4.1 to use scsi_tcq.h for LIO upstream
fabric modules tcm_loop, iscsi-target, tcm_fc, ibmvscsis, qla2xxx,
ib_srpt, and tcm_vhost:
commit 53076ba5838d9653e273c3e43b76fd8220e459a4
Author: Nicholas Bellinger <nab@...ux-iscsi.org>
Date: Wed May 18 22:24:05 2011 -0700
target: Convert TASK_ATTR to scsi_tcq.h definitions
commit ffee1a685bc083b4c0984230affac6b434b1ea4c
Author: Nicholas Bellinger <nab@...ux-iscsi.org>
Date: Wed May 18 22:41:31 2011 -0700
ibmvscsis: Convert TASK_ATTR to scsi_tcq.h definitions
I will get this queued against scsi-misc for .40 with tcm_loop+tcm_fc
bits for tomorrow & respin a fresh iscsi-target pull as well.
> > +/**
> > + * srpt_sdev_name() - Return the name associated with the HCA.
> > + *
> > + * Examples are ib0, ib1, ...
> > + */
> > +static inline const char *srpt_sdev_name(struct srpt_device *sdev)
> > +{
> > + return sdev->device->name;
> > +}
>
> Does this really need a helper?
>
Not sure..
> > +
> > +static enum rdma_ch_state srpt_get_ch_state(struct srpt_rdma_ch *ch)
> > +{
> > + unsigned long flags;
> > + enum rdma_ch_state state;
> > +
> > + spin_lock_irqsave(&ch->spinlock, flags);
> > + state = ch->state;
> > + spin_unlock_irqrestore(&ch->spinlock, flags);
> > + return state;
> > +}
>
> Given that the channel is a 32-bit value taking a lock over reading
> it doesn't help anything. If you need any kind of exclusion it needs
> to be held over the actual use of it, else it can be dropped.
>
Mmm, looks like it's safe to drop. Bart..?
> > +static enum rdma_ch_state
> > +srpt_set_ch_state(struct srpt_rdma_ch *ch, enum rdma_ch_state new_state)
> > +{
> > + unsigned long flags;
> > + enum rdma_ch_state prev;
> > +
> > + spin_lock_irqsave(&ch->spinlock, flags);
> > + prev = ch->state;
> > + ch->state = new_state;
> > + spin_unlock_irqrestore(&ch->spinlock, flags);
> > + return prev;
> > +}
>
> The oly caller of this does a spin_lock_irq so I assume it's from
> process context. So this one could do the same. It would be good idea
> to check what kind of action is done from irq context at all and
> document what data structures / critical sections can be access from
> there.
>
IB folks..?
> > +/**
> > + * srpt_srq_event() - SRQ event callback function.
> > + */
> > +static void srpt_srq_event(struct ib_event *event, void *ctx)
> > +{
> > + printk(KERN_INFO "SRQ event %d\n", event->event);
> > +}
>
> Is this overly useful?
>
IB folks..?
> > +static enum srpt_command_state srpt_get_cmd_state(struct srpt_send_ioctx *ioctx)
> > +{
> > + enum srpt_command_state state;
> > + unsigned long flags;
> > +
> > + BUG_ON(!ioctx);
> > +
> > + spin_lock_irqsave(&ioctx->spinlock, flags);
> > + state = ioctx->state;
> > + spin_unlock_irqrestore(&ioctx->spinlock, flags);
> > + return state;
>
> Same comment about reading a variable as above.
>
Bart..?
> > +/*
> > + * srpt_unpack_lun() - Convert from network LUN to linear LUN.
> > + *
> > + * Convert an 2-byte, 4-byte, 6-byte or 8-byte LUN structure in network byte
> > + * order (big endian) to a linear LUN. Supports three LUN addressing methods:
> > + * peripheral, flat and logical unit. See also SAM-2, section 4.9.4 (page 40).
> > + */
> > +static uint64_t srpt_unpack_lun(const uint8_t *lun, int len)
>
> Nick, didn't you plan to take the LUN addressing to the core? After
> all it's not a transport specific format.
>
Yes, this should be using scsilun_to_int(), but in current mainline code
these is still required to load. The iscsi-target PATCH-v4 series patch
containing this fix wrt to REPORT_LUNs needs to go into mainline before
this can be fixed..
[PATCH-v4 03/14] target: Convert REPORT_LUNs to use int_to_scsilun
http://marc.info/?l=linux-scsi&m=130561442821080&w=2
Will get this converted in ib_srpt for lio-4.1 for now..
> > +static int srpt_compl_thread(void *arg)
> > +{
> > + struct srpt_rdma_ch *ch;
> > +
> > + /* Hibernation / freezing of the SRPT kernel thread is not supported. */
> > + current->flags |= PF_NOFREEZE;
> > +
> > + ch = arg;
> > + BUG_ON(!ch);
> > + printk(KERN_INFO "Session %s: kernel thread %s (PID %d) started\n",
> > + ch->sess_name, ch->thread->comm, current->pid);
>
> Can we please kill all these verbose printks? I know the target core
> does them, but that needs to be fixed to. If you really care for
> debugging you can trivially trace it using the function tracer.
>
Ugh yes, this still needs to be addressed properly for target core..
> > + while (!kthread_should_stop()) {
> > + wait_event_interruptible(ch->wait_queue,
> > + (srpt_process_completion(ch->cq, ch),
> > + kthread_should_stop()));
> > + }
>
> Instead of doing a wait_event_interruptible in a kthread you can just
> do a schedule() in interruptible context and use wake_up_process to wake
> it up.
>
This conversion looks reasonable to me, and will get this resolved for
the next round.
Thanks for your review Christoph!
--nab
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists