[PATCH 0/5] staging: rdma: hfi1: Drop wrapper functions

[PATCH 0/5] staging: rdma: hfi1: Drop wrapper functions

[Amitoj Kaur Chawla]

This patchet removes wrapper functions that can be replaced by a
single line of code.

Amitoj Kaur Chawla (5):
  staging: rdma: hfi1: Remove hfi1_nomsix() wrapper function
  staging: rdma: hfi1: Remove set_sbus_fast_mode() wrapper function
  staging: rdma: hfi1: chip: Remove wrapper function
  staging: rdma: hfi1: chip: Remove wrapper functions
  staging: rdma: hfi1: sdma: Remove wrapper functions

 drivers/staging/rdma/hfi1/chip.c     | 40 +++++++++++-------------------------
 drivers/staging/rdma/hfi1/chip.h     |  1 -
 drivers/staging/rdma/hfi1/firmware.c | 15 ++++++--------
 drivers/staging/rdma/hfi1/hfi.h      |  1 -
 drivers/staging/rdma/hfi1/pcie.c     | 11 ++--------
 drivers/staging/rdma/hfi1/sdma.c     | 40 +++++++++++++-----------------------
 6 files changed, 34 insertions(+), 74 deletions(-)

-- 
1.9.1

[PATCH 1/5] staging: rdma: hfi1: Remove hfi1_nomsix() wrapper function

[Amitoj Kaur Chawla]

Drop wrapper function hfi1_nomsix() that can be replaced by a single line 
of code.

Signed-off-by: Amitoj Kaur Chawla <amitoj1606@gmail.com>
---
 drivers/staging/rdma/hfi1/chip.c | 2 +-
 drivers/staging/rdma/hfi1/hfi.h  | 1 -
 drivers/staging/rdma/hfi1/pcie.c | 8 --------
 3 files changed, 1 insertion(+), 10 deletions(-)

diff --git a/drivers/staging/rdma/hfi1/chip.c b/drivers/staging/rdma/hfi1/chip.c
index aa58e59..beac326d 100644
--- a/drivers/staging/rdma/hfi1/chip.c
+++ b/drivers/staging/rdma/hfi1/chip.c
@@ -8694,7 +8694,7 @@ static void clean_up_interrupts(struct hfi1_devdata *dd)
 	/* turn off interrupts */
 	if (dd->num_msix_entries) {
 		/* MSI-X */
-		hfi1_nomsix(dd);
+		pci_disable_msix(dd->pcidev);
 	} else {
 		/* INTx */
 		disable_intx(dd->pcidev);
diff --git a/drivers/staging/rdma/hfi1/hfi.h b/drivers/staging/rdma/hfi1/hfi.h
index 8ca171b..1f97124 100644
--- a/drivers/staging/rdma/hfi1/hfi.h
+++ b/drivers/staging/rdma/hfi1/hfi.h
@@ -1600,7 +1600,6 @@ void hfi1_pcie_flr(struct hfi1_devdata *);
 int pcie_speeds(struct hfi1_devdata *);
 void request_msix(struct hfi1_devdata *, u32 *, struct hfi1_msix_entry *);
 void hfi1_enable_intx(struct pci_dev *);
-void hfi1_nomsix(struct hfi1_devdata *);
 void restore_pci_variables(struct hfi1_devdata *dd);
 int do_pcie_gen3_transition(struct hfi1_devdata *dd);
 int parse_platform_config(struct hfi1_devdata *dd);
diff --git a/drivers/staging/rdma/hfi1/pcie.c b/drivers/staging/rdma/hfi1/pcie.c
index ac5653c..e6773b3 100644
--- a/drivers/staging/rdma/hfi1/pcie.c
+++ b/drivers/staging/rdma/hfi1/pcie.c
@@ -426,14 +426,6 @@ void request_msix(struct hfi1_devdata *dd, u32 *nent,
 	tune_pcie_caps(dd);
 }
 
-/*
- * Disable MSI-X.
- */
-void hfi1_nomsix(struct hfi1_devdata *dd)
-{
-	pci_disable_msix(dd->pcidev);
-}
-
 void hfi1_enable_intx(struct pci_dev *pdev)
 {
 	/* first, turn on INTx */
-- 
1.9.1

[PATCH 2/5] staging: rdma: hfi1: Remove set_sbus_fast_mode() wrapper function

[Amitoj Kaur Chawla]

Drop set_sbus_fast_mode() wrapper function that can be replaced by
a single line of code.

Signed-off-by: Amitoj Kaur Chawla <amitoj1606@gmail.com>
---
 drivers/staging/rdma/hfi1/chip.h     |  1 -
 drivers/staging/rdma/hfi1/firmware.c | 15 ++++++---------
 drivers/staging/rdma/hfi1/pcie.c     |  3 ++-
 3 files changed, 8 insertions(+), 11 deletions(-)

diff --git a/drivers/staging/rdma/hfi1/chip.h b/drivers/staging/rdma/hfi1/chip.h
index f89a432..092fc9d 100644
--- a/drivers/staging/rdma/hfi1/chip.h
+++ b/drivers/staging/rdma/hfi1/chip.h
@@ -619,7 +619,6 @@ void sbus_request(struct hfi1_devdata *dd,
 		  u8 receiver_addr, u8 data_addr, u8 command, u32 data_in);
 int sbus_request_slow(struct hfi1_devdata *dd,
 		      u8 receiver_addr, u8 data_addr, u8 command, u32 data_in);
-void set_sbus_fast_mode(struct hfi1_devdata *dd);
 void clear_sbus_fast_mode(struct hfi1_devdata *dd);
 int hfi1_firmware_init(struct hfi1_devdata *dd);
 int load_pcie_firmware(struct hfi1_devdata *dd);
diff --git a/drivers/staging/rdma/hfi1/firmware.c b/drivers/staging/rdma/hfi1/firmware.c
index 5c2f2ed..0ce9a16 100644
--- a/drivers/staging/rdma/hfi1/firmware.c
+++ b/drivers/staging/rdma/hfi1/firmware.c
@@ -987,7 +987,8 @@ void fabric_serdes_reset(struct hfi1_devdata *dd)
 	ra = fabric_serdes_broadcast[dd->hfi1_id];
 
 	acquire_hw_mutex(dd);
-	set_sbus_fast_mode(dd);
+	write_csr(dd, ASIC_CFG_SBUS_EXECUTE,
+				ASIC_CFG_SBUS_EXECUTE_FAST_MODE_SMASK);
 	/* place SerDes in reset and disable SPICO */
 	sbus_request(dd, ra, 0x07, WRITE_SBUS_RECEIVER, 0x00000011);
 	/* wait 100 refclk cycles @ 156.25MHz => 640ns */
@@ -1214,12 +1215,6 @@ void release_hw_mutex(struct hfi1_devdata *dd)
 	write_csr(dd, ASIC_CFG_MUTEX, 0);
 }
 
-void set_sbus_fast_mode(struct hfi1_devdata *dd)
-{
-	write_csr(dd, ASIC_CFG_SBUS_EXECUTE,
-				ASIC_CFG_SBUS_EXECUTE_FAST_MODE_SMASK);
-}
-
 void clear_sbus_fast_mode(struct hfi1_devdata *dd)
 {
 	u64 reg, count = 0;
@@ -1244,7 +1239,8 @@ int load_firmware(struct hfi1_devdata *dd)
 		if (ret)
 			return ret;
 
-		set_sbus_fast_mode(dd);
+		write_csr(dd, ASIC_CFG_SBUS_EXECUTE,
+				ASIC_CFG_SBUS_EXECUTE_FAST_MODE_SMASK);
 
 		/*
 		 * The SBus contains part of the fabric firmware and so must
@@ -1583,7 +1579,8 @@ int load_pcie_firmware(struct hfi1_devdata *dd)
 	int ret = 0;
 
 	/* both firmware loads below use the SBus */
-	set_sbus_fast_mode(dd);
+	write_csr(dd, ASIC_CFG_SBUS_EXECUTE,
+				ASIC_CFG_SBUS_EXECUTE_FAST_MODE_SMASK);
 
 	if (fw_sbus_load) {
 		turn_off_spicos(dd, SPICO_SBUS);
diff --git a/drivers/staging/rdma/hfi1/pcie.c b/drivers/staging/rdma/hfi1/pcie.c
index e6773b3..10596b1 100644
--- a/drivers/staging/rdma/hfi1/pcie.c
+++ b/drivers/staging/rdma/hfi1/pcie.c
@@ -770,7 +770,8 @@ static void pcie_post_steps(struct hfi1_devdata *dd)
 {
 	int i;
 
-	set_sbus_fast_mode(dd);
+	write_csr(dd, ASIC_CFG_SBUS_EXECUTE,
+				ASIC_CFG_SBUS_EXECUTE_FAST_MODE_SMASK);
 	/*
 	 * Write to the PCIe PCSes to set the G3_LOCKED_NEXT bits to 1.
 	 * This avoids a spurious framing error that can otherwise be
-- 
1.9.1

[PATCH 3/5] staging: rdma: hfi1: chip: Remove wrapper function

[Amitoj Kaur Chawla]

Drop wrapper function remap_receive_available_interrupt() that can be
replaced by a single line of code.

Signed-off-by: Amitoj Kaur Chawla <amitoj1606@gmail.com>
---
 drivers/staging/rdma/hfi1/chip.c | 8 +-------
 1 file changed, 1 insertion(+), 7 deletions(-)

diff --git a/drivers/staging/rdma/hfi1/chip.c b/drivers/staging/rdma/hfi1/chip.c
index beac326d..a9c6abf 100644
--- a/drivers/staging/rdma/hfi1/chip.c
+++ b/drivers/staging/rdma/hfi1/chip.c
@@ -8749,12 +8749,6 @@ static void remap_sdma_interrupts(struct hfi1_devdata *dd,
 		msix_intr);
 }
 
-static void remap_receive_available_interrupt(struct hfi1_devdata *dd,
-					      int rx, int msix_intr)
-{
-	remap_intr(dd, IS_RCVAVAIL_START + rx, msix_intr);
-}
-
 static int request_intx_irq(struct hfi1_devdata *dd)
 {
 	int ret;
@@ -8890,7 +8884,7 @@ static int request_msix_irqs(struct hfi1_devdata *dd)
 			snprintf(me->name, sizeof(me->name),
 				DRIVER_NAME"_%d kctxt%d", dd->unit, idx);
 			err_info = "receive context";
-			remap_receive_available_interrupt(dd, idx, i);
+			remap_intr(dd, IS_RCVAVAIL_START + idx, i);
 		} else {
 			/* not in our expected range - complain, then
 			   ignore it */
-- 
1.9.1

[PATCH 4/5] staging: rdma: hfi1: chip: Remove wrapper functions

[Amitoj Kaur Chawla]

Drop wrapper functions add_rcvctrl() and clear_rcvctrl() that can be replaced by a single line of code.

Signed-off-by: Amitoj Kaur Chawla <amitoj1606@gmail.com>
---
 drivers/staging/rdma/hfi1/chip.c | 30 ++++++++++--------------------
 1 file changed, 10 insertions(+), 20 deletions(-)

diff --git a/drivers/staging/rdma/hfi1/chip.c b/drivers/staging/rdma/hfi1/chip.c
index a9c6abf..23cacbf 100644
--- a/drivers/staging/rdma/hfi1/chip.c
+++ b/drivers/staging/rdma/hfi1/chip.c
@@ -3176,16 +3176,6 @@ static void adjust_rcvctrl(struct hfi1_devdata *dd, u64 add, u64 clear)
 	spin_unlock_irqrestore(&dd->rcvctrl_lock, flags);
 }
 
-static inline void add_rcvctrl(struct hfi1_devdata *dd, u64 add)
-{
-	adjust_rcvctrl(dd, add, 0);
-}
-
-static inline void clear_rcvctrl(struct hfi1_devdata *dd, u64 clear)
-{
-	adjust_rcvctrl(dd, 0, clear);
-}
-
 /*
  * Called from all interrupt handlers to start handling an SPC freeze.
  */
@@ -3268,7 +3258,7 @@ static void rxe_freeze(struct hfi1_devdata *dd)
 	int i;
 
 	/* disable port */
-	clear_rcvctrl(dd, RCV_CTRL_RCV_PORT_ENABLE_SMASK);
+	adjust_rcvctrl(dd, 0, RCV_CTRL_RCV_PORT_ENABLE_SMASK);
 
 	/* disable all receive contexts */
 	for (i = 0; i < dd->num_rcv_contexts; i++)
@@ -3290,7 +3280,7 @@ static void rxe_kernel_unfreeze(struct hfi1_devdata *dd)
 		hfi1_rcvctrl(dd, HFI1_RCVCTRL_CTXT_ENB, i);
 
 	/* enable port */
-	add_rcvctrl(dd, RCV_CTRL_RCV_PORT_ENABLE_SMASK);
+	adjust_rcvctrl(dd, RCV_CTRL_RCV_PORT_ENABLE_SMASK, 0);
 }
 
 /*
@@ -3441,7 +3431,7 @@ void handle_link_down(struct work_struct *work)
 	reset_neighbor_info(ppd);
 
 	/* disable the port */
-	clear_rcvctrl(ppd->dd, RCV_CTRL_RCV_PORT_ENABLE_SMASK);
+	adjust_rcvctrl(ppd->dd, 0, RCV_CTRL_RCV_PORT_ENABLE_SMASK);
 
 	/* If there is no cable attached, turn the DC off. Otherwise,
 	 * start the link bring up. */
@@ -5771,7 +5761,7 @@ int bringup_serdes(struct hfi1_pportdata *ppd)
 	int ret;
 
 	if (HFI1_CAP_IS_KSET(EXTENDED_PSN))
-		add_rcvctrl(dd, RCV_CTRL_RCV_EXTENDED_PSN_ENABLE_SMASK);
+		adjust_rcvctrl(dd, RCV_CTRL_RCV_EXTENDED_PSN_ENABLE_SMASK, 0);
 
 	guid = ppd->guid;
 	if (!guid) {
@@ -5813,7 +5803,7 @@ void hfi1_quiet_serdes(struct hfi1_pportdata *ppd)
 	set_link_state(ppd, HLS_DN_OFFLINE);
 
 	/* disable the port */
-	clear_rcvctrl(dd, RCV_CTRL_RCV_PORT_ENABLE_SMASK);
+	adjust_rcvctrl(dd, 0, RCV_CTRL_RCV_PORT_ENABLE_SMASK);
 }
 
 static inline int init_cpu_counters(struct hfi1_devdata *dd)
@@ -6461,7 +6451,7 @@ int set_link_state(struct hfi1_pportdata *ppd, u32 state)
 					OPA_LINKINIT_REASON_LINKUP;
 
 			/* enable the port */
-			add_rcvctrl(dd, RCV_CTRL_RCV_PORT_ENABLE_SMASK);
+			adjust_rcvctrl(dd, RCV_CTRL_RCV_PORT_ENABLE_SMASK, 0);
 
 			handle_linkup_change(dd, 1);
 		}
@@ -9193,7 +9183,7 @@ static void set_partition_keys(struct hfi1_pportdata *ppd)
 	}
 
 	/* Always enable HW pkeys check when pkeys table is set */
-	add_rcvctrl(dd, RCV_CTRL_RCV_PARTITION_KEY_ENABLE_SMASK);
+	adjust_rcvctrl(dd, RCV_CTRL_RCV_PARTITION_KEY_ENABLE_SMASK, 0);
 }
 
 /*
@@ -9945,8 +9935,8 @@ static void init_qpmap_table(struct hfi1_devdata *dd,
 	if (i % 8)
 		write_csr(dd, regno, reg);
 
-	add_rcvctrl(dd, RCV_CTRL_RCV_QP_MAP_ENABLE_SMASK
-			| RCV_CTRL_RCV_BYPASS_ENABLE_SMASK);
+	adjust_rcvctrl(dd, RCV_CTRL_RCV_QP_MAP_ENABLE_SMASK
+			| RCV_CTRL_RCV_BYPASS_ENABLE_SMASK, 0);
 }
 
 /**
@@ -10041,7 +10031,7 @@ static void init_qos(struct hfi1_devdata *dd, u32 first_ctxt)
 		LRH_SC_MASK << RCV_RSM_MATCH_MASK2_SHIFT |
 		LRH_SC_VALUE << RCV_RSM_MATCH_VALUE2_SHIFT);
 	/* Enable RSM */
-	add_rcvctrl(dd, RCV_CTRL_RCV_RSM_ENABLE_SMASK);
+	adjust_rcvctrl(dd, RCV_CTRL_RCV_RSM_ENABLE_SMASK, 0);
 	kfree(rsmmap);
 	/* map everything else (non-VL15) to context 0 */
 	init_qpmap_table(
-- 
1.9.1

[PATCH 5/5] staging: rdma: hfi1: sdma: Remove wrapper functions

[Amitoj Kaur Chawla]

Drop wrapper functions sdma_start_err_halt_wait() and sdma_start_sw_clean_up()
that can be replaced by a single line of code.

Signed-off-by: Amitoj Kaur Chawla <amitoj1606@gmail.com>
---
 drivers/staging/rdma/hfi1/sdma.c | 40 ++++++++++++++--------------------------
 1 file changed, 14 insertions(+), 26 deletions(-)

diff --git a/drivers/staging/rdma/hfi1/sdma.c b/drivers/staging/rdma/hfi1/sdma.c
index 63ab721..4e44ac7 100644
--- a/drivers/staging/rdma/hfi1/sdma.c
+++ b/drivers/staging/rdma/hfi1/sdma.c
@@ -230,7 +230,6 @@ static void sdma_hw_clean_up_task(unsigned long);
 static void sdma_put(struct sdma_state *);
 static void sdma_set_state(struct sdma_engine *, enum sdma_states);
 static void sdma_start_hw_clean_up(struct sdma_engine *);
-static void sdma_start_sw_clean_up(struct sdma_engine *);
 static void sdma_sw_clean_up_task(unsigned long);
 static void sdma_sendctrl(struct sdma_engine *, unsigned);
 static void init_sdma_regs(struct sdma_engine *, u32, uint);
@@ -454,12 +453,6 @@ static void sdma_err_halt_wait(struct work_struct *work)
 	sdma_process_event(sde, sdma_event_e15_hw_halt_done);
 }
 
-static void sdma_start_err_halt_wait(struct sdma_engine *sde)
-{
-	schedule_work(&sde->err_halt_worker);
-}
-
-
 static void sdma_err_progress_check_schedule(struct sdma_engine *sde)
 {
 	if (!is_bx(sde->dd) && HFI1_CAP_IS_KSET(SDMA_AHG)) {
@@ -666,11 +659,6 @@ static void sdma_start_hw_clean_up(struct sdma_engine *sde)
 	tasklet_hi_schedule(&sde->sdma_hw_clean_up_task);
 }
 
-static void sdma_start_sw_clean_up(struct sdma_engine *sde)
-{
-	tasklet_hi_schedule(&sde->sdma_sw_clean_up_task);
-}
-
 static void sdma_set_state(struct sdma_engine *sde,
 	enum sdma_states next_state)
 {
@@ -2291,7 +2279,7 @@ static void __sdma_process_event(struct sdma_engine *sde,
 		case sdma_event_e50_hw_cleaned:
 			break;
 		case sdma_event_e60_hw_halted:
-			sdma_start_err_halt_wait(sde);
+			schedule_work(&sde->err_halt_worker);
 			break;
 		case sdma_event_e70_go_idle:
 			ss->go_s99_running = 0;
@@ -2372,7 +2360,7 @@ static void __sdma_process_event(struct sdma_engine *sde,
 			break;
 		case sdma_event_e60_hw_halted:
 			sdma_set_state(sde, sdma_state_s50_hw_halt_wait);
-			sdma_start_err_halt_wait(sde);
+			schedule_work(&sde->err_halt_worker);
 			break;
 		case sdma_event_e70_go_idle:
 			break;
@@ -2435,7 +2423,7 @@ static void __sdma_process_event(struct sdma_engine *sde,
 		switch (event) {
 		case sdma_event_e00_go_hw_down:
 			sdma_set_state(sde, sdma_state_s00_hw_down);
-			sdma_start_sw_clean_up(sde);
+			tasklet_hi_schedule(&sde->sdma_sw_clean_up_task);
 			break;
 		case sdma_event_e10_go_hw_start:
 			break;
@@ -2477,13 +2465,13 @@ static void __sdma_process_event(struct sdma_engine *sde,
 		switch (event) {
 		case sdma_event_e00_go_hw_down:
 			sdma_set_state(sde, sdma_state_s00_hw_down);
-			sdma_start_sw_clean_up(sde);
+			tasklet_hi_schedule(&sde->sdma_sw_clean_up_task);
 			break;
 		case sdma_event_e10_go_hw_start:
 			break;
 		case sdma_event_e15_hw_halt_done:
 			sdma_set_state(sde, sdma_state_s30_sw_clean_up_wait);
-			sdma_start_sw_clean_up(sde);
+			tasklet_hi_schedule(&sde->sdma_sw_clean_up_task);
 			break;
 		case sdma_event_e25_hw_clean_up_done:
 			break;
@@ -2495,7 +2483,7 @@ static void __sdma_process_event(struct sdma_engine *sde,
 		case sdma_event_e50_hw_cleaned:
 			break;
 		case sdma_event_e60_hw_halted:
-			sdma_start_err_halt_wait(sde);
+			schedule_work(&sde->err_halt_worker);
 			break;
 		case sdma_event_e70_go_idle:
 			ss->go_s99_running = 0;
@@ -2518,13 +2506,13 @@ static void __sdma_process_event(struct sdma_engine *sde,
 		switch (event) {
 		case sdma_event_e00_go_hw_down:
 			sdma_set_state(sde, sdma_state_s00_hw_down);
-			sdma_start_sw_clean_up(sde);
+			tasklet_hi_schedule(&sde->sdma_sw_clean_up_task);
 			break;
 		case sdma_event_e10_go_hw_start:
 			break;
 		case sdma_event_e15_hw_halt_done:
 			sdma_set_state(sde, sdma_state_s30_sw_clean_up_wait);
-			sdma_start_sw_clean_up(sde);
+			tasklet_hi_schedule(&sde->sdma_sw_clean_up_task);
 			break;
 		case sdma_event_e25_hw_clean_up_done:
 			break;
@@ -2536,7 +2524,7 @@ static void __sdma_process_event(struct sdma_engine *sde,
 		case sdma_event_e50_hw_cleaned:
 			break;
 		case sdma_event_e60_hw_halted:
-			sdma_start_err_halt_wait(sde);
+			schedule_work(&sde->err_halt_worker);
 			break;
 		case sdma_event_e70_go_idle:
 			ss->go_s99_running = 0;
@@ -2558,7 +2546,7 @@ static void __sdma_process_event(struct sdma_engine *sde,
 		switch (event) {
 		case sdma_event_e00_go_hw_down:
 			sdma_set_state(sde, sdma_state_s00_hw_down);
-			sdma_start_sw_clean_up(sde);
+			tasklet_hi_schedule(&sde->sdma_sw_clean_up_task);
 			break;
 		case sdma_event_e10_go_hw_start:
 			break;
@@ -2582,7 +2570,7 @@ static void __sdma_process_event(struct sdma_engine *sde,
 			break;
 		case sdma_event_e81_hw_frozen:
 			sdma_set_state(sde, sdma_state_s82_freeze_sw_clean);
-			sdma_start_sw_clean_up(sde);
+			tasklet_hi_schedule(&sde->sdma_sw_clean_up_task);
 			break;
 		case sdma_event_e82_hw_unfreeze:
 			break;
@@ -2597,7 +2585,7 @@ static void __sdma_process_event(struct sdma_engine *sde,
 		switch (event) {
 		case sdma_event_e00_go_hw_down:
 			sdma_set_state(sde, sdma_state_s00_hw_down);
-			sdma_start_sw_clean_up(sde);
+			tasklet_hi_schedule(&sde->sdma_sw_clean_up_task);
 			break;
 		case sdma_event_e10_go_hw_start:
 			break;
@@ -2641,7 +2629,7 @@ static void __sdma_process_event(struct sdma_engine *sde,
 		switch (event) {
 		case sdma_event_e00_go_hw_down:
 			sdma_set_state(sde, sdma_state_s00_hw_down);
-			sdma_start_sw_clean_up(sde);
+			tasklet_hi_schedule(&sde->sdma_sw_clean_up_task);
 			break;
 		case sdma_event_e10_go_hw_start:
 			break;
@@ -2664,7 +2652,7 @@ static void __sdma_process_event(struct sdma_engine *sde,
 			* progress check
 			*/
 			sdma_set_state(sde, sdma_state_s50_hw_halt_wait);
-			sdma_start_err_halt_wait(sde);
+			schedule_work(&sde->err_halt_worker);
 			break;
 		case sdma_event_e70_go_idle:
 			sdma_set_state(sde, sdma_state_s60_idle_halt_wait);
-- 
1.9.1

Re: [Outreachy kernel] [PATCH 4/5] staging: rdma: hfi1: chip: Remove wrapper functions

[Julia Lawall]

On Sun, 1 Nov 2015, Amitoj Kaur Chawla wrote:

> Drop wrapper functions add_rcvctrl() and clear_rcvctrl() that can be replaced by a single line of code.
> 
> Signed-off-by: Amitoj Kaur Chawla <amitoj1606@gmail.com>
> ---
>  drivers/staging/rdma/hfi1/chip.c | 30 ++++++++++--------------------
>  1 file changed, 10 insertions(+), 20 deletions(-)
> 
> diff --git a/drivers/staging/rdma/hfi1/chip.c b/drivers/staging/rdma/hfi1/chip.c
> index a9c6abf..23cacbf 100644
> --- a/drivers/staging/rdma/hfi1/chip.c
> +++ b/drivers/staging/rdma/hfi1/chip.c
> @@ -3176,16 +3176,6 @@ static void adjust_rcvctrl(struct hfi1_devdata *dd, u64 add, u64 clear)
>  	spin_unlock_irqrestore(&dd->rcvctrl_lock, flags);
>  }
>  
> -static inline void add_rcvctrl(struct hfi1_devdata *dd, u64 add)
> -{
> -	adjust_rcvctrl(dd, add, 0);
> -}
> -
> -static inline void clear_rcvctrl(struct hfi1_devdata *dd, u64 clear)
> -{
> -	adjust_rcvctrl(dd, 0, clear);
> -}

Is it really an improvement?  adjust_rcvctrl is not a standard kernel 
function.  In the original code, one can undestand what add_rcvctrl and 
clear_rcvctrl do based on their names.  In the modified code, you have to 
know what is the significance of the different argument positions of 
adjust_rcvctrl, so you have to go look at the definition.  So this makes 
more work for someone who wants to understand the code.

Wrapping standard kernel functions is not good.  And wrapping when the 
arguments don't change and the name is almost the same is probably not 
good either.  But here the wrapping would seem to add some value.

julia

>  /*
>   * Called from all interrupt handlers to start handling an SPC freeze.
>   */
> @@ -3268,7 +3258,7 @@ static void rxe_freeze(struct hfi1_devdata *dd)
>  	int i;
>  
>  	/* disable port */
> -	clear_rcvctrl(dd, RCV_CTRL_RCV_PORT_ENABLE_SMASK);
> +	adjust_rcvctrl(dd, 0, RCV_CTRL_RCV_PORT_ENABLE_SMASK);
>  
>  	/* disable all receive contexts */
>  	for (i = 0; i < dd->num_rcv_contexts; i++)
> @@ -3290,7 +3280,7 @@ static void rxe_kernel_unfreeze(struct hfi1_devdata *dd)
>  		hfi1_rcvctrl(dd, HFI1_RCVCTRL_CTXT_ENB, i);
>  
>  	/* enable port */
> -	add_rcvctrl(dd, RCV_CTRL_RCV_PORT_ENABLE_SMASK);
> +	adjust_rcvctrl(dd, RCV_CTRL_RCV_PORT_ENABLE_SMASK, 0);
>  }
>  
>  /*
> @@ -3441,7 +3431,7 @@ void handle_link_down(struct work_struct *work)
>  	reset_neighbor_info(ppd);
>  
>  	/* disable the port */
> -	clear_rcvctrl(ppd->dd, RCV_CTRL_RCV_PORT_ENABLE_SMASK);
> +	adjust_rcvctrl(ppd->dd, 0, RCV_CTRL_RCV_PORT_ENABLE_SMASK);
>  
>  	/* If there is no cable attached, turn the DC off. Otherwise,
>  	 * start the link bring up. */
> @@ -5771,7 +5761,7 @@ int bringup_serdes(struct hfi1_pportdata *ppd)
>  	int ret;
>  
>  	if (HFI1_CAP_IS_KSET(EXTENDED_PSN))
> -		add_rcvctrl(dd, RCV_CTRL_RCV_EXTENDED_PSN_ENABLE_SMASK);
> +		adjust_rcvctrl(dd, RCV_CTRL_RCV_EXTENDED_PSN_ENABLE_SMASK, 0);
>  
>  	guid = ppd->guid;
>  	if (!guid) {
> @@ -5813,7 +5803,7 @@ void hfi1_quiet_serdes(struct hfi1_pportdata *ppd)
>  	set_link_state(ppd, HLS_DN_OFFLINE);
>  
>  	/* disable the port */
> -	clear_rcvctrl(dd, RCV_CTRL_RCV_PORT_ENABLE_SMASK);
> +	adjust_rcvctrl(dd, 0, RCV_CTRL_RCV_PORT_ENABLE_SMASK);
>  }
>  
>  static inline int init_cpu_counters(struct hfi1_devdata *dd)
> @@ -6461,7 +6451,7 @@ int set_link_state(struct hfi1_pportdata *ppd, u32 state)
>  					OPA_LINKINIT_REASON_LINKUP;
>  
>  			/* enable the port */
> -			add_rcvctrl(dd, RCV_CTRL_RCV_PORT_ENABLE_SMASK);
> +			adjust_rcvctrl(dd, RCV_CTRL_RCV_PORT_ENABLE_SMASK, 0);
>  
>  			handle_linkup_change(dd, 1);
>  		}
> @@ -9193,7 +9183,7 @@ static void set_partition_keys(struct hfi1_pportdata *ppd)
>  	}
>  
>  	/* Always enable HW pkeys check when pkeys table is set */
> -	add_rcvctrl(dd, RCV_CTRL_RCV_PARTITION_KEY_ENABLE_SMASK);
> +	adjust_rcvctrl(dd, RCV_CTRL_RCV_PARTITION_KEY_ENABLE_SMASK, 0);
>  }
>  
>  /*
> @@ -9945,8 +9935,8 @@ static void init_qpmap_table(struct hfi1_devdata *dd,
>  	if (i % 8)
>  		write_csr(dd, regno, reg);
>  
> -	add_rcvctrl(dd, RCV_CTRL_RCV_QP_MAP_ENABLE_SMASK
> -			| RCV_CTRL_RCV_BYPASS_ENABLE_SMASK);
> +	adjust_rcvctrl(dd, RCV_CTRL_RCV_QP_MAP_ENABLE_SMASK
> +			| RCV_CTRL_RCV_BYPASS_ENABLE_SMASK, 0);
>  }
>  
>  /**
> @@ -10041,7 +10031,7 @@ static void init_qos(struct hfi1_devdata *dd, u32 first_ctxt)
>  		LRH_SC_MASK << RCV_RSM_MATCH_MASK2_SHIFT |
>  		LRH_SC_VALUE << RCV_RSM_MATCH_VALUE2_SHIFT);
>  	/* Enable RSM */
> -	add_rcvctrl(dd, RCV_CTRL_RCV_RSM_ENABLE_SMASK);
> +	adjust_rcvctrl(dd, RCV_CTRL_RCV_RSM_ENABLE_SMASK, 0);
>  	kfree(rsmmap);
>  	/* map everything else (non-VL15) to context 0 */
>  	init_qpmap_table(
> -- 
> 1.9.1
> 
> -- 
> You received this message because you are subscribed to the Google Groups "outreachy-kernel" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to outreachy-kernel+unsubscribe@googlegroups.com.
> To post to this group, send email to outreachy-kernel@googlegroups.com.
> To view this discussion on the web visit https://groups.google.com/d/msgid/outreachy-kernel/d87be280ac9dc0fe8fc50e25afabf579dcd7b98b.1446366036.git.amitoj1606%40gmail.com.
> For more options, visit https://groups.google.com/d/optout.
> 

Re: [Outreachy kernel] [PATCH 5/5] staging: rdma: hfi1: sdma: Remove wrapper functions

[Julia Lawall]

On Sun, 1 Nov 2015, Amitoj Kaur Chawla wrote:

> Drop wrapper functions sdma_start_err_halt_wait() and sdma_start_sw_clean_up()
> that can be replaced by a single line of code.
> 
> Signed-off-by: Amitoj Kaur Chawla <amitoj1606@gmail.com>
> ---
>  drivers/staging/rdma/hfi1/sdma.c | 40 ++++++++++++++--------------------------
>  1 file changed, 14 insertions(+), 26 deletions(-)
> 
> diff --git a/drivers/staging/rdma/hfi1/sdma.c b/drivers/staging/rdma/hfi1/sdma.c
> index 63ab721..4e44ac7 100644
> --- a/drivers/staging/rdma/hfi1/sdma.c
> +++ b/drivers/staging/rdma/hfi1/sdma.c
> @@ -230,7 +230,6 @@ static void sdma_hw_clean_up_task(unsigned long);
>  static void sdma_put(struct sdma_state *);
>  static void sdma_set_state(struct sdma_engine *, enum sdma_states);
>  static void sdma_start_hw_clean_up(struct sdma_engine *);
> -static void sdma_start_sw_clean_up(struct sdma_engine *);
>  static void sdma_sw_clean_up_task(unsigned long);
>  static void sdma_sendctrl(struct sdma_engine *, unsigned);
>  static void init_sdma_regs(struct sdma_engine *, u32, uint);
> @@ -454,12 +453,6 @@ static void sdma_err_halt_wait(struct work_struct *work)
>  	sdma_process_event(sde, sdma_event_e15_hw_halt_done);
>  }
>  
> -static void sdma_start_err_halt_wait(struct sdma_engine *sde)
> -{
> -	schedule_work(&sde->err_halt_worker);
> -}
> -
> -
>  static void sdma_err_progress_check_schedule(struct sdma_engine *sde)
>  {
>  	if (!is_bx(sde->dd) && HFI1_CAP_IS_KSET(SDMA_AHG)) {
> @@ -666,11 +659,6 @@ static void sdma_start_hw_clean_up(struct sdma_engine *sde)
>  	tasklet_hi_schedule(&sde->sdma_hw_clean_up_task);
>  }
>  
> -static void sdma_start_sw_clean_up(struct sdma_engine *sde)
> -{
> -	tasklet_hi_schedule(&sde->sdma_sw_clean_up_task);
> -}

Maybe these are OK, because you are introducing standard kernel functions, 
and because the field names give pretty much the same information as the 
function names did.

julia

>  static void sdma_set_state(struct sdma_engine *sde,
>  	enum sdma_states next_state)
>  {
> @@ -2291,7 +2279,7 @@ static void __sdma_process_event(struct sdma_engine *sde,
>  		case sdma_event_e50_hw_cleaned:
>  			break;
>  		case sdma_event_e60_hw_halted:
> -			sdma_start_err_halt_wait(sde);
> +			schedule_work(&sde->err_halt_worker);
>  			break;
>  		case sdma_event_e70_go_idle:
>  			ss->go_s99_running = 0;
> @@ -2372,7 +2360,7 @@ static void __sdma_process_event(struct sdma_engine *sde,
>  			break;
>  		case sdma_event_e60_hw_halted:
>  			sdma_set_state(sde, sdma_state_s50_hw_halt_wait);
> -			sdma_start_err_halt_wait(sde);
> +			schedule_work(&sde->err_halt_worker);
>  			break;
>  		case sdma_event_e70_go_idle:
>  			break;
> @@ -2435,7 +2423,7 @@ static void __sdma_process_event(struct sdma_engine *sde,
>  		switch (event) {
>  		case sdma_event_e00_go_hw_down:
>  			sdma_set_state(sde, sdma_state_s00_hw_down);
> -			sdma_start_sw_clean_up(sde);
> +			tasklet_hi_schedule(&sde->sdma_sw_clean_up_task);
>  			break;
>  		case sdma_event_e10_go_hw_start:
>  			break;
> @@ -2477,13 +2465,13 @@ static void __sdma_process_event(struct sdma_engine *sde,
>  		switch (event) {
>  		case sdma_event_e00_go_hw_down:
>  			sdma_set_state(sde, sdma_state_s00_hw_down);
> -			sdma_start_sw_clean_up(sde);
> +			tasklet_hi_schedule(&sde->sdma_sw_clean_up_task);
>  			break;
>  		case sdma_event_e10_go_hw_start:
>  			break;
>  		case sdma_event_e15_hw_halt_done:
>  			sdma_set_state(sde, sdma_state_s30_sw_clean_up_wait);
> -			sdma_start_sw_clean_up(sde);
> +			tasklet_hi_schedule(&sde->sdma_sw_clean_up_task);
>  			break;
>  		case sdma_event_e25_hw_clean_up_done:
>  			break;
> @@ -2495,7 +2483,7 @@ static void __sdma_process_event(struct sdma_engine *sde,
>  		case sdma_event_e50_hw_cleaned:
>  			break;
>  		case sdma_event_e60_hw_halted:
> -			sdma_start_err_halt_wait(sde);
> +			schedule_work(&sde->err_halt_worker);
>  			break;
>  		case sdma_event_e70_go_idle:
>  			ss->go_s99_running = 0;
> @@ -2518,13 +2506,13 @@ static void __sdma_process_event(struct sdma_engine *sde,
>  		switch (event) {
>  		case sdma_event_e00_go_hw_down:
>  			sdma_set_state(sde, sdma_state_s00_hw_down);
> -			sdma_start_sw_clean_up(sde);
> +			tasklet_hi_schedule(&sde->sdma_sw_clean_up_task);
>  			break;
>  		case sdma_event_e10_go_hw_start:
>  			break;
>  		case sdma_event_e15_hw_halt_done:
>  			sdma_set_state(sde, sdma_state_s30_sw_clean_up_wait);
> -			sdma_start_sw_clean_up(sde);
> +			tasklet_hi_schedule(&sde->sdma_sw_clean_up_task);
>  			break;
>  		case sdma_event_e25_hw_clean_up_done:
>  			break;
> @@ -2536,7 +2524,7 @@ static void __sdma_process_event(struct sdma_engine *sde,
>  		case sdma_event_e50_hw_cleaned:
>  			break;
>  		case sdma_event_e60_hw_halted:
> -			sdma_start_err_halt_wait(sde);
> +			schedule_work(&sde->err_halt_worker);
>  			break;
>  		case sdma_event_e70_go_idle:
>  			ss->go_s99_running = 0;
> @@ -2558,7 +2546,7 @@ static void __sdma_process_event(struct sdma_engine *sde,
>  		switch (event) {
>  		case sdma_event_e00_go_hw_down:
>  			sdma_set_state(sde, sdma_state_s00_hw_down);
> -			sdma_start_sw_clean_up(sde);
> +			tasklet_hi_schedule(&sde->sdma_sw_clean_up_task);
>  			break;
>  		case sdma_event_e10_go_hw_start:
>  			break;
> @@ -2582,7 +2570,7 @@ static void __sdma_process_event(struct sdma_engine *sde,
>  			break;
>  		case sdma_event_e81_hw_frozen:
>  			sdma_set_state(sde, sdma_state_s82_freeze_sw_clean);
> -			sdma_start_sw_clean_up(sde);
> +			tasklet_hi_schedule(&sde->sdma_sw_clean_up_task);
>  			break;
>  		case sdma_event_e82_hw_unfreeze:
>  			break;
> @@ -2597,7 +2585,7 @@ static void __sdma_process_event(struct sdma_engine *sde,
>  		switch (event) {
>  		case sdma_event_e00_go_hw_down:
>  			sdma_set_state(sde, sdma_state_s00_hw_down);
> -			sdma_start_sw_clean_up(sde);
> +			tasklet_hi_schedule(&sde->sdma_sw_clean_up_task);
>  			break;
>  		case sdma_event_e10_go_hw_start:
>  			break;
> @@ -2641,7 +2629,7 @@ static void __sdma_process_event(struct sdma_engine *sde,
>  		switch (event) {
>  		case sdma_event_e00_go_hw_down:
>  			sdma_set_state(sde, sdma_state_s00_hw_down);
> -			sdma_start_sw_clean_up(sde);
> +			tasklet_hi_schedule(&sde->sdma_sw_clean_up_task);
>  			break;
>  		case sdma_event_e10_go_hw_start:
>  			break;
> @@ -2664,7 +2652,7 @@ static void __sdma_process_event(struct sdma_engine *sde,
>  			* progress check
>  			*/
>  			sdma_set_state(sde, sdma_state_s50_hw_halt_wait);
> -			sdma_start_err_halt_wait(sde);
> +			schedule_work(&sde->err_halt_worker);
>  			break;
>  		case sdma_event_e70_go_idle:
>  			sdma_set_state(sde, sdma_state_s60_idle_halt_wait);
> -- 
> 1.9.1
> 
> -- 
> You received this message because you are subscribed to the Google Groups "outreachy-kernel" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to outreachy-kernel+unsubscribe@googlegroups.com.
> To post to this group, send email to outreachy-kernel@googlegroups.com.
> To view this discussion on the web visit https://groups.google.com/d/msgid/outreachy-kernel/12c017e4966c52a7f15487e1bd2eb6b907bee4d3.1446366036.git.amitoj1606%40gmail.com.
> For more options, visit https://groups.google.com/d/optout.
> 

Re: [Outreachy kernel] [PATCH 3/5] staging: rdma: hfi1: chip: Remove wrapper function

[Julia Lawall]

On Sun, 1 Nov 2015, Amitoj Kaur Chawla wrote:

> Drop wrapper function remap_receive_available_interrupt() that can be
> replaced by a single line of code.
> 
> Signed-off-by: Amitoj Kaur Chawla <amitoj1606@gmail.com>
> ---
>  drivers/staging/rdma/hfi1/chip.c | 8 +-------
>  1 file changed, 1 insertion(+), 7 deletions(-)
> 
> diff --git a/drivers/staging/rdma/hfi1/chip.c b/drivers/staging/rdma/hfi1/chip.c
> index beac326d..a9c6abf 100644
> --- a/drivers/staging/rdma/hfi1/chip.c
> +++ b/drivers/staging/rdma/hfi1/chip.c
> @@ -8749,12 +8749,6 @@ static void remap_sdma_interrupts(struct hfi1_devdata *dd,
>  		msix_intr);
>  }
>  
> -static void remap_receive_available_interrupt(struct hfi1_devdata *dd,
> -					      int rx, int msix_intr)
> -{
> -	remap_intr(dd, IS_RCVAVAIL_START + rx, msix_intr);
> -}

remap_intr is not a standard kernel function.  But perhaps one could argue 
that the constant name gives the same information as the function name, so 
the function is not needed.

But this function is the neighbor of another very similar function that 
remaps three interrupts at once.  The two functions are also used in a 
very parallel way, at the end of successive if branches.  It seems strange 
to inline one function and not the other.

julia

>  static int request_intx_irq(struct hfi1_devdata *dd)
>  {
>  	int ret;
> @@ -8890,7 +8884,7 @@ static int request_msix_irqs(struct hfi1_devdata *dd)
>  			snprintf(me->name, sizeof(me->name),
>  				DRIVER_NAME"_%d kctxt%d", dd->unit, idx);
>  			err_info = "receive context";
> -			remap_receive_available_interrupt(dd, idx, i);
> +			remap_intr(dd, IS_RCVAVAIL_START + idx, i);
>  		} else {
>  			/* not in our expected range - complain, then
>  			   ignore it */
> -- 
> 1.9.1
> 
> -- 
> You received this message because you are subscribed to the Google Groups "outreachy-kernel" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to outreachy-kernel+unsubscribe@googlegroups.com.
> To post to this group, send email to outreachy-kernel@googlegroups.com.
> To view this discussion on the web visit https://groups.google.com/d/msgid/outreachy-kernel/23ac38a01d5092053da227aaa0741b270125ceeb.1446366036.git.amitoj1606%40gmail.com.
> For more options, visit https://groups.google.com/d/optout.
> 

Re: [Outreachy kernel] [PATCH 2/5] staging: rdma: hfi1: Remove set_sbus_fast_mode() wrapper function

[Julia Lawall]

On Sun, 1 Nov 2015, Amitoj Kaur Chawla wrote:

> Drop set_sbus_fast_mode() wrapper function that can be replaced by
> a single line of code.
> 
> Signed-off-by: Amitoj Kaur Chawla <amitoj1606@gmail.com>
> ---
>  drivers/staging/rdma/hfi1/chip.h     |  1 -
>  drivers/staging/rdma/hfi1/firmware.c | 15 ++++++---------
>  drivers/staging/rdma/hfi1/pcie.c     |  3 ++-
>  3 files changed, 8 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/staging/rdma/hfi1/chip.h b/drivers/staging/rdma/hfi1/chip.h
> index f89a432..092fc9d 100644
> --- a/drivers/staging/rdma/hfi1/chip.h
> +++ b/drivers/staging/rdma/hfi1/chip.h
> @@ -619,7 +619,6 @@ void sbus_request(struct hfi1_devdata *dd,
>  		  u8 receiver_addr, u8 data_addr, u8 command, u32 data_in);
>  int sbus_request_slow(struct hfi1_devdata *dd,
>  		      u8 receiver_addr, u8 data_addr, u8 command, u32 data_in);
> -void set_sbus_fast_mode(struct hfi1_devdata *dd);
>  void clear_sbus_fast_mode(struct hfi1_devdata *dd);
>  int hfi1_firmware_init(struct hfi1_devdata *dd);
>  int load_pcie_firmware(struct hfi1_devdata *dd);
> diff --git a/drivers/staging/rdma/hfi1/firmware.c b/drivers/staging/rdma/hfi1/firmware.c
> index 5c2f2ed..0ce9a16 100644
> --- a/drivers/staging/rdma/hfi1/firmware.c
> +++ b/drivers/staging/rdma/hfi1/firmware.c
> @@ -987,7 +987,8 @@ void fabric_serdes_reset(struct hfi1_devdata *dd)
>  	ra = fabric_serdes_broadcast[dd->hfi1_id];
>  
>  	acquire_hw_mutex(dd);
> -	set_sbus_fast_mode(dd);
> +	write_csr(dd, ASIC_CFG_SBUS_EXECUTE,
> +				ASIC_CFG_SBUS_EXECUTE_FAST_MODE_SMASK);
>  	/* place SerDes in reset and disable SPICO */
>  	sbus_request(dd, ra, 0x07, WRITE_SBUS_RECEIVER, 0x00000011);
>  	/* wait 100 refclk cycles @ 156.25MHz => 640ns */
> @@ -1214,12 +1215,6 @@ void release_hw_mutex(struct hfi1_devdata *dd)
>  	write_csr(dd, ASIC_CFG_MUTEX, 0);
>  }
>  
> -void set_sbus_fast_mode(struct hfi1_devdata *dd)
> -{
> -	write_csr(dd, ASIC_CFG_SBUS_EXECUTE,
> -				ASIC_CFG_SBUS_EXECUTE_FAST_MODE_SMASK);
> -}
> -
>  void clear_sbus_fast_mode(struct hfi1_devdata *dd)

set_sbus_fast_mode seems to be the pair of clear_sbus_fast_mode just below 
it, so it's not clear that it is a good idea to remove one and leave the 
other.

Also, when you put the body of a function in the place of the call site, 
you need to clean up the indentation.  In the call site at the top of the 
patch, there is one more tab than needed.

julia

>  {
>  	u64 reg, count = 0;
> @@ -1244,7 +1239,8 @@ int load_firmware(struct hfi1_devdata *dd)
>  		if (ret)
>  			return ret;
>  
> -		set_sbus_fast_mode(dd);
> +		write_csr(dd, ASIC_CFG_SBUS_EXECUTE,
> +				ASIC_CFG_SBUS_EXECUTE_FAST_MODE_SMASK);
>  
>  		/*
>  		 * The SBus contains part of the fabric firmware and so must
> @@ -1583,7 +1579,8 @@ int load_pcie_firmware(struct hfi1_devdata *dd)
>  	int ret = 0;
>  
>  	/* both firmware loads below use the SBus */
> -	set_sbus_fast_mode(dd);
> +	write_csr(dd, ASIC_CFG_SBUS_EXECUTE,
> +				ASIC_CFG_SBUS_EXECUTE_FAST_MODE_SMASK);
>  
>  	if (fw_sbus_load) {
>  		turn_off_spicos(dd, SPICO_SBUS);
> diff --git a/drivers/staging/rdma/hfi1/pcie.c b/drivers/staging/rdma/hfi1/pcie.c
> index e6773b3..10596b1 100644
> --- a/drivers/staging/rdma/hfi1/pcie.c
> +++ b/drivers/staging/rdma/hfi1/pcie.c
> @@ -770,7 +770,8 @@ static void pcie_post_steps(struct hfi1_devdata *dd)
>  {
>  	int i;
>  
> -	set_sbus_fast_mode(dd);
> +	write_csr(dd, ASIC_CFG_SBUS_EXECUTE,
> +				ASIC_CFG_SBUS_EXECUTE_FAST_MODE_SMASK);
>  	/*
>  	 * Write to the PCIe PCSes to set the G3_LOCKED_NEXT bits to 1.
>  	 * This avoids a spurious framing error that can otherwise be
> -- 
> 1.9.1
> 
> -- 
> You received this message because you are subscribed to the Google Groups "outreachy-kernel" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to outreachy-kernel+unsubscribe@googlegroups.com.
> To post to this group, send email to outreachy-kernel@googlegroups.com.
> To view this discussion on the web visit https://groups.google.com/d/msgid/outreachy-kernel/5cd4f403201b3e1c6b0f44429a5203b3447e2d27.1446366036.git.amitoj1606%40gmail.com.
> For more options, visit https://groups.google.com/d/optout.
> 

Re: [Outreachy kernel] [PATCH 1/5] staging: rdma: hfi1: Remove hfi1_nomsix() wrapper function

[Julia Lawall]

On Sun, 1 Nov 2015, Amitoj Kaur Chawla wrote:

> Drop wrapper function hfi1_nomsix() that can be replaced by a single line 
> of code.
> 
> Signed-off-by: Amitoj Kaur Chawla <amitoj1606@gmail.com>
> ---
>  drivers/staging/rdma/hfi1/chip.c | 2 +-
>  drivers/staging/rdma/hfi1/hfi.h  | 1 -
>  drivers/staging/rdma/hfi1/pcie.c | 8 --------
>  3 files changed, 1 insertion(+), 10 deletions(-)
> 
> diff --git a/drivers/staging/rdma/hfi1/chip.c b/drivers/staging/rdma/hfi1/chip.c
> index aa58e59..beac326d 100644
> --- a/drivers/staging/rdma/hfi1/chip.c
> +++ b/drivers/staging/rdma/hfi1/chip.c
> @@ -8694,7 +8694,7 @@ static void clean_up_interrupts(struct hfi1_devdata *dd)
>  	/* turn off interrupts */
>  	if (dd->num_msix_entries) {
>  		/* MSI-X */
> -		hfi1_nomsix(dd);
> +		pci_disable_msix(dd->pcidev);

This one is probably OK.  pci_disable_msix is a standard kernel function, 
and the name gives pretty much the same information as the name of the 
driver-specific function (and is even easier to parse).

julia

>  	} else {
>  		/* INTx */
>  		disable_intx(dd->pcidev);
> diff --git a/drivers/staging/rdma/hfi1/hfi.h b/drivers/staging/rdma/hfi1/hfi.h
> index 8ca171b..1f97124 100644
> --- a/drivers/staging/rdma/hfi1/hfi.h
> +++ b/drivers/staging/rdma/hfi1/hfi.h
> @@ -1600,7 +1600,6 @@ void hfi1_pcie_flr(struct hfi1_devdata *);
>  int pcie_speeds(struct hfi1_devdata *);
>  void request_msix(struct hfi1_devdata *, u32 *, struct hfi1_msix_entry *);
>  void hfi1_enable_intx(struct pci_dev *);
> -void hfi1_nomsix(struct hfi1_devdata *);
>  void restore_pci_variables(struct hfi1_devdata *dd);
>  int do_pcie_gen3_transition(struct hfi1_devdata *dd);
>  int parse_platform_config(struct hfi1_devdata *dd);
> diff --git a/drivers/staging/rdma/hfi1/pcie.c b/drivers/staging/rdma/hfi1/pcie.c
> index ac5653c..e6773b3 100644
> --- a/drivers/staging/rdma/hfi1/pcie.c
> +++ b/drivers/staging/rdma/hfi1/pcie.c
> @@ -426,14 +426,6 @@ void request_msix(struct hfi1_devdata *dd, u32 *nent,
>  	tune_pcie_caps(dd);
>  }
>  
> -/*
> - * Disable MSI-X.
> - */
> -void hfi1_nomsix(struct hfi1_devdata *dd)
> -{
> -	pci_disable_msix(dd->pcidev);
> -}
> -
>  void hfi1_enable_intx(struct pci_dev *pdev)
>  {
>  	/* first, turn on INTx */
> -- 
> 1.9.1
> 
> -- 
> You received this message because you are subscribed to the Google Groups "outreachy-kernel" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to outreachy-kernel+unsubscribe@googlegroups.com.
> To post to this group, send email to outreachy-kernel@googlegroups.com.
> To view this discussion on the web visit https://groups.google.com/d/msgid/outreachy-kernel/e9e769aacb2c1db52a7a1eb523fa2d73a8f6d09c.1446366036.git.amitoj1606%40gmail.com.
> For more options, visit https://groups.google.com/d/optout.
> 

Re: [Outreachy kernel] [PATCH 3/5] staging: rdma: hfi1: chip: Remove wrapper function

[Amitoj Kaur Chawla]

On Sun, Nov 1, 2015 at 2:11 PM, Julia Lawall <julia.lawall@lip6.fr> wrote:
> On Sun, 1 Nov 2015, Amitoj Kaur Chawla wrote:
>
>> Drop wrapper function remap_receive_available_interrupt() that can be
>> replaced by a single line of code.
>>
>> Signed-off-by: Amitoj Kaur Chawla <amitoj1606@gmail.com>
>> ---
>>  drivers/staging/rdma/hfi1/chip.c | 8 +-------
>>  1 file changed, 1 insertion(+), 7 deletions(-)
>>
>> diff --git a/drivers/staging/rdma/hfi1/chip.c b/drivers/staging/rdma/hfi1/chip.c
>> index beac326d..a9c6abf 100644
>> --- a/drivers/staging/rdma/hfi1/chip.c
>> +++ b/drivers/staging/rdma/hfi1/chip.c
>> @@ -8749,12 +8749,6 @@ static void remap_sdma_interrupts(struct hfi1_devdata *dd,
>>               msix_intr);
>>  }
>>
>> -static void remap_receive_available_interrupt(struct hfi1_devdata *dd,
>> -                                           int rx, int msix_intr)
>> -{
>> -     remap_intr(dd, IS_RCVAVAIL_START + rx, msix_intr);
>> -}
>
> remap_intr is not a standard kernel function.  But perhaps one could argue
> that the constant name gives the same information as the function name, so
> the function is not needed.
>
> But this function is the neighbor of another very similar function that
> remaps three interrupts at once.  The two functions are also used in a
> very parallel way, at the end of successive if branches.  It seems strange
> to inline one function and not the other.
>
> julia

In this case, the function names are very similar and the wrapper just
add the macro IS_RCVAVAIL_START to the second argument, so the change
made sense to me.
>
>>  static int request_intx_irq(struct hfi1_devdata *dd)
>>  {
>>       int ret;
>> @@ -8890,7 +8884,7 @@ static int request_msix_irqs(struct hfi1_devdata *dd)
>>                       snprintf(me->name, sizeof(me->name),
>>                               DRIVER_NAME"_%d kctxt%d", dd->unit, idx);
>>                       err_info = "receive context";
>> -                     remap_receive_available_interrupt(dd, idx, i);
>> +                     remap_intr(dd, IS_RCVAVAIL_START + idx, i);
>>               } else {
>>                       /* not in our expected range - complain, then
>>                          ignore it */
>> --
>> 1.9.1
>>
>> --
>> You received this message because you are subscribed to the Google Groups "outreachy-kernel" group.
>> To unsubscribe from this group and stop receiving emails from it, send an email to outreachy-kernel+unsubscribe@googlegroups.com.
>> To post to this group, send email to outreachy-kernel@googlegroups.com.
>> To view this discussion on the web visit https://groups.google.com/d/msgid/outreachy-kernel/23ac38a01d5092053da227aaa0741b270125ceeb.1446366036.git.amitoj1606%40gmail.com.
>> For more options, visit https://groups.google.com/d/optout.
>>



-- 
Amitoj

Re: [Outreachy kernel] [PATCH 2/5] staging: rdma: hfi1: Remove set_sbus_fast_mode() wrapper function

[Amitoj Kaur Chawla]

On Sun, Nov 1, 2015 at 2:14 PM, Julia Lawall <julia.lawall@lip6.fr> wrote:
> On Sun, 1 Nov 2015, Amitoj Kaur Chawla wrote:
>
>> Drop set_sbus_fast_mode() wrapper function that can be replaced by
>> a single line of code.
>>
>> Signed-off-by: Amitoj Kaur Chawla <amitoj1606@gmail.com>
>> ---
>>  drivers/staging/rdma/hfi1/chip.h     |  1 -
>>  drivers/staging/rdma/hfi1/firmware.c | 15 ++++++---------
>>  drivers/staging/rdma/hfi1/pcie.c     |  3 ++-
>>  3 files changed, 8 insertions(+), 11 deletions(-)
>>
>> diff --git a/drivers/staging/rdma/hfi1/chip.h b/drivers/staging/rdma/hfi1/chip.h
>> index f89a432..092fc9d 100644
>> --- a/drivers/staging/rdma/hfi1/chip.h
>> +++ b/drivers/staging/rdma/hfi1/chip.h
>> @@ -619,7 +619,6 @@ void sbus_request(struct hfi1_devdata *dd,
>>                 u8 receiver_addr, u8 data_addr, u8 command, u32 data_in);
>>  int sbus_request_slow(struct hfi1_devdata *dd,
>>                     u8 receiver_addr, u8 data_addr, u8 command, u32 data_in);
>> -void set_sbus_fast_mode(struct hfi1_devdata *dd);
>>  void clear_sbus_fast_mode(struct hfi1_devdata *dd);
>>  int hfi1_firmware_init(struct hfi1_devdata *dd);
>>  int load_pcie_firmware(struct hfi1_devdata *dd);
>> diff --git a/drivers/staging/rdma/hfi1/firmware.c b/drivers/staging/rdma/hfi1/firmware.c
>> index 5c2f2ed..0ce9a16 100644
>> --- a/drivers/staging/rdma/hfi1/firmware.c
>> +++ b/drivers/staging/rdma/hfi1/firmware.c
>> @@ -987,7 +987,8 @@ void fabric_serdes_reset(struct hfi1_devdata *dd)
>>       ra = fabric_serdes_broadcast[dd->hfi1_id];
>>
>>       acquire_hw_mutex(dd);
>> -     set_sbus_fast_mode(dd);
>> +     write_csr(dd, ASIC_CFG_SBUS_EXECUTE,
>> +                             ASIC_CFG_SBUS_EXECUTE_FAST_MODE_SMASK);
>>       /* place SerDes in reset and disable SPICO */
>>       sbus_request(dd, ra, 0x07, WRITE_SBUS_RECEIVER, 0x00000011);
>>       /* wait 100 refclk cycles @ 156.25MHz => 640ns */
>> @@ -1214,12 +1215,6 @@ void release_hw_mutex(struct hfi1_devdata *dd)
>>       write_csr(dd, ASIC_CFG_MUTEX, 0);
>>  }
>>
>> -void set_sbus_fast_mode(struct hfi1_devdata *dd)
>> -{
>> -     write_csr(dd, ASIC_CFG_SBUS_EXECUTE,
>> -                             ASIC_CFG_SBUS_EXECUTE_FAST_MODE_SMASK);
>> -}
>> -
>>  void clear_sbus_fast_mode(struct hfi1_devdata *dd)
>
> set_sbus_fast_mode seems to be the pair of clear_sbus_fast_mode just below
> it, so it's not clear that it is a good idea to remove one and leave the
> other.
>

clear_sbus_fast_mode() is not a wrapper function. So I didn't remove
it. But if you think one change without the other is pointless then I
can drop this.

-- 
Amitoj

Re: [Outreachy kernel] [PATCH 2/5] staging: rdma: hfi1: Remove set_sbus_fast_mode() wrapper function

[Julia Lawall]

On Sun, 1 Nov 2015, Amitoj Kaur Chawla wrote:

> On Sun, Nov 1, 2015 at 2:14 PM, Julia Lawall <julia.lawall@lip6.fr> wrote:
> > On Sun, 1 Nov 2015, Amitoj Kaur Chawla wrote:
> >
> >> Drop set_sbus_fast_mode() wrapper function that can be replaced by
> >> a single line of code.
> >>
> >> Signed-off-by: Amitoj Kaur Chawla <amitoj1606@gmail.com>
> >> ---
> >>  drivers/staging/rdma/hfi1/chip.h     |  1 -
> >>  drivers/staging/rdma/hfi1/firmware.c | 15 ++++++---------
> >>  drivers/staging/rdma/hfi1/pcie.c     |  3 ++-
> >>  3 files changed, 8 insertions(+), 11 deletions(-)
> >>
> >> diff --git a/drivers/staging/rdma/hfi1/chip.h b/drivers/staging/rdma/hfi1/chip.h
> >> index f89a432..092fc9d 100644
> >> --- a/drivers/staging/rdma/hfi1/chip.h
> >> +++ b/drivers/staging/rdma/hfi1/chip.h
> >> @@ -619,7 +619,6 @@ void sbus_request(struct hfi1_devdata *dd,
> >>                 u8 receiver_addr, u8 data_addr, u8 command, u32 data_in);
> >>  int sbus_request_slow(struct hfi1_devdata *dd,
> >>                     u8 receiver_addr, u8 data_addr, u8 command, u32 data_in);
> >> -void set_sbus_fast_mode(struct hfi1_devdata *dd);
> >>  void clear_sbus_fast_mode(struct hfi1_devdata *dd);
> >>  int hfi1_firmware_init(struct hfi1_devdata *dd);
> >>  int load_pcie_firmware(struct hfi1_devdata *dd);
> >> diff --git a/drivers/staging/rdma/hfi1/firmware.c b/drivers/staging/rdma/hfi1/firmware.c
> >> index 5c2f2ed..0ce9a16 100644
> >> --- a/drivers/staging/rdma/hfi1/firmware.c
> >> +++ b/drivers/staging/rdma/hfi1/firmware.c
> >> @@ -987,7 +987,8 @@ void fabric_serdes_reset(struct hfi1_devdata *dd)
> >>       ra = fabric_serdes_broadcast[dd->hfi1_id];
> >>
> >>       acquire_hw_mutex(dd);
> >> -     set_sbus_fast_mode(dd);
> >> +     write_csr(dd, ASIC_CFG_SBUS_EXECUTE,
> >> +                             ASIC_CFG_SBUS_EXECUTE_FAST_MODE_SMASK);
> >>       /* place SerDes in reset and disable SPICO */
> >>       sbus_request(dd, ra, 0x07, WRITE_SBUS_RECEIVER, 0x00000011);
> >>       /* wait 100 refclk cycles @ 156.25MHz => 640ns */
> >> @@ -1214,12 +1215,6 @@ void release_hw_mutex(struct hfi1_devdata *dd)
> >>       write_csr(dd, ASIC_CFG_MUTEX, 0);
> >>  }
> >>
> >> -void set_sbus_fast_mode(struct hfi1_devdata *dd)
> >> -{
> >> -     write_csr(dd, ASIC_CFG_SBUS_EXECUTE,
> >> -                             ASIC_CFG_SBUS_EXECUTE_FAST_MODE_SMASK);
> >> -}
> >> -
> >>  void clear_sbus_fast_mode(struct hfi1_devdata *dd)
> >
> > set_sbus_fast_mode seems to be the pair of clear_sbus_fast_mode just below
> > it, so it's not clear that it is a good idea to remove one and leave the
> > other.
> >
> 
> clear_sbus_fast_mode() is not a wrapper function. So I didn't remove
> it. But if you think one change without the other is pointless then I
> can drop this.

Pointless is not quite the right word.  I think that it hurts readability 
to drop one without the other.  So I think it would be better to drop it.

julia

Re: [Outreachy kernel] [PATCH 4/5] staging: rdma: hfi1: chip: Remove wrapper functions

[Amitoj Kaur Chawla]

On Sun, Nov 1, 2015 at 2:05 PM, Julia Lawall <julia.lawall@lip6.fr> wrote:
> On Sun, 1 Nov 2015, Amitoj Kaur Chawla wrote:
>
>> Drop wrapper functions add_rcvctrl() and clear_rcvctrl() that can be replaced by a single line of code.
>>
>> Signed-off-by: Amitoj Kaur Chawla <amitoj1606@gmail.com>
>> ---
>>  drivers/staging/rdma/hfi1/chip.c | 30 ++++++++++--------------------
>>  1 file changed, 10 insertions(+), 20 deletions(-)
>>
>> diff --git a/drivers/staging/rdma/hfi1/chip.c b/drivers/staging/rdma/hfi1/chip.c
>> index a9c6abf..23cacbf 100644
>> --- a/drivers/staging/rdma/hfi1/chip.c
>> +++ b/drivers/staging/rdma/hfi1/chip.c
>> @@ -3176,16 +3176,6 @@ static void adjust_rcvctrl(struct hfi1_devdata *dd, u64 add, u64 clear)
>>       spin_unlock_irqrestore(&dd->rcvctrl_lock, flags);
>>  }
>>
>> -static inline void add_rcvctrl(struct hfi1_devdata *dd, u64 add)
>> -{
>> -     adjust_rcvctrl(dd, add, 0);
>> -}
>> -
>> -static inline void clear_rcvctrl(struct hfi1_devdata *dd, u64 clear)
>> -{
>> -     adjust_rcvctrl(dd, 0, clear);
>> -}
>
> Is it really an improvement?  adjust_rcvctrl is not a standard kernel
> function.  In the original code, one can undestand what add_rcvctrl and
> clear_rcvctrl do based on their names.  In the modified code, you have to
> know what is the significance of the different argument positions of
> adjust_rcvctrl, so you have to go look at the definition.  So this makes
> more work for someone who wants to understand the code.
>
> Wrapping standard kernel functions is not good.  And wrapping when the
> arguments don't change and the name is almost the same is probably not
> good either.  But here the wrapping would seem to add some value.
>
> julia

Since both were using the same function and one would have to look at
adjust_rcvctrl to understand what add_rcvctrl and clear_rcvctrl are
doing, I thought this would work by removing that extra 2 functions.

-- 
Amitoj

Re: [Outreachy kernel] [PATCH 2/5] staging: rdma: hfi1: Remove set_sbus_fast_mode() wrapper function

[Amitoj Kaur Chawla]

On Sun, Nov 1, 2015 at 2:36 PM, Julia Lawall <julia.lawall@lip6.fr> wrote:
>
>
> On Sun, 1 Nov 2015, Amitoj Kaur Chawla wrote:
>
>> On Sun, Nov 1, 2015 at 2:14 PM, Julia Lawall <julia.lawall@lip6.fr> wrote:
>> > On Sun, 1 Nov 2015, Amitoj Kaur Chawla wrote:
>> >
>> >> Drop set_sbus_fast_mode() wrapper function that can be replaced by
>> >> a single line of code.
>> >>
>> >> Signed-off-by: Amitoj Kaur Chawla <amitoj1606@gmail.com>
>> >> ---
>> >>  drivers/staging/rdma/hfi1/chip.h     |  1 -
>> >>  drivers/staging/rdma/hfi1/firmware.c | 15 ++++++---------
>> >>  drivers/staging/rdma/hfi1/pcie.c     |  3 ++-
>> >>  3 files changed, 8 insertions(+), 11 deletions(-)
>> >>
>> >> diff --git a/drivers/staging/rdma/hfi1/chip.h b/drivers/staging/rdma/hfi1/chip.h
>> >> index f89a432..092fc9d 100644
>> >> --- a/drivers/staging/rdma/hfi1/chip.h
>> >> +++ b/drivers/staging/rdma/hfi1/chip.h
>> >> @@ -619,7 +619,6 @@ void sbus_request(struct hfi1_devdata *dd,
>> >>                 u8 receiver_addr, u8 data_addr, u8 command, u32 data_in);
>> >>  int sbus_request_slow(struct hfi1_devdata *dd,
>> >>                     u8 receiver_addr, u8 data_addr, u8 command, u32 data_in);
>> >> -void set_sbus_fast_mode(struct hfi1_devdata *dd);
>> >>  void clear_sbus_fast_mode(struct hfi1_devdata *dd);
>> >>  int hfi1_firmware_init(struct hfi1_devdata *dd);
>> >>  int load_pcie_firmware(struct hfi1_devdata *dd);
>> >> diff --git a/drivers/staging/rdma/hfi1/firmware.c b/drivers/staging/rdma/hfi1/firmware.c
>> >> index 5c2f2ed..0ce9a16 100644
>> >> --- a/drivers/staging/rdma/hfi1/firmware.c
>> >> +++ b/drivers/staging/rdma/hfi1/firmware.c
>> >> @@ -987,7 +987,8 @@ void fabric_serdes_reset(struct hfi1_devdata *dd)
>> >>       ra = fabric_serdes_broadcast[dd->hfi1_id];
>> >>
>> >>       acquire_hw_mutex(dd);
>> >> -     set_sbus_fast_mode(dd);
>> >> +     write_csr(dd, ASIC_CFG_SBUS_EXECUTE,
>> >> +                             ASIC_CFG_SBUS_EXECUTE_FAST_MODE_SMASK);
>> >>       /* place SerDes in reset and disable SPICO */
>> >>       sbus_request(dd, ra, 0x07, WRITE_SBUS_RECEIVER, 0x00000011);
>> >>       /* wait 100 refclk cycles @ 156.25MHz => 640ns */
>> >> @@ -1214,12 +1215,6 @@ void release_hw_mutex(struct hfi1_devdata *dd)
>> >>       write_csr(dd, ASIC_CFG_MUTEX, 0);
>> >>  }
>> >>
>> >> -void set_sbus_fast_mode(struct hfi1_devdata *dd)
>> >> -{
>> >> -     write_csr(dd, ASIC_CFG_SBUS_EXECUTE,
>> >> -                             ASIC_CFG_SBUS_EXECUTE_FAST_MODE_SMASK);
>> >> -}
>> >> -
>> >>  void clear_sbus_fast_mode(struct hfi1_devdata *dd)
>> >
>> > set_sbus_fast_mode seems to be the pair of clear_sbus_fast_mode just below
>> > it, so it's not clear that it is a good idea to remove one and leave the
>> > other.
>> >
>>
>> clear_sbus_fast_mode() is not a wrapper function. So I didn't remove
>> it. But if you think one change without the other is pointless then I
>> can drop this.
>
> Pointless is not quite the right word.  I think that it hurts readability
> to drop one without the other.  So I think it would be better to drop it.
>
> julia

Okay. Will accordingly send v2 soon.

-- 
Amitoj

Re: [Outreachy kernel] [PATCH 3/5] staging: rdma: hfi1: chip: Remove wrapper function

[Julia Lawall]

On Sun, 1 Nov 2015, Amitoj Kaur Chawla wrote:

> On Sun, Nov 1, 2015 at 2:11 PM, Julia Lawall <julia.lawall@lip6.fr> wrote:
> > On Sun, 1 Nov 2015, Amitoj Kaur Chawla wrote:
> >
> >> Drop wrapper function remap_receive_available_interrupt() that can be
> >> replaced by a single line of code.
> >>
> >> Signed-off-by: Amitoj Kaur Chawla <amitoj1606@gmail.com>
> >> ---
> >>  drivers/staging/rdma/hfi1/chip.c | 8 +-------
> >>  1 file changed, 1 insertion(+), 7 deletions(-)
> >>
> >> diff --git a/drivers/staging/rdma/hfi1/chip.c b/drivers/staging/rdma/hfi1/chip.c
> >> index beac326d..a9c6abf 100644
> >> --- a/drivers/staging/rdma/hfi1/chip.c
> >> +++ b/drivers/staging/rdma/hfi1/chip.c
> >> @@ -8749,12 +8749,6 @@ static void remap_sdma_interrupts(struct hfi1_devdata *dd,
> >>               msix_intr);
> >>  }
> >>
> >> -static void remap_receive_available_interrupt(struct hfi1_devdata *dd,
> >> -                                           int rx, int msix_intr)
> >> -{
> >> -     remap_intr(dd, IS_RCVAVAIL_START + rx, msix_intr);
> >> -}
> >
> > remap_intr is not a standard kernel function.  But perhaps one could argue
> > that the constant name gives the same information as the function name, so
> > the function is not needed.
> >
> > But this function is the neighbor of another very similar function that
> > remaps three interrupts at once.  The two functions are also used in a
> > very parallel way, at the end of successive if branches.  It seems strange
> > to inline one function and not the other.
> >
> > julia
> 
> In this case, the function names are very similar and the wrapper just
> add the macro IS_RCVAVAIL_START to the second argument, so the change
> made sense to me.

Personally, I would find the lack of parallelism confusing.  
But one could have an opinion either way on this one.

julia

> >
> >>  static int request_intx_irq(struct hfi1_devdata *dd)
> >>  {
> >>       int ret;
> >> @@ -8890,7 +8884,7 @@ static int request_msix_irqs(struct hfi1_devdata *dd)
> >>                       snprintf(me->name, sizeof(me->name),
> >>                               DRIVER_NAME"_%d kctxt%d", dd->unit, idx);
> >>                       err_info = "receive context";
> >> -                     remap_receive_available_interrupt(dd, idx, i);
> >> +                     remap_intr(dd, IS_RCVAVAIL_START + idx, i);
> >>               } else {
> >>                       /* not in our expected range - complain, then
> >>                          ignore it */
> >> --
> >> 1.9.1
> >>
> >> --
> >> You received this message because you are subscribed to the Google Groups "outreachy-kernel" group.
> >> To unsubscribe from this group and stop receiving emails from it, send an email to outreachy-kernel+unsubscribe@googlegroups.com.
> >> To post to this group, send email to outreachy-kernel@googlegroups.com.
> >> To view this discussion on the web visit https://groups.google.com/d/msgid/outreachy-kernel/23ac38a01d5092053da227aaa0741b270125ceeb.1446366036.git.amitoj1606%40gmail.com.
> >> For more options, visit https://groups.google.com/d/optout.
> >>
> 
> 
> 
> -- 
> Amitoj
> 

Re: [Outreachy kernel] [PATCH 4/5] staging: rdma: hfi1: chip: Remove wrapper functions

[Julia Lawall]

On Sun, 1 Nov 2015, Amitoj Kaur Chawla wrote:

> On Sun, Nov 1, 2015 at 2:05 PM, Julia Lawall <julia.lawall@lip6.fr> wrote:
> > On Sun, 1 Nov 2015, Amitoj Kaur Chawla wrote:
> >
> >> Drop wrapper functions add_rcvctrl() and clear_rcvctrl() that can be replaced by a single line of code.
> >>
> >> Signed-off-by: Amitoj Kaur Chawla <amitoj1606@gmail.com>
> >> ---
> >>  drivers/staging/rdma/hfi1/chip.c | 30 ++++++++++--------------------
> >>  1 file changed, 10 insertions(+), 20 deletions(-)
> >>
> >> diff --git a/drivers/staging/rdma/hfi1/chip.c b/drivers/staging/rdma/hfi1/chip.c
> >> index a9c6abf..23cacbf 100644
> >> --- a/drivers/staging/rdma/hfi1/chip.c
> >> +++ b/drivers/staging/rdma/hfi1/chip.c
> >> @@ -3176,16 +3176,6 @@ static void adjust_rcvctrl(struct hfi1_devdata *dd, u64 add, u64 clear)
> >>       spin_unlock_irqrestore(&dd->rcvctrl_lock, flags);
> >>  }
> >>
> >> -static inline void add_rcvctrl(struct hfi1_devdata *dd, u64 add)
> >> -{
> >> -     adjust_rcvctrl(dd, add, 0);
> >> -}
> >> -
> >> -static inline void clear_rcvctrl(struct hfi1_devdata *dd, u64 clear)
> >> -{
> >> -     adjust_rcvctrl(dd, 0, clear);
> >> -}
> >
> > Is it really an improvement?  adjust_rcvctrl is not a standard kernel
> > function.  In the original code, one can undestand what add_rcvctrl and
> > clear_rcvctrl do based on their names.  In the modified code, you have to
> > know what is the significance of the different argument positions of
> > adjust_rcvctrl, so you have to go look at the definition.  So this makes
> > more work for someone who wants to understand the code.
> >
> > Wrapping standard kernel functions is not good.  And wrapping when the
> > arguments don't change and the name is almost the same is probably not
> > good either.  But here the wrapping would seem to add some value.
> >
> > julia
> 
> Since both were using the same function and one would have to look at
> adjust_rcvctrl to understand what add_rcvctrl and clear_rcvctrl are
> doing, I thought this would work by removing that extra 2 functions.

At a high level, one knows that one adds something and one clears the same 
thing.  Adding and clearing are understandable concepts, so to get a 
minimum understanding of the behavior, one doesn't have to look at the 
definition.

julia

Re: [Outreachy kernel] [PATCH 3/5] staging: rdma: hfi1: chip: Remove wrapper function

[Amitoj Kaur Chawla]

On Sun, Nov 1, 2015 at 2:38 PM, Julia Lawall <julia.lawall@lip6.fr> wrote:
> On Sun, 1 Nov 2015, Amitoj Kaur Chawla wrote:
>
>> On Sun, Nov 1, 2015 at 2:11 PM, Julia Lawall <julia.lawall@lip6.fr> wrote:
>> > On Sun, 1 Nov 2015, Amitoj Kaur Chawla wrote:
>> >
>> >> Drop wrapper function remap_receive_available_interrupt() that can be
>> >> replaced by a single line of code.
>> >>
>> >> Signed-off-by: Amitoj Kaur Chawla <amitoj1606@gmail.com>
>> >> ---
>> >>  drivers/staging/rdma/hfi1/chip.c | 8 +-------
>> >>  1 file changed, 1 insertion(+), 7 deletions(-)
>> >>
>> >> diff --git a/drivers/staging/rdma/hfi1/chip.c b/drivers/staging/rdma/hfi1/chip.c
>> >> index beac326d..a9c6abf 100644
>> >> --- a/drivers/staging/rdma/hfi1/chip.c
>> >> +++ b/drivers/staging/rdma/hfi1/chip.c
>> >> @@ -8749,12 +8749,6 @@ static void remap_sdma_interrupts(struct hfi1_devdata *dd,
>> >>               msix_intr);
>> >>  }
>> >>
>> >> -static void remap_receive_available_interrupt(struct hfi1_devdata *dd,
>> >> -                                           int rx, int msix_intr)
>> >> -{
>> >> -     remap_intr(dd, IS_RCVAVAIL_START + rx, msix_intr);
>> >> -}
>> >
>> > remap_intr is not a standard kernel function.  But perhaps one could argue
>> > that the constant name gives the same information as the function name, so
>> > the function is not needed.
>> >
>> > But this function is the neighbor of another very similar function that
>> > remaps three interrupts at once.  The two functions are also used in a
>> > very parallel way, at the end of successive if branches.  It seems strange
>> > to inline one function and not the other.
>> >
>> > julia
>>
>> In this case, the function names are very similar and the wrapper just
>> add the macro IS_RCVAVAIL_START to the second argument, so the change
>> made sense to me.
>
> Personally, I would find the lack of parallelism confusing.
> But one could have an opinion either way on this one.
>
> julia
>

So I should leave this as is or drop it? Confused.

-- 
Amitoj

Re: [Outreachy kernel] [PATCH 4/5] staging: rdma: hfi1: chip: Remove wrapper functions

[Julia Lawall]

On Sun, 1 Nov 2015, Amitoj Kaur Chawla wrote:

> On Sun, Nov 1, 2015 at 2:05 PM, Julia Lawall <julia.lawall@lip6.fr> wrote:
> > On Sun, 1 Nov 2015, Amitoj Kaur Chawla wrote:
> >
> >> Drop wrapper functions add_rcvctrl() and clear_rcvctrl() that can be replaced by a single line of code.
> >>
> >> Signed-off-by: Amitoj Kaur Chawla <amitoj1606@gmail.com>
> >> ---
> >>  drivers/staging/rdma/hfi1/chip.c | 30 ++++++++++--------------------
> >>  1 file changed, 10 insertions(+), 20 deletions(-)
> >>
> >> diff --git a/drivers/staging/rdma/hfi1/chip.c b/drivers/staging/rdma/hfi1/chip.c
> >> index a9c6abf..23cacbf 100644
> >> --- a/drivers/staging/rdma/hfi1/chip.c
> >> +++ b/drivers/staging/rdma/hfi1/chip.c
> >> @@ -3176,16 +3176,6 @@ static void adjust_rcvctrl(struct hfi1_devdata *dd, u64 add, u64 clear)
> >>       spin_unlock_irqrestore(&dd->rcvctrl_lock, flags);
> >>  }
> >>
> >> -static inline void add_rcvctrl(struct hfi1_devdata *dd, u64 add)
> >> -{
> >> -     adjust_rcvctrl(dd, add, 0);
> >> -}
> >> -
> >> -static inline void clear_rcvctrl(struct hfi1_devdata *dd, u64 clear)
> >> -{
> >> -     adjust_rcvctrl(dd, 0, clear);
> >> -}
> >
> > Is it really an improvement?  adjust_rcvctrl is not a standard kernel
> > function.  In the original code, one can undestand what add_rcvctrl and
> > clear_rcvctrl do based on their names.  In the modified code, you have to
> > know what is the significance of the different argument positions of
> > adjust_rcvctrl, so you have to go look at the definition.  So this makes
> > more work for someone who wants to understand the code.
> >
> > Wrapping standard kernel functions is not good.  And wrapping when the
> > arguments don't change and the name is almost the same is probably not
> > good either.  But here the wrapping would seem to add some value.
> >
> > julia
> 
> Since both were using the same function and one would have to look at
> adjust_rcvctrl to understand what add_rcvctrl and clear_rcvctrl are
> doing, I thought this would work by removing that extra 2 functions.

In any case, all of this discussion shows that the commit message needs to 
be better.  Just saying that something is a one-line function is not 
enough justification to remove it.  There needs to be an explanation about 
why the code will be improved as a result.

julia

Re: [Outreachy kernel] [PATCH 4/5] staging: rdma: hfi1: chip: Remove wrapper functions

[Amitoj Kaur Chawla]

On Sun, Nov 1, 2015 at 2:41 PM, Julia Lawall <julia.lawall@lip6.fr> wrote:
>
>
> On Sun, 1 Nov 2015, Amitoj Kaur Chawla wrote:
>
>> On Sun, Nov 1, 2015 at 2:05 PM, Julia Lawall <julia.lawall@lip6.fr> wrote:
>> > On Sun, 1 Nov 2015, Amitoj Kaur Chawla wrote:
>> >
>> >> Drop wrapper functions add_rcvctrl() and clear_rcvctrl() that can be replaced by a single line of code.
>> >>
>> >> Signed-off-by: Amitoj Kaur Chawla <amitoj1606@gmail.com>
>> >> ---
>> >>  drivers/staging/rdma/hfi1/chip.c | 30 ++++++++++--------------------
>> >>  1 file changed, 10 insertions(+), 20 deletions(-)
>> >>
>> >> diff --git a/drivers/staging/rdma/hfi1/chip.c b/drivers/staging/rdma/hfi1/chip.c
>> >> index a9c6abf..23cacbf 100644
>> >> --- a/drivers/staging/rdma/hfi1/chip.c
>> >> +++ b/drivers/staging/rdma/hfi1/chip.c
>> >> @@ -3176,16 +3176,6 @@ static void adjust_rcvctrl(struct hfi1_devdata *dd, u64 add, u64 clear)
>> >>       spin_unlock_irqrestore(&dd->rcvctrl_lock, flags);
>> >>  }
>> >>
>> >> -static inline void add_rcvctrl(struct hfi1_devdata *dd, u64 add)
>> >> -{
>> >> -     adjust_rcvctrl(dd, add, 0);
>> >> -}
>> >> -
>> >> -static inline void clear_rcvctrl(struct hfi1_devdata *dd, u64 clear)
>> >> -{
>> >> -     adjust_rcvctrl(dd, 0, clear);
>> >> -}
>> >
>> > Is it really an improvement?  adjust_rcvctrl is not a standard kernel
>> > function.  In the original code, one can undestand what add_rcvctrl and
>> > clear_rcvctrl do based on their names.  In the modified code, you have to
>> > know what is the significance of the different argument positions of
>> > adjust_rcvctrl, so you have to go look at the definition.  So this makes
>> > more work for someone who wants to understand the code.
>> >
>> > Wrapping standard kernel functions is not good.  And wrapping when the
>> > arguments don't change and the name is almost the same is probably not
>> > good either.  But here the wrapping would seem to add some value.
>> >
>> > julia
>>
>> Since both were using the same function and one would have to look at
>> adjust_rcvctrl to understand what add_rcvctrl and clear_rcvctrl are
>> doing, I thought this would work by removing that extra 2 functions.
>
> In any case, all of this discussion shows that the commit message needs to
> be better.  Just saying that something is a one-line function is not
> enough justification to remove it.  There needs to be an explanation about
> why the code will be improved as a result.
>
> julia

Okay will further improve on the commit message. And drop this since
add and clear make it more readable.

-- 
Amitoj

Re: [Outreachy kernel] [PATCH 3/5] staging: rdma: hfi1: chip: Remove wrapper function

[Julia Lawall]

On Sun, 1 Nov 2015, Amitoj Kaur Chawla wrote:

> On Sun, Nov 1, 2015 at 2:38 PM, Julia Lawall <julia.lawall@lip6.fr> wrote:
> > On Sun, 1 Nov 2015, Amitoj Kaur Chawla wrote:
> >
> >> On Sun, Nov 1, 2015 at 2:11 PM, Julia Lawall <julia.lawall@lip6.fr> wrote:
> >> > On Sun, 1 Nov 2015, Amitoj Kaur Chawla wrote:
> >> >
> >> >> Drop wrapper function remap_receive_available_interrupt() that can be
> >> >> replaced by a single line of code.
> >> >>
> >> >> Signed-off-by: Amitoj Kaur Chawla <amitoj1606@gmail.com>
> >> >> ---
> >> >>  drivers/staging/rdma/hfi1/chip.c | 8 +-------
> >> >>  1 file changed, 1 insertion(+), 7 deletions(-)
> >> >>
> >> >> diff --git a/drivers/staging/rdma/hfi1/chip.c b/drivers/staging/rdma/hfi1/chip.c
> >> >> index beac326d..a9c6abf 100644
> >> >> --- a/drivers/staging/rdma/hfi1/chip.c
> >> >> +++ b/drivers/staging/rdma/hfi1/chip.c
> >> >> @@ -8749,12 +8749,6 @@ static void remap_sdma_interrupts(struct hfi1_devdata *dd,
> >> >>               msix_intr);
> >> >>  }
> >> >>
> >> >> -static void remap_receive_available_interrupt(struct hfi1_devdata *dd,
> >> >> -                                           int rx, int msix_intr)
> >> >> -{
> >> >> -     remap_intr(dd, IS_RCVAVAIL_START + rx, msix_intr);
> >> >> -}
> >> >
> >> > remap_intr is not a standard kernel function.  But perhaps one could argue
> >> > that the constant name gives the same information as the function name, so
> >> > the function is not needed.
> >> >
> >> > But this function is the neighbor of another very similar function that
> >> > remaps three interrupts at once.  The two functions are also used in a
> >> > very parallel way, at the end of successive if branches.  It seems strange
> >> > to inline one function and not the other.
> >> >
> >> > julia
> >>
> >> In this case, the function names are very similar and the wrapper just
> >> add the macro IS_RCVAVAIL_START to the second argument, so the change
> >> made sense to me.
> >
> > Personally, I would find the lack of parallelism confusing.
> > But one could have an opinion either way on this one.
> >
> > julia
> >
> 
> So I should leave this as is or drop it? Confused.

Up to you in this case.  I can see both points of view.  In one point of 
view, it is nice to have parallel function names.  In another point of 
view, in the case with three operations, a helper function is being 
introduced for a complicated case.  This case is different than set/clear 
because the two functions are not eg opposites of each other.  There are 
just four registers, three that fit together and one that is different.

julia

Re: [Outreachy kernel] [PATCH 3/5] staging: rdma: hfi1: chip: Remove wrapper function

[Amitoj Kaur Chawla]

On Sun, Nov 1, 2015 at 2:43 PM, Julia Lawall <julia.lawall@lip6.fr> wrote:
>
>
> On Sun, 1 Nov 2015, Amitoj Kaur Chawla wrote:
>
>> On Sun, Nov 1, 2015 at 2:38 PM, Julia Lawall <julia.lawall@lip6.fr> wrote:
>> > On Sun, 1 Nov 2015, Amitoj Kaur Chawla wrote:
>> >
>> >> On Sun, Nov 1, 2015 at 2:11 PM, Julia Lawall <julia.lawall@lip6.fr> wrote:
>> >> > On Sun, 1 Nov 2015, Amitoj Kaur Chawla wrote:
>> >> >
>> >> >> Drop wrapper function remap_receive_available_interrupt() that can be
>> >> >> replaced by a single line of code.
>> >> >>
>> >> >> Signed-off-by: Amitoj Kaur Chawla <amitoj1606@gmail.com>
>> >> >> ---
>> >> >>  drivers/staging/rdma/hfi1/chip.c | 8 +-------
>> >> >>  1 file changed, 1 insertion(+), 7 deletions(-)
>> >> >>
>> >> >> diff --git a/drivers/staging/rdma/hfi1/chip.c b/drivers/staging/rdma/hfi1/chip.c
>> >> >> index beac326d..a9c6abf 100644
>> >> >> --- a/drivers/staging/rdma/hfi1/chip.c
>> >> >> +++ b/drivers/staging/rdma/hfi1/chip.c
>> >> >> @@ -8749,12 +8749,6 @@ static void remap_sdma_interrupts(struct hfi1_devdata *dd,
>> >> >>               msix_intr);
>> >> >>  }
>> >> >>
>> >> >> -static void remap_receive_available_interrupt(struct hfi1_devdata *dd,
>> >> >> -                                           int rx, int msix_intr)
>> >> >> -{
>> >> >> -     remap_intr(dd, IS_RCVAVAIL_START + rx, msix_intr);
>> >> >> -}
>> >> >
>> >> > remap_intr is not a standard kernel function.  But perhaps one could argue
>> >> > that the constant name gives the same information as the function name, so
>> >> > the function is not needed.
>> >> >
>> >> > But this function is the neighbor of another very similar function that
>> >> > remaps three interrupts at once.  The two functions are also used in a
>> >> > very parallel way, at the end of successive if branches.  It seems strange
>> >> > to inline one function and not the other.
>> >> >
>> >> > julia
>> >>
>> >> In this case, the function names are very similar and the wrapper just
>> >> add the macro IS_RCVAVAIL_START to the second argument, so the change
>> >> made sense to me.
>> >
>> > Personally, I would find the lack of parallelism confusing.
>> > But one could have an opinion either way on this one.
>> >
>> > julia
>> >
>>
>> So I should leave this as is or drop it? Confused.
>
> Up to you in this case.  I can see both points of view.  In one point of
> view, it is nice to have parallel function names.  In another point of
> view, in the case with three operations, a helper function is being
> introduced for a complicated case.  This case is different than set/clear
> because the two functions are not eg opposites of each other.  There are
> just four registers, three that fit together and one that is different.
>
> julia

I'll send this one as a separate patch , since Greg would then decide
whether the change is worth being made and send the changed patch
series separately.

Thanks,
-- 
Amitoj

Re: [Outreachy kernel] [PATCH 3/5] staging: rdma: hfi1: chip: Remove wrapper function

[Julia Lawall]

On Sun, 1 Nov 2015, Amitoj Kaur Chawla wrote:

> On Sun, Nov 1, 2015 at 2:43 PM, Julia Lawall <julia.lawall@lip6.fr> wrote:
> >
> >
> > On Sun, 1 Nov 2015, Amitoj Kaur Chawla wrote:
> >
> >> On Sun, Nov 1, 2015 at 2:38 PM, Julia Lawall <julia.lawall@lip6.fr> wrote:
> >> > On Sun, 1 Nov 2015, Amitoj Kaur Chawla wrote:
> >> >
> >> >> On Sun, Nov 1, 2015 at 2:11 PM, Julia Lawall <julia.lawall@lip6.fr> wrote:
> >> >> > On Sun, 1 Nov 2015, Amitoj Kaur Chawla wrote:
> >> >> >
> >> >> >> Drop wrapper function remap_receive_available_interrupt() that can be
> >> >> >> replaced by a single line of code.
> >> >> >>
> >> >> >> Signed-off-by: Amitoj Kaur Chawla <amitoj1606@gmail.com>
> >> >> >> ---
> >> >> >>  drivers/staging/rdma/hfi1/chip.c | 8 +-------
> >> >> >>  1 file changed, 1 insertion(+), 7 deletions(-)
> >> >> >>
> >> >> >> diff --git a/drivers/staging/rdma/hfi1/chip.c b/drivers/staging/rdma/hfi1/chip.c
> >> >> >> index beac326d..a9c6abf 100644
> >> >> >> --- a/drivers/staging/rdma/hfi1/chip.c
> >> >> >> +++ b/drivers/staging/rdma/hfi1/chip.c
> >> >> >> @@ -8749,12 +8749,6 @@ static void remap_sdma_interrupts(struct hfi1_devdata *dd,
> >> >> >>               msix_intr);
> >> >> >>  }
> >> >> >>
> >> >> >> -static void remap_receive_available_interrupt(struct hfi1_devdata *dd,
> >> >> >> -                                           int rx, int msix_intr)
> >> >> >> -{
> >> >> >> -     remap_intr(dd, IS_RCVAVAIL_START + rx, msix_intr);
> >> >> >> -}
> >> >> >
> >> >> > remap_intr is not a standard kernel function.  But perhaps one could argue
> >> >> > that the constant name gives the same information as the function name, so
> >> >> > the function is not needed.
> >> >> >
> >> >> > But this function is the neighbor of another very similar function that
> >> >> > remaps three interrupts at once.  The two functions are also used in a
> >> >> > very parallel way, at the end of successive if branches.  It seems strange
> >> >> > to inline one function and not the other.
> >> >> >
> >> >> > julia
> >> >>
> >> >> In this case, the function names are very similar and the wrapper just
> >> >> add the macro IS_RCVAVAIL_START to the second argument, so the change
> >> >> made sense to me.
> >> >
> >> > Personally, I would find the lack of parallelism confusing.
> >> > But one could have an opinion either way on this one.
> >> >
> >> > julia
> >> >
> >>
> >> So I should leave this as is or drop it? Confused.
> >
> > Up to you in this case.  I can see both points of view.  In one point of
> > view, it is nice to have parallel function names.  In another point of
> > view, in the case with three operations, a helper function is being
> > introduced for a complicated case.  This case is different than set/clear
> > because the two functions are not eg opposites of each other.  There are
> > just four registers, three that fit together and one that is different.
> >
> > julia
> 
> I'll send this one as a separate patch , since Greg would then decide
> whether the change is worth being made and send the changed patch
> series separately.

If it's a different file than the others, then it's fine.

julia

Re: [Outreachy kernel] [PATCH 3/5] staging: rdma: hfi1: chip: Remove wrapper function

[Amitoj Kaur Chawla]

On Sun, Nov 1, 2015 at 2:56 PM, Julia Lawall <julia.lawall@lip6.fr> wrote:
>
>
> On Sun, 1 Nov 2015, Amitoj Kaur Chawla wrote:
>
>> On Sun, Nov 1, 2015 at 2:43 PM, Julia Lawall <julia.lawall@lip6.fr> wrote:
>> >
>> >
>> > On Sun, 1 Nov 2015, Amitoj Kaur Chawla wrote:
>> >
>> >> On Sun, Nov 1, 2015 at 2:38 PM, Julia Lawall <julia.lawall@lip6.fr> wrote:
>> >> > On Sun, 1 Nov 2015, Amitoj Kaur Chawla wrote:
>> >> >
>> >> >> On Sun, Nov 1, 2015 at 2:11 PM, Julia Lawall <julia.lawall@lip6.fr> wrote:
>> >> >> > On Sun, 1 Nov 2015, Amitoj Kaur Chawla wrote:
>> >> >> >
>> >> >> >> Drop wrapper function remap_receive_available_interrupt() that can be
>> >> >> >> replaced by a single line of code.
>> >> >> >>
>> >> >> >> Signed-off-by: Amitoj Kaur Chawla <amitoj1606@gmail.com>
>> >> >> >> ---
>> >> >> >>  drivers/staging/rdma/hfi1/chip.c | 8 +-------
>> >> >> >>  1 file changed, 1 insertion(+), 7 deletions(-)
>> >> >> >>
>> >> >> >> diff --git a/drivers/staging/rdma/hfi1/chip.c b/drivers/staging/rdma/hfi1/chip.c
>> >> >> >> index beac326d..a9c6abf 100644
>> >> >> >> --- a/drivers/staging/rdma/hfi1/chip.c
>> >> >> >> +++ b/drivers/staging/rdma/hfi1/chip.c
>> >> >> >> @@ -8749,12 +8749,6 @@ static void remap_sdma_interrupts(struct hfi1_devdata *dd,
>> >> >> >>               msix_intr);
>> >> >> >>  }
>> >> >> >>
>> >> >> >> -static void remap_receive_available_interrupt(struct hfi1_devdata *dd,
>> >> >> >> -                                           int rx, int msix_intr)
>> >> >> >> -{
>> >> >> >> -     remap_intr(dd, IS_RCVAVAIL_START + rx, msix_intr);
>> >> >> >> -}
>> >> >> >
>> >> >> > remap_intr is not a standard kernel function.  But perhaps one could argue
>> >> >> > that the constant name gives the same information as the function name, so
>> >> >> > the function is not needed.
>> >> >> >
>> >> >> > But this function is the neighbor of another very similar function that
>> >> >> > remaps three interrupts at once.  The two functions are also used in a
>> >> >> > very parallel way, at the end of successive if branches.  It seems strange
>> >> >> > to inline one function and not the other.
>> >> >> >
>> >> >> > julia
>> >> >>
>> >> >> In this case, the function names are very similar and the wrapper just
>> >> >> add the macro IS_RCVAVAIL_START to the second argument, so the change
>> >> >> made sense to me.
>> >> >
>> >> > Personally, I would find the lack of parallelism confusing.
>> >> > But one could have an opinion either way on this one.
>> >> >
>> >> > julia
>> >> >
>> >>
>> >> So I should leave this as is or drop it? Confused.
>> >
>> > Up to you in this case.  I can see both points of view.  In one point of
>> > view, it is nice to have parallel function names.  In another point of
>> > view, in the case with three operations, a helper function is being
>> > introduced for a complicated case.  This case is different than set/clear
>> > because the two functions are not eg opposites of each other.  There are
>> > just four registers, three that fit together and one that is different.
>> >
>> > julia
>>
>> I'll send this one as a separate patch , since Greg would then decide
>> whether the change is worth being made and send the changed patch
>> series separately.
>
> If it's a different file than the others, then it's fine.
>
> julia

It isn't :(
The first one is changing the same file, so I'll have to send in the series.

-- 
Amitoj