Re: [PATCH 1/2] cxl: Move mailbox related bits to the same context

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

On Thu, 15 Aug 2024 10:10:23 -0700
Dave Jiang <dave.jiang@intel.com> wrote:

> On 8/15/24 9:57 AM, Jonathan Cameron wrote:
> > On Wed, 24 Jul 2024 11:55:16 -0700
> > Dave Jiang <dave.jiang@intel.com> wrote:
> >   
> >> Create a new 'struct cxl_mailbox' and move all mailbox related bits to
> >> it. This allows isolation of all CXL mailbox data in order to export
> >> some of the calls to external callers and avoid exporting of CXL driver
> >> specific bits such has device states. The allocation of
> >> 'struct cxl_mailbox' is also split out with cxl_mailbox_create() so the
> >> mailbox can be created independently.
> >>
> >> Signed-off-by: Dave Jiang <dave.jiang@intel.com>  
> > Hi Dave,
> > 
> > Mostly good, but cxl_mailbox_create() needs another look.
> > Feels like a bit of cut and paste gone too far ;)
> > 
> > I'm not entirely convinced it shouldn't just remain embedded in
> > the parent structure with create replace with just an init()
> > function.  I don't mind that much though.  
> 
> I made it a pointer to deal with something like a type2 device without mailbox at all. So I feel like it needs to stand on its own.... but are you saying we should just keep it as an embedded struct in the device context whether a mailbox exists or not?

It can be optional for a given driver, so type 3 just embeds
it, a type 2 with the support also embeds and init() the mailbox.
A type 2 without doesn't embed it in the driver specific structures
and obviously doesn't init what it doesn't have.

So just like a mutex inside a driver specific struct
and similar things.
> 
> Also, probably need to start thinking about this. Once PCIe MMIO mailbox shows up, we will want to move the register pointer into the mailbox as well to make it independent and having a shared struct between CXL and PCIe. It'll probably end up looking like:
> 
> struct cxl_mailbox {
> 	struct mmio_mailbox mbox;
> }; 
Yeah. I'd expect a nest like this with container_of() magic used where
appropriate to get to the parents and probably a few callbacks
set in the mbox to tie everything together.

Jonathan

> 
> > 
> > Jonathan
> > 
> >   
> >> diff --git a/drivers/cxl/core/mbox.c b/drivers/cxl/core/mbox.c
> >> index 2626f3fff201..9501d2576ccd 100644  
> > 
> >   
> >> @@ -1408,6 +1454,34 @@ int cxl_poison_state_init(struct cxl_memdev_state *mds)
> >>  }
> >>  EXPORT_SYMBOL_NS_GPL(cxl_poison_state_init, CXL);
> >>  
> >> +static void free_cxl_mailbox(void *cxl_mbox)
> >> +{
> >> +	kfree(cxl_mbox);
> >> +}
> >> +
> >> +struct cxl_mailbox *cxl_mailbox_create(struct device *dev)
> >> +{
> >> +	struct cxl_mailbox *cxl_mbox __free(kfree) =
> >> +		kzalloc(sizeof(*cxl_mbox), GFP_KERNEL);  
> > This is not somewhere I'd use the __free magic, because
> > failure to create a mailbox is very likely fatal and doing
> > so requires the dance with devm_add_action_or_reset() to
> > just free a structure.
> > 
> > So I'd just use devm_kzalloc()
> > and rely on caller to fail probe() or if not to do manual
> > cleanup of the structure.
> > 
> > There aren't any failure paths currently where the auto cleanup
> > gets used anyway other than the devm_add_action_or_reset()
> > which is not using auto cleanup anyway.
> >   
> >> +	int rc;
> >> +
> >> +	if (!cxl_mbox)
> >> +		return ERR_PTR(-ENOMEM);
> >> +
> >> +	cxl_mbox->host = dev;
> >> +	mutex_init(&cxl_mbox->mbox_mutex);
> >> +	rcuwait_init(&cxl_mbox->mbox_wait);
> >> +
> >> +	rc = devm_add_action_or_reset(dev, free_cxl_mailbox, cxl_mbox);
> >> +	if (rc) {
> >> +		cxl_mbox = NULL;  
> > Non obvious as that's a suppression of what would otherwise
> > be a double free.  Having to do this rather defeats the point
> > of the simplification __free normally brings.
> >   
> >> +		return ERR_PTR(rc);
> >> +	}
> >> +
> >> +	return no_free_ptr(cxl_mbox);
> >> +}
> >> +EXPORT_SYMBOL_NS_GPL(cxl_mailbox_create, CXL);  
>