On 08/11/2009 06:13 PM, James Bottomley wrote: > On Tue, 2009-08-11 at 10:53 -0400, Alan Stern wrote: >> On Tue, 11 Aug 2009, Boaz Harrosh wrote: >> >>> This is certainly a bug. Otherwise I would get all my pages 4 bytes short >>> and wonder why. >>> >>> I wish the bug would explain that stupid USB device Martin was fixing. >>> "I die if evpd page=0 is read" is a very brain dead thing. But there >>> is no overflow in current code, only underflow. >>> >>> If you are at it could you please fix all the bugs in this code: ;-) >> >> The USB problem shouldn't affect anything thanks to Martin's other >> changes (sd won't read VPD for devices with scsi_level <= SCSI_2). So >> how does this revised patch look? >> >> Alan Stern >> >> >> Index: usb-2.6/drivers/scsi/scsi.c >> =================================================================== >> --- usb-2.6.orig/drivers/scsi/scsi.c >> +++ usb-2.6/drivers/scsi/scsi.c >> @@ -969,7 +969,7 @@ EXPORT_SYMBOL(scsi_track_queue_full); >> * @sdev: The device to ask >> * @buffer: Where to put the result >> * @page: Which Vital Product Data to return >> - * @len: The length of the buffer >> + * @len: The length of the data (= buffer length - 4) > > Really, no. The former is the correct (and universally used > definition). This one you propose is asking for confusion and misuse. > This is what is done today Only documented If you want to fix it to be the full buffer size the user code should be fixed >> * >> * This is an internal helper function. You probably want to use >> * scsi_get_vpd_page instead. >> @@ -980,7 +980,10 @@ static int scsi_vpd_inquiry(struct scsi_ >> u8 page, unsigned len) >> { >> int result; >> - unsigned char cmd[16]; >> + int resid; >> + unsigned char cmd[6]; >> + >> + len += 4; /* Include room for the header bytes */ >> >> cmd[0] = INQUIRY; >> cmd[1] = 1; /* EVPD */ >> @@ -989,17 +992,19 @@ static int scsi_vpd_inquiry(struct scsi_ >> cmd[4] = len & 0xff; >> cmd[5] = 0; /* Control byte */ >> >> + buffer[1] = ~page; > > This is pointless and dangerous: Some architectures will invalidate > caches for DMA not flush them, so it might not do what you think it > does. > Then you can't do that "after check" either. It's a simple minefield. What do you suggest? > James > > Boaz -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html