[PATCH v3 0/3] s390/dasd: remove ioctl_by_bdev from DASD driver

[PATCH v3 0/3] s390/dasd: remove ioctl_by_bdev from DASD driver

[Stefan Haberland]

Hi Christoph,

as discussed yesterday I have picked your first patch, adopted the second
one and created a new third patch to remove the ioctl_by_bdev calls.
I export the symbol for the dasd_biodasdinfo function and obtain the
pointer with a kallsyms_lookup_name.
The checking if the gendisk is owned by a DASD is done by comparing the
fops pointer as you suggested. Looks like the most suitable method here.

Please note that this patch is not ready for inclusion and only basic
function tested. It is just for discussion.

Christoph Hellwig (2):
  dasd: refactor dasd_ioctl_information
  block: add a s390-only biodasdinfo method

Stefan Haberland (1):
  s390/dasd: remove ioctl_by_bdev calls

 block/partitions/ibm.c          | 15 +++++++--
 drivers/s390/block/dasd_int.h   |  1 +
 drivers/s390/block/dasd_ioctl.c | 59 ++++++++++++++++++++++++---------
 include/linux/blkdev.h          |  1 +
 4 files changed, 57 insertions(+), 19 deletions(-)

-- 
2.17.1

[PATCH v3 1/3] dasd: refactor dasd_ioctl_information

[Stefan Haberland]

From: Christoph Hellwig <hch@lst.de>

Prepare for in-kernel callers of this functionality.

Signed-off-by: Christoph Hellwig <hch@lst.de>
Signed-off-by: Stefan Haberland <sth@linux.ibm.com>
---
 drivers/s390/block/dasd_ioctl.c | 38 +++++++++++++++++++--------------
 1 file changed, 22 insertions(+), 16 deletions(-)

diff --git a/drivers/s390/block/dasd_ioctl.c b/drivers/s390/block/dasd_ioctl.c
index 9a5f3add325f..dabcb4ce92da 100644
--- a/drivers/s390/block/dasd_ioctl.c
+++ b/drivers/s390/block/dasd_ioctl.c
@@ -457,10 +457,9 @@ static int dasd_ioctl_read_profile(struct dasd_block *block, void __user *argp)
 /*
  * Return dasd information. Used for BIODASDINFO and BIODASDINFO2.
  */
-static int dasd_ioctl_information(struct dasd_block *block,
-				  unsigned int cmd, void __user *argp)
+static int __dasd_ioctl_information(struct dasd_block *block,
+		struct dasd_information2_t *dasd_info)
 {
-	struct dasd_information2_t *dasd_info;
 	struct subchannel_id sch_id;
 	struct ccw_dev_id dev_id;
 	struct dasd_device *base;
@@ -473,10 +472,6 @@ static int dasd_ioctl_information(struct dasd_block *block,
 	if (!base->discipline || !base->discipline->fill_info)
 		return -EINVAL;
 
-	dasd_info = kzalloc(sizeof(struct dasd_information2_t), GFP_KERNEL);
-	if (dasd_info == NULL)
-		return -ENOMEM;
-
 	rc = base->discipline->fill_info(base, dasd_info);
 	if (rc) {
 		kfree(dasd_info);
@@ -520,15 +515,24 @@ static int dasd_ioctl_information(struct dasd_block *block,
 	list_for_each(l, &base->ccw_queue)
 		dasd_info->chanq_len++;
 	spin_unlock_irqrestore(&block->queue_lock, flags);
+	return 0;
+}
 
-	rc = 0;
-	if (copy_to_user(argp, dasd_info,
-			 ((cmd == (unsigned int) BIODASDINFO2) ?
-			  sizeof(struct dasd_information2_t) :
-			  sizeof(struct dasd_information_t))))
-		rc = -EFAULT;
+static int dasd_ioctl_information(struct dasd_block *block, void __user *argp,
+		size_t copy_size)
+{
+	struct dasd_information2_t *dasd_info;
+	int error = 0;
+
+	dasd_info = kzalloc(sizeof(*dasd_info), GFP_KERNEL);
+	if (!dasd_info)
+		return -ENOMEM;
+
+	error = __dasd_ioctl_information(block, dasd_info);
+	if (!error && copy_to_user(argp, dasd_info, copy_size))
+		error = -EFAULT;
 	kfree(dasd_info);
-	return rc;
+	return error;
 }
 
 /*
@@ -622,10 +626,12 @@ int dasd_ioctl(struct block_device *bdev, fmode_t mode,
 		rc = dasd_ioctl_check_format(bdev, argp);
 		break;
 	case BIODASDINFO:
-		rc = dasd_ioctl_information(block, cmd, argp);
+		rc = dasd_ioctl_information(block, argp,
+				sizeof(struct dasd_information_t));
 		break;
 	case BIODASDINFO2:
-		rc = dasd_ioctl_information(block, cmd, argp);
+		rc = dasd_ioctl_information(block, argp,
+				sizeof(struct dasd_information2_t));
 		break;
 	case BIODASDPRRD:
 		rc = dasd_ioctl_read_profile(block, argp);
-- 
2.17.1

[PATCH v3 2/3] block: add a s390-only biodasdinfo method

[Stefan Haberland]

From: Christoph Hellwig <hch@lst.de>

The IBM partition parser needs to query the DASD driver for details that
are very s390 specific.  Instead of using ioctl_by_bdev with a fake user
space pointer just add a s390-specific method to get the information
directly.

Signed-off-by: Christoph Hellwig <hch@lst.de>
[sth@linux.ibm.com: remove fop, add gendisk check, export funcion]
Signed-off-by: Stefan Haberland <sth@linux.ibm.com>
---
 drivers/s390/block/dasd_int.h   |  1 +
 drivers/s390/block/dasd_ioctl.c | 21 +++++++++++++++++++++
 include/linux/blkdev.h          |  1 +
 3 files changed, 23 insertions(+)

diff --git a/drivers/s390/block/dasd_int.h b/drivers/s390/block/dasd_int.h
index fa552f9f1666..6eac7b11c75b 100644
--- a/drivers/s390/block/dasd_int.h
+++ b/drivers/s390/block/dasd_int.h
@@ -845,6 +845,7 @@ void dasd_destroy_partitions(struct dasd_block *);
 
 /* externals in dasd_ioctl.c */
 int  dasd_ioctl(struct block_device *, fmode_t, unsigned int, unsigned long);
+int dasd_biodasdinfo(struct gendisk *disk, struct dasd_information2_t *info);
 
 /* externals in dasd_proc.c */
 int dasd_proc_init(void);
diff --git a/drivers/s390/block/dasd_ioctl.c b/drivers/s390/block/dasd_ioctl.c
index dabcb4ce92da..d29045a37b92 100644
--- a/drivers/s390/block/dasd_ioctl.c
+++ b/drivers/s390/block/dasd_ioctl.c
@@ -666,3 +666,24 @@ int dasd_ioctl(struct block_device *bdev, fmode_t mode,
 	dasd_put_device(base);
 	return rc;
 }
+
+int dasd_biodasdinfo(struct gendisk *disk, struct dasd_information2_t *info)
+{
+	struct dasd_device *base;
+	int error;
+
+	/*
+	 * we might get called externaly, so check if the gendisk belongs
+	 * to a DASD by checking the fops pointer
+	 */
+	if (disk->fops != &dasd_device_operations)
+		return -EINVAL;
+
+	base = dasd_device_from_gendisk(disk);
+	if (!base)
+		return -ENODEV;
+	error = __dasd_ioctl_information(base->block, info);
+	dasd_put_device(base);
+	return error;
+}
+EXPORT_SYMBOL(dasd_biodasdinfo);
diff --git a/include/linux/blkdev.h b/include/linux/blkdev.h
index 32868fbedc9e..915465aa8e43 100644
--- a/include/linux/blkdev.h
+++ b/include/linux/blkdev.h
@@ -43,6 +43,7 @@ struct pr_ops;
 struct rq_qos;
 struct blk_queue_stats;
 struct blk_stat_callback;
+struct dasd_information2_t;
 
 #define BLKDEV_MIN_RQ	4
 #define BLKDEV_MAX_RQ	128	/* Default maximum */
-- 
2.17.1

Re: [PATCH v3 2/3] block: add a s390-only biodasdinfo method

[Christoph Hellwig]

On Fri, May 08, 2020 at 03:14:54PM +0200, Stefan Haberland wrote:
> From: Christoph Hellwig <hch@lst.de>
> 
> The IBM partition parser needs to query the DASD driver for details that
> are very s390 specific.  Instead of using ioctl_by_bdev with a fake user
> space pointer just add a s390-specific method to get the information
> directly.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> [sth@linux.ibm.com: remove fop, add gendisk check, export funcion]
> Signed-off-by: Stefan Haberland <sth@linux.ibm.com>

The subject and changelog need updates for your changes.  I think you
should also claim authorship, even if a few bits are originally from me.
Probaby it would make sense to even just merge this into the next patch.

> index fa552f9f1666..6eac7b11c75b 100644
> --- a/drivers/s390/block/dasd_int.h
> +++ b/drivers/s390/block/dasd_int.h
> @@ -845,6 +845,7 @@ void dasd_destroy_partitions(struct dasd_block *);
>  
>  /* externals in dasd_ioctl.c */
>  int  dasd_ioctl(struct block_device *, fmode_t, unsigned int, unsigned long);
> +int dasd_biodasdinfo(struct gendisk *disk, struct dasd_information2_t *info);

I think this needs to go to a public include/linux/ header for the
partitioning code to share the prototype.

> +int dasd_biodasdinfo(struct gendisk *disk, struct dasd_information2_t *info)
> +{
> +	struct dasd_device *base;
> +	int error;
> +
> +	/*
> +	 * we might get called externaly, so check if the gendisk belongs
> +	 * to a DASD by checking the fops pointer
> +	 */
> +	if (disk->fops != &dasd_device_operations)
> +		return -EINVAL;

I think a function comment (e.g. kernel doc) explaining the use case and
this detail might be useful.

[PATCH v3 3/3] s390/dasd: remove ioctl_by_bdev calls

[Stefan Haberland]

Call getgeo method directly and obtain pointer to dasd_biodasdinfo
function and use this instead of ioctl.

Suggested-by: Christoph Hellwig <hch@lst.de>
Signed-off-by: Stefan Haberland <sth@linux.ibm.com>
---
 block/partitions/ibm.c | 15 ++++++++++++---
 1 file changed, 12 insertions(+), 3 deletions(-)

diff --git a/block/partitions/ibm.c b/block/partitions/ibm.c
index 073faa6a69b8..69c27b8bee97 100644
--- a/block/partitions/ibm.c
+++ b/block/partitions/ibm.c
@@ -13,10 +13,10 @@
 #include <asm/ebcdic.h>
 #include <linux/uaccess.h>
 #include <asm/vtoc.h>
+#include <linux/kallsyms.h>
 
 #include "check.h"
 
-
 union label_t {
 	struct vtoc_volume_label_cdl vol;
 	struct vtoc_volume_label_ldl lnx;
@@ -288,7 +288,9 @@ static int find_cms1_partitions(struct parsed_partitions *state,
  */
 int ibm_partition(struct parsed_partitions *state)
 {
+	int (*dasd_biodasdinfo)(struct gendisk *, dasd_information2_t *);
 	struct block_device *bdev = state->bdev;
+	struct gendisk *disk = bdev->bd_disk;
 	int blocksize, res;
 	loff_t i_size, offset, size;
 	dasd_information2_t *info;
@@ -297,6 +299,7 @@ int ibm_partition(struct parsed_partitions *state)
 	char name[7] = {0,};
 	sector_t labelsect;
 	union label_t *label;
+	int rc = 0;
 
 	res = 0;
 	blocksize = bdev_logical_block_size(bdev);
@@ -314,9 +317,15 @@ int ibm_partition(struct parsed_partitions *state)
 	label = kmalloc(sizeof(union label_t), GFP_KERNEL);
 	if (label == NULL)
 		goto out_nolab;
-	if (ioctl_by_bdev(bdev, HDIO_GETGEO, (unsigned long)geo) != 0)
+	geo->start = get_start_sect(bdev);
+	if (!disk->fops->getgeo || disk->fops->getgeo(bdev, geo))
+		goto out_freeall;
+	dasd_biodasdinfo = (void *)kallsyms_lookup_name("dasd_biodasdinfo");
+	if (dasd_biodasdinfo)
+		rc = dasd_biodasdinfo(disk, info);
+	if (rc == -EINVAL)
 		goto out_freeall;
-	if (ioctl_by_bdev(bdev, BIODASDINFO2, (unsigned long)info) != 0) {
+	if (rc) {
 		kfree(info);
 		info = NULL;
 	}
-- 
2.17.1

Re: [PATCH v3 3/3] s390/dasd: remove ioctl_by_bdev calls

[Christoph Hellwig]

I think this should use symbol_get instead.

Re: [PATCH v3 3/3] s390/dasd: remove ioctl_by_bdev calls

[Stefan Haberland]

Am 08.05.20 um 17:53 schrieb Christoph Hellwig:
> I think this should use symbol_get instead.

Thanks for the Feedback, also for the previous patch.
I will incorporate it, run some test cycles and submit the patches
again when I am ready.

Re: [PATCH v3 3/3] s390/dasd: remove ioctl_by_bdev calls

[Christoph Hellwig]

On Mon, May 11, 2020 at 06:30:44PM +0200, Stefan Haberland wrote:
> Am 08.05.20 um 17:53 schrieb Christoph Hellwig:
> > I think this should use symbol_get instead.
> 
> Thanks for the Feedback, also for the previous patch.
> I will incorporate it, run some test cycles and submit the patches
> again when I am ready.

Did you manage to get back to this?

Re: [PATCH v3 3/3] s390/dasd: remove ioctl_by_bdev calls

[Stefan Haberland]

Am 16.05.20 um 17:43 schrieb Christoph Hellwig:
> On Mon, May 11, 2020 at 06:30:44PM +0200, Stefan Haberland wrote:
>> Am 08.05.20 um 17:53 schrieb Christoph Hellwig:
>>> I think this should use symbol_get instead.
>> Thanks for the Feedback, also for the previous patch.
>> I will incorporate it, run some test cycles and submit the patches
>> again when I am ready.
> Did you manage to get back to this?

Hi, yes. We had some internal discussions about the patch and I am running
some tests.
I need to have a look at virtblk devices with DASD layout and afterwards
I will send the patches again.