Re: xcopy testing with ddpt

[Chris Boot <bootc@bootc.net>]

On 07/10/2013 23:38, Nicholas A. Bellinger wrote:
> On Mon, 2013-10-07 at 15:18 -0700, Nicholas A. Bellinger wrote:
>> On Mon, 2013-10-07 at 06:03 +0200, Thomas Glanzmann wrote:
>>> Hello Doug,
>>>
>>> * Douglas Gilbert <dgilbert@interlog.com> [2013-10-07 00:58]:
>>>> Great, another one working.
>>
>> (CC'ing Hannes)
>>
>>>> BTW list_id=0 has a special meaning in some context
>>>> (buried deep in T10 documents: spc4r36j.pdf). That is
>>>> probably why Hannes Reinecke defaulted that list_id to
>>>> 1. I could understand the target XCOPY implementation
>>>> only accepting one xcopy sequence at a time, but why
>>>> restrict it to list_id=0 ? A question for NaB ...
>>>
>>> Nab, do you have any input for us?
>>>
>>
>> It was my original understanding that when OPERATING_PARAMETERS is
>> reporting SNLID=1 (Supports No ListID), the initiator is expected to
>> send EXTENDED_COPY parameter lists with ListID Usage 11b + ListID=0.
>> Since we're ignoring the value of ListID for now anyways, I agree that
>> it doesn't make much sense to fail for a non zero value here..
>>
>> However, the main concern that made me add this check to begin with was
>> the case with ListID Usage 00b + 10b, where the copy server is expected
>> to keep a per I_T list of in-use ListIDs, and return CHECK_CONDITION +
>> ILLEGAL REQUEST/OPERATION IN PROGRESS for a ListID for a copy sequence
>> already in progress.
>>
> 
> How about the following patch to allow non zero ListIDs, but only when
> ListID Usage is set to 11b..?
> 
> diff --git a/drivers/target/target_core_xcopy.c b/drivers/target/target_core_xcopy.c
> index 6b9774c..3a3ea31 100644
> --- a/drivers/target/target_core_xcopy.c
> +++ b/drivers/target/target_core_xcopy.c
> @@ -911,11 +911,12 @@ sense_reason_t target_do_xcopy(struct se_cmd *se_cmd)
>         }
>  
>         list_id = p[0];
> -       if (list_id != 0x00) {
> -               pr_err("XCOPY with non zero list_id: 0x%02x\n", list_id);
> +       list_id_usage = (p[1] & 0x18);
> +       if (list_id != 0x00 && list_id_usage != 0x11) {
> +               pr_err("XCOPY with non zero list_id: 0x%02x, and list_id_usage:"
> +                      " 0x%02x\n", list_id, list_id_usage);
>                 goto out;
>         }
> -       list_id_usage = (p[1] & 0x18);
>         /*
>          * Determine TARGET DESCRIPTOR LIST LENGTH + SEGMENT DESCRIPTOR LIST LENGTH
>          */
> 
> AFAICT this should make ddpt happy, as it's already be setting ListID
> Usage = 11b when it gets OPERATING PARAMETERS -> HELD_DATA = 0.

0x11 != 11b (but == 11h)

If 0x18 is the correct mask I think you want to compare against 0x18,
otherwise you probably want to shift down by 3 bits and compare against
0x03 or 0b11...

HTH,
Chris

-- 
Chris Boot
bootc@bootc.net