From: Naresh Kumar Inna <naresh@chelsio•com>
To: James Bottomley <James.Bottomley@HansenPartnership•com>
Cc: "linux-scsi@vger•kernel.org" <linux-scsi@vger•kernel.org>,
"netdev@vger•kernel.org" <netdev@vger•kernel.org>
Subject: Re: [V4 PATCH 0/8] csiostor: Chelsio FCoE offload driver submission
Date: Tue, 18 Sep 2012 23:20:51 +0530 [thread overview]
Message-ID: <5058B47B.1050604@chelsio.com> (raw)
In-Reply-To: <1347957374.2388.15.camel@dabdike.int.hansenpartnership.com>
On 9/18/2012 2:06 PM, James Bottomley wrote:
> On Tue, 2012-09-18 at 09:54, Naresh Kumar Inna wrote:
>> Hi James,
>>
>> Could you please consider merging version V4 of the driver patches, if
>> you think they are in good shape now?
>
> Well, definitely not yet; you seem to have missed the memo on readq:
>
> CC [M] drivers/scsi/cxgbi/cxgb4i/cxgb4i.o
> drivers/scsi/csiostor/csio_hw.c: In function csio_hw_mc_read:
> drivers/scsi/csiostor/csio_hw.c:194:3: error: implicit declaration of
> function readq [-Werror=implicit-function-declaration]
>
> It's only defined on platforms which can support an atomic 64 bit
> operation (which is mostly not any 32 bit platforms) ... so this could
> do with compile testing on those.
>
> To see how to handle readq/writeq in the 32 bit case, see the uses in
> fnic or qla2xxx
>
Thanks for reviewing. I will fix up readq/writeq, as well as other
32-bit compilation issues, if any.
> You also have a couple of unnecessary version.h includes.
>
I will get rid of them.
> Since you're a new driver, could you not do a correctly unlocked
> queuecommand routine? You'll find the way you've currently got it coded
> (holding the host lock for the entire queuecommand routine) is very
> performance detrimental.
>
Yes, I am aware of that. However, some of this code was written and
tested before the lock-less queuecommand came into existence. Going the
lock-less route would require me to test the driver thoroughly again. It
definitely is in my to-do list, but I would like to take that up after
the initial submission goes through. Would that be OK?
> You have a lot of locking statements which aren't easy to audit by hand
> because there are multiple unlocks. Things like this:
>
> csio_scan_finished(struct Scsi_Host *shost, unsigned long time)
> {
> struct csio_lnode *ln = shost_priv(shost);
> int rv = 0;
>
> spin_lock_irq(shost->host_lock);
> if (!ln->hwp || csio_list_deleted(&ln->sm.sm_list)) {
> spin_unlock_irq(shost->host_lock);
> return 1;
> }
>
> rv = csio_scan_done(ln, jiffies, time, csio_max_scan_tmo * HZ,
> csio_delta_scan_tmo * HZ);
>
> spin_unlock_irq(shost->host_lock);
>
> return rv;
> }
>
> Are better coded as
>
> csio_scan_finished(struct Scsi_Host *shost, unsigned long time)
> {
> struct csio_lnode *ln = shost_priv(shost);
> int rv = 1;
>
> spin_lock_irq(shost->host_lock);
> if (!ln->hwp || csio_list_deleted(&ln->sm.sm_list))
> goto out;
>
> rv = csio_scan_done(ln, jiffies, time, csio_max_scan_tmo * HZ,
> csio_delta_scan_tmo * HZ);
>
> out:
> spin_unlock_irq(shost->host_lock);
>
> return rv;
> }
>
> It's shorter and the unlock clearly matches the lock. You could even
> invert the if logic and just make the csio_scan_done() conditional on it
> avoiding the goto.
>
I will try to minimize the lock/unlock mismatch instances per your
suggestions, if not eliminate them altogether.
> I'd also really like the people who understand FC to take a look over
> this as well.
>
Sure.
Thanks,
Naresh.
prev parent reply other threads:[~2012-09-18 17:50 UTC|newest]
Thread overview: 12+ messages / expand[flat|nested] mbox.gz Atom feed top
2012-09-12 17:18 [V4 PATCH 0/8] csiostor: Chelsio FCoE offload driver submission Naresh Kumar Inna
2012-09-12 17:18 ` [V4 PATCH 1/8] cxgb4/cxgb4vf: Chelsio FCoE offload driver submission (common header updates) Naresh Kumar Inna
2012-09-12 17:18 ` [V4 PATCH 2/8] csiostor: Chelsio FCoE offload driver submission (headers part 1) Naresh Kumar Inna
2012-09-12 17:18 ` [V4 PATCH 3/8] csiostor: Chelsio FCoE offload driver submission (headers part 2) Naresh Kumar Inna
2012-09-12 17:18 ` [V4 PATCH 4/8] csiostor: Chelsio FCoE offload driver submission (sources part 1) Naresh Kumar Inna
2012-09-12 17:18 ` [V4 PATCH 5/8] csiostor: Chelsio FCoE offload driver submission (sources part 2) Naresh Kumar Inna
2012-09-12 17:18 ` [V4 PATCH 6/8] csiostor: Chelsio FCoE offload driver submission (sources part 3) Naresh Kumar Inna
2012-09-12 17:18 ` [V4 PATCH 7/8] csiostor: Chelsio FCoE offload driver submission (sources part 4) Naresh Kumar Inna
2012-09-12 17:18 ` [V4 PATCH 8/8] csiostor: Chelsio FCoE offload driver submission (sources part 5) Naresh Kumar Inna
2012-09-18 4:24 ` [V4 PATCH 0/8] csiostor: Chelsio FCoE offload driver submission Naresh Kumar Inna
2012-09-18 8:36 ` James Bottomley
2012-09-18 17:50 ` Naresh Kumar Inna [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=5058B47B.1050604@chelsio.com \
--to=naresh@chelsio$(echo .)com \
--cc=James.Bottomley@HansenPartnership$(echo .)com \
--cc=linux-scsi@vger$(echo .)kernel.org \
--cc=netdev@vger$(echo .)kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox