* [PATCH 1/2] libata-scsi: use dev->max_sectors from libata-core appropriately
@ 2016-08-12 11:56 tom.ty89
2016-08-12 11:56 ` [PATCH 2/2] sd: check BLK_DEF_MAX_SECTORS against max_dev_sectors tom.ty89
` (2 more replies)
0 siblings, 3 replies; 6+ messages in thread
From: tom.ty89 @ 2016-08-12 11:56 UTC (permalink / raw)
To: tj, martin.petersen; +Cc: linux-ide, linux-scsi, linux-block, Tom Yan
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
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. */
+ 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
--
2.9.2
^ permalink raw reply related [flat|nested] 6+ messages in thread
* [PATCH 2/2] sd: check BLK_DEF_MAX_SECTORS against max_dev_sectors
2016-08-12 11:56 [PATCH 1/2] libata-scsi: use dev->max_sectors from libata-core appropriately tom.ty89
@ 2016-08-12 11:56 ` tom.ty89
2016-08-12 13:42 ` [PATCH 1/2] libata-scsi: use dev->max_sectors from libata-core appropriately Sergei Shtylyov
2016-08-12 14:31 ` Tom Yan
2 siblings, 0 replies; 6+ messages in thread
From: tom.ty89 @ 2016-08-12 11:56 UTC (permalink / raw)
To: tj, martin.petersen; +Cc: linux-ide, linux-scsi, linux-block, Tom Yan
From: Tom Yan <tom.ty89@gmail.com>
The SCSI disk driver sets max_sector to BLK_DEF_MAX_SECTORS when
the device does not report Optimal Transfer Length. However, it
checks only whether it is smaller than max_hw_sectors, but not
max_dev_sectors.
Signed-off-by: Tom Yan <tom.ty89@gmail.com>
diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
index d3e852a..2b6da13 100644
--- a/drivers/scsi/sd.c
+++ b/drivers/scsi/sd.c
@@ -2875,8 +2875,11 @@ static int sd_revalidate_disk(struct gendisk *disk)
logical_to_bytes(sdp, sdkp->opt_xfer_blocks) >= PAGE_SIZE) {
q->limits.io_opt = logical_to_bytes(sdp, sdkp->opt_xfer_blocks);
rw_max = logical_to_sectors(sdp, sdkp->opt_xfer_blocks);
- } else
+ } else {
rw_max = BLK_DEF_MAX_SECTORS;
+ /* Combine with device limits */
+ rw_max = min(rw_max, q->limits.max_dev_sectors);
+ }
/* Combine with controller limits */
q->limits.max_sectors = min(rw_max, queue_max_hw_sectors(q));
--
2.9.2
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH 1/2] libata-scsi: use dev->max_sectors from libata-core appropriately
2016-08-12 11:56 [PATCH 1/2] libata-scsi: use dev->max_sectors from libata-core appropriately tom.ty89
2016-08-12 11:56 ` [PATCH 2/2] sd: check BLK_DEF_MAX_SECTORS against max_dev_sectors tom.ty89
@ 2016-08-12 13:42 ` Sergei Shtylyov
2016-08-12 14:36 ` Tom Yan
2016-08-12 14:31 ` Tom Yan
2 siblings, 1 reply; 6+ messages in thread
From: Sergei Shtylyov @ 2016-08-12 13:42 UTC (permalink / raw)
To: tom.ty89, tj, martin.petersen; +Cc: linux-ide, linux-scsi, linux-block
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 1/2] libata-scsi: use dev->max_sectors from libata-core appropriately
2016-08-12 11:56 [PATCH 1/2] libata-scsi: use dev->max_sectors from libata-core appropriately tom.ty89
2016-08-12 11:56 ` [PATCH 2/2] sd: check BLK_DEF_MAX_SECTORS against max_dev_sectors tom.ty89
2016-08-12 13:42 ` [PATCH 1/2] libata-scsi: use dev->max_sectors from libata-core appropriately Sergei Shtylyov
@ 2016-08-12 14:31 ` Tom Yan
2 siblings, 0 replies; 6+ messages in thread
From: Tom Yan @ 2016-08-12 14:31 UTC (permalink / raw)
To: Tejun Heo, Martin K. Petersen; +Cc: linux-ide, linux-scsi, linux-block, Tom Yan
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.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 1/2] libata-scsi: use dev->max_sectors from libata-core appropriately
2016-08-12 13:42 ` [PATCH 1/2] libata-scsi: use dev->max_sectors from libata-core appropriately Sergei Shtylyov
@ 2016-08-12 14:36 ` Tom Yan
2016-08-13 10:10 ` Sergei Shtylyov
0 siblings, 1 reply; 6+ messages in thread
From: Tom Yan @ 2016-08-12 14:36 UTC (permalink / raw)
To: Sergei Shtylyov
Cc: Tejun Heo, Martin K. Petersen, linux-ide, linux-scsi, linux-block
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.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 1/2] libata-scsi: use dev->max_sectors from libata-core appropriately
2016-08-12 14:36 ` Tom Yan
@ 2016-08-13 10:10 ` Sergei Shtylyov
0 siblings, 0 replies; 6+ messages in thread
From: Sergei Shtylyov @ 2016-08-13 10:10 UTC (permalink / raw)
To: Tom Yan; +Cc: Tejun Heo, Martin K. Petersen, linux-ide, linux-scsi, linux-block
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
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2016-08-13 10:10 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2016-08-12 11:56 [PATCH 1/2] libata-scsi: use dev->max_sectors from libata-core appropriately tom.ty89
2016-08-12 11:56 ` [PATCH 2/2] sd: check BLK_DEF_MAX_SECTORS against max_dev_sectors tom.ty89
2016-08-12 13:42 ` [PATCH 1/2] libata-scsi: use dev->max_sectors from libata-core appropriately Sergei Shtylyov
2016-08-12 14:36 ` Tom Yan
2016-08-13 10:10 ` Sergei Shtylyov
2016-08-12 14:31 ` Tom Yan
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox