On Mon, Nov 29, 2021 at 04:35:41PM +0900, Damien Le Moal wrote: > 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. There is no problem with the existing code. I simply think that it is strictly more correct and slightly less confusing to use the BLK_ZONE_ enums when accessing members of struct blk_zone. I didn't see the weirdness of doing so, especially considering that NVMe uses the BLK_ZONE_ enums when assigning members of struct blk_zone, and since struct blk_zone, which is the type we are using here, is itself defined in (and only in) the UAPI header include/uapi/linux/blkzoned.h. Anyway, I will drop this patch from the series and send out a V2 of patch 2/2. Kind regards, Niklas
