Re: [PATCH v7 10/19] s390/dasd: Extend prepare_itcw() to support WRITE_FULL_TRACK

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] Hardware protocol violation in newly added WRITE_FULL_TRACK support
--

--- Patch [10]: [PATCH v7 10/19] s390/dasd: Extend prepare_itcw() to support WRITE_FULL_TRACK ---
commit 957b430fd410ae687b07b4353fc0a1d151996c40
Author: Stefan Haberland <[email protected]>

s390/dasd: Extend prepare_itcw() to support WRITE_FULL_TRACK

This commit extends prepare_itcw() to handle the
DASD_ECKD_CCW_WRITE_FULL_TRACK command. It appends two extra bytes to the LRE
for the bitmask and sizes the prefix block buffer dynamically on the stack
to prevent allocation failures under memory pressure during writeback.

> diff --git a/drivers/s390/block/dasd_eckd.c b/drivers/s390/block/dasd_eckd.c
> index 678d6f353e882..b54d1858db160 100644
> --- a/drivers/s390/block/dasd_eckd.c
> +++ b/drivers/s390/block/dasd_eckd.c
[ ... ]
> @@ -4448,7 +4455,38 @@ static int prepare_itcw(struct itcw *itcw,
>  		 * data as well.
>  		 */
>  		if (dedata->ga_extended & 0x08 && dedata->ga_extended & 0x02)
> -			pfxdata.validity.time_stamp = 1; /* 'Time Stamp Valid' */
> +			pfxdata->validity.time_stamp = 1; /* 'Time Stamp Valid' */
> +		pfx_cmd = DASD_ECKD_CCW_PFX;
> +		break;
> +	case DASD_ECKD_CCW_WRITE_FULL_TRACK:
> +		dedata->mask.perm = 0x3;
> +		dedata->mask.auth = 0x00;
> +		dedata->attributes.operation = basepriv->attrib.operation;
> +		dedata->blk_size = blksize;
[ ... ]
> +		if (dedata->ga_extended & 0x08 && dedata->ga_extended & 0x02)
> +			pfxdata->validity.time_stamp = 1; /* 'Time Stamp Valid' */
>  		pfx_cmd = DASD_ECKD_CCW_PFX;
>  		break;
>  	case DASD_ECKD_CCW_READ_COUNT_MT:

[Severity: Medium]
Will this result in a hardware protocol violation when the new
DASD_ECKD_CCW_WRITE_FULL_TRACK command is processed further down in the
prepare_itcw() function?

After the command switch statement in prepare_itcw(), there is a catch-all
else block that handles lredata->auxiliary flags. For commands not explicitly
handled by the preceding if statements, it falls into this branch:

	} else {
		lredata->auxiliary.length_valid = 1;
		lredata->auxiliary.length_scope = 1;
		lredata->sector = sector;
	}

Because DASD_ECKD_CCW_WRITE_FULL_TRACK is not explicitly excluded, it falls
into this branch and unconditionally sets length_valid = 1. Formatting
tracks mandates no single valid record length, requiring length_valid = 0
and length = 0 to avoid a Command Reject.

Should the conditions leading to this else branch be updated to properly
handle the LRE format protocol for WRITE_FULL_TRACK?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=10