Re: [PATCH v5 09/17] s390/dasd: Add dasd_eckd_build_cp_tpm_writefulltrack()

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] Unconditional iteration over payload segments violates subsystem guidelines and risks a NULL pointer dereference for payload-less requests.
- [Low] Unused static function `dasd_eckd_build_cp_tpm_writefulltrack` causes a build warning/breakage.
--

--- Patch [9]: [PATCH v5 09/17] s390/dasd: Add dasd_eckd_build_cp_tpm_writefulltrack() ---
commit 2c25716c288b80bde9127da58f3afd957ee39c9c
Author: Stefan Haberland <[email protected]>
s390/dasd: Add dasd_eckd_build_cp_tpm_writefulltrack()

This commit adds the channel program builder for WRITE_FULL_TRACK requests.
It emits an eckd_r0 header, an eckd_count plus data pair for every record,
and a terminating pseudo-count to format and write tracks atomically.

> diff --git a/drivers/s390/block/dasd_eckd.c b/drivers/s390/block/dasd_eckd.c
> index c1a26485f506d..8d8bd40ba896b 100644
> --- a/drivers/s390/block/dasd_eckd.c
> +++ b/drivers/s390/block/dasd_eckd.c
[ ... ]
> @@ -4741,6 +4749,339 @@ static struct dasd_ccw_req *dasd_eckd_build_cp_tpm_track(
[ ... ]
> +static struct dasd_ccw_req *dasd_eckd_build_cp_tpm_writefulltrack(struct dasd_device *startdev,

[Severity: Low]
Because this static function is defined but not called anywhere in this
commit, will this generate a -Wunused-function warning and potentially break
builds with -Werror?

> +								  struct dasd_block *block,
> +								  struct request *req,
> +								  sector_t first_rec,
> +								  sector_t last_rec,
> +								  sector_t first_trk,
> +								  sector_t last_trk,
> +								  unsigned int first_offs,
> +								  unsigned int last_offs,
> +								  unsigned int blk_per_trk,
> +								  unsigned int blksize,
> +								  struct dasd_ccw_req *ocqr)
> +{
[ ... ]
> +	/* process data records */
> +	rec = first_offs + 1;
> +	rec_count = 0;
> +	rq_for_each_segment(bv, req, iter) {
> +		dst = bvec_virt(&bv);

[Severity: Medium]
Does this code risk a NULL pointer dereference for payload-less requests?

Iterating over a bio without data using rq_for_each_segment() triggers a
NULL pointer dereference on bio->bi_io_vec. Subsystem guidelines require
using a bio_has_data() guard before accessing data fields. If a payload-less
request (like FLUSH or DISCARD) is passed here, it could crash.

> +		seg_len = bv.bv_len;
> +		while (seg_len) {
[ ... ]

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