[PATCH] staging: qlge: Remove macro FILL_SEG

[PATCH] staging: qlge: Remove macro FILL_SEG

[Sumitra Sharma]

Remove macro FILL_SEG to fix the checkpatch warning:

WARNING: Macros with flow control statements should be avoided

Macros with flow control statements must be avoided as they
break the flow of the calling function and make it harder to
test the code.

Replace all FILL_SEG() macro calls with:

err = err || qlge_fill_seg_(...);

Signed-off-by: Sumitra Sharma <sumitraartsy@gmail.com>
---
 drivers/staging/qlge/qlge_devlink.c | 109 ++++++++++++----------------
 1 file changed, 47 insertions(+), 62 deletions(-)

diff --git a/drivers/staging/qlge/qlge_devlink.c b/drivers/staging/qlge/qlge_devlink.c
index 0ab02d6d3817..96328207f685 100644
--- a/drivers/staging/qlge/qlge_devlink.c
+++ b/drivers/staging/qlge/qlge_devlink.c
@@ -39,15 +39,6 @@ static int qlge_fill_seg_(struct devlink_fmsg *fmsg,
 	return err;
 }
 
-#define FILL_SEG(seg_hdr, seg_regs)			                    \
-	do {                                                                \
-		err = qlge_fill_seg_(fmsg, &dump->seg_hdr, dump->seg_regs); \
-		if (err) {					            \
-			kvfree(dump);                                       \
-			return err;				            \
-		}                                                           \
-	} while (0)
-
 static int qlge_reporter_coredump(struct devlink_health_reporter *reporter,
 				  struct devlink_fmsg *fmsg, void *priv_ctx,
 				  struct netlink_ext_ack *extack)
@@ -85,59 +76,53 @@ static int qlge_reporter_coredump(struct devlink_health_reporter *reporter,
 
 	qlge_soft_reset_mpi_risc(qdev);
 
-	FILL_SEG(core_regs_seg_hdr, mpi_core_regs);
-	FILL_SEG(test_logic_regs_seg_hdr, test_logic_regs);
-	FILL_SEG(rmii_regs_seg_hdr, rmii_regs);
-	FILL_SEG(fcmac1_regs_seg_hdr, fcmac1_regs);
-	FILL_SEG(fcmac2_regs_seg_hdr, fcmac2_regs);
-	FILL_SEG(fc1_mbx_regs_seg_hdr, fc1_mbx_regs);
-	FILL_SEG(ide_regs_seg_hdr, ide_regs);
-	FILL_SEG(nic1_mbx_regs_seg_hdr, nic1_mbx_regs);
-	FILL_SEG(smbus_regs_seg_hdr, smbus_regs);
-	FILL_SEG(fc2_mbx_regs_seg_hdr, fc2_mbx_regs);
-	FILL_SEG(nic2_mbx_regs_seg_hdr, nic2_mbx_regs);
-	FILL_SEG(i2c_regs_seg_hdr, i2c_regs);
-	FILL_SEG(memc_regs_seg_hdr, memc_regs);
-	FILL_SEG(pbus_regs_seg_hdr, pbus_regs);
-	FILL_SEG(mde_regs_seg_hdr, mde_regs);
-	FILL_SEG(nic_regs_seg_hdr, nic_regs);
-	FILL_SEG(nic2_regs_seg_hdr, nic2_regs);
-	FILL_SEG(xgmac1_seg_hdr, xgmac1);
-	FILL_SEG(xgmac2_seg_hdr, xgmac2);
-	FILL_SEG(code_ram_seg_hdr, code_ram);
-	FILL_SEG(memc_ram_seg_hdr, memc_ram);
-	FILL_SEG(xaui_an_hdr, serdes_xaui_an);
-	FILL_SEG(xaui_hss_pcs_hdr, serdes_xaui_hss_pcs);
-	FILL_SEG(xfi_an_hdr, serdes_xfi_an);
-	FILL_SEG(xfi_train_hdr, serdes_xfi_train);
-	FILL_SEG(xfi_hss_pcs_hdr, serdes_xfi_hss_pcs);
-	FILL_SEG(xfi_hss_tx_hdr, serdes_xfi_hss_tx);
-	FILL_SEG(xfi_hss_rx_hdr, serdes_xfi_hss_rx);
-	FILL_SEG(xfi_hss_pll_hdr, serdes_xfi_hss_pll);
-
-	err = qlge_fill_seg_(fmsg, &dump->misc_nic_seg_hdr,
-			     (u32 *)&dump->misc_nic_info);
-	if (err) {
-		kvfree(dump);
-		return err;
-	}
-
-	FILL_SEG(intr_states_seg_hdr, intr_states);
-	FILL_SEG(cam_entries_seg_hdr, cam_entries);
-	FILL_SEG(nic_routing_words_seg_hdr, nic_routing_words);
-	FILL_SEG(ets_seg_hdr, ets);
-	FILL_SEG(probe_dump_seg_hdr, probe_dump);
-	FILL_SEG(routing_reg_seg_hdr, routing_regs);
-	FILL_SEG(mac_prot_reg_seg_hdr, mac_prot_regs);
-	FILL_SEG(xaui2_an_hdr, serdes2_xaui_an);
-	FILL_SEG(xaui2_hss_pcs_hdr, serdes2_xaui_hss_pcs);
-	FILL_SEG(xfi2_an_hdr, serdes2_xfi_an);
-	FILL_SEG(xfi2_train_hdr, serdes2_xfi_train);
-	FILL_SEG(xfi2_hss_pcs_hdr, serdes2_xfi_hss_pcs);
-	FILL_SEG(xfi2_hss_tx_hdr, serdes2_xfi_hss_tx);
-	FILL_SEG(xfi2_hss_rx_hdr, serdes2_xfi_hss_rx);
-	FILL_SEG(xfi2_hss_pll_hdr, serdes2_xfi_hss_pll);
-	FILL_SEG(sem_regs_seg_hdr, sem_regs);
+	err = err || qlge_fill_seg_(fmsg, &dump->core_regs_seg_hdr, dump->mpi_core_regs);
+	err = err || qlge_fill_seg_(fmsg, &dump->test_logic_regs_seg_hdr, dump->test_logic_regs);
+	err = err || qlge_fill_seg_(fmsg, &dump->rmii_regs_seg_hdr, dump->rmii_regs);
+	err = err || qlge_fill_seg_(fmsg, &dump->fcmac1_regs_seg_hdr, dump->fcmac1_regs);
+	err = err || qlge_fill_seg_(fmsg, &dump->fcmac2_regs_seg_hdr, dump->fcmac2_regs);
+	err = err || qlge_fill_seg_(fmsg, &dump->fc1_mbx_regs_seg_hdr, dump->fc1_mbx_regs);
+	err = err || qlge_fill_seg_(fmsg, &dump->ide_regs_seg_hdr, dump->ide_regs);
+	err = err || qlge_fill_seg_(fmsg, &dump->nic1_mbx_regs_seg_hdr, dump->nic1_mbx_regs);
+	err = err || qlge_fill_seg_(fmsg, &dump->smbus_regs_seg_hdr, dump->smbus_regs);
+	err = err || qlge_fill_seg_(fmsg, &dump->fc2_mbx_regs_seg_hdr, dump->fc2_mbx_regs);
+	err = err || qlge_fill_seg_(fmsg, &dump->nic2_mbx_regs_seg_hdr, dump->nic2_mbx_regs);
+	err = err || qlge_fill_seg_(fmsg, &dump->i2c_regs_seg_hdr, dump->i2c_regs);
+	err = err || qlge_fill_seg_(fmsg, &dump->memc_regs_seg_hdr, dump->memc_regs);
+	err = err || qlge_fill_seg_(fmsg, &dump->pbus_regs_seg_hdr, dump->pbus_regs);
+	err = err || qlge_fill_seg_(fmsg, &dump->mde_regs_seg_hdr, dump->mde_regs);
+	err = err || qlge_fill_seg_(fmsg, &dump->nic_regs_seg_hdr, dump->nic_regs);
+	err = err || qlge_fill_seg_(fmsg, &dump->nic2_regs_seg_hdr, dump->nic2_regs);
+	err = err || qlge_fill_seg_(fmsg, &dump->xgmac1_seg_hdr, dump->xgmac1);
+	err = err || qlge_fill_seg_(fmsg, &dump->xgmac2_seg_hdr, dump->xgmac2);
+	err = err || qlge_fill_seg_(fmsg, &dump->code_ram_seg_hdr, dump->code_ram);
+	err = err || qlge_fill_seg_(fmsg, &dump->memc_ram_seg_hdr, dump->memc_ram);
+	err = err || qlge_fill_seg_(fmsg, &dump->xaui_an_hdr, dump->serdes_xaui_an);
+	err = err || qlge_fill_seg_(fmsg, &dump->xaui_hss_pcs_hdr, dump->serdes_xaui_hss_pcs);
+	err = err || qlge_fill_seg_(fmsg, &dump->xfi_an_hdr, dump->serdes_xfi_an);
+	err = err || qlge_fill_seg_(fmsg, &dump->xfi_train_hdr, dump->serdes_xfi_train);
+	err = err || qlge_fill_seg_(fmsg, &dump->xfi_hss_pcs_hdr, dump->serdes_xfi_hss_pcs);
+	err = err || qlge_fill_seg_(fmsg, &dump->xfi_hss_tx_hdr, dump->serdes_xfi_hss_tx);
+	err = err || qlge_fill_seg_(fmsg, &dump->xfi_hss_rx_hdr, dump->serdes_xfi_hss_rx);
+	err = err || qlge_fill_seg_(fmsg, &dump->xfi_hss_pll_hdr, dump->serdes_xfi_hss_pll);
+	err = err || qlge_fill_seg_(fmsg, &dump->misc_nic_seg_hdr, (u32 *)&dump->misc_nic_info);
+	err = err || qlge_fill_seg_(fmsg, &dump->intr_states_seg_hdr, dump->intr_states);
+	err = err || qlge_fill_seg_(fmsg, &dump->cam_entries_seg_hdr, dump->cam_entries);
+	err = err || qlge_fill_seg_(fmsg, &dump->nic_routing_words_seg_hdr,
+				    dump->nic_routing_words);
+	err = err || qlge_fill_seg_(fmsg, &dump->ets_seg_hdr, dump->ets);
+	err = err || qlge_fill_seg_(fmsg, &dump->probe_dump_seg_hdr, dump->probe_dump);
+	err = err || qlge_fill_seg_(fmsg, &dump->routing_reg_seg_hdr, dump->routing_regs);
+	err = err || qlge_fill_seg_(fmsg, &dump->mac_prot_reg_seg_hdr, dump->mac_prot_regs);
+	err = err || qlge_fill_seg_(fmsg, &dump->xaui2_an_hdr, dump->serdes2_xaui_an);
+	err = err || qlge_fill_seg_(fmsg, &dump->xaui2_hss_pcs_hdr, dump->serdes2_xaui_hss_pcs);
+	err = err || qlge_fill_seg_(fmsg, &dump->xfi2_an_hdr, dump->serdes2_xfi_an);
+	err = err || qlge_fill_seg_(fmsg, &dump->xfi2_train_hdr, dump->serdes2_xfi_train);
+	err = err || qlge_fill_seg_(fmsg, &dump->xfi2_hss_pcs_hdr, dump->serdes2_xfi_hss_pcs);
+	err = err || qlge_fill_seg_(fmsg, &dump->xfi2_hss_tx_hdr, dump->serdes2_xfi_hss_tx);
+	err = err || qlge_fill_seg_(fmsg, &dump->xfi2_hss_rx_hdr, dump->serdes2_xfi_hss_rx);
+	err = err || qlge_fill_seg_(fmsg, &dump->xfi2_hss_pll_hdr, dump->serdes2_xfi_hss_pll);
+	err = err || qlge_fill_seg_(fmsg, &dump->sem_regs_seg_hdr, dump->sem_regs);
 
 	kvfree(dump);
 	return err;
-- 
2.25.1

Re: [PATCH] staging: qlge: Remove macro FILL_SEG

[Simon Horman]

On Wed, Apr 05, 2023 at 08:06:27AM -0700, Sumitra Sharma wrote:
> Remove macro FILL_SEG to fix the checkpatch warning:
> 
> WARNING: Macros with flow control statements should be avoided
> 
> Macros with flow control statements must be avoided as they
> break the flow of the calling function and make it harder to
> test the code.
> 
> Replace all FILL_SEG() macro calls with:
> 
> err = err || qlge_fill_seg_(...);

Perhaps I'm missing the point here.
But won't this lead to err always either being true or false (1 or 0).
Rather than the current arrangement where err can be
either 0 or a negative error value, such as -EINVAL.

...

Re: [PATCH] staging: qlge: Remove macro FILL_SEG

[Sumitra Sharma]

On Wed, Apr 05, 2023 at 06:21:57PM +0200, Simon Horman wrote:
> On Wed, Apr 05, 2023 at 08:06:27AM -0700, Sumitra Sharma wrote:
> > Remove macro FILL_SEG to fix the checkpatch warning:
> > 
> > WARNING: Macros with flow control statements should be avoided
> > 
> > Macros with flow control statements must be avoided as they
> > break the flow of the calling function and make it harder to
> > test the code.
> > 
> > Replace all FILL_SEG() macro calls with:
> > 
> > err = err || qlge_fill_seg_(...);
> 
> Perhaps I'm missing the point here.
> But won't this lead to err always either being true or false (1 or 0).
> Rather than the current arrangement where err can be
> either 0 or a negative error value, such as -EINVAL.
>

Hi Simon


Thank you for the point you mentioned which I missed while working on this
patch. 

However, after thinking on it, I am still not able to get any fix to this
except that we can possibly implement the Ira's solution here which is:

https://lore.kernel.org/outreachy/64154d438f0c8_28ae5229421@iweiny-mobl.notmuch/

Although we have to then deal with 40 lines of ifs.



Regards

Sumitra



> ...

Re: [PATCH] staging: qlge: Remove macro FILL_SEG

[Greg Kroah-Hartman]

On Thu, Apr 06, 2023 at 07:46:44AM -0700, Sumitra Sharma wrote:
> On Wed, Apr 05, 2023 at 06:21:57PM +0200, Simon Horman wrote:
> > On Wed, Apr 05, 2023 at 08:06:27AM -0700, Sumitra Sharma wrote:
> > > Remove macro FILL_SEG to fix the checkpatch warning:
> > > 
> > > WARNING: Macros with flow control statements should be avoided
> > > 
> > > Macros with flow control statements must be avoided as they
> > > break the flow of the calling function and make it harder to
> > > test the code.
> > > 
> > > Replace all FILL_SEG() macro calls with:
> > > 
> > > err = err || qlge_fill_seg_(...);
> > 
> > Perhaps I'm missing the point here.
> > But won't this lead to err always either being true or false (1 or 0).
> > Rather than the current arrangement where err can be
> > either 0 or a negative error value, such as -EINVAL.
> >
> 
> Hi Simon
> 
> 
> Thank you for the point you mentioned which I missed while working on this
> patch. 
> 
> However, after thinking on it, I am still not able to get any fix to this
> except that we can possibly implement the Ira's solution here which is:
> 
> https://lore.kernel.org/outreachy/64154d438f0c8_28ae5229421@iweiny-mobl.notmuch/
> 
> Although we have to then deal with 40 lines of ifs.

Which implies that the current solution is the best one, so I would
recommend just leaving it as-is.

Remember, checkpatch.pl is a tool to provide hints, it does not have
much context, if any, to determine if it's hints actually make sense.

thanks,

greg k-h

Re: [PATCH] staging: qlge: Remove macro FILL_SEG

[Dan Carpenter]

On Wed, Apr 05, 2023 at 08:06:27AM -0700, Sumitra Sharma wrote:
> +	err = err || qlge_fill_seg_(fmsg, &dump->core_regs_seg_hdr, dump->mpi_core_regs);

I have not seen anyone do this before.  I sometimes see people do:

	err |= frob1();
	err |= frob2();
	err |= frob3();

I don't like this very much, but it basically works-ish...  I don't like
that it ORs all the errors together and that it continues after it has
errors.

Another idea would be to do:

	err = err ?: frob1();
	err = err ?: frob2();
	err = err ?: frob3();

BPF and networking have a couple place which do it this way so maybe
it's going to become trendy.

regards,
dan carpenter

Re: [PATCH] staging: qlge: Remove macro FILL_SEG

[Sumitra Sharma]

On Thu, Apr 06, 2023 at 04:57:44PM +0200, Greg Kroah-Hartman wrote:
> On Thu, Apr 06, 2023 at 07:46:44AM -0700, Sumitra Sharma wrote:
> > On Wed, Apr 05, 2023 at 06:21:57PM +0200, Simon Horman wrote:
> > > On Wed, Apr 05, 2023 at 08:06:27AM -0700, Sumitra Sharma wrote:
> > > > Remove macro FILL_SEG to fix the checkpatch warning:
> > > > 
> > > > WARNING: Macros with flow control statements should be avoided
> > > > 
> > > > Macros with flow control statements must be avoided as they
> > > > break the flow of the calling function and make it harder to
> > > > test the code.
> > > > 
> > > > Replace all FILL_SEG() macro calls with:
> > > > 
> > > > err = err || qlge_fill_seg_(...);
> > > 
> > > Perhaps I'm missing the point here.
> > > But won't this lead to err always either being true or false (1 or 0).
> > > Rather than the current arrangement where err can be
> > > either 0 or a negative error value, such as -EINVAL.
> > >
> > 
> > Hi Simon
> > 
> > 
> > Thank you for the point you mentioned which I missed while working on this
> > patch. 
> > 
> > However, after thinking on it, I am still not able to get any fix to this
> > except that we can possibly implement the Ira's solution here which is:
> > 
> > https://lore.kernel.org/outreachy/64154d438f0c8_28ae5229421@iweiny-mobl.notmuch/
> > 
> > Although we have to then deal with 40 lines of ifs.
> 
> Which implies that the current solution is the best one, so I would
> recommend just leaving it as-is.
>

Hi greg

Before leaving it I would like to know your opinion on the solution Dan is offering.


Regards
Sumitra

> Remember, checkpatch.pl is a tool to provide hints, it does not have
> much context, if any, to determine if it's hints actually make sense.
> 
> thanks,
> 
> greg k-h

Re: [PATCH] staging: qlge: Remove macro FILL_SEG

[Greg Kroah-Hartman]

On Thu, Apr 06, 2023 at 08:28:55AM -0700, Sumitra Sharma wrote:
> On Thu, Apr 06, 2023 at 04:57:44PM +0200, Greg Kroah-Hartman wrote:
> > On Thu, Apr 06, 2023 at 07:46:44AM -0700, Sumitra Sharma wrote:
> > > On Wed, Apr 05, 2023 at 06:21:57PM +0200, Simon Horman wrote:
> > > > On Wed, Apr 05, 2023 at 08:06:27AM -0700, Sumitra Sharma wrote:
> > > > > Remove macro FILL_SEG to fix the checkpatch warning:
> > > > > 
> > > > > WARNING: Macros with flow control statements should be avoided
> > > > > 
> > > > > Macros with flow control statements must be avoided as they
> > > > > break the flow of the calling function and make it harder to
> > > > > test the code.
> > > > > 
> > > > > Replace all FILL_SEG() macro calls with:
> > > > > 
> > > > > err = err || qlge_fill_seg_(...);
> > > > 
> > > > Perhaps I'm missing the point here.
> > > > But won't this lead to err always either being true or false (1 or 0).
> > > > Rather than the current arrangement where err can be
> > > > either 0 or a negative error value, such as -EINVAL.
> > > >
> > > 
> > > Hi Simon
> > > 
> > > 
> > > Thank you for the point you mentioned which I missed while working on this
> > > patch. 
> > > 
> > > However, after thinking on it, I am still not able to get any fix to this
> > > except that we can possibly implement the Ira's solution here which is:
> > > 
> > > https://lore.kernel.org/outreachy/64154d438f0c8_28ae5229421@iweiny-mobl.notmuch/
> > > 
> > > Although we have to then deal with 40 lines of ifs.
> > 
> > Which implies that the current solution is the best one, so I would
> > recommend just leaving it as-is.
> >
> 
> Hi greg
> 
> Before leaving it I would like to know your opinion on the solution Dan is offering.

I still think you should leave it as-is.

thanks,

greg k-h

Re: [PATCH] staging: qlge: Remove macro FILL_SEG

[Sumitra Sharma]

On Thu, Apr 06, 2023 at 06:33:31PM +0200, Greg Kroah-Hartman wrote:
> On Thu, Apr 06, 2023 at 08:28:55AM -0700, Sumitra Sharma wrote:
> > On Thu, Apr 06, 2023 at 04:57:44PM +0200, Greg Kroah-Hartman wrote:
> > > On Thu, Apr 06, 2023 at 07:46:44AM -0700, Sumitra Sharma wrote:
> > > > On Wed, Apr 05, 2023 at 06:21:57PM +0200, Simon Horman wrote:
> > > > > On Wed, Apr 05, 2023 at 08:06:27AM -0700, Sumitra Sharma wrote:
> > > > > > Remove macro FILL_SEG to fix the checkpatch warning:
> > > > > > 
> > > > > > WARNING: Macros with flow control statements should be avoided
> > > > > > 
> > > > > > Macros with flow control statements must be avoided as they
> > > > > > break the flow of the calling function and make it harder to
> > > > > > test the code.
> > > > > > 
> > > > > > Replace all FILL_SEG() macro calls with:
> > > > > > 
> > > > > > err = err || qlge_fill_seg_(...);
> > > > > 
> > > > > Perhaps I'm missing the point here.
> > > > > But won't this lead to err always either being true or false (1 or 0).
> > > > > Rather than the current arrangement where err can be
> > > > > either 0 or a negative error value, such as -EINVAL.
> > > > >
> > > > 
> > > > Hi Simon
> > > > 
> > > > 
> > > > Thank you for the point you mentioned which I missed while working on this
> > > > patch. 
> > > > 
> > > > However, after thinking on it, I am still not able to get any fix to this
> > > > except that we can possibly implement the Ira's solution here which is:
> > > > 
> > > > https://lore.kernel.org/outreachy/64154d438f0c8_28ae5229421@iweiny-mobl.notmuch/
> > > > 
> > > > Although we have to then deal with 40 lines of ifs.
> > > 
> > > Which implies that the current solution is the best one, so I would
> > > recommend just leaving it as-is.
> > >
> > 
> > Hi greg
> > 
> > Before leaving it I would like to know your opinion on the solution Dan is offering.
> 
> I still think you should leave it as-is.
>

Okay, Thank you.


Regards

Sumitra Sharma
> thanks,
> 
> greg k-h