Re: [PATCH 1/1] sym53c8xx_2: Fix validation (Fix hotplug support).

["Koskinen Aaro (NSN - FI/Helsinki)" <aaro.koskinen@nsn.com>]

James Bottomley wrote:
> This seems to be a bug in sym_prepare_nego(), yes ... it should do
> either a PPR or WDTR/SDTR sequence ... there is a case where it won't do
> anything when asked.
> 
>> - The patch deletes code from sym_sir_task_recovery()/M_RESET branch. It's
>> unnecessary to reset negotiation status there, since any reset eventually
>> triggers S_CHECK_COND.
> 
> It looks like a reasonable safety feature against problem drives ... I'd
> be hesitant to remove it.

To me, the code looked like it was added there to compensate the bug 
mentioned above. Anyway, it probably does no harm to leave it there.

>> - It seems that the SPI "domain validation" consists solely of INQUIRY
>> commands.
> 
> Actually, no.  DV uses Inquiry solely for non DT negotiations because of
> potential echo buffer problems in certain drives but it uses the echo
> buffer for DT verification.

Yes, you are correct.

>>  As a result, if (b) is followed, targets that support PPR would
>> never get to the point of doing PPR transfers during domain validation
>> (but immediately after on the next command following the dv), since each
>> INQUIRY re-starts the negotiation cycle. This patch solves this by making
>> it possible to do the negotiation cycle WIDE -> SYNC -> PPR during a
>> single SCSI command.
> 
> I don't quite follow this.  The way SPI DV works is that we start out
> fully async for the first INQUIRY, then move to wide async (to check no
> bus width domain problems, which are the most common) then jump to the
> highest speed supported by the device and transport.  i.e. we always do
> 
> async -> WDTR -> SDTR
> 
> or
> 
> async -> WDTR -> PPR
> 
> we never go
> 
> async -> WDTR -> SDTR -> PPR.
> 
> What's the actual problem you are seeing?

If you consider this patch, the WDTR message will be resent when the DV 
jumps to the highest speed, and then, if you look at the code in 
sym_wide_nego(), the driver will also send SDTR after the target 
responds to WDTR since the goal offset is now set. So what you will get 
is WDTR -> SDTR -> PPR.

But now that I think of it, perhaps the code in sym_wide_nego() should 
already check whether to send PPR instead of SDTR if the goals are 
appropriate.

A.