[CARE]: number of LUNS from REPORT LUNS data report and macros

[CARE]: number of LUNS from REPORT LUNS data report and macros

[Luben Tuikov]

Two issues here.

Firstly, I don't think that one can quite do
``num_luns = (length / sizeof(ScsiLun));'' in scsi_scan.c.

The reason is that we should keep clear separation of what/how
the kernel represents some standard/drafts data and how it comes
from the device. I.e. one wouldn't assume (one step further) that
_also_ the byte ordering is the same in the report data and in ScsiLun,
i.e.  (ScsiLun *)(data+i*sizeof(ScsiLun)) ... not!

A SCSI LUN should be represented by ``u64''. Futhermore, only
a LLDD should poplulate such a variable (as outlined in another email)
directly or indirectly (though REPORT LUNS data).

Secondly, please get rid of the big
#ifdef CONFIG_SCSI_REPORT_LUNS in scsi_scan.c.
Linus has made this a point numerous times.

Lets keep this great subsystem great!

(Those issues should naturally go away once u64 LUN is adopted.)

-- 
Luben

Re: [CARE]: number of LUNS from REPORT LUNS data report and macros

[Patrick Mansfield]

Luben -

On Mon, Aug 26, 2002 at 08:49:38PM -0400, Luben Tuikov wrote:
> Two issues here.

Thanks for the feedback.

> 
> Firstly, I don't think that one can quite do
> ``num_luns = (length / sizeof(ScsiLun));'' in scsi_scan.c.

length is the length returned from the REPORT LUNS, and sizeof(ScsiLun)
is the size of an element in the REPORT LUNS. I don't see any better
way to get the number of luns.

> The reason is that we should keep clear separation of what/how
> the kernel represents some standard/drafts data and how it comes
> from the device. I.e. one wouldn't assume (one step further) that
> _also_ the byte ordering is the same in the report data and in ScsiLun,
> i.e.  (ScsiLun *)(data+i*sizeof(ScsiLun)) ... not!

I don't quite follow the above, is there some line of code in scsi_scan.c
you can reference? Are you talking about the horrific scsilun_to_int?
Yes, it's ugly but as long as we have to make a four byte int from
an 8 byte not-even-close to an integer value, we need something like it
around.

> A SCSI LUN should be represented by ``u64''. Futhermore, only
> a LLDD should poplulate such a variable (as outlined in another email)
> directly or indirectly (though REPORT LUNS data).

IMO all the SCSI data is best represented as u8 chunks, but having a
ScsiLun type (even with the butchered case) makes this simple to change.
I see there are a bunch of u_char's, I'm not even sure if u8 or u_char
is right.

> Secondly, please get rid of the big
> #ifdef CONFIG_SCSI_REPORT_LUNS in scsi_scan.c.
> Linus has made this a point numerous times.

Yes, but it is a static function, so to get rid of the ifdefs here
I'd have to move code to a .h or something; I did do an inline
on the #else portion, so there is no #ifdef in the caller.

A patch (or two) might better explain specifics of what you want.

-- Patrick

Re: [CARE]: number of LUNS from REPORT LUNS data report and macros

[Luben Tuikov]

Patrick Mansfield wrote:
> 
> >
> > Firstly, I don't think that one can quite do
> > ``num_luns = (length / sizeof(ScsiLun));'' in scsi_scan.c.
> 
> length is the length returned from the REPORT LUNS, and sizeof(ScsiLun)
> is the size of an element in the REPORT LUNS. I don't see any better
> way to get the number of luns.

This is fine. My objection is ScsiLun as a type. Certainly looks
more like a big struct, e.g. Scsi_Device <=> pid_t, do you see
what I'm hinting at?

Someone may decide to add something to ScsiLun and
then surprize, surprize.

I don't know what will be a good way of representing
64 bit LUN, u64 seems attractive, but so does u8[8].

(We also have to take into account the fact than one day
this will be exported to userspace. What does Sun do?)

> > A SCSI LUN should be represented by ``u64''. Futhermore, only
> > a LLDD should poplulate such a variable (as outlined in another email)
> > directly or indirectly (though REPORT LUNS data).
> 
> IMO all the SCSI data is best represented as u8 chunks, but having a
> ScsiLun type (even with the butchered case) makes this simple to change.
> I see there are a bunch of u_char's, I'm not even sure if u8 or u_char
> is right.

Use u8 (if _that_ is what you are going to use).

Yes, most data is ok being represented as u8 [], am I'm not quite so
sure about a 64 bit LUN. Probably a u64, and when in a register (YKWIM),
it is ordered as per the spec -- what'd be kinda easier for userspace
users.
 
> Yes, but it is a static function, so to get rid of the ifdefs here
> I'd have to move code to a .h or something; I did do an inline
> on the #else portion, so there is no #ifdef in the caller.

Just _declare_ it as static -- that should do for the linker...

-- 
Luben