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 Thu, 3 Aug 2023 17:47:51 +0100
Jonathan Cameron <Jonathan.Cameron@Huawei.com> wrote:

> 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.
Was definitely a useful exercise. One was never called and the other
one was only used in one local path in pci.c so no need to have a callback.

Thanks :)

Jonathan

> > 
> > 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  
> 
>