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

[<dan.j.williams@intel.com>]

Jonathan Cameron wrote:
> From: Yushan Wang <wangyushan12@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@hisilicon.com>
> Co-developed-by: Yicong Yang <yangyicong@hisilicon.com>
> Signed-off-by: Yushan Wang <wangyushan12@huawei.com>
> Signed-off-by: Jonathan Cameron <Jonathan.Cameron@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.