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.
> 
> >+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? It would be
> nice to have these callbacks somewhat documented.
> 
> Also the 'can_run' name is a bit disconnected from the sanitize
> special case, maybe be rename to something like 'special_canrun'?

I thought a bit more on this and the special_can_run is too specific
as it's not the special command that is restricted, but rather could
be any command as a result of a special command being in flight.
I can't think of a better name for that :(

> 
> >+	/* Also needs access to registers */
> >+	void __iomem *status, *mbox;
> >+};
> >+  
> 
> Thanks,
> Davidlohr