ECKD DASD - Read Tracks CCW x'DE' Fails When Chained From Prefix CCW

[arfineman]

Read Track is failing when chained to a Prefix command.

It appears code at label 0xDE is not checking for Prefix command. I'm sure there are other commands that are checking for LRE and not Prefix. But a format 0 Prefix with Define Extent valid bit should satisfy Define Extent chaining requirement and a format 1 Prefix must satisfy LRE chaining requirement.

Best regards,

This is a sample Read Track CCW chain with LRE that successfully executes:

DEFEXT    CCW1  X'63',DEFXTP,X'40',16
LOCRE     CCW1  X'4B',LOCREP,X'40',20
RDTRK     CCW1  X'DE',BUFFER,X'20',65535      Track 0
*
DEFXTP  DC   X'00C00000 00000000 00000000 00000000'
LOCREP  DC   X'0C000001 00000000 00000000 00000000 00000000'

This is a sample Read Track CCW chain with Prefix that fails:

PFX   CCW1  X'E7',PFXP,X'40',64       
      CCW1  X'DE',BUFFER,X'20',65535    

PFXP  DC    X'01800000 00000000 00000000'                           
DEP   DC    X'00C00000 00000000 00000000 00000000'                  
      DC    X'00000000 00000000 00000000 00000000'                  
LREP  DC    X'0C000001 00000000 00000000 00000000 00000000'

    case 0xDE:
    /*---------------------------------------------------------------*/
    /* READ TRACK                                                    */
    /*---------------------------------------------------------------*/

        /* Command reject if not chained from a Locate Record
           command or from another Read Track command */
        if (0
            || chained == 0
            || (1
                && prevcode != 0x47 // LOCATE RECORD
                && prevcode != 0x4B // LOCATE RECORD EXTENDED
                && prevcode != 0xDE // READ TRACK
               )
        )
        {

[arfineman]

I added Prefix CCW to the Read Track chaining requirements and recompiled using Hercules Helper and it fixed the issue:

 case 0xDE:
    /*---------------------------------------------------------------*/
    /* READ TRACK                                                    */
    /*---------------------------------------------------------------*/
        /* Command reject if not within the domain of a Locate Record
           that specifies a read tracks operation */
        if (dev->ckdlcount == 0
         || (((dev->ckdloper & CKDOPER_CODE) != CKDOPER_RDTRKS)
          && ((dev->ckdloper & CKDOPER_CODE) != CKDOPER_RDTSET)))
        {
            ckd_build_sense( dev, SENSE_CR, 0, 0, FORMAT_0, MESSAGE_2 );
            *unitstat = CSW_CE | CSW_DE | CSW_UC;
            break;
        }

        /* Command reject if not chained from a Locate Record
           command or from another Read Track command */
        if (0
            || chained == 0
            || (1
                && prevcode != 0x47 // LOCATE RECORD
                && prevcode != 0x4B // LOCATE RECORD EXTENDED
                && prevcode != 0xDE // READ TRACK
                && prevcode != 0xE7 // PREFIX
               )

Best regards,

[Peter-J-Jansen]

Aaron,

Am I correct in assuming that your correction in ckddasd.c would be like this single addition? :

--- SDL-hyperion_4f007805/ckddasd.c 2023-11-11 16:56:04.337029153 +0100
+++ SDL-hyperion/ckddasd.c 2023-11-14 11:24:48.974347751 +0100
@@ -3208,6 +3208,7 @@
                 && prevcode != 0x47 // LOCATE RECORD
                 && prevcode != 0x4B // LOCATE RECORD EXTENDED
                 && prevcode != 0xDE // READ TRACK
+                && prevcode != 0xE7 // PREFIX
                )
         )
         {

I have already tested this correction of yours, hoping that perhaps it would also solve issue #608 which I created today, but it did not. However, your correction did not affect that test at all, so it didn't break things. Other than that, I must admit that I know way to little about DASD CCW programming to understand the issues and their solutions. :-)

Cheers,

Peter

[arfineman]

Hi Peter,

Yes, my change was applied to ckddasd.c. I'm certain in addition of the x'DE' there are other CCWs that would need fixing if chained to a Prefix.

Although Fish did a great job fixing the Prefix CCW code, there was one very important requirement was left out: if you look at my comment of June 15 in issue #572 about recommendation of how Prefix should be implemented, "Set flag DX received for chaining requirements" and "Set flag LRE received for chaining requirements" were left out.

The correct fix was after the 'Define_Extent' subroutine is called to set prevcode='63' and after "Locate_Record_Extended" subroutine is called to set prevcode=4B. This would have eliminated the need of having a reference to Prefix to any other CCW.

I did make this change to my own version of ckddasd.c, but when I compile it, I get compilation warnings and Hercules Helper does not continue. I'll try to fix this at some future time.

In your case, if you send me an I/O trace of the first level system for the failing devices, I maybe able to explain the cause of the problem and determine if it is related to this issue.

Best regards,

[wrljet]

Aaron, I can't help with channel programming, but I can probably solve your build issues.

Please zip and post the full log that Hercules-Helper produced for your failing build.

Bill

[arfineman]

Hi Bill,
I'm not implying there is an issue with Hercules Helper. The problem is when I add the below three 'prevcode = 0x63/4B" the compilation fails, meaning I'm introducing syntax error. I wonder if Peter can fix this ? If this fix is implemented my previous update for 0xDE is no longer needed.
Best regards,

    switch (dev->ckdformat)
    {
    case PFX_F_DE:

        /* Process Define Extent field only if provided (valid) */
        if (dev->ckdvalid & PFX_V_DE_VALID)
        {
            DefineExtent( dev, code, flags, chained, count, prevcode,
                          ccwseq, iobuf, more, unitstat, residual );

            prevcode = 0x63;     
        }
        else
        {
            /* Return normal status */
            *unitstat = CSW_CE | CSW_DE;
        }
        break; // Done!

    case PFX_F_DE_LRE:

        /* Process Define Extent field only if provided (valid) */
        if (dev->ckdvalid & PFX_V_DE_VALID)
        {
            DefineExtent( dev, code, flags, chained, count, prevcode,
                          ccwseq, iobuf, more, unitstat, residual );
            if (*unitstat != (CSW_CE | CSW_DE))
                break; // (error!)

            prevcode = 0x63;         
        }

       

        LocateRecordExtended( dev, code, flags, chained, count, prevcode,
                              ccwseq, iobuf, more, unitstat, residual );
        
        prevcode = 0x4B;         

        break; // Done!

[wrljet]

Hi Bill, I'm not implying there is an issue with Hercules Helper.

I understand that.

The problem is when I add the below three 'prevcode = 0x63/4B" the compilation fails, meaning I'm introducing syntax error. I wonder if Peter can fix this ? If this fix is implemented my previous update for 0xDE is no longer needed. Best regards,

The log will show me/us what the compiler thinks about your code. Such as the exact error messages.

On the surface from what you've posted, that seems like legit C.
(of course I have NO idea what effect is has on the processing in that code)

Bill

[Peter-J-Jansen]

Hi Aaron,

I've added those three prevcode = 0x63/4B statements and removed your previous && prevcode != 0xE7 // PREFIX, and the result compiled perfectly clean for me. Also my most stringent test (z/VM plus z/OS 2nd level) worked identically as bebore.

(I.e. still exhibiting issue #608 as before, somehow as expected. I'll next try to get an I/O trace from DASD 0A82 as you requested for that issue.)

--- SDL-hyperion_4f007805/ckddasd.c 2023-11-11 16:56:04.337029153 +0100
+++ SDL-hyperion/ckddasd.c 2023-11-15 15:08:39.708625661 +0100
@@ -3322,6 +3322,7 @@
             {
                 DefineExtent( dev, code, flags, chained, count, prevcode,
                               ccwseq, iobuf, more, unitstat, residual );
+                prevcode = 0x63;                              
             }
             else
             {
@@ -3339,10 +3340,12 @@
                               ccwseq, iobuf, more, unitstat, residual );
                 if (*unitstat != (CSW_CE | CSW_DE))
                     break; // (error!)
+                prevcode = 0x63;                    
             }
 
             LocateRecordExtended( dev, code, flags, chained, count, prevcode,
                                   ccwseq, iobuf, more, unitstat, residual );
+            prevcode = 0x48;                                  
             break; // Done!
 
         case PFX_F_DE_PSF:

Does this match the source change you're wishing to implement ?

I'm in the same position as Bill, having absolutely no clue as the the effect this change has. All I can testify is that it seems to have no effects, i.e. also no ill effects, on my testing as mentionned.

So far I've only tried this on my most up-to-date Linux system, Ubuntu LTS 22.04, using CLANG compiler version 14.0.0 :

hercules@pjjs12:~$ clang --version
Ubuntu clang version 14.0.0-1ubuntu1.1
Target: x86_64-pc-linux-gnu
Thread model: posix
InstalledDir: /usr/bin
hercules@pjjs12:~$ 

Perhaps Bill can assist you with a possible build problem.

Cheers,

Peter

[arfineman]

Hi Peter,
Yes, this was the code that I wanted to add to correct the Read Track CCW problem.

Please change the 0x48 to 0x4B (CCW code for "Locate Record Extended") and try again.

If it did not resolve the error, either do a t+uuuu on Hercules console for the failing device or a #CP TR I/O uuuu RUN on the first level VM system. I need to see the complete CCW chain for the failing I/O.
Best regards,

[wrljet]

I have no idea what those constants mean. Before any of this is considered to be incorporated, you'll need to use symbolic equates and comment what's being done.

Also providing some test cases would be encouraged.

Bill

[arfineman]

My compilation issue got resolved by updating the ckddasd.c with Microsoft Notepad, not with WordPad. I think WordPad was adding some control characters to the file that was causing ckddasd.c(5527): error C4101: 'orig_iobuf': unreferenced local variable error. I'm good for now on both my issues.
Best regards,

[arfineman]

This issue is resolved.

[Fish-Git]

This issue is resolved.

Aaron! (@arfineman) Please don't close issues until the actual fix has actually been committed to the official repository! Just because the fix is in your local repository, doesn't mean it's in our publicly accessible repository!

That is to say, technically, once you commit a fix, you should do a "push" to apply your local changes to the remote repository as well. Only then has the fix been officially resolved and the issue may be closed.

But because you are not a developer (would you like to be one? Please?), you have no "write" (commit) access to our repository, and so therefore cannot push your changes. When this happens, your local Hercules that you use works fine, but the problem still exists for everyone else! (because the fix never got committed!)

Thanks.

Your fix has now been committed.

[arfineman]

But because you are not a developer (would you like to be one? Please?),

Thank you, Fish.

I'm starting to understand the ckddasd.c code structure. Time permitting, at some point I would like to provide updates to support Write Track Set (RDC byte 2, bit 6) and Locate Record Erase (RDC Byte 2, bit 7).

Best regards,

[arfineman]

Hi Fish,
Peter's original update had a typo that I made the correction on my Nov 15th update:

Please change the 0x48 to 0x4B (CCW code for "Locate Record Extended") and try again.

Did you make this correction?
Best regards,

[Fish-Git]

Peter's original update had a typo that I made the correction on my Nov 15th update:

Please change the 0x48 to 0x4B (CCW code for "Locate Record Extended") and try again.

Did you make this correction?

I apologize for not mentioning it, but yes, if you look at what was actually committed, you will see that change was included in the commit (ba93eef).