On 2021/11/27 18:58, Niklas Cassel wrote: > On Sat, Nov 27, 2021 at 10:00:57AM +0900, Damien Le Moal wrote: >> On 2021/11/26 21:55, Niklas Cassel wrote: >>> From: Niklas Cassel <niklas.cassel@xxxxxxx> >>> >>> sd_zbc_parse_report() fills in a struct blk_zone, which is the block layer >>> representation of a zone. This struct is also what will be copied to user >>> for a BLKREPORTZONE ioctl. >>> >>> Since sd_zbc_parse_report() compares against zone.type and zone.cond, which >>> are members of a struct blk_zone, the correct enum values to compare >>> against are the enum values defined by the block layer. >>> >>> These specific enum values for ZBC and the block layer happen to have the >>> same enum constants, but they could theoretically have been different. >>> >>> Compare against the block layer enum values, to make it more obvious that >>> struct blk_zone is the block layer representation of a zone, and not the >>> SCSI/ZBC representation of a zone. >>> >>> Signed-off-by: Niklas Cassel <niklas.cassel@xxxxxxx> >>> --- >>> drivers/scsi/sd_zbc.c | 4 ++-- >>> 1 file changed, 2 insertions(+), 2 deletions(-) >>> >>> diff --git a/drivers/scsi/sd_zbc.c b/drivers/scsi/sd_zbc.c >>> index ed06798983f8..024f1bec6e5a 100644 >>> --- a/drivers/scsi/sd_zbc.c >>> +++ b/drivers/scsi/sd_zbc.c >>> @@ -62,8 +62,8 @@ static int sd_zbc_parse_report(struct scsi_disk *sdkp, u8 *buf, >>> zone.capacity = zone.len; >>> zone.start = logical_to_sectors(sdp, get_unaligned_be64(&buf[16])); >>> zone.wp = logical_to_sectors(sdp, get_unaligned_be64(&buf[24])); >>> - if (zone.type != ZBC_ZONE_TYPE_CONV && >>> - zone.cond == ZBC_ZONE_COND_FULL) >>> + if (zone.type != BLK_ZONE_TYPE_CONVENTIONAL && >>> + zone.cond == BLK_ZONE_COND_FULL) >>> zone.wp = zone.start + zone.len; >> >> For the sake of avoiding layering violation, I would keep the code as is, unles >> Martin and James are OK with this ? > > Sorry, but I don't understand this comment. > > The whole point of sd_zbc_parse_report() is to take a ZBC zone representation, > stored in u8 *buf, and to convert it to a struct blk_zone used by the block > layer. Yes. So what is the problem with using the scsi_proto.h defined ZBC_ZONE_* macros ? We are deep in scsi territory with this code, so using an UAPI defined macro is weird. > Similarly, nvme_zone_parse_entry() takes a ZNS zone representation, stored in a > struct nvme_zone_descriptor *entry, and to convert it to a struct blk_zone. > > > When comparing against struct members inside entry, the NVMe enums have to be > used, i.e. NVME_ZONE_TYPE_SEQWRITE_REQ. > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/nvme/host/zns.c#n158 > > However, assigning, or comparing against struct members of struct blk_zone, > the blk layer enums have to be used, i.e. BLK_ZONE_TYPE_SEQWRITE_REQ: > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/nvme/host/zns.c#n164 > > And why did you give me your Reviewed-by on the NVMe patch that uses the > blk later enums here: > https://lore.kernel.org/linux-nvme/ef1c39ab-7b56-6a37-0f4f-1ca111d5b48b@xxxxxxxxxxxxxxxxxx/T/#t > > Be consistent, either ack both or nack both :) I am not nacking anything. I am giving my opinion, which is that I find this code change useless. >> A more sensible patch may be to add a static checking that all BLK_ZONE_COND_* >> and BLK_ZONE_TYPE_* enum values are equal to the ZBC defined values in >> include/scsi/scsi_proto.h (ZBC_ZONE_COND_* and ZBC_ZONE_TYPE_* macros). > > The blk-zoned block layer is obviously modeled after ZBC, that is why all the > enum constants happen to be the same. But this obviously doesn't have to be > true for all existing/future lower level interfaces which supports zones. If you are worried that sd_zbc_parse_report() does not fill the values as defined for struct blk_zone, then add something like: static_assert(BLK_ZONE_COND_FULL == ZBC_ZONE_COND_FULL); static_assert(BLK_ZONE_TYPE_CONVENTIONAL == ZBC_ZONE_TYPE_CONV); at the beginning of that function. blk_dev_revalidate_zones() will check everything is valid anyway. -- Damien Le Moal Western Digital Research
