On Tue, 06 Sep 2022 14:47:58 +0100,
Nipun Gupta [off-list ref] wrote:

Why do we need such a wrapper around find_host_domain()?

+	if (!cdx_msi_domain) {
+		cdx_msi_domain = cdx_msi_domain_init(parent);

This is racy. If device are populated in parallel, bad things will
happen.

+		if (!cdx_msi_domain) {
+			dev_err(&cdx_dev->dev,
+				"cdx_msi_domain_init() failed: %d", ret);
+			kfree(cdx_dev);
+			return -1;

Use standard error codes.

No, please. Use shifts, and use a script if decimal conversion fails
you. We're not playing these games. And the cast is pointless.

Yes, you have lifted it from the FSL code, bad move. /me makes a note
to go and clean-up this crap.

+}
+
+static void cdx_msi_set_desc(msi_alloc_info_t *arg,
+			     struct msi_desc *desc)
+{
+	arg->desc = desc;
+	arg->hwirq = cdx_domain_calc_hwirq(to_cdx_device(desc->dev), desc);
+}
+#else
+#define cdx_msi_set_desc NULL

Why the ifdefery? This should *only* be supported with
GENERIC_MSI_DOMAIN_OPS.

+#endif
+
+static void cdx_msi_update_dom_ops(struct msi_domain_info *info)
+{
+	struct msi_domain_ops *ops = info->ops;
+
+	if (!ops)
+		return;
+
+	/* set_desc should not be set by the caller */
+	if (!ops->set_desc)
+		ops->set_desc = cdx_msi_set_desc;

Then why are you allowing this to be overridden?

+}
+
+static void cdx_msi_write_msg(struct irq_data *irq_data,
+			      struct msi_msg *msg)
+{
+	/*
+	 * Do nothing as CDX devices have these pre-populated
+	 * in the hardware itself.
+	 */

We talked about this in a separate thread. This is a major problem.

+}
+
+static void cdx_msi_update_chip_ops(struct msi_domain_info *info)
+{
+	struct irq_chip *chip = info->chip;
+
+	if (!chip)
+		return;
+
+	/*
+	 * irq_write_msi_msg should not be set by the caller
+	 */
+	if (!chip->irq_write_msi_msg)
+		chip->irq_write_msi_msg = cdx_msi_write_msg;

Then why the check?

+}
+/**
+ * cdx_msi_create_irq_domain - Create a CDX MSI interrupt domain
+ * @fwnode:	Optional firmware node of the interrupt controller
+ * @info:	MSI domain info
+ * @parent:	Parent irq domain
+ *
+ * Updates the domain and chip ops and creates a CDX MSI
+ * interrupt domain.
+ *
+ * Returns:
+ * A domain pointer or NULL in case of failure.
+ */
+static struct irq_domain *cdx_msi_create_irq_domain(struct fwnode_handle *fwnode,
+						    struct msi_domain_info *info,
+						    struct irq_domain *parent)
+{
+	if (WARN_ON((info->flags & MSI_FLAG_LEVEL_CAPABLE)))
+		info->flags &= ~MSI_FLAG_LEVEL_CAPABLE;

No. Just fail the domain creation. We shouldn't paper over these things.

+	if (info->flags & MSI_FLAG_USE_DEF_DOM_OPS)
+		cdx_msi_update_dom_ops(info);
+	if (info->flags & MSI_FLAG_USE_DEF_CHIP_OPS)
+		cdx_msi_update_chip_ops(info);

Under what circumstances would the default ops not be used? The only
caller is in this file and has pre-computed values.

This looks like a copy/paste from platform-msi.c.

+	info->flags |= MSI_FLAG_ALLOC_SIMPLE_MSI_DESCS | MSI_FLAG_FREE_MSI_DESCS;
+
+	return msi_create_irq_domain(fwnode, info, parent);

This whole function makes no sense. You should move everything to the
relevant structures, and simply call msi_create_irq_domain() from the
sole caller of this function.

+}
+
+int cdx_msi_domain_alloc_irqs(struct device *dev, unsigned int irq_count)
+{
+	struct irq_domain *msi_domain;
+	int ret;
+
+	msi_domain = dev_get_msi_domain(dev);
+	if (!msi_domain) {

How can that happen?

+		dev_err(dev, "msi domain get failed\n");
+		return -EINVAL;
+	}
+
+	ret = msi_setup_device_data(dev);
+	if (ret) {
+		dev_err(dev, "msi setup device failed: %d\n", ret);
+		return ret;
+	}
+
+	msi_lock_descs(dev);
+	if (msi_first_desc(dev, MSI_DESC_ALL))
+		ret = -EINVAL;
+	msi_unlock_descs(dev);
+	if (ret) {
+		dev_err(dev, "msi setup device failed: %d\n", ret);

Same message twice, not very useful. Consider grouping these things at
the end of the function and make use of a (oh Gawd) goto...

+		return ret;
+	}
+
+	ret = msi_domain_alloc_irqs(msi_domain, dev, irq_count);
+	if (ret)
+		dev_err(dev, "Failed to allocate IRQs\n");
+
+	return ret;
+}
+EXPORT_SYMBOL(cdx_msi_domain_alloc_irqs);

EXPORT_SYMBOL_GPL(), please, for all the exports.

+
+void cdx_msi_domain_free_irqs(struct device *dev)
+{
+	struct irq_domain *msi_domain;
+
+	msi_domain = dev_get_msi_domain(dev);
+	if (!msi_domain)

Again, how can that happen?

+		return;
+
+	msi_domain_free_irqs(msi_domain, dev);
+}
+EXPORT_SYMBOL(cdx_msi_domain_free_irqs);

This feels like a very pointless helper, and again a copy/paste from
the FSL code. I'd rather you change msi_domain_free_irqs() to only
take a device and use the implicit MSI domain.

+
+static struct irq_chip cdx_msi_irq_chip = {
+	.name = "CDX-MSI",
+	.irq_mask = irq_chip_mask_parent,
+	.irq_unmask = irq_chip_unmask_parent,
+	.irq_eoi = irq_chip_eoi_parent,
+	.irq_set_affinity = msi_domain_set_affinity

nit: please align things vertically.

+};
+
+static int cdx_msi_prepare(struct irq_domain *msi_domain,
+			   struct device *dev,
+			   int nvec, msi_alloc_info_t *info)
+{
+	struct cdx_device *cdx_dev = to_cdx_device(dev);
+	struct msi_domain_info *msi_info;
+	struct device *parent = dev->parent;
+	u32 dev_id;
+	int ret;
+
+	/* Retrieve device ID from requestor ID using parent device */
+	ret = of_map_id(parent->of_node, cdx_dev->req_id, "msi-map",
+			"msi-map-mask",	NULL, &dev_id);
+	if (ret) {
+		dev_err(dev, "of_map_id failed for MSI: %d\n", ret);
+		return ret;
+	}
+
+	/* Set the device Id to be passed to the GIC-ITS */
+	info->scratchpad[0].ul = dev_id;
+
+	msi_info = msi_get_domain_info(msi_domain->parent);
+
+	/* Allocate at least 32 MSIs, and always as a power of 2 */

Where is this requirement coming from?

+	nvec = max_t(int, 32, roundup_pow_of_two(nvec));
+	return msi_info->ops->msi_prepare(msi_domain->parent, dev, nvec, info);
+}
+
+static struct msi_domain_ops cdx_msi_ops __ro_after_init = {
+	.msi_prepare = cdx_msi_prepare,
+};
+
+static struct msi_domain_info cdx_msi_domain_info = {
+	.flags	= (MSI_FLAG_USE_DEF_DOM_OPS | MSI_FLAG_USE_DEF_CHIP_OPS),
+	.ops	= &cdx_msi_ops,
+	.chip	= &cdx_msi_irq_chip,
+};
+
+struct irq_domain *cdx_msi_domain_init(struct device *dev)
+{
+	struct irq_domain *parent;
+	struct irq_domain *cdx_msi_domain;
+	struct fwnode_handle *fwnode_handle;
+	struct device_node *parent_node;
+	struct device_node *np = dev->of_node;
+
+	fwnode_handle = of_node_to_fwnode(np);
+
+	parent_node = of_parse_phandle(np, "msi-map", 1);

Huh. This only works because you are stuck with a single ITS per system.

Why the hardcoded 4? Is that part of the firmware emulation stuff?

	M.

-- 
Without deviation from the norm, progress is not possible.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel