* [PATCH v3 1/3] dasd: refactor dasd_ioctl_information
2020-05-08 13:14 [PATCH v3 0/3] s390/dasd: remove ioctl_by_bdev from DASD driver Stefan Haberland
@ 2020-05-08 13:14 ` Stefan Haberland
2020-05-08 13:14 ` [PATCH v3 2/3] block: add a s390-only biodasdinfo method Stefan Haberland
2020-05-08 13:14 ` [PATCH v3 3/3] s390/dasd: remove ioctl_by_bdev calls Stefan Haberland
2 siblings, 0 replies; 9+ messages in thread
From: Stefan Haberland @ 2020-05-08 13:14 UTC (permalink / raw)
To: hch
Cc: axboe, hoeppner, linux-s390, heiko.carstens, gor, linux-kernel,
borntraeger
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
^ permalink raw reply related [flat|nested] 9+ messages in thread
* [PATCH v3 2/3] block: add a s390-only biodasdinfo method
2020-05-08 13:14 [PATCH v3 0/3] s390/dasd: remove ioctl_by_bdev from DASD driver Stefan Haberland
2020-05-08 13:14 ` [PATCH v3 1/3] dasd: refactor dasd_ioctl_information Stefan Haberland
@ 2020-05-08 13:14 ` Stefan Haberland
2020-05-08 15:53 ` Christoph Hellwig
2020-05-08 13:14 ` [PATCH v3 3/3] s390/dasd: remove ioctl_by_bdev calls Stefan Haberland
2 siblings, 1 reply; 9+ messages in thread
From: Stefan Haberland @ 2020-05-08 13:14 UTC (permalink / raw)
To: hch
Cc: axboe, hoeppner, linux-s390, heiko.carstens, gor, linux-kernel,
borntraeger
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
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH v3 2/3] block: add a s390-only biodasdinfo method
2020-05-08 13:14 ` [PATCH v3 2/3] block: add a s390-only biodasdinfo method Stefan Haberland
@ 2020-05-08 15:53 ` Christoph Hellwig
0 siblings, 0 replies; 9+ messages in thread
From: Christoph Hellwig @ 2020-05-08 15:53 UTC (permalink / raw)
To: Stefan Haberland
Cc: hch, axboe, hoeppner, linux-s390, heiko.carstens, gor,
linux-kernel, borntraeger
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.
^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH v3 3/3] s390/dasd: remove ioctl_by_bdev calls
2020-05-08 13:14 [PATCH v3 0/3] s390/dasd: remove ioctl_by_bdev from DASD driver Stefan Haberland
2020-05-08 13:14 ` [PATCH v3 1/3] dasd: refactor dasd_ioctl_information Stefan Haberland
2020-05-08 13:14 ` [PATCH v3 2/3] block: add a s390-only biodasdinfo method Stefan Haberland
@ 2020-05-08 13:14 ` Stefan Haberland
2020-05-08 15:53 ` Christoph Hellwig
2 siblings, 1 reply; 9+ messages in thread
From: Stefan Haberland @ 2020-05-08 13:14 UTC (permalink / raw)
To: hch
Cc: axboe, hoeppner, linux-s390, heiko.carstens, gor, linux-kernel,
borntraeger
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
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH v3 3/3] s390/dasd: remove ioctl_by_bdev calls
2020-05-08 13:14 ` [PATCH v3 3/3] s390/dasd: remove ioctl_by_bdev calls Stefan Haberland
@ 2020-05-08 15:53 ` Christoph Hellwig
2020-05-11 16:30 ` Stefan Haberland
0 siblings, 1 reply; 9+ messages in thread
From: Christoph Hellwig @ 2020-05-08 15:53 UTC (permalink / raw)
To: Stefan Haberland
Cc: hch, axboe, hoeppner, linux-s390, heiko.carstens, gor,
linux-kernel, borntraeger
I think this should use symbol_get instead.
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v3 3/3] s390/dasd: remove ioctl_by_bdev calls
2020-05-08 15:53 ` Christoph Hellwig
@ 2020-05-11 16:30 ` Stefan Haberland
2020-05-16 15:43 ` Christoph Hellwig
0 siblings, 1 reply; 9+ messages in thread
From: Stefan Haberland @ 2020-05-11 16:30 UTC (permalink / raw)
To: Christoph Hellwig
Cc: axboe, hoeppner, linux-s390, heiko.carstens, gor, linux-kernel,
borntraeger
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.
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v3 3/3] s390/dasd: remove ioctl_by_bdev calls
2020-05-11 16:30 ` Stefan Haberland
@ 2020-05-16 15:43 ` Christoph Hellwig
2020-05-18 8:13 ` Stefan Haberland
0 siblings, 1 reply; 9+ messages in thread
From: Christoph Hellwig @ 2020-05-16 15:43 UTC (permalink / raw)
To: Stefan Haberland
Cc: Christoph Hellwig, axboe, hoeppner, linux-s390, heiko.carstens,
gor, linux-kernel, borntraeger
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?
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v3 3/3] s390/dasd: remove ioctl_by_bdev calls
2020-05-16 15:43 ` Christoph Hellwig
@ 2020-05-18 8:13 ` Stefan Haberland
0 siblings, 0 replies; 9+ messages in thread
From: Stefan Haberland @ 2020-05-18 8:13 UTC (permalink / raw)
To: Christoph Hellwig
Cc: axboe, hoeppner, linux-s390, heiko.carstens, gor, linux-kernel,
borntraeger
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.
^ permalink raw reply [flat|nested] 9+ messages in thread