Re: [RFC PATCH v4 2/4] cxl: mbox: Factor out the mbox specific data for reuse in switch cci

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

On Fri, 21 Jul 2023 09:48:16 -0700
Davidlohr Bueso <dave@stgolabs.net> wrote:

> On Wed, 19 Jul 2023, Jonathan Cameron wrote:
> 
> > #ifndef __CXLMBOX_H__
> > #define __CXLMBOX_H__  
> 
> Unrelated but looks like cxlmem.h needs s/__CXL_MEM_H__/__CXLMEM_H__
> 
> >
> >-struct cxl_dev_state;
> >-int cxl_pci_mbox_wait_for_doorbell(struct cxl_dev_state *cxlds);
> >-bool cxl_mbox_background_complete(struct cxl_dev_state *cxlds);
> >+#include <linux/irqreturn.h>
> >+#include <linux/export.h>
> >+#include <linux/io.h>
> >+
> >+#include <uapi/linux/cxl_mem.h>
> >+
> >+struct device;
> >+struct cxl_mbox_cmd;  
> 
> Would it make sense to instead move the whole cxl_mbox_cmd out of
> cxlmem.h into here? Same for the cmd rc table stuff. Then cxlmem
> can include cxlmbox.

That makes sense but I'll do it in an additional patch as the chances
of that sort of move being rebase pain is very high.
Rebasing this is nasty already (just did so on top of the currently
cxl/fixes and it wasn't as bad, but still not trivial)

Maybe we can squish it in with this patch for a final merge.

Having had a go at this, it gets a little fiddly to work out what
to move - for example the event logs are general and can turn up
on the switch cci (I think anyway) but hopefully not a DRAM event
record.

For now I've gone for moving less rather that more, particularly
as not that many messages are yet supported on the switch-cci.

Ideally the switch-cci.c file wouldn't include cxlmem.h at all
and would not use a struct cxl_dev_state.  That makes the handling
of register mapping a bit more ugly as the status may or may not
have been mapped before the mbox mappings.  Let's see how bad it
is for RFC v5.



> 
> >+struct cxl_mbox {
> >+	struct device *dev; /* Used for debug prints */
> >+	size_t payload_size;
> >+	struct mutex mbox_mutex; /* Protects device mailbox and firmware */
> >+	DECLARE_BITMAP(enabled_cmds, CXL_MEM_COMMAND_ID_MAX);
> >+	DECLARE_BITMAP(exclusive_cmds, CXL_MEM_COMMAND_ID_MAX);
> >+	struct rcuwait mbox_wait;
> >+	int (*mbox_send)(struct cxl_mbox *mbox,
> >+			 struct cxl_mbox_cmd *cmd);
> >+	bool (*special_irq)(struct cxl_mbox *mbox, u16 opcode);
> >+	void (*special_init_poll)(struct cxl_mbox *mbox);
> >+	bool (*special_bg)(struct cxl_mbox *mbox, u16 opcode);
> >+	u64 (*get_status)(struct cxl_mbox *mbox);
> >+	bool (*can_run)(struct cxl_mbox *mbox, u16 opcode);
> >+	void (*extra_cmds)(struct cxl_mbox *mbox, u16 opcode);  
> 
> Ok, so most of these corner cases are wrt Sanitize. Do you have
> anything in mind what would require such any additional users
> in the future (such as completely taking over the device), beyond
> pci mailbox? Otherwise this feels too ad-hoc with only the naming
> being generic. Perhaps instead have some sort of mbox->type and
> handle accordingly directly in the core mbox calls? 

I don't like the idea of dragging the stuff related to sanitize and
poison into the general mbox code and I do like the ease this gives
of doing something special for the weird corners without making too much
spaghetti. So whilst I agree it's a fair bit of complexity for
a few small corner cases, I think it's striking roughly the right
balance between device specific and generic.

> It would be
> nice to have these callbacks somewhat documented

And spoil the fun? Sure I'll add some stuff.
> 
> Also the 'can_run' name is a bit disconnected from the sanitize
> special case, maybe be rename to something like 'special_canrun'?
> 
> >+	/* Also needs access to registers */
> >+	void __iomem *status, *mbox;
> >+};
> >+  
> 
> Thanks,
> Davidlohr