FC_PORTSPEED semantics confusion and zfcp regression

FC_PORTSPEED semantics confusion and zfcp regression

[Steffen Maier]

Earlier, commit
1832a5862f2e1b4e5835611ee14bc30a8ed3cad5
"[SCSI] change port speed definitions for scsi_transport_fc"
changed the semantics of FC_PORTSPEED defines to match the Report Port 
Speed Capabilities (RPSC) ELS of FC-GS/FS-LS instead of FC-HBA/SM-HBA. 
It also mentions the problem that FC-HBA/SM-HBA and FC-GS/FC-LS contain 
somewhat contradicting bit definitions for port speed.

Lately, commit
a9277e7783651d4e0a849f7988340b1c1cf748a4
"[SCSI] scsi_transport_fc: Getting FC Port Speed in sync with FC-GS"
reverted this.
(See also http://marc.info/?l=linux-scsi&m=132892310322550&w=2.)

However, there does not seem to be an explicit explanation of this.
While the source code comment of struct fc_host_attrs says its fields 
adhere to HBAAPI 2.0, the comment of the FC_PORTSPEED defines does not 
explicitly do so and a (new) user of the latter might not see the 
relation to the structure fields, esp. since there is no obvious 
relation in terms of C types other than being type compatible to a 
generic int.
The commit message also does not say so but mentions SM-HBA along with 
FC-GS without stating that they contradict themselves and FS-GS even 
within the same document since it defines both RPSC ELS and FDMI port 
attributes.

1) Is this change of semantics deliberate?

The commit message of a9277e7 states that user space is not affected 
since it only sees text strings in the corresponding sysfs attributes.

Actually, commit c3d2350a8420dbf9d48f5f8a0fb72117bfcbc1b0
"[SCSI] fc_transport: update potential link speeds"
deliberately decided against syncing the bit definitions because of a 
potential binary-incompatibility.

Is there some use of the bit definition that is not as transparent as 
the text string based sysfs interface of an fc_host? E.g. using 
scsi_transport_fc.h in user space libraries or applications (if this is 
even allowed to be exported to user space)? I could find an in-kernel 
binary use in libfc/fc_lport.c which could alternatively do a conversion 
between the old RPSC ELS format of fc_host_{supported_}speed{s}() (and 
libfc/fc_lport.c which has probably the same format as fc_host) when 
building the FDMI blob in scsi/fc_encode.h:fc_ct_ms_fill()?

2) Why did commit a9277e7 change the kernel internal bit definitions?

The zfcp LLDD has been copying the supported speeds value directly from 
what is reported by the HBA since the latter adheres to the RPSC ELS 
format for line speeds. Since commit a9277e7 this is broken now and we 
see 10 Gbit instead of 4 Gbit in the supported_speeds sysfs attribute of 
the fc_host. I don't mind converting zfcp to an explicit finite bit 
conversion based on individually defined constants, esp. since all other 
FC LLDDs (qla2xxx, bfa, lpfc, ibmvscsi, fnic, mptfc) seem to do so 
already. The only slight drawback I can see is that the code needs to be 
touched for new HBA generations supporting new port speeds.

3) Should we have port speed defines for both RPSC ELS (in 
include/fc/fc_els.h?) and FDMI port attributes (in 
include/scsi/scsi_transport_fc.h? unless the bit semantic change gets 
reverted)?

This way I could reuse the former to convert to the latter within zfcp 
and other users can reuse it with regard to RPSC ELS. I would send a 
patch if we agree on this.

4) Either way, should the port speed defines indicate RPSC ELS vs. FDMI 
more explicitly in their identifier names, in order to avoid future 
confusion?

Regards,
Steffen

Linux on System z Development

IBM Deutschland Research & Development GmbH
Vorsitzende des Aufsichtsrats: Martina Koederitz
Geschäftsführung: Dirk Wittkopp
Sitz der Gesellschaft: Böblingen
Registergericht: Amtsgericht Stuttgart, HRB 243294

--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: FC_PORTSPEED semantics confusion and zfcp regression

[Parikh, Neerav]

> -----Original Message-----
> From: linux-scsi-owner@vger.kernel.org [mailto:linux-scsi-
> owner@vger.kernel.org] On Behalf Of Steffen Maier
> Sent: Thursday, August 09, 2012 9:15 AM
> To: linux-scsi@vger.kernel.org
> Cc: Parikh, Neerav; Brattain, Ross B; Love, Robert W; James Bottomley;
> Martin Peschke; Daniel Hansel
> Subject: FC_PORTSPEED semantics confusion and zfcp regression
> 
> Earlier, commit
> 1832a5862f2e1b4e5835611ee14bc30a8ed3cad5
> "[SCSI] change port speed definitions for scsi_transport_fc"
> changed the semantics of FC_PORTSPEED defines to match the Report Port
> Speed Capabilities (RPSC) ELS of FC-GS/FS-LS instead of FC-HBA/SM-HBA.
> It also mentions the problem that FC-HBA/SM-HBA and FC-GS/FC-LS contain
> somewhat contradicting bit definitions for port speed.
> 
> Lately, commit
> a9277e7783651d4e0a849f7988340b1c1cf748a4
> "[SCSI] scsi_transport_fc: Getting FC Port Speed in sync with FC-GS"
> reverted this.
> (See also http://marc.info/?l=linux-scsi&m=132892310322550&w=2.)
> 
> However, there does not seem to be an explicit explanation of this.
> While the source code comment of struct fc_host_attrs says its fields
> adhere to HBAAPI 2.0, the comment of the FC_PORTSPEED defines does not
> explicitly do so and a (new) user of the latter might not see the
> relation to the structure fields, esp. since there is no obvious
> relation in terms of C types other than being type compatible to a
> generic int.
> The commit message also does not say so but mentions SM-HBA along with
> FC-GS without stating that they contradict themselves and FS-GS even
> within the same document since it defines both RPSC ELS and FDMI port
> attributes.
> 
> 1) Is this change of semantics deliberate?
> 
> The commit message of a9277e7 states that user space is not affected
> since it only sees text strings in the corresponding sysfs attributes.
> 
> Actually, commit c3d2350a8420dbf9d48f5f8a0fb72117bfcbc1b0
> "[SCSI] fc_transport: update potential link speeds"
> deliberately decided against syncing the bit definitions because of a
> potential binary-incompatibility.
> 
> Is there some use of the bit definition that is not as transparent as
> the text string based sysfs interface of an fc_host? E.g. using
> scsi_transport_fc.h in user space libraries or applications (if this is
> even allowed to be exported to user space)? I could find an in-kernel
> binary use in libfc/fc_lport.c which could alternatively do a
> conversion
> between the old RPSC ELS format of fc_host_{supported_}speed{s}() (and
> libfc/fc_lport.c which has probably the same format as fc_host) when
> building the FDMI blob in scsi/fc_encode.h:fc_ct_ms_fill()?

While adding FC-GS FDMI support for the open FCoE stack (that uses libfc)
to register the port speed attributes(via FDMI) correctly either I would
need local libfc definitions that would translate the in kernel 
FC_PORTSPEED to specification values or get these defines in sync with 
the FC-GS/SM-HBA/HBAAPI spec level to not do these translations that other
drivers seem to be doing. I chose to do the former in this commit. 

One thing that I seem to miss is the RPSC ELS have some different definitions
for speed as compared to the FC_PORTSPEED before this commit as well. Does the 
zfcp HBA FW do some internal conversion resulting into speed values that 
were in sync with the prior FC_PORTSPEED values?

> 
> 2) Why did commit a9277e7 change the kernel internal bit definitions?
> 
> The zfcp LLDD has been copying the supported speeds value directly from
> what is reported by the HBA since the latter adheres to the RPSC ELS
> format for line speeds. Since commit a9277e7 this is broken now and we
> see 10 Gbit instead of 4 Gbit in the supported_speeds sysfs attribute
> of
> the fc_host. I don't mind converting zfcp to an explicit finite bit
> conversion based on individually defined constants, esp. since all
> other
> FC LLDDs (qla2xxx, bfa, lpfc, ibmvscsi, fnic, mptfc) seem to do so
> already. The only slight drawback I can see is that the code needs to
> be
> touched for new HBA generations supporting new port speeds.
This is the same problem I was trying to avoid while you were trying to reuse
the values reported by HBA to the OS; whereas I was copying the speed values 
set for the libfc fc_lport instance to FDMI speed related attributes as is. 
Copying them as is was reporting incorrect speeds on the FC switch side FDMI 
database. 

> 
> 3) Should we have port speed defines for both RPSC ELS (in
> include/fc/fc_els.h?) and FDMI port attributes (in
> include/scsi/scsi_transport_fc.h? unless the bit semantic change gets
> reverted)?
> 
> This way I could reuse the former to convert to the latter within zfcp
> and other users can reuse it with regard to RPSC ELS. I would send a
> patch if we agree on this.
I'm okay with either of the approach and if you choose the former (revert)
then I'll make necessary changes in libfc part to convert values internally
there.
Also, regardless of the approach you decide on wouldn't zfcp need to make
sure the speed values reported by HBA FW is in sync with the in kernel 
values?

> 
> 4) Either way, should the port speed defines indicate RPSC ELS vs. FDMI
> more explicitly in their identifier names, in order to avoid future
> confusion?
If you chose to revert the a9277e7 then perhaps you don't need multiple 
speed definitions. 

> 
> Regards,
> Steffen
> 
> Linux on System z Development
> 
> IBM Deutschland Research & Development GmbH
> Vorsitzende des Aufsichtsrats: Martina Koederitz
> Geschäftsführung: Dirk Wittkopp
> Sitz der Gesellschaft: Böblingen
> Registergericht: Amtsgericht Stuttgart, HRB 243294
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-scsi"
> in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: FC_PORTSPEED semantics confusion and zfcp regression

[Steffen Maier]

Hi Neerav,

thanks very much for your quick reply!

On 08/09/2012 09:29 PM, Parikh, Neerav wrote:
>> -----Original Message-----
>> From: linux-scsi-owner@vger.kernel.org [mailto:linux-scsi-
>> owner@vger.kernel.org] On Behalf Of Steffen Maier
>> Sent: Thursday, August 09, 2012 9:15 AM

>> Lately, commit
>> a9277e7783651d4e0a849f7988340b1c1cf748a4
>> "[SCSI] scsi_transport_fc: Getting FC Port Speed in sync with FC-GS"
>> reverted this.
>> (See also http://marc.info/?l=linux-scsi&m=132892310322550&w=2.)

> One thing that I seem to miss is the RPSC ELS have some different definitions
> for speed as compared to the FC_PORTSPEED before this commit as well. Does the
> zfcp HBA FW do some internal conversion resulting into speed values that
> were in sync with the prior FC_PORTSPEED values?

Assuming you refer to the reversed bit order, then this is handled by 
the HBA and therefore matches the prior FC_PORTSPEED values.

kernel before a9277e7                FC-GS RPSC ELS Sec.6.2.3.3.8+9
                                      Bit  Description
------------------------------------|------------------------------
#define FC_PORTSPEED_UNKNOWN      0  -
#define FC_PORTSPEED_1GBIT        1  15    1 Gb/s capable
#define FC_PORTSPEED_2GBIT        2  14    2 Gb/s capable
#define FC_PORTSPEED_4GBIT        4  13    4 Gb/s capable
#define FC_PORTSPEED_10GBIT       8  12   10 Gb/s capable
#define FC_PORTSPEED_8GBIT     0x10  11    8 Gb/s capable
#define FC_PORTSPEED_16GBIT    0x20  10   16 Gb/s capable
                                       9   20 Gb/s capable
                                       8   32 Gb/s capable
                                       7   40 Gb/s capable
                                      6-2  Reserved
                                       1   Unknown
#define FC_PORTSPEED_NOT_NEGOTIATED
                           (1 << 15)   0   Speed not established

>> I don't mind converting zfcp to an explicit finite bit
>> conversion based on individually defined constants, esp. since all
>> other
>> FC LLDDs (qla2xxx, bfa, lpfc, ibmvscsi, fnic, mptfc) seem to do so
>> already. The only slight drawback I can see is that the code needs to
>> be touched for new HBA generations supporting new port speeds.

I think, I'm going to bring zfcp in line with the other LLDs and do an 
explicit conversion for the following reasons:
Independence of any current kernel port speed semantics.
I have to do service for enterprise distros anyway (even if you revert 
your commit upstream) since some seem to have to picked up a9277e7 already.

>> 3) Should we have port speed defines for both RPSC ELS (in
>> include/fc/fc_els.h?) and FDMI port attributes (in
>> include/scsi/scsi_transport_fc.h? unless the bit semantic change gets
>> reverted)?
>>
>> This way I could reuse the former to convert to the latter within zfcp
>> and other users can reuse it with regard to RPSC ELS. I would send a
>> patch if we agree on this.

I guess, there is no other user currently so I might just code it inside 
zfcp without definitions in fc_els.h unless someone wants me to.

> Also, regardless of the approach you decide on wouldn't zfcp need to make
> sure the speed values reported by HBA FW is in sync with the in kernel
> values?

Not necessarily, because we don't synchronize the bit semantics between 
HBA and kernel, but between HBA and FC-GS RPSC ELS in reversed bit 
order. If the kernel would also remain using the semantics of FC-GS RPSC 
ELS in reversed bit order, then the HBA values would automatically match 
the kernel definitions.
However, that's no longer relevant with an explicit conversion in zfcp.

Regards,
Steffen

Linux on System z Development

IBM Deutschland Research & Development GmbH
Vorsitzende des Aufsichtsrats: Martina Koederitz
Geschäftsführung: Dirk Wittkopp
Sitz der Gesellschaft: Böblingen
Registergericht: Amtsgericht Stuttgart, HRB 243294

--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html