On Tue, Jan 30, 2024 at 11:46:24AM +0000, John Garry wrote:
>> +{
>> + if (!lim->zoned) {
>> + if (WARN_ON_ONCE(lim->max_open_zones) ||
>> + WARN_ON_ONCE(lim->max_active_zones) ||
>> + WARN_ON_ONCE(lim->zone_write_granularity) ||
>> + WARN_ON_ONCE(lim->max_zone_append_sectors))
>
> nit: some - like me - prefer {} for multi-line if statements, but that's
> personal taste
>
>> + return -EINVAL;
That would be really weird and contrary to the normal Linux style.
>> + if (!lim->logical_block_size)
>> + lim->logical_block_size = SECTOR_SIZE;
>> + if (lim->physical_block_size < lim->logical_block_size)
>> + lim->physical_block_size = lim->physical_block_size;
>
> I guess that should really be:
> lim->physical_block_size = lim->logical_block_size;
Thanks, that does need fixing.
>> + lim->max_hw_sectors = round_down(lim->max_hw_sectors,
>> + lim->logical_block_size >> SECTOR_SHIFT);
>> +
>> + /*
>> + * The actual max_sectors value is a complex beast and also takes the
>> + * max_dev_sectors value (set by SCSI ULPs) and a user configurable
>> + * value into account. The ->max_sectors value is always calculated
>> + * from these, so directly setting it won't have any effect.
>> + */
>> + max_hw_sectors = min_not_zero(lim->max_hw_sectors,
>> + lim->max_dev_sectors);
>
> nit: maybe we should use a different variable for this for sake of clarity
What variable name would work better for you?
>> + /*
>> + * We require drivers to at least do logical block aligned I/O, but
>> + * historically could not check for that due to the separate calls
>> + * to set the limits. Once the transition is finished the check
>> + * below should be narrowed down to check the logical block size.
>> + */
>> + if (!lim->dma_alignment)
>> + lim->dma_alignment = SECTOR_SIZE - 1;
>
> It would be also nice to update blk_set_default_limits to use this (and not
> '511') or also any other instances of hard-coding SECTOR_SIZE for 512
That would be nice, but defintively not in scope for this patch.
