drivers/ata/libata-eh.c:1509 unneeded memset()

PATCH In review René Bolldorf 5 participants 8 messages Dec 30, 2009 – Jan 8, 2010

Subsystems (2)

SCSI libata

lore

Share & tools

lore

René Bolldorf

author

2009-12-30 22:59 UTC #0 lore raw

PATCH 1/1 drivers/ata/libata-eh.c:1509 unneeded memset()

We don't need this ;-).

Best regards René Bolldorf & a happy new year in advance.

drivers/ata/libata-eh.c 2009-12-30 23:45:06.991987607 +0100 renamed from drivers/ata/libata-eh.c 2009-12-30 23:44:05.578988545 +0100 +0 -3

@@ -1505,9 +1505,6 @@ static unsigned int atapi_eh_request_sen
 
 	DPRINTK("ATAPI request sense\n");
 
-	/* FIXME: is this needed? */
-	memset(sense_buf, 0, SCSI_SENSE_BUFFERSIZE);
-
 	/* initialize sense_buf with the error register,
 	 * for the case where they are -not- overwritten
 	 */

Jeff Garzik

2010-01-07 19:15 UTC #1 lore raw

drivers/ata/libata-eh.c 2009-12-30 23:45:06.991987607 +0100 static unsigned int atapi_eh_request_sen · line 1509

Quoted 12 lines · René Bolldorf

We don't need this ;-).
Best regards René Bolldorf & a happy new year in advance.
drivers/ata/libata-eh.c 2009-12-30 23:45:06.991987607 +0100 renamed from drivers/ata/libata-eh.c 2009-12-30 23:44:05.578988545 +0100 +0 -2
@@ -1505,9 +1505,6 @@ static unsigned int atapi_eh_request_sen
 
 DPRINTK("ATAPI request sense\n");
 
- /* FIXME: is this needed? */
- memset(sense_buf, 0, SCSI_SENSE_BUFFERSIZE);

I need a little bit more detail than an unqualified statement... Did you audit all paths leading to this code point?

Jeff

René Bolldorf

author

2010-01-07 20:33 UTC #2 lore raw

Quoted 2 lines · Jeff Garzik

I need a little bit more detail than an unqualified statement... Did you audit all paths leading to this code point?

Yes, and my two systems running fine with the patch, no oops or panic's.

René Bolldorf

author

2010-01-07 20:40 UTC #3 lore raw

Quoted 5 lines · René Bolldorf

Quoted 2 lines · Jeff Garzik
I need a little bit more detail than an unqualified statement... Did you audit all paths leading to this code point?
Yes, and my two systems running fine with the patch, no oops or panic's.

Sry forgot that:

	/* initialize sense_buf with the error register,
	 * for the case where they are -not- overwritten
	 */
	sense_buf[0] = 0x70;
	sense_buf[2] = dfl_sense_key;

So i think memset() is not needed and works very well without it.

Rolf Eike Beer

2010-01-08 11:30 UTC #4 lore raw

Quoted 7 lines · René Bolldorf

Quoted 5 lines · René Bolldorf
Quoted 2 lines · Jeff Garzik
I need a little bit more detail than an unqualified statement... Did you audit all paths leading to this code point?
Yes, and my two systems running fine with the patch, no oops or panic's.

Quoted 8 lines

Sry forgot that:
	/* initialize sense_buf with the error register,
	 * for the case where they are -not- overwritten
	 */
	sense_buf[0] = 0x70;
	sense_buf[2] = dfl_sense_key;
So i think memset() is not needed and works very well without it.

What happens to sense_buf[1]?

James Bottomley

2010-01-08 15:28 UTC #5 lore raw

In reply to Jeff Garzik

Quoted 9 lines · Jeff Garzik

Quoted 8 lines · René Bolldorf
We don't need this ;-).
Best regards René Bolldorf & a happy new year in advance.
drivers/ata/libata-eh.c 2009-12-30 23:45:06.991987607 +0100 renamed from drivers/ata/libata-eh.c 2009-12-30 23:44:05.578988545 +0100 +0 -0
@@ -1505,9 +1505,6 @@ static unsigned int atapi_eh_request_sen
 

Quoted 7 lines

drivers/ata/libata-eh.c 2009-12-30 23:45:06.991987607 +0100 static unsigned int atapi_eh_request_sen · line 1509
Quoted 4 lines
drivers/ata/libata-eh.c 2009-12-30 23:45:06.991987607 +0100 renamed from drivers/ata/libata-eh.c 2009-12-30 23:44:05.578988545 +0100 +0 -2
 DPRINTK("ATAPI request sense\n");
 
- /* FIXME: is this needed? */
- memset(sense_buf, 0, SCSI_SENSE_BUFFERSIZE);
I need a little bit more detail than an unqualified statement... Did you audit all paths leading to this code point?

There are two code paths coming into here. One directly from the scsi sense buffer:

		if (!(qc->ap->pflags & ATA_PFLAG_FROZEN)) {
			tmp = atapi_eh_request_sense(qc->dev,
						qc->scsicmd->sense_buffer,
						qc->result_tf.feature >> 4);

Which is fine because SCSI zeros the sense buffer.

But one also here:

		u8 *sense_buffer = dev->link->ap->sector_buf;
		[...]
		err_mask = atapi_eh_request_sense(dev, sense_buffer, sense_key);

Which doesn't look OK because it looks like the sector_buf isn't cleared (and it is reused).

James

René Bolldorf

author

2010-01-08 20:47 UTC #6 lore raw

Quoted 30 lines · James Bottomley

Quoted 9 lines · Jeff Garzik
Quoted 8 lines · René Bolldorf
We don't need this ;-).
Best regards René Bolldorf& a happy new year in advance.
drivers/ata/libata-eh.c 2009-12-30 23:45:06.991987607 +0100 renamed from drivers/ata/libata-eh.c 2009-12-30 23:44:05.578988545 +0100 +0 -0
@@ -1505,9 +1505,6 @@ static unsigned int atapi_eh_request_sen
 
Quoted 7 lines
drivers/ata/libata-eh.c 2009-12-30 23:45:06.991987607 +0100 static unsigned int atapi_eh_request_sen · line 1509
Quoted 4 lines
drivers/ata/libata-eh.c 2009-12-30 23:45:06.991987607 +0100 renamed from drivers/ata/libata-eh.c 2009-12-30 23:44:05.578988545 +0100 +0 -2
 DPRINTK("ATAPI request sense\n");
 
- /* FIXME: is this needed? */
- memset(sense_buf, 0, SCSI_SENSE_BUFFERSIZE);
I need a little bit more detail than an unqualified statement... Did you audit all paths leading to this code point?
There are two code paths coming into here. One directly from the scsi sense buffer:
		if (!(qc->ap->pflags&  ATA_PFLAG_FROZEN)) {
			tmp = atapi_eh_request_sense(qc->dev,
						qc->scsicmd->sense_buffer,
						qc->result_tf.feature>>  4);
Which is fine because SCSI zeros the sense buffer.
But one also here:

Quoted 10 lines

		u8 *sense_buffer = dev->link->ap->sector_buf;
		[...]
		err_mask = atapi_eh_request_sense(dev, sense_buffer, sense_key);
Which doesn't look OK because it looks like the sector_buf isn't cleared (and it is reused).
James

Thank's, you're right. I have overseen this, sry for that. --
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Marc Bejarano

2010-01-08 22:25 UTC #7 lore raw

Quoted 17 lines · René Bolldorf <xsecute <at> googlemail.com> writes:

Quoted 16 lines · James Bottomley
Quoted 8 lines · Jeff Garzik
Quoted 4 lines · René Bolldorf
We don't need this .
- /* FIXME: is this needed? */
- memset(sense_buf, 0, SCSI_SENSE_BUFFERSIZE);
I need a little bit more detail than an unqualified statement... Did you audit all paths leading to this code point?
But one also here:
		u8 *sense_buffer = dev->link->ap->sector_buf;
		[...]
		err_mask = atapi_eh_request_sense(dev, sense_buffer, sense_key);

Quoted 8 lines

Quoted 6 lines
Which doesn't look OK because it looks like the sector_buf isn't cleared (and it is reused).
James
Thank's, you're right. I have overseen this, sry for that.

René: perhaps you'd like to submit a patch that substitutes FIXME comment for one that explains why the memset is needed, crediting James in the description? we may as well gain something permanent from this discussion that you started :)

cheers,
marc

--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html