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

[Jeff Garzik <jeff@garzik.org>]

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.

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.

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