Re: [PATCH]drivers/ata/libata-eh.c:1509 unneeded memset()

Re: [PATCH]drivers/ata/libata-eh.c:1509 unneeded memset()

[Jeff Garzik]

On 12/30/2009 05:59 PM, René Bolldorf wrote:
> We don't need this ;-).
>
> Best regards René Bolldorf & a happy new year in advance.
>
> --- ./drivers/ata/libata-eh.c 2009-12-30 23:44:05.578988545 +0100
> +++ ./drivers/ata/libata-eh.c 2009-12-30 23:45:06.991987607 +0100
> @@ -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

Re: [PATCH]drivers/ata/libata-eh.c:1509 unneeded memset()

[René Bolldorf]

On 01/07/10 20:15, Jeff Garzik wrote:
> 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.

Re: [PATCH]drivers/ata/libata-eh.c:1509 unneeded memset()

[René Bolldorf]

On 01/07/10 21:34, René Bolldorf wrote:
> On 01/07/10 20:15, Jeff Garzik wrote:
>> 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.

Re: [PATCH]drivers/ata/libata-eh.c:1509 unneeded memset()

[Rolf Eike Beer]

[-- Attachment #1: Type: Text/Plain, Size: 623 bytes --]

René Bolldorf wrote:
> On 01/07/10 21:34, René Bolldorf wrote:
> > On 01/07/10 20:15, Jeff Garzik wrote:
> >> 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.

What happens to sense_buf[1]?

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

Re: [PATCH]drivers/ata/libata-eh.c:1509 unneeded memset()

[James Bottomley]

On Thu, 2010-01-07 at 14:15 -0500, Jeff Garzik wrote:
> On 12/30/2009 05:59 PM, René Bolldorf wrote:
> > We don't need this ;-).
> >
> > Best regards René Bolldorf & a happy new year in advance.
> >
> > --- ./drivers/ata/libata-eh.c 2009-12-30 23:44:05.578988545 +0100
> > +++ ./drivers/ata/libata-eh.c 2009-12-30 23:45:06.991987607 +0100
> > @@ -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?

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

Re: [PATCH]drivers/ata/libata-eh.c:1509 unneeded memset()

[René Bolldorf]

On 01/08/10 16:28, James Bottomley wrote:
> On Thu, 2010-01-07 at 14:15 -0500, Jeff Garzik wrote:
>> On 12/30/2009 05:59 PM, René Bolldorf wrote:
>>> We don't need this ;-).
>>>
>>> Best regards René Bolldorf&  a happy new year in advance.
>>>
>>> --- ./drivers/ata/libata-eh.c 2009-12-30 23:44:05.578988545 +0100
>>> +++ ./drivers/ata/libata-eh.c 2009-12-30 23:45:06.991987607 +0100
>>> @@ -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?
>
> 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
>
>

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 majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH]drivers/ata/libata-eh.c:1509 unneeded memset()

[Marc Bejarano]

René Bolldorf <xsecute <at> googlemail.com> writes:
> On 01/08/10 16:28, James Bottomley wrote:
> > On Thu, 2010-01-07 at 14:15 -0500, Jeff Garzik wrote:
> >> On 12/30/2009 05:59 PM, René Bolldorf wrote:
> >>> 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);
> >
> > 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 majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH]drivers/ata/libata-eh.c:1509 unneeded memset()

[Marc Bejarano]

and once more in plain text.  (sorry vger)

2010/1/11 Marc Bejarano <beej@beej.org>:
> oops.  seems that GMANE's reply function only goes to a single "newsgroup".
>  original recipients re-added.
> marc
> ----
> From: Marc Bejarano <beej <at> beej.org>
> Subject: Re: [PATCH]drivers/ata/libata-eh.c:1509 unneeded memset()
> Newsgroups: gmane.linux.scsi
> Date: 2010-01-08 22:21:09 GMT (2 days, 20 hours and 40 minutes ago)
>
> René Bolldorf <xsecute <at> googlemail.com> writes:
>> On 01/08/10 16:28, James Bottomley wrote:
>> > On Thu, 2010-01-07 at 14:15 -0500, Jeff Garzik wrote:
>> >> On 12/30/2009 05:59 PM, René Bolldorf wrote:
>> >>> 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);
>> >
>> > 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 majordomo <at> vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>
> ----

Re: [PATCH]drivers/ata/libata-eh.c:1509 unneeded memset()

[René Bolldorf]

On 01/11/10 20:41, Marc Bejarano wrote:
> and once more in plain text.  (sorry vger)
>
> 2010/1/11 Marc Bejarano<beej@beej.org>:
>> oops.  seems that GMANE's reply function only goes to a single "newsgroup".
>>   original recipients re-added.
>> marc
>> ----
>> From: Marc Bejarano<beej<at>  beej.org>
>> Subject: Re: [PATCH]drivers/ata/libata-eh.c:1509 unneeded memset()
>> Newsgroups: gmane.linux.scsi
>> Date: 2010-01-08 22:21:09 GMT (2 days, 20 hours and 40 minutes ago)
>>
>> René Bolldorf<xsecute<at>  googlemail.com>  writes:
>>> On 01/08/10 16:28, James Bottomley wrote:
>>>> On Thu, 2010-01-07 at 14:15 -0500, Jeff Garzik wrote:
>>>>> On 12/30/2009 05:59 PM, René Bolldorf wrote:
>>>>>> 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);
>>>>
>>>> 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 majordomo<at>  vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>
>> ----

I hope that's good enough explained :-).

Best regards
René

--- ./drivers/ata/libata-eh.c   2010-01-07 23:21:23.622464012 +0100
+++ ./drivers/ata/libata-eh.c   2010-01-11 21:11:19.869092897 +0100
@@ -1505,7 +1505,10 @@ static unsigned int atapi_eh_request_sen

         DPRINTK("ATAPI request sense\n");

-       /* FIXME: is this needed? */
+       /* make sure sense_buf is cleared then atapi_eh_request_sense is 
called.
+        * (to make sure nothing get's reused.)
+        * thanks to James Bottomley
+        */
         memset(sense_buf, 0, SCSI_SENSE_BUFFERSIZE);

         /* initialize sense_buf with the error register,

Re: [PATCH]drivers/ata/libata-eh.c:1509 unneeded memset()

[Marc Bejarano]

2010/1/11 René Bolldorf <xsecute@googlemail.com>:
> I hope that's good enough explained :-).

definitely an improvement, but i think jeff is going to want your
S-O-B and at least a patch title, if no description :)


> +       /* make sure sense_buf is cleared then atapi_eh_request_sense is
> called.

s/then/when/ ?

> +        * (to make sure nothing get's reused.)

probably not necessary

> +        * thanks to James Bottomley

this may be better in a patch description

cheers,
marc