[PATCH v3 6/8] cache: Support cache maintenance for HiSilicon SoC Hydra Home Agent

[Jonathan Cameron]

On Mon, 8 Sep 2025 15:04:42 -0700
dan.j.williams at intel.com wrote:

> Jonathan Cameron wrote:
> > From: Yushan Wang <wangyushan12 at huawei.com>
> > 
> > Hydra Home Agent is a device used to maintain cache coherency. Add support
> > of explicit cache maintenance operations for it.
> > 
> > Memory resource of HHA conflicts with that of HHA PMU. A workaround is
> > implemented here by replacing devm_ioremap_resource() to devm_ioremap() to
> > workaround the resource conflict check.
> > 
> > Signed-off-by: Yicong Yang <yangyicong at hisilicon.com>
> > Co-developed-by: Yicong Yang <yangyicong at hisilicon.com>
> > Signed-off-by: Yushan Wang <wangyushan12 at huawei.com>
> > Signed-off-by: Jonathan Cameron <Jonathan.Cameron at huawei.com>  
> [..]
> > +static int hisi_soc_hha_probe(struct platform_device *pdev)
> > +{
> > +	struct hisi_soc_hha *soc_hha;
> > +	struct resource *mem;
> > +	int ret;
> > +
> > +	soc_hha = cache_coherency_device_alloc(&hha_ops, struct hisi_soc_hha,
> > +					       ccd);
> > +	if (!soc_hha)
> > +		return -ENOMEM;
> > +
> > +	platform_set_drvdata(pdev, soc_hha);
> > +
> > +	mutex_init(&soc_hha->lock);
> > +
> > +	mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > +	if (!mem)
> > +		return -ENODEV;
> > +
> > +	/*
> > +	 * HHA cache driver share the same register region with HHA uncore PMU
> > +	 * driver in hardware's perspective, none of them should reserve the
> > +	 * resource to itself only.  Here exclusive access verification is
> > +	 * avoided by calling devm_ioremap instead of devm_ioremap_resource to
> > +	 * allow both drivers to exist at the same time.
> > +	 */
> > +	soc_hha->base = ioremap(mem->start, resource_size(mem));
> > +	if (IS_ERR_OR_NULL(soc_hha->base)) {
> > +		ret = dev_err_probe(&pdev->dev, PTR_ERR(soc_hha->base),
> > +				"failed to remap io memory");
> > +		goto err_free_ccd;
> > +	}
> > +
> > +	ret = cache_coherency_device_register(&soc_hha->ccd);
> > +	if (ret)
> > +		goto err_iounmap;
> > +
> > +	return 0;
> > +
> > +err_iounmap:
> > +	iounmap(soc_hha->base);
> > +err_free_ccd:
> > +	cache_coherency_device_free(&soc_hha->ccd);  
> 
> I understand that this scheme works because ccd is the first member and
> that is forced at alloc the same way fwctl does it. However, fwctl hides
> confusing code like this behind put_device() in the free path. So I would
> say if you want to borrow the "_alloc(ops, drv_struct, member)" approach do
> also hide the "offsetof(drv_struct, member) == 0" in the object release
> path and not have eye-popping code like:
> 
>     cache_coherency_device_free(&soc_hha->ccd)
> 
> ...that throws away the allocation side cleverness into a cloud of reader
> confusion. Either make this an actual "device" or otherwise have a kref.
> 
The device option is out because Greg KH was not keen on me using that
infrastructure when we don't have any userspace ABI. 

Kref seems fine but because we have to pass an explicit release to kref_put()
we end up either with the odd looking

kfree_put(&soc_hha->cci, cache_coherency_ops_inst_free);

or wrapping it up with a helper along the lines of
cache_coherency_ops_instance_put(&soc_hha->cci);

That seems reasonable but given there is no real reference counting going on
(the reference count == 1 from alloc to this call) using an actual kref is
perhaps overkill because this is really the same as having no kref and
just implementing.

void cache_coherency_ops_instance_put(struct cache_coherency_ops_inst *cci)
{
	kfree(cci);
}

So other than a rename it is the same as current implementation. :(

So for now I'm thinking have the helper and use a kref even if it's rather
silly just because it will then behave how people (hopefully) expect it to.

Jonathan