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.
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
