Removed unreachable code and fixed one compiler warning

PATCH In review ville palo 4 participants 7 messages Aug 28, 2006 Kernel janitors

Share & tools

ville palo

author

2006-08-28 16:57 UTC #0 lore raw

PATCH 1/1 Removed unreachable code and fixed one compiler warning

There were some unreachable code segments in drivers/cdrom. This patch removes them and fixes one compiler warning:
"gscd.c:269: warning: unsigned int format, long unsigned int arg"

Patch is made against 2.6.18-rc5

Signed-off-by Ville Palo <[email protected]>

drivers/cdrom/gscd.c +2 -5

index fa70824..03884df 100644
@@ -266,7 +266,7 @@ repeat:
 		goto out;
 
 	if (req->cmd != READ) {
-		printk("GSCD: bad cmd %u\n", rq_data_dir(req));
+		printk("GSCD: bad cmd %lu\n", rq_data_dir(req));
 		end_request(req, 0);
 		goto repeat;
 	}

@@ -569,7 +569,6 @@ static void cmd_out(int cmd_type, char *
 							     respo_count,
 							     CD_FRAMESIZE /
 							     2);
-							return;
 						} else {
 							/* read the data to the buffer (byte) */

@@ -578,15 +577,13 @@ static void cmd_out(int cmd_type, char *
 							    (respo_buf,
 							     respo_count,
 							     CD_FRAMESIZE);
-							return;
 						}
 					} else {
 						/* read the info to the buffer */
 						cmd_info_in(respo_buf,
 							    respo_count);
-						return;
 					}
-
+					
 					return;
 				}
 			}

drivers/cdrom/isp16.c +0 -1

index db0fd9a..3c1b493 100644
@@ -318,7 +318,6 @@ isp16_cdi_config(int base, u_char drive_
 		printk("ISP16: dma 1 cannot be used by cdrom interface,"
 		       " due to conflict with the sound card.\n");
 		return -1;
-		break;
 	case 3:
 		dma_code = ISP16_DMA_3;
 		break;

drivers/cdrom/mcdx.c +1 -8

index dcd1ab6..b0ab77b 100644
@@ -622,9 +622,6 @@ static void do_mcdx_request(request_queu
 		}
 		end_request(req, 1);
 		goto again;
-
-		xtrace(REQUEST, "end_request(1)\n");
-		end_request(req, 1);
 	}
 
 	goto again;

@@ -1710,11 +1707,7 @@ static int mcdx_tray_move(struct cdrom_d
 	if (!(stuffp->present & DOOR))
 		return -ENOSYS;
 
-	if (position)		/* 1: eject */
-		return mcdx_talk(stuffp, "\xf6", 1, NULL, 1, 5 * HZ, 3);
-	else			/* 0: close */
-		return mcdx_talk(stuffp, "\xf8", 1, NULL, 1, 5 * HZ, 3);
-	return 1;
+	return mcdx_talk(stuffp, position ? "\xf6" : "\xf8", 1, NULL, 1, 5 * HZ, 3);
 }
 
 static int mcdx_stop(struct s_drive_stuff *stuffp, int tries)

drivers/cdrom/sonycd535.c +0 -9

index 30ab562..65bdd64 100644
@@ -1028,7 +1028,6 @@ sony_get_subchnl_info(void __user *arg)
 		if (copy_to_user(arg, &schi, sizeof schi))
 			return -EFAULT;
 		return 0;
-		break;
 
 	case CDROM_AUDIO_INVALID:
 	case CDROM_AUDIO_ERROR:

@@ -1083,7 +1082,6 @@ cdu_ioctl(struct inode *inode,
 			return -EIO;
 		}
 		return 0;
-		break;
 
 	case CDROMSTOP:			/* Spin down the drive */
 		cmd_buff[0] = SONY535_HOLD;

@@ -1103,7 +1101,6 @@ cdu_ioctl(struct inode *inode,
 			return -EIO;
 		}
 		return 0;
-		break;
 
 	case CDROMPAUSE:			/* Pause the drive */
 		cmd_buff[0] = SONY535_HOLD;		/* CDU-31 driver uses AUDIO_STOP, not pause */

@@ -1121,7 +1118,6 @@ cdu_ioctl(struct inode *inode,
 		cur_pos_msf[2] = last_sony_subcode->abs_msf[2];
 		sony_audio_status = CDROM_AUDIO_PAUSED;
 		return 0;
-		break;
 
 	case CDROMRESUME:			/* Start the drive after being paused */
 		set_drive_mode(SONY535_AUDIO_DRIVE_MODE, status);

@@ -1150,7 +1146,6 @@ cdu_ioctl(struct inode *inode,
 		}
 		sony_audio_status = CDROM_AUDIO_PLAY;
 		return 0;
-		break;
 
 	case CDROMPLAYMSF:			/* Play starting at the given MSF address. */
 		if (copy_from_user(params, argp, 6))

@@ -1181,7 +1176,6 @@ cdu_ioctl(struct inode *inode,
 		final_pos_msf[2] = cmd_buff[9];
 		sony_audio_status = CDROM_AUDIO_PLAY;
 		return 0;
-		break;
 
 	case CDROMREADTOCHDR:		/* Read the table of contents header */
 		{

@@ -1197,7 +1191,6 @@ cdu_ioctl(struct inode *inode,
 				return -EFAULT;
 		}
 		return 0;
-		break;
 
 	case CDROMREADTOCENTRY:	/* Read a given table of contents entry */
 		{

@@ -1240,7 +1233,6 @@ cdu_ioctl(struct inode *inode,
 				return -EFAULT;
 		}
 		return 0;
-		break;
 
 	case CDROMPLAYTRKIND:		/* Play a track.  This currently ignores index. */
 		{

@@ -1346,7 +1338,6 @@ cdu_ioctl(struct inode *inode,
 			return -EIO;
 		}
 		return 0;
-		break;
 
 	default:
 		return -EINVAL;

_______________________________________________
Kernel-janitors mailing list
[email protected]
https://lists.osdl.org/mailman/listinfo/kernel-janitors

Benoit Boissinot

2006-08-28 16:59 UTC #1 lore raw

Re: [KJ] [PATCH] Removed unreachable code and fixed one compiler

Quoted 8 lines · ville palo

There were some unreachable code segments in drivers/cdrom. This patch removes them and fixes one compiler warning:
"gscd.c:269: warning: unsigned int format, long unsigned int arg"
Patch is made against 2.6.18-rc5
drivers/cdrom/mcdx.c +0 -0
@@ -1710,11 +1707,7 @@ static int mcdx_tray_move(struct cdrom_d
        if (!(stuffp->present & DOOR))

drivers/cdrom/mcdx.c static int mcdx_tray_move(struct cdrom_d · line 1712

Quoted 10 lines

drivers/cdrom/mcdx.c +1 -5

                return -ENOSYS;
 
-       if (position)           /* 1: eject */
-               return mcdx_talk(stuffp, "\xf6", 1, NULL, 1, 5 * HZ, 3);
-       else                    /* 0: close */
-               return mcdx_talk(stuffp, "\xf8", 1, NULL, 1, 5 * HZ, 3);
-       return 1;
+       return mcdx_talk(stuffp, position ? "\xf6" : "\xf8", 1, NULL, 1, 5 * HZ, 3);
 }

You are removing the comments here, and it is useful information to understand the code.

regards,

Benoit
_______________________________________________
Kernel-janitors mailing list
[email protected]
https://lists.osdl.org/mailman/listinfo/kernel-janitors

Nishanth Aravamudan

2006-08-28 17:26 UTC #2 lore raw

Re: [KJ] [PATCH] Removed unreachable code and fixed one compiler

In reply to ville palo

Quoted 5 lines · ville palo

There were some unreachable code segments in drivers/cdrom. This patch removes them and fixes one compiler warning:
"gscd.c:269: warning: unsigned int format, long unsigned int arg"

Should be separate patches then. Please read tpp
(http://www.zip.com.au/~akpm/linux/patches/stuff/tpp.txt) and Documentation/SubmittingPatches if you haven't already (reread them, if you have).

Quoted 1 line

Patch is made against 2.6.18-rc5

Not necessary in general.

Quoted 1 line

Signed-off-by Ville Palo <[email protected]>

Canonical form requires the colon, please

Signed-off-by Random J Developer <[email protected]>

drivers/cdrom/gscd.c repeat: · line 272

Quoted 12 lines

index fa70824..03884df 100644

drivers/cdrom/gscd.c +1 -1

@@ -266,7 +266,7 @@ repeat:
 		goto out;
 
 	if (req->cmd != READ) {
-		printk("GSCD: bad cmd %u\n", rq_data_dir(req));
+		printk("GSCD: bad cmd %lu\n", rq_data_dir(req));
 		end_request(req, 0);
 		goto repeat;
 	}

Just to be clear, this should be a separate patch.

Quoted 4 lines

drivers/cdrom/gscd.c +0 -0

@@ -578,15 +577,13 @@ static void cmd_out(int cmd_type, char *
 							    (respo_buf,
 							     respo_count,
 							     CD_FRAMESIZE);

drivers/cdrom/gscd.c static void cmd_out(int cmd_type, char * · line 586

Quoted 10 lines

drivers/cdrom/gscd.c +1 -3

-							return;
 						}
 					} else {
 						/* read the info to the buffer */
 						cmd_info_in(respo_buf,
 							    respo_count);
-						return;
 					}
-
+

Whitespace insertion?

Quoted 20 lines

drivers/cdrom/gscd.c +0 -0

 					return;
 				}
 			}

drivers/cdrom/isp16.c +0 -1

index db0fd9a..3c1b493 100644
@@ -318,7 +318,6 @@ isp16_cdi_config(int base, u_char drive_
 		printk("ISP16: dma 1 cannot be used by cdrom interface,"
 		       " due to conflict with the sound card.\n");
 		return -1;
-		break;
 	case 3:
 		dma_code = ISP16_DMA_3;
 		break;

drivers/cdrom/mcdx.c +0 -0

index dcd1ab6..b0ab77b 100644

drivers/cdrom/mcdx.c static void do_mcdx_request(request_queu · line 627

Quoted 6 lines

drivers/cdrom/mcdx.c +0 -3

@@ -622,9 +622,6 @@ static void do_mcdx_request(request_queu
 		}
 		end_request(req, 1);
 		goto again;
-
-		xtrace(REQUEST, "end_request(1)\n");
-		end_request(req, 1);

Perhapse move the xtrace (which is presumably useful) to before the goto?

drivers/cdrom/mcdx.c static int mcdx_tray_move(struct cdrom_d · line 1710

Quoted 13 lines

drivers/cdrom/mcdx.c +1 -5

 	}
 
 	goto again;

@@ -1710,11 +1707,7 @@ static int mcdx_tray_move(struct cdrom_d
 	if (!(stuffp->present & DOOR))
 		return -ENOSYS;
 
-	if (position)		/* 1: eject */
-		return mcdx_talk(stuffp, "\xf6", 1, NULL, 1, 5 * HZ, 3);
-	else			/* 0: close */
-		return mcdx_talk(stuffp, "\xf8", 1, NULL, 1, 5 * HZ, 3);
-	return 1;
+	return mcdx_talk(stuffp, position ? "\xf6" : "\xf8", 1, NULL, 1, 5 * HZ, 3);

As mentioned elsewhere the comments are useful. This also doesn't seem to fit into either category of your description and thus should have been in a different patch (Have you (re)read tpp or SubmittingPatches yet?).

Thanks,
Nish

Signature · 6 lines

Nishanth Aravamudan <[email protected]>
IBM Linux Technology Center
_______________________________________________
Kernel-janitors mailing list
[email protected]
https://lists.osdl.org/mailman/listinfo/kernel-janitors

ville palo

author

2006-08-28 17:26 UTC #3 lore raw

Re: [KJ] [PATCH] Removed unreachable code and fixed one

In reply to ville palo

Quoted 3 lines · Benoit Boissinot

You are removing the comments here, and it is useful information to understand the code.

Yes, you are right, here's the fixed patch:

Signed-off-by Ville Palo <[email protected]>

drivers/cdrom/gscd.c +2 -5

index fa70824..03884df 100644
@@ -266,7 +266,7 @@ repeat:
 		goto out;
 
 	if (req->cmd != READ) {
-		printk("GSCD: bad cmd %u\n", rq_data_dir(req));
+		printk("GSCD: bad cmd %lu\n", rq_data_dir(req));
 		end_request(req, 0);
 		goto repeat;
 	}

@@ -569,7 +569,6 @@ static void cmd_out(int cmd_type, char *
 							     respo_count,
 							     CD_FRAMESIZE /
 							     2);
-							return;
 						} else {
 							/* read the data to the buffer (byte) */

@@ -578,15 +577,13 @@ static void cmd_out(int cmd_type, char *
 							    (respo_buf,
 							     respo_count,
 							     CD_FRAMESIZE);
-							return;
 						}
 					} else {
 						/* read the info to the buffer */
 						cmd_info_in(respo_buf,
 							    respo_count);
-						return;
 					}
-
+					
 					return;
 				}
 			}

drivers/cdrom/isp16.c +0 -1

index db0fd9a..3c1b493 100644
@@ -318,7 +318,6 @@ isp16_cdi_config(int base, u_char drive_
 		printk("ISP16: dma 1 cannot be used by cdrom interface,"
 		       " due to conflict with the sound card.\n");
 		return -1;
-		break;
 	case 3:
 		dma_code = ISP16_DMA_3;
 		break;

drivers/cdrom/mcdx.c +2 -8

index dcd1ab6..bcff5a8 100644
@@ -622,9 +622,6 @@ static void do_mcdx_request(request_queu
 		}
 		end_request(req, 1);
 		goto again;
-
-		xtrace(REQUEST, "end_request(1)\n");
-		end_request(req, 1);
 	}
 
 	goto again;

@@ -1710,11 +1707,8 @@ static int mcdx_tray_move(struct cdrom_d
 	if (!(stuffp->present & DOOR))
 		return -ENOSYS;
 
-	if (position)		/* 1: eject */
-		return mcdx_talk(stuffp, "\xf6", 1, NULL, 1, 5 * HZ, 3);
-	else			/* 0: close */
-		return mcdx_talk(stuffp, "\xf8", 1, NULL, 1, 5 * HZ, 3);
-	return 1;
+	/* If position is 1 then eject, otherwise close */
+	return mcdx_talk(stuffp, position ? "\xf6" : "\xf8", 1, NULL, 1, 5 * HZ, 3);
 }
 
 static int mcdx_stop(struct s_drive_stuff *stuffp, int tries)

drivers/cdrom/sonycd535.c +0 -9

index 30ab562..65bdd64 100644
@@ -1028,7 +1028,6 @@ sony_get_subchnl_info(void __user *arg)
 		if (copy_to_user(arg, &schi, sizeof schi))
 			return -EFAULT;
 		return 0;
-		break;
 
 	case CDROM_AUDIO_INVALID:
 	case CDROM_AUDIO_ERROR:

@@ -1083,7 +1082,6 @@ cdu_ioctl(struct inode *inode,
 			return -EIO;
 		}
 		return 0;
-		break;
 
 	case CDROMSTOP:			/* Spin down the drive */
 		cmd_buff[0] = SONY535_HOLD;

@@ -1103,7 +1101,6 @@ cdu_ioctl(struct inode *inode,
 			return -EIO;
 		}
 		return 0;
-		break;
 
 	case CDROMPAUSE:			/* Pause the drive */
 		cmd_buff[0] = SONY535_HOLD;		/* CDU-31 driver uses AUDIO_STOP, not pause */

@@ -1121,7 +1118,6 @@ cdu_ioctl(struct inode *inode,
 		cur_pos_msf[2] = last_sony_subcode->abs_msf[2];
 		sony_audio_status = CDROM_AUDIO_PAUSED;
 		return 0;
-		break;
 
 	case CDROMRESUME:			/* Start the drive after being paused */
 		set_drive_mode(SONY535_AUDIO_DRIVE_MODE, status);

@@ -1150,7 +1146,6 @@ cdu_ioctl(struct inode *inode,
 		}
 		sony_audio_status = CDROM_AUDIO_PLAY;
 		return 0;
-		break;
 
 	case CDROMPLAYMSF:			/* Play starting at the given MSF address. */
 		if (copy_from_user(params, argp, 6))

@@ -1181,7 +1176,6 @@ cdu_ioctl(struct inode *inode,
 		final_pos_msf[2] = cmd_buff[9];
 		sony_audio_status = CDROM_AUDIO_PLAY;
 		return 0;
-		break;
 
 	case CDROMREADTOCHDR:		/* Read the table of contents header */
 		{

@@ -1197,7 +1191,6 @@ cdu_ioctl(struct inode *inode,
 				return -EFAULT;
 		}
 		return 0;
-		break;
 
 	case CDROMREADTOCENTRY:	/* Read a given table of contents entry */
 		{

@@ -1240,7 +1233,6 @@ cdu_ioctl(struct inode *inode,
 				return -EFAULT;
 		}
 		return 0;
-		break;
 
 	case CDROMPLAYTRKIND:		/* Play a track.  This currently ignores index. */
 		{

@@ -1346,7 +1338,6 @@ cdu_ioctl(struct inode *inode,
 			return -EIO;
 		}
 		return 0;
-		break;
 
 	default:
 		return -EINVAL;

_______________________________________________
Kernel-janitors mailing list
[email protected]
https://lists.osdl.org/mailman/listinfo/kernel-janitors

ville palo

author

2006-08-28 17:49 UTC #4 lore raw

Re: [KJ] [PATCH] Removed unreachable code and fixed one

In reply to ville palo

Quoted 11 lines · Nishanth Aravamudan

Quoted 5 lines · ville palo
There were some unreachable code segments in drivers/cdrom. This patch removes them and fixes one compiler warning:
"gscd.c:269: warning: unsigned int format, long unsigned int arg"
Should be separate patches then. Please read tpp
(http://www.zip.com.au/~akpm/linux/patches/stuff/tpp.txt) and Documentation/SubmittingPatches if you haven't already (reread them, if you have).

Ok, I'll reread them :)

Quoted 10 lines

Perhapse move the xtrace (which is presumably useful) to before the goto?
Quoted 7 lines
drivers/cdrom/mcdx.c +0 -0
 	}
 
 	goto again;
@@ -1710,11 +1707,7 @@ static int mcdx_tray_move(struct cdrom_d
 	if (!(stuffp->present & DOOR))
 		return -ENOSYS;
 

Quoted 10 lines

drivers/cdrom/mcdx.c static int mcdx_tray_move(struct cdrom_d · line 1710
Quoted 6 lines
drivers/cdrom/mcdx.c +1 -5
-	if (position)		/* 1: eject */
-		return mcdx_talk(stuffp, "\xf6", 1, NULL, 1, 5 * HZ, 3);
-	else			/* 0: close */
-		return mcdx_talk(stuffp, "\xf8", 1, NULL, 1, 5 * HZ, 3);
-	return 1;
+	return mcdx_talk(stuffp, position ? "\xf6" : "\xf8", 1, NULL, 1, 5 * HZ, 3);
As mentioned elsewhere the comments are useful. This also doesn't seem to fit into either category of your description and thus should have been in a different patch (Have you (re)read tpp or SubmittingPatches yet?).

I already fixed the comments part but I have to disagree about the description part. IMHO this fits in "removed unreachable code" category, because 'return 1;' was unreachable.

_______________________________________________
Kernel-janitors mailing list
[email protected]
https://lists.osdl.org/mailman/listinfo/kernel-janitors

Nishanth Aravamudan

2006-08-28 17:51 UTC #5 lore raw

Re: [KJ] [PATCH] Removed unreachable code and fixed one compiler

In reply to ville palo

Quoted 36 lines · ville palo

Quoted 11 lines · Nishanth Aravamudan
Quoted 5 lines · ville palo
There were some unreachable code segments in drivers/cdrom. This patch removes them and fixes one compiler warning:
"gscd.c:269: warning: unsigned int format, long unsigned int arg"
Should be separate patches then. Please read tpp
(http://www.zip.com.au/~akpm/linux/patches/stuff/tpp.txt) and Documentation/SubmittingPatches if you haven't already (reread them, if you have).
Ok, I'll reread them :)
Quoted 10 lines
Perhapse move the xtrace (which is presumably useful) to before the goto?
Quoted 7 lines
drivers/cdrom/mcdx.c +0 -0
 	}
 
 	goto again;
@@ -1710,11 +1707,7 @@ static int mcdx_tray_move(struct cdrom_d
 	if (!(stuffp->present & DOOR))
 		return -ENOSYS;
 
Quoted 10 lines
drivers/cdrom/mcdx.c static int mcdx_tray_move(struct cdrom_d · line 1710
Quoted 6 lines
drivers/cdrom/mcdx.c +1 -5
-	if (position)		/* 1: eject */
-		return mcdx_talk(stuffp, "\xf6", 1, NULL, 1, 5 * HZ, 3);
-	else			/* 0: close */
-		return mcdx_talk(stuffp, "\xf8", 1, NULL, 1, 5 * HZ, 3);
-	return 1;
+	return mcdx_talk(stuffp, position ? "\xf6" : "\xf8", 1, NULL, 1, 5 * HZ, 3);
As mentioned elsewhere the comments are useful. This also doesn't seem to fit into either category of your description and thus should have been in a different patch (Have you (re)read tpp or SubmittingPatches yet?).

Quoted 3 lines

I already fixed the comments part but I have to disagree about the description part. IMHO this fits in "removed unreachable code" category, because 'return 1;' was unreachable.

Only the return 1; removal does. The other is code rearrangement, and is a distinct (and unnecessary) logical change.

Thanks,
Nish

Signature · 6 lines

Nishanth Aravamudan <[email protected]>
IBM Linux Technology Center
_______________________________________________
Kernel-janitors mailing list
[email protected]
https://lists.osdl.org/mailman/listinfo/kernel-janitors

Richard Knutsson

2006-08-28 18:00 UTC #6 lore raw

Re: [KJ] [PATCH] Removed unreachable code and fixed one compiler

In reply to ville palo

Quoted 9 lines · ville palo

Quoted 5 lines · Benoit Boissinot
You are removing the comments here, and it is useful information to understand the code.

Quoted 7 lines

Yes, you are right, here's the fixed patch:
Signed-off-by Ville Palo <[email protected]>

<slice>

Quoted 5 lines

	goto again;

drivers/cdrom/mcdx.c +0 -0

@@ -1710,11 +1707,8 @@ static int mcdx_tray_move(struct cdrom_d
 	if (!(stuffp->present & DOOR))
 		return -ENOSYS;

drivers/cdrom/mcdx.c static int mcdx_tray_move(struct cdrom_d · line 1714

Quoted 10 lines

drivers/cdrom/mcdx.c +2 -5

-	if (position)		/* 1: eject */
-		return mcdx_talk(stuffp, "\xf6", 1, NULL, 1, 5 * HZ, 3);
-	else			/* 0: close */
-		return mcdx_talk(stuffp, "\xf8", 1, NULL, 1, 5 * HZ, 3);
-	return 1;
+	/* If position is 1 then eject, otherwise close */
+	return mcdx_talk(stuffp, position ? "\xf6" : "\xf8", 1, NULL, 1, 5 * HZ, 3);
 }

Not to be picky, but isn't it: "If position is 0 then close, otherwise eject"? ;)

Richard Knutsson

_______________________________________________
Kernel-janitors mailing list
[email protected]
https://lists.osdl.org/mailman/listinfo/kernel-janitors