Re: [PATCH] Snoop SET FEATURES - WRITE CACHE ENABLE/DISABLE command

[Tejun Heo <htejun@gmail.com>]

Jeff Garzik wrote:
> zhao, forrest wrote:
>> Hi, all
>>
>> This patch makes libata "Snoop SET FEATURES - WRITE CACHE ENABLE/DISABLE
>> command" and clean the things up(e.g. revalidate and rescan).
>> Here is the processing path:
>>
>> 1 in ata_scsi_qc_complete(), if we find that a "SET FEATURES - WRITE 
>> CACHE
>> ENABLE/DISABLE" command is executed successfully, we schedule the 
>> according
>> port to do revalidation
>> 2 in ata_dev_revalidate(), if we find that the "write cahce enable 
>> bit" was
>> changed, set port flags in order to schedule scsi_rescan_device() 
>> later in
>> ata_scsi_error()
>> 3 in ata_scsi_error(), if ATA_FLAG_SCSI_RESCAN is set for ap->flags, we
>> queue the ata_scsi_dev_rescan() to work_queue "ata_scsi_wq"
>> 4 at last, scsi_rescan_device() is invoked, so the current state of 
>> "write
>> cahce enable bit" is propogated to SCSI layer.
> 
> I agree with the concept, and agree that the SCSI layer must be notified 
> when the ATA write cache type changes.  However, I NAK the patch for the 
> following reasons:
> 
> 1) Tejun's revalidate should trigger scsi_rescan_device(), as your patch 
> indicates.  But that's pretty much all that needs to be done.
> 
> 2) Thus, a new bit (ATA_FLAG_SCSI_RESCAN) and a new workqueue are 
> unnecessary.

SCSI rescan is done by issuing commands using scsi_execute_req().  The 
commands are supposed to be processed via normal SCSI command execution 
path which is blocked during EH is in progress, so we need to rescan in 
separate context.

> 3) While the following test is correct,
> +    if ((dev->id[85] & (1 << 5)) != (id[85] & (1 << 5)))
> +        dev->ap->flags |= ATA_FLAG_SCSI_RESCAN;
> 
> there is no pressing need to avoid unnecessary rescans, during 
> revalidation.

Agreed.  Just rescan unconditionally.

> 4) Using [__]scsi_add_device() is a regression from using 
> scsi_scan_target()

I think it's taken from the hotplug patch store-attached-SCSI-device[1]. 
  Using [__]scsi_add_device() seems to be the only way to reliably 
obtain the attached sdev.

More notes...

* I've renamed ata_hotplug_wq to ata_scsi_wq in hotplug patches.  I 
think rescan can use this wq instead of creating its own.

* When snooping for SETFEATURES in ata_scsi_qc_complete(), why check 
cdb[0] for ATA_16/12?  Isn't simply checking tf.command enough?

* There's a race window between ATA revalidation and SCSI rescan.  We 
can plug this hole by deferring commands till rescan is complete.  But I 
doubt it would be worth the trouble.

-- 
tejun

[1] http://article.gmane.org/gmane.linux.ide/10898