linux-kernel - Re: [PATCH 2/3] soundwire: qcom: add auto enumeration support

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:   Wed, 3 Mar 2021 09:38:21 +0000
From:   Srinivas Kandagatla <srinivas.kandagatla@...aro.org>
To:     Pierre-Louis Bossart <pierre-louis.bossart@...ux.intel.com>,
        vkoul@...nel.org
Cc:     yung-chuan.liao@...ux.intel.com, sanyog.r.kale@...el.com,
        linux-kernel@...r.kernel.org, alsa-devel@...a-project.org
Subject: Re: [PATCH 2/3] soundwire: qcom: add auto enumeration support



On 02/03/2021 14:34, Pierre-Louis Bossart wrote:
> 
> 
>>>> +        if (!val1 && !val2)
>>>> +            break;
>>>> +
>>>> +        addr = buf2[1] | (buf2[0] << 8) | (buf1[3] << 16) |
>>>> +            ((u64)buf1[2] << 24) | ((u64)buf1[1] << 32) |
>>>> +            ((u64)buf1[0] << 40);
>>>> +
>>>> +        sdw_extract_slave_id(bus, addr, &id);
>>>> +        /* Now compare with entries */
>>>> +        list_for_each_entry_safe(slave, _s, &bus->slaves, node) {
>>>> +            if (sdw_compare_devid(slave, id) == 0) {
>>>> +                u32 status = qcom_swrm_get_n_device_status(ctrl, i);
>>>> +                if (status == SDW_SLAVE_ATTACHED) {
>>>> +                    slave->dev_num = i;
>>>> +                    mutex_lock(&bus->bus_lock);
>>>> +                    set_bit(i, bus->assigned);
>>>> +                    mutex_unlock(&bus->bus_lock);
>>>> +
>>>> +                }
>>>
>>> And that part is strange as well. The bus->assigned bit should be set 
>>> even if the Slave is not in the list provided by platform firmware. 
>>> It's really tracking the state of the hardware, and it should not be 
>>> influenced by what software knows to manage.
>>
>> Am not 100% sure If I understand the concern here, but In normal (non 
>> auto enum) cases this bit is set by the bus code while its doing 
>> enumeration to assign a dev number from the assigned bitmap!
>>
>> However in this case where auto enumeration happens it makes sense to 
>> set this here with matching dev number!
>>
>> AFAIU from code, each bit in this bitmap corresponds to slave dev number!
> 
> Yes, but the point was "why do you compare with information coming from 
> platform firmware"? if the hardware reports the presence of devices on 

This is the logic that hardware IP document suggests to use to get get 
the correct the device number associated with the slave!


> the link, why not use the information as is?
> 
> You recently added code that helps us deal with devices that are not 
> listed in DT or ACPI tables, so why would we filter in this specific loop?
> 
>>>> +    complete(&ctrl->enumeration);
>>>
>>> you have init_completion() and complete() in this patch, but no 
>>> wait_for_completion(), so that should be added in a later patch, no?
>>
>> make sense, will move that to other patch!
> 
> Actually on this one comment that I missed last time is that you are 
> using a completion only for the resume() case, and I think it should 
> also be used for the regular probe() case, no?
Good Idea, I can try that and see how to works out!

--srini
>