| Message ID | 911c68378b91bf275a4bbed5717b4639641d7029.1471002480.git.tom.ty89@gmail.com (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
On 08/12/2016 02:56 PM, tom.ty89@gmail.com wrote: > From: Tom Yan <tom.ty89@gmail.com> > > Currently we use dev->max_sectors to set max_hw_sectors, which > is actually supposed to be a host controller limit (that get set Gets. > by the host controller driver like ahci, and if not it would be > the fallback SCSI_DEFAULT_MAX_SECTORS). > > That means we have been doing the wrong thing. Therefore, use it > to set the correct request queue limit that is used to report > device limit: max_dev_sectors. > > For disk devices, it is reported through the Maximum Transfer Length > field on the Block Limits VPD to the SCSI disk driver, which will > then set and make use of max_dev_sectors. > > For cdrom devices, since they are not supposed to have any VPD, and > neither will the SCSI cdrom (sr) driver touch any of the max sectors > limits, we are hence setting the limit directly in libata-scsi. > > Note that when max_dev_sectors is larger than max_hw_sectors, it > does not have any effect. Therefore, setting dev->max_sectors to > ATA_MAX_SECTORS_LBA48 will only have some effect when the host > driver has set max_hw_sectors to a value that is as large as that. > > This should not matter for typical devices, since according to our > past experience, 1024 (the current SCSI_DEFAULT_MAX_SECTORS) is a > decent and safe value for max_sectors. ATA_MAX_SECTORS_LBA48 has > actually appeared to be too large for some devices. > > Also note that ATA_HORKAGE_MAX_SEC_LBA48 is not supposed to work > automatically anyway, even when max_hw_sectors is as high as 65535, > since the effective max_sectors will be set by the SCSI disk driver. > > Signed-off-by: Tom Yan <tom.ty89@gmail.com> > > diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c > index be9c76c..4e2d8e7 100644 > --- a/drivers/ata/libata-scsi.c > +++ b/drivers/ata/libata-scsi.c > @@ -1204,14 +1204,26 @@ static int ata_scsi_dev_config(struct scsi_device *sdev, > if (!ata_id_has_unload(dev->id)) > dev->flags |= ATA_DFLAG_NO_UNLOAD; > > - /* configure max sectors */ > - blk_queue_max_hw_sectors(q, dev->max_sectors); > - > if (dev->class == ATA_DEV_ATAPI) { > void *buf; > > sdev->sector_size = ATA_SECT_SIZE; > > + /* > + * We are setting the limit here merely because CD/DVD device does not > + * have Block Limits VPD. > + * > + * Supposedly dev->max_sectors should be left shifted by > + * (ilog2(sdev->sector_size) - 9). But since ATAPI class device has a > + * static logical sector size of 512 (ATA_SECT_SIZE), the shift became > + * unnecessary. > + */ > + q->limits.max_dev_sectors = dev->max_sectors; > + /* Make max_dev_sectors effective by adjusting max_sectors accordingly, > + while leave max_hw_sectors, which is supposed to be host controller > + limit, untouched. */ Why 2 different comment styles? The previous comment's style is actually preferred in the kernel. [...] MBR, Sergei -- 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
On 12 August 2016 at 19:56, <tom.ty89@gmail.com> wrote: > > Also note that ATA_HORKAGE_MAX_SEC_LBA48 is not supposed to work > automatically anyway, even when max_hw_sectors is as high as 65535, > since the effective max_sectors will be set by the SCSI disk driver. > I missed the fact that ATA_HORKAGE_MAX_SEC_LBA48 is only (and should only be) used by a few ATAPI devices, and the SCSI cdrom devices will not touch max_sectors like the SCSI disk driver. Therefore, I guess I should really just use blk_queue_max_hw_sectors() to set both max_hw_sectors and max_sectors for ATAPI devices. Sending a v2. -- 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
On 12 August 2016 at 21:42, Sergei Shtylyov <sergei.shtylyov@cogentembedded.com> wrote: > On 08/12/2016 02:56 PM, tom.ty89@gmail.com wrote: > >> From: Tom Yan <tom.ty89@gmail.com> >> >> Currently we use dev->max_sectors to set max_hw_sectors, which >> is actually supposed to be a host controller limit (that get set > > > Gets. > > Thanks, but I read too late. I'll try to bare in mind to correct that if I'll need to send a v3 or so. >> diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c >> index be9c76c..4e2d8e7 100644 >> --- a/drivers/ata/libata-scsi.c >> +++ b/drivers/ata/libata-scsi.c >> @@ -1204,14 +1204,26 @@ static int ata_scsi_dev_config(struct scsi_device >> *sdev, >> if (!ata_id_has_unload(dev->id)) >> dev->flags |= ATA_DFLAG_NO_UNLOAD; >> >> - /* configure max sectors */ >> - blk_queue_max_hw_sectors(q, dev->max_sectors); >> - >> if (dev->class == ATA_DEV_ATAPI) { >> void *buf; >> >> sdev->sector_size = ATA_SECT_SIZE; >> >> + /* >> + * We are setting the limit here merely because CD/DVD >> device does not >> + * have Block Limits VPD. >> + * >> + * Supposedly dev->max_sectors should be left shifted by >> + * (ilog2(sdev->sector_size) - 9). But since ATAPI class >> device has a >> + * static logical sector size of 512 (ATA_SECT_SIZE), the >> shift became >> + * unnecessary. >> + */ >> + q->limits.max_dev_sectors = dev->max_sectors; >> + /* Make max_dev_sectors effective by adjusting max_sectors >> accordingly, >> + while leave max_hw_sectors, which is supposed to be >> host controller >> + limit, untouched. */ > > > Why 2 different comment styles? The previous comment's style is actually > preferred in the kernel. > I just tried to follow the styles of the existing comments. Apparently the first style is used for multi-paragraph comments, while the other one is used for single-paragraph one. -- 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
Hello. On 8/12/2016 5:36 PM, Tom Yan wrote: >>> diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c >>> index be9c76c..4e2d8e7 100644 >>> --- a/drivers/ata/libata-scsi.c >>> +++ b/drivers/ata/libata-scsi.c >>> @@ -1204,14 +1204,26 @@ static int ata_scsi_dev_config(struct scsi_device >>> *sdev, >>> if (!ata_id_has_unload(dev->id)) >>> dev->flags |= ATA_DFLAG_NO_UNLOAD; >>> >>> - /* configure max sectors */ >>> - blk_queue_max_hw_sectors(q, dev->max_sectors); >>> - >>> if (dev->class == ATA_DEV_ATAPI) { >>> void *buf; >>> >>> sdev->sector_size = ATA_SECT_SIZE; >>> >>> + /* >>> + * We are setting the limit here merely because CD/DVD >>> device does not >>> + * have Block Limits VPD. >>> + * >>> + * Supposedly dev->max_sectors should be left shifted by >>> + * (ilog2(sdev->sector_size) - 9). But since ATAPI class >>> device has a >>> + * static logical sector size of 512 (ATA_SECT_SIZE), the >>> shift became >>> + * unnecessary. >>> + */ >>> + q->limits.max_dev_sectors = dev->max_sectors; >>> + /* Make max_dev_sectors effective by adjusting max_sectors >>> accordingly, >>> + while leave max_hw_sectors, which is supposed to be >>> host controller >>> + limit, untouched. */ >> >> >> Why 2 different comment styles? The previous comment's style is actually >> preferred in the kernel. >> > > I just tried to follow the styles of the existing comments. Apparently > the first style is used for multi-paragraph comments, while the other > one is used for single-paragraph one. Nevertheless, only the 1st comment is formatted in the preferred manner; see Documentation/CodingStyle, chapter 8. MBR, Sergei -- 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
diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c index be9c76c..4e2d8e7 100644 --- a/drivers/ata/libata-scsi.c +++ b/drivers/ata/libata-scsi.c @@ -1204,14 +1204,26 @@ static int ata_scsi_dev_config(struct scsi_device *sdev, if (!ata_id_has_unload(dev->id)) dev->flags |= ATA_DFLAG_NO_UNLOAD; - /* configure max sectors */ - blk_queue_max_hw_sectors(q, dev->max_sectors); - if (dev->class == ATA_DEV_ATAPI) { void *buf; sdev->sector_size = ATA_SECT_SIZE; + /* + * We are setting the limit here merely because CD/DVD device does not + * have Block Limits VPD. + * + * Supposedly dev->max_sectors should be left shifted by + * (ilog2(sdev->sector_size) - 9). But since ATAPI class device has a + * static logical sector size of 512 (ATA_SECT_SIZE), the shift became + * unnecessary. + */ + q->limits.max_dev_sectors = dev->max_sectors; + /* Make max_dev_sectors effective by adjusting max_sectors accordingly, + while leave max_hw_sectors, which is supposed to be host controller + limit, untouched. */ + blk_queue_max_hw_sectors(q, queue_max_hw_sectors(q)); + /* set DMA padding */ blk_queue_update_dma_pad(q, ATA_DMA_PAD_SZ - 1); @@ -2310,6 +2322,13 @@ static unsigned int ata_scsiop_inq_b0(struct ata_scsi_args *args, u8 *rbuf) put_unaligned_be16(min_io_sectors, &rbuf[6]); /* + * Maximum transfer length. + * + * This will be used by the SCSI disk driver to set max_dev_sectors. + */ + put_unaligned_be32(args->dev->max_sectors, &rbuf[8]); + + /* * Optimal unmap granularity. * * The ATA spec doesn't even know about a granularity or alignment