Re: [PATCH 03/19] cxl/mbox: Move mailbox related driver state to its own data structure

From: Dan Williams <dan.j.williams@intel.com>
To: Jonathan Cameron <Jonathan.Cameron@huawei.com>,
	Dan Williams <dan.j.williams@intel.com>
Cc: <linux-cxl@vger.kernel.org>, <ira.weiny@intel.com>,
	<navneet.singh@intel.com>
Subject: Re: [PATCH 03/19] cxl/mbox: Move mailbox related driver state to its own data structure
Date: Tue, 13 Jun 2023 17:45:13 -0700	[thread overview]
Message-ID: <64890d99746c5_2f4fca29457@dwillia2-xfh.jf.intel.com.notmuch> (raw)
In-Reply-To: <20230606121054.000069e1@Huawei.com>

Jonathan Cameron wrote:
> On Sun, 04 Jun 2023 16:31:54 -0700
> Dan Williams <dan.j.williams@intel.com> wrote:
> 
> > 'struct cxl_dev_state' makes too many assumptions about the capabilities
> > of a CXL device. In particular it assumes a CXL device has a mailbox and
> > all of the infrastructure and state that comes along with that.
> > 
> > In preparation for supporting accelerator / Type-2 devices that may not
> > have a mailbox and in general maintain a minimal core context structure,
> > make mailbox functionality a super-set of  'struct cxl_dev_state' with
> > 'struct cxl_memdev_state'.
> > 
> > With this reorganization it allows for CXL devices that support HDM
> > decoder mapping, but not other general-expander / Type-3 capabilities,
> > to only enable that subset without the rest of the mailbox
> > infrastructure coming along for the ride.
> > 
> > Signed-off-by: Dan Williams <dan.j.williams@intel.com>
> 
> I'm not yet sure that the division in exactly in the right place, but we
> can move things later if it turns out some elements are more general than
> we currently think.

Agree, it is more along the lines of: the current trajectory of 'struct
cxl_dev_state' is unsustainable, stem the tide, and revisit as needed.

> 
> A few trivial things inline.
> Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> 
> 
> > ---
> 
>  
> > -static struct cxl_mbox_get_supported_logs *cxl_get_gsl(struct cxl_dev_state *cxlds)
> > +static struct cxl_mbox_get_supported_logs *
> > +cxl_get_gsl(struct cxl_memdev_state *mds)
> 
> I'd consider keeping this on one line.  It was between 80 and 90 before and still is...
> 
> 
> >  {
> 
> 
> > diff --git a/drivers/cxl/cxlmem.h b/drivers/cxl/cxlmem.h
> > index a2845a7a69d8..d3fe73d5ba4d 100644
> > --- a/drivers/cxl/cxlmem.h
> > +++ b/drivers/cxl/cxlmem.h
> > @@ -267,6 +267,35 @@ struct cxl_poison_state {
> >   * @cxl_dvsec: Offset to the PCIe device DVSEC
> >   * @rcd: operating in RCD mode (CXL 3.0 9.11.8 CXL Devices Attached to an RCH)
> >   * @media_ready: Indicate whether the device media is usable
> > + * @dpa_res: Overall DPA resource tree for the device
> > + * @pmem_res: Active Persistent memory capacity configuration
> > + * @ram_res: Active Volatile memory capacity configuration
> > + * @component_reg_phys: register base of component registers
> > + * @info: Cached DVSEC information about the device.
> 
> Not seeing info in this structure.
> 
> > + * @serial: PCIe Device Serial Number
> > + */
> > +struct cxl_dev_state {
> > +	struct device *dev;
> > +	struct cxl_memdev *cxlmd;
> > +	struct cxl_regs regs;
> > +	int cxl_dvsec;
> > +	bool rcd;
> > +	bool media_ready;
> > +	struct resource dpa_res;
> > +	struct resource pmem_res;
> > +	struct resource ram_res;
> > +	resource_size_t component_reg_phys;
> > +	u64 serial;
> > +};
> > +
> > +/**
> > + * struct cxl_memdev_state - Generic Type-3 Memory Device Class driver data
> > + *
> > + * CXL 8.1.12.1 PCI Header - Class Code Register Memory Device defines
> > + * common memory device functionality like the presence of a mailbox and
> > + * the functionality related to that like Identify Memory Device and Get
> > + * Partition Info
> > + * @cxlds: Core driver state common across Type-2 and Type-3 devices
> >   * @payload_size: Size of space for payload
> >   *                (CXL 2.0 8.2.8.4.3 Mailbox Capabilities Register)
> >   * @lsa_size: Size of Label Storage Area
> > @@ -275,9 +304,6 @@ struct cxl_poison_state {
> >   * @firmware_version: Firmware version for the memory device.
> >   * @enabled_cmds: Hardware commands found enabled in CEL.
> >   * @exclusive_cmds: Commands that are kernel-internal only
> > - * @dpa_res: Overall DPA resource tree for the device
> > - * @pmem_res: Active Persistent memory capacity configuration
> > - * @ram_res: Active Volatile memory capacity configuration
> >   * @total_bytes: sum of all possible capacities
> >   * @volatile_only_bytes: hard volatile capacity
> >   * @persistent_only_bytes: hard persistent capacity
> > @@ -286,54 +312,41 @@ struct cxl_poison_state {
> >   * @active_persistent_bytes: sum of hard + soft persistent
> >   * @next_volatile_bytes: volatile capacity change pending device reset
> >   * @next_persistent_bytes: persistent capacity change pending device reset
> > - * @component_reg_phys: register base of component registers
> > - * @info: Cached DVSEC information about the device.
> 
> Not seeing this removed from this structure in this patch.
> Curiously doesn't seem to be here in first place.
> 
> Probably wants precursor fix patch to get rid of it from the docs.

I did some digging and it turns out the cxlmem.h is not even built
during a docs build, but I went ahead and added another patch to clean
up warnings from:

./scripts/kernel-doc drivers/cxl/cxlmem.h

I will note though that the extra kdoc descriptor was not flagged, only
missing attribute definitions are flagged.