--- Begin Message ---
- To: Vinod Koul <vkoul@xxxxxxxxxx>
- Subject: Re: [PATCH V6 5/8] soundwire: amd: add SoundWire manager interrupt handling
- From: "Mukunda,Vijendar" <vijendar.mukunda@xxxxxxx>
- Date: Fri, 17 Mar 2023 20:16:36 +0530
- Cc: alsa-devel@xxxxxxxxxxxxxxxx, pierre-louis.bossart@xxxxxxxxxxxxxxx, Basavaraj.Hiregoudar@xxxxxxx, Sunil-kumar.Dommati@xxxxxxx, Mario.Limonciello@xxxxxxx, amadeuszx.slawinski@xxxxxxxxxxxxxxx, Mastan.Katragadda@xxxxxxx, Arungopal.kondaveeti@xxxxxxx, claudiu.beznea@xxxxxxxxxxxxx, Bard Liao <yung-chuan.liao@xxxxxxxxxxxxxxx>, Sanyog Kale <sanyog.r.kale@xxxxxxxxx>, open list <linux-kernel@xxxxxxxxxxxxxxx>
- In-reply-to: <ZBRs5LAXRQ2S1Htc@matsya>
- References: <20230307133135.545952-1-Vijendar.Mukunda@amd.com> <20230307133135.545952-6-Vijendar.Mukunda@amd.com> <ZBGYubOYyu7E8ueo@matsya> <3aa704b0-1142-8afe-b0f1-28bd2a254cc2@amd.com> <ZBRs5LAXRQ2S1Htc@matsya>
- User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101 Thunderbird/102.8.0
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. >
--- End Message ---
