[ewg] RE: [PATCH 3/6] nes: add missing unlock in error path of nes_alloc_fmr()

[Roland Dreier]

 > Another question - How do you like to see multiple patches in a set
 > where more than one of those patches affects the same file?  I've
 > been creating them where the second patch relies on the first patch
 > being successfully applied.  Correct?

Yes, if you have multiple patches that touch the same area, then order
dependencies are kind of inevitable.  Just make sure you number the
patches so it's clear what order they go in.

By the way, I was looking at nes_verbs.c, and I see:

	static struct ib_mw *nes_alloc_mw(struct ib_pd *ibpd) {
	...
		cqp_request = nes_get_cqp_request(nesdev);
		if (cqp_request == NULL) {
			kfree(nesmr);
			nes_free_resource(nesadapter, nesadapter->allocated_mrs, stag_index);
			return ERR_PTR(-ENOMEM);
		}
	
		cqp_request->waiting = 1;
		cqp_wqe = &cqp_request->cqp_wqe;
	
		cqp_wqe->wqe_words[NES_CQP_WQE_OPCODE_IDX] =
				cpu_to_le32( NES_CQP_ALLOCATE_STAG | NES_CQP_STAG_RIGHTS_REMOTE_READ |
				NES_CQP_STAG_RIGHTS_REMOTE_WRITE | NES_CQP_STAG_VA_TO |
				NES_CQP_STAG_REM_ACC_EN);
	
		cqp_wqe->wqe_words[NES_CQP_WQE_COMP_CTX_LOW_IDX] =
				cpu_to_le32((u32)((u64)(&nesdev->cqp)));
		cqp_wqe->wqe_words[NES_CQP_WQE_COMP_CTX_HIGH_IDX] =
				cpu_to_le32((u32)(((u64)(&nesdev->cqp))>>32));
		cqp_wqe->wqe_words[NES_CQP_WQE_COMP_SCRATCH_LOW_IDX] = 0;
		cqp_wqe->wqe_words[NES_CQP_WQE_COMP_SCRATCH_HIGH_IDX] = 0;
	
		cqp_wqe->wqe_words[NES_CQP_STAG_WQE_LEN_LOW_IDX] = 0;
		cqp_wqe->wqe_words[NES_CQP_STAG_WQE_LEN_HIGH_PD_IDX] =
				cpu_to_le32(nespd->pd_id & 0x00007fff);
		cqp_wqe->wqe_words[NES_CQP_STAG_WQE_STAG_IDX] = cpu_to_le32(stag);
	
		cqp_wqe->wqe_words[NES_CQP_STAG_WQE_PA_LOW_IDX] = 0;
		cqp_wqe->wqe_words[NES_CQP_STAG_WQE_PA_HIGH_IDX] = 0;
		cqp_wqe->wqe_words[NES_CQP_STAG_WQE_PBL_BLK_COUNT_IDX] = 0;
		cqp_wqe->wqe_words[NES_CQP_STAG_WQE_PBL_LEN_IDX] = 0;
	
		atomic_set(&cqp_request->refcount, 2);
		nes_post_cqp_request(nesdev, cqp_request, NES_CQP_REQUEST_RING_DOORBELL);
	
		/* Wait for CQP */
		ret = wait_event_timeout(cqp_request->waitq, (cqp_request->request_done != 0),
				NES_EVENT_TIMEOUT);
		nes_debug(NES_DBG_MR, "Register STag 0x%08X completed, wait_event_timeout ret = %u,"
				" CQP Major:Minor codes = 0x%04X:0x%04X.\n",
				stag, ret, cqp_request->major_code, cqp_request->minor_code);
		if ((!ret) || (cqp_request->major_code)) {
			if (atomic_dec_and_test(&cqp_request->refcount)) {
				if (cqp_request->dynamic) {
					kfree(cqp_request);
				} else {
					spin_lock_irqsave(&nesdev->cqp.lock, flags);
					list_add_tail(&cqp_request->list, &nesdev->cqp_avail_reqs);
					spin_unlock_irqrestore(&nesdev->cqp.lock, flags);
				}
			}
	
and

	static int nes_dealloc_mw(struct ib_mw *ibmw)
	...
		cqp_request = nes_get_cqp_request(nesdev);
		if (cqp_request == NULL) {
			nes_debug(NES_DBG_MR, "Failed to get a cqp_request.\n");
			return -ENOMEM;
		}
		cqp_request->waiting = 1;
		cqp_wqe = &cqp_request->cqp_wqe;
	
		cqp_wqe->wqe_words[NES_CQP_WQE_OPCODE_IDX] = cpu_to_le32(NES_CQP_DEALLOCATE_STAG);
		cqp_wqe->wqe_words[NES_CQP_WQE_COMP_CTX_LOW_IDX] =
				cpu_to_le32((u32)((u64)(&nesdev->cqp)));
		cqp_wqe->wqe_words[NES_CQP_WQE_COMP_CTX_HIGH_IDX] =
				cpu_to_le32((u32)(((u64)(&nesdev->cqp)) >> 32));
		cqp_wqe->wqe_words[NES_CQP_WQE_COMP_SCRATCH_LOW_IDX] = 0;
		cqp_wqe->wqe_words[NES_CQP_WQE_COMP_SCRATCH_HIGH_IDX] = 0;
		cqp_wqe->wqe_words[NES_CQP_STAG_WQE_PBL_BLK_COUNT_IDX] = 0;
		cqp_wqe->wqe_words[NES_CQP_STAG_WQE_PBL_LEN_IDX] = 0;
		cqp_wqe->wqe_words[NES_CQP_STAG_WQE_STAG_IDX] = cpu_to_le32(ibmw->rkey);
	
		atomic_set(&cqp_request->refcount, 2);
		nes_post_cqp_request(nesdev, cqp_request, NES_CQP_REQUEST_RING_DOORBELL);
	
		/* Wait for CQP */
		nes_debug(NES_DBG_MR, "Waiting for deallocate STag 0x%08X to complete.\n",
				ibmw->rkey);
		ret = wait_event_timeout(cqp_request->waitq, (0 != cqp_request->request_done),
				NES_EVENT_TIMEOUT);
		nes_debug(NES_DBG_MR, "Deallocate STag completed, wait_event_timeout ret = %u,"
				" CQP Major:Minor codes = 0x%04X:0x%04X.\n",
				ret, cqp_request->major_code, cqp_request->minor_code);
		if ((!ret) || (cqp_request->major_code)) {
			if (atomic_dec_and_test(&cqp_request->refcount)) {
				if (cqp_request->dynamic) {
					kfree(cqp_request);
				} else {
					spin_lock_irqsave(&nesdev->cqp.lock, flags);
					list_add_tail(&cqp_request->list, &nesdev->cqp_avail_reqs);
					spin_unlock_irqrestore(&nesdev->cqp.lock, flags);
				}
			}
			if (!ret) {
				err = -ETIME;
			} else {
				err = -EIO;
			}
		} else {
			if (atomic_dec_and_test(&cqp_request->refcount)) {
				if (cqp_request->dynamic) {
					kfree(cqp_request);
				} else {
					spin_lock_irqsave(&nesdev->cqp.lock, flags);
					list_add_tail(&cqp_request->list, &nesdev->cqp_avail_reqs);
					spin_unlock_irqrestore(&nesdev->cqp.lock, flags);
				}
			}
		}
	
and so on.  It seems the driver would be a lot cleaner if you factored
more of this cqp request stuff out rather than repeating it with
slight variations so much.

 - R.