linux-kernel - Re: [PATCH 2/8] soundwire: intel: transition to sdw_master

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date:   Tue, 3 Mar 2020 11:35:47 +0530
From:   Vinod Koul <vkoul@...nel.org>
To:     Pierre-Louis Bossart <pierre-louis.bossart@...ux.intel.com>
Cc:     alsa-devel@...a-project.org, linux-kernel@...r.kernel.org,
        tiwai@...e.de, broonie@...nel.org, gregkh@...uxfoundation.org,
        jank@...ence.com, srinivas.kandagatla@...aro.org,
        slawomir.blauciak@...el.com,
        Bard liao <yung-chuan.liao@...ux.intel.com>,
        Rander Wang <rander.wang@...ux.intel.com>,
        Ranjani Sridharan <ranjani.sridharan@...ux.intel.com>,
        Hui Wang <hui.wang@...onical.com>,
        Sanyog Kale <sanyog.r.kale@...el.com>
Subject: Re: [PATCH 2/8] soundwire: intel: transition to
 sdw_master_device/driver support

On 27-02-20, 16:32, Pierre-Louis Bossart wrote:

> +static struct sdw_intel_ctx
> +*sdw_intel_probe_controller(struct sdw_intel_res *res)
> +{
> +	struct sdw_intel_link_res *link;
> +	struct sdw_intel_ctx *ctx;
> +	struct acpi_device *adev;
> +	struct sdw_master_device *md;
> +	unsigned long time;
> +	u32 link_mask;
> +	int count;
> +	int i;
> +
> +	if (!res)
> +		return NULL;
> +
> +	if (acpi_bus_get_device(res->handle, &adev))
> +		return NULL;
> +
> +	if (!res->count)
> +		return NULL;
> +
> +	count = res->count;
>  	dev_dbg(&adev->dev, "Creating %d SDW Link devices\n", count);
>  
>  	ctx = kzalloc(sizeof(*ctx), GFP_KERNEL);
>  	if (!ctx)
>  		return NULL;
>  
> -	ctx->count = count;
> -	ctx->links = kcalloc(ctx->count, sizeof(*ctx->links), GFP_KERNEL);
> +	ctx->links = kcalloc(count, sizeof(*ctx->links), GFP_KERNEL);
>  	if (!ctx->links)
>  		goto link_err;
>  
> +	ctx->count = count;
> +	ctx->mmio_base = res->mmio_base;
> +	ctx->link_mask = res->link_mask;
> +	ctx->handle = res->handle;
> +
>  	link = ctx->links;
> +	link_mask = ctx->link_mask;
>  
>  	/* Create SDW Master devices */
> -	for (i = 0; i < count; i++) {
> -		if (link_mask && !(link_mask & BIT(i))) {
> -			dev_dbg(&adev->dev,
> -				"Link %d masked, will not be enabled\n", i);
> -			link++;
> +	for (i = 0; i < count; i++, link++) {
> +		if (link_mask && !(link_mask & BIT(i)))
>  			continue;
> -		}
>  
> +		link->mmio_base = res->mmio_base;
>  		link->registers = res->mmio_base + SDW_LINK_BASE
> -					+ (SDW_LINK_SIZE * i);
> +			+ (SDW_LINK_SIZE * i);
>  		link->shim = res->mmio_base + SDW_SHIM_BASE;
>  		link->alh = res->mmio_base + SDW_ALH_BASE;
> -
> +		link->irq = res->irq;
>  		link->ops = res->ops;
>  		link->dev = res->dev;
> +		link->clock_stop_quirks = res->clock_stop_quirks;
>  
> -		memset(&pdevinfo, 0, sizeof(pdevinfo));
> +		md = sdw_master_device_add("intel-master",
> +					   res->parent,
> +					   acpi_fwnode_handle(adev),
> +					   i,
> +					   link);

So we add the device in intel layer

>  
> -		pdevinfo.parent = res->parent;
> -		pdevinfo.name = "int-sdw";
> -		pdevinfo.id = i;
> -		pdevinfo.fwnode = acpi_fwnode_handle(adev);
> +		if (IS_ERR(md)) {
> +			dev_err(&adev->dev, "Could not create link %d\n", i);
> +			goto err;
> +		}
>  
> -		pdev = platform_device_register_full(&pdevinfo);
> -		if (IS_ERR(pdev)) {
> -			dev_err(&adev->dev,
> -				"platform device creation failed: %ld\n",
> -				PTR_ERR(pdev));
> -			goto pdev_err;
> +		/*
> +		 * we need to wait for the bus to be probed so that we
> +		 * can report ACPI information to the upper layers
> +		 */
> +		time = wait_for_completion_timeout(&md->probe_complete,
> +				msecs_to_jiffies(SDW_INTEL_MASTER_PROBE_TIMEOUT));

Then wait for probe to complete..

> +static int
> +sdw_intel_startup_controller(struct sdw_intel_ctx *ctx)
> +{
> +	struct acpi_device *adev;
> +	struct sdw_intel_link_res *link;
> +	struct sdw_master_device *md;
> +	u32 caps;
> +	u32 link_mask;
> +	int i;
> +
> +	if (acpi_bus_get_device(ctx->handle, &adev))
> +		return -EINVAL;
> +
> +	/* Check SNDWLCAP.LCOUNT */
> +	caps = ioread32(ctx->mmio_base + SDW_SHIM_BASE + SDW_SHIM_LCAP);
> +	caps &= GENMASK(2, 0);
> +
> +	/* Check HW supported vs property value */
> +	if (caps < ctx->count) {
> +		dev_err(&adev->dev,
> +			"BIOS master count is larger than hardware capabilities\n");
> +		return -EINVAL;
> +	}
> +
> +	if (!ctx->links)
> +		return -EINVAL;
> +
> +	link = ctx->links;
> +	link_mask = ctx->link_mask;
> +
> +	/* Startup SDW Master devices */
> +	for (i = 0; i < ctx->count; i++, link++) {
> +		if (link_mask && !(link_mask & BIT(i)))
> +			continue;
> +
> +		md = link->md;
> +
> +		sdw_master_device_startup(md);

And finally start the device.

If i look at bigger picture:

PCI SOF driver scans the capabilities and finds SoundWire supported:
 - Invokes sdw_intel_probe_controller() 
        - This adds sdw_master_device and waits till the probe is
          complete.
 - Invokes sdw_intel_startup_controller()
        - It starts up the controller by calling
          sdw_master_device_startup()

Now, I guess at the peril of repeating myself again:

Why not do:

- PCI SOF driver scans the capabilities and finds SoundWire supported:
  - Invokes sdw_intel_probe_controller()
        - This adds sdw_master_device
        - Does rest of device init and context init
  - Invoked sdw_intel_startup_controller()
        - Calls sdw_master_device_startup() to start

You will get more fine grained control of the sequencing, no need to
wait for dummy probe to be over. The device for these would be parent
PCI SOF device and driver PCI SOF driver.

So in summary I do not see a reason for even Intel to have a dummy
soundwire_master_driver.

-- 
~Vinod