Re: [PATCH v3 2/2][BNX2]: Add iSCSI support to BNX2 devices.

[Mike Christie <michaelc@cs.wisc.edu>]

Anil Veerabhadrappa wrote:
>>
>>> +
>>> +/* iSCSI stages */
>>> +#define ISCSI_STAGE_SECURITY_NEGOTIATION (0)
>>> +#define ISCSI_STAGE_LOGIN_OPERATIONAL_NEGOTIATION (1)
>>> +#define ISCSI_STAGE_FULL_FEATURE_PHASE (3)
>>> +/* Logout response codes */
>>> +#define ISCSI_LOGOUT_RESPONSE_CONNECTION_CLOSED (0)
>>> +#define ISCSI_LOGOUT_RESPONSE_CID_NOT_FOUND (1)
>>> +#define ISCSI_LOGOUT_RESPONSE_CLEANUP_FAILED (3)
>>> +
>>> +/* iSCSI task types */
>>> +#define ISCSI_TASK_TYPE_READ    (0)
>>> +#define ISCSI_TASK_TYPE_WRITE   (1)
>>> +#define ISCSI_TASK_TYPE_MPATH   (2)
>>
>>
>>
>> All of these iscsi code shoulds be in iscsi_proto.h or should be added 
>> there.
> This is a very tricky proposal as this header file is automatically
> generated by a well defined process and is shared between various driver
> supporting multiple platform/OS and the firmware. If it is not of a big
> issue I would like to keep it the way it is.

The values that are iscsi RFC values should come from the iscsi_proto.h 
file and not be duplicated for each driver.


>>> +/*
>>> + * hardware reset
>>> + */
>>> +int bnx2i_reset(struct scsi_cmnd *sc)
>>> +{
>>> +	return 0;
>>> +}
>>
>> So what is up with this one? It seems like if there is a way to reset 
>> hardware then you would want it as the scsi eh host reset callout 
>> instead of dropping the session. We could add some transport level 
>> recovery callouts for the iscsi specifics.
> 
> We may not be able to support HBA cold reset as bnx2 driver is the
> primary owner of chip reset and initialization. This is the drawback of
> sharing network interface with the NIC driver. If there is a need for
> administrator to reset the iSCSI port same can be achieved by running
> 'ifdown eth#' and 'ifup eth#'.
> Current driver even allows ethernet interface reset when there are
> active iSCSI connection, all active iscsi sessions will be reinstated
> when the network link comes back live
>  
> 

If you cannot support it or it does not make sense just remove the stub 
then. I say it is not a big deal now, but hopefully we do not hit fun 
like with qla3xxx and qla4xxx :)

>>> +
>>> +void bnx2i_sysfs_cleanup(void)
>>> +{
>>> +	class_device_unregister(&port_class_dev);
>>> +	class_unregister(&bnx2i_class);
>>> +}
>> The sysfs bits related to the hba should be use one of the scsi sysfs 
>> facilities or if they are related to iscsi bits and are generic then 
>> through the iscsi hba
> 
> bnx2i needs 2 sysfs entries -
> 1. QP size info - this is used to size per connection shared data
> structures to issue work requests to chip (login, scsi cmd, tmf, nopin)
> and get completions from the chip (scsi completions, async messages,
> etc'). This is a iSCSI HBA attribute
> 2. port mapper - we can be more flexible on classifying this as either
> iSCSI HBA attribute or bnx2i driver global attribute
> Can hooks be added to iSCSI transport class to include these?
> 

Which ones were they exactly? I think JamesB wanted only common 
transport values in the transport class. If it is driver specific then 
it should go on the host or target or device with the scsi_host_template 
attrs.