On Wed, 2009-11-04 at 18:18 +0200, Boaz Harrosh wrote:
> On 11/04/2009 05:09 PM, James Bottomley wrote:
> > On Wed, 2009-11-04 at 10:54 +0200, Boaz Harrosh wrote:
> >>> +
> >>> + if (i < buf[3] && i > buf_len)
> >>> + /* ran off the end of the buffer, give us benefit of doubt */
> >>> + goto found;
> >>
> >> Some cheep devices are known to break when asked for pages they do not support
> >> better return an -ETOOSMALL the user can check for. (And the comment above should
> >> also take care of it)
> >
> > OK, so I struggled with this. The reason for the behaviour is that
> > only USB devices with incredibly small page lists are likely to exhibit
> > the problem. Whereas the page lists in array type devices are likely to
> > grow. I don't want to run into the situation that your new $10m array
> > suddenly gives an error with DIF/DIX because the page list is too big
> > and the problem this is checking for only afflicts cheap and badly
> > implemented HW.
> >
>
> If I remember the standard correctly and from inspecting the code 260 is the
> maximum size for page 0. (And the maximum we supported until today for page
> 0 was 256) so there is no speculations here. 256 it should be. If you want
> you can do an:
The standard actually lists only two mandatory pages and about eight
optional ones (plus the 127 vendor specific ones that no-one seems to
use).
The point isn't to exercise the standard to the maximum, it's to
generate working code. The point is that problem devices likely only
list a couple of VPD pages, so we'll correctly not probe them.
Conforming devices don't matter because they'll do the right thing when
asked for a VPD page they don't have.
> if (i < buf[3] && i > buf_len) {
> if (likely(buf_len >= 255))
> /* ran off the end of the buffer, give us benefit of doubt */
> goto found;
> else
> return -ETOOSMALL;
> }
>
> which is only theoretical because we only check against buf[3] that cannot
> be more then 255 so all is left is return -ETOOSMALL;
>
> Callers that allocate smaller then 256 for this (hence the comment)
> must check for -ETOOSMALL;. Or should call with >= 256.
>
> <snip>
> >>> diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
> >>> index 9093c72..fd1bd8f 100644
> >>> --- a/drivers/scsi/sd.c
> >>> +++ b/drivers/scsi/sd.c
> >>> @@ -1864,19 +1864,20 @@ void sd_read_app_tag_own(struct scsi_disk *sdkp, unsigned char *buffer)
> >>> static void sd_read_block_limits(struct scsi_disk *sdkp)
> >>> {
> >>> unsigned int sector_sz = sdkp->device->sector_size;
> >>> - char *buffer;
> >>> + const int vpd_len = 32;
> >>> + unsigned char *buffer = kmalloc(vpd_len, GFP_KERNEL);
> >>>
> >>
> >> 32 sounds two small for me. Not because of the page but because of the
> >> first pass. Why not just 255 as a rule. We kmalloc it anyway.
> >
> > It only needs 16 if you look at the code. 32 is actually the smallest
> > kmalloc slab on 32 bit systems.
> >
>
> >> We used to allocate 255 here, for some devices out there this is surly
> >> a regression.
> >
> > We did 255 because we were concerned about space for the page list ...
> > hopefully I answered that one above.
> >
>
> No, I still disagree. Whats the point of checking at all? Surly there is a device
> out there that has more then 12 pages but will breaks with wrong page. So what's
> the limit? the standards say 260 why not stick with that. It's not at any hot path.
The point of checking is not to send a VPD inquiry to USB devices that
don't support it. These have a very limited range of supported VPD
pages.
James
--
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
