linux-kernel - Re: [PATCH 03/14] soundwire: Add Master registration

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:   Sat, 21 Oct 2017 17:05:13 +0530
From:   Vinod Koul <vinod.koul@...el.com>
To:     Mark Brown <broonie@...nel.org>
Cc:     Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        LKML <linux-kernel@...r.kernel.org>,
        ALSA <alsa-devel@...a-project.org>, Takashi <tiwai@...e.de>,
        Pierre <pierre-louis.bossart@...ux.intel.com>,
        Sanyog Kale <sanyog.r.kale@...el.com>,
        Shreyas NC <shreyas.nc@...el.com>, patches.audio@...el.com,
        alan@...ux.intel.com,
        Charles Keepax <ckeepax@...nsource.cirrus.com>,
        Sagar Dharia <sdharia@...eaurora.org>,
        srinivas.kandagatla@...aro.org, plai@...eaurora.org,
        Sudheer Papothi <spapothi@...eaurora.org>
Subject: Re: [PATCH 03/14] soundwire: Add Master registration

On Sat, Oct 21, 2017 at 10:12:30AM +0100, Mark Brown wrote:
> On Thu, Oct 19, 2017 at 08:33:19AM +0530, Vinod Koul wrote:
> 
> > +	/*
> > +	 * SDW is an enumerable bus, but devices can be powered off. So,
> > +	 * they won't be able to report as present.
> > +	 *
> > +	 * Create Slave devices based on Slaves described in
> > +	 * the respective firmware (ACPI/DT)
> > +	 */
> > +
> > +	if (IS_ENABLED(CONFIG_ACPI) && bus->dev && ACPI_HANDLE(bus->dev))
> > +		ret = sdw_acpi_find_slaves(bus);
> > +	else if (IS_ENABLED(CONFIG_OF) && bus->dev && bus->dev->of_node)
> > +		ret = sdw_of_find_slaves(bus);
> > +	else
> > +		ret = -ENOTSUPP; /* No ACPI/DT so error out */
> 
> Devices *can* be powered off but is there any reason that a system
> couldn't be designed with them always powered?  Also given that we don't
> actually have any DT support the stubs for it are at best misleading,
> it's not going to be hard for someone to add them later.

Some people asked me to add it so it is clear how it would be supported on
DT based platforms. I would be too glad to remove if we have consensus on
that. Agree it is not too hard to add :)

> 
> > +	mutex_lock(&bus->bus_lock);
> > +	if (!list_empty(&bus->slaves))
> > +		list_del(&slave->node);
> 
> Shouldn't that be a while?  Or at least warn if there's anything extra
> there.  The code just looks very wrong as is.

I think you missed that it is called from sdw_delete_bus_master() which does
the loop by invoking device_for_each_child(), so this ones is supposed to
ensure one Slave is removed cleaned.

Let me know if i misread your comment.


Thanks
-- 
~Vinod