Re: [PATCH 05/14] cxl/mem: Register CXL memX devices

2021-02-02 Thread Christoph Hellwig
> diff --git a/drivers/base/core.c b/drivers/base/core.c
> index 25e08e5f40bd..33432a4cbe23 100644
> --- a/drivers/base/core.c
> +++ b/drivers/base/core.c
> @@ -3179,6 +3179,20 @@ struct device *get_device(struct device *dev)
>  }
>  EXPORT_SYMBOL_GPL(get_device);
>  
> +/**
> + * get_live_device() - increment reference count for device iff !dead
> + * @dev: device.
> + *
> + * Forward the call to get_device() if the device is still alive. If
> + * this is called with the device_lock() held then the device is
> + * guaranteed to not die until the device_lock() is dropped.
> + */
> +struct device *get_live_device(struct device *dev)
> +{
> + return dev && !dev->p->dead ? get_device(dev) : NULL;
> +}
> +EXPORT_SYMBOL_GPL(get_live_device);

Err, if you want to add new core functionality that needs to be in a
separate well documented prep patch, and also CCed to the relevant
maintainers.

>   mutex_unlock(>mbox.mutex);
>  }
>  
> +static int cxl_memdev_open(struct inode *inode, struct file *file)
> +{
> + struct cxl_memdev *cxlmd =
> + container_of(inode->i_cdev, typeof(*cxlmd), cdev);
> +
> + file->private_data = cxlmd;

There is no good reason to ever mirror stuff from the inode into
file->private_data, as you can just trivially get at the original
location using file_inode(file).


Re: [PATCH 05/14] cxl/mem: Register CXL memX devices

2021-02-01 Thread Dan Williams
On Mon, Feb 1, 2021 at 1:53 PM David Rientjes  wrote:
>
> On Mon, 1 Feb 2021, Ben Widawsky wrote:
>
> > > > diff --git a/Documentation/ABI/testing/sysfs-bus-cxl 
> > > > b/Documentation/ABI/testing/sysfs-bus-cxl
> > > > new file mode 100644
> > > > index ..fe7b87eba988
> > > > --- /dev/null
> > > > +++ b/Documentation/ABI/testing/sysfs-bus-cxl
> > > > @@ -0,0 +1,26 @@
> > > > +What:/sys/bus/cxl/devices/memX/firmware_version
> > > > +Date:December, 2020
> > > > +KernelVersion:   v5.12
> > > > +Contact: linux-...@vger.kernel.org
> > > > +Description:
> > > > + (RO) "FW Revision" string as reported by the Identify
> > > > + Memory Device Output Payload in the CXL-2.0
> > > > + specification.
> > > > +
> > > > +What:/sys/bus/cxl/devices/memX/ram/size
> > > > +Date:December, 2020
> > > > +KernelVersion:   v5.12
> > > > +Contact: linux-...@vger.kernel.org
> > > > +Description:
> > > > + (RO) "Volatile Only Capacity" as reported by the
> > > > + Identify Memory Device Output Payload in the CXL-2.0
> > > > + specification.
> > > > +
> > > > +What:/sys/bus/cxl/devices/memX/pmem/size
> > > > +Date:December, 2020
> > > > +KernelVersion:   v5.12
> > > > +Contact: linux-...@vger.kernel.org
> > > > +Description:
> > > > + (RO) "Persistent Only Capacity" as reported by the
> > > > + Identify Memory Device Output Payload in the CXL-2.0
> > > > + specification.
> > >
> > > Aren't volatile and persistent capacities expressed in multiples of 256MB?
> >
> > As of the spec today, volatile and persistent capacities are required to be
> > in multiples of 256MB, however, future specs may not have such a 
> > requirement and
> > I think keeping sysfs ABI easily forward portable makes sense.
> >
>
> Makes sense, can we add that these are expressed in bytes or is that
> already implied?

Makes sense to declare units here.


Re: [PATCH 05/14] cxl/mem: Register CXL memX devices

2021-02-01 Thread David Rientjes
On Mon, 1 Feb 2021, Ben Widawsky wrote:

> > > diff --git a/Documentation/ABI/testing/sysfs-bus-cxl 
> > > b/Documentation/ABI/testing/sysfs-bus-cxl
> > > new file mode 100644
> > > index ..fe7b87eba988
> > > --- /dev/null
> > > +++ b/Documentation/ABI/testing/sysfs-bus-cxl
> > > @@ -0,0 +1,26 @@
> > > +What:/sys/bus/cxl/devices/memX/firmware_version
> > > +Date:December, 2020
> > > +KernelVersion:   v5.12
> > > +Contact: linux-...@vger.kernel.org
> > > +Description:
> > > + (RO) "FW Revision" string as reported by the Identify
> > > + Memory Device Output Payload in the CXL-2.0
> > > + specification.
> > > +
> > > +What:/sys/bus/cxl/devices/memX/ram/size
> > > +Date:December, 2020
> > > +KernelVersion:   v5.12
> > > +Contact: linux-...@vger.kernel.org
> > > +Description:
> > > + (RO) "Volatile Only Capacity" as reported by the
> > > + Identify Memory Device Output Payload in the CXL-2.0
> > > + specification.
> > > +
> > > +What:/sys/bus/cxl/devices/memX/pmem/size
> > > +Date:December, 2020
> > > +KernelVersion:   v5.12
> > > +Contact: linux-...@vger.kernel.org
> > > +Description:
> > > + (RO) "Persistent Only Capacity" as reported by the
> > > + Identify Memory Device Output Payload in the CXL-2.0
> > > + specification.
> > 
> > Aren't volatile and persistent capacities expressed in multiples of 256MB?
> 
> As of the spec today, volatile and persistent capacities are required to be
> in multiples of 256MB, however, future specs may not have such a requirement 
> and
> I think keeping sysfs ABI easily forward portable makes sense.
> 

Makes sense, can we add that these are expressed in bytes or is that 
already implied?


Re: [PATCH 05/14] cxl/mem: Register CXL memX devices

2021-02-01 Thread Ben Widawsky
On 21-01-30 15:52:01, David Rientjes wrote:
> On Fri, 29 Jan 2021, Ben Widawsky wrote:
> 
> > diff --git a/Documentation/ABI/testing/sysfs-bus-cxl 
> > b/Documentation/ABI/testing/sysfs-bus-cxl
> > new file mode 100644
> > index ..fe7b87eba988
> > --- /dev/null
> > +++ b/Documentation/ABI/testing/sysfs-bus-cxl
> > @@ -0,0 +1,26 @@
> > +What:  /sys/bus/cxl/devices/memX/firmware_version
> > +Date:  December, 2020
> > +KernelVersion: v5.12
> > +Contact:   linux-...@vger.kernel.org
> > +Description:
> > +   (RO) "FW Revision" string as reported by the Identify
> > +   Memory Device Output Payload in the CXL-2.0
> > +   specification.
> > +
> > +What:  /sys/bus/cxl/devices/memX/ram/size
> > +Date:  December, 2020
> > +KernelVersion: v5.12
> > +Contact:   linux-...@vger.kernel.org
> > +Description:
> > +   (RO) "Volatile Only Capacity" as reported by the
> > +   Identify Memory Device Output Payload in the CXL-2.0
> > +   specification.
> > +
> > +What:  /sys/bus/cxl/devices/memX/pmem/size
> > +Date:  December, 2020
> > +KernelVersion: v5.12
> > +Contact:   linux-...@vger.kernel.org
> > +Description:
> > +   (RO) "Persistent Only Capacity" as reported by the
> > +   Identify Memory Device Output Payload in the CXL-2.0
> > +   specification.
> 
> Aren't volatile and persistent capacities expressed in multiples of 256MB?

As of the spec today, volatile and persistent capacities are required to be
in multiples of 256MB, however, future specs may not have such a requirement and
I think keeping sysfs ABI easily forward portable makes sense.


Re: [PATCH 05/14] cxl/mem: Register CXL memX devices

2021-01-30 Thread David Rientjes
On Fri, 29 Jan 2021, Ben Widawsky wrote:

> diff --git a/Documentation/ABI/testing/sysfs-bus-cxl 
> b/Documentation/ABI/testing/sysfs-bus-cxl
> new file mode 100644
> index ..fe7b87eba988
> --- /dev/null
> +++ b/Documentation/ABI/testing/sysfs-bus-cxl
> @@ -0,0 +1,26 @@
> +What:/sys/bus/cxl/devices/memX/firmware_version
> +Date:December, 2020
> +KernelVersion:   v5.12
> +Contact: linux-...@vger.kernel.org
> +Description:
> + (RO) "FW Revision" string as reported by the Identify
> + Memory Device Output Payload in the CXL-2.0
> + specification.
> +
> +What:/sys/bus/cxl/devices/memX/ram/size
> +Date:December, 2020
> +KernelVersion:   v5.12
> +Contact: linux-...@vger.kernel.org
> +Description:
> + (RO) "Volatile Only Capacity" as reported by the
> + Identify Memory Device Output Payload in the CXL-2.0
> + specification.
> +
> +What:/sys/bus/cxl/devices/memX/pmem/size
> +Date:December, 2020
> +KernelVersion:   v5.12
> +Contact: linux-...@vger.kernel.org
> +Description:
> + (RO) "Persistent Only Capacity" as reported by the
> + Identify Memory Device Output Payload in the CXL-2.0
> + specification.

Aren't volatile and persistent capacities expressed in multiples of 256MB?


Re: [PATCH 05/14] cxl/mem: Register CXL memX devices

2021-01-29 Thread Dan Williams
On Fri, Jan 29, 2021 at 4:25 PM Ben Widawsky  wrote:
>
> From: Dan Williams 
>
> Create the /sys/bus/cxl hierarchy to enumerate:
>
> * Memory Devices (per-endpoint control devices)
>
> * Memory Address Space Devices (platform address ranges with
>   interleaving, performance, and persistence attributes)
>
> * Memory Regions (active provisioned memory from an address space device
>   that is in use as System RAM or delegated to libnvdimm as Persistent
>   Memory regions).
>
> For now, only the per-endpoint control devices are registered on the
> 'cxl' bus. However, going forward it will provide a mechanism to
> coordinate cross-device interleave.
>
> Signed-off-by: Dan Williams 
> Signed-off-by: Ben Widawsky 
> ---
>  Documentation/ABI/testing/sysfs-bus-cxl   |  26 ++
>  .../driver-api/cxl/memory-devices.rst |  17 +
>  drivers/base/core.c   |  14 +
>  drivers/cxl/Makefile  |   3 +
>  drivers/cxl/bus.c |  29 ++
>  drivers/cxl/cxl.h |   4 +
>  drivers/cxl/mem.c | 308 +-
>  include/linux/device.h|   1 +
>  8 files changed, 400 insertions(+), 2 deletions(-)
>  create mode 100644 Documentation/ABI/testing/sysfs-bus-cxl
>  create mode 100644 drivers/cxl/bus.c
[..]
> diff --git a/drivers/base/core.c b/drivers/base/core.c
> index 25e08e5f40bd..33432a4cbe23 100644
> --- a/drivers/base/core.c
> +++ b/drivers/base/core.c
> @@ -3179,6 +3179,20 @@ struct device *get_device(struct device *dev)
>  }
>  EXPORT_SYMBOL_GPL(get_device);
>
> +/**
> + * get_live_device() - increment reference count for device iff !dead
> + * @dev: device.
> + *
> + * Forward the call to get_device() if the device is still alive. If
> + * this is called with the device_lock() held then the device is
> + * guaranteed to not die until the device_lock() is dropped.
> + */
> +struct device *get_live_device(struct device *dev)
> +{
> +   return dev && !dev->p->dead ? get_device(dev) : NULL;
> +}
> +EXPORT_SYMBOL_GPL(get_live_device);

Disregard this hunk, it's an abandoned idea that I failed to cleanup.