Subject: RE: [PATCH 2/4] pmem: Add support for getgeo()



> -----Original Message-----
> From: [email protected] [mailto:linux-kernel-
> [email protected]] On Behalf Of Ross Zwisler
> Sent: Wednesday, 27 August, 2014 4:12 PM
> To: Jens Axboe; Matthew Wilcox; Boaz Harrosh; Nick Piggin; linux-
> [email protected]; [email protected]; linux-
> [email protected]
> Cc: Ross Zwisler
> Subject: [PATCH 2/4] pmem: Add support for getgeo()
>
> Some programs require HDIO_GETGEO work, which requires we implement
> getgeo. Based off of the work done to the NVMe driver in this
> commit:
>
> commit 4cc09e2dc4cb ("NVMe: Add getgeo to block ops")
>
> Signed-off-by: Ross Zwisler <[email protected]>
> ---
> drivers/block/pmem.c | 10 ++++++++++
> 1 file changed, 10 insertions(+)
>
> diff --git a/drivers/block/pmem.c b/drivers/block/pmem.c
> index d366b9b..60bbe0d 100644
> --- a/drivers/block/pmem.c
> +++ b/drivers/block/pmem.c
> @@ -50,6 +50,15 @@ struct pmem_device {
> size_t size;
> };
>
> +static int pmem_getgeo(struct block_device *bd, struct hd_geometry
> *geo)
> +{
> + /* some standard values */
> + geo->heads = 1 << 6;
> + geo->sectors = 1 << 5;
> + geo->cylinders = get_capacity(bd->bd_disk) >> 11;

Just stuffing the result of get_capacity into the 16-bit
cylinders field will overflow/wrap on large capacities.
0xFFFF << 11 = 0x7FF_F800 = 64 GiB (68.7 GB)

How many programs still need these meaningless fields?
Could the bogus information be created elsewhere so
each block driver doesn't need to do this?


> + return 0;
> +}
> +
> /*
> * direct translation from (pmem,sector) => void*
> * We do not require that sector be page aligned.
> @@ -176,6 +185,7 @@ out:
>
> static const struct block_device_operations pmem_fops = {
> .owner = THIS_MODULE,
> + .getgeo = pmem_getgeo,
> };
>
> /* Kernel module stuff */
> --


---
Rob Elliott HP Server Storage



2014-11-03 16:36:09

by Matthew Wilcox

[permalink] [raw]
Subject: RE: [PATCH 2/4] pmem: Add support for getgeo()

I agree that there should be a generic fake getgeo routine; but fixing that is a topic for a different patchset and it doesn't need to get folded into this driver submission process.

-----Original Message-----
From: Elliott, Robert (Server Storage) [mailto:[email protected]]
Sent: Saturday, November 01, 2014 8:28 PM
To: Ross Zwisler; Jens Axboe; Wilcox, Matthew R; Boaz Harrosh; Nick Piggin; Kani, Toshimitsu; Knippers, Linda; [email protected]; [email protected]; [email protected]
Subject: RE: [PATCH 2/4] pmem: Add support for getgeo()



> -----Original Message-----
> From: [email protected] [mailto:linux-kernel-
> [email protected]] On Behalf Of Ross Zwisler
> Sent: Wednesday, 27 August, 2014 4:12 PM
> To: Jens Axboe; Matthew Wilcox; Boaz Harrosh; Nick Piggin; linux-
> [email protected]; [email protected]; linux-
> [email protected]
> Cc: Ross Zwisler
> Subject: [PATCH 2/4] pmem: Add support for getgeo()
>
> Some programs require HDIO_GETGEO work, which requires we implement
> getgeo. Based off of the work done to the NVMe driver in this
> commit:
>
> commit 4cc09e2dc4cb ("NVMe: Add getgeo to block ops")
>
> Signed-off-by: Ross Zwisler <[email protected]>
> ---
> drivers/block/pmem.c | 10 ++++++++++
> 1 file changed, 10 insertions(+)
>
> diff --git a/drivers/block/pmem.c b/drivers/block/pmem.c
> index d366b9b..60bbe0d 100644
> --- a/drivers/block/pmem.c
> +++ b/drivers/block/pmem.c
> @@ -50,6 +50,15 @@ struct pmem_device {
> size_t size;
> };
>
> +static int pmem_getgeo(struct block_device *bd, struct hd_geometry
> *geo)
> +{
> + /* some standard values */
> + geo->heads = 1 << 6;
> + geo->sectors = 1 << 5;
> + geo->cylinders = get_capacity(bd->bd_disk) >> 11;

Just stuffing the result of get_capacity into the 16-bit
cylinders field will overflow/wrap on large capacities.
0xFFFF << 11 = 0x7FF_F800 = 64 GiB (68.7 GB)

How many programs still need these meaningless fields?
Could the bogus information be created elsewhere so
each block driver doesn't need to do this?


> + return 0;
> +}
> +
> /*
> * direct translation from (pmem,sector) => void*
> * We do not require that sector be page aligned.
> @@ -176,6 +185,7 @@ out:
>
> static const struct block_device_operations pmem_fops = {
> .owner = THIS_MODULE,
> + .getgeo = pmem_getgeo,
> };
>
> /* Kernel module stuff */
> --


---
Rob Elliott HP Server Storage