linux-kernel - Re: [PATCH v2 2/4] soundwire: core: add device tree support for slave devices

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]

Message-ID: <20190809054602.GK12733@vkoul-mobl.Dlink>
Date:   Fri, 9 Aug 2019 11:16:02 +0530
From:   Vinod Koul <vkoul@...nel.org>
To:     Srinivas Kandagatla <srinivas.kandagatla@...aro.org>
Cc:     Pierre-Louis Bossart <pierre-louis.bossart@...ux.intel.com>,
        broonie@...nel.org, bgoswami@...eaurora.org, plai@...eaurora.org,
        robh+dt@...nel.org, devicetree@...r.kernel.org,
        lgirdwood@...il.com, alsa-devel@...a-project.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 2/4] soundwire: core: add device tree support for
 slave devices

On 08-08-19, 16:17, Srinivas Kandagatla wrote:
> Thanks for taking time to review.
> 
> On 08/08/2019 16:00, Pierre-Louis Bossart wrote:
> > 
> > > @@ -35,6 +36,7 @@ static int sdw_slave_add(struct sdw_bus *bus,
> > >       slave->dev.release = sdw_slave_release;
> > >       slave->dev.bus = &sdw_bus_type;
> > > +    slave->dev.of_node = of_node_get(to_of_node(fwnode));
> > 
> > shouldn't this protected by
> > #if IS_ENABLED(CONFIG_OF) ?
> > 
> These macros and functions have dummy entries, so it should not be an issue.
> I did build soundwire with i386_defconfig with no issues.

That means this function was compiled without errors, that is not strange nowadays
given the ARM compiles ACPI and x86 OF, so check with OF being disable
just to be safe :) I think dummy entries are helping

> 
> > >       slave->bus = bus;
> > >       slave->status = SDW_SLAVE_UNATTACHED;
> > >       slave->dev_num = 0;
> > > @@ -112,3 +114,48 @@ int sdw_acpi_find_slaves(struct sdw_bus *bus)
> > >   }
> > >   #endif
> > > +
> > > +/*
> > > + * sdw_of_find_slaves() - Find Slave devices in master device tree node
> > > + * @bus: SDW bus instance
> > > + *
> > > + * Scans Master DT node for SDW child Slave devices and registers it.
> > > + */
> > > +int sdw_of_find_slaves(struct sdw_bus *bus)
> > > +{
> > > +    struct device *dev = bus->dev;
> > > +    struct device_node *node;
> > > +
> > > +    for_each_child_of_node(bus->dev->of_node, node) {
> > > +        struct sdw_slave_id id;
> > > +        const char *compat = NULL;
> > > +        int unique_id, ret;
> > > +        int ver, mfg_id, part_id, class_id;
> > > +
> > > +        compat = of_get_property(node, "compatible", NULL);
> > > +        if (!compat)
> > > +            continue;
> > > +
> > > +        ret = sscanf(compat, "sdw%x,%x,%x,%x",
> > > +                 &ver, &mfg_id, &part_id, &class_id);
> > > +        if (ret != 4) {
> > > +            dev_err(dev, "Manf ID & Product code not found %s\n",
> > > +                compat);
> > > +            continue;
> > > +        }
> > > +
> > > +        ret = of_property_read_u32(node, "sdw-instance-id", &unique_id);
> > > +        if (ret) {
> > > +            dev_err(dev, "Instance id not found:%d\n", ret);
> > > +            continue;
> > 
> > I am confused here.
> > If you have two identical devices on the same link, isn't this property
> > required and that should be a real error instead of a continue?
> 
> Yes, I agree it will be mandatory in such cases.
> 
> Am okay either way, I dont mind changing it to returning EINVAL in all the
> cases.

Do we want to abort? We are in loop scanning for devices so makes sense
if we do not do that and continue to check next one..

-- 
~Vinod