IB/srp: Implement SRP_CRED_REQ and SRP_AER_REQ
Subsystems (3)
PATCH v5 2/3 IB/srp: Implement SRP_CRED_REQ and SRP_AER_REQ
Implements SRP_CRED_REQ and SRP_AER_REQ, which are information units defined in the SRP (draft) standard. Adds declarations for the SRP_CRED_REQ, SRP_CRED_RSP, SRP_AER_REQ and SRP_AER_RSP information units to include/scsi/srp.h. Changes function definition order in ib_srp in order to avoid having to add more forward declarations.
--- drivers/infiniband/ulp/srp/ib_srp.c | 244 ++++++++++++++++++++++++++--------- drivers/infiniband/ulp/srp/ib_srp.h | 7 +- include/scsi/srp.h | 38 ++++++ 3 files changed, 225 insertions(+), 64 deletions(-)
drivers/infiniband/ulp/srp/ib_srp.c +183 -61
@@ -896,6 +896,180 @@ static void srp_process_rsp(struct srp_target_port *target, struct srp_rsp *rsp)spin_unlock_irqrestore (target->scsi_host->host_lock, flags); } +/* +* Must be called with target->scsi_host->host_lock held to protect +* tx_head. Lock cannot be dropped between call here and call to +* __srp_post_send_iu(). +* +* Note: +* An upper limit for the number of allocated information units for each +* request type is: +* - SRP_TX_IU_REQ_NORMAL: SRP_NORMAL_REQ_SQ_SIZE, since the SCSI mid-layer +* never queues more than Scsi_Host.can_queue requests. +* - SRP_TX_IU_REQ_TASK_MGMT: SRP_TASK_MGMT_SQ_SIZE. +* - SRP_TX_IU_RSP: 1, since a conforming SRP target never sends more than +* one unanswered SRP request to an initiator. +*/ +static struct srp_iu *__srp_get_tx_iu (struct srp_target_port *target, +enum srp_tx_iu_type tx_iu_type) +{ +s32 rsv; + +srp_send_completion (target->send_cq, target); + + rsv = (tx_iu_type ==SRP_TX_IU_REQ_TASK_MGMT ) ?0 :SRP_TASK_MGMT_SQ_SIZE ; + +if (target->tx_head - target->tx_tail >=SRP_SQ_SIZE ) +return NULL ; + +if (tx_iu_type !=SRP_TX_IU_RSP && target->req_lim <= rsv) { + ++target->zero_req_lim; +return NULL ; + } + +return target->tx_ring[target->tx_head &SRP_SQ_MASK ]; +} + +/* +* Must be called with target->scsi_host->host_lock held to protect tx_head. +*/ +static int __srp_post_send_iu (struct srp_target_port *target, +struct srp_iu *iu,int len) +{ +struct ib_sge list; +struct ib_send_wr wr, *bad_wr; +int ret =0 ; + + list.addr = iu->dma; + list.length = len; + list.lkey = target->srp_host->srp_dev->mr->lkey; + + wr.next =NULL ; + wr.wr_id = target->tx_head &SRP_SQ_MASK ; + wr.sg_list = &list; + wr.num_sge =1 ; + wr.opcode =IB_WR_SEND ; + wr.send_flags =IB_SEND_SIGNALED ; + + ret =ib_post_send (target->qp, &wr, &bad_wr); + +if (!ret) + ++target->tx_head; + +return ret; +} + +/* +* Must be called with target->scsi_host->host_lock held to protect req_lim. +*/ +static int __srp_post_send_req (struct srp_target_port *target, +struct srp_iu *iu,int len) +{ +int ret; + + ret =__srp_post_send_iu (target, iu, len); +if (ret ==0 ) + --target->req_lim; +return ret; +} + +/* +* Must be called with target->scsi_host->host_lock held. +*/ +static int __srp_post_send_rsp (struct srp_target_port *target, +struct srp_iu *iu,int len) +{ +return __srp_post_send_iu (target, iu, len); +} + +/* +* Must be called with target->scsi_host->host_lock locked to protect +* target->req_lim. +*/ +static int srp_handle_cred_req (struct srp_target_port *target, +struct srp_cred_req *req, +struct srp_cred_rsp *rsp) +{ + target->req_lim +=be32_to_cpu (req->req_lim_delta); + +memset (rsp,0 ,sizeof *rsp); + rsp->opcode =SRP_CRED_RSP ; + rsp->tag = req->tag; + +return 0 ; +} + +/* +* Must be called with target->scsi_host->host_lock locked to protect +* target->req_lim. +*/ +static int srp_handle_aer_req (struct srp_target_port *target, +struct srp_aer_req *req, +struct srp_aer_rsp *rsp) +{ + target->req_lim +=be32_to_cpu (req->req_lim_delta); + +shost_printk (KERN_ERR , target->scsi_host, +PFX "ignoring AER for LUN %llu\n" ,be64_to_cpu (req->lun)); + +memset (rsp,0 ,sizeof *rsp); + rsp->opcode =SRP_AER_RSP ; + rsp->tag = req->tag; + +return 0 ; +} + +static void srp_handle_req (struct srp_target_port *target, +struct srp_iu *req_iu) +{ +struct ib_device *dev; +u8 *req_buf; +unsigned long flags; +struct srp_iu *rsp_iu; +u8 *rsp_buf; +int res; + + dev = target->srp_host->srp_dev->dev; + req_buf = req_iu->buf; + +spin_lock_irqsave (target->scsi_host->host_lock, flags); + + rsp_iu =__srp_get_tx_iu (target,SRP_TX_IU_RSP ); +if (!rsp_iu) + goto out_unlock; + + rsp_buf = rsp_iu->buf; + + res = -EINVAL ; + +switch (req_buf[0 ]) { +case SRP_CRED_REQ : + res =srp_handle_cred_req (target, + (struct srp_cred_req *)req_buf, + (struct srp_cred_rsp *)rsp_buf); +break ; +case SRP_AER_REQ : + res =srp_handle_aer_req (target, + (struct srp_aer_req *)req_buf, + (struct srp_aer_rsp *)rsp_buf); +break ; + } + +if (res) + goto out_unlock; + +ib_dma_sync_single_for_device (dev, rsp_iu->dma, srp_max_iu_len, +DMA_TO_DEVICE ); + + res =__srp_post_send_rsp (target, rsp_iu,sizeof *rsp_iu); +if (res) +shost_printk (KERN_ERR , target->scsi_host, +PFX "Sending response failed -- res = %d\n" , res); + +out_unlock: +spin_unlock_irqrestore (target->scsi_host->host_lock, flags); +} +static void srp_handle_recv (struct srp_target_port *target,struct ib_wc *wc) {struct ib_device *dev;
@@ -929,6 +1103,11 @@ static void srp_handle_recv(struct srp_target_port *target, struct ib_wc *wc) PFX "Got target logout request\n");break ; +case SRP_CRED_REQ : +case SRP_AER_REQ : +srp_handle_req (target, iu); +break ; +default :shost_printk (KERN_WARNING , target->scsi_host,PFX "Unhandled SRP opcode 0x%02x\n" , opcode);
@@ -981,63 +1160,6 @@ static void srp_send_completion(struct ib_cq *cq, void *target_ptr) } } -/* -* Must be called with target->scsi_host->host_lock held to protect -* req_lim and tx_head. Lock cannot be dropped between call here and -* call to __srp_post_send(). -*/ -static struct srp_iu *__srp_get_tx_iu (struct srp_target_port *target, -enum srp_request_type req_type) -{ -s32 rsv; - - rsv = (req_type ==SRP_REQ_TASK_MGMT ) ?0 :SRP_TASK_MGMT_SQ_SIZE ; - -srp_send_completion (target->send_cq, target); - -if (target->tx_head - target->tx_tail >=SRP_SQ_SIZE ) -return NULL ; - -if (target->req_lim <= rsv) { - ++target->zero_req_lim; -return NULL ; - } - -return target->tx_ring[target->tx_head &SRP_SQ_MASK ]; -} - -/* -* Must be called with target->scsi_host->host_lock held to protect -* req_lim and tx_head. -*/ -static int __srp_post_send (struct srp_target_port *target, -struct srp_iu *iu,int len) -{ -struct ib_sge list; -struct ib_send_wr wr, *bad_wr; -int ret =0 ; - - list.addr = iu->dma; - list.length = len; - list.lkey = target->srp_host->srp_dev->mr->lkey; - - wr.next =NULL ; - wr.wr_id = target->tx_head &SRP_SQ_MASK ; - wr.sg_list = &list; - wr.num_sge =1 ; - wr.opcode =IB_WR_SEND ; - wr.send_flags =IB_SEND_SIGNALED ; - - ret =ib_post_send (target->qp, &wr, &bad_wr); - -if (!ret) { - ++target->tx_head; - --target->req_lim; - } - -return ret; -} -static int srp_queuecommand (struct scsi_cmnd *scmnd,void (*done)(struct scsi_cmnd *)) {
@@ -1058,7 +1180,7 @@ static int srp_queuecommand(struct scsi_cmnd *scmnd,return 0 ; } - iu =__srp_get_tx_iu (target,SRP_REQ_NORMAL ); + iu =__srp_get_tx_iu (target,SRP_TX_IU_REQ_NORMAL );if (!iu) goto err;
@@ -1095,7 +1217,7 @@ static int srp_queuecommand(struct scsi_cmnd *scmnd,ib_dma_sync_single_for_device (dev, iu->dma, srp_max_iu_len,DMA_TO_DEVICE ); -if (__srp_post_send (target, iu, len)) { +if (__srp_post_send_req (target, iu, len)) {shost_printk (KERN_ERR , target->scsi_host,PFX "Send failed\n" ); goto err_unmap; }
@@ -1365,7 +1487,7 @@ static int srp_send_tsk_mgmt(struct srp_target_port *target,init_completion (&req->done); - iu =__srp_get_tx_iu (target,SRP_REQ_TASK_MGMT ); + iu =__srp_get_tx_iu (target,SRP_TX_IU_REQ_TASK_MGMT );if (!iu) goto out;
@@ -1378,7 +1500,7 @@ static int srp_send_tsk_mgmt(struct srp_target_port *target, tsk_mgmt->tsk_mgmt_func = func; tsk_mgmt->task_tag = req->index; -if (__srp_post_send (target, iu,sizeof *tsk_mgmt)) +if (__srp_post_send_req (target, iu,sizeof *tsk_mgmt)) goto out; req->tsk_mgmt = iu;
drivers/infiniband/ulp/srp/ib_srp.h +4 -3
@@ -82,9 +82,10 @@ enum srp_target_state {SRP_TARGET_REMOVED }; -enum srp_request_type { -SRP_REQ_NORMAL , -SRP_REQ_TASK_MGMT , +enum srp_tx_iu_type { +SRP_TX_IU_REQ_NORMAL , +SRP_TX_IU_REQ_TASK_MGMT , +SRP_TX_IU_RSP , };struct srp_device {
include/scsi/srp.h +38 -0
@@ -239,4 +239,42 @@ struct srp_rsp {u8 data[0 ]; } __attribute__((packed)); +struct srp_cred_req { +u8 opcode; +u8 sol_not; +u8 reserved[2 ]; +__be32 req_lim_delta; +u64 tag; +}; + +struct srp_cred_rsp { +u8 opcode; +u8 reserved[7 ]; +u64 tag; +}; + +/* +* The SRP spec defines the fixed portion of the AER_REQ structure to be +* 36 bytes, so it needs to be packed to avoid having it padded to 40 bytes +* on 64-bit architectures. +*/ +struct srp_aer_req { +u8 opcode; +u8 sol_not; +u8 reserved[2 ]; +__be32 req_lim_delta; +u64 tag; +u32 reserved2; +__be64 lun; +__be32 sense_data_len; +u32 reserved3; +u8 sense_data[0 ]; +} __attribute__((packed)); + +struct srp_aer_rsp { +u8 opcode; +u8 reserved[7 ]; +u64 tag; +}; +#endif /* SCSI_SRP_H */
Signature · 6 lines
1.6.4.2 -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
A message that is not in the archive (sent off-list or not yet received): [email protected]
Quoted 5 lines · Bart Van Assche
Implements
SRP_CRED_REQandSRP_AER_REQ, which are information units defined in the SRP (draft) standard. Adds declarations for theSRP_CRED_REQ,SRP_CRED_RSP,SRP_AER_REQandSRP_AER_RSPinformation units toinclude/scsi/srp.h. Changes function definition order inib_srpin order to avoid having to add more forward declarations.
I still don't like the style of this patch -- too much code duplication and the SRP_TX_IU_* constants are too ugly to live. I'd also prefer to put the code movement in a separate patch, so in this case I'd accept the forward declaration and we can do a separate patch later to move the code around in one swoop. I think there's quite a bit that can potentially be cleaned up there, and I don't expect you to do it as part of this series.
In that light, do you have any technical comment on the version I posted? It is my preference to go with that version.
Dave
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
A message that is not in the archive (sent off-list or not yet received): 1282178932.25848.27.camel-1q1vX8mYZiGLUyTwlgNVppKKF0rrzTr+@public.gmane.org
Quoted 7 lines · David Dillow
Quoted 5 lines · Bart Van Assche
Implements
SRP_CRED_REQandSRP_AER_REQ, which are information units defined in the SRP (draft) standard. Adds declarations for theSRP_CRED_REQ,SRP_CRED_RSP,SRP_AER_REQandSRP_AER_RSPinformation units toinclude/scsi/srp.h. Changes function definition order inib_srpin order to avoid having to add more forward declarations.
Quoted 7 lines
I still don't like the style of this patch -- too much code duplication and the SRP_TX_IU_* constants are too ugly to live. I'd also prefer to put the code movement in a separate patch, so in this case I'd accept the forward declaration and we can do a separate patch later to move the code around in one swoop. I think there's quite a bit that can potentially be cleaned up there, and I don't expect you to do it as part of this series.
I will put the code movement in a separate patch.
Regarding code duplication: are you referring to the duplicated test of the SRP response opcode ? If I do not receive any further feedback, I will modify srp_handle_req() such that it accepts a third argument, a function pointer, and will change the call sites of that function such that either a pointer to the srp_handle_cred_req function or a pointer to the srp_handle_aer_req function is passed.
Bart.
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
A message that is not in the archive (sent off-list or not yet received): AANLkTimhOGgMr4SXzz4-pKXVyfCUjtrO+Avi=FGCacDW-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org
Quoted 16 lines · Bart Van Assche
Quoted 7 lines · David Dillow
Quoted 5 lines · Bart Van Assche
Implements
SRP_CRED_REQandSRP_AER_REQ, which are information units defined in the SRP (draft) standard. Adds declarations for theSRP_CRED_REQ,SRP_CRED_RSP,SRP_AER_REQandSRP_AER_RSPinformation units toinclude/scsi/srp.h. Changes function definition order inib_srpin order to avoid having to add more forward declarations.Quoted 7 lines
I still don't like the style of this patch -- too much code duplication and the SRP_TX_IU_* constants are too ugly to live. I'd also prefer to put the code movement in a separate patch, so in this case I'd accept the forward declaration and we can do a separate patch later to move the code around in one swoop. I think there's quite a bit that can potentially be cleaned up there, and I don't expect you to do it as part of this series.
Quoted 4 lines
I will put the code movement in a separate patch.
Regarding code duplication: are you referring to the duplicated test of the SRP response opcode ?
There's less duplication than your first go at this, but I don't like the __srp_post_send_req/__srp_post_send_rsp split and the way you've broken out the processing.
Again, it comes down to style and the shape of the code.
Quoted 5 lines
If I do not receive any further feedback,
I will modifysrp_handle_req()such that it accepts a third argument, a function pointer, and will change the call sites of that function such that either a pointer to thesrp_handle_cred_reqfunction or a pointer to thesrp_handle_aer_reqfunction is passed.
There's no need to do that; I don't think it will make it look better. I've given you a clean, simple way to get there. What objections did you have to the code I posted?
Dave
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
A message that is not in the archive (sent off-list or not yet received): 1282215126.25848.42.camel-1q1vX8mYZiGLUyTwlgNVppKKF0rrzTr+@public.gmane.org
Quoted 10 lines · David Dillow
Quoted 7 lines · Bart Van Assche
On Thu, Aug 19, 2010 at 2:48 AM, David Dillow <dave-i1Mk8JYDVaaSihdK6806/[email protected]> wrote:
[ ... ]
If I do not receive any further feedback,
I will modifysrp_handle_req()such that it accepts a third argument,
a function pointer, and will change the call sites of that function
such that either a pointer to thesrp_handle_cred_reqfunction or a
pointer to thesrp_handle_aer_reqfunction is passed.
Quoted 3 lines
There's no need to do that; I don't think it will make it look better. I've given you a clean, simple way to get there. What objections did you have to the code I posted?
I don't like its style and aesthetics.
Bart.
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html