On Sun, 2011-10-30 at 11:26 -0500, Madhuranath N Iyengar wrote:
> From: Madhuranath Iyengar <mni@xxxxxxxxxxxxxxx>
>
> Also, removed the atio_t struct in qla_def.h.
>
> Signed-off-by: Madhuranath Iyengar <mni@xxxxxxxxxxxxxxx>
> ---
> drivers/scsi/qla2xxx/qla_def.h | 15 +-
> drivers/scsi/qla2xxx/qla_target.c | 314 +++++++++++------------
> drivers/scsi/qla2xxx/qla_target.h | 84 ++++---
> drivers/target/tcm_qla2xxx/tcm_qla2xxx_fabric.c | 12 +-
> 4 files changed, 202 insertions(+), 223 deletions(-)
>
Hi Madhu,
I ran into a couple of issues while testing this change.. My comments
are inline.
> diff --git a/drivers/scsi/qla2xxx/qla_def.h b/drivers/scsi/qla2xxx/qla_def.h
> index ae6dfd0..f1ed22b 100644
> --- a/drivers/scsi/qla2xxx/qla_def.h
> +++ b/drivers/scsi/qla2xxx/qla_def.h
> @@ -1231,17 +1231,6 @@ typedef struct {
> #define RESPONSE_PROCESSED 0xDEADDEAD /* Signature */
> } response_t;
>
> -/*
> - * ISP queue - ATIO queue entry definition.
> - */
> -typedef struct {
> - uint8_t entry_type; /* Entry type. */
> - uint8_t entry_count; /* Entry count. */
> - uint8_t data[58];
> - uint32_t signature;
> -#define ATIO_PROCESSED 0xDEADDEAD /* Signature */
> -} atio_t;
> -
> typedef union {
> uint16_t extended;
> struct {
> @@ -2851,8 +2840,8 @@ struct qla_hw_data {
> uint32_t node_name_set:1;
>
> dma_addr_t atio_dma; /* Physical address. */
> - atio_t *atio_ring; /* Base virtual address */
> - atio_t *atio_ring_ptr; /* Current address. */
> + void *atio_ring; /* Base virtual address */
> + void *atio_ring_ptr; /* Current address. */
> uint16_t atio_ring_index; /* Current index. */
> uint16_t atio_q_length;
>
> diff --git a/drivers/scsi/qla2xxx/qla_target.c b/drivers/scsi/qla2xxx/qla_target.c
> index 8d419d9..e48f88e 100644
> --- a/drivers/scsi/qla2xxx/qla_target.c
> +++ b/drivers/scsi/qla2xxx/qla_target.c
<SNIP>
> @@ -5098,17 +5088,17 @@ qla_tgt_24xx_process_atio_queue(struct scsi_qla_host *vha)
> {
> struct qla_hw_data *ha = vha->hw;
> struct device_reg_24xx __iomem *reg = &ha->iobase->isp24;
> - atio_t *pkt;
> + atio_from_isp_t *pkt;
> int cnt, i;
>
> if (!vha->flags.online)
> return;
>
> - while (ha->atio_ring_ptr->signature != ATIO_PROCESSED) {
> - pkt = ha->atio_ring_ptr;
> + pkt = ha->atio_ring_ptr;
> + while (pkt->u.raw.signature != ATIO_PROCESSED) {
> cnt = pkt->entry_count;
>
So the change here to not loop on ha->atio_ring_ptr->signature is
problematic, and ends producing bogus ATIO packet entry_types in
qla_tgt_24xx_atio_pkt_all_vps()..
I played around with this a bit, but in the end i've had to reinstate
the usage of atio_t in order to get things functioning again..
> - qla_tgt_24xx_atio_pkt_all_vps(vha, (atio7_from_24xx_t *)pkt);
> + qla_tgt_24xx_atio_pkt_all_vps(vha, (atio_from_isp_t *)pkt);
>
> for (i = 0; i < cnt; i++) {
> ha->atio_ring_index++;
> @@ -5118,7 +5108,7 @@ qla_tgt_24xx_process_atio_queue(struct scsi_qla_host *vha)
> } else
> ha->atio_ring_ptr++;
>
> - pkt->signature = ATIO_PROCESSED;
> + pkt->u.raw.signature = ATIO_PROCESSED;
> pkt = ha->atio_ring_ptr;
> }
> wmb();
> diff --git a/drivers/scsi/qla2xxx/qla_target.h b/drivers/scsi/qla2xxx/qla_target.h
> index 3e4d284..d585751 100644
> --- a/drivers/scsi/qla2xxx/qla_target.h
> +++ b/drivers/scsi/qla2xxx/qla_target.h
<SNIP>
> /*
> - * ISP queue - Accept Target I/O (ATIO) type 7 entry for 24xx structure.
> - * This is sent from ISP 24xx to the target driver.
> + * ISP queue - Accept Target I/O (ATIO) type entry IOCB structure.
> + * This is sent from the ISP to the target driver.
> */
> typedef struct {
> uint8_t entry_type; /* Entry type. */
> uint8_t entry_count; /* Entry count. */
> - uint8_t fcp_cmnd_len_low;
> - uint8_t fcp_cmnd_len_high:4;
> - uint8_t attr:4;
> - uint32_t exchange_addr;
> -#define ATIO_EXCHANGE_ADDRESS_UNKNOWN 0xFFFFFFFF
> - fcp_hdr_t fcp_hdr;
> - atio7_fcp_cmnd_t fcp_cmnd;
> -} __attribute__((packed)) atio7_from_24xx_t;
> + union {
> + struct {
> + uint8_t sys_define; /* System defined. */
> + uint8_t entry_status; /* Entry Status. */
> + uint32_t sys_define_2; /* System defined. */
> + target_id_t target;
> + uint16_t rx_id;
> + uint16_t flags;
> + uint16_t status;
> + uint8_t command_ref;
> + uint8_t task_codes;
> + uint8_t task_flags;
> + uint8_t execution_codes;
> + uint8_t cdb[MAX_CMDSZ];
> + uint32_t data_length;
> + uint16_t lun;
> + uint8_t initiator_port_name[WWN_SIZE]; /* on qla23xx */
> + uint16_t reserved_32[6];
> + uint16_t ox_id;
> + } isp2x;
> + struct {
> + uint8_t fcp_cmnd_len_low;
> + uint8_t fcp_cmnd_len_high:4;
> + uint8_t attr:4;
> + uint32_t exchange_addr;
> +#define ATIO_EXCHANGE_ADDRESS_UNKNOWN 0xFFFFFFFF
> + fcp_hdr_t fcp_hdr;
> + atio7_fcp_cmnd_t fcp_cmnd;
> + } isp24;
> + struct {
> + uint8_t data[58];
> + uint32_t signature;
> +#define ATIO_PROCESSED 0xDEADDEAD /* Signature */
> + } raw;
> + } u;
> +} __attribute__((packed)) atio_from_isp_t;
>
Also, the new atio_from_isp_t is 66 bytes here, instead of the expected
64 bytes originally for atio7_from_24xx_t, et al.. I've included
entry_type and entry_count into the union to address this.
Sending out a patch shortly to fix-up these two areas..
Thanks,
--nab
--
To unsubscribe from this list: send the line "unsubscribe target-devel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
