iochannel.h SET_NO_DISK_INQUIRY_RESULT is an unsightly macro[staging-testing]

[AlexanderCurtin]

It's a functiony macro, basically, and it only is the way it is because it's used for both windows and linux requests. Would it be better to add it directly to the do_scsi_nolinuxstat command in visorhba_main? (the only place it's used) Or would it be better as a function.

Current Definition

#define SET_NO_DISK_INQUIRY_RESULT(buf, len, lun, lun0notpresent, notpresent) { \
    MEMSET(buf, 0, MINNUM(len, (unsigned int)NO_DISK_INQUIRY_RESULT_LEN)); \
    buf[2] = (U8)SCSI_SPC2_VER; \
    if(lun == 0) { \
        buf[0] = (U8)lun0notpresent; \
        buf[3] = (U8)DEV_HISUPPORT; \
    } \
    else \
        buf[0] = (U8)notpresent; \
    buf[4] = (U8)(MINNUM(len, (unsigned int)NO_DISK_INQUIRY_RESULT_LEN)-5); \
    if(len >= NO_DISK_INQUIRY_RESULT_LEN) { \
        buf[8] = 'D'; \
        buf[9] = 'E'; \
        buf[10] = 'L'; \
        buf[11] = 'L'; \
        buf[16] = 'P'; \
        buf[17] = 'S'; \
        buf[18] = 'E'; \
        buf[19] = 'U'; \
        buf[20] = 'D'; \
        buf[21] = 'O'; \
        buf[22] = ' '; \
        buf[23] = 'D'; \
        buf[24] = 'E'; \
        buf[25] = 'V'; \
        buf[26] = 'I'; \
        buf[27] = 'C'; \
        buf[28] = 'E'; \
        buf[30] = ' '; \
        buf[31] = '.'; \
    }\
 }

Proposed function

This is an attempt I made in the branch visor_hba_macro_fix. I think the commit would be 7b6b16a0b470fa6983135bd8cb0e59383839fb30

static int set_no_disk_inquiry_result(char * buf, u32 len, u32 lun){
       const char * disk_name;

       disk_name  = "DELLPSEUDO DEVICE .";
       /* len has to be more than the min result length*/
       if (len < NO_DISK_INQUIRY_RESULT_LEN){
               return EINVAL;
       }
       memset(buf, 0, MINNUM(len, NO_DISK_INQUIRY_RESULT_LEN));
       buf[2] = (u8)SCSI_SPC2_VER;
       if (lun) {
               buf[0] = (u8)DEV_NOT_CAPABLE;
       } else {
               buf[0] = (u8)DEV_DISK_CAPABLE_NOT_PRESENT;
               buf[3] = (u8)DEV_HISUPPORT;
       }
       buf[4] = (u8)(MINNUM(len, NO_DISK_INQUIRY_RESULT_LEN) - 5);
       if (len >= NO_DISK_INQUIRY_RESULT_LEN) {
               memcpy(buf + 8, disk_name, strlen(disk_name));
       }
       return 0;
}

[AlexanderCurtin]

Interestingly, the macro as it stands in the upstream is a do/while where the one that I copied in from grok just does the thing without a do/while wrapper.

[selltc]

This is MUCH better Alex, but it just occurred to me that perhaps the kernel already has infrastructure somewhere for for accessing inquiry data. I'll look around a little.

In your code, I think you meant return -EINVAL instead of return EINVAL.

[selltc]

Interestingly, the macro as it stands in the upstream is a do/while where the one that I copied in from grok just does the thing without a do/while wrapper

Non-trivial macros without do/while are typically bad form, and fragile

[AlexanderCurtin]

I was thinking the same thing, and I may have been looking around the wrong parts, but the ones that I found were driver specific, and seemed kind of kludgey on their own.

eg. http://lxr.free-electrons.com/source/drivers/staging/rts5208/rtsx_scsi.c#L474

[selltc]

Have you run your patch thru checkpatch.pl? I figured it would complain about a few of the missing spaces before "{".

[AlexanderCurtin]

Not yet, I was just trying to get a feel for what might actually work. I'd imagine there are multiple issues with the patch as it currently stands. I'll give it a look.

[selltc]

Your strategy is good... I was nit-picking too soon. Don't worry about checkpatch.pl yet.

[AlexanderCurtin]

So I did end up running it through checkpatch.pl, which caught just about as many issues as I had lines. I updated it. It would still need comments, and I'm not really sure what to actually do with the return value in the calling function, or if I should just modify the new func to fail silently. Effectively, there'd be no change in functionality. I just know for sure that we don't want to write anything if the buffer length is less than the message we're putting in. Also, I didn't check for null, so that's not gonna fly.

[davidker]

I'm fine with this change, but just remember this is to visorhba, not visorbus. We are currently trying to get visorbus out of staging. So if we have something we can send up but we should be looking at other issues as well.

[selltc]

I agree this can wait, but I'll comment now while I'm thinking about it.

It really stinks that the Linux kernel doesn't appear to have a global struct defined for SCSI inquiry data. IBM has it defined in ips.h, but that's not in a public include directory, and is only used by their driver:

typedef struct {
   uint8_t   DeviceType;
   uint8_t   DeviceTypeQualifier;
   uint8_t   Version;
   uint8_t   ResponseDataFormat;
   uint8_t   AdditionalLength;
   uint8_t   Reserved;
   uint8_t   Flags[2];
   uint8_t   VendorId[8];
   uint8_t   ProductId[16];
   uint8_t   ProductRevisionLevel[4];
   uint8_t   Reserved2;                                  /* Provides NULL terminator to name */
} IPS_SCSI_INQ_DATA, *PIPS_SCSI_INQ_DATA;

Maybe I just haven't look hard enough yet. I would love for us to be able to define buf as a struct/typedef instead of using a char[] and hard-coding literals as array indices.

On with the nit-picking.

Your if (len >= NO_DISK_INQUIRY_RESULT_LEN) check is extraneous, because you already returned from the function at the beginning (with -EINVAL) if that check failed.

It seems unnecessary to me to have a separate variable for disk_name, when it would be easier to just include include the string in the memcpy, if you change to strncpy, like this:

strncpy(buf + 8, "DELLPSEUDO DEVICE .", NO_DISK_INQUIRY_RESULT_LEN-8);

I think this comment is just noise, as it just echoes what the code is obviously doing:

/* len has to be more than the min result length*/

The u8 casts are extraneous. If we really want to emphasize that all of this data is 8-bit UNsigned, the parameter passed into the function should by u8 * instead of char *.

We are going to get called out for our use of MINNUM. That's our define in iochannel.h, but Linux already has equivalent min() / max() macros defined in include/linux/kernel.h:

/*
 * min()/max()/clamp() macros that also do
 * strict type-checking.. See the
 * "unnecessary" pointer comparison.
 */
#define min(x, y) ({                \
    typeof(x) _min1 = (x);          \
    typeof(y) _min2 = (y);          \
    (void) (&_min1 == &_min2);      \
    _min1 < _min2 ? _min1 : _min2; })

#define max(x, y) ({                \
    typeof(x) _max1 = (x);          \
    typeof(y) _max2 = (y);          \
    (void) (&_max1 == &_max2);      \
    _max1 > _max2 ? _max1 : _max2; })

But wait... at the spots where MINNUM is used, we already know that this is true (because we returned at the top of this function if it wasn't):

len >= NO_DISK_INQUIRY_RESULT_LEN

Therefore, both of the MINNUM(len, NO_DISK_INQUIRY_RESULT_LEN) can be replaced with just NO_DISK_INQUIRY_RESULT_LEN.

[AlexanderCurtin]

Awesome, thanks for the feedback. I'll give your suggestions a go.

I wonder if I should just commit on top of the previous ones since I'll probably squash the entire function commit into one patch. I'm pretty sure greg would be alright with that, and for my own sanity it might be nice to not have to constantly revise history.

Dave, as a small suggestion should we have a tag for each driver? Then we can prioritize issues tagged as 'visorbus' or 'documentation'.

[AlexanderCurtin]

@selltc - Just a sort of a side note. Do you think the linux maintainers might be receptive to increasing the scsi related header files to include those sorts of structs? I'm sure we're not going to be the last people implementing new scsi device drivers, and it could certainly cut down extraneous code over time. My guess is that it would probably have to come from someone that they actually know and trust, and I'm sure they've thought of it before. It would definitely make our lives easier though.

[davidker]

We can try to push it into the Linux upstream, I'm guessing we would have to do some work to modify other drivers as well. Might be an interesting project and see how the maintainers react. I don't think they will be upset with us and I think they will either tell how to do it properly (it exists just under an obscured name) or it doesn't need to because they think disks are dead. But I don't think SCSI is dead so we might be good.

[selltc]

Do you think the linux maintainers might be receptive to increasing the scsi related header files to include those sorts of structs?

Well, it would be an improvement, but based on perusing thru the Linux scsi code, I can't say how receptive the maintainers would be to this sort of thing. The use of literals to access bytes within SCSI CDBs and result data seems common practice there.

As David put it, it would be "an interesting project".

[selltc]

Commit 4723e698ec7a19554f446c117e4786cbff78b25b is just a few minor tweaks to your latest patch, which I like.

[selltc]

Erik, I made some comments on your commit ab161a25d240fcbc924052349289bfd927c148d7.

[selltc]

Greg committed this to staging-testing on 5/6, then later to staging-next on 5/9:

• commit 9136d04fe9255522c4c8ef5124be899906de7dd1 - staging: unisys: visorhba: replace functionlike macro with function