| Message ID | 4B3BDB66.6000008@googlemail.com |
|---|---|
| State | Not Applicable |
| Delegated to: | David Miller |
| Headers | show |
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 -- To unsubscribe from this list: send the line "unsubscribe linux-ide" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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. -- To unsubscribe from this list: send the line "unsubscribe linux-ide" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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. -- To unsubscribe from this list: send the line "unsubscribe linux-ide" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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]?
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 -- To unsubscribe from this list: send the line "unsubscribe linux-ide" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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-ide" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
--- ./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); - /* initialize sense_buf with the error register, * for the case where they are -not- overwritten */