[PATCH 0/4] s390/dasd: Minor cleanups and copy-pair swap fix

[PATCH 0/4] s390/dasd: Minor cleanups and copy-pair swap fix

[Stefan Haberland]

Hi Jens,

this small series contains cleanups in the DASD driver
(string formatting, naming helper refactoring, debugfs simplification)
as well as a fix for the gendisk parent after a copy-pair swap.

Please apply. 

Thanks,
Stefan

Jan Höppner (2):
  s390/dasd: Move device name formatting into separate function
  s390/dasd: Use scnprintf() instead of sprintf()

Stefan Haberland (2):
  s390/dasd: Fix gendisk parent after copy pair swap
  s390/dasd: Remove unnecessary debugfs_create() return checks

 drivers/s390/block/dasd.c        | 64 ++++---------------------
 drivers/s390/block/dasd_devmap.c |  3 +-
 drivers/s390/block/dasd_eckd.c   |  8 ++++
 drivers/s390/block/dasd_genhd.c  | 80 +++++++++++++++++++++-----------
 4 files changed, 72 insertions(+), 83 deletions(-)

-- 
2.51.0

[PATCH 1/4] s390/dasd: Fix gendisk parent after copy pair swap

[Stefan Haberland]

After a copy pair swap the block device's "device" symlink points to
the secondary CCW device, but the gendisk's parent remained the
primary, leaving /sys/block/<dasdx> under the wrong parent.

Move the gendisk to the secondary's device with device_move(), keeping
the sysfs topology consistent after the swap.

Fixes: 413862caad6f ("s390/dasd: add copy pair swap capability")
Cc: stable@vger.kernel.org #6.1
Reviewed-by: Jan Hoeppner <hoeppner@linux.ibm.com>
Signed-off-by: Stefan Haberland <sth@linux.ibm.com>
---
 drivers/s390/block/dasd_eckd.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/drivers/s390/block/dasd_eckd.c b/drivers/s390/block/dasd_eckd.c
index 88fa17aea2ec..ec0c62e5ef73 100644
--- a/drivers/s390/block/dasd_eckd.c
+++ b/drivers/s390/block/dasd_eckd.c
@@ -6150,6 +6150,7 @@ static int dasd_eckd_copy_pair_swap(struct dasd_device *device, char *prim_busid
 	struct dasd_copy_relation *copy;
 	struct dasd_block *block;
 	struct gendisk *gdp;
+	int rc;
 
 	copy = device->copy;
 	if (!copy)
@@ -6184,6 +6185,13 @@ static int dasd_eckd_copy_pair_swap(struct dasd_device *device, char *prim_busid
 	/* swap blocklayer device link */
 	gdp = block->gdp;
 	dasd_add_link_to_gendisk(gdp, secondary);
+	rc = device_move(disk_to_dev(gdp), &secondary->cdev->dev, DPM_ORDER_NONE);
+	if (rc) {
+		dev_err(&primary->cdev->dev,
+			"copy_pair_swap: moving blockdevice parent %s->%s failed (%d)\n",
+			dev_name(&primary->cdev->dev),
+			dev_name(&secondary->cdev->dev), rc);
+	}
 
 	/* re-enable device */
 	dasd_device_remove_stop_bits(primary, DASD_STOPPED_PPRC);
-- 
2.51.0

[PATCH 2/4] s390/dasd: Remove unnecessary debugfs_create() return checks

[Stefan Haberland]

The DASD driver only uses the dentry pointers when removing debugfs
entries, and debugfs_remove() can safely handle both NULL and ERR_PTR.
There is therefore no need to check debugfs_create() return values.

This simplifies the debugfs setup code without changing functionality.

Suggested-by: Heiko Carstens <hca@linux.ibm.com>
Reviewed-by: Heiko Carstens <hca@linux.ibm.com>
Reviewed-by: Jan Hoeppner <hoeppner@linux.ibm.com>
Signed-off-by: Stefan Haberland <sth@linux.ibm.com>
---
 drivers/s390/block/dasd.c | 64 +++++----------------------------------
 1 file changed, 8 insertions(+), 56 deletions(-)

diff --git a/drivers/s390/block/dasd.c b/drivers/s390/block/dasd.c
index 7765e40f7cea..496f95745ade 100644
--- a/drivers/s390/block/dasd.c
+++ b/drivers/s390/block/dasd.c
@@ -207,19 +207,6 @@ static int dasd_state_known_to_new(struct dasd_device *device)
 	return 0;
 }
 
-static struct dentry *dasd_debugfs_setup(const char *name,
-					 struct dentry *base_dentry)
-{
-	struct dentry *pde;
-
-	if (!base_dentry)
-		return NULL;
-	pde = debugfs_create_dir(name, base_dentry);
-	if (!pde || IS_ERR(pde))
-		return NULL;
-	return pde;
-}
-
 /*
  * Request the irq line for the device.
  */
@@ -234,14 +221,14 @@ static int dasd_state_known_to_basic(struct dasd_device *device)
 		if (rc)
 			return rc;
 		block->debugfs_dentry =
-			dasd_debugfs_setup(block->gdp->disk_name,
+			debugfs_create_dir(block->gdp->disk_name,
 					   dasd_debugfs_root_entry);
 		dasd_profile_init(&block->profile, block->debugfs_dentry);
 		if (dasd_global_profile_level == DASD_PROFILE_ON)
 			dasd_profile_on(&device->block->profile);
 	}
 	device->debugfs_dentry =
-		dasd_debugfs_setup(dev_name(&device->cdev->dev),
+		debugfs_create_dir(dev_name(&device->cdev->dev),
 				   dasd_debugfs_root_entry);
 	dasd_profile_init(&device->profile, device->debugfs_dentry);
 	dasd_hosts_init(device->debugfs_dentry, device);
@@ -1058,19 +1045,9 @@ static const struct file_operations dasd_stats_raw_fops = {
 static void dasd_profile_init(struct dasd_profile *profile,
 			      struct dentry *base_dentry)
 {
-	umode_t mode;
-	struct dentry *pde;
-
-	if (!base_dentry)
-		return;
-	profile->dentry = NULL;
 	profile->data = NULL;
-	mode = (S_IRUSR | S_IWUSR | S_IFREG);
-	pde = debugfs_create_file("statistics", mode, base_dentry,
-				  profile, &dasd_stats_raw_fops);
-	if (pde && !IS_ERR(pde))
-		profile->dentry = pde;
-	return;
+	profile->dentry = debugfs_create_file("statistics", 0600, base_dentry,
+					      profile, &dasd_stats_raw_fops);
 }
 
 static void dasd_profile_exit(struct dasd_profile *profile)
@@ -1090,25 +1067,9 @@ static void dasd_statistics_removeroot(void)
 
 static void dasd_statistics_createroot(void)
 {
-	struct dentry *pde;
-
-	dasd_debugfs_root_entry = NULL;
-	pde = debugfs_create_dir("dasd", NULL);
-	if (!pde || IS_ERR(pde))
-		goto error;
-	dasd_debugfs_root_entry = pde;
-	pde = debugfs_create_dir("global", dasd_debugfs_root_entry);
-	if (!pde || IS_ERR(pde))
-		goto error;
-	dasd_debugfs_global_entry = pde;
+	dasd_debugfs_root_entry = debugfs_create_dir("dasd", NULL);
+	dasd_debugfs_global_entry = debugfs_create_dir("global", dasd_debugfs_root_entry);
 	dasd_profile_init(&dasd_global_profile, dasd_debugfs_global_entry);
-	return;
-
-error:
-	DBF_EVENT(DBF_ERR, "%s",
-		  "Creation of the dasd debugfs interface failed");
-	dasd_statistics_removeroot();
-	return;
 }
 
 #else
@@ -1169,17 +1130,8 @@ static void dasd_hosts_exit(struct dasd_device *device)
 static void dasd_hosts_init(struct dentry *base_dentry,
 			    struct dasd_device *device)
 {
-	struct dentry *pde;
-	umode_t mode;
-
-	if (!base_dentry)
-		return;
-
-	mode = S_IRUSR | S_IFREG;
-	pde = debugfs_create_file("host_access_list", mode, base_dentry,
-				  device, &dasd_hosts_fops);
-	if (pde && !IS_ERR(pde))
-		device->hosts_dentry = pde;
+	device->hosts_dentry = debugfs_create_file("host_access_list", 0400, base_dentry,
+						   device, &dasd_hosts_fops);
 }
 
 struct dasd_ccw_req *dasd_smalloc_request(int magic, int cplength, int datasize,
-- 
2.51.0

[PATCH 3/4] s390/dasd: Move device name formatting into separate function

[Stefan Haberland]

From: Jan Höppner <hoeppner@linux.ibm.com>

The device name formatting can be generalized and made more readable
compared to the current state. SCSI already provides a generalized way
to format many devices in the same naming scheme as DASD does, which was
introduced with commit 3e1a7ff8a0a7 ("block: allow disk to have extended
device number").

Use this much cleaner code from drivers/scsi/sd.c to handle the legacy
naming scheme in DASD as a replacement for the current implementation.

For easier error handling for the new function, move the gendisk free
portion of dasd_gendisk_free() out into a new function dasd_gd_free().

Signed-off-by: Jan Höppner <hoeppner@linux.ibm.com>
Reviewed-by: Stefan Haberland <sth@linux.ibm.com>
Signed-off-by: Stefan Haberland <sth@linux.ibm.com>
---
 drivers/s390/block/dasd_genhd.c | 80 ++++++++++++++++++++++-----------
 1 file changed, 54 insertions(+), 26 deletions(-)

diff --git a/drivers/s390/block/dasd_genhd.c b/drivers/s390/block/dasd_genhd.c
index 28e92fad0ca1..6ee3d952412e 100644
--- a/drivers/s390/block/dasd_genhd.c
+++ b/drivers/s390/block/dasd_genhd.c
@@ -22,6 +22,7 @@
 
 static unsigned int queue_depth = 32;
 static unsigned int nr_hw_queues = 4;
+static void dasd_gd_free(struct gendisk *gdp);
 
 module_param(queue_depth, uint, 0444);
 MODULE_PARM_DESC(queue_depth, "Default queue depth for new DASD devices");
@@ -29,6 +30,37 @@ MODULE_PARM_DESC(queue_depth, "Default queue depth for new DASD devices");
 module_param(nr_hw_queues, uint, 0444);
 MODULE_PARM_DESC(nr_hw_queues, "Default number of hardware queues for new DASD devices");
 
+/*
+ * Set device name.
+ *   dasda - dasdz : 26 devices
+ *   dasdaa - dasdzz : 676 devices, added up = 702
+ *   dasdaaa - dasdzzz : 17576 devices, added up = 18278
+ *   dasdaaaa - dasdzzzz : 456976 devices, added up = 475252
+ */
+static int dasd_name_format(char *prefix, int index, char *buf, int buflen)
+{
+	const int base = 'z' - 'a' + 1;
+	char *begin = buf + strlen(prefix);
+	char *end = buf + buflen;
+	char *p;
+	int unit;
+
+	p = end - 1;
+	*p = '\0';
+	unit = base;
+	do {
+		if (p == begin)
+			return -EINVAL;
+		*--p = 'a' + (index % unit);
+		index = (index / unit) - 1;
+	} while (index >= 0);
+
+	memmove(begin, p, end - p);
+	memcpy(buf, prefix, strlen(prefix));
+
+	return 0;
+}
+
 /*
  * Allocate and register gendisk structure for device.
  */
@@ -45,11 +77,13 @@ int dasd_gendisk_alloc(struct dasd_block *block)
 	};
 	struct gendisk *gdp;
 	struct dasd_device *base;
-	int len, rc;
+	unsigned int devindex;
+	int rc;
 
 	/* Make sure the minor for this device exists. */
 	base = block->base;
-	if (base->devindex >= DASD_PER_MAJOR)
+	devindex = base->devindex;
+	if (devindex >= DASD_PER_MAJOR)
 		return -EBUSY;
 
 	block->tag_set.ops = &dasd_mq_ops;
@@ -69,31 +103,17 @@ int dasd_gendisk_alloc(struct dasd_block *block)
 
 	/* Initialize gendisk structure. */
 	gdp->major = DASD_MAJOR;
-	gdp->first_minor = base->devindex << DASD_PARTN_BITS;
+	gdp->first_minor = devindex << DASD_PARTN_BITS;
 	gdp->minors = 1 << DASD_PARTN_BITS;
 	gdp->fops = &dasd_device_operations;
 
-	/*
-	 * Set device name.
-	 *   dasda - dasdz : 26 devices
-	 *   dasdaa - dasdzz : 676 devices, added up = 702
-	 *   dasdaaa - dasdzzz : 17576 devices, added up = 18278
-	 *   dasdaaaa - dasdzzzz : 456976 devices, added up = 475252
-	 */
-	len = sprintf(gdp->disk_name, "dasd");
-	if (base->devindex > 25) {
-		if (base->devindex > 701) {
-			if (base->devindex > 18277)
-			        len += sprintf(gdp->disk_name + len, "%c",
-					       'a'+(((base->devindex-18278)
-						     /17576)%26));
-			len += sprintf(gdp->disk_name + len, "%c",
-				       'a'+(((base->devindex-702)/676)%26));
-		}
-		len += sprintf(gdp->disk_name + len, "%c",
-			       'a'+(((base->devindex-26)/26)%26));
+	rc = dasd_name_format("dasd", devindex, gdp->disk_name, sizeof(gdp->disk_name));
+	if (rc) {
+		DBF_DEV_EVENT(DBF_ERR, block->base,
+			      "setting disk name failed, rc %d", rc);
+		dasd_gd_free(gdp);
+		return rc;
 	}
-	len += sprintf(gdp->disk_name + len, "%c", 'a'+(base->devindex%26));
 
 	if (base->features & DASD_FEATURE_READONLY ||
 	    test_bit(DASD_FLAG_DEVICE_RO, &base->flags))
@@ -111,15 +131,23 @@ int dasd_gendisk_alloc(struct dasd_block *block)
 	return 0;
 }
 
+/*
+ * Free gendisk structure
+ */
+static void dasd_gd_free(struct gendisk *gd)
+{
+	del_gendisk(gd);
+	gd->private_data = NULL;
+	put_disk(gd);
+}
+
 /*
  * Unregister and free gendisk structure for device.
  */
 void dasd_gendisk_free(struct dasd_block *block)
 {
 	if (block->gdp) {
-		del_gendisk(block->gdp);
-		block->gdp->private_data = NULL;
-		put_disk(block->gdp);
+		dasd_gd_free(block->gdp);
 		block->gdp = NULL;
 		blk_mq_free_tag_set(&block->tag_set);
 	}
-- 
2.51.0

[PATCH 4/4] s390/dasd: Use scnprintf() instead of sprintf()

[Stefan Haberland]

From: Jan Höppner <hoeppner@linux.ibm.com>

Use scnprintf() instead of sprintf() for those cases where the
destination is an array and the size of the array is known at compile
time.

This prevents theoretical buffer overflows, but also avoids that people
again and again spend time to figure out if the code is actually safe.

Signed-off-by: Jan Höppner <hoeppner@linux.ibm.com>
Reviewed-by: Stefan Haberland <sth@linux.ibm.com>
Signed-off-by: Stefan Haberland <sth@linux.ibm.com>
---
 drivers/s390/block/dasd_devmap.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/s390/block/dasd_devmap.c b/drivers/s390/block/dasd_devmap.c
index ddbdf1f85d44..73972900fc55 100644
--- a/drivers/s390/block/dasd_devmap.c
+++ b/drivers/s390/block/dasd_devmap.c
@@ -355,7 +355,8 @@ static int __init dasd_parse_range(const char *range)
 	/* each device in dasd= parameter should be set initially online */
 	features |= DASD_FEATURE_INITIAL_ONLINE;
 	while (from <= to) {
-		sprintf(bus_id, "%01x.%01x.%04x", from_id0, from_id1, from++);
+		scnprintf(bus_id, sizeof(bus_id),
+			  "%01x.%01x.%04x", from_id0, from_id1, from++);
 		devmap = dasd_add_busid(bus_id, features);
 		if (IS_ERR(devmap)) {
 			rc = PTR_ERR(devmap);
-- 
2.51.0

Re: [PATCH 0/4] s390/dasd: Minor cleanups and copy-pair swap fix

[Jens Axboe]

On Wed, 26 Nov 2025 17:06:30 +0100, Stefan Haberland wrote:
> this small series contains cleanups in the DASD driver
> (string formatting, naming helper refactoring, debugfs simplification)
> as well as a fix for the gendisk parent after a copy-pair swap.
> 
> Please apply.
> 
> Thanks,
> Stefan
> 
> [...]

Applied, thanks!

[1/4] s390/dasd: Fix gendisk parent after copy pair swap
      commit: c943bfc6afb8d0e781b9b7406f36caa8bbf95cb9
[2/4] s390/dasd: Remove unnecessary debugfs_create() return checks
      commit: 764def9e8eaf1b1ccdcd89b8c16db4194ade775f
[3/4] s390/dasd: Move device name formatting into separate function
      commit: 43198756ee8cade0acc17a89f959764cd17776bb
[4/4] s390/dasd: Use scnprintf() instead of sprintf()
      commit: a857d99201cc4eb3cb78b9dcb6f1d027ef3ae699

Best regards,
-- 
Jens Axboe