Re: [PATCH v3 15/40] cxl: Prove CXL locking

[Jonathan Cameron <Jonathan.Cameron@Huawei.com>]

On Sun, 23 Jan 2022 16:29:58 -0800
Dan Williams <dan.j.williams@intel.com> wrote:

> When CONFIG_PROVE_LOCKING is enabled the 'struct device' definition gets
> an additional mutex that is not clobbered by
> lockdep_set_novalidate_class() like the typical device_lock(). This
> allows for local annotation of subsystem locks with mutex_lock_nested()
> per the subsystem's object/lock hierarchy. For CXL, this primarily needs
> the ability to lock ports by depth and child objects of ports by their
> parent parent-port lock.
> 
> Signed-off-by: Dan Williams <dan.j.williams@intel.com>

Hi Dan,

This infrastructure is nice.

A few comments inline - mostly requests for a few comments to make
life easier when reading this in future.  Also, I'd slightly prefer
this as 2 patches so the trivial nvdimm / Kconfig.debug stuff is separate
from the patch actually introducing support for this in CXL.

Anyhow, all trivial stuff so as far as I'm concerned.

Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>

Thanks,

Jonathan

> ---
>  drivers/cxl/acpi.c       |   10 +++---
>  drivers/cxl/core/pmem.c  |    4 +-
>  drivers/cxl/core/port.c  |   43 ++++++++++++++++++++-------
>  drivers/cxl/cxl.h        |   74 ++++++++++++++++++++++++++++++++++++++++++++++
>  drivers/cxl/pmem.c       |   12 ++++---
>  drivers/nvdimm/nd-core.h |    2 +
>  lib/Kconfig.debug        |   23 ++++++++++++++
>  7 files changed, 143 insertions(+), 25 deletions(-)
> 


> @@ -712,15 +725,23 @@ static int cxl_bus_match(struct device *dev, struct device_driver *drv)
>  
>  static int cxl_bus_probe(struct device *dev)
>  {
> -	return to_cxl_drv(dev->driver)->probe(dev);
> +	int rc;
> +
> +	cxl_nested_lock(dev);

I guess it is 'fairly' obvious why this call is here (I assume because the device
lock is already held), but maybe worth a comment?

> +	rc = to_cxl_drv(dev->driver)->probe(dev);
> +	cxl_nested_unlock(dev);
> +
> +	return rc;
>  }
>  
>  static void cxl_bus_remove(struct device *dev)
>  {
>  	struct cxl_driver *cxl_drv = to_cxl_drv(dev->driver);
>  
> +	cxl_nested_lock(dev);
>  	if (cxl_drv->remove)
>  		cxl_drv->remove(dev);
> +	cxl_nested_unlock(dev);
>  }
>  
>  struct bus_type cxl_bus_type = {
> diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h
> index c1dc53492773..569cbe7f23d6 100644
> --- a/drivers/cxl/cxl.h
> +++ b/drivers/cxl/cxl.h
> @@ -285,6 +285,7 @@ static inline bool is_cxl_root(struct cxl_port *port)
>  	return port->uport == port->dev.parent;
>  }
>  
> +bool is_cxl_port(struct device *dev);
>  struct cxl_port *to_cxl_port(struct device *dev);
>  struct cxl_port *devm_cxl_add_port(struct device *host, struct device *uport,
>  				   resource_size_t component_reg_phys,
> @@ -295,6 +296,7 @@ int cxl_add_dport(struct cxl_port *port, struct device *dport, int port_id,
>  
>  struct cxl_decoder *to_cxl_decoder(struct device *dev);
>  bool is_root_decoder(struct device *dev);
> +bool is_cxl_decoder(struct device *dev);
>  struct cxl_decoder *cxl_root_decoder_alloc(struct cxl_port *port,
>  					   unsigned int nr_targets);
>  struct cxl_decoder *cxl_switch_decoder_alloc(struct cxl_port *port,
> @@ -347,4 +349,76 @@ struct cxl_nvdimm_bridge *cxl_find_nvdimm_bridge(struct cxl_nvdimm *cxl_nvd);
>  #ifndef __mock
>  #define __mock static
>  #endif
> +
> +#ifdef CONFIG_PROVE_CXL_LOCKING
> +enum cxl_lock_class {
> +	CXL_ANON_LOCK,
> +	CXL_NVDIMM_LOCK,
> +	CXL_NVDIMM_BRIDGE_LOCK,
> +	CXL_PORT_LOCK,

As you are going to increment off the end of this perhaps a comment
here so that no one thinks "I'll just add another entry after CXL_PORT_LOCK"

> +};
> +
> +static inline void cxl_nested_lock(struct device *dev)
> +{
> +	if (is_cxl_port(dev)) {
> +		struct cxl_port *port = to_cxl_port(dev);
> +
> +		mutex_lock_nested(&dev->lockdep_mutex,
> +				  CXL_PORT_LOCK + port->depth);
> +	} else if (is_cxl_decoder(dev)) {
> +		struct cxl_port *port = to_cxl_port(dev->parent);
> +
> +		mutex_lock_nested(&dev->lockdep_mutex,
> +				  CXL_PORT_LOCK + port->depth + 1);

Perhaps a comment on why port->dev + 1 is a safe choice?
Not immediately obvious to me and I'm too lazy to figure it out :)

> +	} else if (is_cxl_nvdimm_bridge(dev))
> +		mutex_lock_nested(&dev->lockdep_mutex, CXL_NVDIMM_BRIDGE_LOCK);
> +	else if (is_cxl_nvdimm(dev))
> +		mutex_lock_nested(&dev->lockdep_mutex, CXL_NVDIMM_LOCK);
> +	else
> +		mutex_lock_nested(&dev->lockdep_mutex, CXL_ANON_LOCK);
> +}
> +
> +static inline void cxl_nested_unlock(struct device *dev)
> +{
> +	mutex_unlock(&dev->lockdep_mutex);
> +}
> +
> +static inline void cxl_device_lock(struct device *dev)
> +{
> +	/*
> +	 * For double lock errors the lockup will happen before lockdep
> +	 * warns at cxl_nested_lock(), so assert explicitly.
> +	 */
> +	lockdep_assert_not_held(&dev->lockdep_mutex);
> +
> +	device_lock(dev);
> +	cxl_nested_lock(dev);
> +}
> +
> +static inline void cxl_device_unlock(struct device *dev)
> +{
> +	cxl_nested_unlock(dev);
> +	device_unlock(dev);
> +}
> +#else
> +static inline void cxl_nested_lock(struct device *dev)
> +{
> +}
> +
> +static inline void cxl_nested_unlock(struct device *dev)
> +{
> +}
> +
> +static inline void cxl_device_lock(struct device *dev)
> +{
> +	device_lock(dev);
> +}
> +
> +static inline void cxl_device_unlock(struct device *dev)
> +{
> +	device_unlock(dev);
> +}
> +#endif
> +
> +

One blank line only.

>  #endif /* __CXL_H__ */
...
> diff --git a/drivers/nvdimm/nd-core.h b/drivers/nvdimm/nd-core.h
> index a11850dd475d..2650a852eeaf 100644
> --- a/drivers/nvdimm/nd-core.h
> +++ b/drivers/nvdimm/nd-core.h
> @@ -185,7 +185,7 @@ static inline void devm_nsio_disable(struct device *dev,
>  }
>  #endif
>  
> -#ifdef CONFIG_PROVE_LOCKING
> +#ifdef CONFIG_PROVE_NVDIMM_LOCKING
>  extern struct class *nd_class;
>  
>  enum {
> diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug
> index 9ef7ce18b4f5..ea9291723d06 100644
> --- a/lib/Kconfig.debug
> +++ b/lib/Kconfig.debug
> @@ -1509,6 +1509,29 @@ config CSD_LOCK_WAIT_DEBUG
>  	  include the IPI handler function currently executing (if any)
>  	  and relevant stack traces.
>  
> +choice
> +	prompt "Lock debugging: prove subsystem device_lock() correctness"
> +	depends on PROVE_LOCKING
> +	help
> +	  For subsystems that have instrumented their usage of the device_lock()
> +	  with nested annotations, enable lock dependency checking. The locking
> +	  hierarchy 'subclass' identifiers are not compatible across
> +	  sub-systems, so only one can be enabled at a time.
> +
> +config PROVE_NVDIMM_LOCKING
> +	bool "NVDIMM"
> +	depends on LIBNVDIMM
> +	help
> +	  Enable lockdep to validate nd_device_lock() usage.

I would slightly have preferred a first patch that pulled out the NVDIMM parts
and a second that introduced it for CXL.

> +
> +config PROVE_CXL_LOCKING
> +	bool "CXL"
> +	depends on CXL_BUS
> +	help
> +	  Enable lockdep to validate cxl_device_lock() usage.
> +
> +endchoice
> +
>  endmenu # lock debugging
>  
>  config TRACE_IRQFLAGS
>