linux-kernel - Re: [PATCH V6 5/8] soundwire: amd: add SoundWire manager interrupt handling

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] [day] [month] [year] [list]

Message-ID: <de3ae97a-50de-8906-d370-e93122e2fb16@amd.com>
Date:   Fri, 17 Mar 2023 20:16:36 +0530
From:   "Mukunda,Vijendar" <vijendar.mukunda@....com>
To:     Vinod Koul <vkoul@...nel.org>
Cc:     alsa-devel@...a-project.org, pierre-louis.bossart@...ux.intel.com,
        Basavaraj.Hiregoudar@....com, Sunil-kumar.Dommati@....com,
        Mario.Limonciello@....com, amadeuszx.slawinski@...ux.intel.com,
        Mastan.Katragadda@....com, Arungopal.kondaveeti@....com,
        claudiu.beznea@...rochip.com,
        Bard Liao <yung-chuan.liao@...ux.intel.com>,
        Sanyog Kale <sanyog.r.kale@...el.com>,
        open list <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH V6 5/8] soundwire: amd: add SoundWire manager interrupt
 handling

On 17/03/23 19:06, Vinod Koul wrote:
> On 16-03-23, 22:34, Mukunda,Vijendar wrote:
>> On 15/03/23 15:36, Vinod Koul wrote:
>>> On 07-03-23, 19:01, Vijendar Mukunda wrote:
>>>> +static void amd_sdw_update_slave_status_work(struct work_struct *work)
>>>> +{
>>>> +	struct amd_sdw_manager *amd_manager =
>>>> +		container_of(work, struct amd_sdw_manager, amd_sdw_work);
>>>> +	int retry_count = 0;
>>>> +
>>>> +	if (amd_manager->status[0] == SDW_SLAVE_ATTACHED) {
>>>> +		acp_reg_writel(0, amd_manager->mmio + ACP_SW_STATE_CHANGE_STATUS_MASK_0TO7);
>>>> +		acp_reg_writel(0, amd_manager->mmio + ACP_SW_STATE_CHANGE_STATUS_MASK_8TO11);
>>>> +	}
>>>> +
>>>> +update_status:
>>>> +	sdw_handle_slave_status(&amd_manager->bus, amd_manager->status);
>>>> +	/*
>>>> +	 * During the peripheral enumeration sequence, the SoundWire manager interrupts
>>>> +	 * are masked. Once the device number programming is done for all peripherals,
>>>> +	 * interrupts will be unmasked. Read the peripheral device status from ping command
>>>> +	 * and process the response. This sequence will ensure all peripheral devices enumerated
>>>> +	 * and initialized properly.
>>>> +	 */
>>>> +	if (amd_manager->status[0] == SDW_SLAVE_ATTACHED) {
>>>> +		if (retry_count++ < SDW_MAX_DEVICES) {
>>>> +			acp_reg_writel(AMD_SDW_IRQ_MASK_0TO7, amd_manager->mmio +
>>>> +				       ACP_SW_STATE_CHANGE_STATUS_MASK_0TO7);
>>>> +			acp_reg_writel(AMD_SDW_IRQ_MASK_8TO11,
>>>> +				       amd_manager->mmio + ACP_SW_STATE_CHANGE_STATUS_MASK_8TO11);
>>>> +			amd_sdw_read_and_process_ping_status(amd_manager);
>>>> +			goto update_status;
>>> goto are mostly used for error handling, i dont thing case here deserves
>>> a goto, can you please change this...
>> I agree. goto statements will be used mostly for error handling.
>> But this is a different scenario. We have used goto statement to call sdw_handle_slave_status()
>> from if statement to make sure all peripheral devices are enumerated and initialized properly.
>> Please let us know if you are expecting code to be modified as mentioned below.
>>
>> sdw_handle_slave_status(&amd_manager->bus, amd_manager->status);
>>
>> if (amd_manager->status[0] == SDW_SLAVE_ATTACHED) {
>> 	acp_reg_writel(AMD_SDW_IRQ_MASK_0TO7, amd_manager->mmio +
>> 		       ACP_SW_STATE_CHANGE_STATUS_MASK_0TO7);
>> 	acp_reg_writel(AMD_SDW_IRQ_MASK_8TO11,
>> 		       amd_manager->mmio + ACP_SW_STATE_CHANGE_STATUS_MASK_8TO11);
>> 	amd_sdw_read_and_process_ping_status(amd_manager);
>> 	sdw_handle_slave_status(&amd_manager->bus, amd_manager->status);
>> }
>>
>> We have to check any race conditions occurs or not if we implement code as mentioned
>> above.
> what race are you talking about
Our intention is to convey that we need to verify the above logic and check
for faulty case handling where status[0] is keep on updated as "ATTACHED"
when multiple peripheral devices are connected over the link.


>
>> IMHO, it is still good to go with goto statement implementation.
> Since you keep checking, essentially this seems to be a loop?
In normal scenario , if condition gets executed once and exits the function.
In faulty case, if status[0] is still reported as ATTACHED, it runs in loop till retry_count
reaches SDW_MAX_DEVICES.
We want to keep checking the status until ping command reports
correct status on all other devices other than "ATTACHED" on device0 in a loop using goto
statement with max retry count.
>