linux-kernel - Re: [PATCH 3/3] soundwire: ignore uniqueID when irrelevant

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:   Thu, 24 Oct 2019 17:09:11 +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>,
        Sanyog Kale <sanyog.r.kale@...el.com>
Subject: Re: [PATCH 3/3] soundwire: ignore uniqueID when irrelevant

On 22-10-19, 18:48, Pierre-Louis Bossart wrote:
> The uniqueID is useful when there are two or more devices of the same
> type (identical manufacturer ID, part ID) on the same link.

Right!

> When there is a single device of a given type on a link, its uniqueID
> is irrelevant. It's not uncommon on actual platforms to see variations
> of the uniqueID, or differences between devID registers and ACPI _ADR
> fields.

Ideally this should be fixed in firmware, I do not like the fact the we are
poking in core for firmware issues!

> This patch suggests a filter on startup to identify 'single' devices
> and tag them accordingly. 

So you try to see if the board has a single device and mark them so that
you can skip the unique id, did I get that right?

What about the boards which have multiple devices? How doing solve
these?

> The uniqueID is then not used for the probe,
> and the device name omits the uniqueID as well.
> 
> Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@...ux.intel.com>
> ---
>  drivers/soundwire/bus.c   |  7 +++---
>  drivers/soundwire/slave.c | 52 ++++++++++++++++++++++++++++++++++++---
>  2 files changed, 52 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/soundwire/bus.c b/drivers/soundwire/bus.c
> index fc53dbe57f85..be5d437058ed 100644
> --- a/drivers/soundwire/bus.c
> +++ b/drivers/soundwire/bus.c
> @@ -422,10 +422,11 @@ static struct sdw_slave *sdw_get_slave(struct sdw_bus *bus, int i)
>  
>  static int sdw_compare_devid(struct sdw_slave *slave, struct sdw_slave_id id)
>  {
> -	if (slave->id.unique_id != id.unique_id ||
> -	    slave->id.mfg_id != id.mfg_id ||
> +	if (slave->id.mfg_id != id.mfg_id ||
>  	    slave->id.part_id != id.part_id ||
> -	    slave->id.class_id != id.class_id)
> +	    slave->id.class_id != id.class_id ||
> +	    (slave->id.unique_id != SDW_IGNORED_UNIQUE_ID &&
> +	     slave->id.unique_id != id.unique_id))
>  		return -ENODEV;
>  
>  	return 0;
> diff --git a/drivers/soundwire/slave.c b/drivers/soundwire/slave.c
> index 5dbc76772d21..19919975bb6d 100644
> --- a/drivers/soundwire/slave.c
> +++ b/drivers/soundwire/slave.c
> @@ -29,10 +29,17 @@ static int sdw_slave_add(struct sdw_bus *bus,
>  	slave->dev.parent = bus->dev;
>  	slave->dev.fwnode = fwnode;
>  
> -	/* name shall be sdw:link:mfg:part:class:unique */
> -	dev_set_name(&slave->dev, "sdw:%x:%x:%x:%x:%x",
> -		     bus->link_id, id->mfg_id, id->part_id,
> -		     id->class_id, id->unique_id);
> +	if (id->unique_id == SDW_IGNORED_UNIQUE_ID) {
> +		/* name shall be sdw:link:mfg:part:class */
> +		dev_set_name(&slave->dev, "sdw:%x:%x:%x:%x",
> +			     bus->link_id, id->mfg_id, id->part_id,
> +			     id->class_id);
> +	} else {
> +		/* name shall be sdw:link:mfg:part:class:unique */
> +		dev_set_name(&slave->dev, "sdw:%x:%x:%x:%x:%x",
> +			     bus->link_id, id->mfg_id, id->part_id,
> +			     id->class_id, id->unique_id);
> +	}
>  
>  	slave->dev.release = sdw_slave_release;
>  	slave->dev.bus = &sdw_bus_type;
> @@ -103,6 +110,7 @@ static bool find_slave(struct sdw_bus *bus,
>  int sdw_acpi_find_slaves(struct sdw_bus *bus)
>  {
>  	struct acpi_device *adev, *parent;
> +	struct acpi_device *adev2, *parent2;
>  
>  	parent = ACPI_COMPANION(bus->dev);
>  	if (!parent) {
> @@ -112,10 +120,46 @@ int sdw_acpi_find_slaves(struct sdw_bus *bus)
>  
>  	list_for_each_entry(adev, &parent->children, node) {
>  		struct sdw_slave_id id;
> +		struct sdw_slave_id id2;
> +		bool ignore_unique_id = true;
>  
>  		if (!find_slave(bus, adev, &id))
>  			continue;
>  
> +		/* brute-force O(N^2) search for duplicates */
> +		parent2 = parent;
> +		list_for_each_entry(adev2, &parent2->children, node) {
> +
> +			if (adev == adev2)
> +				continue;
> +
> +			if (!find_slave(bus, adev2, &id2))
> +				continue;
> +
> +			if (id.sdw_version != id2.sdw_version ||
> +			    id.mfg_id != id2.mfg_id ||
> +			    id.part_id != id2.part_id ||
> +			    id.class_id != id2.class_id)
> +				continue;
> +
> +			if (id.unique_id != id2.unique_id) {
> +				dev_dbg(bus->dev,
> +					"Valid unique IDs %x %x for Slave mfg %x part %d\n",
> +					id.unique_id, id2.unique_id,
> +					id.mfg_id, id.part_id);
> +				ignore_unique_id = false;
> +			} else {
> +				dev_err(bus->dev,
> +					"Invalid unique IDs %x %x for Slave mfg %x part %d\n",
> +					id.unique_id, id2.unique_id,
> +					id.mfg_id, id.part_id);
> +				return -ENODEV;
> +			}
> +		}
> +
> +		if (ignore_unique_id)
> +			id.unique_id = SDW_IGNORED_UNIQUE_ID;
> +
>  		/*
>  		 * don't error check for sdw_slave_add as we want to continue
>  		 * adding Slaves
> -- 
> 2.20.1

-- 
~Vinod